From: Linus Arver <linusa@google.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v3 2/2] doc: revert: add discussion
Date: Thu, 07 Sep 2023 14:29:25 -0700 [thread overview]
Message-ID: <owlytts5llje.fsf@fine.c.googlers.com> (raw)
In-Reply-To: <ZNcyhUL89WVXOv3F@ugly>
First, I apologize for the long delay in my response. I only work on Git
20% of the time, and that 20% can become 0% due to factors outside my
control.
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Fri, Aug 11, 2023 at 04:00:53PM -0700, Linus Arver wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> On Thu, Aug 10, 2023 at 02:50:59PM -0700, Linus Arver wrote:
>>>>Nit: the "doc: revert: add discussion" subject line should probably be more
>>>>like "revert doc: suggest adding the 'why' behind reverts".
>>>>
>>> this is counter to the prevalent "big endian" prefix style, and is in
>>> this case really easy to misread.
>>
>>I also learned recently that there should just be one colon ":" in the
>>subject, which is why I suggested "revert doc" as the prefix instead of
>>"doc: revert: ...".
>>
> in what context was this preference expressed?
IIRC, it was from a conversation off-list with the folks at Google's
Git-core team.
> because here, it's rather counter-productive: most commands are verbs
> for obvious reasons, so using that style sets the reader up for
> misparsing the subject on first try.
I think the convention for commit titles is
<prefix>: <action>
so the phrase "revert doc: add discussion", where the <prefix> is
"revert doc" does not parse any worse than "doc: revert: add
discussion". That is, the <prefix> is never confused with the <action>
(they are separated by the colon).
> this could be avoided by quoting
> the command, but that looks noisy in the subject.
> so rather, i'd follow another precedent, 'git-revert.txt: ', which is
> unambiguous.
SGTM.
>>> i also intentionally kept the subject generic, because the content
>>> covers two matters (the reasoning and the subjects, which is also the
>>> reason why this is a separate patch to start with).
>>
>>I think the phrase "add discussion" in "doc: revert: add discussion"
>>doesn't add much value, because your patch's diff is very easy to read
>>(in that it adds a new DISCUSSION section). I just wanted to replace it
>>with something more useful that gives more information than
>
>>just repeat
>>(somewhat redundantly) what is obvious by looking at the patch.
>>
> but ... that's exactly what a subject is supposed to do!
I think the rule of thumb is to explain the goodness of what a commit
brings, rather than focus on what is literally happening. This is
because the former is more valuable. So instead of
"git-revert.txt: add discussion"
you could say
"git-revert.txt: advise against default commit message"
and now you don't have to look at the patch to see (roughly) what kind
of discussion was added.
>>>>Please consider rewording such
>>>> subject lines to reflect the reason why the original commit is being
>>>> reapplied again.
>>>>
>>> the reasoning most likely wouldn't fit into the subject.
>>
>>Hence the language "to _reflect_ the reason", because the "reason"
>>should belong in the commit message body text.
>>
> i don't think that's how most people would actually read this.
> and i still don't see how that instruction could be meaningfully
> followed.
OK, you may be right.
>>> also, the original request to explain the reasoning applies
>>> transitively, so i don't think it's really necessary to point it out
>>> explicitly.
>>
>>It may be that a user will think only giving the revert reason in the
>>body text is enough, while leaving the subject line as is. I wanted to
>>break this line of thinking by providing additional instructions.
>>
> yes, that's the whole intention of this patch. but i don't see how
> making it more convoluted than my proposal helps in any way.
Well, even if a review makes something more convoluted, it may generate
discussion and drive consensus on the better way(s) of doing something.
I see value in that course of events.
Of course you are free to reject review comments that you truly believe
are inferior to the approach you've already taken.
But overall, when I see a reviewer's comment on this mailing list, I
assume they are trying to make my patch better. Similarly when I
reviewed your patch my intent was to provide actionable feedback to try
to make it better. I'm sorry if I did not come across that way.
>>This is definitely better. But others in this thread have already
>>commented that my version looks good (after seeing your version also,
>>presumably).
>>
> well, i'm also an "others" when it comes to your proposal, and i find it
> confusing.
I think you did the right thing by responding to my comments, and
pointing to things you found confusing.
next prev parent reply other threads:[~2023-09-07 21:29 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-28 8:35 [PATCH v2] sequencer: beautify subject of reverts of reverts Oswald Buddenhagen
2023-04-28 18:35 ` Junio C Hamano
2023-04-28 19:11 ` Oswald Buddenhagen
2023-05-01 16:44 ` Junio C Hamano
2023-05-01 19:10 ` Oswald Buddenhagen
2023-05-01 19:12 ` Junio C Hamano
2023-05-05 17:25 ` Junio C Hamano
2023-05-17 9:05 ` Phillip Wood
2023-05-17 10:00 ` Oswald Buddenhagen
2023-05-17 11:20 ` Phillip Wood
2023-05-17 17:02 ` Oswald Buddenhagen
2023-05-18 9:58 ` Phillip Wood
2023-05-18 16:28 ` Junio C Hamano
2023-07-28 5:26 ` Linus Arver
2023-07-28 9:45 ` Oswald Buddenhagen
2023-07-28 15:10 ` Junio C Hamano
2023-07-28 15:37 ` Oswald Buddenhagen
2023-07-28 16:31 ` Junio C Hamano
2023-07-28 16:47 ` Oswald Buddenhagen
2023-07-28 17:36 ` Linus Arver
2023-08-09 17:15 ` [PATCH v3 1/2] " Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v3 2/2] doc: revert: add discussion Oswald Buddenhagen
2023-08-10 21:50 ` Linus Arver
2023-08-10 22:00 ` Linus Arver
2023-08-11 15:10 ` Phillip Wood
2023-08-12 6:25 ` Oswald Buddenhagen
2023-08-13 22:09 ` Junio C Hamano
2023-08-14 14:13 ` Oswald Buddenhagen
2023-08-11 12:49 ` Oswald Buddenhagen
2023-08-11 23:00 ` Linus Arver
2023-08-12 7:19 ` Oswald Buddenhagen
2023-09-07 21:29 ` Linus Arver [this message]
2023-08-11 15:08 ` Phillip Wood
2023-08-11 17:10 ` Junio C Hamano
2023-08-11 17:05 ` Junio C Hamano
2023-08-11 17:44 ` Re* " Junio C Hamano
2023-08-11 17:53 ` Junio C Hamano
2023-08-11 17:56 ` rsbecker
2023-08-11 18:16 ` Eric Sunshine
2023-08-11 18:16 ` Oswald Buddenhagen
2023-08-11 19:43 ` Junio C Hamano
2023-08-11 15:05 ` [PATCH v3 1/2] sequencer: beautify subject of reverts of reverts Phillip Wood
2023-08-11 16:59 ` Junio C Hamano
2023-08-11 17:13 ` Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 " Oswald Buddenhagen
2023-08-21 17:07 ` [PATCH v4 2/2] git-revert.txt: add discussion Oswald Buddenhagen
2023-08-21 18:32 ` [PATCH v4 1/2] sequencer: beautify subject of reverts of reverts Junio C Hamano
2023-08-23 20:08 ` Taylor Blau
2023-08-23 21:38 ` Junio C Hamano
2023-08-24 6:14 ` Oswald Buddenhagen
2023-09-02 7:20 ` [PATCH v5] " Oswald Buddenhagen
2023-09-02 22:24 ` Junio C Hamano
2023-09-11 20:12 ` Kristoffer Haugsbakk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=owlytts5llje.fsf@fine.c.googlers.com \
--to=linusa@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=oswald.buddenhagen@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).