On Tue, Oct 31, 2023 at 4:18 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote: > > 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. Ah, that might be jogging my memory slightly. I had a patch to put a comment before the one-line commit summaries in the TODO list (https://github.com/git/git/commit/f1ae608477e010b96557d6fc87eed9f3f39b905e). I think I at some point noticed comment_line_char, and went to switch to it, probably also switching the mid-line comment char for merges, and then noticed the potential for breakage due to the manual parsing of those. Anyway, I trust your analysis, but I believe some of that analysis belongs in the relevant commit messages if we push forward with these changes. > > 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) Thanks for digging up the old commit and the explicit mention of the TODO file. Kind of disappointing. While I can't imagine anything that would actually break by reverting this, it's not worth it at this point. > > [...] > > 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. That would seem more logical to me.
