On Wed, May 13, 2009 at 7:40 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> Paul Burba wrote on Wed, 13 May 2009 at 10:22 -0400:
>> 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 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!
> ....
>> >
>> > 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?
>
> Yes, I suppose we could rewrite the logic to detect the "no renumbering"
> case more explicitly if we wanted to. Yes, I don't think there is
> a reason to do that right now (regardless of any CRLF fixes we have in
> mind).
>
>> 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.
>>
>> > 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.
>>
>
> So, the terminating NUL will be there, and embedded NULs cannot happen
> for svn:mergeinfo. Thanks for clarifying.
>
>> Paul
>>
>
> Daniel
Committed this r37768 and nominated for backport to 1.5.x and
1.6.x...though immediately after I did that it hit me that r37768 can
change the output of svnadmin load, so maybe it is not a valid
backport(?). Regardless, it is fixed for 1.7.
Leaving the question of a no-munge option an open one.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2301198
Received on 2009-05-18 18:09:21 CEST