You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2003/12/01 10:43:26 UTC
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
Re: svn commit: rev 7857 - in trunk/subversion: libsvn_client
libsvn_ra_svn libsvn_subr mod_dav_svn svnserve
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2003-12-01 at 05:43, Greg Stein wrote:
> > - 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?
I agree with Phillip; the svn_error_clear() makes it quite clear that
ignoring the result is intentional. A comment is only necessary if it's
not obvious why the error is being ignored, and I think in this case
it's pretty obvious (we already have an error to return).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7857 - in trunk/subversion: libsvn_client
libsvn_ra_svn libsvn_subr mod_dav_svn svnserve
Posted by Philip Martin <ph...@codematters.co.uk>.
Greg Stein <gs...@lyra.org> writes:
>> +++ 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?
I don't think "ignore return value" adds anything--svn_error_clear
documents that adequately. It may be appropriate to add a comment
explaining why any error should be ignored, but this need for a
rationale applies equally to the original code.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 7857 - in trunk/subversion: libsvn_client libsvn_ra_svn libsvn_subr mod_dav_svn svnserve
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Dec 01, 2003 at 06:13:18PM +0000, Julian Foad wrote:
> Greg Stein wrote:
>...
> >>+++ 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.
Sure... sorry, that is kind of what I meant: saying to the reader, "hey...
we tossed this error; here is why". In this case, it would be something
like, "ignore the error, we already have one to return". Altho we may want
to compose the two errors so that we can return both.
>...
> > 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:
>...
> >>> In all these cases there is some error more important than the result
> >>> of svn_repos_abort_report(). So, perhaps just do:
There is your comment to add :-)
>...
> > 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.
I think we're in agreement. "remove the txn" isn't useful, but saying that
we don't want it, in favor of another, is probably handy.
Greg Hudson said that the svn_error_clear() makes it obvious that we're
tossing the error. Agreed. But it isn't clear why. Throwing out an error
is a very clear case of losing (potentially useful) information. I don't
think it should be taken too lightly.
And yes, in some cases, the context is clear enough. Reading the diff, it
isn't always clear whether there is enough context for the reader. But in
a few cases here, it seemed that we regressed a bit in understandability.
>...
> 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.
Oh, I have no disagreement with the change -- a definite win! The problem
is that if the reason isn't documented now, then it probably won't ever
be. Especially if ### markers aren't left in the code.
> 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.
No worries... Go ahead with the changes at your discretion.
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
Re: svn commit: rev 7857 - in trunk/subversion: libsvn_client libsvn_ra_svn
libsvn_subr mod_dav_svn svnserve
Posted by Julian Foad <ju...@btopenworld.com>.
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 <ju...@btopenworld.com> writes:
>
>> C. Michael Pilato wrote:
>>
>>> Julian Foad <ju...@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