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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Am 05.07.24 um 23:47 schrieb Eric Sunshine:
> On Fri, Jul 5, 2024 at 4:56 PM Sean Allred <allred.sean@xxxxxxxxx> wrote:
>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>>> On Fri, Jul 5, 2024 at 3:23 PM brianmlyles <brianmlyles@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxx/

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





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux