Re: [PATCH v2 0/2] sequencer: remove use of hardcoded comment char

> Hi,
> 
> On Mon, Oct 30, 2023 at 10:09 PM Tony Tung via GitGitGadget
> <gitgitgadget@gmail.com> 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@gmail.com
> $ git am ./v2_20231031_gitgitgadget_sequencer_remove_use_of_hardcoded_comment_char.mbx
> 
> $ git grep -c -e '".*#' -e "'#'" -- sequencer.c
> sequencer.c:12

> 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@mail.gmail.com):
> 
> """
> 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 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.