You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by John Skopis <js...@backstopsolutions.com> on 2009/05/08 20:34:43 UTC
[PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Hello,
svnadmin load fails while importing a dump that contains a windows newline in svn:mergeinfo prop. I have not done extensive testing on this patch, but it should work (in that it doesn't segfault when I attempt to import a revision with \r\n in mergeinfo). Be advised I am not actually a developer.
Thanks,
John Skopis
Systems Administration
[[[
* subversion/libsvn_subr/mergeinfo.c
(parse_revlist): Ignore windows newlines in svn:mergeinfo
]]]
Index: subversion/libsvn_subr/mergeinfo.c
===================================================================
--- subversion/libsvn_subr/mergeinfo.c (revision 37647)
+++ subversion/libsvn_subr/mergeinfo.c (working copy)
@@ -402,7 +403,7 @@
svn_revnum_t firstrev;
SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
- if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*'
+ if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*' && *curr != '\r'
&& curr != end)
return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
_("Invalid character '%c' found in revision "
@@ -430,8 +431,10 @@
mrange->end = secondrev;
}
- if (*curr == '\n' || curr == end)
+ if (*curr == '\r' || *curr == '\n' || curr == end)
{
+ if ( *curr == '\r' )
+ curr++;
APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
*input = curr;
return SVN_NO_ERROR;
@@ -445,10 +448,10 @@
{
mrange->inheritable = FALSE;
curr++;
- if (*curr == ',' || *curr == '\n' || curr == end)
+ if (*curr == ',' || *curr == '\n' || *curr == '\r' || curr == end )
{
APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
- if (*curr == ',')
+ if (*curr == ',' || *curr == '\r' )
{
curr++;
}
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2121222
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by John Skopis <js...@backstopsolutions.com>.
Paul, please excuse the top post.
More than likely they used the official 1.5.4 client as that was the latest and greatest available at the time (rev 40998 was the day after we upgraded the server to 1.5.4). I am leaning towards a propedit.
Hth,
John
----- Original Message -----
From: Paul Burba <pt...@gmail.com>
To: dev@subversion.tigris.org <de...@subversion.tigris.org>
Cc: Daniel Shahaf <d....@daniel.shahaf.name>; John Skopis; B. Smith-Mannschott <bs...@gmail.com>; David Glasser <gl...@davidglasser.net>; senthil@collab.net <se...@collab.net>
Sent: Mon May 11 16:01:14 2009
Subject: Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo
On Mon, May 11, 2009 at 1:36 PM, David Glasser <gl...@davidglasser.net> wrote:
> On Mon, May 11, 2009 at 9:49 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> John Skopis wrote on Mon, 11 May 2009 at 10:52 -0500:
>>>
>>>
>>> >-----Original Message-----
>>> >From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>> >Sent: Monday, May 11, 2009 9:52 AM
>>> >To: B. Smith-Mannschott
>>> >Cc: David Glasser; John Skopis; dev@subversion.tigris.org
>>> >Subject: Re: [PATCH] svnadmin load will not import dump with windows
>>> >newline character in svn:mergeinfo
>>> >
>>> >What Ben said.
>>> >
>>> >Also, I think the patch here is orthogonal to issue #3404 et al, since
>>> >John said something about a segfault. John, could you say where the
>>> >segfault is? Can you reproduce it without using svnadmin (by direct API
>>> >usage)?
>>>
>>> I just said that the patch below *does not* trigger segfault, implying
>>> that it "works", however correctly it works is more appropriate for
>>> this list to decide.
>>>
>>
>> You said that it didn't segfault with the patch, I inferred that it did
>> segfault without. I can call it a misunderstanding.
>>
>>> It seems to me that no one is too happy about silently filtering out
>>> '\r' from svn:mergeinfo.
>>>
>>> I am not too happy that my svndump won't load.
>>>
>>
>> 'svnadmin load' does allow loading properties with CRLFs, even though
>> you can't commit/svnsync them. Per my other mail --- it seems that
>> svn:mergeinfo is special-cased somewhere (svn_mergeinfo_parse is called
>> and does its own validation).
>
> Haven't looked at this in a while, but I seem to recall that svnadmin
> load will renumber revs in mergeinfo if it's renumbering revs at all.
> I guess that's where this comes into play.
>
> --dave
Dave's recollection is correct. The fix for issue #3020
(http://subversion.tigris.org/issues/show_bug.cgi?id=3020) added
libsvn_repos/load.c:renumber_mergeinfo_revs() which is called from the
svn_repos_parse_fns2_t set_node_property function (Senthil - I'm ccing
you because you made this change).
Maybe the solution is to basically do what John's patch suggests, but
instead of simply tolerating \r\n in mergeinfo properties during
dump/load, actually canonicalize the line endings to \n. Is there any
reason not to do this?
>>> I was able to dump revs 1-40997, 40998, and 40998-head, hand edit
>>> 40998, import all the revs and dump the repo and reimport it (my issue
>>> has been resolved). I then created a patch, should some unfortunate
>>> user experience the same issue as me, and sent it upstream. The rest
>>> is up to you.
Hi John - Do you know how the mergeinfo in r40998 was created? Was it
the result of a merge or a propset/propedit or something else
entirely? Also, can you hazard a guess as to what version of
Subversion (or some other client tool) was used to make it?
If possible I'd like to figure out how the <CR> got in your repository
in the first place before making any fixes to dump/load.
Thanks,
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2198371
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by "B. Smith-Mannschott" <bs...@gmail.com>.
On Mon, May 11, 2009 at 07:31, B Smith-Mannschott <bs...@gmail.com> wrote:
> This new-found strictness lead to issue 3404 (svnload fails on ^M),
should say: "( *svnsync* fails on ^M)", my bad.
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2186218
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by David Glasser <gl...@davidglasser.net>.
On Wed, May 13, 2009 at 12:04 PM, Paul Burba <pt...@gmail.com> wrote:
> On Wed, May 13, 2009 at 2:47 PM, David Glasser <gl...@davidglasser.net> wrote:
>> We could also just add a
>> --dont-munge-mergeinfo-just-pass-the-bytes-through-kthxbye option to
>> svnadmin load.
>>
>> --dave
>
> Dave,
>
> What exactly do you think is the best course of action here then? I
> respect your opinion, but often have a tough time extracting it from
> your humor.
Eh, I dunno. I think some sort of --no-munge option is reasonable in
general, but it might also be specifically useful to even without the
option recover gracefully from a flawed svn:mergeinfo during the load.
--dave
--
glasser@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2264372
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Wed, May 13, 2009 at 2:47 PM, David Glasser <gl...@davidglasser.net> wrote:
> We could also just add a
> --dont-munge-mergeinfo-just-pass-the-bytes-through-kthxbye option to
> svnadmin load.
>
> --dave
Dave,
What exactly do you think is the best course of action here then? I
respect your opinion, but often have a tough time extracting it from
your humor.
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2241957
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by David Glasser <gl...@davidglasser.net>.
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 <pt...@gmail.com> wrote:
> On Wed, May 13, 2009 at 1:37 AM, Daniel Shahaf <d....@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....@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@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2241115
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Wed, May 13, 2009 at 7:40 PM, Daniel Shahaf <d....@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....@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
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Paul Burba wrote on Wed, 13 May 2009 at 10:22 -0400:
> On Wed, May 13, 2009 at 1:37 AM, Daniel Shahaf <d....@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
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2246035
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Wed, May 13, 2009 at 1:37 AM, Daniel Shahaf <d....@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....@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
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Paul Burba wrote on Tue, 12 May 2009 at 15:57 -0400:
> On Tue, May 12, 2009 at 2:32 PM, Daniel Shahaf <d....@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.
> > 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).
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.
> Paul
>
Thanks,
Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2229583
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Tue, May 12, 2009 at 2:32 PM, Daniel Shahaf <d....@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 :-)
> (The "remove CRs from versioned props" work is already done centrally for
> all props as part of #3404.)
You mean this patch by B. Smith-Mannschott?
http://svn.haxx.se/dev/archive-2009-04/0991.shtml
That work doesn't directly help here, it is for svnsync only. We
could make use of normalize_svn_string_eol_style if it was public, but
that's about it, unless I am missing something.
> Agreed, I don't see a reason against this fix.
> 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 1
* adding path : A ... done.
* adding path : A/B ... done.
* adding path : A/B/E ... done.
* adding path : A/B/E/alpha ... done.
* adding path : A/B/E/beta ... done.
* adding path : A/B/F ... done.
* adding path : A/B/lambda ... done.
* adding path : A/C ... done.
* adding path : A/D ... done.
* adding path : A/D/G ... done.
* adding path : A/D/G/pi ... done.
* adding path : A/D/G/rho ... done.
* adding path : A/D/G/tau ... done.
* adding path : A/D/H ... done.
* adding path : A/D/H/chi ... done.
* adding path : A/D/H/omega ... done.
* adding path : A/D/H/psi ... done.
* adding path : A/D/gamma ... done.
* adding path : A/mu ... done.
* adding path : iota ... done.
------- Committed revision 1 >>>
<<< Started new transaction, based on original revision 2
* adding path : A_COPY ...COPIED... done.
------- Committed revision 2 >>>
<<< Started new transaction, based on original revision 3
* adding path : A_COPY_2 ...COPIED... done.
------- Committed revision 3 >>>
<<< Started new transaction, based on original revision 4
* editing path : A/D/H/psi ... done.
------- Committed revision 4 >>>
<<< 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 >>>
]]]
[[[
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().
]]]
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2220062
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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?
(The "remove CRs from versioned props" work is already done centrally for
all props as part of #3404.)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2218768
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Tue, May 12, 2009 at 12:35 PM, Mark Phippard <ma...@gmail.com> wrote:
> On Tue, May 12, 2009 at 12:31 PM, Paul Burba <pt...@gmail.com> wrote:
>> On Mon, May 11, 2009 at 6:20 PM, Mark Phippard <ma...@gmail.com> wrote:
>>> On Mon, May 11, 2009 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
>>>
>>>> Hi John - Do you know how the mergeinfo in r40998 was created? Was it
>>>> the result of a merge or a propset/propedit or something else
>>>> entirely? Also, can you hazard a guess as to what version of
>>>> Subversion (or some other client tool) was used to make it?
>>>>
>>>> If possible I'd like to figure out how the <CR> got in your repository
>>>> in the first place before making any fixes to dump/load.
>>>
>>> I am not sure why we are thinking this is anything special (related to
>>> mergeinfo). We have had many reports of people running into this
>>> problem for all sorts of properties.
>>
>> Mark,
>>
>> I hadn't realized that, I see the various threads and issue #3404
>> related to svnsync. But dump/load with 1.6.2 works fine if there are
>> "\r\n" in, say, an svn:log property...
>
> Did you try it with svn:externals or any of the versioned properties?
Just did now, with svn:externals and the load works fine* with trunk
and 1.6.2. The reason svn:mergeinfo is a problem during a load is
that it is subject to special handling in the svn_repos_parse_fns2_t
set_node_property function (Senthil's change in r28978).
Paul
* 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!
> I might have been combining svnsync problems with dump/load but I
> thought we have seen it with both.
>
> --
> Thanks
>
> Mark Phippard
> http://markphip.blogspot.com/
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2218191
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Mark Phippard <ma...@gmail.com>.
On Tue, May 12, 2009 at 12:31 PM, Paul Burba <pt...@gmail.com> wrote:
> On Mon, May 11, 2009 at 6:20 PM, Mark Phippard <ma...@gmail.com> wrote:
>> On Mon, May 11, 2009 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
>>
>>> Hi John - Do you know how the mergeinfo in r40998 was created? Was it
>>> the result of a merge or a propset/propedit or something else
>>> entirely? Also, can you hazard a guess as to what version of
>>> Subversion (or some other client tool) was used to make it?
>>>
>>> If possible I'd like to figure out how the <CR> got in your repository
>>> in the first place before making any fixes to dump/load.
>>
>> I am not sure why we are thinking this is anything special (related to
>> mergeinfo). We have had many reports of people running into this
>> problem for all sorts of properties.
>
> Mark,
>
> I hadn't realized that, I see the various threads and issue #3404
> related to svnsync. But dump/load with 1.6.2 works fine if there are
> "\r\n" in, say, an svn:log property...
Did you try it with svn:externals or any of the versioned properties?
I might have been combining svnsync problems with dump/load but I
thought we have seen it with both.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2217060
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Paul Burba wrote on Tue, 12 May 2009 at 12:31 -0400:
> On Mon, May 11, 2009 at 6:20 PM, Mark Phippard <ma...@gmail.com> wrote:
> > On Mon, May 11, 2009 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
> >
> >> Hi John - Do you know how the mergeinfo in r40998 was created? Was it
> >> the result of a merge or a propset/propedit or something else
> >> entirely? Also, can you hazard a guess as to what version of
> >> Subversion (or some other client tool) was used to make it?
> >>
> >> If possible I'd like to figure out how the <CR> got in your repository
> >> in the first place before making any fixes to dump/load.
> >
> > I am not sure why we are thinking this is anything special (related to
> > mergeinfo). We have had many reports of people running into this
> > problem for all sorts of properties.
>
> Mark,
>
> I hadn't realized that, I see the various threads and issue #3404
> related to svnsync. But dump/load with 1.6.2 works fine if there are
> "\r\n" in, say, an svn:log property...
>
Indeed.
> > SVN 1.6 requires that all
> > properties only have LF line endings. If you ever edited the
> > properties on Windows and did not normalize the line endings yourself,
> > it is just a given you are going to have this problem.
> >
> > We know that
> > users are regularly editing svn:mergeinfo so why should we be any more
> > surprised about this than we are with all of the reports for
> > svn:ignore, svn:externals and/or log messages?
> >
> > Personally, I am not sure there was much value in suddenly making the
> > server enforce this restriction when our client libraries have never
> > done anything to help with this normalization. If anything, it seems
> > like we should have put this in the client and let the server still
> > accept the old property values.
>
> ...so if what you are saying here is to tolerate "" in a loaded dump
> file, then the very simple attached patch does just that. Actually it
> does more than tolerate it, it normalizes the line endings to '\n' in
> the loaded repos.
>
> Any reason not to do this? I hesitate to commit only because I've not
> hacked on the svnadmin code in general, this seems like a no-brainer.
>
Agreed, I don't see a reason against this fix.
Though perhaps you should notify (on stderr) that you modify the data
being loaded.
> Paul
>
> [[[
> Make svnadmin load tolerate mergeinfo with "\r\n".
>
> * subversion/libsvn_repos/load.c
> (svn_subst.h): New include.
> (renumber_mergeinfo_revs): Normalize mergeinfo line endings to '\n'.
> ]]]
>
s/intitial/initial/ (in the name of the new variable)
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2217346
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Mon, May 11, 2009 at 6:20 PM, Mark Phippard <ma...@gmail.com> wrote:
> On Mon, May 11, 2009 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
>
>> Hi John - Do you know how the mergeinfo in r40998 was created? Was it
>> the result of a merge or a propset/propedit or something else
>> entirely? Also, can you hazard a guess as to what version of
>> Subversion (or some other client tool) was used to make it?
>>
>> If possible I'd like to figure out how the <CR> got in your repository
>> in the first place before making any fixes to dump/load.
>
> I am not sure why we are thinking this is anything special (related to
> mergeinfo). We have had many reports of people running into this
> problem for all sorts of properties.
Mark,
I hadn't realized that, I see the various threads and issue #3404
related to svnsync. But dump/load with 1.6.2 works fine if there are
"\r\n" in, say, an svn:log property...
> SVN 1.6 requires that all
> properties only have LF line endings. If you ever edited the
> properties on Windows and did not normalize the line endings yourself,
> it is just a given you are going to have this problem.
>
> We know that
> users are regularly editing svn:mergeinfo so why should we be any more
> surprised about this than we are with all of the reports for
> svn:ignore, svn:externals and/or log messages?
>
> Personally, I am not sure there was much value in suddenly making the
> server enforce this restriction when our client libraries have never
> done anything to help with this normalization. If anything, it seems
> like we should have put this in the client and let the server still
> accept the old property values.
...so if what you are saying here is to tolerate "" in a loaded dump
file, then the very simple attached patch does just that. Actually it
does more than tolerate it, it normalizes the line endings to '\n' in
the loaded repos.
Any reason not to do this? I hesitate to commit only because I've not
hacked on the svnadmin code in general, this seems like a no-brainer.
Paul
[[[
Make svnadmin load tolerate mergeinfo with "\r\n".
* subversion/libsvn_repos/load.c
(svn_subst.h): New include.
(renumber_mergeinfo_revs): Normalize mergeinfo line endings to '\n'.
]]]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2217077
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Mark Phippard <ma...@gmail.com>.
On Mon, May 11, 2009 at 5:01 PM, Paul Burba <pt...@gmail.com> wrote:
> Hi John - Do you know how the mergeinfo in r40998 was created? Was it
> the result of a merge or a propset/propedit or something else
> entirely? Also, can you hazard a guess as to what version of
> Subversion (or some other client tool) was used to make it?
>
> If possible I'd like to figure out how the <CR> got in your repository
> in the first place before making any fixes to dump/load.
I am not sure why we are thinking this is anything special (related to
mergeinfo). We have had many reports of people running into this
problem for all sorts of properties. SVN 1.6 requires that all
properties only have LF line endings. If you ever edited the
properties on Windows and did not normalize the line endings yourself,
it is just a given you are going to have this problem. We know that
users are regularly editing svn:mergeinfo so why should we be any more
surprised about this than we are with all of the reports for
svn:ignore, svn:externals and/or log messages?
Personally, I am not sure there was much value in suddenly making the
server enforce this restriction when our client libraries have never
done anything to help with this normalization. If anything, it seems
like we should have put this in the client and let the server still
accept the old property values.
--
Thanks
Mark Phippard
http://markphip.blogspot.com/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2198266
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Mon, May 11, 2009 at 1:36 PM, David Glasser <gl...@davidglasser.net> wrote:
> On Mon, May 11, 2009 at 9:49 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> John Skopis wrote on Mon, 11 May 2009 at 10:52 -0500:
>>>
>>>
>>> >-----Original Message-----
>>> >From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>>> >Sent: Monday, May 11, 2009 9:52 AM
>>> >To: B. Smith-Mannschott
>>> >Cc: David Glasser; John Skopis; dev@subversion.tigris.org
>>> >Subject: Re: [PATCH] svnadmin load will not import dump with windows
>>> >newline character in svn:mergeinfo
>>> >
>>> >What Ben said.
>>> >
>>> >Also, I think the patch here is orthogonal to issue #3404 et al, since
>>> >John said something about a segfault. John, could you say where the
>>> >segfault is? Can you reproduce it without using svnadmin (by direct API
>>> >usage)?
>>>
>>> I just said that the patch below *does not* trigger segfault, implying
>>> that it "works", however correctly it works is more appropriate for
>>> this list to decide.
>>>
>>
>> You said that it didn't segfault with the patch, I inferred that it did
>> segfault without. I can call it a misunderstanding.
>>
>>> It seems to me that no one is too happy about silently filtering out
>>> '\r' from svn:mergeinfo.
>>>
>>> I am not too happy that my svndump won't load.
>>>
>>
>> 'svnadmin load' does allow loading properties with CRLFs, even though
>> you can't commit/svnsync them. Per my other mail --- it seems that
>> svn:mergeinfo is special-cased somewhere (svn_mergeinfo_parse is called
>> and does its own validation).
>
> Haven't looked at this in a while, but I seem to recall that svnadmin
> load will renumber revs in mergeinfo if it's renumbering revs at all.
> I guess that's where this comes into play.
>
> --dave
Dave's recollection is correct. The fix for issue #3020
(http://subversion.tigris.org/issues/show_bug.cgi?id=3020) added
libsvn_repos/load.c:renumber_mergeinfo_revs() which is called from the
svn_repos_parse_fns2_t set_node_property function (Senthil - I'm ccing
you because you made this change).
Maybe the solution is to basically do what John's patch suggests, but
instead of simply tolerating \r\n in mergeinfo properties during
dump/load, actually canonicalize the line endings to \n. Is there any
reason not to do this?
>>> I was able to dump revs 1-40997, 40998, and 40998-head, hand edit
>>> 40998, import all the revs and dump the repo and reimport it (my issue
>>> has been resolved). I then created a patch, should some unfortunate
>>> user experience the same issue as me, and sent it upstream. The rest
>>> is up to you.
Hi John - Do you know how the mergeinfo in r40998 was created? Was it
the result of a merge or a propset/propedit or something else
entirely? Also, can you hazard a guess as to what version of
Subversion (or some other client tool) was used to make it?
If possible I'd like to figure out how the <CR> got in your repository
in the first place before making any fixes to dump/load.
Thanks,
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2197303
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by David Glasser <gl...@davidglasser.net>.
On Mon, May 11, 2009 at 9:49 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> John Skopis wrote on Mon, 11 May 2009 at 10:52 -0500:
>>
>>
>> >-----Original Message-----
>> >From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> >Sent: Monday, May 11, 2009 9:52 AM
>> >To: B. Smith-Mannschott
>> >Cc: David Glasser; John Skopis; dev@subversion.tigris.org
>> >Subject: Re: [PATCH] svnadmin load will not import dump with windows
>> >newline character in svn:mergeinfo
>> >
>> >What Ben said.
>> >
>> >Also, I think the patch here is orthogonal to issue #3404 et al, since
>> >John said something about a segfault. John, could you say where the
>> >segfault is? Can you reproduce it without using svnadmin (by direct API
>> >usage)?
>>
>> I just said that the patch below *does not* trigger segfault, implying
>> that it "works", however correctly it works is more appropriate for
>> this list to decide.
>>
>
> You said that it didn't segfault with the patch, I inferred that it did
> segfault without. I can call it a misunderstanding.
>
>> It seems to me that no one is too happy about silently filtering out
>> '\r' from svn:mergeinfo.
>>
>> I am not too happy that my svndump won't load.
>>
>
> 'svnadmin load' does allow loading properties with CRLFs, even though
> you can't commit/svnsync them. Per my other mail --- it seems that
> svn:mergeinfo is special-cased somewhere (svn_mergeinfo_parse is called
> and does its own validation).
Haven't looked at this in a while, but I seem to recall that svnadmin
load will renumber revs in mergeinfo if it's renumbering revs at all.
I guess that's where this comes into play.
--dave
>> I think svn dump/load should be versatile enough to handle unexpected
>> data. Whether or not it happens at dump time or load time, I don't
>> really care, as long as it works. =]
>>
>> If I was a svn developer, which I am not, I would start an effort to
>> bump the svndump fileformat version so that it is consistent with the
>> current SVN behavior and update the svndump file format specification
>> format (using BNF, and grammer like SHOULD NOT, MUST NOT, et al) so
>> that the file format is well documented.
>>
>> When the svn behavior changes the spec should be updated, the
>> fs-dump-format-version attr bumped, and users provided an upgrade
>> path.
>>
>> svndump *always* produce dumpfiles that conform to the file spec; and
>> svnload *always* able to import dumpfiles conforming to the file spec.
>>
>> Additionally, "Be liberal in what you accept, and conservative in what
>> you send", is a very relevant and applicable quote from rfc 791/1122.
>> ;]
>>
>> I was able to dump revs 1-40997, 40998, and 40998-head, hand edit
>> 40998, import all the revs and dump the repo and reimport it (my issue
>> has been resolved). I then created a patch, should some unfortunate
>> user experience the same issue as me, and sent it upstream. The rest
>> is up to you.
>>
>> Thanks,
>> John
>>
>>
>> >
>> >B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
>> >> > On May 8, 2009 3:23 PM, "John Skopis"
>> ><js...@backstopsolutions.com> wrote:
>> >> >
>> >> > Hello,
>> >> >
>> >> > svnadmin load fails while importing a dump that contains a windows
>> >> > newline in svn:mergeinfo prop. I have not done extensive testing on
>> >> > this patch, but it should work (in that it doesn't segfault when I
>> >> > attempt to import a revision with \r\n in mergeinfo). Be advised I
>> >am not actually a developer.
>> >> >
>> >> > Thanks,
>> >> > John Skopis
>> >> > Systems Administration
>> >> >
>> >> >
>> >> > [[[
>> >> > * subversion/libsvn_subr/mergeinfo.c
>> >> > (parse_revlist): Ignore windows newlines in svn:mergeinfo ]]]
>> >> >
>> >> > Index: subversion/libsvn_subr/mergeinfo.c
>> >> > ===================================================================
>> >> > --- subversion/libsvn_subr/mergeinfo.c (revision 37647)
>> >> > +++ subversion/libsvn_subr/mergeinfo.c (working copy)
>> >> > @@ -402,7 +403,7 @@
>> >> > svn_revnum_t firstrev;
>> >> >
>> >> > SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
>> >> > - if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
>> >'*'
>> >> > + if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
>> >> > + '*' &&
>> >> > *curr != '\r'
>> >> > && curr != end)
>> >> > return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
>> >> > _("Invalid character '%c' found in
>> >> > revision "
>> >> > @@ -430,8 +431,10 @@
>> >> > mrange->end = secondrev;
>> >> > }
>> >> >
>> >> > - if (*curr == '\n' || curr == end)
>> >> > + if (*curr == '\r' || *curr == '\n' || curr == end)
>> >> > {
>> >> > + if ( *curr == '\r' )
>> >> > + curr++;
>> >> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
>> >> > *input = curr;
>> >> > return SVN_NO_ERROR;
>> >> > @@ -445,10 +448,10 @@
>> >> > {
>> >> > mrange->inheritable = FALSE;
>> >> > curr++;
>> >> > - if (*curr == ',' || *curr == '\n' || curr == end)
>> >> > + if (*curr == ',' || *curr == '\n' || *curr == '\r' ||
>> >> > + curr == end
>> >> > )
>> >> > {
>> >> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
>> >> > - if (*curr == ',')
>> >> > + if (*curr == ',' || *curr == '\r' )
>> >> > {
>> >> > curr++;
>> >> > }
>> >> >
>> >>
>> >> On Sun, May 10, 2009 at 22:32, David Glasser
>> ><gl...@davidglasser.net> wrote:
>> >> > Why is there a windows newline in your dump file? Svn dumps are
>> >> > binary files, not text.
>> >> >
>> >> > Now, maybe the svn:mergeinfo property in general should allow
>> >> > windows newlines (which is what your patch does), but why?
>> >>
>> >> No, I don't believe that svn:mergeinfo should allow windows newlines.
>> >> In fact, the fixes for issues 1796 and 3313 conspired to make
>> >> Subversion 1.6 very particular about the newline cleanliness of its
>> >> internal (i.e. svn:*) properties.
>> >>
>> >> http://subversion.tigris.org/issues/show_bug.cgi?id=1796
>> >> http://subversion.tigris.org/issues/show_bug.cgi?id=3313
>> >>
>> >> This new-found strictness lead to issue 3404 (svnload fails on ^M),
>> >> for which there is an as yet unreviewed patch linked to from the
>> >> issue.
>> >>
>> >> http://subversion.tigris.org/issues/show_bug.cgi?id=3404
>> >>
>> >> In any event, it appears that Subversion deliberately uses only \n
>> >> internally. some other parts of the code may well depend on this. I
>> >> don't think it would be advisable to relax the requirements, as above,
>> >> without first investigating this.
>> >>
>> >> // Ben
>> >>
>> >> ------------------------------------------------------
>> >> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessage
>> >> Id=2184013
>> >>
>>
>
--
glasser@davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2194161
RE: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
John Skopis wrote on Mon, 11 May 2009 at 10:52 -0500:
>
>
> >-----Original Message-----
> >From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> >Sent: Monday, May 11, 2009 9:52 AM
> >To: B. Smith-Mannschott
> >Cc: David Glasser; John Skopis; dev@subversion.tigris.org
> >Subject: Re: [PATCH] svnadmin load will not import dump with windows
> >newline character in svn:mergeinfo
> >
> >What Ben said.
> >
> >Also, I think the patch here is orthogonal to issue #3404 et al, since
> >John said something about a segfault. John, could you say where the
> >segfault is? Can you reproduce it without using svnadmin (by direct API
> >usage)?
>
> I just said that the patch below *does not* trigger segfault, implying
> that it "works", however correctly it works is more appropriate for
> this list to decide.
>
You said that it didn't segfault with the patch, I inferred that it did
segfault without. I can call it a misunderstanding.
> It seems to me that no one is too happy about silently filtering out
> '\r' from svn:mergeinfo.
>
> I am not too happy that my svndump won't load.
>
'svnadmin load' does allow loading properties with CRLFs, even though
you can't commit/svnsync them. Per my other mail --- it seems that
svn:mergeinfo is special-cased somewhere (svn_mergeinfo_parse is called
and does its own validation).
Daniel
> I think svn dump/load should be versatile enough to handle unexpected
> data. Whether or not it happens at dump time or load time, I don't
> really care, as long as it works. =]
>
> If I was a svn developer, which I am not, I would start an effort to
> bump the svndump fileformat version so that it is consistent with the
> current SVN behavior and update the svndump file format specification
> format (using BNF, and grammer like SHOULD NOT, MUST NOT, et al) so
> that the file format is well documented.
>
> When the svn behavior changes the spec should be updated, the
> fs-dump-format-version attr bumped, and users provided an upgrade
> path.
>
> svndump *always* produce dumpfiles that conform to the file spec; and
> svnload *always* able to import dumpfiles conforming to the file spec.
>
> Additionally, "Be liberal in what you accept, and conservative in what
> you send", is a very relevant and applicable quote from rfc 791/1122.
> ;]
>
> I was able to dump revs 1-40997, 40998, and 40998-head, hand edit
> 40998, import all the revs and dump the repo and reimport it (my issue
> has been resolved). I then created a patch, should some unfortunate
> user experience the same issue as me, and sent it upstream. The rest
> is up to you.
>
> Thanks,
> John
>
>
> >
> >B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
> >> > On May 8, 2009 3:23 PM, "John Skopis"
> ><js...@backstopsolutions.com> wrote:
> >> >
> >> > Hello,
> >> >
> >> > svnadmin load fails while importing a dump that contains a windows
> >> > newline in svn:mergeinfo prop. I have not done extensive testing on
> >> > this patch, but it should work (in that it doesn't segfault when I
> >> > attempt to import a revision with \r\n in mergeinfo). Be advised I
> >am not actually a developer.
> >> >
> >> > Thanks,
> >> > John Skopis
> >> > Systems Administration
> >> >
> >> >
> >> > [[[
> >> > * subversion/libsvn_subr/mergeinfo.c
> >> > (parse_revlist): Ignore windows newlines in svn:mergeinfo ]]]
> >> >
> >> > Index: subversion/libsvn_subr/mergeinfo.c
> >> > ===================================================================
> >> > --- subversion/libsvn_subr/mergeinfo.c (revision 37647)
> >> > +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> >> > @@ -402,7 +403,7 @@
> >> > svn_revnum_t firstrev;
> >> >
> >> > SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
> >> > - if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
> >'*'
> >> > + if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
> >> > + '*' &&
> >> > *curr != '\r'
> >> > && curr != end)
> >> > return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
> >> > _("Invalid character '%c' found in
> >> > revision "
> >> > @@ -430,8 +431,10 @@
> >> > mrange->end = secondrev;
> >> > }
> >> >
> >> > - if (*curr == '\n' || curr == end)
> >> > + if (*curr == '\r' || *curr == '\n' || curr == end)
> >> > {
> >> > + if ( *curr == '\r' )
> >> > + curr++;
> >> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
> >> > *input = curr;
> >> > return SVN_NO_ERROR;
> >> > @@ -445,10 +448,10 @@
> >> > {
> >> > mrange->inheritable = FALSE;
> >> > curr++;
> >> > - if (*curr == ',' || *curr == '\n' || curr == end)
> >> > + if (*curr == ',' || *curr == '\n' || *curr == '\r' ||
> >> > + curr == end
> >> > )
> >> > {
> >> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
> >> > - if (*curr == ',')
> >> > + if (*curr == ',' || *curr == '\r' )
> >> > {
> >> > curr++;
> >> > }
> >> >
> >>
> >> On Sun, May 10, 2009 at 22:32, David Glasser
> ><gl...@davidglasser.net> wrote:
> >> > Why is there a windows newline in your dump file? Svn dumps are
> >> > binary files, not text.
> >> >
> >> > Now, maybe the svn:mergeinfo property in general should allow
> >> > windows newlines (which is what your patch does), but why?
> >>
> >> No, I don't believe that svn:mergeinfo should allow windows newlines.
> >> In fact, the fixes for issues 1796 and 3313 conspired to make
> >> Subversion 1.6 very particular about the newline cleanliness of its
> >> internal (i.e. svn:*) properties.
> >>
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=1796
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=3313
> >>
> >> This new-found strictness lead to issue 3404 (svnload fails on ^M),
> >> for which there is an as yet unreviewed patch linked to from the
> >> issue.
> >>
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=3404
> >>
> >> In any event, it appears that Subversion deliberately uses only \n
> >> internally. some other parts of the code may well depend on this. I
> >> don't think it would be advisable to relax the requirements, as above,
> >> without first investigating this.
> >>
> >> // Ben
> >>
> >> ------------------------------------------------------
> >> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessage
> >> Id=2184013
> >>
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2193527
RE: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by John Skopis <js...@backstopsolutions.com>.
>-----Original Message-----
>From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>Sent: Monday, May 11, 2009 9:52 AM
>To: B. Smith-Mannschott
>Cc: David Glasser; John Skopis; dev@subversion.tigris.org
>Subject: Re: [PATCH] svnadmin load will not import dump with windows
>newline character in svn:mergeinfo
>
>What Ben said.
>
>Also, I think the patch here is orthogonal to issue #3404 et al, since
>John said something about a segfault. John, could you say where the
>segfault is? Can you reproduce it without using svnadmin (by direct API
>usage)?
I just said that the patch below *does not* trigger segfault, implying that it "works", however correctly it works is more appropriate for this list to decide.
It seems to me that no one is too happy about silently filtering out '\r' from svn:mergeinfo.
I am not too happy that my svndump won't load.
I think svn dump/load should be versatile enough to handle unexpected data. Whether or not it happens at dump time or load time, I don't really care, as long as it works. =]
If I was a svn developer, which I am not, I would start an effort to bump the svndump fileformat version so that it is consistent with the current SVN behavior and update the svndump file format specification format (using BNF, and grammer like SHOULD NOT, MUST NOT, et al) so that the file format is well documented.
When the svn behavior changes the spec should be updated, the fs-dump-format-version attr bumped, and users provided an upgrade path.
svndump *always* produce dumpfiles that conform to the file spec; and svnload *always* able to import dumpfiles conforming to the file spec.
Additionally, "Be liberal in what you accept, and conservative in what you send", is a very relevant and applicable quote from rfc 791/1122. ;]
I was able to dump revs 1-40997, 40998, and 40998-head, hand edit 40998, import all the revs and dump the repo and reimport it (my issue has been resolved). I then created a patch, should some unfortunate user experience the same issue as me, and sent it upstream. The rest is up to you.
Thanks,
John
>
>B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
>> > On May 8, 2009 3:23 PM, "John Skopis"
><js...@backstopsolutions.com> wrote:
>> >
>> > Hello,
>> >
>> > svnadmin load fails while importing a dump that contains a windows
>> > newline in svn:mergeinfo prop. I have not done extensive testing on
>> > this patch, but it should work (in that it doesn't segfault when I
>> > attempt to import a revision with \r\n in mergeinfo). Be advised I
>am not actually a developer.
>> >
>> > Thanks,
>> > John Skopis
>> > Systems Administration
>> >
>> >
>> > [[[
>> > * subversion/libsvn_subr/mergeinfo.c
>> > (parse_revlist): Ignore windows newlines in svn:mergeinfo ]]]
>> >
>> > Index: subversion/libsvn_subr/mergeinfo.c
>> > ===================================================================
>> > --- subversion/libsvn_subr/mergeinfo.c (revision 37647)
>> > +++ subversion/libsvn_subr/mergeinfo.c (working copy)
>> > @@ -402,7 +403,7 @@
>> > svn_revnum_t firstrev;
>> >
>> > SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
>> > - if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
>'*'
>> > + if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
>> > + '*' &&
>> > *curr != '\r'
>> > && curr != end)
>> > return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
>> > _("Invalid character '%c' found in
>> > revision "
>> > @@ -430,8 +431,10 @@
>> > mrange->end = secondrev;
>> > }
>> >
>> > - if (*curr == '\n' || curr == end)
>> > + if (*curr == '\r' || *curr == '\n' || curr == end)
>> > {
>> > + if ( *curr == '\r' )
>> > + curr++;
>> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
>> > *input = curr;
>> > return SVN_NO_ERROR;
>> > @@ -445,10 +448,10 @@
>> > {
>> > mrange->inheritable = FALSE;
>> > curr++;
>> > - if (*curr == ',' || *curr == '\n' || curr == end)
>> > + if (*curr == ',' || *curr == '\n' || *curr == '\r' ||
>> > + curr == end
>> > )
>> > {
>> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
>> > - if (*curr == ',')
>> > + if (*curr == ',' || *curr == '\r' )
>> > {
>> > curr++;
>> > }
>> >
>>
>> On Sun, May 10, 2009 at 22:32, David Glasser
><gl...@davidglasser.net> wrote:
>> > Why is there a windows newline in your dump file? Svn dumps are
>> > binary files, not text.
>> >
>> > Now, maybe the svn:mergeinfo property in general should allow
>> > windows newlines (which is what your patch does), but why?
>>
>> No, I don't believe that svn:mergeinfo should allow windows newlines.
>> In fact, the fixes for issues 1796 and 3313 conspired to make
>> Subversion 1.6 very particular about the newline cleanliness of its
>> internal (i.e. svn:*) properties.
>>
>> http://subversion.tigris.org/issues/show_bug.cgi?id=1796
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3313
>>
>> This new-found strictness lead to issue 3404 (svnload fails on ^M),
>> for which there is an as yet unreviewed patch linked to from the
>> issue.
>>
>> http://subversion.tigris.org/issues/show_bug.cgi?id=3404
>>
>> In any event, it appears that Subversion deliberately uses only \n
>> internally. some other parts of the code may well depend on this. I
>> don't think it would be advisable to relax the requirements, as above,
>> without first investigating this.
>>
>> // Ben
>>
>> ------------------------------------------------------
>> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessage
>> Id=2184013
>>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2192780
RE: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
John Skopis wrote on Mon, 11 May 2009 at 10:54 -0500:
>
>
> >-----Original Message-----
> >From: Paul Burba [mailto:ptburba@gmail.com]
> >Sent: Monday, May 11, 2009 10:49 AM
> >To: Daniel Shahaf
> >Cc: B. Smith-Mannschott; David Glasser; John Skopis;
> >dev@subversion.tigris.org
> >Subject: Re: [PATCH] svnadmin load will not import dump with windows
> >newline character in svn:mergeinfo
> >
> >On Mon, May 11, 2009 at 10:51 AM, Daniel Shahaf <d....@daniel.shahaf.name>
> >wrote:
> >> What Ben said.
> >>
> >> Also, I think the patch here is orthogonal to issue #3404 et al, since
> >> John said something about a segfault. John, could you say where the
> >> segfault is? Can you reproduce it without using svnadmin (by direct
> >> API usage)?
> >>
> >> B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
> >>> > On May 8, 2009 3:23 PM, "John Skopis"
> ><js...@backstopsolutions.com> wrote:
> >>> >
> >>> > Hello,
> >>> >
> >>> > svnadmin load fails while importing a dump that contains a windows
> >>> > newline in svn:mergeinfo prop. I have not done extensive testing on
> >>> > this patch, but it should work (in that it doesn't segfault when I
> >>> > attempt to import a revision with \r\n in mergeinfo). Be advised I
> >am not actually a developer.
> >
> >John,
> >
> >
> >To follow-up on what Daniel asked: How exactly did svnadmin load fail?
> > Did you actually get a segfault or did you get an error something like
> >this?
> >
> > 1.6.2>svnadmin load ..\repositories\t1 < 131.dump
> >
> > <SNIP successful txn>
> >
> > ------- Committed revision 7 >>>
> >
> > <<< Started new transaction, based on original revision 8
> > * editing path : A_COPY ...svnadmin: Dumpstream data appears to
> >be malformed
> >
> >Or something else entirely?
> >
> >Paul
>
> Paul, here's what it looks like:
> ------- Committed revision 40997 >>>
>
> <<< Started new transaction, based on original revision 40998
> ' found in revision listter '
> # echo $?
> 1
>
> This made it extra hard to find where it actually failed as the \r in the printf made it nearly impossible to grep for the error printf string. =]
>
>
>
RE: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by John Skopis <js...@backstopsolutions.com>.
>-----Original Message-----
>From: Paul Burba [mailto:ptburba@gmail.com]
>Sent: Monday, May 11, 2009 10:49 AM
>To: Daniel Shahaf
>Cc: B. Smith-Mannschott; David Glasser; John Skopis;
>dev@subversion.tigris.org
>Subject: Re: [PATCH] svnadmin load will not import dump with windows
>newline character in svn:mergeinfo
>
>On Mon, May 11, 2009 at 10:51 AM, Daniel Shahaf <d....@daniel.shahaf.name>
>wrote:
>> What Ben said.
>>
>> Also, I think the patch here is orthogonal to issue #3404 et al, since
>> John said something about a segfault. John, could you say where the
>> segfault is? Can you reproduce it without using svnadmin (by direct
>> API usage)?
>>
>> B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
>>> > On May 8, 2009 3:23 PM, "John Skopis"
><js...@backstopsolutions.com> wrote:
>>> >
>>> > Hello,
>>> >
>>> > svnadmin load fails while importing a dump that contains a windows
>>> > newline in svn:mergeinfo prop. I have not done extensive testing on
>>> > this patch, but it should work (in that it doesn't segfault when I
>>> > attempt to import a revision with \r\n in mergeinfo). Be advised I
>am not actually a developer.
>
>John,
>
>
>To follow-up on what Daniel asked: How exactly did svnadmin load fail?
> Did you actually get a segfault or did you get an error something like
>this?
>
> 1.6.2>svnadmin load ..\repositories\t1 < 131.dump
>
> <SNIP successful txn>
>
> ------- Committed revision 7 >>>
>
> <<< Started new transaction, based on original revision 8
> * editing path : A_COPY ...svnadmin: Dumpstream data appears to
>be malformed
>
>Or something else entirely?
>
>Paul
Paul, here's what it looks like:
------- Committed revision 40997 >>>
<<< Started new transaction, based on original revision 40998
' found in revision listter '
# echo $?
1
This made it extra hard to find where it actually failed as the \r in the printf made it nearly impossible to grep for the error printf string. =]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2192782
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Paul Burba <pt...@gmail.com>.
On Mon, May 11, 2009 at 10:51 AM, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> What Ben said.
>
> Also, I think the patch here is orthogonal to issue #3404 et al, since
> John said something about a segfault. John, could you say where the
> segfault is? Can you reproduce it without using svnadmin (by direct API
> usage)?
>
> B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
>> > On May 8, 2009 3:23 PM, "John Skopis" <js...@backstopsolutions.com> wrote:
>> >
>> > Hello,
>> >
>> > svnadmin load fails while importing a dump that contains a windows newline
>> > in svn:mergeinfo prop. I have not done extensive testing on this patch, but
>> > it should work (in that it doesn't segfault when I attempt to import a
>> > revision with \r\n in mergeinfo). Be advised I am not actually a developer.
John,
To follow-up on what Daniel asked: How exactly did svnadmin load fail?
Did you actually get a segfault or did you get an error something
like this?
1.6.2>svnadmin load ..\repositories\t1 < 131.dump
<SNIP successful txn>
------- Committed revision 7 >>>
<<< Started new transaction, based on original revision 8
* editing path : A_COPY ...svnadmin: Dumpstream data appears to
be malformed
Or something else entirely?
Paul
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2192755
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
What Ben said.
Also, I think the patch here is orthogonal to issue #3404 et al, since
John said something about a segfault. John, could you say where the
segfault is? Can you reproduce it without using svnadmin (by direct API
usage)?
B. Smith-Mannschott wrote on Mon, 11 May 2009 at 07:31 +0200:
> > On May 8, 2009 3:23 PM, "John Skopis" <js...@backstopsolutions.com> wrote:
> >
> > Hello,
> >
> > svnadmin load fails while importing a dump that contains a windows newline
> > in svn:mergeinfo prop. I have not done extensive testing on this patch, but
> > it should work (in that it doesn't segfault when I attempt to import a
> > revision with \r\n in mergeinfo). Be advised I am not actually a developer.
> >
> > Thanks,
> > John Skopis
> > Systems Administration
> >
> >
> > [[[
> > * subversion/libsvn_subr/mergeinfo.c
> > (parse_revlist): Ignore windows newlines in svn:mergeinfo
> > ]]]
> >
> > Index: subversion/libsvn_subr/mergeinfo.c
> > ===================================================================
> > --- subversion/libsvn_subr/mergeinfo.c (revision 37647)
> > +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> > @@ -402,7 +403,7 @@
> > svn_revnum_t firstrev;
> >
> > SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
> > - if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*'
> > + if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*' &&
> > *curr != '\r'
> > && curr != end)
> > return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
> > _("Invalid character '%c' found in revision
> > "
> > @@ -430,8 +431,10 @@
> > mrange->end = secondrev;
> > }
> >
> > - if (*curr == '\n' || curr == end)
> > + if (*curr == '\r' || *curr == '\n' || curr == end)
> > {
> > + if ( *curr == '\r' )
> > + curr++;
> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
> > *input = curr;
> > return SVN_NO_ERROR;
> > @@ -445,10 +448,10 @@
> > {
> > mrange->inheritable = FALSE;
> > curr++;
> > - if (*curr == ',' || *curr == '\n' || curr == end)
> > + if (*curr == ',' || *curr == '\n' || *curr == '\r' || curr == end
> > )
> > {
> > APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
> > - if (*curr == ',')
> > + if (*curr == ',' || *curr == '\r' )
> > {
> > curr++;
> > }
> >
>
> On Sun, May 10, 2009 at 22:32, David Glasser <gl...@davidglasser.net> wrote:
> > Why is there a windows newline in your dump file? Svn dumps are binary
> > files, not text.
> >
> > Now, maybe the svn:mergeinfo property in general should allow windows
> > newlines (which is what your patch does), but why?
>
> No, I don't believe that svn:mergeinfo should allow windows newlines.
> In fact, the fixes for issues 1796 and 3313 conspired to make
> Subversion 1.6 very particular about the newline cleanliness of its
> internal (i.e. svn:*) properties.
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=1796
> http://subversion.tigris.org/issues/show_bug.cgi?id=3313
>
> This new-found strictness lead to issue 3404 (svnload fails on ^M),
> for which there is an as yet unreviewed patch linked to from the
> issue.
>
> http://subversion.tigris.org/issues/show_bug.cgi?id=3404
>
> In any event, it appears that Subversion deliberately uses only \n
> internally. some other parts of the code may well depend on this. I
> don't think it would be advisable to relax the requirements, as above,
> without first investigating this.
>
> // Ben
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2184013
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2191834
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by "B. Smith-Mannschott" <bs...@gmail.com>.
> On May 8, 2009 3:23 PM, "John Skopis" <js...@backstopsolutions.com> wrote:
>
> Hello,
>
> svnadmin load fails while importing a dump that contains a windows newline
> in svn:mergeinfo prop. I have not done extensive testing on this patch, but
> it should work (in that it doesn't segfault when I attempt to import a
> revision with \r\n in mergeinfo). Be advised I am not actually a developer.
>
> Thanks,
> John Skopis
> Systems Administration
>
>
> [[[
> * subversion/libsvn_subr/mergeinfo.c
> (parse_revlist): Ignore windows newlines in svn:mergeinfo
> ]]]
>
> Index: subversion/libsvn_subr/mergeinfo.c
> ===================================================================
> --- subversion/libsvn_subr/mergeinfo.c (revision 37647)
> +++ subversion/libsvn_subr/mergeinfo.c (working copy)
> @@ -402,7 +403,7 @@
> svn_revnum_t firstrev;
>
> SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
> - if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*'
> + if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*' &&
> *curr != '\r'
> && curr != end)
> return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
> _("Invalid character '%c' found in revision
> "
> @@ -430,8 +431,10 @@
> mrange->end = secondrev;
> }
>
> - if (*curr == '\n' || curr == end)
> + if (*curr == '\r' || *curr == '\n' || curr == end)
> {
> + if ( *curr == '\r' )
> + curr++;
> APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
> *input = curr;
> return SVN_NO_ERROR;
> @@ -445,10 +448,10 @@
> {
> mrange->inheritable = FALSE;
> curr++;
> - if (*curr == ',' || *curr == '\n' || curr == end)
> + if (*curr == ',' || *curr == '\n' || *curr == '\r' || curr == end
> )
> {
> APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
> - if (*curr == ',')
> + if (*curr == ',' || *curr == '\r' )
> {
> curr++;
> }
>
On Sun, May 10, 2009 at 22:32, David Glasser <gl...@davidglasser.net> wrote:
> Why is there a windows newline in your dump file? Svn dumps are binary
> files, not text.
>
> Now, maybe the svn:mergeinfo property in general should allow windows
> newlines (which is what your patch does), but why?
No, I don't believe that svn:mergeinfo should allow windows newlines.
In fact, the fixes for issues 1796 and 3313 conspired to make
Subversion 1.6 very particular about the newline cleanliness of its
internal (i.e. svn:*) properties.
http://subversion.tigris.org/issues/show_bug.cgi?id=1796
http://subversion.tigris.org/issues/show_bug.cgi?id=3313
This new-found strictness lead to issue 3404 (svnload fails on ^M),
for which there is an as yet unreviewed patch linked to from the
issue.
http://subversion.tigris.org/issues/show_bug.cgi?id=3404
In any event, it appears that Subversion deliberately uses only \n
internally. some other parts of the code may well depend on this. I
don't think it would be advisable to relax the requirements, as above,
without first investigating this.
// Ben
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2184013
Re: [PATCH] svnadmin load will not import dump with windows newline
character in svn:mergeinfo
Posted by David Glasser <gl...@davidglasser.net>.
Why is there a windows newline in your dump file? Svn dumps are binary
files, not text.
Now, maybe the svn:mergeinfo property in general should allow windows
newlines (which is what your patch does), but why?
--dave
On May 8, 2009 3:23 PM, "John Skopis" <js...@backstopsolutions.com> wrote:
Hello,
svnadmin load fails while importing a dump that contains a windows newline
in svn:mergeinfo prop. I have not done extensive testing on this patch, but
it should work (in that it doesn't segfault when I attempt to import a
revision with \r\n in mergeinfo). Be advised I am not actually a developer.
Thanks,
John Skopis
Systems Administration
[[[
* subversion/libsvn_subr/mergeinfo.c
(parse_revlist): Ignore windows newlines in svn:mergeinfo
]]]
Index: subversion/libsvn_subr/mergeinfo.c
===================================================================
--- subversion/libsvn_subr/mergeinfo.c (revision 37647)
+++ subversion/libsvn_subr/mergeinfo.c (working copy)
@@ -402,7 +403,7 @@
svn_revnum_t firstrev;
SVN_ERR(svn_revnum_parse(&firstrev, curr, &curr));
- if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*'
+ if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr != '*' &&
*curr != '\r'
&& curr != end)
return svn_error_createf(SVN_ERR_MERGEINFO_PARSE_ERROR, NULL,
_("Invalid character '%c' found in revision
"
@@ -430,8 +431,10 @@
mrange->end = secondrev;
}
- if (*curr == '\n' || curr == end)
+ if (*curr == '\r' || *curr == '\n' || curr == end)
{
+ if ( *curr == '\r' )
+ curr++;
APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
*input = curr;
return SVN_NO_ERROR;
@@ -445,10 +448,10 @@
{
mrange->inheritable = FALSE;
curr++;
- if (*curr == ',' || *curr == '\n' || curr == end)
+ if (*curr == ',' || *curr == '\n' || *curr == '\r' || curr == end
)
{
APR_ARRAY_PUSH(revlist, svn_merge_range_t *) = mrange;
- if (*curr == ',')
+ if (*curr == ',' || *curr == '\r' )
{
curr++;
}
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2121222
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2176270