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).
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"
> ><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=2193527
Received on 2009-05-11 18:50:04 CEST