[RFC] Contributor doc: more on the proposed log message

[RFC] Contributor doc: more on the proposed log message

[Junio C Hamano]

I have been thinking about making it more clear why we care about
the log message, and noticed that we have CodingGuidelines and
SubmittingPatches, both are specifically targetted for the
contributors of THIS project (not to users contributing to a project
that happens to use Git).

I think the first thing to fix is that we have the "describe your
changes well" section in the latter, as if it is not part of the
code that is covered by CodingGuidelines.  You formulate the thought
on how to explain/sell your changes to others, and you sift the text
you add to help fellow developers into the ones you leave in in-code
comments and in the proposed log message, while you code.  I am
tempted to propose moving the part about proposed log message from
SubmittingPatches to CodingGuidelines for this reason.

Independent of the above, here is a small update I would add to
clarify the project convention on the log message.

Thoughts?

---
 Documentation/SubmittingPatches | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches
index 92b80d94d4..11d0c85988 100644
--- c/Documentation/SubmittingPatches
+++ w/Documentation/SubmittingPatches
@@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:
 
 . alternate solutions considered but discarded, if any.
 
+[[present-tense]]
+The problem statement that describes the status quo is written in the
+present tense.  Write "The code does X when it is given input Y",
+instead of "The code used to do Y when given input X".  You do not
+have to say "Currently"---the status quo in the problem statement is
+about the code _without_ your change, by project convention.
+
 [[imperative-mood]]
 Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
 instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy

Re: [RFC] Contributor doc: more on the proposed log message

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1275 bytes --]

On 2022-01-23 at 20:37:18, Junio C Hamano wrote:
> I have been thinking about making it more clear why we care about
> the log message, and noticed that we have CodingGuidelines and
> SubmittingPatches, both are specifically targetted for the
> contributors of THIS project (not to users contributing to a project
> that happens to use Git).
> 
> I think the first thing to fix is that we have the "describe your
> changes well" section in the latter, as if it is not part of the
> code that is covered by CodingGuidelines.  You formulate the thought
> on how to explain/sell your changes to others, and you sift the text
> you add to help fellow developers into the ones you leave in in-code
> comments and in the proposed log message, while you code.  I am
> tempted to propose moving the part about proposed log message from
> SubmittingPatches to CodingGuidelines for this reason.
> 
> Independent of the above, here is a small update I would add to
> clarify the project convention on the log message.
> 
> Thoughts?

I think this is a good idea, since it differs from the way people
usually discuss things outside of literature, and it's very common for
this to trip people up.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [RFC] Contributor doc: more on the proposed log message

[Kaartic Sivaraam]

On 24/01/22 3:02 am, brian m. carlson wrote:
> On 2022-01-23 at 20:37:18, Junio C Hamano wrote:
>> I have been thinking about making it more clear why we care about
>> the log message, and noticed that we have CodingGuidelines and
>> SubmittingPatches, both are specifically targetted for the
>> contributors of THIS project (not to users contributing to a project
>> that happens to use Git).
>>
>> I think the first thing to fix is that we have the "describe your
>> changes well" section in the latter, as if it is not part of the
>> code that is covered by CodingGuidelines.  You formulate the thought
>> on how to explain/sell your changes to others, and you sift the text
>> you add to help fellow developers into the ones you leave in in-code
>> comments and in the proposed log message, while you code.  I am
>> tempted to propose moving the part about proposed log message from
>> SubmittingPatches to CodingGuidelines for this reason.
>>
>> Independent of the above, here is a small update I would add to
>> clarify the project convention on the log message.
>>
>> Thoughts?
> 
> I think this is a good idea, since it differs from the way people
> usually discuss things outside of literature, and it's very common for
> this to trip people up.
> 

Precisely. I often get tripped up due to this whenever I write commit
messages in general. So, good to have this clarified through a guideline.

-- 
Sivaraam

[PATCH v2 0/3] contributor doc update around log messages

[Junio C Hamano]

Here is an updated "our 'present' is before the patch is applied in
the log messages" patch, together with a couple of changes to
explain _why_ we care about your log messages.

