We could also just add a
--dont-munge-mergeinfo-just-pass-the-bytes-through-kthxbye option to
svnadmin load.
--dave
On Wed, May 13, 2009 at 7:22 AM, Paul Burba <ptburba_at_gmail.com> wrote:
> On Wed, May 13, 2009 at 1:37 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
>> 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.
>
> Daniel,
>
> We could tweak load.c:renumber_mergeinfo_revs() to simply set
> *FINAL_VAL to INITIAL_VAL if no renumbering is done, but that is a
> separate issue yes? Regardless of whether or not that change is made,
> load.c:renumber_mergeinfo_revs() needs to call svn_mergeinfo_parse()
> *before* it can determine if renumbering is necessary. So if we don't
> clean up the windows EOLs before then the load will fail when
> svn_mergeinfo_parse() encounters those.
>
>>> > 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).
>
> That makes two of us!
>
>> 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.
>
> read_key_or_val(), which provides both keybuf and propstring (the
> arguments to strcmp and strstr respectively), always returns at least
> an empty string, so this is safe. The svn_repos_parse_fns2_t
> set_node_property() immediately following makes the same assumption.
>
> Paul
>
--
glasser_at_davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2241115
Received on 2009-05-13 20:48:07 CEST