Greg Stein wrote:
> 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).
I agree that a comment would be good in situations like this.
>
>>...
>>+++ 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?
Keeping the comment that just said "ignore return value" would be redundant against an "svn_error_clear" call. The addition of a comment that indicates _why_ the error is being ignored would be a good thing.
>>...
>>+++ 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.
Such a comment would be a good thing but the pre-existing comment did not explain the decision and the "to do" has been done (or rather deemed unnecessary). C-M Pilato and Philip Martin reviewed this on 2003-11-26:
Philip Martin wrote:
> Julian Foad <julianfoad@btopenworld.com> writes:
>
>> C. Michael Pilato wrote:
>>
>>> Julian Foad <julianfoad@btopenworld.com> writes:
>>>
>>>> I also found the following, but we might as well evaluate the "###
>>>> ... todo" comments and do the right thing while we are thinking
>>>> about it, so what is the right thing? Just clear and ignore the
>>>> error, or not?
>>>>
>>>> ~/src/subversion> grep -C3 "svn_repos_abort_report" subversion/mod_dav_svn/update.c
>>>> if (! SVN_IS_VALID_REVNUM (rev))
>>>> {
>>>> /* ### This removes the fs txn. todo: check error. */
>>>> svn_repos_abort_report(rbaton);
>>>> serr = svn_error_create (SVN_ERR_XML_ATTRIB_NOT_FOUND,
>>>> NULL, "rev");
>>>> return dav_svn_convert_err(serr, HTTP_INTERNAL_SERVER_ERROR,
>>>
>>>
>>> In all these cases there is some error more important than the result
>>> of svn_repos_abort_report(). So, perhaps just do:
>>>
>>> svn_error_clear(svn_repos_abort_report(rbaton));
>>>
>>> It's not as if the user is going to be able to do a darned thing about
>>> a transaction that didn't get cleaned up from alllll the way across
>>> the network. -)
>>
>> Indeed. I'll do that.
>>
>> I'm not clear on the intent of the existing comment. Should I delete the comment, or leave in place a comment saying "Remove the fs txn." if removing the txn is the intent of the call, or "(This removes the fs txn.)" if that is just a notable side-effect of the call?
>
> I would not put a comment there. If you do add a comment then "Remove
> the fs txn" is not useful as that's the documented behavior of
> svn_repos_abort_report. A comment should tell me something special.
Please let me know if you disagree with this assessment.
Again, the addition of a useful comment would of course be 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.
Indeed it would.
This commit was about fixing the behaviour, not about documenting the reason for the behaviour. Of course it would have been OK to do both together in one commit, but I don't think it was wrong not to do so, and I don't think I removed any useful comments or clues.
I might have a look through the source code and try to add explanatory comments where we are ignoring errors. Maybe these places that I touched are the only places that lack such comments, but I have no reason to think so.
Anyway, thanks for the critique, and I hope we can work this out.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 1 19:10:17 2003