Hopefully by knowing whom they are trying to help with their log
messages, our contributors will be able to write a more useful
proposed log messages when they send their patches to the list.

Junio C Hamano (3):
  SubmittingPatches: write problem statement in the log in the present
    tense
  CodingGuidelines: hint why we value clearly written log messages
  SubmittingPatches: explain why we care about log messages

 Documentation/CodingGuidelines  |  7 +++++++
 Documentation/SubmittingPatches | 35 +++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

-- 
2.35.0-155-g0eb5153edc

[PATCH v2 1/3] SubmittingPatches: write problem statement in the log in the present tense

[Junio C Hamano]

We give a guidance for proposed log message to write problem
statement first, followed by the reasoning behind, and recipe for,
the solution.  Clarify that we describe the situation _before_ the
proposed patch is applied in the present tense (not in the past
tense e.g. "we used to do X, but thanks to this commit we now do Y")
for consistency.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 92b80d94d4..7225a0fb52 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:
 
 . alternate solutions considered but discarded, if any.
 
+[[present-tense]]
+The problem statement that describes the status quo is written in the
+present tense.  Write "The code does X when it is given input Y",
+instead of "The code used to do Y when given input X".  You do not
+have to say "Currently"---the status quo in the problem statement is
+about the code _without_ your change, by project convention.
+
 [[imperative-mood]]
 Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
 instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
-- 
2.35.0-155-g0eb5153edc

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

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 0e27b5395d..5f40595f6e 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -26,6 +26,13 @@ code.  For Git in general, a few rough rules are:
    go and fix it up."
    Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
 
+ - Log messages to explain your changes are as much 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).
+
 Make your code readable and sensible, and don't try to be clever.
 
 As for more concrete guidelines, just imitate the existing code
-- 
2.35.0-155-g0eb5153edc

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

[Eric Sunshine]

On Thu, Jan 27, 2022 at 1:32 AM Junio C Hamano <gitster@pobox.com> wrote:
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> @@ -26,6 +26,13 @@ code.  For Git in general, a few rough rules are:
> + - Log messages to explain your changes are as much important as the

Nit: Dropping "much" makes this sound a bit better grammatically:

"Log messages which 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).

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Nit: Dropping "much" makes this sound a bit better grammatically:

Thanks.

[PATCH v2 3/3] SubmittingPatches: explain why we care about log messages

[Junio C Hamano]

Extend the "describe your changes well" section to cover whom we are
trying to help by doing so in the first place.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 7225a0fb52..9c5977fac3 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -110,6 +110,34 @@ run `git diff --check` on your changes before you commit.
 [[describe-changes]]
 === Describe your changes well.
 
