hwright_at_apache.org wrote on Tue, Jan 17, 2012 at 22:32:54 -0000:
> Author: hwright
> Date: Tue Jan 17 22:32:53 2012
> New Revision: 1232614
>
> URL: http://svn.apache.org/viewvc?rev=1232614&view=rev
> Log:
> On the ev2-export branch:
> Implement export with an Ev2 editor, and use that inplace of the delta editor.
> Most of the content was just copied over, and I'm opptomistic the memory usage
> will be better (no more long-lived file batons).
>
> I'll remove the excess dead code in a subsequent commit.
>
> * subversion/libsvn_client/export.c
> (add_file_ev2, set_props_ev2, complete_ev2, add_directory_ev2): New.
> (get_editor): Instantiate an Ev2 editor, and set up the proper callbacks.
>
> Modified:
> subversion/branches/ev2-export/subversion/libsvn_client/export.c
>
> Modified: subversion/branches/ev2-export/subversion/libsvn_client/export.c
> URL: http://svn.apache.org/viewvc/subversion/branches/ev2-export/subversion/libsvn_client/export.c?rev=1232614&r1=1232613&r2=1232614&view=diff
> ==============================================================================
> --- subversion/branches/ev2-export/subversion/libsvn_client/export.c (original)
> +++ subversion/branches/ev2-export/subversion/libsvn_client/export.c Tue Jan 17 22:32:53 2012
> @@ -1070,6 +1070,185 @@ fetch_base_func(const char **filename,
> }
>
> static svn_error_t *
> +add_file_ev2(void *baton,
> + const char *relpath,
> + const svn_checksum_t *checksum,
> + svn_stream_t *contents,
> + apr_hash_t *props,
> + svn_revnum_t replaces_rev,
> + apr_pool_t *scratch_pool)
> +{
> + struct edit_baton *eb = baton;
> + const char *full_path = svn_dirent_join(eb->root_path, relpath,
> + scratch_pool);
> + /* PATH is not canonicalized, i.e. it may still contain spaces etc.
> + * but EB->root_url is. */
> + const char *full_url = svn_path_url_add_component2(eb->root_url,
> + relpath,
> + scratch_pool);
There is no variable named PATH. (And, besides, I'm pretty sure that
_in relpaths_ spaces are valid --- though API docs and unit tests don't
seem to have any examples of space-ful relpaths, encoded or otherwise.)
Should the comment say: "RELPATH may contain spaces; FULL_URL is canonical."?
> + svn_stream_t *tmp_stream;
> + const char *tmppath;
> + const svn_string_t *val;
> + /* The three svn: properties we might actually care about. */
> + const svn_string_t *eol_style_val = NULL;
> + const svn_string_t *keywords_val = NULL;
> + const svn_string_t *executable_val = NULL;
> + svn_boolean_t special = FALSE;
(warning: nitpick) s/three/four/
> + /* Any keyword vals to be substituted */
> + const char *revision = NULL;
> + const char *author = NULL;
> + apr_time_t date = 0;
> +
> + /* Create a temporary file in the same directory as the file. We're going
> + to rename the thing into place when we're done. */
> + SVN_ERR(svn_stream_open_unique(&tmp_stream, &tmppath,
> + svn_dirent_dirname(full_path, scratch_pool),
> + svn_io_file_del_none,
> + scratch_pool, scratch_pool));
> +
> + SVN_ERR(svn_stream_copy3(contents, tmp_stream,
> + eb->cancel_func, eb->cancel_baton, scratch_pool));
> +
> + /* Look at any properties for additional information. */
> + if ( (val = apr_hash_get(props, SVN_PROP_EOL_STYLE, APR_HASH_KEY_STRING)) )
> + eol_style_val = val;
> +
> + if ( !eb->ignore_keywords && (val = apr_hash_get(props, SVN_PROP_KEYWORDS,
> + APR_HASH_KEY_STRING)) )
> + keywords_val = val;
> +
> + if ( (val = apr_hash_get(props, SVN_PROP_EXECUTABLE, APR_HASH_KEY_STRING)) )
> + executable_val = val;
> +
Two things.
One, you're in libsvn_client. Shouldn't it be libsvn_wc which worries
about whether or not to call svn_io_set_file_executable() based on the
nodeprops? (Yes, I know that there is no wc in 'export'.)
Two, why are you first creating the tempfile and only then checking the
svn:keywords property? The natural thing to do would be to do the
keyword expansion as you're streaming data from the wire into a tempfile
in the target dir, rather than afterwards. (That saves O(filesize) I/O
--- pretty significant.)
...
> + SVN_ERR(svn_subst_copy_and_translate4(tmppath, full_path,
> + eol, repair, final_kw,
> + TRUE, /* expand */
> + special,
> + eb->cancel_func, eb->cancel_baton,
> + scratch_pool));
> +
> + SVN_ERR(svn_io_remove_file2(tmppath, FALSE, scratch_pool));
> + }
> +
> + if (executable_val)
> + SVN_ERR(svn_io_set_file_executable(full_path, TRUE, FALSE, scratch_pool));
> +
> + if (date && (! special))
> + SVN_ERR(svn_io_set_file_affected_time(date, full_path, scratch_pool));
> +
...
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +set_props_ev2(void *baton,
> + const char *relpath,
> + svn_revnum_t revision,
> + apr_hash_t *props,
> + svn_boolean_t complete,
> + apr_pool_t *scratch_pool)
> +{
> + /* This is an export, we don't care about properties. */
> + return SVN_NO_ERROR;
Shouldn't the comment say "about property changes"? add_file_ev2()
does care about properties, but this is the callback for propchanges to
existing nodes, so it shouldn't be called or needed.
(Yes, I'm probably missing something, but I'm not sure what.)
> +}
Received on 2012-01-18 06:06:58 CET