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
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2238825
Received on 2009-05-13 16:30:43 CEST