+The log message that explains your changes is as much important as the
+changes themselves.  Your code may be clearly written with in-code
+comment to sufficiently explain how it works with the surrounding
+code, but those who need to fix or enhance your code in the future
+will need to know _why_ your code does what it does, for a few reasons:
+
+. Your code may be doing something differently from what you wanted it
+  to do.  Writing down what you actually wanted to achieve will help
+  them fix your code and make it do what it should have been doing
+  (also, you often discover your own bug while writing your log
+  message yourself).
+
+. Your code may be doing things that were only necessary for your
+  immediate needs (e.g. "do X to directories" without implementing or
+  even designing what is to be done on files).  Writing down why you
+  excluded what the code does not do will help guide future developers
+  (e.g. writing down "we do X to directories, because directories have
+  characteristic Y" would help them infer "oh, files also have the
+  same characteristic Y, so perhaps doing X to them would also make
+  sense?".  Or "we don't do the same X to files, because ..." will
+  help them decide if the reasoning is sound (in which case they do
+  not waste time extending your code to cover files), or reason
+  differently (in which case they explain why they extend your code to
+  cover files, too).
+
+The goal of your log message is to convey the _why_ behind your
+change to help them.
+
 The first line of the commit message should be a short description (50
 characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
 and should skip the full stop.  It is also conventional in most cases to
-- 
2.35.0-155-g0eb5153edc

[PATCH v3 0/3] contributor doc update around log messages

[Junio C Hamano]

We already explain _how_ to write log messages, but did not tell
readers why we care.  Add a few paragraphs to SubmittingPatches
and CodingGuidelines to do so.

Junio C Hamano (3):
  SubmittingPatches: write problem statement in the log in the present
    tense
  CodingGuidelines: hint why we value clearly written log messages
  SubmittingPatches: explain why we care about log messages

 Documentation/CodingGuidelines  |  7 +++++++
 Documentation/SubmittingPatches | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

Range-diff against v2:
-:  ---------- > 1:  b86a706bfd SubmittingPatches: write problem statement in the log in the present tense
1:  35e39deb7d ! 2:  a65df87939 CodingGuidelines: hint why we value clearly written log messages
    @@ Documentation/CodingGuidelines: code.  For Git in general, a few rough rules are
         go and fix it up."
         Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
      
    -+ - Log messages to explain your changes are as much important as the
    ++ - 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
2:  fb54bcfcda < -:  ---------- SubmittingPatches: explain why we care about log messages
-:  ---------- > 3:  77d918fb98 SubmittingPatches: explain why we care about log messages
-- 
2.35.0-177-g7d269f5170

Re: [PATCH v3 0/3] contributor doc update around log messages

[Emily Shaffer]

On Thu, Jan 27, 2022 at 11:02:56AM -0800, Junio C Hamano wrote:
> 
> We already explain _how_ to write log messages, but did not tell
> readers why we care.  Add a few paragraphs to SubmittingPatches
> and CodingGuidelines to do so.

Bah, after I reviewed this I went and checked 'next' to see you already
merged. Sorry for the noise. :X

> 
> Junio C Hamano (3):
>   SubmittingPatches: write problem statement in the log in the present
>     tense
>   CodingGuidelines: hint why we value clearly written log messages
>   SubmittingPatches: explain why we care about log messages
> 
>  Documentation/CodingGuidelines  |  7 +++++++
>  Documentation/SubmittingPatches | 36 +++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> Range-diff against v2:
> -:  ---------- > 1:  b86a706bfd SubmittingPatches: write problem statement in the log in the present tense
> 1:  35e39deb7d ! 2:  a65df87939 CodingGuidelines: hint why we value clearly written log messages
>     @@ Documentation/CodingGuidelines: code.  For Git in general, a few rough rules are
>          go and fix it up."
>          Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
>       
>     -+ - Log messages to explain your changes are as much important as the
>     ++ - 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
> 2:  fb54bcfcda < -:  ---------- SubmittingPatches: explain why we care about log messages
> -:  ---------- > 3:  77d918fb98 SubmittingPatches: explain why we care about log messages
> -- 
> 2.35.0-177-g7d269f5170
> 

[PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense

[Junio C Hamano]

We give a guidance for proposed log message to write problem
statement first, followed by the reasoning behind, and recipe for,
the solution.  Clarify that we describe the situation _before_ the
proposed patch is applied in the present tense (not in the past
tense e.g. "we used to do X, but thanks to this commit we now do Y")
for consistency.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 92b80d94d4..7225a0fb52 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:
 
 . alternate solutions considered but discarded, if any.
 
+[[present-tense]]
+The problem statement that describes the status quo is written in the
+present tense.  Write "The code does X when it is given input Y",
+instead of "The code used to do Y when given input X".  You do not
+have to say "Currently"---the status quo in the problem statement is
+about the code _without_ your change, by project convention.
+
 [[imperative-mood]]
 Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
 instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
-- 
2.35.0-177-g7d269f5170

Re: [PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense

[Emily Shaffer]

On Thu, Jan 27, 2022 at 11:02:57AM -0800, Junio C Hamano wrote:
> 
> We give a guidance for proposed log message to write problem
> statement first, followed by the reasoning behind, and recipe for,
> the solution.  Clarify that we describe the situation _before_ the
> proposed patch is applied in the present tense (not in the past
> tense e.g. "we used to do X, but thanks to this commit we now do Y")
> for consistency.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/SubmittingPatches | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 92b80d94d4..7225a0fb52 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:
>  
>  . alternate solutions considered but discarded, if any.
>  
> +[[present-tense]]
> +The problem statement that describes the status quo is written in the
> +present tense.  Write "The code does X when it is given input Y",
> +instead of "The code used to do Y when given input X".  You do not
> +have to say "Currently"---the status quo in the problem statement is
> +about the code _without_ your change, by project convention.
> +

The writing sample helps a lot here. "Present tense" is a poor example,
but I think it's common for native language speakers to know less about
specific grammar words (like "imperative mood" below), even though they
can often instinctively tell what form you're describing when seeing a
sample.

As for the norm itself, I think it is a good one, and I've seen it
pointed out in code reviews frequently. Thanks.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

>  [[imperative-mood]]
>  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
>  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
> -- 
> 2.35.0-177-g7d269f5170
> 

Re: [PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> As for the norm itself, I think it is a good one, and I've seen it
> pointed out in code reviews frequently. Thanks.
>
> Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

Thanks.

Was this meant to be sent long after the topic was merged to
'master' at 83760938 (Merge branch 'jc/doc-log-messages',
2022-02-11)?

Re: [PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense

[Emily Shaffer]

On Thu, Mar 03, 2022 at 04:23:45PM -0800, Junio C Hamano wrote:
> 
> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > As for the norm itself, I think it is a good one, and I've seen it
> > pointed out in code reviews frequently. Thanks.
> >
> > Reviewed-by: Emily Shaffer <emilyshaffer@google.com>
> 
> Thanks.
> 
> Was this meant to be sent long after the topic was merged to
> 'master' at 83760938 (Merge branch 'jc/doc-log-messages',
> 2022-02-11)?

As mentioned in
https://lore.kernel.org/git/YiFZicz69mDyFzXB%40google.com - no, it
wasn't. In fact, in conversation at $dayjob someone said "whatever
happened to Junio planning to write updates to SubmittingPatches?" and
someone else said "it didn't really get any review" - so I said "okay, I
will do review" instead of checking whether you merged anyway. So mostly
useless noise :)

 - Emily

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

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 0e27b5395d..c37c43186e 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -26,6 +26,13 @@ code.  For Git in general, a few rough rules are:
    go and fix it up."
    Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
 
+ - 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).
+
 Make your code readable and sensible, and don't try to be clever.
 
 As for more concrete guidelines, just imitate the existing code
-- 
2.35.0-177-g7d269f5170

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

[Emily Shaffer]

On Thu, Jan 27, 2022 at 11:02:58AM -0800, Junio C Hamano wrote:
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/CodingGuidelines | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 0e27b5395d..c37c43186e 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -26,6 +26,13 @@ code.  For Git in general, a few rough rules are:
>     go and fix it up."
>     Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
>  
> + - 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.

 - Emily

>  Make your code readable and sensible, and don't try to be clever.
>  
>  As for more concrete guidelines, just imitate the existing code
> -- 
> 2.35.0-177-g7d269f5170
> 

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

[Junio C Hamano]

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'.

Thanks.

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

[Junio C Hamano]

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.

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

[Ævar Arnfjörð Bjarmason]

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.

(Now, if anyone glances at my own commit messages they'll see I'm
thoroughly in "throwing rocks from a glass house" territory here :) I'm
not saying I'm consistency practicing what I'm preaching).

But just like comments there's no right answer, when one person thinks
an explanation is different from another.

But it is unambiguously the case that we can often replace prose with
tests, and in those cases we should almost always prefer that.

It's also the case that even if everyone agrees that a "why" is needed
there's multiple ways to store that information. One is via commit
messages, another would e.g. be that same commit updating some shared
guidelines about goals/examples of CLI usage.

So in this case, if a Documentation/CodingGuidelines had clear examples
of preferred usage, we could just point briefly point to that as
rationale.

While git's commit messages are excellent, I think that's one area where
we really need improvement. It's rare to dig into some old code where no
rationale can be found for it, either in the commit itself, or in the
preceding ML discussion.

But it's unfortunately (at least in my experience) more often than not
the case that you really do need to consult those commit messages or ML
archives, even for things that have come up a *lot* of times, they were
just never documented in-tree.

There's all sorts of reasons for that which are not the result of any
person doing anything wrong, but I do think it's something we could and
should focus more on as a project.

The barriers of entry for adding documentation or adjusting existing
documentation are much higher than adding a one-off explanation in a
commit message.

Partially (and probably mostly) that's a really good thing, but I can't
help but wonder if we're getting that balance right given the (in my
subjective experience) end result of us often lacking good docs, while
we're not lacking if one searches for replacements for those docs in
commit messages or the ML archive.

One more thing that I think is not explicitly covered (I skimmed the
slides, but haven't gone throug the full back yet): Minimizing diffs.

E.g. the talk shows 287fd17e3a1 (sparse-index: prevent repo root from
becoming sparse, 2022-03-01) as an example, which has this hunk:
	
	diff --git a/dir.c b/dir.c
	index d91295f2bcd..a136377eb49 100644
	--- a/dir.c
	+++ b/dir.c
	@@ -1463,10 +1463,11 @@ static int path_in_sparse_checkout_1(const char *path,
	 	const char *end, *slash;
	 
	 	/*
	-	 * We default to accepting a path if there are no patterns or
	-	 * they are of the wrong type.
	+	 * We default to accepting a path if the path is empty, there are no
	+	 * patterns, or the patterns are of the wrong type.
	 	 */
	-	if (init_sparse_checkout_patterns(istate) ||
	+	if (!*path ||
	+	    init_sparse_checkout_patterns(istate) ||
	 	    (require_cone_mode &&
	 	     !istate->sparse_checkout_patterns->use_cone_patterns))
	 		return 1;

I think this is a worthwhile thing to consider as a replacement:
	
	diff --git a/dir.c b/dir.c
	index d91295f2bcd..93a2320ae57 100644
	--- a/dir.c
	+++ b/dir.c
	@@ -1466,7 +1466,8 @@ static int path_in_sparse_checkout_1(const char *path,
	 	 * We default to accepting a path if there are no patterns or
	 	 * they are of the wrong type.
	 	 */
	-	if (init_sparse_checkout_patterns(istate) ||
	+	if (!*path || /* we consider an empty pattern to be no pattern */
	+	    init_sparse_checkout_patterns(istate) ||
	 	    (require_cone_mode &&
	 	     !istate->sparse_checkout_patterns->use_cone_patterns))
	 		return 1;

I.e. trying to optimize for smaller diffs whenever possible. It this
case the word-diff for the original is:

        /*
         * We default to accepting a path if {+the path is empty,+} there are no
         {+*+} patterns{+,+} or [-* they-]{+the patterns+} are of the wrong type.
         */

Now, obviously another small isolated example that's not worth
nitpicking in itself, but just serves to make a larger point. It's clear
why the rephrasing was done in that case, because the patch adds the
"!*path" check, so it makes sense a-priory to have the comment reflect
that.

But one thing where advice about "narrative structure" and good prose
tends to break down when it comes to software development is that we're
much more focused on reviews of incremental additions than many other
fields, where it tends to be more about the final product.

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

[Emily Shaffer]

On Thu, Apr 14, 2022 at 04:04:59PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> 
> 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

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> On Thu, Apr 14, 2022 at 04:04:59PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>  ...
>> 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 was going to ignore this part as it is merely showing personal
preference, but I guess I need to weigh in here.

Demonstrating what you meant to say in the log message with tests is
fine, but that should be in addition to prose, explaining how the
scenario is set up and what the user wanted to do, before showing
that a command is giving an outcome that does not help what the user
wanted to do.

IOW, in our CodingGUidelines, we should have "tests can be a good
way to augument what you want to say, but explain it well to those
who are not so familiar with the area."  You do not necessarily have
to explain it to 5 year old, but the audience should not have to be
whoever writes the patch themself to understand it.

> 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.

I have nothing to add here.

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

I do not necessarily agree with the presentation order in a proposed
log message she suggests, but overall, it's good investment of your
time.  Highly recommended.

[PATCH v3 3/3] SubmittingPatches: explain why we care about log messages

[Junio C Hamano]

Extend the "describe your changes well" section to cover whom we are
trying to help by doing so in the first place.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/SubmittingPatches | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 7225a0fb52..a6121d1d42 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -110,6 +110,35 @@ run `git diff --check` on your changes before you commit.
 [[describe-changes]]
 === Describe your changes well.
 
+The log message that explains your changes is just as important as the
+changes themselves.  Your code may be clearly written with in-code
+comment to sufficiently explain how it works with the surrounding
+code, but those who need to fix or enhance your code in the future
+will need to know _why_ your code does what it does, for a few
+reasons:
+
+. Your code may be doing something differently from what you wanted it
+  to do.  Writing down what you actually wanted to achieve will help
+  them fix your code and make it do what it should have been doing
+  (also, you often discover your own bugs yourself, while writing the
+  log message to summarize the thought behind it).
+
+. Your code may be doing things that were only necessary for your
+  immediate needs (e.g. "do X to directories" without implementing or
+  even designing what is to be done on files).  Writing down why you
+  excluded what the code does not do will help guide future developers.
+  Writing down "we do X to directories, because directories have
+  characteristic Y" would help them infer "oh, files also have the same
+  characteristic Y, so perhaps doing X to them would also make sense?".
+  Saying "we don't do the same X to files, because ..." will help them
+  decide if the reasoning is sound (in which case they do not waste
+  time extending your code to cover files), or reason differently (in
+  which case, they can explain why they extend your code to cover
+  files, too).
+
+The goal of your log message is to convey the _why_ behind your
+change to help future developers.
+
 The first line of the commit message should be a short description (50
 characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
 and should skip the full stop.  It is also conventional in most cases to
-- 
2.35.0-177-g7d269f5170

Re: [PATCH v3 3/3] SubmittingPatches: explain why we care about log messages

[Emily Shaffer]

On Thu, Jan 27, 2022 at 11:02:59AM -0800, Junio C Hamano wrote:
> 
> Extend the "describe your changes well" section to cover whom we are
> trying to help by doing so in the first place.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/SubmittingPatches | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 7225a0fb52..a6121d1d42 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -110,6 +110,35 @@ run `git diff --check` on your changes before you commit.
>  [[describe-changes]]
>  === Describe your changes well.
>  
> +The log message that explains your changes is just as important as the
> +changes themselves.  Your code may be clearly written with in-code
> +comment to sufficiently explain how it works with the surrounding
> +code, but those who need to fix or enhance your code in the future
> +will need to know _why_ your code does what it does, for a few
> +reasons:
> +
> +. Your code may be doing something differently from what you wanted it
> +  to do.  Writing down what you actually wanted to achieve will help
> +  them fix your code and make it do what it should have been doing
> +  (also, you often discover your own bugs yourself, while writing the
> +  log message to summarize the thought behind it).
> +
> +. Your code may be doing things that were only necessary for your
> +  immediate needs (e.g. "do X to directories" without implementing or
> +  even designing what is to be done on files).  Writing down why you
> +  excluded what the code does not do will help guide future developers.
> +  Writing down "we do X to directories, because directories have
> +  characteristic Y" would help them infer "oh, files also have the same
> +  characteristic Y, so perhaps doing X to them would also make sense?".
> +  Saying "we don't do the same X to files, because ..." will help them
> +  decide if the reasoning is sound (in which case they do not waste
> +  time extending your code to cover files), or reason differently (in
> +  which case, they can explain why they extend your code to cover
> +  files, too).
> +
> +The goal of your log message is to convey the _why_ behind your
> +change to help future developers.
> +

This is pretty compelling. Is it clear enough why we care about this in
the commit message, as opposed to in the patch description (cover letter
or post-"---" blurb)? Is it too obvious to explicitly mention that the
commit message is the first thing we try to make sense of during a 'git
blame' or 'git bisect'?

Either way, I think this is an improvement on the doc as it was before.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>

>  The first line of the commit message should be a short description (50
>  characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),
>  and should skip the full stop.  It is also conventional in most cases to
> -- 
> 2.35.0-177-g7d269f5170
> 

Re: [PATCH v3 3/3] SubmittingPatches: explain why we care about log messages

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

>> +The goal of your log message is to convey the _why_ behind your
>> +change to help future developers.
>> +
>
> This is pretty compelling. Is it clear enough why we care about this in
> the commit message, as opposed to in the patch description (cover letter
> or post-"---" blurb)? Is it too obvious to explicitly mention that the
> commit message is the first thing we try to make sense of during a 'git
> blame' or 'git bisect'?

Having to say "this may be better in the in-code comment rather than
the log message" to some patch recently (I do not remember), I tend
to agree that some guidance would help people decide between the two
(or writing both).

Again, patches welcome ;-)

log messages > comments (was: [PATCH v3 3/3] SubmittingPatches: explain why we care about log messages)

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 03 2022, Junio C Hamano wrote:

> Emily Shaffer <emilyshaffer@google.com> writes:
>
>>> +The goal of your log message is to convey the _why_ behind your
>>> +change to help future developers.
>>> +
>>
>> This is pretty compelling. Is it clear enough why we care about this in
>> the commit message, as opposed to in the patch description (cover letter
>> or post-"---" blurb)? Is it too obvious to explicitly mention that the
>> commit message is the first thing we try to make sense of during a 'git
>> blame' or 'git bisect'?
>
> Again, patches welcome ;-)

I think for something that's a stylistic preference I'd see why Emily
would try to see how you feel about it first.

I.e. in this project it's ultimately up to you to decide on those
things, so for coding guidelines you've just updated I'd probably do the
same.

Don't take that as a complaint on the end result b.t.w., I think the
overall excellent state of the codebase in this project speaks for
itself on that front.

I just see why other contributors would be gun shy about pulling the
trigger on a patch submission on this particular front :)

> Having to say "this may be better in the in-code comment rather than
> the log message" to some patch recently (I do not remember), I tend
> to agree that some guidance would help people decide between the two
> (or writing both).

[A somewhat sneaky $subject change :)]

I think you're referring to this comment on (some admittedly tricky)
code I wrote[1].

First, given the above I'll adjust that to your preferences on a
re-submission. So don't take this as some argument on *that* specific
point. I'll add a comment in a re-roll.

But on reflection I still wouldn't put a comment on that code if it were
purely up to me. Why?

First, I think all programmers go through a phase of learning where they
feel more compelled to comment on the "how" v.s. "why" early on.

At the most extremes beginner programmers explaining how say a common
standard library function works in code that's relatively
straightforward.

But that's really not the case in [1], that code really is doing
something odd and worth explaining. So why not add a comment?

Because access to ubiquitous and *local* source control from git changed
a lot of people's habits on this front, including mine. For this code I
*would* definitely add a comment there if it was the pre-DVCS[2] days.

But I'd say that today my criteria for adding a comment is closer to:

    Is this so essential to note for the understanding of the rest of
    this code that nobody who's skimming past this line or reading this
    part *wouldn't* want this information?

    Or rather, they might not absolutely want to know, but it might be
    useful, *and* there's nothing odd about the pattern itself that
    makes you go "hrm?" enough to run "git blame/log" on it.

In this case it's clearly pretty weird that we run the exact "test-tool
regex" command twice. But I think that "hrm?" applies. To elaborate:

 A. It's setting up a self-contained prereq, so it's not essential
    to understand that implementation detail to read the rest of the
    code.

 B. Anyone who does want to see why that odd case is the way it is can
    run some version "git blame" or say:

        git log -p -L14,21:t/t7812-grep-icase-non-ascii.sh

 C. Each comment you add, even within a function or other scope dilutes
    the value of other more important comments. It trains people not to
    read them, as they're probably not that important.

 D. Comments that are "frozen in time" by adding them to the code
    itself are almost always in danger of drifting in accuracy from the
    rest of the implementation and its assumptions.

    Even in well-curated codebases like git.git they're *much more*
    likely to drift away from the "ground truth" than code is.

 E. Even if "D" isn't true, commit messages (in this case my [3]) are
    almost always at an advantage over comments in that they accompany
    a change to the pre-image.

    So e.g. that commit message doesn't need to waste time explaining
    what pattern we'd prefer not to have here instead of the post-image,
    you get that context for free.

Due to a combination of "D" and "E" I almost never read comments in
their current context, unless it's painfully obvious that they *must*
still apply to the current code. I'll usually run some variant of "git
blame" or say:

    git log -p -L:relevant_function:file

And then see how the code looked when that comment was added, and page
through what's changed since then.

1. https://lore.kernel.org/git/xmqqsfryah42.fsf@gitster.g/
2. To those who'd nitpick DVCS v.s. VCS: Yes nothing changed in theory from CVS/SVN
   etc. on this front, but in practice it did.
   
   Access to version control didn't tend to be ubiquitous, and even if it was
   accessible many people browsing or contributing to your code would probably
   do so via a downloaded tarball than trying to get CVS or whatever to work.
   So the "D" in DVCS really did change this.
3. https://lore.kernel.org/git/patch-12.15-f3cc5bc7eb9-20220302T171755Z-avarab@gmail.com/

Re: log messages > comments

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Mar 03 2022, Junio C Hamano wrote:
>
>> Emily Shaffer <emilyshaffer@google.com> writes:
>>
>>>> +The goal of your log message is to convey the _why_ behind your
>>>> +change to help future developers.
>>>> +
>>>
>>> This is pretty compelling. Is it clear enough why we care about this in
>>> the commit message, as opposed to in the patch description (cover letter
>>> or post-"---" blurb)? Is it too obvious to explicitly mention that the
>>> commit message is the first thing we try to make sense of during a 'git
>>> blame' or 'git bisect'?
>>
>> Again, patches welcome ;-)
>
> I think for something that's a stylistic preference I'd see why Emily
> would try to see how you feel about it first.

I do not think there is a need to write down stylistic preference.
I may show my preference during my reviews, of course, but I won't
take preference-only things as a blocker.

I also do not think things like "'We used to do X here but we do Y
because ...' does not belong to in-code comment, but to log message"
is "stylistic preference", and if people are unclear about, I agree
that we should spell it out.

An example, I can think of offhand, of what should be in comment,
whether we also write in the log, is "We do X here because that
other code expects us to", when it is tricky to figure out by
reading the code by itself without going back to "git blame".

"git blame" certainly can be used to figure out which commit touched
the line that does X (which is hard to figure out why), and the log
message can refer to "that other code expects us to", but that is an
extra operation.

Also, when we really need to figure out, it is wonderful that we can
ask "blame" to give us the commit, and can look at "that other code"
in the same commit by checking it out to the working tree,
especially when "that other code" may have drifted and the original
reasoning no longer applies (iow, what we find out from "git blame"
may become stale, and it will stay stale forever because we cannot
rewrite the history that old).

Now, it is certainly not black/white decision to say what is and
what is not tricky to figure out in the code.  We shouldn't be
commenting obvious things.  Two yardsticks I use are

 (1) if reviewers raise questions during a review, it may indicate
     that it is worth commenting.

 (2) if an earlier round of the same series had a bug around the
     area, it may indicate that the fixed code is worth commenting.

but the way I use them is more to say "I found this uncommented code
somewhat tricky---but nobody asked a question and it has stayed the
same from the initial round, so it may be clear enough for other
people, and after all I managed to figure out myself, so it probably
is OK to leave it uncommented".

Re: log messages > comments

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I also do not think things like "'We used to do X here but we do Y
> because ...' does not belong to in-code comment, but to log message"

Sorry for too many nagations.  "I also do not" -> "I also do".