Re: svn commit: rev 7857 - in trunk/subversion: libsvn_client libsvn_ra_svn libsvn_subr mod_dav_svn svnserve

On Wed, Nov 26, 2003 at 03:45:55PM -0600, julianfoad@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_client/commit.c Wed Nov 26 15:45:54 2003
> @@ -662,7 +662,7 @@
> if ((err = import (path, new_entry,
> editor, edit_baton, nonrecursive, excludes, ctx, pool)))
> {
> - editor->abort_edit (edit_baton, pool);
> + svn_error_clear (editor->abort_edit (edit_baton, pool));
> return err;

I think a comment is warranted here, about throwing out the error. In this
case, it is desired. In some cases, it is simply because the developer
wasn't sure what to do (as below).

>...
> +++ trunk/subversion/libsvn_client/copy.c Wed Nov 26 15:45:54 2003
> @@ -718,7 +718,7 @@
> cleanup:
> /* Abort the commit if it is still in progress. */
> if (commit_in_progress)
> - editor->abort_edit (edit_baton, pool); /* ignore return value */
> + svn_error_clear (editor->abort_edit (edit_baton, pool));

You lost the comment here. I think it should stick around. The code is
specifically ignoring an error result. The question that arises is: was
that intended, or accidental?

>...
> +++ trunk/subversion/mod_dav_svn/update.c Wed Nov 26 15:45:54 2003
> @@ -1274,8 +1274,7 @@
> /* we require the `rev' attribute for this to make sense */
> if (! SVN_IS_VALID_REVNUM (rev))
> {
> - /* ### This removes the fs txn. todo: check error. */
> - svn_repos_abort_report(rbaton);
> + svn_error_clear(svn_repos_abort_report(rbaton));

Whoa. There is a comment there: "todo: check error". You tossed the error
*and* removed the todo that something ought to be done about it. The is
the exact case were we may have wanted to do something with that error, so
a comment explaining the decision/thinking is a Good Thing.

[ same for a few other changes in this file ]

>...
> +++ trunk/subversion/svnserve/serve.c Wed Nov 26 15:45:54 2003
> @@ -354,7 +354,7 @@
> else if (rb.err)
> {
> /* Some failure during the reporting or editing operations. */
> - editor->abort_edit(edit_baton, pool);
> + svn_error_clear(editor->abort_edit(edit_baton, pool));
> SVN_CMD_ERR(rb.err);
> }
> SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));

Again, a comment describing the rationale for ignoring the error would be
nice here.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org