Reviewing, and log messages [was: [patch #4610] Consolidated documentation patch]

[Julian Foad]

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