Ben, I know that I'm CCed, but I won't have time for more than a quick
glance at the patch in the near future. (University comes first.) I'll
look at it as soon as I'm working on svn again.
i.e., the patch appears good; I just don't have the time to review, test,
etc., it.
Daniel
B Smith-Mannschott wrote on Wed, 29 Apr 2009 at 22:30 +0200:
> http://subversion.tigris.org/issues/show_bug.cgi?id=3404
>
> [[[
> Fix issue 3404: svnsync fails when source has ^M in svn:* properties (eol style)
>
> svnsync subcommands sync and copy-revprops have been taught how to do
> eol-style normalization of revision and node properties which
> svn_prop_needs_translation.
>
> Normalization will always be performed for a target repository
> initialized with --fix-svn-prop-eol-style. Alternatively this option
> is also understood by the sync and copy-revprops subcommands to mean
> perform eol-normalization just for the current execution.
>
> * subversion/include/svn_props.h
> (SVNSYNC_PROP_FIX_SVN_PROP_EOL_STYLE):
>
> * subversion/libsvn_subr/properties.c
> (svn_prop_is_boolean):
>
> * subversion/svnsync/main.c
> (svn_subst.h, svn_string.h): #include
> (svnsync_opt_fix_svn_prop_eol_style): new enum value. Represents the option
> --fix-svn-prop-eol-style on subcommands init, sync and copy-revprops.
> (svnsync_cmd_table): svnsync_opt_fix_svn_prop_eol_style added to entries for
> init, sync and copy-revprops.
> (svnsync_options): added svnsync_opt_fix_svn_prop_eol_style.
> (opt_baton_t): new fix_svn_prop_eol_style, records presence of option.
> (subcommand_baton_t): new fix_svn_prop_eol_style, meaning subcommand should
> perform eol-style normalization on svn:* properties as appropriate.
> (normalize_svn_string_eol_style): new, eol normalization for 1 svn_string_t.
> (normalize_revprop_eol_style): new, eol normalization for a revprop hash.
> (copy_revprops): new argument fix_svn_prop_eol_style which causes eol-style
> normalization to be done during revprop copying.
> (make_subcommand_baton): copy fix_svn_prop_eol_style form opt_baton.
> (do_initialize): record presence of --fix-svn-prop-eol-style as property
> svn:sync-fix-svn-prop-eol-style of revision zero of target. Also
> optionally perform eol-style normalization on revprops copied from source
> repository.
> (edit_baton_t): new fix_svn_prop_eol_style, which determines if the edit
> drive should do eol-style normalization on svn:* properties.
> (change_file_prop, change_dir_prop): may perform eol-style normalization.
> (get_sync_editor): new argument fix_svn_prop_eol_style, which serves to
> get this flag from the subcommand_baton into the edit_baton with some help
> form replay_rev_started.
> (replay_rev_started): may perform normalization of copied revprops.
> Provides fix_svn_prop_eol_style for sync_baton (an edit_baton_t).
> (replay_rev_finished): may perform normalization of copied revprops.
> (do_synchronize): may perform normalization of copied revrops while
> cleaning up form a previous call that didn't run to completion.
> (synchronize_cmd, copy_revprops_cmd): sets fix_svn_prop_eol_style on the
> subcommand_baton when init has left the corresponding property behind in
> revision zero.
> (do_copy_revprops): may perform normalization of copied revprops.
> (info_cmd): one line of ourput generated iff svn:sync-fix-svn-prop-eol-style
> is present in revision zero.
> (main): handle svnsync_opt_fix_svn_prop_eol_style, set flag in opt_baton.
> ]]]
>
> * Naming
>
> I've deliberately kept "fix svn prop eol style" consistent everywhere
> it appears (option name, #defines, struct fields, function arguments).
>
> In an earlier version of this patch I had been using the name
> normalize-subversion-property-eol-style, but found it was too long
> both as an option name and in code.
>
> After some deliberation, I settled on fix_svn_prop_eol_style.
> "Normalize" tells us a little more than "fix", but sufficiently so to
> justify its length. The remaining parts of the name are all
> abbreviations that are already established in Subversion, so I don't
> feel using them impedes readablility.
>
> * apr_hash_set usage
>
> Please have a look at what I'm doing in normalize_revprop_eol_style.
> Is apr_hash_set gonig to be cool with that?
>
> * Code readability
>
> I was pleasantly surprised by the readability of the parts of
> Subversion's code I read while developing this patch. The comments
> are plentiful and generally useful.
>
> Before embarking on this, I'd never used GNU coding style in anger and
> have considered endline layout quirky (at best) since reading the
> first edition of Code Complete. Luckily though Emacs makes endline
> layout easy to maintain and I found it aided readability particularly
> in light of Subversions penchant for long argument lists.
>
> I've tried to code in a way consistent with what I found.
>
> * Testing
>
> I have not yet had the time to grok Subversion's test, but I'm
> attaching a dump file and a small bash script which at least
> demonstrate (think "smoke test") that the code works (for me!).
>
> // Ben
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2001936
Received on 2009-04-30 23:56:03 CEST