{"thread":{"id":"57291","subject":"[RFC] Contributor doc: more on the proposed log message","startedAt":"2022-01-23T20:37:23Z","lastAt":"2022-04-20T08:23:34Z","messageCount":28,"participants":["Junio C Hamano","brian m. carlson","Kaartic Sivaraam","Eric Sunshine","Emily Shaffer","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"446727","messageId":"xmqqilua89z5.fsf@gitster.g","threadId":"57291","inReplyTo":null,"subject":"[RFC] Contributor doc: more on the proposed log message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-23T20:37:18Z","receivedAt":"2022-01-23T20:37:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I have been thinking about making it more clear why we care about\nthe log message, and noticed that we have CodingGuidelines and\nSubmittingPatches, both are specifically targetted for the\ncontributors of THIS project (not to users contributing to a project\nthat happens to use Git).\n\nI think the first thing to fix is that we have the \"describe your\nchanges well\" section in the latter, as if it is not part of the\ncode that is covered by CodingGuidelines. You formulate the thought\non how to explain/sell your changes to others, and you sift the text\nyou add to help fellow developers into the ones you leave in in-code\ncomments and in the proposed log message, while you code. I am\ntempted to propose moving the part about proposed log message from\nSubmittingPatches to CodingGuidelines for this reason.\n\nIndependent of the above, here is a small update I would add to\nclarify the project convention on the log message.\n\nThoughts?\n\n---\n Documentation/SubmittingPatches | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches\nindex 92b80d94d4..11d0c85988 100644\n--- c/Documentation/SubmittingPatches\n+++ w/Documentation/SubmittingPatches\n@@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:\n \n . alternate solutions considered but discarded, if any.\n \n+[[present-tense]]\n+The problem statement that describes the status quo is written in the\n+present tense. Write \"The code does X when it is given input Y\",\n+instead of \"The code used to do Y when given input X\". You do not\n+have to say \"Currently\"---the status quo in the problem statement is\n+about the code _without_ your change, by project convention.\n+\n [[imperative-mood]]\n Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n instead of \"[This patch] makes xyzzy do frotz\" or \"[I] changed xyzzy\n"},{"id":"446729","messageId":"Ye3JVEm77d7zMU5L@camp.crustytoothpaste.net","threadId":"57291","inReplyTo":"xmqqilua89z5.fsf@gitster.g","subject":"Re: [RFC] Contributor doc: more on the proposed log message","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2022-01-23T21:32:04Z","receivedAt":"2022-01-23T21:32:08Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2022-01-23 at 20:37:18, Junio C Hamano wrote:\n> I have been thinking about making it more clear why we care about\n> the log message, and noticed that we have CodingGuidelines and\n> SubmittingPatches, both are specifically targetted for the\n> contributors of THIS project (not to users contributing to a project\n> that happens to use Git).\n> \n> I think the first thing to fix is that we have the \"describe your\n> changes well\" section in the latter, as if it is not part of the\n> code that is covered by CodingGuidelines. You formulate the thought\n> on how to explain/sell your changes to others, and you sift the text\n> you add to help fellow developers into the ones you leave in in-code\n> comments and in the proposed log message, while you code. I am\n> tempted to propose moving the part about proposed log message from\n> SubmittingPatches to CodingGuidelines for this reason.\n> \n> Independent of the above, here is a small update I would add to\n> clarify the project convention on the log message.\n> \n> Thoughts?\n\nI think this is a good idea, since it differs from the way people\nusually discuss things outside of literature, and it's very common for\nthis to trip people up.\n-- \nbrian m. carlson (he/him or they/them)\nToronto, Ontario, CA\n"},{"id":"446933","messageId":"700d8602-1efa-fe70-e9ff-5f5b7eb1a43c@gmail.com","threadId":"57291","inReplyTo":"Ye3JVEm77d7zMU5L@camp.crustytoothpaste.net","subject":"Re: [RFC] Contributor doc: more on the proposed log message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2022-01-26T11:07:27Z","receivedAt":"2022-01-26T11:07:33Z","isPatch":false,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 24/01/22 3:02 am, brian m. carlson wrote:\n> On 2022-01-23 at 20:37:18, Junio C Hamano wrote:\n>> I have been thinking about making it more clear why we care about\n>> the log message, and noticed that we have CodingGuidelines and\n>> SubmittingPatches, both are specifically targetted for the\n>> contributors of THIS project (not to users contributing to a project\n>> that happens to use Git).\n>>\n>> I think the first thing to fix is that we have the \"describe your\n>> changes well\" section in the latter, as if it is not part of the\n>> code that is covered by CodingGuidelines. You formulate the thought\n>> on how to explain/sell your changes to others, and you sift the text\n>> you add to help fellow developers into the ones you leave in in-code\n>> comments and in the proposed log message, while you code. I am\n>> tempted to propose moving the part about proposed log message from\n>> SubmittingPatches to CodingGuidelines for this reason.\n>>\n>> Independent of the above, here is a small update I would add to\n>> clarify the project convention on the log message.\n>>\n>> Thoughts?\n> \n> I think this is a good idea, since it differs from the way people\n> usually discuss things outside of literature, and it's very common for\n> this to trip people up.\n> \n\nPrecisely. I often get tripped up due to this whenever I write commit\nmessages in general. So, good to have this clarified through a guideline.\n\n-- \nSivaraam\n"},{"id":"447016","messageId":"20220126234205.2923388-1-gitster@pobox.com","threadId":"57291","inReplyTo":"xmqqilua89z5.fsf@gitster.g","subject":"[PATCH v2 0/3] contributor doc update around log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T23:42:02Z","receivedAt":"2022-01-26T23:42:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is an updated \"our 'present' is before the patch is applied in\nthe log messages\" patch, together with a couple of changes to\nexplain _why_ we care about your log messages.\n\nHopefully by knowing whom they are trying to help with their log\nmessages, our contributors will be able to write a more useful\nproposed log messages when they send their patches to the list.\n\nJunio C Hamano (3):\n SubmittingPatches: write problem statement in the log in the present\n tense\n CodingGuidelines: hint why we value clearly written log messages\n SubmittingPatches: explain why we care about log messages\n\n Documentation/CodingGuidelines | 7 +++++++\n Documentation/SubmittingPatches | 35 +++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+)\n\n-- \n2.35.0-155-g0eb5153edc\n\n"},{"id":"447017","messageId":"20220126234205.2923388-2-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v2 1/3] SubmittingPatches: write problem statement in the log in the present tense","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T23:42:03Z","receivedAt":"2022-01-26T23:42:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We give a guidance for proposed log message to write problem\nstatement first, followed by the reasoning behind, and recipe for,\nthe solution. Clarify that we describe the situation _before_ the\nproposed patch is applied in the present tense (not in the past\ntense e.g. \"we used to do X, but thanks to this commit we now do Y\")\nfor consistency.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/SubmittingPatches | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 92b80d94d4..7225a0fb52 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:\n \n . alternate solutions considered but discarded, if any.\n \n+[[present-tense]]\n+The problem statement that describes the status quo is written in the\n+present tense. Write \"The code does X when it is given input Y\",\n+instead of \"The code used to do Y when given input X\". You do not\n+have to say \"Currently\"---the status quo in the problem statement is\n+about the code _without_ your change, by project convention.\n+\n [[imperative-mood]]\n Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n instead of \"[This patch] makes xyzzy do frotz\" or \"[I] changed xyzzy\n-- \n2.35.0-155-g0eb5153edc\n\n"},{"id":"447018","messageId":"20220126234205.2923388-3-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v2 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T23:42:04Z","receivedAt":"2022-01-26T23:42:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 0e27b5395d..5f40595f6e 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -26,6 +26,13 @@ code. For Git in general, a few rough rules are:\n go and fix it up.\"\n Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n \n+ - Log messages to explain your changes are as much important as the\n+ changes themselves. Clearly written code and in-code comments\n+ explain how the code works and what is assumed from the surrounding\n+ context. The log messages explain what the changes wanted to\n+ achieve and why the changes were necessary (more on this in the\n+ accompanying SubmittingPatches document).\n+\n Make your code readable and sensible, and don't try to be clever.\n \n As for more concrete guidelines, just imitate the existing code\n-- \n2.35.0-155-g0eb5153edc\n\n"},{"id":"447019","messageId":"20220126234205.2923388-4-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v2 3/3] SubmittingPatches: explain why we care about log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-26T23:42:05Z","receivedAt":"2022-01-26T23:42:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Extend the \"describe your changes well\" section to cover whom we are\ntrying to help by doing so in the first place.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/SubmittingPatches | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 7225a0fb52..9c5977fac3 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -110,6 +110,34 @@ run `git diff --check` on your changes before you commit.\n [[describe-changes]]\n === Describe your changes well.\n \n+The log message that explains your changes is as much important as the\n+changes themselves. Your code may be clearly written with in-code\n+comment to sufficiently explain how it works with the surrounding\n+code, but those who need to fix or enhance your code in the future\n+will need to know _why_ your code does what it does, for a few reasons:\n+\n+. Your code may be doing something differently from what you wanted it\n+ to do. Writing down what you actually wanted to achieve will help\n+ them fix your code and make it do what it should have been doing\n+ (also, you often discover your own bug while writing your log\n+ message yourself).\n+\n+. Your code may be doing things that were only necessary for your\n+ immediate needs (e.g. \"do X to directories\" without implementing or\n+ even designing what is to be done on files). Writing down why you\n+ excluded what the code does not do will help guide future developers\n+ (e.g. writing down \"we do X to directories, because directories have\n+ characteristic Y\" would help them infer \"oh, files also have the\n+ same characteristic Y, so perhaps doing X to them would also make\n+ sense?\". Or \"we don't do the same X to files, because ...\" will\n+ help them decide if the reasoning is sound (in which case they do\n+ not waste time extending your code to cover files), or reason\n+ differently (in which case they explain why they extend your code to\n+ cover files, too).\n+\n+The goal of your log message is to convey the _why_ behind your\n+change to help them.\n+\n The first line of the commit message should be a short description (50\n characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),\n and should skip the full stop. It is also conventional in most cases to\n-- \n2.35.0-155-g0eb5153edc\n\n"},{"id":"447054","messageId":"CAPig+cRNwjaRtqLcGrz-oE+C+atAHiRgXTDTUTisgDabkOvcMw@mail.gmail.com","threadId":"57291","inReplyTo":"20220126234205.2923388-3-gitster@pobox.com","subject":"Re: [PATCH v2 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-01-27T07:31:34Z","receivedAt":"2022-01-27T07:31:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jan 27, 2022 at 1:32 AM Junio C Hamano wrote:\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> @@ -26,6 +26,13 @@ code. For Git in general, a few rough rules are:\n> + - Log messages to explain your changes are as much important as the\n\nNit: Dropping \"much\" makes this sound a bit better grammatically:\n\n\"Log messages which explain your changes are as important as the...\"\n\n> + changes themselves. Clearly written code and in-code comments\n> + explain how the code works and what is assumed from the surrounding\n> + context. The log messages explain what the changes wanted to\n> + achieve and why the changes were necessary (more on this in the\n> + accompanying SubmittingPatches document).\n"},{"id":"447108","messageId":"xmqqh79p2fi4.fsf@gitster.g","threadId":"57291","inReplyTo":"CAPig+cRNwjaRtqLcGrz-oE+C+atAHiRgXTDTUTisgDabkOvcMw@mail.gmail.com","subject":"Re: [PATCH v2 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T18:35:47Z","receivedAt":"2022-01-27T18:35:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Nit: Dropping \"much\" makes this sound a bit better grammatically:\n\nThanks.\n"},{"id":"447112","messageId":"20220127190259.2470753-1-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v3 0/3] contributor doc update around log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T19:02:56Z","receivedAt":"2022-01-27T19:03:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We already explain _how_ to write log messages, but did not tell\nreaders why we care. Add a few paragraphs to SubmittingPatches\nand CodingGuidelines to do so.\n\nJunio C Hamano (3):\n SubmittingPatches: write problem statement in the log in the present\n tense\n CodingGuidelines: hint why we value clearly written log messages\n SubmittingPatches: explain why we care about log messages\n\n Documentation/CodingGuidelines | 7 +++++++\n Documentation/SubmittingPatches | 36 +++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n\nRange-diff against v2:\n-: ---------- > 1: b86a706bfd SubmittingPatches: write problem statement in the log in the present tense\n1: 35e39deb7d ! 2: a65df87939 CodingGuidelines: hint why we value clearly written log messages\n @@ Documentation/CodingGuidelines: code. For Git in general, a few rough rules are\n go and fix it up.\"\n Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n \n -+ - Log messages to explain your changes are as much important as the\n ++ - Log messages to explain your changes are as important as the\n + changes themselves. Clearly written code and in-code comments\n + explain how the code works and what is assumed from the surrounding\n + context. The log messages explain what the changes wanted to\n2: fb54bcfcda < -: ---------- SubmittingPatches: explain why we care about log messages\n-: ---------- > 3: 77d918fb98 SubmittingPatches: explain why we care about log messages\n-- \n2.35.0-177-g7d269f5170\n\n"},{"id":"447113","messageId":"20220127190259.2470753-2-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T19:02:57Z","receivedAt":"2022-01-27T19:03:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We give a guidance for proposed log message to write problem\nstatement first, followed by the reasoning behind, and recipe for,\nthe solution. Clarify that we describe the situation _before_ the\nproposed patch is applied in the present tense (not in the past\ntense e.g. \"we used to do X, but thanks to this commit we now do Y\")\nfor consistency.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/SubmittingPatches | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 92b80d94d4..7225a0fb52 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:\n \n . alternate solutions considered but discarded, if any.\n \n+[[present-tense]]\n+The problem statement that describes the status quo is written in the\n+present tense. Write \"The code does X when it is given input Y\",\n+instead of \"The code used to do Y when given input X\". You do not\n+have to say \"Currently\"---the status quo in the problem statement is\n+about the code _without_ your change, by project convention.\n+\n [[imperative-mood]]\n Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n instead of \"[This patch] makes xyzzy do frotz\" or \"[I] changed xyzzy\n-- \n2.35.0-177-g7d269f5170\n\n"},{"id":"447114","messageId":"20220127190259.2470753-3-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T19:02:58Z","receivedAt":"2022-01-27T19:03:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 0e27b5395d..c37c43186e 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -26,6 +26,13 @@ code. For Git in general, a few rough rules are:\n go and fix it up.\"\n Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n \n+ - Log messages to explain your changes are as important as the\n+ changes themselves. Clearly written code and in-code comments\n+ explain how the code works and what is assumed from the surrounding\n+ context. The log messages explain what the changes wanted to\n+ achieve and why the changes were necessary (more on this in the\n+ accompanying SubmittingPatches document).\n+\n Make your code readable and sensible, and don't try to be clever.\n \n As for more concrete guidelines, just imitate the existing code\n-- \n2.35.0-177-g7d269f5170\n\n"},{"id":"447115","messageId":"20220127190259.2470753-4-gitster@pobox.com","threadId":"57291","inReplyTo":"20220126234205.2923388-1-gitster@pobox.com","subject":"[PATCH v3 3/3] SubmittingPatches: explain why we care about log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-27T19:02:59Z","receivedAt":"2022-01-27T19:03:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Extend the \"describe your changes well\" section to cover whom we are\ntrying to help by doing so in the first place.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/SubmittingPatches | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\nindex 7225a0fb52..a6121d1d42 100644\n--- a/Documentation/SubmittingPatches\n+++ b/Documentation/SubmittingPatches\n@@ -110,6 +110,35 @@ run `git diff --check` on your changes before you commit.\n [[describe-changes]]\n === Describe your changes well.\n \n+The log message that explains your changes is just as important as the\n+changes themselves. Your code may be clearly written with in-code\n+comment to sufficiently explain how it works with the surrounding\n+code, but those who need to fix or enhance your code in the future\n+will need to know _why_ your code does what it does, for a few\n+reasons:\n+\n+. Your code may be doing something differently from what you wanted it\n+ to do. Writing down what you actually wanted to achieve will help\n+ them fix your code and make it do what it should have been doing\n+ (also, you often discover your own bugs yourself, while writing the\n+ log message to summarize the thought behind it).\n+\n+. Your code may be doing things that were only necessary for your\n+ immediate needs (e.g. \"do X to directories\" without implementing or\n+ even designing what is to be done on files). Writing down why you\n+ excluded what the code does not do will help guide future developers.\n+ Writing down \"we do X to directories, because directories have\n+ characteristic Y\" would help them infer \"oh, files also have the same\n+ characteristic Y, so perhaps doing X to them would also make sense?\".\n+ Saying \"we don't do the same X to files, because ...\" will help them\n+ decide if the reasoning is sound (in which case they do not waste\n+ time extending your code to cover files), or reason differently (in\n+ which case, they can explain why they extend your code to cover\n+ files, too).\n+\n+The goal of your log message is to convey the _why_ behind your\n+change to help future developers.\n+\n The first line of the commit message should be a short description (50\n characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),\n and should skip the full stop. It is also conventional in most cases to\n-- \n2.35.0-177-g7d269f5170\n\n"},{"id":"450324","messageId":"YiFWZal+k6ixnYPU@google.com","threadId":"57291","inReplyTo":"20220127190259.2470753-2-gitster@pobox.com","subject":"Re: [PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-03-03T23:59:33Z","receivedAt":"2022-03-03T23:59:43Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jan 27, 2022 at 11:02:57AM -0800, Junio C Hamano wrote:\n> \n> We give a guidance for proposed log message to write problem\n> statement first, followed by the reasoning behind, and recipe for,\n> the solution. Clarify that we describe the situation _before_ the\n> proposed patch is applied in the present tense (not in the past\n> tense e.g. \"we used to do X, but thanks to this commit we now do Y\")\n> for consistency.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/SubmittingPatches | 7 +++++++\n> 1 file changed, 7 insertions(+)\n> \n> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\n> index 92b80d94d4..7225a0fb52 100644\n> --- a/Documentation/SubmittingPatches\n> +++ b/Documentation/SubmittingPatches\n> @@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which:\n> \n> . alternate solutions considered but discarded, if any.\n> \n> +[[present-tense]]\n> +The problem statement that describes the status quo is written in the\n> +present tense. Write \"The code does X when it is given input Y\",\n> +instead of \"The code used to do Y when given input X\". You do not\n> +have to say \"Currently\"---the status quo in the problem statement is\n> +about the code _without_ your change, by project convention.\n> +\n\nThe writing sample helps a lot here. \"Present tense\" is a poor example,\nbut I think it's common for native language speakers to know less about\nspecific grammar words (like \"imperative mood\" below), even though they\ncan often instinctively tell what form you're describing when seeing a\nsample.\n\nAs for the norm itself, I think it is a good one, and I've seen it\npointed out in code reviews frequently. Thanks.\n\nReviewed-by: Emily Shaffer \n\n> [[imperative-mood]]\n> Describe your changes in imperative mood, e.g. \"make xyzzy do frotz\"\n> instead of \"[This patch] makes xyzzy do frotz\" or \"[I] changed xyzzy\n> -- \n> 2.35.0-177-g7d269f5170\n> \n"},{"id":"450325","messageId":"YiFYQBWBVq/HQULM@google.com","threadId":"57291","inReplyTo":"20220127190259.2470753-3-gitster@pobox.com","subject":"Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-03-04T00:07:28Z","receivedAt":"2022-03-04T00:07:39Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jan 27, 2022 at 11:02:58AM -0800, Junio C Hamano wrote:\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/CodingGuidelines | 7 +++++++\n> 1 file changed, 7 insertions(+)\n> \n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 0e27b5395d..c37c43186e 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -26,6 +26,13 @@ code. For Git in general, a few rough rules are:\n> go and fix it up.\"\n> Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n> \n> + - Log messages to explain your changes are as important as the\n> + changes themselves. Clearly written code and in-code comments\n> + explain how the code works and what is assumed from the surrounding\n> + context. The log messages explain what the changes wanted to\n> + achieve and why the changes were necessary (more on this in the\n> + accompanying SubmittingPatches document).\n> +\n\nOne thing not listed here, that I often hope to find from the commit\nmessage (and don't), is \"why we did it this way instead of \".\nI am not sure how to phrase it in this document, though. Maybe:\n\n The log messages explain what the changes wanted to achieve, any\n decisions that were made between alternative approaches, and why the\n changes were necessary (more on this in blah blah)\n\nOr maybe \"...whether any alternative approaches were considered...\" fits\nthe form of the surrounding sentence better.\n\n - Emily\n\n> Make your code readable and sensible, and don't try to be clever.\n> \n> As for more concrete guidelines, just imitate the existing code\n> -- \n> 2.35.0-177-g7d269f5170\n> \n"},{"id":"450326","messageId":"YiFY693P6E/eVS3k@google.com","threadId":"57291","inReplyTo":"20220127190259.2470753-4-gitster@pobox.com","subject":"Re: [PATCH v3 3/3] SubmittingPatches: explain why we care about log messages","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-03-04T00:10:19Z","receivedAt":"2022-03-04T00:10:30Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jan 27, 2022 at 11:02:59AM -0800, Junio C Hamano wrote:\n> \n> Extend the \"describe your changes well\" section to cover whom we are\n> trying to help by doing so in the first place.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/SubmittingPatches | 29 +++++++++++++++++++++++++++++\n> 1 file changed, 29 insertions(+)\n> \n> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches\n> index 7225a0fb52..a6121d1d42 100644\n> --- a/Documentation/SubmittingPatches\n> +++ b/Documentation/SubmittingPatches\n> @@ -110,6 +110,35 @@ run `git diff --check` on your changes before you commit.\n> [[describe-changes]]\n> === Describe your changes well.\n> \n> +The log message that explains your changes is just as important as the\n> +changes themselves. Your code may be clearly written with in-code\n> +comment to sufficiently explain how it works with the surrounding\n> +code, but those who need to fix or enhance your code in the future\n> +will need to know _why_ your code does what it does, for a few\n> +reasons:\n> +\n> +. Your code may be doing something differently from what you wanted it\n> + to do. Writing down what you actually wanted to achieve will help\n> + them fix your code and make it do what it should have been doing\n> + (also, you often discover your own bugs yourself, while writing the\n> + log message to summarize the thought behind it).\n> +\n> +. Your code may be doing things that were only necessary for your\n> + immediate needs (e.g. \"do X to directories\" without implementing or\n> + even designing what is to be done on files). Writing down why you\n> + excluded what the code does not do will help guide future developers.\n> + Writing down \"we do X to directories, because directories have\n> + characteristic Y\" would help them infer \"oh, files also have the same\n> + characteristic Y, so perhaps doing X to them would also make sense?\".\n> + Saying \"we don't do the same X to files, because ...\" will help them\n> + decide if the reasoning is sound (in which case they do not waste\n> + time extending your code to cover files), or reason differently (in\n> + which case, they can explain why they extend your code to cover\n> + files, too).\n> +\n> +The goal of your log message is to convey the _why_ behind your\n> +change to help future developers.\n> +\n\nThis is pretty compelling. Is it clear enough why we care about this in\nthe commit message, as opposed to in the patch description (cover letter\nor post-\"---\" blurb)? Is it too obvious to explicitly mention that the\ncommit message is the first thing we try to make sense of during a 'git\nblame' or 'git bisect'?\n\nEither way, I think this is an improvement on the doc as it was before.\n\nReviewed-by: Emily Shaffer \n\n> The first line of the commit message should be a short description (50\n> characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]),\n> and should skip the full stop. It is also conventional in most cases to\n> -- \n> 2.35.0-177-g7d269f5170\n> \n"},{"id":"450327","messageId":"YiFZicz69mDyFzXB@google.com","threadId":"57291","inReplyTo":"20220127190259.2470753-1-gitster@pobox.com","subject":"Re: [PATCH v3 0/3] contributor doc update around log messages","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-03-04T00:12:57Z","receivedAt":"2022-03-04T00:13:07Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jan 27, 2022 at 11:02:56AM -0800, Junio C Hamano wrote:\n> \n> We already explain _how_ to write log messages, but did not tell\n> readers why we care. Add a few paragraphs to SubmittingPatches\n> and CodingGuidelines to do so.\n\nBah, after I reviewed this I went and checked 'next' to see you already\nmerged. Sorry for the noise. :X\n\n> \n> Junio C Hamano (3):\n> SubmittingPatches: write problem statement in the log in the present\n> tense\n> CodingGuidelines: hint why we value clearly written log messages\n> SubmittingPatches: explain why we care about log messages\n> \n> Documentation/CodingGuidelines | 7 +++++++\n> Documentation/SubmittingPatches | 36 +++++++++++++++++++++++++++++++++\n> 2 files changed, 43 insertions(+)\n> \n> Range-diff against v2:\n> -: ---------- > 1: b86a706bfd SubmittingPatches: write problem statement in the log in the present tense\n> 1: 35e39deb7d ! 2: a65df87939 CodingGuidelines: hint why we value clearly written log messages\n> @@ Documentation/CodingGuidelines: code. For Git in general, a few rough rules are\n> go and fix it up.\"\n> Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n> \n> -+ - Log messages to explain your changes are as much important as the\n> ++ - Log messages to explain your changes are as important as the\n> + changes themselves. Clearly written code and in-code comments\n> + explain how the code works and what is assumed from the surrounding\n> + context. The log messages explain what the changes wanted to\n> 2: fb54bcfcda < -: ---------- SubmittingPatches: explain why we care about log messages\n> -: ---------- > 3: 77d918fb98 SubmittingPatches: explain why we care about log messages\n> -- \n> 2.35.0-177-g7d269f5170\n> \n"},{"id":"450329","messageId":"xmqqzgm65zvy.fsf@gitster.g","threadId":"57291","inReplyTo":"YiFWZal+k6ixnYPU@google.com","subject":"Re: [PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T00:23:45Z","receivedAt":"2022-03-04T00:23:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> As for the norm itself, I think it is a good one, and I've seen it\n> pointed out in code reviews frequently. Thanks.\n>\n> Reviewed-by: Emily Shaffer \n\nThanks.\n\nWas this meant to be sent long after the topic was merged to\n'master' at 83760938 (Merge branch 'jc/doc-log-messages',\n2022-02-11)?\n"},{"id":"450330","messageId":"xmqqv8wu5zpr.fsf@gitster.g","threadId":"57291","inReplyTo":"YiFYQBWBVq/HQULM@google.com","subject":"Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T00:27:28Z","receivedAt":"2022-03-04T00:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n>> + - Log messages to explain your changes are as important as the\n>> + changes themselves. Clearly written code and in-code comments\n>> + explain how the code works and what is assumed from the surrounding\n>> + context. The log messages explain what the changes wanted to\n>> + achieve and why the changes were necessary (more on this in the\n>> + accompanying SubmittingPatches document).\n>> +\n>\n> One thing not listed here, that I often hope to find from the commit\n> message (and don't), is \"why we did it this way instead of \".\n> I am not sure how to phrase it in this document, though. Maybe:\n>\n> The log messages explain what the changes wanted to achieve, any\n> decisions that were made between alternative approaches, and why the\n> changes were necessary (more on this in blah blah)\n>\n> Or maybe \"...whether any alternative approaches were considered...\" fits\n> the form of the surrounding sentence better.\n\nQuite valid observation.\n\nDocumentation/SubmittingPatches::meaningful-message makes a note on\nthese points, and the above may want to be more aligned to them.\n\nPatches welcome, as these have long been merged to 'master/main'.\n\nThanks.\n"},{"id":"450331","messageId":"xmqqr17i5zlu.fsf@gitster.g","threadId":"57291","inReplyTo":"YiFY693P6E/eVS3k@google.com","subject":"Re: [PATCH v3 3/3] SubmittingPatches: explain why we care about log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T00:29:49Z","receivedAt":"2022-03-04T00:29:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n>> +The goal of your log message is to convey the _why_ behind your\n>> +change to help future developers.\n>> +\n>\n> This is pretty compelling. Is it clear enough why we care about this in\n> the commit message, as opposed to in the patch description (cover letter\n> or post-\"---\" blurb)? Is it too obvious to explicitly mention that the\n> commit message is the first thing we try to make sense of during a 'git\n> blame' or 'git bisect'?\n\nHaving to say \"this may be better in the in-code comment rather than\nthe log message\" to some patch recently (I do not remember), I tend\nto agree that some guidance would help people decide between the two\n(or writing both).\n\nAgain, patches welcome ;-)\n\n"},{"id":"450371","messageId":"220304.86ilsuf1e8.gmgdl@evledraar.gmail.com","threadId":"57291","inReplyTo":"xmqqr17i5zlu.fsf@gitster.g","subject":"log messages > comments (was: [PATCH v3 3/3] SubmittingPatches: explain why we care about log messages)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-04T09:52:01Z","receivedAt":"2022-03-04T10:38:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 03 2022, Junio C Hamano wrote:\n\n> Emily Shaffer writes:\n>\n>>> +The goal of your log message is to convey the _why_ behind your\n>>> +change to help future developers.\n>>> +\n>>\n>> This is pretty compelling. Is it clear enough why we care about this in\n>> the commit message, as opposed to in the patch description (cover letter\n>> or post-\"---\" blurb)? Is it too obvious to explicitly mention that the\n>> commit message is the first thing we try to make sense of during a 'git\n>> blame' or 'git bisect'?\n>\n> Again, patches welcome ;-)\n\nI think for something that's a stylistic preference I'd see why Emily\nwould try to see how you feel about it first.\n\nI.e. in this project it's ultimately up to you to decide on those\nthings, so for coding guidelines you've just updated I'd probably do the\nsame.\n\nDon't take that as a complaint on the end result b.t.w., I think the\noverall excellent state of the codebase in this project speaks for\nitself on that front.\n\nI just see why other contributors would be gun shy about pulling the\ntrigger on a patch submission on this particular front :)\n\n> Having to say \"this may be better in the in-code comment rather than\n> the log message\" to some patch recently (I do not remember), I tend\n> to agree that some guidance would help people decide between the two\n> (or writing both).\n\n[A somewhat sneaky $subject change :)]\n\nI think you're referring to this comment on (some admittedly tricky)\ncode I wrote[1].\n\nFirst, given the above I'll adjust that to your preferences on a\nre-submission. So don't take this as some argument on *that* specific\npoint. I'll add a comment in a re-roll.\n\nBut on reflection I still wouldn't put a comment on that code if it were\npurely up to me. Why?\n\nFirst, I think all programmers go through a phase of learning where they\nfeel more compelled to comment on the \"how\" v.s. \"why\" early on.\n\nAt the most extremes beginner programmers explaining how say a common\nstandard library function works in code that's relatively\nstraightforward.\n\nBut that's really not the case in [1], that code really is doing\nsomething odd and worth explaining. So why not add a comment?\n\nBecause access to ubiquitous and *local* source control from git changed\na lot of people's habits on this front, including mine. For this code I\n*would* definitely add a comment there if it was the pre-DVCS[2] days.\n\nBut I'd say that today my criteria for adding a comment is closer to:\n\n Is this so essential to note for the understanding of the rest of\n this code that nobody who's skimming past this line or reading this\n part *wouldn't* want this information?\n\n Or rather, they might not absolutely want to know, but it might be\n useful, *and* there's nothing odd about the pattern itself that\n makes you go \"hrm?\" enough to run \"git blame/log\" on it.\n\nIn this case it's clearly pretty weird that we run the exact \"test-tool\nregex\" command twice. But I think that \"hrm?\" applies. To elaborate:\n\n A. It's setting up a self-contained prereq, so it's not essential\n to understand that implementation detail to read the rest of the\n code.\n\n B. Anyone who does want to see why that odd case is the way it is can\n run some version \"git blame\" or say:\n\n git log -p -L14,21:t/t7812-grep-icase-non-ascii.sh\n\n C. Each comment you add, even within a function or other scope dilutes\n the value of other more important comments. It trains people not to\n read them, as they're probably not that important.\n\n D. Comments that are \"frozen in time\" by adding them to the code\n itself are almost always in danger of drifting in accuracy from the\n rest of the implementation and its assumptions.\n\n Even in well-curated codebases like git.git they're *much more*\n likely to drift away from the \"ground truth\" than code is.\n\n E. Even if \"D\" isn't true, commit messages (in this case my [3]) are\n almost always at an advantage over comments in that they accompany\n a change to the pre-image.\n\n So e.g. that commit message doesn't need to waste time explaining\n what pattern we'd prefer not to have here instead of the post-image,\n you get that context for free.\n\nDue to a combination of \"D\" and \"E\" I almost never read comments in\ntheir current context, unless it's painfully obvious that they *must*\nstill apply to the current code. I'll usually run some variant of \"git\nblame\" or say:\n\n git log -p -L:relevant_function:file\n\nAnd then see how the code looked when that comment was added, and page\nthrough what's changed since then.\n\n1. https://lore.kernel.org/git/xmqqsfryah42.fsf@gitster.g/\n2. To those who'd nitpick DVCS v.s. VCS: Yes nothing changed in theory from CVS/SVN\n etc. on this front, but in practice it did.\n \n Access to version control didn't tend to be ubiquitous, and even if it was\n accessible many people browsing or contributing to your code would probably\n do so via a downloaded tarball than trying to get CVS or whatever to work.\n So the \"D\" in DVCS really did change this.\n3. https://lore.kernel.org/git/patch-12.15-f3cc5bc7eb9-20220302T171755Z-avarab@gmail.com/\n\n\n\n"},{"id":"450436","messageId":"xmqqk0d91p5s.fsf@gitster.g","threadId":"57291","inReplyTo":"220304.86ilsuf1e8.gmgdl@evledraar.gmail.com","subject":"Re: log messages > comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T19:41:19Z","receivedAt":"2022-03-04T20:01:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Mar 03 2022, Junio C Hamano wrote:\n>\n>> Emily Shaffer writes:\n>>\n>>>> +The goal of your log message is to convey the _why_ behind your\n>>>> +change to help future developers.\n>>>> +\n>>>\n>>> This is pretty compelling. Is it clear enough why we care about this in\n>>> the commit message, as opposed to in the patch description (cover letter\n>>> or post-\"---\" blurb)? Is it too obvious to explicitly mention that the\n>>> commit message is the first thing we try to make sense of during a 'git\n>>> blame' or 'git bisect'?\n>>\n>> Again, patches welcome ;-)\n>\n> I think for something that's a stylistic preference I'd see why Emily\n> would try to see how you feel about it first.\n\nI do not think there is a need to write down stylistic preference.\nI may show my preference during my reviews, of course, but I won't\ntake preference-only things as a blocker.\n\nI also do not think things like \"'We used to do X here but we do Y\nbecause ...' does not belong to in-code comment, but to log message\"\nis \"stylistic preference\", and if people are unclear about, I agree\nthat we should spell it out.\n\nAn example, I can think of offhand, of what should be in comment,\nwhether we also write in the log, is \"We do X here because that\nother code expects us to\", when it is tricky to figure out by\nreading the code by itself without going back to \"git blame\".\n\n\"git blame\" certainly can be used to figure out which commit touched\nthe line that does X (which is hard to figure out why), and the log\nmessage can refer to \"that other code expects us to\", but that is an\nextra operation.\n\nAlso, when we really need to figure out, it is wonderful that we can\nask \"blame\" to give us the commit, and can look at \"that other code\"\nin the same commit by checking it out to the working tree,\nespecially when \"that other code\" may have drifted and the original\nreasoning no longer applies (iow, what we find out from \"git blame\"\nmay become stale, and it will stay stale forever because we cannot\nrewrite the history that old).\n\nNow, it is certainly not black/white decision to say what is and\nwhat is not tricky to figure out in the code. We shouldn't be\ncommenting obvious things. Two yardsticks I use are\n\n (1) if reviewers raise questions during a review, it may indicate\n that it is worth commenting.\n\n (2) if an earlier round of the same series had a bug around the\n area, it may indicate that the fixed code is worth commenting.\n\nbut the way I use them is more to say \"I found this uncommented code\nsomewhat tricky---but nobody asked a question and it has stayed the\nsame from the initial round, so it may be clear enough for other\npeople, and after all I managed to figure out myself, so it probably\nis OK to leave it uncommented\".\n"},{"id":"450444","messageId":"xmqq4k4d1k3u.fsf@gitster.g","threadId":"57291","inReplyTo":"xmqqk0d91p5s.fsf@gitster.g","subject":"Re: log messages > comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-04T21:30:29Z","receivedAt":"2022-03-04T21:30:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I also do not think things like \"'We used to do X here but we do Y\n> because ...' does not belong to in-code comment, but to log message\"\n\nSorry for too many nagations. \"I also do not\" -> \"I also do\".\n"},{"id":"450459","messageId":"YiKjtcHSBXWky7Ze@google.com","threadId":"57291","inReplyTo":"xmqqzgm65zvy.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] SubmittingPatches: write problem statement in the log in the present tense","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-03-04T23:41:41Z","receivedAt":"2022-03-04T23:41:52Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Mar 03, 2022 at 04:23:45PM -0800, Junio C Hamano wrote:\n> \n> Emily Shaffer writes:\n> \n> > As for the norm itself, I think it is a good one, and I've seen it\n> > pointed out in code reviews frequently. Thanks.\n> >\n> > Reviewed-by: Emily Shaffer \n> \n> Thanks.\n> \n> Was this meant to be sent long after the topic was merged to\n> 'master' at 83760938 (Merge branch 'jc/doc-log-messages',\n> 2022-02-11)?\n\nAs mentioned in\nhttps://lore.kernel.org/git/YiFZicz69mDyFzXB%40google.com - no, it\nwasn't. In fact, in conversation at $dayjob someone said \"whatever\nhappened to Junio planning to write updates to SubmittingPatches?\" and\nsomeone else said \"it didn't really get any review\" - so I said \"okay, I\nwill do review\" instead of checking whether you merged anyway. So mostly\nuseless noise :)\n\n - Emily\n"},{"id":"453634","messageId":"xmqq8rs82m4f.fsf@gitster.g","threadId":"57291","inReplyTo":"xmqqv8wu5zpr.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-14T06:51:44Z","receivedAt":"2022-04-14T06:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Emily Shaffer writes:\n>\n>>> + - Log messages to explain your changes are as important as the\n>>> + changes themselves. Clearly written code and in-code comments\n>>> + explain how the code works and what is assumed from the surrounding\n>>> + context. The log messages explain what the changes wanted to\n>>> + achieve and why the changes were necessary (more on this in the\n>>> + accompanying SubmittingPatches document).\n>>> +\n>>\n>> One thing not listed here, that I often hope to find from the commit\n>> message (and don't), is \"why we did it this way instead of \".\n>> I am not sure how to phrase it in this document, though. Maybe:\n>>\n>> The log messages explain what the changes wanted to achieve, any\n>> decisions that were made between alternative approaches, and why the\n>> changes were necessary (more on this in blah blah)\n>>\n>> Or maybe \"...whether any alternative approaches were considered...\" fits\n>> the form of the surrounding sentence better.\n>\n> Quite valid observation.\n>\n> Documentation/SubmittingPatches::meaningful-message makes a note on\n> these points, and the above may want to be more aligned to them.\n>\n> Patches welcome, as these have long been merged to 'master/main'.\n\nAnother thing. If you (not Emily, but figuratively) haven't watched\nVictoria's talk https://www.youtube.com/watch?v=4qLtKx9S9a8 on the\ntopic of clearly written commits, you should drop everything you are\ndoing and go watch it.\n\nAnd with what we learn from it, we may be able to rewrite this part\nof the documentation much more clearly.\n\n\n"},{"id":"453653","messageId":"220414.86lew7d7tb.gmgdl@evledraar.gmail.com","threadId":"57291","inReplyTo":"xmqq8rs82m4f.fsf@gitster.g","subject":"Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-14T14:04:59Z","receivedAt":"2022-04-14T15:47:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Apr 13 2022, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n>> Emily Shaffer writes:\n>>\n>>>> + - Log messages to explain your changes are as important as the\n>>>> + changes themselves. Clearly written code and in-code comments\n>>>> + explain how the code works and what is assumed from the surrounding\n>>>> + context. The log messages explain what the changes wanted to\n>>>> + achieve and why the changes were necessary (more on this in the\n>>>> + accompanying SubmittingPatches document).\n>>>> +\n>>>\n>>> One thing not listed here, that I often hope to find from the commit\n>>> message (and don't), is \"why we did it this way instead of \".\n>>> I am not sure how to phrase it in this document, though. Maybe:\n>>>\n>>> The log messages explain what the changes wanted to achieve, any\n>>> decisions that were made between alternative approaches, and why the\n>>> changes were necessary (more on this in blah blah)\n>>>\n>>> Or maybe \"...whether any alternative approaches were considered...\" fits\n>>> the form of the surrounding sentence better.\n>>\n>> Quite valid observation.\n>>\n>> Documentation/SubmittingPatches::meaningful-message makes a note on\n>> these points, and the above may want to be more aligned to them.\n>>\n>> Patches welcome, as these have long been merged to 'master/main'.\n>\n> Another thing. If you (not Emily, but figuratively) haven't watched\n> Victoria's talk https://www.youtube.com/watch?v=4qLtKx9S9a8 on the\n> topic of clearly written commits, you should drop everything you are\n> doing and go watch it.\n>\n> And with what we learn from it, we may be able to rewrite this part\n> of the documentation much more clearly.\n\nThe slides for it are at\nhttps://vdye.github..io/2022/OS101-Writing-Commits.pdf (not in the video\ndescription, but at the very end of the video).\n\nIt's easy to nitpick/improve existing examples, so here goes :)\n\nThe main commit message example in that talk starts as just \"Make error\ntext more helpful\", and ends with a better version as:\n\n\tgit-portable.sh: make error text more helpful\n\t\n\tThe message “Not a valid command: ” is\n\tintended to notify the user that their subcommand is invalid.\n\tHowever, when no subcommand is given, the \"empty\" subcommand\n\tresults in the same message: \"Not a valid command:\". This does\n\tnot clearly guide the user to the correct behavior, so print\n\t\"Please specify a command\" when no subcommand is specified.\n\nFor our CodingGuidelines I think it would be useful to have some version\nof \"if you can explain something with prose or tests, prefer\ntests\".\n\nI.e. other things being equal I'd much prefer this version\n(pseudo-patch):\n\n\tgit-portable.sh: don't conflate invalid and non-existing command\n\n\t git-portable-test.sh | 2 +-\n\t 1 file changed, 1 insertion(+), 1 deletion(-)\n\t\n\tdiff --git a/git-portable-test.sh b/git-portable-test.sh\n\tindex c8bd464..e03f4a8 100644\n\t--- a/git-portable-test.sh\n\t+++ b/git-portable-test.sh\n\t@@ -5,7 +5,7 @@ test_expect_failure 'usage: invalid command' '\n\t '\n\t \n\t test_expect_failure 'usage: no command' '\n\t-\ttest_expect_code_output 129 \"Not a valid command: \" ./gitportable.sh\n\t+\ttest_expect_code_output 129 \"Please specify a command\" ./gitportable.sh\n\t '\n\t \n\t test_done\n\nIt ends up basically saying the same thing, but now we're saying it with\na regression test (test_expect_code_output doesn't exist, but let's\npretend it's test_expect_code + a test_cmp-alike).\n\nWhat it does entirely omit is the \"why\".\n\nNow I realize I'm nitpicking a slide shown at a conference, which by its\nnature needs to show a small pseudo-example, but I think this applies in\ngeneral:\n\nWhile \"why\" is a good rule of thumb I think it's just as important to\nknow when not to include explanations and when to include one.\n\nFor cases where something is straightforward enough (as in this case,\nthe RHS of \": \" is clearly missing) I'd think omitting the explanation\nwould be better, as we should also be concerned about the overall signal\nratio.\n\n(Now, if anyone glances at my own commit messages they'll see I'm\nthoroughly in \"throwing rocks from a glass house\" territory here :) I'm\nnot saying I'm consistency practicing what I'm preaching).\n\nBut just like comments there's no right answer, when one person thinks\nan explanation is different from another.\n\nBut it is unambiguously the case that we can often replace prose with\ntests, and in those cases we should almost always prefer that.\n\nIt's also the case that even if everyone agrees that a \"why\" is needed\nthere's multiple ways to store that information. One is via commit\nmessages, another would e.g. be that same commit updating some shared\nguidelines about goals/examples of CLI usage.\n\nSo in this case, if a Documentation/CodingGuidelines had clear examples\nof preferred usage, we could just point briefly point to that as\nrationale.\n\nWhile git's commit messages are excellent, I think that's one area where\nwe really need improvement. It's rare to dig into some old code where no\nrationale can be found for it, either in the commit itself, or in the\npreceding ML discussion.\n\nBut it's unfortunately (at least in my experience) more often than not\nthe case that you really do need to consult those commit messages or ML\narchives, even for things that have come up a *lot* of times, they were\njust never documented in-tree.\n\nThere's all sorts of reasons for that which are not the result of any\nperson doing anything wrong, but I do think it's something we could and\nshould focus more on as a project.\n\nThe barriers of entry for adding documentation or adjusting existing\ndocumentation are much higher than adding a one-off explanation in a\ncommit message.\n\nPartially (and probably mostly) that's a really good thing, but I can't\nhelp but wonder if we're getting that balance right given the (in my\nsubjective experience) end result of us often lacking good docs, while\nwe're not lacking if one searches for replacements for those docs in\ncommit messages or the ML archive.\n\nOne more thing that I think is not explicitly covered (I skimmed the\nslides, but haven't gone throug the full back yet): Minimizing diffs.\n\nE.g. the talk shows 287fd17e3a1 (sparse-index: prevent repo root from\nbecoming sparse, 2022-03-01) as an example, which has this hunk:\n\t\n\tdiff --git a/dir.c b/dir.c\n\tindex d91295f2bcd..a136377eb49 100644\n\t--- a/dir.c\n\t+++ b/dir.c\n\t@@ -1463,10 +1463,11 @@ static int path_in_sparse_checkout_1(const char *path,\n\t \tconst char *end, *slash;\n\t \n\t \t/*\n\t-\t * We default to accepting a path if there are no patterns or\n\t-\t * they are of the wrong type.\n\t+\t * We default to accepting a path if the path is empty, there are no\n\t+\t * patterns, or the patterns are of the wrong type.\n\t \t */\n\t-\tif (init_sparse_checkout_patterns(istate) ||\n\t+\tif (!*path ||\n\t+\t init_sparse_checkout_patterns(istate) ||\n\t \t (require_cone_mode &&\n\t \t !istate->sparse_checkout_patterns->use_cone_patterns))\n\t \t\treturn 1;\n\nI think this is a worthwhile thing to consider as a replacement:\n\t\n\tdiff --git a/dir.c b/dir.c\n\tindex d91295f2bcd..93a2320ae57 100644\n\t--- a/dir.c\n\t+++ b/dir.c\n\t@@ -1466,7 +1466,8 @@ static int path_in_sparse_checkout_1(const char *path,\n\t \t * We default to accepting a path if there are no patterns or\n\t \t * they are of the wrong type.\n\t \t */\n\t-\tif (init_sparse_checkout_patterns(istate) ||\n\t+\tif (!*path || /* we consider an empty pattern to be no pattern */\n\t+\t init_sparse_checkout_patterns(istate) ||\n\t \t (require_cone_mode &&\n\t \t !istate->sparse_checkout_patterns->use_cone_patterns))\n\t \t\treturn 1;\n\nI.e. trying to optimize for smaller diffs whenever possible. It this\ncase the word-diff for the original is:\n\n /*\n * We default to accepting a path if {+the path is empty,+} there are no\n {+*+} patterns{+,+} or [-* they-]{+the patterns+} are of the wrong type.\n */\n\nNow, obviously another small isolated example that's not worth\nnitpicking in itself, but just serves to make a larger point. It's clear\nwhy the rephrasing was done in that case, because the patch adds the\n\"!*path\" check, so it makes sense a-priory to have the comment reflect\nthat.\n\nBut one thing where advice about \"narrative structure\" and good prose\ntends to break down when it comes to software development is that we're\nmuch more focused on reviews of incremental additions than many other\nfields, where it tends to be more about the final product.\n"},{"id":"453944","messageId":"Yl89hMGLN3DqIkJ7@google.com","threadId":"57291","inReplyTo":"220414.86lew7d7tb.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2022-04-19T22:53:56Z","receivedAt":"2022-04-19T22:54:07Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Apr 14, 2022 at 04:04:59PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> On Wed, Apr 13 2022, Junio C Hamano wrote:\n> \n> > Junio C Hamano writes:\n> >\n> >> Emily Shaffer writes:\n> >>\n> >>>> + - Log messages to explain your changes are as important as the\n> >>>> + changes themselves. Clearly written code and in-code comments\n> >>>> + explain how the code works and what is assumed from the surrounding\n> >>>> + context. The log messages explain what the changes wanted to\n> >>>> + achieve and why the changes were necessary (more on this in the\n> >>>> + accompanying SubmittingPatches document).\n> >>>> +\n> >>>\n> >>> One thing not listed here, that I often hope to find from the commit\n> >>> message (and don't), is \"why we did it this way instead of \".\n> >>> I am not sure how to phrase it in this document, though. Maybe:\n> >>>\n> >>> The log messages explain what the changes wanted to achieve, any\n> >>> decisions that were made between alternative approaches, and why the\n> >>> changes were necessary (more on this in blah blah)\n> >>>\n> >>> Or maybe \"...whether any alternative approaches were considered...\" fits\n> >>> the form of the surrounding sentence better.\n> >>\n> >> Quite valid observation.\n> >>\n> >> Documentation/SubmittingPatches::meaningful-message makes a note on\n> >> these points, and the above may want to be more aligned to them.\n> >>\n> >> Patches welcome, as these have long been merged to 'master/main'.\n> >\n> > Another thing. If you (not Emily, but figuratively) haven't watched\n> > Victoria's talk https://www.youtube.com/watch?v=4qLtKx9S9a8 on the\n> > topic of clearly written commits, you should drop everything you are\n> > doing and go watch it.\n> >\n> > And with what we learn from it, we may be able to rewrite this part\n> > of the documentation much more clearly.\n> \n> The slides for it are at\n> https://vdye.github..io/2022/OS101-Writing-Commits.pdf (not in the video\n> description, but at the very end of the video).\n> \n> It's easy to nitpick/improve existing examples, so here goes :)\n> \n> The main commit message example in that talk starts as just \"Make error\n> text more helpful\", and ends with a better version as:\n> \n> \tgit-portable.sh: make error text more helpful\n> \t\n> \tThe message “Not a valid command: ” is\n> \tintended to notify the user that their subcommand is invalid.\n> \tHowever, when no subcommand is given, the \"empty\" subcommand\n> \tresults in the same message: \"Not a valid command:\". This does\n> \tnot clearly guide the user to the correct behavior, so print\n> \t\"Please specify a command\" when no subcommand is specified.\n> \n> For our CodingGuidelines I think it would be useful to have some version\n> of \"if you can explain something with prose or tests, prefer\n> tests\".\n> \n> I.e. other things being equal I'd much prefer this version\n> (pseudo-patch):\n> \n> \tgit-portable.sh: don't conflate invalid and non-existing command\n> \n> \t git-portable-test.sh | 2 +-\n> \t 1 file changed, 1 insertion(+), 1 deletion(-)\n> \t\n> \tdiff --git a/git-portable-test.sh b/git-portable-test.sh\n> \tindex c8bd464..e03f4a8 100644\n> \t--- a/git-portable-test.sh\n> \t+++ b/git-portable-test.sh\n> \t@@ -5,7 +5,7 @@ test_expect_failure 'usage: invalid command' '\n> \t '\n> \t \n> \t test_expect_failure 'usage: no command' '\n> \t-\ttest_expect_code_output 129 \"Not a valid command: \" ./gitportable.sh\n> \t+\ttest_expect_code_output 129 \"Please specify a command\" ./gitportable.sh\n> \t '\n> \t \n> \t test_done\n> \n> It ends up basically saying the same thing, but now we're saying it with\n> a regression test (test_expect_code_output doesn't exist, but let's\n> pretend it's test_expect_code + a test_cmp-alike).\n> \n> What it does entirely omit is the \"why\".\n> \n> Now I realize I'm nitpicking a slide shown at a conference, which by its\n> nature needs to show a small pseudo-example, but I think this applies in\n> general:\n> \n> While \"why\" is a good rule of thumb I think it's just as important to\n> know when not to include explanations and when to include one.\n> \n> For cases where something is straightforward enough (as in this case,\n> the RHS of \": \" is clearly missing) I'd think omitting the explanation\n> would be better, as we should also be concerned about the overall signal\n> ratio.\n\nPreface: I don't want to start a fight ;)\n\nI think if you are in a position where you already will read every\nsingle patch that comes across the mailing list, including its diff,\nthen you make a really valid point. I can read the negative line of your\ndiff, infer the problem (\"oh, there's nothing after :\"), and examine the\nsolution. Fine.\n\nBut I also don't think that most of us working on Git have the time to\nread every patch and its diff. I certainly don't. I'd agree that your\npatch's subject line is a little more informative than Victoria's, but\npast that, if the commit message is empty, I have no idea what problem\nyou were trying to solve until I have scrolled through lines of context,\ndiff lines, and finally arrive to the regression test (which ends up at\nthe very end of the patch in Git, because of the way the codebase is\norganized). Whereas, with Victoria's proposed commit message, I can read\nthe paragraph and decide whether I need to review, and from there decide\nwhether the diff does what she says it should.\n\nSo as I'm deciding what to review, I definitely would prefer Victoria's\ncommit message. Plus, like I mentioned, it gives the extra safeguard of\nallowing reviewers to check: does the patch actually do what the author\nmeant for it to do? If we're never told what the author meant for it to\ndo, then we are missing information needed for that part of the review.\n\nAnyway, I haven't watched Victoria's talk yet, but I will do so soon :)\n\n - Emily\n"},{"id":"453955","messageId":"xmqqbkwwrwn7.fsf@gitster.g","threadId":"57291","inReplyTo":"Yl89hMGLN3DqIkJ7@google.com","subject":"Re: [PATCH v3 2/3] CodingGuidelines: hint why we value clearly written log messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-20T08:23:24Z","receivedAt":"2022-04-20T08:23:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer writes:\n\n> On Thu, Apr 14, 2022 at 04:04:59PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> ...\n>> For our CodingGuidelines I think it would be useful to have some version\n>> of \"if you can explain something with prose or tests, prefer\n>> tests\".\n\nI was going to ignore this part as it is merely showing personal\npreference, but I guess I need to weigh in here.\n\nDemonstrating what you meant to say in the log message with tests is\nfine, but that should be in addition to prose, explaining how the\nscenario is set up and what the user wanted to do, before showing\nthat a command is giving an outcome that does not help what the user\nwanted to do.\n\nIOW, in our CodingGUidelines, we should have \"tests can be a good\nway to augument what you want to say, but explain it well to those\nwho are not so familiar with the area.\" You do not necessarily have\nto explain it to 5 year old, but the audience should not have to be\nwhoever writes the patch themself to understand it.\n\n> So as I'm deciding what to review, I definitely would prefer Victoria's\n> commit message. Plus, like I mentioned, it gives the extra safeguard of\n> allowing reviewers to check: does the patch actually do what the author\n> meant for it to do? If we're never told what the author meant for it to\n> do, then we are missing information needed for that part of the review.\n\nI have nothing to add here.\n\n> Anyway, I haven't watched Victoria's talk yet, but I will do so soon :)\n\nI do not necessarily agree with the presentation order in a proposed\nlog message she suggests, but overall, it's good investment of your\ntime. Highly recommended.\n"}]}