Login | Register
My pages Projects Community openCollabNet

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

subversion
Discussion topic

Back to topic list

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

Author glasser
Full name David Glasser
Date 2009-05-13 11:47:08 PDT
Message We could also just add a
--dont-munge-mergein​fo-just-pass-the-byt​es-through-kthxbye option to
svnadmin load.

--dave

On Wed, May 13, 2009 at 7:22 AM, Paul Burba <ptburba at gmail dot com> wrote:
> On Wed, May 13, 2009 at 1:37 AM, Daniel Shahaf <d dot s at daniel dot shahaf dot 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 dot s at daniel dot shahaf dot 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_merg​einfo_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_merg​einfo_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_merg​einfo_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 dot net | langtonlabs.org | flickr.com/photos/glasser/

« Previous message in topic | 7 of 11 | Next message in topic »

Messages

Show all messages in topic

Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo markphip Mark Phippard 2009-05-12 09:35:53 PDT
     Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-12 10:49:42 PDT
         Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-12 11:33:34 PDT
             Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-12 12:58:00 PDT
                 Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-12 22:37:35 PDT
                     Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-13 07:29:10 PDT
                         Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo glasser David Glasser 2009-05-13 11:47:08 PDT
                             Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-13 12:34:17 PDT
                                 Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo glasser David Glasser 2009-05-14 17:33:50 PDT
                         Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-13 16:40:25 PDT
                             Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-18 08:02:15 PDT
Messages per page: