Ramkumar Ramachandra wrote on Thu, Aug 19, 2010 at 21:28:37 +0530:
> Hi Daniel,
>
> Daniel Shahaf writes:
> > > - SVN_ERR(svn_repos_parse_dumpstream2(stream, parser, parse_baton,
> > > - NULL, NULL, pool));
> > > + SVN_ERR(svn_repos_parse_dumpstream3(stream, parser, parse_baton,
> > > + NULL, NULL,
> > > + SVN_REPOS_DUMPFILE_FORMAT_VERSION,
> >
> > Wrong macro: when we introduce dumpfile format 4, this will become "4",
> > but I think you want it to remain "3" even then, right?
>
> Actually, I'd want to svnrdump tests to fail so someone (or me)
> immediately fixes it to use version 4: it'll probably be a performance
> boost as well.
>
Feel free to suggest a test that will remind us to patch up v4 support
into svnrdump. However, as the quoted patch is written, svnrdump will
start croaking on v3 dumpfiles as soon as v4 is defined. And I do not
think that's acceptable.
(Limit case: if I just go today and write a patch that defines v4 dump
to be *exactly the same as* v3 dumps, then svnrdump will start to die on
v3 dumps.)
> > > + * it is ignored and the dumpstream is parsed without this
> > > + * information. Else, the function checks the dumpstream's version
> > > + * number, and errors out if there's a mismatch.
> > > + *
> >
> > Sometimes, in situations like this we guarantee exactly which error code
> > will be returned, for the benefit of API users who want to trap a
> > specific (non-fatal) error condition. See svn_ra_open4() for an
> > example.
>
> Oh. Is this necessary though? I'm going to move out this code into a
> callback soon- users can handle it there after that.
>
> > I'm not sure what we gain by committing this patch when you already have
> > a parse_fns3_t patch outstanding. (I haven't looked at that thread
> > yet.) Wouldn't it make more sense to first commit that patch, than to
> > commit this one, then that one, and then a revision of that one to use
> > the new API?
>
> That patch is still an RFC, and it's unlikely to be approved soon I
> think. If I were able to send a series, it would roughly look like
> this:
> 1. Create parse_dumpstream3 to include the logic for checking equality
> in version.
> 2. Note the lack of flexibility in 1, and create a new struct (the
> other patch).
> 3. Modify parse_dumpstream3 to use the new struct, and move out the
> logic for checking the version into a fresh callback.
>
> > Note: it's acceptable to post patches that depend on previous patches.
> > (So you could write this patch in terms of parse_fns3_t directly.)
So we first create parse_dumpstream3() and then fix it a second later?
I'd rather just revv the parse_fns3_t API (with "the other patch") and
then touch parse_dumpstream3() once. Does that make sense to you?
Received on 2010-08-19 19:28:44 CEST