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

RE: svn commit: r1678151 - in /subversion/trunk/subversion/libsvn_fs_fs: cached_data.c fs_fs.c recovery.c transaction.c

From: Bert Huijben <bert_at_qqmail.nl>
Date: Thu, 7 May 2015 15:19:55 +0200

> -----Original Message-----
> From: stsp_at_apache.org [mailto:stsp_at_apache.org]
> Sent: donderdag 7 mei 2015 11:48
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1678151 - in /subversion/trunk/subversion/libsvn_fs_fs:
> cached_data.c fs_fs.c recovery.c transaction.c
>
> Author: stsp
> Date: Thu May 7 09:48:25 2015
> New Revision: 1678151
>
> URL: http://svn.apache.org/r1678151
> Log:
> Follow-up to r1678150:
>
> * subversion/libsvn_fs_fs/cached_data.c
> (svn_fs_fs__get_proplist): Don't quick-wrap hash parsing errors but add an
> error to the chain. This way, the hash parser's error message is preserved.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (get_node_origins_from_file): Likewise.
>
> * subversion/libsvn_fs_fs/recovery.c
> (recover_find_max_ids): Likewise.
>
> * subversion/libsvn_fs_fs/transaction.c
> (get_txn_proplist): Likewise.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> subversion/trunk/subversion/libsvn_fs_fs/recovery.c
> subversion/trunk/subversion/libsvn_fs_fs/transaction.c
>
> Modified: subversion/trunk/subversion/libsvn_fs_fs/cached_data.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/cached
> _data.c?rev=1678151&r1=1678150&r2=1678151&view=diff
> ================================================================
> ==============
> --- subversion/trunk/subversion/libsvn_fs_fs/cached_data.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/cached_data.c Thu May 7
> 09:48:25 2015
> @@ -2736,7 +2736,7 @@ svn_fs_fs__get_proplist(apr_hash_t **pro
> svn_string_t *id_str = svn_fs_fs__id_unparse(noderev->id, pool);
>
> svn_error_clear(svn_stream_close(stream));
> - return svn_error_quick_wrapf(err,
> + return svn_error_createf(SVN_ERR_FS_CORRUPT, err,
> _("malformed property list for node-revision '%s' in '%s'"),
> id_str->data, filename);

Your log message documents this change as 'This way, the hash parser's error message is preserved.', which is not what this change does.

Before and after this change the message was preserved, but now a different error code is returned; that is a different kind of change.

Personally I would have also changed the line above this to something like:
err = svn_error_compose_creater(err, svn_stream_close(stream));

To avoid dropping more error details than necessary.

See also below for a behavior change, that also applies here.

> }
> @@ -2769,7 +2769,7 @@ svn_fs_fs__get_proplist(apr_hash_t **pro
> svn_string_t *id_str = svn_fs_fs__id_unparse(noderev->id, pool);
>
> svn_error_clear(svn_stream_close(stream));
> - return svn_error_quick_wrapf(err,
> + return svn_error_createf(SVN_ERR_FS_CORRUPT, err,
> _("malformed property list for node-revision '%s'"),
> id_str->data);

Same thing applies here.
> }

The quick wrap code, does the same wrapping as the svn_error_createf(). It just explicitly doesn't alter the error code.

*and* more importantly it doesn't return an error if the inner error is not an error after all.

So this change changes the code from potentially returning NULL, to always returning an explicit error.
(Whether this is a good thing or not, requires a more closely review of te surrounding code)

        Bert
Received on 2015-05-07 15:20:11 CEST

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