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

On Mon, May 11, 2009 at 9:49 AM, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> John Skopis wrote on Mon, 11 May 2009 at 10:52 -0500:
>>
>>
>> >-----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.
>>
>
> 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"
>> ><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
>> >>
>>
>

-- 
glasser_at_davidglasser.net | langtonlabs.org | flickr.com/photos/glasser/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2194161