Re: svnsync 1.6.0 fails to sync repositories with ^M in an svn:ignore

[Daniel Shahaf <d....@daniel.shahaf.name>]

[ CCing dev@.  Background: svnsync blows up on svn:ignore with non-LF
line endings when syncing 1.5 src to 1.6 dest.  Issues #1796 and #3313. ]

B Smith-Mannschott wrote on Sun, 29 Mar 2009 at 15:38 +0200:
> On Sat, Mar 28, 2009 at 14:58, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > B Smith-Mannschott wrote on Sat, 28 Mar 2009 at 12:43 +0100:
> 
> >> I've made a hacked-up variant of svnsync which will correct impure
> >> eol-style in property values where the real 1.6.0 complains and dies.
> >> (see patch at end of mail).
> >>
...
> Thanks for the tip. This is what I came up with:
> 
> ----------------------------------
> 

Bottom line, the patch looks good.  However, assuming you want it
applied, I have a few changes to suggest:

> --- a/subversion/svnsync/main.c
> +++ b/subversion/svnsync/main.c
> @@ -754,6 +754,7 @@ typedef struct {
>    svn_boolean_t mergeinfo_stripped; /* Did we strip svn:mergeinfo? */
>    svn_boolean_t svnmerge_migrated;  /* Did we convert svnmerge.py data? */
>    svn_boolean_t svnmerge_blocked;   /* Was there any blocked svnmerge data? */
> +  svn_boolean_t fix_prop_val_eol_style; /*  in svn:* props */
>  } edit_baton_t;
> 
> 
> @@ -1084,6 +1085,22 @@ change_dir_prop(void *dir_baton,
>        name = SVN_PROP_MERGEINFO;
>        eb->svnmerge_migrated = TRUE;

It would be good to have the same logic applied in change_file_prop()
too (although currently it should be a no-op 99% of the time).

I'm not sure whether to apply the logic to revprops?
(Those can be fixed *without* svnsync... but, OTOH, it's probably more
convenient/ consistent to have svnsync fix both kinds of properties.
And so far, the issue was reported more often with revprops than with
versioned props.)

>      }
> +  /* Here we force consistent eol-style for svn:* properties that
> +     don't have it.  not, that by being on the else branch of the
> +     previous if, this only kicks in if we haven't already

*nod*
(and typo: s/not/note/)

> +     rewritten the property as part of a merge-info conversion. */
> +  else if (eb->fix_prop_val_eol_style && svn_prop_is_svn_prop(name) && value)
> +    {

s/svn_prop_is_svn_prop/svn_prop_needs_translation/

> +      /* Only actually do the conversion if there's actually at
> +         least one \r present. Is this premature  optimzation? */
> +      if (strchr(value->data, '\r'))
> +        {
> +          apr_array_header_t *lines =
> +            svn_cstring_split(value->data, "\r\n", FALSE, pool);
> +          const char *joined_lines = svn_cstring_join(lines, "\n", pool);
> +          real_value = svn_string_create(joined_lines, pool);
> +        }

Okay, this is *much* clearer than in your original patch :-)

(How would it handle "foo\n\nbar", though?  For svn:ignore it doesn't 
matter, but if we want to apply this logic to svn:log too, we should try 
to avoid erasing blank lines, if possible.)

> +    }
> 
>    /* Remember if we see any svnmerge-blocked properties. */
>    if (eb->migrate_svnmerge && (strcmp(name, "svnmerge-blocked") == 0))
> @@ -1198,6 +1215,12 @@ get_sync_editor(const svn_delta_editor_t *wrapped_editor,
>        eb->migrate_svnmerge = TRUE;
>        eb->strip_mergeinfo = TRUE;
>      }
> +  if (getenv("SVNSYNC_FIX_PROP_VAL_EOL_STYLE"))
> +    {
> +      /* force consistent eol-style on svn:* properties by stripping
> +         \r, leaving only \n as line terminator. */
> +      eb->fix_prop_val_eol_style = TRUE;
> +    }
> 
>    *editor = tree_editor;
>    *edit_baton = eb;
> 
> -----------------------
> 


> It works for me, and I think I'll patch my local svnsync this way
> until we've migrated the central servers to 1.6 and cleaned up our
> repositories accordingly.
> 
> The svn internals are still pretty opaque to me, so this is monkey
> (see, monkey do) code. Am I doing anything here that seems obviously
> wrong?
> 

I think the comments I didn't make above should answer this question.  :-)

> The bash script below demonstrates the behavior in combination with
> the repo.dump file attached to the first message in this thread.
> 
> --------------------------
> 


> #!/bin/bash
> 
> # set as appropriate
> SVNSYNC=../installed/bin/svnsync
> SVNADMIN=../installed/bin/svnadmin
> DUMPFILE=repo.dump
> 
> test ! -d repo || rm -rf repo
> $SVNADMIN create repo
> $SVNADMIN load repo < $DUMPFILE
> 
> test ! -d clone || rm -rf clone
> $SVNADMIN create clone
> ln -s /bin/true clone/hooks/pre-revprop-change
> $SVNSYNC init file://$PWD/clone file://$PWD/repo
> 
> unset SVNSYNC_FIX_PROP_VAL_EOL_STYLE
> echo "* svnsync without SVNSYNC_FIX_PROP_VAL_EOL_STYLE"
> if $SVNSYNC sync file://$PWD/clone
> then echo "** svnsync fix should have failed here."
> else echo "** svnsync failed as expected"
> fi
> 
> export SVNSYNC_FIX_PROP_VAL_EOL_STYLE=1
> echo "* svnsync with SVNSYNC_FIX_PROP_VAL_EOL_STYLE"
> if $SVNSYNC sync file://$PWD/clone
> then echo "** svnsync succeeded as expected"
> else echo "** svnsync should have succeeded, but failed instead"
> fi
> 
> --------------------------
> 
> // Ben
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1484531