On 02.12.2010 08:18, Daniel Shahaf wrote:
>> On Wed, 2010-12-01, stefan2_at_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.
Received on 2010-12-05 23:49:46 CET