>-----Original Message-----
>From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
>Sent: Monday, May 11, 2009 9:52 AM
>To: B. Smith-Mannschott
>Cc: David Glasser; John Skopis; dev_at_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"
><jskopis_at_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
><glasser_at_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
Received on 2009-05-11 17:52:53 CEST