Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages

From: Emily Shaffer <emilyshaffer@google.com>
Date: Apr 19, 2022, 22:53 UTC
Message-ID: <Yl89hMGLN3DqIkJ7@google.com>
In-Reply-To: <220414.86lew7d7tb.gmgdl@evledraar.gmail.com>

On Thu, Apr 14, 2022 at 04:04:59PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 103 quoted lines

> 
> 
> On Wed, Apr 13 2022, Junio C Hamano wrote:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> >
> >> Emily Shaffer <emilyshaffer@google.com> writes:
> >>
> >>>> + - Log messages to explain your changes are as important as the
> >>>> +   changes themselves.  Clearly written code and in-code comments
> >>>> +   explain how the code works and what is assumed from the surrounding
> >>>> +   context.  The log messages explain what the changes wanted to
> >>>> +   achieve and why the changes were necessary (more on this in the
> >>>> +   accompanying SubmittingPatches document).
> >>>> +
> >>>
> >>> One thing not listed here, that I often hope to find from the commit
> >>> message (and don't), is "why we did it this way instead of <other way>".
> >>> I am not sure how to phrase it in this document, though. Maybe:
> >>>
> >>>   The log messages explain what the changes wanted to achieve, any
> >>>   decisions that were made between alternative approaches, and why the
> >>>   changes were necessary (more on this in blah blah)
> >>>
> >>> Or maybe "...whether any alternative approaches were considered..." fits
> >>> the form of the surrounding sentence better.
> >>
> >> Quite valid observation.
> >>
> >> Documentation/SubmittingPatches::meaningful-message makes a note on
> >> these points, and the above may want to be more aligned to them.
> >>
> >> Patches welcome, as these have long been merged to 'master/main'.
> >
> > Another thing.  If you (not Emily, but figuratively) haven't watched
> > Victoria's talk https://www.youtube.com/watch?v=4qLtKx9S9a8 on the
> > topic of clearly written commits, you should drop everything you are
> > doing and go watch it.
> >
> > And with what we learn from it, we may be able to rewrite this part
> > of the documentation much more clearly.
> 
> The slides for it are at
> https://vdye.github..io/2022/OS101-Writing-Commits.pdf (not in the video
> description, but at the very end of the video).
> 
> It's easy to nitpick/improve existing examples, so here goes :)
> 
> The main commit message example in that talk starts as just "Make error
> text more helpful", and ends with a better version as:
> 
> 	git-portable.sh: make error text more helpful
> 	
> 	The message “Not a valid command: <invalid command>” is
> 	intended to notify the user that their subcommand is invalid.
> 	However, when no subcommand is given, the "empty" subcommand
> 	results in the same message: "Not a valid command:". This does
> 	not clearly guide the user to the correct behavior, so print
> 	"Please specify a command" when no subcommand is specified.
> 
> For our CodingGuidelines I think it would be useful to have some version
> of "if you can explain something with prose or tests, prefer
> tests".
> 
> I.e. other things being equal I'd much prefer this version
> (pseudo-patch):
> 
> 	git-portable.sh: don't conflate invalid and non-existing command
> 
> 	 git-portable-test.sh | 2 +-
> 	 1 file changed, 1 insertion(+), 1 deletion(-)
> 	
> 	diff --git a/git-portable-test.sh b/git-portable-test.sh
> 	index c8bd464..e03f4a8 100644
> 	--- a/git-portable-test.sh
> 	+++ b/git-portable-test.sh
> 	@@ -5,7 +5,7 @@ test_expect_failure 'usage: invalid command' '
> 	 '
> 	 
> 	 test_expect_failure 'usage: no command' '
> 	-	test_expect_code_output 129 "Not a valid command: " ./gitportable.sh
> 	+	test_expect_code_output 129 "Please specify a command" ./gitportable.sh
> 	 '
> 	 
> 	 test_done
> 
> It ends up basically saying the same thing, but now we're saying it with
> a regression test (test_expect_code_output doesn't exist, but let's
> pretend it's test_expect_code + a test_cmp-alike).
> 
> What it does entirely omit is the "why".
> 
> Now I realize I'm nitpicking a slide shown at a conference, which by its
> nature needs to show a small pseudo-example, but I think this applies in
> general:
> 
> While "why" is a good rule of thumb I think it's just as important to
> know when not to include explanations and when to include one.
> 
> For cases where something is straightforward enough (as in this case,
> the RHS of ": " is clearly missing) I'd think omitting the explanation
> would be better, as we should also be concerned about the overall signal
> ratio.

Preface: I don't want to start a fight ;)

I think if you are in a position where you already will read every single patch that comes across the mailing list, including its diff, then you make a really valid point. I can read the negative line of your diff, infer the problem ("oh, there's nothing after :"), and examine the solution. Fine.

But I also don't think that most of us working on Git have the time to read every patch and its diff. I certainly don't. I'd agree that your patch's subject line is a little more informative than Victoria's, but past that, if the commit message is empty, I have no idea what problem you were trying to solve until I have scrolled through lines of context, diff lines, and finally arrive to the regression test (which ends up at the very end of the patch in Git, because of the way the codebase is organized). Whereas, with Victoria's proposed commit message, I can read the paragraph and decide whether I need to review, and from there decide whether the diff does what she says it should.

So as I'm deciding what to review, I definitely would prefer Victoria's commit message. Plus, like I mentioned, it gives the extra safeguard of allowing reviewers to check: does the patch actually do what the author meant for it to do? If we're never told what the author meant for it to do, then we are missing information needed for that part of the review.

Anyway, I haven't watched Victoria's talk yet, but I will do so soon :)

 - Emily