[BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[brianmlyles]

I noticed that commits from certain users were ending up in our
repository with comment-like lines in the commit message. I traced the
cause back to the combination of:

- Those users are using git-gui to make their commits
- A `prepare-commit-msg` hook is adding a dynamic commit message
  template using comment lines starting with `#`
- git-gui creates the commit in a way that circumvents the message
  washing similar to if one used `git commit -F`, but invokes the
  `prepare-commit-msg` hook without any additional arguments like
  "message" [1] that would tell the hook that `-F` is being used

[1]: https://git-scm.com/docs/githooks#_prepare_commit_msg

The result here is that even though the `prepare-commit-msg` hook is
already correctly short-circuiting when given the "message" parameter,
it is providing these comment lines when called by git-gui, and thus the
commits have these comment lines in them.

This seems like a bug in git-gui. I see two fixes, but I'm not sure
which is more correct:

- Have git-gui pass "message" as an argument to the
  `prepare-commit-msg` hook so that the hook knows that `-F`-like
  behavior is being used
- Have git-gui create the commit in a way that causes the message to be
  washed

The latter seems like it would be more consistent with other workflows
where the user is seeing the message in an editor, so my instinct is
that it would be the better fix.

-- 
Thank you,
Brian Lyles

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Eric Sunshine]

[cc: +j6t]

On Fri, Jul 5, 2024 at 3:23 PM brianmlyles <brianmlyles@gmail.com> wrote:
> I noticed that commits from certain users were ending up in our
> repository with comment-like lines in the commit message. [...]
> [...]
> This seems like a bug in git-gui. I see two fixes, but I'm not sure
> which is more correct:
> [...]
> - Have git-gui create the commit in a way that causes the message to be
>   washed
>
> The latter seems like it would be more consistent with other workflows
> where the user is seeing the message in an editor, so my instinct is
> that it would be the better fix.

A patch to make git-gui strip comment lines had been previously
applied[1,2], however, it badly broke git-gui when running with old
Tcl versions, such as on macOS[3,4]. The breakage was not
insurmountable, and a patch[5,6] was submitted to resolve it.
Unfortunately, the then-maintainer of git-gui lost interest in the
project about that point, thus left the issue hanging. Thus, to this
day, git-gui still doesn't strip comment lines.

Resurrecting these patches would be one way forward, assuming the new
git-gui maintainer[7] (who is Cc:'d) would be interested.

[1]: v2: https://lore.kernel.org/git/20210218181937.83419-1-me@yadavpratyush.com/
[2]: v1: https://lore.kernel.org/git/20210202200301.44282-1-me@yadavpratyush.com/
[3]: https://lore.kernel.org/git/CAPig+cT-sfgMDi9-6AEKF85NtOiXeqddJjk-pYuhDtTVAE-UEw@mail.gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cSC8uNfoAjDKdBNheod9_0-pCD-K_2kwt+J8USnoyQ7Aw@mail.gmail.com/
[5]: https://lore.kernel.org/git/20210228231110.24076-1-sunshine@sunshineco.com/
[6]: https://lore.kernel.org/git/CAPig+cRQN4PjfxEOZ8ZBA_uttsRPS8DPDgToM_JFvichDDh_HQ@mail.gmail.com/
[7]: https://lore.kernel.org/git/0241021e-0b17-4031-ad9f-8abe8e0c0097@kdbg.org/

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Sean Allred]

Eric Sunshine <sunshine@sunshineco.com> writes:
> On Fri, Jul 5, 2024 at 3:23 PM brianmlyles <brianmlyles@gmail.com> wrote:
>> I noticed that commits from certain users were ending up in our
>> repository with comment-like lines in the commit message. [...]
>> [...]
>> This seems like a bug in git-gui. I see two fixes, but I'm not sure
>> which is more correct:
>> [...]
>> - Have git-gui create the commit in a way that causes the message to be
>>   washed
>>
>> The latter seems like it would be more consistent with other workflows
>> where the user is seeing the message in an editor, so my instinct is
>> that it would be the better fix.

There is a third option -- new plumbing in git (a la
git-interpret-trailers) to expose the logic of `cleanup_message`. This
comes with some nice flexibility, but introduces complexity around
transferring state (e.g. passed options to git-commit) that would
probably be best to avoid.

The second option above does seem simpler.

> A patch to make git-gui strip comment lines had been previously
> applied[1,2], however, it badly broke git-gui when running with old
> Tcl versions, such as on macOS[3,4]. The breakage was not
> insurmountable, and a patch[5,6] was submitted to resolve it.
> Unfortunately, the then-maintainer of git-gui lost interest in the
> project about that point, thus left the issue hanging. Thus, to this
> day, git-gui still doesn't strip comment lines.
>
> Resurrecting these patches would be one way forward, assuming the new
> git-gui maintainer[7] (who is Cc:'d) would be interested.
>
> [1]: v2: https://lore.kernel.org/git/20210218181937.83419-1-me@yadavpratyush.com/
> [2]: v1: https://lore.kernel.org/git/20210202200301.44282-1-me@yadavpratyush.com/
> [3]: https://lore.kernel.org/git/CAPig+cT-sfgMDi9-6AEKF85NtOiXeqddJjk-pYuhDtTVAE-UEw@mail.gmail.com/
> [4]: https://lore.kernel.org/git/CAPig+cSC8uNfoAjDKdBNheod9_0-pCD-K_2kwt+J8USnoyQ7Aw@mail.gmail.com/
> [5]: https://lore.kernel.org/git/20210228231110.24076-1-sunshine@sunshineco.com/
> [6]: https://lore.kernel.org/git/CAPig+cRQN4PjfxEOZ8ZBA_uttsRPS8DPDgToM_JFvichDDh_HQ@mail.gmail.com/
> [7]: https://lore.kernel.org/git/0241021e-0b17-4031-ad9f-8abe8e0c0097@kdbg.org/

I haven't looked super closely at the patches you've linked, Eric, but
it seems like those are specific to stripping comment characters. As
I've noted elsewhere[1], there's potentially more to strip than just
comments (like patch scissors). I suspect the only paths forward to
guarantee that message-washing happens would either be an option to
git-commit to explicitly enable it OR (probably preferred) have git-gui
invoke git-commit with an appropriate editor instead of using -F.

[1]: https://lore.kernel.org/git/m0h6d3pphu.fsf@epic96565.epic.com/T/#u

-Sean

-- 
Sean Allred

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Eric Sunshine]

On Fri, Jul 5, 2024 at 4:56 PM Sean Allred <allred.sean@gmail.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Fri, Jul 5, 2024 at 3:23 PM brianmlyles <brianmlyles@gmail.com> wrote:
> >> - Have git-gui create the commit in a way that causes the message to be
> >>   washed
>>
> > A patch to make git-gui strip comment lines had been previously
> > applied[1,2], however, it badly broke git-gui when running with old
> > Tcl versions, such as on macOS[3,4]. The breakage was not
> > insurmountable, and a patch[5,6] was submitted to resolve it.
> > Unfortunately, the then-maintainer of git-gui lost interest in the
> > project about that point, thus left the issue hanging. Thus, to this
> > day, git-gui still doesn't strip comment lines.
>
> There is a third option -- new plumbing in git (a la
> git-interpret-trailers) to expose the logic of `cleanup_message`. This
> comes with some nice flexibility, but introduces complexity around
> transferring state (e.g. passed options to git-commit) that would
> probably be best to avoid.

Could the cleanup_message() functionality be exposed as a new option
of git-stripspace?

> I haven't looked super closely at the patches you've linked, Eric, but
> it seems like those are specific to stripping comment characters. As
> I've noted elsewhere[1], there's potentially more to strip than just
> comments (like patch scissors). I suspect the only paths forward to
> guarantee that message-washing happens would either be an option to
> git-commit to explicitly enable it OR (probably preferred) have git-gui
> invoke git-commit with an appropriate editor instead of using -F.
>
> [1]: https://lore.kernel.org/git/m0h6d3pphu.fsf@epic96565.epic.com/T/#u

You're correct that my interest in the issue was strictly due to the
annoyance of git-gui failing to strip comments (in particular, the
list of conflicted files automatically inserted into
.git/MERGE_MSG)[*]. The subject of patch scissors did not come up in
the linked discussions, and it wasn't apparent from Brian's message
which started this thread that he was also concerned about patch
scissors (his message mentioned only comments).

I responded separately to the message you cited above.

[*]: https://lore.kernel.org/git/CAPig+cTQaPTNnGcd583B=xoVUR1qPb372Y_x9szROfMcA5h+tA@mail.gmail.com/

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Johannes Sixt]

Am 05.07.24 um 23:47 schrieb Eric Sunshine:
> On Fri, Jul 5, 2024 at 4:56 PM Sean Allred <allred.sean@gmail.com> wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> On Fri, Jul 5, 2024 at 3:23 PM brianmlyles <brianmlyles@gmail.com> wrote:
>>>> - Have git-gui create the commit in a way that causes the message to be
>>>>   washed
>>>
>>> A patch to make git-gui strip comment lines had been previously
>>> applied[1,2], however, it badly broke git-gui when running with old
>>> Tcl versions, such as on macOS[3,4]. The breakage was not
>>> insurmountable, and a patch[5,6] was submitted to resolve it.
>>> Unfortunately, the then-maintainer of git-gui lost interest in the
>>> project about that point, thus left the issue hanging. Thus, to this
>>> day, git-gui still doesn't strip comment lines.
>>
>> There is a third option -- new plumbing in git (a la
>> git-interpret-trailers) to expose the logic of `cleanup_message`. This
>> comes with some nice flexibility, but introduces complexity around
>> transferring state (e.g. passed options to git-commit) that would
>> probably be best to avoid.
> 
> Could the cleanup_message() functionality be exposed as a new option
> of git-stripspace?
> 
>> I haven't looked super closely at the patches you've linked, Eric, but
>> it seems like those are specific to stripping comment characters. As
>> I've noted elsewhere[1], there's potentially more to strip than just
>> comments (like patch scissors). I suspect the only paths forward to
>> guarantee that message-washing happens would either be an option to
>> git-commit to explicitly enable it OR (probably preferred) have git-gui
>> invoke git-commit with an appropriate editor instead of using -F.
>>
>> [1]: https://lore.kernel.org/git/m0h6d3pphu.fsf@epic96565.epic.com/T/#u
> 
> You're correct that my interest in the issue was strictly due to the
> annoyance of git-gui failing to strip comments (in particular, the
> list of conflicted files automatically inserted into
> .git/MERGE_MSG)[*]. The subject of patch scissors did not come up in
> the linked discussions, and it wasn't apparent from Brian's message
> which started this thread that he was also concerned about patch
> scissors (his message mentioned only comments).
> 
> I responded separately to the message you cited above.
> 
> [*]: https://lore.kernel.org/git/CAPig+cTQaPTNnGcd583B=xoVUR1qPb372Y_x9szROfMcA5h+tA@mail.gmail.com/

Let's take a step back and ask why is there cruft in a commit message
that needs to be cleaned in the first place? It is because with the
command line `git commit` there is no side-channel that could
communicate the circumstances that lead up to a commit. This is not the
case in git gui. There are many instruments that can be used at the same
time that the commit message is authored; there is no reason to have a
list of conflicted files or the commit's patch text in the commit message.

My take-away is:

- The commit message that is entered in the edit box must appear in the
commit unmodified. There is no such concept as "comment lines" in git
gui's commit message edit box. The commit-msg hook can overrule
nevertheless as a means to enforce message hygiene, but otherwise the
user must have full authority.

- A commit message template and the MERGE_MSG file are populated in a
manner that is suitable for `git commit`, i.e. can (and do) contain
comment lines. It is, therefore, necessary to remove them when their
text is used to populate git gui's edit box.

I suggest that removing comment lines ("message-washing") should not
happen as a post-processing step, but as a preprocessing step when text
is gathered from particular sources that are known to contain
inessential cruft.

-- Hannes

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> My take-away is:
>
> - The commit message that is entered in the edit box must appear in the
> commit unmodified. There is no such concept as "comment lines" in git
> gui's commit message edit box. The commit-msg hook can overrule
> nevertheless as a means to enforce message hygiene, but otherwise the
> user must have full authority.
>
> - A commit message template and the MERGE_MSG file are populated in a
> manner that is suitable for `git commit`, i.e. can (and do) contain
> comment lines. It is, therefore, necessary to remove them when their
> text is used to populate git gui's edit box.
>
> I suggest that removing comment lines ("message-washing") should not
> happen as a post-processing step, but as a preprocessing step when text
> is gathered from particular sources that are known to contain
> inessential cruft.

I agree most of the things you said, but with one reservation.

