Paul Burba wrote on Tue, 12 May 2009 at 15:57 -0400:
> On Tue, May 12, 2009 at 2:32 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > Paul Burba wrote on Tue, 12 May 2009 at 13:49 -0400:
> >> * Unlike mergeinfo with '\r', other versioned and unversioned props
> >> are loaded intact, that is to say the '\r\n' are still present in the
> >> loaded repos. My patch "fixes" the loaded mergeinfo props so there
> >> are no '\r'. I could make loaded mergeinfo be treated the same way,
> >> but I see no point in doing this!
> >
> > Consistency? Less special-cases logic?
>
> Hi Daniel,
>
> It would be consistent yes, but consistently wrong no? We don't want
> '\r\n' in our svn:* properties right?
>
> And AFAICT it would take *more* special case logic to preserve the
> '\r\n' in loaded mergeinfo than simply fixing it.
> Sure, we can tweak svn_parse_mergeinfo() as John suggested at the
> start of this thread, but svn_parse_mergeinfo() is used elsewhere to
> *prevent* '\r\n' in mergeinfo via propedit and propset, so we'd have
> to rev svn_parse_mergeinfo() to add a "fix_eol" argument or something
> similar. Not only that, but we'd have to keep track of where in the
> loaded mergeinfo the offending '\r\n' where so we can put them back in
> the name of "consistency" (keep in mind that we have no guarantee that
> the prop is all '\n' or all '\r\n' it might be mixed). This would
> also mean we couldn't simply use svn_mergeinfo_to_string() in
> load.c:renumber_mergeinfo_revs() but would have to create
> a specialized function to put the "\r\n"'s back in the right spots.
> This seems a bit crazy to me :-)
>
You've convinced me. :-)
For the record, I thought we could just pass through the value
unmodified (at least when we aren't doing any renumbering at all);
however, it appears the logic doesn't take that shortcut explicitly,
so that's not possible.
> > Though perhaps you should notify (on stderr) that you modify the data
> > being loaded.
>
> How's this look (see r5)?
>
> [[[
> <<< Started new transaction, based on original revision 5
> * editing path : A_COPY ... removing '\r' from svn:mergeinfo ... done.
> * editing path : A_COPY/D/H/psi ... done.
>
> ------- Committed revision 5 >>>
> ]]]
>
+1
> [[[
> Make svnadmin load tolerate mergeinfo with "\r\n".
>
> * subversion/libsvn_repos/load.c
> (svn_subst.h): New include.
> (parse_property_block): Add parse_baton argument. If necessary normalize
> mergeinfo line endings to '\n' and print notification that this has
> been done.
> (svn_repos_parse_dumpstream2): Update call to parse_property_block().
> ]]]
>
Looks good (though I haven't dived too deeply into the load logic).
I see an strstr(svn_string_t.data) --- which normally makes me ask what
happens when the svn_string_t's value contains NULs --- but I suppose
the answer (in this case) is that svn:mergeinfo cannot legitimately
contain NULs.
> Paul
>
Thanks,
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2229583
Received on 2009-05-13 07:37:52 CEST