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 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). However, if someone prefers to make the TODO file also respect comment_line_char, despite its dubious value, then I expect any patch should 1) audit *every* reference found via git grep -e '".*#' -e "'#'" sequencer.c 2) add a test case (or cases) involving --rebase-merges -i that trigger the relevant code paths If they don't do that, then I fear we might make the bug more likely to be triggered rather than less. """ Personally, I would rather not accept patches changing the handling of the TODO script relative to comment_line_char until the above is done, and I worry that half measures _might_ end up being more hurtful than helpful. 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.