There may be two classes of comments CLI "git commit" users would be
seeing, ones coming from the "git commit" itself that describe what
CLI "git commit" does (e.g., "lines starting with '#' are ignored",
"absolutely empty message buffer aborts the command"), and others
coming from project specific template and other mechanisms that
describe what the project expects (e.g., "please keep your lines
shorter than 72 columns").

I agree that it makes perfect sense not to show the former at all to
the end-user in git-gui UI, especially if git-gui does not ignore
lines starting with '#' or abort commit with an empty message.

I am not sure if it is safe to strip the latter out of user's view,
though.

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Johannes Sixt]

Am 06.07.24 um 20:15 schrieb Junio C Hamano:
> There may be two classes of comments CLI "git commit" users would be
> seeing, ones coming from the "git commit" itself that describe what
> CLI "git commit" does (e.g., "lines starting with '#' are ignored",
> "absolutely empty message buffer aborts the command"), and others
> coming from project specific template and other mechanisms that
> describe what the project expects (e.g., "please keep your lines
> shorter than 72 columns").
> 
> I agree that it makes perfect sense not to show the former at all to
> the end-user in git-gui UI, especially if git-gui does not ignore
> lines starting with '#' or abort commit with an empty message.
> 
> I am not sure if it is safe to strip the latter out of user's view,
> though.

I see your point. These two kinds of comments have different topics
(usage of the tool being used vs. project conventions concerning the
commit message itself).

It is easy to clean only MERGE_MSG to solve the annoyance caused by the
list of conflicted files. We could have this a first step, and then we
can consider later whether cleaning other sources is worth it. But it
would not help OP, where the comments come from the commit message template.

-- Hannes

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Brian Lyles]

Hi Johannes,

Johannes Sixt <j6t@kdbg.org> wrote:
> My take-away is:
>
> - The commit message that is entered in the edit box must appear in the
>   commit unmodified. There is no such concept as "comment lines" in git
>   gui's commit message edit box. The commit-msg hook can overrule
>   nevertheless as a means to enforce message hygiene, but otherwise the
>   user must have full authority.

Could you elaborate on why git-gui's commit message edit box should
behave differently than any other commit message editor? Why is there no
concept as "comment lines" in git-gui?

Johannes Sixt <j6t@kdbg.org> wrote:
> - A commit message template and the MERGE_MSG file are populated in a
> manner that is suitable for `git commit`, i.e. can (and do) contain
> comment lines. It is, therefore, necessary to remove them when their
> text is used to populate git gui's edit box.

> I suggest that removing comment lines ("message-washing") should not
> happen as a post-processing step, but as a preprocessing step when text
> is gathered from particular sources that are known to contain
> inessential cruft.

While I agree in theory that it would be ideal for git-gui to wash only
content from sources that are known to contain content meant to be
washed, but I don't think that's possible since git-gui can't possibly
know *why* a given line appears in the message, in particular when
running the prepare-commit-msg hook.

I think that whatever path forward is taken, it needs to be predictable
and consistent with normal `git commit` behaviors. I think that's the
root problem here in my mind: From the perspective of the
prepare-commit-msg hook, it's impossible to do the right thing because
git-gui is invoking the hook consistent with normal `git commit`
behaviors, but then creating the commit with `git commit -F` behaviors.
This is an inconsistency with git-gui specifically.

So it still seems like we have two real options:

- Start washing the message, allowing the prepare-commit-msg hook to
  provide template-like guidance to the user regardless of if they are
  using git-gui or some other editor, or
- Pass the "message" argument along to the prepare-commit-msg hook so
  that it can at least avoid adding template-like content (but of course
  then lose the value added by that template).

The former seems most intuitive to me, though I have admittedly little
context for git-gui. Hopefully the elaboration I requested further up in
this message will shed some light things if you still disagree with
washing the message.

I'm certainly open to other ideas as well so long as they allow the hook
author the ability to add comments when the message will be washed and
not add comments when it won't be washed, regardless of whether git-gui
is in use.

-- 
Thank you,
Brian Lyles

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Johannes Sixt]

Am 08.07.24 um 21:29 schrieb Brian Lyles:
> Could you elaborate on why git-gui's commit message edit box should
> behave differently than any other commit message editor? Why is there no
> concept as "comment lines" in git-gui?

First of all, Git GUI is not a commit message editor, not even in its
git citool incarnation. You cannot instruct git commit to use it as
message editor.

Consider the commit message that git commit presents in the editor. It
contains the message text, instructions about how to use the tool, a
list of files, and sometimes even patch text.

Git GUI does that, too: There is the part where the message is entered,
there is a list or two of files, and there is patch text. (OK, there are
no instructions.) What the user writes into the part for the message
text must go into the commit. Except that the git commit's message
editor has a limitation: it can't tell the subsequent post processing
with absolute certainty which text is message text due to the possible
comment lines. Git GUI can offer this certainty because its
corresponding section is a dedicated text edit box.

> I think that whatever path forward is taken, it needs to be predictable
> and consistent with normal `git commit` behaviors. I think that's the
> root problem here in my mind: From the perspective of the
> prepare-commit-msg hook, it's impossible to do the right thing because
> git-gui is invoking the hook consistent with normal `git commit`
> behaviors, but then creating the commit with `git commit -F` behaviors.
> This is an inconsistency with git-gui specifically.

Good that you point that out. Git GUI does the wrong thing here. It
should really request the form corresponding to git commit -F. The
second option that you suggest looks correct to me:

> So it still seems like we have two real options:
> 
> - Start washing the message, allowing the prepare-commit-msg hook to
>   provide template-like guidance to the user regardless of if they are
>   using git-gui or some other editor, or
> - Pass the "message" argument along to the prepare-commit-msg hook so
>   that it can at least avoid adding template-like content (but of course
>   then lose the value added by that template).

-- Hannes

Re: [BUG REPORT] git-gui invokes prepare-commit-msg hook incorrectly

[Oswald Buddenhagen]

On Mon, Jul 08, 2024 at 10:40:19PM +0200, Johannes Sixt wrote:
>Am 08.07.24 um 21:29 schrieb Brian Lyles:
>> Could you elaborate on why git-gui's commit message edit box should
>> behave differently than any other commit message editor? Why is there no
>> concept as "comment lines" in git-gui?
>
>First of all, Git GUI is not a commit message editor, not even in its
>git citool incarnation. You cannot instruct git commit to use it as
>message editor.
>
nobody suggested that.

>Consider the commit message that git commit presents in the editor. It
>contains the message text, instructions about how to use the tool, a
>list of files, and sometimes even patch text.
>
>Git GUI does that, too: There is the part where the message is entered,
>there is a list or two of files, and there is patch text. (OK, there are
>no instructions.) What the user writes into the part for the message
>text must go into the commit. Except that the git commit's message
>editor has a limitation: it can't tell the subsequent post processing
>with absolute certainty which text is message text due to the possible
>comment lines.
>
>Git GUI can offer this certainty because its
>corresponding section is a dedicated text edit box.
>
no, it can't, as others already pointed out. the attempt to structure
the info is woefully incomplete, pretty much inherently. the text-based
workflow is just "too core" to have interactive frontends deviate from
it. not presenting and interpreting the text as "real" git would will
always be a source of problems, regardless of how many workarounds are
added.

i'll note that the qt creator ide as an example of a git frontend does
strip the message.

>> I think that whatever path forward is taken, it needs to be predictable
>> and consistent with normal `git commit` behaviors. I think that's the
>> root problem here in my mind: From the perspective of the
>> prepare-commit-msg hook, it's impossible to do the right thing because
>> git-gui is invoking the hook consistent with normal `git commit`
>> behaviors, but then creating the commit with `git commit -F` behaviors.
>> This is an inconsistency with git-gui specifically.
>
>Good that you point that out. Git GUI does the wrong thing here. It
>should really request the form corresponding to git commit -F. The
>second option that you suggest looks correct to me:
>
firstly, there is no parameter which would actually tell it whether the
message will be stripped. the 'message' token is unreliable for this
purpose, as -F merely imposes a default on [-no]-edit and thereby
--cleanup.

secondly, it seems a bit optimistic to expect that the hook would
actually implement different output modes.

>> So it still seems like we have two real options:
>>
>> - Start washing the message, allowing the prepare-commit-msg hook to
>>   provide template-like guidance to the user regardless of if they are
>>   using git-gui or some other editor, or
>> - Pass the "message" argument along to the prepare-commit-msg hook so
>>   that it can at least avoid adding template-like content (but of course
>>   then lose the value added by that template).
>
i'm strongly in favor of the first option.
it also seems to be the much easier one to implement.