Login | Register
My pages Projects Community openCollabNet

Discussions > dev [DISABLED] > RE: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo

subversion
Discussion topic

Back to topic list

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

Author Daniel Shahaf <d dot s at daniel dot shahaf dot name>
Full name Daniel Shahaf <d dot s at daniel dot shahaf dot name>
Date 2009-05-11 09:49:33 PDT
Message John Skopis wrote on Mon, 11 May 2009 at 10:52 -0500:
>
>
> >-----Original Message-----
> >From: Daniel Shahaf [mailto:d dot s at daniel dot shahaf dot name]
> >Sent: Monday, May 11, 2009 9:52 AM
> >To: B. Smith-Mannschott
> >Cc: David Glasser; John Skopis; dev at subversion dot tigris dot 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@back​stopsolutions.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_su​br/mergeinfo.c
> >> > (parse_revlist): Ignore windows newlines in svn:mergeinfo ]]]
> >> >
> >> > Index: subversion/libsvn_su​br/mergeinfo.c
> >> > ====================​====================​====================​=======
> >> > --- subversion/libsvn_su​br/mergeinfo.c (revision 37647)
> >> > +++ subversion/libsvn_su​br/mergeinfo.c (working copy)
> >> > @@ -402,7 +403,7 @@
> >> > svn_revnum_t firstrev;
> >> >
> >> > SVN_ERR(svn_revnum_p​arse(&firstrev, curr, &curr));
> >> > - if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
> >'*'
> >> > + if (*curr != '-' && *curr != '\n' && *curr != ',' && *curr !=
> >> > + '*' &&
> >> > *curr != '\r'
> >> > && curr != end)
> >> > return svn_error_createf(SV​N_ERR_MERGEINFO_PARS​E_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@davi​dglasser.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.ti​gris.org/issues/show​_bug.cgi?id=1796
> >> http://subversion.ti​gris.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.ti​gris.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.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessage
> >> Id=2184013
> >>
>

« Previous message in topic | 10 of 16 | Next message in topic »

Messages

Show all messages in topic

[PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo John Skopis <jskopis at backstopsolutions dot com> John Skopis <jskopis at backstopsolutions dot com> 2009-05-08 13:34:43 PDT
     Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo glasser David Glasser 2009-05-10 13:33:00 PDT
         Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo bpsm B. Smith-Mannschott 2009-05-10 22:31:18 PDT
             Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo bpsm B. Smith-Mannschott 2009-05-11 01:24:32 PDT
             Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-11 07:52:11 PDT
                 Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-11 08:49:12 PDT
                     RE: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo John Skopis <jskopis at backstopsolutions dot com> John Skopis <jskopis at backstopsolutions dot com> 2009-05-11 08:54:19 PDT
                         RE: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-11 09:38:31 PDT
                 RE: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo John Skopis <jskopis at backstopsolutions dot com> John Skopis <jskopis at backstopsolutions dot com> 2009-05-11 08:52:39 PDT
                     RE: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-11 09:49:33 PDT
                         Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo glasser David Glasser 2009-05-11 10:36:20 PDT
                             Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-11 14:01:16 PDT
                                 Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo markphip Mark Phippard 2009-05-11 15:21:12 PDT
                                     Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Paul Burba <ptburba at gmail dot com> Paul Burba <ptburba at gmail dot com> 2009-05-12 09:37:54 PDT
                                         Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo Daniel Shahaf <d dot s at daniel dot shahaf dot name> Daniel Shahaf <d dot s at daniel dot shahaf dot name> 2009-05-12 09:53:35 PDT
     Re: [PATCH] svnadmin load will not import dump with windows newline character in svn:mergeinfo John Skopis <jskopis at backstopsolutions dot com> John Skopis <jskopis at backstopsolutions dot com> 2009-05-11 15:32:28 PDT
Messages per page: