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
Received on Mon Dec 1 21:11:47 2003