Hi Elijah
On 31/10/2023 06:55, Elijah Newren wrote:
Hi,
On Mon, Oct 30, 2023 at 10:09 PM Tony Tung via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
Instead of using the hardcoded # , use the user-defined comment_line_char.
Adds a test to prevent regressions.
Tony Tung (2):
sequencer: remove use of comment character
sequencer: fix remaining hardcoded comment char
The second commit message seems to suggest that the two commits should
just be squashed; there's no explicit or even implicit reason provided
for why the two small patches are logically independent. After
reading them carefully, and digging through the particular changes
being made and what part of the code they touch, I think I can guess
at a potential reason, but I feel like I'm crossing into the territory
of mind reading trying to articulate that reason. (Besides, my
rationale would argue that the two patches should be split
differently.) Perhaps a comment could be added, to either the second
commit message or the cover letter, to explain that better?
More importantly, though, I think the second commit message is
actually wrong. Before and after applying this series:
$ git grep -c -e '".*#' -e "'#'" -- sequencer.c
sequencer.c:16
$ b4 am c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@xxxxxxxxx
$ git am ./v2_20231031_gitgitgadget_sequencer_remove_use_of_hardcoded_comment_char.mbx
$ git grep -c -e '".*#' -e "'#'" -- sequencer.c
sequencer.c:12
As far as I can see those remaining instances are all to do with the '#'
that separates a merge subject line from its parents. I don't think we
need to complicate things anymore by respecting core.commentchar there
as the '#' is not denoting a commented line, it is being used as an
intra-line separator instead.
Granted, four of those lines are code comments, but that still leaves
8 hard coded references to '#' in the code at the end (i.e. the
majority are still left), meaning your second patch doesn't do what
its subject line claims.
And, most important of all is still the first patch. As I stated
elsewhere in this thread (at
CABPp-BFY7m_g+sT131_Ubxqo5FsHGKOPMng7=90_0-+xCS9NEQ@xxxxxxxxxxxxxx):
"""
I think supporting comment_line_char for the TODO file provides no
value, and I think the easier fix would be undoing the uses of
comment_line_char relative to the TODO file (perhaps also leaving
in-code comments to the effect that comment_line_char just doesn't
apply to the TODO file).
I agree that I don't see much point in respecting core.commentchar in
the TODO file as unlike a commit message a legitimate non-commented line
will never begin with '#'. Unfortunately I think we're committed to
respecting it - see 180bad3d10f (rebase -i: respect core.commentchar,
2013-02-11)
[...]
I feel quite differently about patches that make COMMIT_EDITMSG
handling use comment_line_char more consistently since that code
simply writes the file without re-parsing it; although fixing
everything would be best, even fixing some of them to use
comment_line_char would be welcome. I think the first two hunks of
your second patch happen to fall into this category, so if those were
split out, then I'd say those are good partial solutions.
I think splitting the changes so that we have one patch that fixes the
TODO file generation and another that fixes the commit message
generation for fixup commands would be best.
Best Wishes
Phillip
