[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1629854 - in /subversion/trunk/subversion/libsvn_fs_fs: fs.c fs_fs.c

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Tue, 7 Oct 2014 13:41:38 +0000

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_at_elego.de> wrote:
> > On Tue, Oct 07, 2014 at 10:41:14AM -0000, stefan2_at_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


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.

Received on 2014-10-07 15:42:09 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.