On Tue, Jan 17, 2012 at 11:05 PM, Daniel Shahaf <danielsh_at_elego.de> wrote:
> 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."?
Fixed.
>> + 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/
Fixed.
>> + /* 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'.)
Correct, since we don't have a working copy, it's kinda pointless to
invoke libsvn_wc. There is probably some way we could fix the problem
by introducing a utility function of some kind, but that's Future
Work.
> 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.)
Agreed. I'm looking at reworking this function now.
It should be noted that the current add_file() is essentially a
concatenation of the delta editor add_file(), change_file_prop(),
apply_textdelta() and close_file(), so any issues have been around for
a while in the existing code. The first round of changes was just
getting everything in the same function, now it's on to figuring out
how we can leverage this locality in the ways you mention.
> ...
>> + 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.
I've excised this function, so this is no longer valid.
> (Yes, I'm probably missing something, but I'm not sure what.)
Thanks for the review! I'm glad somebody else is looking at this...
-Hyrum
--
uberSVN: Apache Subversion Made Easy
http://www.uberSVN.com/
Received on 2012-01-18 16:04:16 CET