> On Wed, 2010-12-01, stef...@apache.org wrote:
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1040832&r1=1040831&r2=1040832&view=diff
> > ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Wed Dec 1 00:15:11
> > 2010
> > @@ -1903,21 +1903,42 @@ open_pack_or_rev_file(apr_file_t **file,
> > {
> > svn_error_t *err;
> > const char *path;
> > svn_boolean_t retry = FALSE;
> >
I agree the code below is correct, but I found it confusing:
> > do
> > {
> > err = svn_fs_fs__path_rev_absolute(&path, fs, rev, pool);
> >
> > /* open the revision file in buffered r/o mode */
> > if (! err)
> > err = svn_io_file_open(file, path,
> > APR_READ | APR_BUFFERED, APR_OS_DEFAULT,
> > pool);
> >
> > if (err && APR_STATUS_IS_ENOENT(err->apr_err))
> > {
> > /* Could not open the file. This may happen if the
> > * file once existed but got packed later. */
> > svn_error_clear(err);
> >
> > /* if that was our 2nd attempt, leave it at that. */
> > if (retry)
> > return svn_error_createf(SVN_ERR_FS_NO_SUCH_REVISION, NULL,
> > _("No such revision %ld"), rev);
> >
> > /* we failed for the first time. Refresh cache & retry. */
> > SVN_ERR(update_min_unpacked_rev(fs, pool));
> >
Philip noted that this call should be guarded by a format number check
(otherwise we would assert on format-3 repositories that are missing
a rev file). I've fixed that.
> > retry = TRUE;
> > }
> > else
> > {
> > /* the file exists but something prevented us from opnening it */
> > return svn_error_return(err);
The comment doesn't indicate that the else{} block is also entered in
the rare case where ERR is SVN_NO_ERROR.
In other words, the "success" case is handled not by the 'return SVN_NO_ERROR'
below (which in fact is never reached), but by this else{} block.
> > }
> > }
> > while (err);
> >
> > return SVN_NO_ERROR;
> > }
The error handling confused me here: it clears ERR but then checks that
it's non-NULL, and right after that check (which normally means "there
is an error") it overwrites ERR. I think the loop would be clearer if
were just 'while (1)'.
>
>
