Stefan Fuhrmann wrote on Tue, Oct 07, 2014 at 15:02:41 +0200:
> On Tue, Oct 7, 2014 at 1:27 PM, Stefan Sperling <s...@elego.de> wrote:
> > On Tue, Oct 07, 2014 at 10:41:14AM -0000, stef...@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
