[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo

From: David Glasser <glasser_at_davidglasser.net>
Date: Wed, 13 May 2009 11:47:06 -0700

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

This is an archived mail posted to the Subversion Dev mailing list.