[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Reviewing, and log messages [was: [patch #4610] Consolidated documentati
From: |
Julian Foad |
Subject: |
Reviewing, and log messages [was: [patch #4610] Consolidated documentation patch] |
Date: |
Sat, 19 Nov 2005 22:00:15 +0000 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 |
Charles Levert wrote:
A long essay deserves a long answer...
And it is a very interesting and well written one. Thanks. Not that we agree,
yet :-)
Let's rather start by affirming where we most
likely have a strong connect: dislike for cowboy
practices, and valuing individual concentration,
strong review, and good documentation (of code
and in manuals).
Right.
One can't really review changes without the log entry.
I would argue that really strong and stringent
review is in fact only possible by initially
putting aside the log entry, or any in-line
comment.
That's the ideal, and that's important for some critical changes. However,
that degree of care is not practical or worthwhile for most changes.
Or, that works well if, for each little part of the change, I look at the code
change first, see what I can make of it on its own, and then look at the log
message to put it into perspective and see if it does what it was intended to
do. The problem comes when I have to go through the _whole_ change without
knowing what it's attempting, and then wait hours or days to find out, and then
have to go through it all again.
(If I had performed a really strong bottom-up review I might have deduced the
purpose of each part and the whole, and composed the log message during review,
and then I could just compare it with your log message later. But that's just
not feasible except in trivial cases.)
[...]
The main reason to include the documentative
explanation is to put it up for review as
well to evaluate its ability to fulfill that
further-development-aid goal. [...]
The "documentative explanation" is like the high-level design specification for
the change. Normally (and I do lots of reviewing on the Subversion project) I
review the log message first, to check the intent and design of the change. If
much is wrong with the design, I just reply to that and don't even bother
looking at the code change because I know that it will have to change
substantially.
If I review the code first, and make what few comments I can about it, and then
see the log message later, at that stage I may say "Oh no! You can't do that.
I assumed there was a good reason for those changes in the code, but that's
not a good reason." And then we've both wasted time and effort.
-.\" grep man page
+.\" GNU grep man page
Without knowing why you did this, all I can say is,
"Fair enough; it looks
like a harmless comment that's of no consequence at all. I can't see that
causing any problems nor any benefit ...
One can have the same no-problem reaction if the
intent is fully described. The real question is:
what are the _actual_ effects and consequences,
regardless of spin/intent?
No, wait. My example reaction there was neutral, no feedback at all (not
exactly "no problem"). If we assume that I'm not prepared to review the change
with all possible care in a bottom-up manner to determine what consequences it
might have, then at least if you tell me the intention I will be able to check
that it matches the intention; otherwise I can't (or won't) check anything.
There's a big difference between how deeply I should or could review in theory,
and how deeply I am going to review in reality. Is the lack of a log message
going to make me do a deeper, bottom-up review? No, it's not. It could in
theory, but it's not going to in practice.
but I would still
like to know what the INTENTION of the change was so that I can check
whether the actual change implements the intention.
That I can agree with. But this is like an
answer-to-the-exercises section. One eventually
looks at it, but not right away. [...]
My "eventually" is after a few seconds to a few minutes for each small change
within the overall change.
To reiterate: there will be a ChangeLog entry.
Looking at my previous work, you can notice
that there always has been an invested one up
Yes, indeed: I have no complaint in that respect.
to now, and that it's been submitted for review
in all but very trivial to explain changes.
I also have acted on or answered late-submitted
after-commit reviews.
You're doing a great job.
To summarise the way I see it:
* It is possible to partially review a change without its log message.
* That review may have been able to start sooner (by the amount of time
required to write the log message).
* The writer will later post a log message (taking another
human-network-human round-trip).
* The reviewer will have to review the code again (in a different way).
* Any design or intention bugs are more likely to be found at the second
review. Those tend to require changes that obsolete the first review.
My feeling is that reviewing the same code twice is slower than doing it once,
despite looking for different things in each pass. The elapsed time could be
much greater. I am certain that finding bugs in design or intention after
reviewing the code is wasteful.
(p.s. I am a big fan of ISO-8601 date formats!)
- Julian
- Re: [patch #4610] Consolidated documentation patch, (continued)
- [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/18
- [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/18
- Re: [patch #4610] Consolidated documentation patch, Julian Foad, 2005/11/18
- Re: [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/18
- Re: [patch #4610] Consolidated documentation patch, Julian Foad, 2005/11/18
- Re: [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/18
- Re: [patch #4610] Consolidated documentation patch, Julian Foad, 2005/11/19
- Re: [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/19
- Re: [patch #4610] Consolidated documentation patch, Julian Foad, 2005/11/20
- Reviewing, and log messages [was: [patch #4610] Consolidated documentation patch],
Julian Foad <=
- [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/18
- [patch #4610] Consolidated documentation patch, Charles Levert, 2005/11/18