On 02.12.2010 08:18, Daniel Shahaf wrote:
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.
Thanks.
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.
Yup. The comment is just one of those "last minute improvements" ...
}
}
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)'.
r1042478 should make the logic more obvious.
-- Stefan^2.
