subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c
Date Tue, 07 Oct 2014 13:41:38 GMT
Stefan Fuhrmann wrote on Tue, Oct 07, 2014 at 15:02:41 +0200:
> On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <stsp@elego.de> wrote:
> > On Tue, Oct 07, 2014 at 10:41:14AM -0000, stefan2@apache.org wrote:
> > > +  /* Figure out the repo format and check that we can even handle it. */
> > > +  SVN_ERR(svn_fs_fs__read_format_file(fs, subpool));
> > > +
> > > +  /* Now, read 'current' and try to patch it if necessary. */
> > > +  err = svn_fs_fs__youngest_rev(&youngest_rev, fs, subpool);
> > > +  if (err)
> >
> > Can't we check for a specific error code here, and return the
> > error otherwise? This would make the intention of the error handling
> > code explicit and avoid masking of arbitrary error conditions.
> >
> 
> If we wanted to enumerate all "unsurprising" error conditions,
> it might become quite a long list. After all, things are most
> likely broken when you run recover. To me, it seems best
> to try to get into a working state *despite* any previous errors.
> r1629879 tries to explain that.

I was also alarmed by the lack of specific error check, but my reasoning
for considering it okay was different: the worst-case if we unlink and
rewrite the 'current' file is that we do a bunch of work (namely,
figuring out youngest and highest-used-id by crawling the revs/ tree)
that we didn't actually have to do (e.g., because the error condition
was transient).  So the failure mode is being inefficient^W
unnecessarily thorough, but not incorrectness.

There is a separate issue about malfunctions: when the API consumer
installs svn_error_raise_on_malfunction() as the malfunction handler,
any code that does

    if (err)

may want to instead do

    if (err && !SVN_ERROR_IN_CATEGORY(SVN_ERR_MALFUNC_CATEGORY_START))

so as to propagate the error immediately, without trying to handle it.
But this is a wider issue --- even the SVN_ERR() macro suffers from it.

Daniel

Mime
View raw message