{"thread":{"id":"60446","subject":"[PATCH] sequencer: remove use of comment character","startedAt":"2023-10-30T03:08:17Z","lastAt":"2023-11-01T04:59:25Z","messageCount":18,"participants":["Tony Tung via GitGitGadget","Junio C Hamano","Elijah Newren","Tony Tung","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484069","messageId":"pull.1603.git.1698635292629.gitgitgadget@gmail.com","threadId":"60446","inReplyTo":null,"subject":"[PATCH] sequencer: remove use of comment character","fromName":"Tony Tung via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-30T03:08:12Z","receivedAt":"2023-10-30T03:08:17Z","isPatch":true,"sender":{"key":"tonytung@merly.org","avatar":null},"body":"From: Tony Tung <tonytung@merly.org>\n\nInstead of using the hardcoded `# `, use the\nuser-defined comment_line_char.  Adds a test\nto prevent regressions.\n\nSigned-off-by: Tony Tung <tonytung@merly.org>\n---\n    sequencer: remove use of hardcoded comment char\n    \n    Instead of using the hardcoded # , use the user-defined\n    comment_line_char. Adds a test to prevent regressions.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1603%2Fttung%2Fttung%2Fcommentchar-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1603/ttung/ttung/commentchar-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1603\n\n sequencer.c                   |  5 +++--\n t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed9..8c6666d5e43 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6082,8 +6082,9 @@ static int add_decorations_to_list(const struct commit *commit,\n \t\t/* If the branch is checked out, then leave a comment instead. */\n \t\tif ((path = branch_checked_out(decoration->name))) {\n \t\t\titem->command = TODO_COMMENT;\n-\t\t\tstrbuf_addf(ctx->buf, \"# Ref %s checked out at '%s'\\n\",\n-\t\t\t\t    decoration->name, path);\n+\t\t\tstrbuf_commented_addf(ctx->buf, comment_line_char,\n+\t\t\t\t\t      \"Ref %s checked out at '%s'\\n\",\n+\t\t\t\t\t      decoration->name, path);\n \t\t} else {\n \t\t\tstruct string_list_item *sti;\n \t\t\titem->command = TODO_UPDATE_REF;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 8ea2bf13026..076dca87871 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' '\n \t)\n '\n \n+test_expect_success '--update-refs works with core.commentChar' '\n+\tgit checkout -b update-refs-with-commentchar no-conflict-branch &&\n+\ttest_config core.commentChar : &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit commit --allow-empty --fixup=third &&\n+\tgit branch -f is-not-reordered &&\n+\tgit commit --allow-empty --fixup=HEAD~4 &&\n+\tgit branch -f shared-tip &&\n+\tgit checkout update-refs &&\n+\t(\n+\t\twrite_script fake-editor.sh <<-\\EOF &&\n+\t\tgrep \"^[^:]\" \"$1\"\n+\t\texit 1\n+\t\tEOF\n+\t\ttest_set_editor \"$(pwd)/fake-editor.sh\" &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\tpick $(git log -1 --format=%h J) J\n+\t\tfixup $(git log -1 --format=%h update-refs) fixup! J : empty\n+\t\tupdate-ref refs/heads/second\n+\t\tupdate-ref refs/heads/first\n+\t\tpick $(git log -1 --format=%h K) K\n+\t\tpick $(git log -1 --format=%h L) L\n+\t\tfixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty\n+\t\tupdate-ref refs/heads/third\n+\t\tpick $(git log -1 --format=%h M) M\n+\t\tupdate-ref refs/heads/no-conflict-branch\n+\t\tupdate-ref refs/heads/is-not-reordered\n+\t\tupdate-ref refs/heads/update-refs-with-commentchar\n+\t\tEOF\n+\n+\t\ttest_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo &&\n+\t\ttest_cmp expect todo\n+\t)\n+'\n+\n test_expect_success '--update-refs adds commands with --rebase-merges' '\n \tgit checkout -b update-refs-with-merge no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n\nbase-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14\n-- \ngitgitgadget\n"},{"id":"484070","messageId":"xmqq7cn4g3nx.fsf@gitster.g","threadId":"60446","inReplyTo":"pull.1603.git.1698635292629.gitgitgadget@gmail.com","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-30T04:00:50Z","receivedAt":"2023-10-30T04:00:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tony Tung via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Tony Tung <tonytung@merly.org>\n>\n> Instead of using the hardcoded `# `, use the\n> user-defined comment_line_char.  Adds a test\n> to prevent regressions.\n\nGood spotting.\n\nTwo observations.\n\n (1) There are a few more places that need similar treatment in the\n     same file; you may want to fix them all while at it.\n\n (2) The second argument to strbuf_commented_addf() is always the\n     comment_line_char global variable, not just inside this file\n     but all callers across the codebase.  We probably should drop\n     it and have the strbuf_commented_addf() helper itself refer to\n     the global.  That way, if we ever want to change the global\n     variable reference to something else (e.g. function call), we\n     only have to touch a single place.\n\nThe latter is meant as #leftoverbits and will be a lot wider\nclean-up that we may want to do long after this patch hits out\ncodebase.  The \"other places\" I spotted for the former are the\nfollowing, but needs to be taken with a huge grain of salt, as it\nhas not even been compile tested.\n\nThanks.\n\n sequencer.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git c/sequencer.c w/sequencer.c\nindex d584cac8ed..33208b1660 100644\n--- c/sequencer.c\n+++ w/sequencer.c\n@@ -1893,8 +1893,8 @@ static void update_squash_message_for_fixup(struct strbuf *msg)\n \tsize_t orig_msg_len;\n \tint i = 1;\n \n-\tstrbuf_addf(&buf1, \"# %s\\n\", _(first_commit_msg_str));\n-\tstrbuf_addf(&buf2, \"# %s\\n\", _(skip_first_commit_msg_str));\n+\tstrbuf_addf(&buf1, comment_line_char, \"%s\\n\", _(first_commit_msg_str));\n+\tstrbuf_addf(&buf2, comment_line_char, \"%s\\n\", _(skip_first_commit_msg_str));\n \ts = start = orig_msg = strbuf_detach(msg, &orig_msg_len);\n \twhile (s) {\n \t\tconst char *next;\n@@ -2269,8 +2269,8 @@ static int do_pick_commit(struct repository *r,\n \t\tnext = parent;\n \t\tnext_label = msg.parent_label;\n \t\tif (opts->commit_use_reference) {\n-\t\t\tstrbuf_addstr(&msgbuf,\n-\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n+\t\t\tstrbuf_commented_addf(&msgbuf, comment_line_char, \"%s\",\n+\t\t\t\t\"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n \t\t} else if (skip_prefix(msg.subject, \"Revert \\\"\", &orig_subject) &&\n \t\t\t   /*\n \t\t\t    * We don't touch pre-existing repeated reverts, because\n"},{"id":"484133","messageId":"CABPp-BE6_nuMeiqOAMGwP8SH=d1+i57-STgTNKU8-Gnkv2jW=Q@mail.gmail.com","threadId":"60446","inReplyTo":"xmqq7cn4g3nx.fsf@gitster.g","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-10-30T17:26:18Z","receivedAt":"2023-10-30T17:26:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Oct 29, 2023 at 9:01 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Tony Tung via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Tony Tung <tonytung@merly.org>\n> >\n> > Instead of using the hardcoded `# `, use the\n> > user-defined comment_line_char.  Adds a test\n> > to prevent regressions.\n>\n> Good spotting.\n>\n> Two observations.\n>\n>  (1) There are a few more places that need similar treatment in the\n>      same file; you may want to fix them all while at it.\n>\n>  (2) The second argument to strbuf_commented_addf() is always the\n>      comment_line_char global variable, not just inside this file\n>      but all callers across the codebase.  We probably should drop\n>      it and have the strbuf_commented_addf() helper itself refer to\n>      the global.  That way, if we ever want to change the global\n>      variable reference to something else (e.g. function call), we\n>      only have to touch a single place.\n>\n> The latter is meant as #leftoverbits and will be a lot wider\n> clean-up that we may want to do long after this patch hits out\n> codebase.  The \"other places\" I spotted for the former are the\n> following, but needs to be taken with a huge grain of salt, as it\n> has not even been compile tested.\n>\n> Thanks.\n>\n>  sequencer.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git c/sequencer.c w/sequencer.c\n> index d584cac8ed..33208b1660 100644\n> --- c/sequencer.c\n> +++ w/sequencer.c\n> @@ -1893,8 +1893,8 @@ static void update_squash_message_for_fixup(struct strbuf *msg)\n>         size_t orig_msg_len;\n>         int i = 1;\n>\n> -       strbuf_addf(&buf1, \"# %s\\n\", _(first_commit_msg_str));\n> -       strbuf_addf(&buf2, \"# %s\\n\", _(skip_first_commit_msg_str));\n> +       strbuf_addf(&buf1, comment_line_char, \"%s\\n\", _(first_commit_msg_str));\n> +       strbuf_addf(&buf2, comment_line_char, \"%s\\n\", _(skip_first_commit_msg_str));\n>         s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);\n>         while (s) {\n>                 const char *next;\n> @@ -2269,8 +2269,8 @@ static int do_pick_commit(struct repository *r,\n>                 next = parent;\n>                 next_label = msg.parent_label;\n>                 if (opts->commit_use_reference) {\n> -                       strbuf_addstr(&msgbuf,\n> -                               \"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +                       strbuf_commented_addf(&msgbuf, comment_line_char, \"%s\",\n> +                               \"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n>                 } else if (skip_prefix(msg.subject, \"Revert \\\"\", &orig_subject) &&\n>                            /*\n>                             * We don't touch pre-existing repeated reverts, because\n>\n\nI thought the point of the comment_line_char was so that commit\nmessages could have lines starting with '#'.  That rationale doesn't\napply to the TODO list generation or parsing, and I'm not sure if we\nwant to add the same complexity there.  If we do want to add the same\ncomplexity there, I'm worried that making these changes are\ninsufficent; there are some other hardcoded '#' references in the code\n(as a quick greps for '\".*#' and \"'#'\" will turn up).  Since those\nother references include parsing as well as generation, I think we\nmight actually be introducing bugs in the TODO list handling if we\nonly partially convert it, but someone would need to double check.\n"},{"id":"484165","messageId":"xmqqa5rzadlh.fsf@gitster.g","threadId":"60446","inReplyTo":"CABPp-BE6_nuMeiqOAMGwP8SH=d1+i57-STgTNKU8-Gnkv2jW=Q@mail.gmail.com","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-30T23:35:06Z","receivedAt":"2023-10-30T23:35:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> I thought the point of the comment_line_char was so that commit\n> messages could have lines starting with '#'.  That rationale doesn't\n> apply to the TODO list generation or parsing, and I'm not sure if we\n> want to add the same complexity there.\n\nThanks for a healthy dose of sanity.  I noticed existing use of\ncomment_line_char everywhere in sequencer.c and assumed we would\nwant to be consistent, but you are right to point out that they are\nall about the COMMIT_EDITMSG kind of thing, and not about what\nappears in \"sequencer/todo\".\n"},{"id":"484179","messageId":"5C268049-561B-4E1A-AD6A-493BCD4F2351@merly.org","threadId":"60446","inReplyTo":"xmqqa5rzadlh.fsf@gitster.g","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Tony Tung","fromEmail":"tonytung@merly.org","sentAt":"2023-10-31T04:42:45Z","receivedAt":"2023-10-31T04:43:06Z","isPatch":true,"sender":{"key":"tonytung@merly.org","avatar":null},"body":"\n> On Oct 30, 2023, at 4:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Elijah Newren <newren@gmail.com> writes:\n> \n>> I thought the point of the comment_line_char was so that commit\n>> messages could have lines starting with '#'.  That rationale doesn't\n>> apply to the TODO list generation or parsing, and I'm not sure if we\n>> want to add the same complexity there.\n> \n> Thanks for a healthy dose of sanity.  I noticed existing use of\n> comment_line_char everywhere in sequencer.c and assumed we would\n> want to be consistent, but you are right to point out that they are\n> all about the COMMIT_EDITMSG kind of thing, and not about what\n> appears in \"sequencer/todo”.\n\nI believe comment_line_char is being applied when the sequencer reads back the instructions, which is why I ran into this problem to begin with.\n\nIf the intent is not to apply it to the sequencer, then the bugfix is in the wrong place.\n\nThanks\n\n"},{"id":"484180","messageId":"10283034-816F-4184-A02B-100B2EADF31E@merly.org","threadId":"60446","inReplyTo":"xmqqa5rzadlh.fsf@gitster.g","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Tony Tung","fromEmail":"tonytung@merly.org","sentAt":"2023-10-31T04:50:11Z","receivedAt":"2023-10-31T04:50:26Z","isPatch":true,"sender":{"key":"tonytung@merly.org","avatar":null},"body":"\n> On Oct 30, 2023, at 4:35 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> Elijah Newren <newren@gmail.com> writes:\n> \n>> I thought the point of the comment_line_char was so that commit\n>> messages could have lines starting with '#'.  That rationale doesn't\n>> apply to the TODO list generation or parsing, and I'm not sure if we\n>> want to add the same complexity there.\n> \n> Thanks for a healthy dose of sanity.  I noticed existing use of\n> comment_line_char everywhere in sequencer.c and assumed we would\n> want to be consistent, but you are right to point out that they are\n> all about the COMMIT_EDITMSG kind of thing, and not about what\n> appears in \"sequencer/todo”.\n\nActually, I withdraw my previous comment.  comment_line_char is all over sequencer.c, and there are tests that say, \"rebase -i respects core.commentchar”, which strongly implies that the *intent* is that rebase -i respects comment_line_char.\n\nI would propose that we move forward with this, except perhaps removing more instances of comment_line_char.\n\nThanks.\n\n\n\n"},{"id":"484182","messageId":"pull.1603.v2.git.1698728952.gitgitgadget@gmail.com","threadId":"60446","inReplyTo":"pull.1603.git.1698635292629.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] sequencer: remove use of hardcoded comment char","fromName":"Tony Tung via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-31T05:09:10Z","receivedAt":"2023-10-31T05:09:19Z","isPatch":true,"sender":{"key":"tonytung@merly.org","avatar":null},"body":"Instead of using the hardcoded # , use the user-defined comment_line_char.\nAdds a test to prevent regressions.\n\nTony Tung (2):\n  sequencer: remove use of comment character\n  sequencer: fix remaining hardcoded comment char\n\n sequencer.c                   | 21 +++++++++++++------\n t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 6 deletions(-)\n\n\nbase-commit: 2e8e77cbac8ac17f94eee2087187fa1718e38b14\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1603%2Fttung%2Fttung%2Fcommentchar-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1603/ttung/ttung/commentchar-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1603\n\nRange-diff vs v1:\n\n 1:  10598a56d64 = 1:  10598a56d64 sequencer: remove use of comment character\n -:  ----------- > 2:  c9f4ff34dbd sequencer: fix remaining hardcoded comment char\n\n-- \ngitgitgadget\n"},{"id":"484183","messageId":"10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com","threadId":"60446","inReplyTo":"pull.1603.v2.git.1698728952.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] sequencer: remove use of comment character","fromName":"Tony Tung via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-31T05:09:11Z","receivedAt":"2023-10-31T05:09:24Z","isPatch":true,"sender":{"key":"tonytung@merly.org","avatar":null},"body":"From: Tony Tung <tonytung@merly.org>\n\nInstead of using the hardcoded `# `, use the\nuser-defined comment_line_char.  Adds a test\nto prevent regressions.\n\nSigned-off-by: Tony Tung <tonytung@merly.org>\n---\n sequencer.c                   |  5 +++--\n t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d584cac8ed9..8c6666d5e43 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6082,8 +6082,9 @@ static int add_decorations_to_list(const struct commit *commit,\n \t\t/* If the branch is checked out, then leave a comment instead. */\n \t\tif ((path = branch_checked_out(decoration->name))) {\n \t\t\titem->command = TODO_COMMENT;\n-\t\t\tstrbuf_addf(ctx->buf, \"# Ref %s checked out at '%s'\\n\",\n-\t\t\t\t    decoration->name, path);\n+\t\t\tstrbuf_commented_addf(ctx->buf, comment_line_char,\n+\t\t\t\t\t      \"Ref %s checked out at '%s'\\n\",\n+\t\t\t\t\t      decoration->name, path);\n \t\t} else {\n \t\t\tstruct string_list_item *sti;\n \t\t\titem->command = TODO_UPDATE_REF;\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 8ea2bf13026..076dca87871 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' '\n \t)\n '\n \n+test_expect_success '--update-refs works with core.commentChar' '\n+\tgit checkout -b update-refs-with-commentchar no-conflict-branch &&\n+\ttest_config core.commentChar : &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit commit --allow-empty --fixup=third &&\n+\tgit branch -f is-not-reordered &&\n+\tgit commit --allow-empty --fixup=HEAD~4 &&\n+\tgit branch -f shared-tip &&\n+\tgit checkout update-refs &&\n+\t(\n+\t\twrite_script fake-editor.sh <<-\\EOF &&\n+\t\tgrep \"^[^:]\" \"$1\"\n+\t\texit 1\n+\t\tEOF\n+\t\ttest_set_editor \"$(pwd)/fake-editor.sh\" &&\n+\n+\t\tcat >expect <<-EOF &&\n+\t\tpick $(git log -1 --format=%h J) J\n+\t\tfixup $(git log -1 --format=%h update-refs) fixup! J : empty\n+\t\tupdate-ref refs/heads/second\n+\t\tupdate-ref refs/heads/first\n+\t\tpick $(git log -1 --format=%h K) K\n+\t\tpick $(git log -1 --format=%h L) L\n+\t\tfixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty\n+\t\tupdate-ref refs/heads/third\n+\t\tpick $(git log -1 --format=%h M) M\n+\t\tupdate-ref refs/heads/no-conflict-branch\n+\t\tupdate-ref refs/heads/is-not-reordered\n+\t\tupdate-ref refs/heads/update-refs-with-commentchar\n+\t\tEOF\n+\n+\t\ttest_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo &&\n+\t\ttest_cmp expect todo\n+\t)\n+'\n+\n test_expect_success '--update-refs adds commands with --rebase-merges' '\n \tgit checkout -b update-refs-with-merge no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n-- \ngitgitgadget\n\n"},{"id":"484184","messageId":"c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com","threadId":"60446","inReplyTo":"pull.1603.v2.git.1698728952.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] sequencer: fix remaining hardcoded comment char","fromName":"Tony Tung via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-10-31T05:09:12Z","receivedAt":"2023-10-31T05:09:28Z","isPatch":true,"sender":{"key":"tonytung@merly.org","avatar":null},"body":"From: Tony Tung <tonytung@merly.org>\n\nSigned-off-by: Tony Tung <tonytung@merly.org>\n---\n sequencer.c | 18 +++++++++++++-----\n 1 file changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 8c6666d5e43..bbadc3fb710 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1893,8 +1893,14 @@ static void update_squash_message_for_fixup(struct strbuf *msg)\n \tsize_t orig_msg_len;\n \tint i = 1;\n \n-\tstrbuf_addf(&buf1, \"# %s\\n\", _(first_commit_msg_str));\n-\tstrbuf_addf(&buf2, \"# %s\\n\", _(skip_first_commit_msg_str));\n+\tstrbuf_commented_addf(&buf1,\n+\t\t\t      comment_line_char,\n+\t\t\t      \"%s\\n\",\n+\t\t\t      _(first_commit_msg_str));\n+\tstrbuf_commented_addf(&buf2,\n+\t\t\t      comment_line_char,\n+\t\t\t      \"%s\\n\",\n+\t\t\t      _(skip_first_commit_msg_str));\n \ts = start = orig_msg = strbuf_detach(msg, &orig_msg_len);\n \twhile (s) {\n \t\tconst char *next;\n@@ -2269,8 +2275,9 @@ static int do_pick_commit(struct repository *r,\n \t\tnext = parent;\n \t\tnext_label = msg.parent_label;\n \t\tif (opts->commit_use_reference) {\n-\t\t\tstrbuf_addstr(&msgbuf,\n-\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n+\t\t\tstrbuf_commented_addf(&msgbuf,\n+\t\t\t\t\t      \"%s\",\n+\t\t\t\t\t      \"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n \t\t} else if (skip_prefix(msg.subject, \"Revert \\\"\", &orig_subject) &&\n \t\t\t   /*\n \t\t\t    * We don't touch pre-existing repeated reverts, because\n@@ -6082,7 +6089,8 @@ static int add_decorations_to_list(const struct commit *commit,\n \t\t/* If the branch is checked out, then leave a comment instead. */\n \t\tif ((path = branch_checked_out(decoration->name))) {\n \t\t\titem->command = TODO_COMMENT;\n-\t\t\tstrbuf_commented_addf(ctx->buf, comment_line_char,\n+\t\t\tstrbuf_commented_addf(ctx->buf,\n+\t\t\t\t\t      comment_line_char,\n \t\t\t\t\t      \"Ref %s checked out at '%s'\\n\",\n \t\t\t\t\t      decoration->name, path);\n \t\t} else {\n-- \ngitgitgadget\n"},{"id":"484190","messageId":"xmqqcywv4ar2.fsf@gitster.g","threadId":"60446","inReplyTo":"xmqqa5rzadlh.fsf@gitster.g","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-31T05:33:05Z","receivedAt":"2023-10-31T05:33:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Elijah Newren <newren@gmail.com> writes:\n>\n>> I thought the point of the comment_line_char was so that commit\n>> messages could have lines starting with '#'.  That rationale doesn't\n>> apply to the TODO list generation or parsing, and I'm not sure if we\n>> want to add the same complexity there.\n\nEarlier I said\n\n> Thanks for a healthy dose of sanity.  I noticed existing use of\n> comment_line_char everywhere in sequencer.c and assumed we would\n> want to be consistent, but you are right to point out that they are\n> all about the COMMIT_EDITMSG kind of thing, and not about what\n> appears in \"sequencer/todo\".\n\nbut with something as simple as\n\n    $ git -c core.commentchar='@' rebase -i master seen^2\n\nI can see that the references to comment_line_char in sequencer.c\nare about the commented lines after the list of insn in the\ngenerated sequencer/todo file, so even though the rationale does not\napply, isn't this already \"broken\" in the current code anyway?\n\nThanks.\n"},{"id":"484198","messageId":"CABPp-BFY7m_g+sT131_Ubxqo5FsHGKOPMng7=90_0-+xCS9NEQ@mail.gmail.com","threadId":"60446","inReplyTo":"xmqqcywv4ar2.fsf@gitster.g","subject":"Re: [PATCH] sequencer: remove use of comment character","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-10-31T06:20:37Z","receivedAt":"2023-10-31T06:20:57Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Oct 30, 2023 at 10:33 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Elijah Newren <newren@gmail.com> writes:\n> >\n> >> I thought the point of the comment_line_char was so that commit\n> >> messages could have lines starting with '#'.  That rationale doesn't\n> >> apply to the TODO list generation or parsing, and I'm not sure if we\n> >> want to add the same complexity there.\n>\n> Earlier I said\n>\n> > Thanks for a healthy dose of sanity.  I noticed existing use of\n> > comment_line_char everywhere in sequencer.c and assumed we would\n> > want to be consistent, but you are right to point out that they are\n> > all about the COMMIT_EDITMSG kind of thing, and not about what\n> > appears in \"sequencer/todo\".\n>\n> but with something as simple as\n>\n>     $ git -c core.commentchar='@' rebase -i master seen^2\n>\n> I can see that the references to comment_line_char in sequencer.c\n> are about the commented lines after the list of insn in the\n> generated sequencer/todo file, so even though the rationale does not\n> apply, isn't this already \"broken\" in the current code anyway?\n\nYes, I believe it is.  However, I remember specifically looking at\ncases with --rebase-merges about a year and a half ago, and noted that\nthere was a mixture of hardcoded '#' references along with\ncomment_line_char.  I noted at the time that changing\ncomment_line_char looked like it had a bug, and that the parsing in\nparticular would be fooled and do wrong things if it changed.\nUnfortunately, I can't find any notes from the time with the details,\nso I don't remember exactly what or how it was triggered.\n\nHowever, I do suspect that the references to comment_line_char in the\n`rebase -i` codepaths was not for any actual intended purpose, but\njust noting that they were used elsewhere in the file (for\nCOMMIT_EDITMSG, where it made sense) and just mimicking that code\nwithout realizing the lack of rationale.  That would have been mere\nwasted effort had the comment_line_char been consistently supported in\nthe TODO file editing and parsing, but it wasn't, which left TODO\nediting & parsing somewhat broken.\n\nI think supporting comment_line_char for the TODO file provides no\nvalue, and I think the easier fix would be undoing the uses of\ncomment_line_char relative to the TODO file (perhaps also leaving\nin-code comments to the effect that comment_line_char just doesn't\napply to the TODO file).\n\nHowever, if someone prefers to make the TODO file also respect\ncomment_line_char, despite its dubious value, then I expect any patch\nshould\n  1) audit *every* reference found via git grep -e '\".*#' -e \"'#'\" sequencer.c\n  2) add a test case (or cases) involving --rebase-merges -i that\ntrigger the relevant code paths\nIf they don't do that, then I fear we might make the bug more likely\nto be triggered rather than less.\n"},{"id":"484201","messageId":"CABPp-BEjV0H=waNQfKNNqibs3g_BU1CCrNjb8G8h_jXrt8kaiw@mail.gmail.com","threadId":"60446","inReplyTo":"pull.1603.v2.git.1698728952.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] sequencer: remove use of hardcoded comment char","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-10-31T06:55:07Z","receivedAt":"2023-10-31T06:55:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Mon, Oct 30, 2023 at 10:09 PM Tony Tung via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> Instead of using the hardcoded # , use the user-defined comment_line_char.\n> Adds a test to prevent regressions.\n>\n> Tony Tung (2):\n>   sequencer: remove use of comment character\n>   sequencer: fix remaining hardcoded comment char\n\nThe second commit message seems to suggest that the two commits should\njust be squashed; there's no explicit or even implicit reason provided\nfor why the two small patches are logically independent.  After\nreading them carefully, and digging through the particular changes\nbeing made and what part of the code they touch, I think I can guess\nat a potential reason, but I feel like I'm crossing into the territory\nof mind reading trying to articulate that reason.  (Besides, my\nrationale would argue that the two patches should be split\ndifferently.)  Perhaps a comment could be added, to either the second\ncommit message or the cover letter, to explain that better?\n\nMore importantly, though, I think the second commit message is\nactually wrong.  Before and after applying this series:\n\n$ git grep -c -e '\".*#' -e \"'#'\" -- sequencer.c\nsequencer.c:16\n\n$ b4 am c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com\n$ git am ./v2_20231031_gitgitgadget_sequencer_remove_use_of_hardcoded_comment_char.mbx\n\n$ git grep -c -e '\".*#' -e \"'#'\" -- sequencer.c\nsequencer.c:12\n\nGranted, four of those lines are code comments, but that still leaves\n8 hard coded references to '#' in the code at the end (i.e. the\nmajority are still left), meaning your second patch doesn't do what\nits subject line claims.\n\nAnd, most important of all is still the first patch.  As I stated\nelsewhere in this thread (at\nCABPp-BFY7m_g+sT131_Ubxqo5FsHGKOPMng7=90_0-+xCS9NEQ@mail.gmail.com):\n\n\"\"\"\nI think supporting comment_line_char for the TODO file provides no\nvalue, and I think the easier fix would be undoing the uses of\ncomment_line_char relative to the TODO file (perhaps also leaving\nin-code comments to the effect that comment_line_char just doesn't\napply to the TODO file).\n\nHowever, if someone prefers to make the TODO file also respect\ncomment_line_char, despite its dubious value, then I expect any patch\nshould\n  1) audit *every* reference found via git grep -e '\".*#' -e \"'#'\" sequencer.c\n  2) add a test case (or cases) involving --rebase-merges -i that\ntrigger the relevant code paths\nIf they don't do that, then I fear we might make the bug more likely\nto be triggered rather than less.\n\"\"\"\n\nPersonally, I would rather not accept patches changing the handling of\nthe TODO script relative to comment_line_char until the above is done,\nand I worry that half measures _might_ end up being more hurtful than\nhelpful.\n\nI feel quite differently about patches that make COMMIT_EDITMSG\nhandling use comment_line_char more consistently since that code\nsimply writes the file without re-parsing it; although fixing\neverything would be best, even fixing some of them to use\ncomment_line_char would be welcome.  I think the first two hunks of\nyour second patch happen to fall into this category, so if those were\nsplit out, then I'd say those are good partial solutions.\n"},{"id":"484234","messageId":"f59f75cd-1991-456a-9ea6-638b39711bf1@gmail.com","threadId":"60446","inReplyTo":"CABPp-BEjV0H=waNQfKNNqibs3g_BU1CCrNjb8G8h_jXrt8kaiw@mail.gmail.com","subject":"Re: [PATCH v2 0/2] sequencer: remove use of hardcoded comment char","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-10-31T11:18:10Z","receivedAt":"2023-10-31T11:18:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 31/10/2023 06:55, Elijah Newren wrote:\n> Hi,\n> \n> On Mon, Oct 30, 2023 at 10:09 PM Tony Tung via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n>>\n>> Instead of using the hardcoded # , use the user-defined comment_line_char.\n>> Adds a test to prevent regressions.\n>>\n>> Tony Tung (2):\n>>    sequencer: remove use of comment character\n>>    sequencer: fix remaining hardcoded comment char\n> \n> The second commit message seems to suggest that the two commits should\n> just be squashed; there's no explicit or even implicit reason provided\n> for why the two small patches are logically independent.  After\n> reading them carefully, and digging through the particular changes\n> being made and what part of the code they touch, I think I can guess\n> at a potential reason, but I feel like I'm crossing into the territory\n> of mind reading trying to articulate that reason.  (Besides, my\n> rationale would argue that the two patches should be split\n> differently.)  Perhaps a comment could be added, to either the second\n> commit message or the cover letter, to explain that better?\n> \n> More importantly, though, I think the second commit message is\n> actually wrong.  Before and after applying this series:\n> \n> $ git grep -c -e '\".*#' -e \"'#'\" -- sequencer.c\n> sequencer.c:16\n> \n> $ b4 am c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com\n> $ git am ./v2_20231031_gitgitgadget_sequencer_remove_use_of_hardcoded_comment_char.mbx\n> \n> $ git grep -c -e '\".*#' -e \"'#'\" -- sequencer.c\n> sequencer.c:12\n\nAs far as I can see those remaining instances are all to do with the '#' \nthat separates a merge subject line from its parents. I don't think we \nneed to complicate things anymore by respecting core.commentchar there \nas the '#' is not denoting a commented line, it is being used as an \nintra-line separator instead.\n\n> Granted, four of those lines are code comments, but that still leaves\n> 8 hard coded references to '#' in the code at the end (i.e. the\n> majority are still left), meaning your second patch doesn't do what\n> its subject line claims.\n> \n> And, most important of all is still the first patch.  As I stated\n> elsewhere in this thread (at\n> CABPp-BFY7m_g+sT131_Ubxqo5FsHGKOPMng7=90_0-+xCS9NEQ@mail.gmail.com):\n> \n> \"\"\"\n> I think supporting comment_line_char for the TODO file provides no\n> value, and I think the easier fix would be undoing the uses of\n> comment_line_char relative to the TODO file (perhaps also leaving\n> in-code comments to the effect that comment_line_char just doesn't\n> apply to the TODO file).\n\nI agree that I don't see much point in respecting core.commentchar in \nthe TODO file as unlike a commit message a legitimate non-commented line \nwill never begin with '#'. Unfortunately I think we're committed to \nrespecting it - see 180bad3d10f (rebase -i: respect core.commentchar, \n2013-02-11)\n\n> [...] \n> I feel quite differently about patches that make COMMIT_EDITMSG\n> handling use comment_line_char more consistently since that code\n> simply writes the file without re-parsing it; although fixing\n> everything would be best, even fixing some of them to use\n> comment_line_char would be welcome.  I think the first two hunks of\n> your second patch happen to fall into this category, so if those were\n> split out, then I'd say those are good partial solutions.\n\nI think splitting the changes so that we have one patch that fixes the \nTODO file generation and another that fixes the commit message \ngeneration for fixup commands would be best.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"484235","messageId":"2e0a9d0f-e801-42a5-aad8-f336bd023295@gmail.com","threadId":"60446","inReplyTo":"c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] sequencer: fix remaining hardcoded comment char","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-10-31T11:27:27Z","receivedAt":"2023-10-31T11:27:30Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Tony\n\nThanks for working on this. When you're submitting patches please try \ntesting them locally and check the CI before doing '/submit'. In this \ncase all the jobs that try to compile git are failing - see \nhttps://github.com/gitgitgadget/git/actions/runs/6702301267/job/18211090139?pr=1603#step:4:260 \nfor example.\n\nOn 31/10/2023 05:09, Tony Tung via GitGitGadget wrote:\n> From: Tony Tung <tonytung@merly.org>\n> \n> Signed-off-by: Tony Tung <tonytung@merly.org>\n> ---\n>   sequencer.c | 18 +++++++++++++-----\n>   1 file changed, 13 insertions(+), 5 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 8c6666d5e43..bbadc3fb710 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1893,8 +1893,14 @@ static void update_squash_message_for_fixup(struct strbuf *msg)\n>   \tsize_t orig_msg_len;\n>   \tint i = 1;\n>   \n> -\tstrbuf_addf(&buf1, \"# %s\\n\", _(first_commit_msg_str));\n> -\tstrbuf_addf(&buf2, \"# %s\\n\", _(skip_first_commit_msg_str));\n> +\tstrbuf_commented_addf(&buf1,\n> +\t\t\t      comment_line_char,\n> +\t\t\t      \"%s\\n\",\n> +\t\t\t      _(first_commit_msg_str));\n\nThis is what is causing the compilation the fail. It should be\n\n\tstrbuf_commented_addf(&buf1, \"%c $s\\n\", comment_char_line,\n\t\t\t      _(first_commit_msg_str));\n\n> +\tstrbuf_commented_addf(&buf2,\n> +\t\t\t      comment_line_char,\n> +\t\t\t      \"%s\\n\",\n> +\t\t\t      _(skip_first_commit_msg_str));\n\nThis needs changing in the same way.\n\nIt would be nice to add a test for this. I think we can do that by adding\n\n\ttest_config core.commentchar :\n\nTo the beginning of the test '--skip after failed fixup cleans commit \nmessage' in t3418-rebase-continue.sh and changing the expected message \nso that it uses ':' instead of '#' for the comments.\n\n>   \ts = start = orig_msg = strbuf_detach(msg, &orig_msg_len);\n>   \twhile (s) {\n>   \t\tconst char *next;\n> @@ -2269,8 +2275,9 @@ static int do_pick_commit(struct repository *r,\n>   \t\tnext = parent;\n>   \t\tnext_label = msg.parent_label;\n>   \t\tif (opts->commit_use_reference) {\n> -\t\t\tstrbuf_addstr(&msgbuf,\n> -\t\t\t\t\"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n> +\t\t\tstrbuf_commented_addf(&msgbuf,\n> +\t\t\t\t\t      \"%s\",\n> +\t\t\t\t\t      \"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***\");\n>   \t\t} else if (skip_prefix(msg.subject, \"Revert \\\"\", &orig_subject) &&\n>   \t\t\t   /*\n>   \t\t\t    * We don't touch pre-existing repeated reverts, because\n> @@ -6082,7 +6089,8 @@ static int add_decorations_to_list(const struct commit *commit,\n>   \t\t/* If the branch is checked out, then leave a comment instead. */\n>   \t\tif ((path = branch_checked_out(decoration->name))) {\n>   \t\t\titem->command = TODO_COMMENT;\n> -\t\t\tstrbuf_commented_addf(ctx->buf, comment_line_char,\n> +\t\t\tstrbuf_commented_addf(ctx->buf,\n> +\t\t\t\t\t      comment_line_char,\n>   \t\t\t\t\t      \"Ref %s checked out at '%s'\\n\",\n>   \t\t\t\t\t      decoration->name, path);\n\nThis hunk is unnecessary - it is just changing the code you added in the \nfirst patch.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"484236","messageId":"e37d2943-3cb3-4cf9-9f69-26fb42bf1027@gmail.com","threadId":"60446","inReplyTo":"10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: remove use of comment character","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-10-31T11:43:20Z","receivedAt":"2023-10-31T11:43:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Tony\n\nOn 31/10/2023 05:09, Tony Tung via GitGitGadget wrote:\n> From: Tony Tung <tonytung@merly.org>\n> \n> Instead of using the hardcoded `# `, use the\n> user-defined comment_line_char.  Adds a test\n> to prevent regressions.\n\nWell spotted and thanks for fixing this. Normally we wrap the commit \nmessage at ~72 chars.\n\n> Signed-off-by: Tony Tung <tonytung@merly.org>\n> ---\n>   sequencer.c                   |  5 +++--\n>   t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++\n>   2 files changed, 42 insertions(+), 2 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index d584cac8ed9..8c6666d5e43 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -6082,8 +6082,9 @@ static int add_decorations_to_list(const struct commit *commit,\n>   \t\t/* If the branch is checked out, then leave a comment instead. */\n>   \t\tif ((path = branch_checked_out(decoration->name))) {\n>   \t\t\titem->command = TODO_COMMENT;\n> -\t\t\tstrbuf_addf(ctx->buf, \"# Ref %s checked out at '%s'\\n\",\n> -\t\t\t\t    decoration->name, path);\n> +\t\t\tstrbuf_commented_addf(ctx->buf, comment_line_char,\n> +\t\t\t\t\t      \"Ref %s checked out at '%s'\\n\",\n> +\t\t\t\t\t      decoration->name, path);\n>   \t\t} else {\n>   \t\t\tstruct string_list_item *sti;\n>   \t\t\titem->command = TODO_UPDATE_REF;\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 8ea2bf13026..076dca87871 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' '\n>   \t)\n>   '\n\nThank you for taking the time to add a test. I think it could be \nsimplified though as all we really need to do is check that the expected \ncomment is present in the todo list. Something like (untested)\n\ntest_expect_success '--update-refs works with core.commentChar' '\n\tgit worktree add new-branch &&\n\ttest_when_finished \"git worktree remove new-branch\" &&\n\ttest_config core.commentchar : &&\n\twrite_script fake-editor.sh <<-\\EOF &&\n\tgrep \"^: Ref refs/heads/new-branch checked out at .*new-branch\" \"$1\" &&\n\t# no need to rebase\n\t>\"$1\"\n\tEOF\n\t(\n\t\ttest_set_editor \"$(pwd)/fake-editor.sh\" &&\n\t\tgit rebase -i --update-refs HEAD^\n\t)\n'\n\nBest Wishes\n\nPhillip\n\n> +test_expect_success '--update-refs works with core.commentChar' '\n> +\tgit checkout -b update-refs-with-commentchar no-conflict-branch &&\n> +\ttest_config core.commentChar : &&\n> +\tgit branch -f base HEAD~4 &&\n> +\tgit branch -f first HEAD~3 &&\n> +\tgit branch -f second HEAD~3 &&\n> +\tgit branch -f third HEAD~1 &&\n> +\tgit commit --allow-empty --fixup=third &&\n> +\tgit branch -f is-not-reordered &&\n> +\tgit commit --allow-empty --fixup=HEAD~4 &&\n> +\tgit branch -f shared-tip &&\n> +\tgit checkout update-refs &&\n> +\t(\n> +\t\twrite_script fake-editor.sh <<-\\EOF &&\n> +\t\tgrep \"^[^:]\" \"$1\"\n> +\t\texit 1\n> +\t\tEOF\n> +\t\ttest_set_editor \"$(pwd)/fake-editor.sh\" &&\n> +\n> +\t\tcat >expect <<-EOF &&\n> +\t\tpick $(git log -1 --format=%h J) J\n> +\t\tfixup $(git log -1 --format=%h update-refs) fixup! J : empty\n> +\t\tupdate-ref refs/heads/second\n> +\t\tupdate-ref refs/heads/first\n> +\t\tpick $(git log -1 --format=%h K) K\n> +\t\tpick $(git log -1 --format=%h L) L\n> +\t\tfixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty\n> +\t\tupdate-ref refs/heads/third\n> +\t\tpick $(git log -1 --format=%h M) M\n> +\t\tupdate-ref refs/heads/no-conflict-branch\n> +\t\tupdate-ref refs/heads/is-not-reordered\n> +\t\tupdate-ref refs/heads/update-refs-with-commentchar\n> +\t\tEOF\n> +\n> +\t\ttest_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo &&\n> +\t\ttest_cmp expect todo\n> +\t)\n> +'\n> +\n>   test_expect_success '--update-refs adds commands with --rebase-merges' '\n>   \tgit checkout -b update-refs-with-merge no-conflict-branch &&\n>   \tgit branch -f base HEAD~4 &&\n\n"},{"id":"484272","messageId":"xmqqo7gexr9a.fsf@gitster.g","threadId":"60446","inReplyTo":"f59f75cd-1991-456a-9ea6-638b39711bf1@gmail.com","subject":"Re: [PATCH v2 0/2] sequencer: remove use of hardcoded comment char","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-01T00:16:01Z","receivedAt":"2023-11-01T00:16:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> As far as I can see those remaining instances are all to do with the\n> '#' that separates a merge subject line from its parents. I don't\n> think we need to complicate things anymore by respecting\n> core.commentchar there as the '#' is not denoting a commented line, it\n> is being used as an intra-line separator instead.\n\nIt is unfortunate that the format of the file needs an intra-line\nseparator in the first place, but I tend to agree with you that the\ncomment-line-char would be a terrible fit there.  '#' or any\nreplacement character at the beginning of a line is easy to spot as\na signal that the line in its entirety is commented out, but it is\nmuch harder to eyeball-spot a single punctuation character in the\nmiddle of a line.  If we do not have to look for a different\ncharacter depending on the comment-line-char setting, it would make\nthe system (slightly) easier to use.\n\n> I agree that I don't see much point in respecting core.commentchar in\n> the TODO file as unlike a commit message a legitimate non-commented\n> line will never begin with '#'. Unfortunately I think we're committed\n> to respecting it - see 180bad3d10f (rebase -i: respect\n> core.commentchar, 2013-02-11)\n\nYeah, the ship has long sailed.\n\n> I think splitting the changes so that we have one patch that fixes the\n> TODO file generation and another that fixes the commit message\n> generation for fixup commands would be best.\n\nYes, it would be great.\n\nThanks.\n"},{"id":"484273","messageId":"CABPp-BFzY8S=tE7JHy0MUrW2R+X7=pcUBVdPbRC5CpEVv=G+pg@mail.gmail.com","threadId":"60446","inReplyTo":"f59f75cd-1991-456a-9ea6-638b39711bf1@gmail.com","subject":"Re: [PATCH v2 0/2] sequencer: remove use of hardcoded comment char","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-11-01T00:21:44Z","receivedAt":"2023-11-01T00:22:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Oct 31, 2023 at 4:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>\n> Hi Elijah\n>\n> On 31/10/2023 06:55, Elijah Newren wrote:\n> > Hi,\n> >\n> > On Mon, Oct 30, 2023 at 10:09 PM Tony Tung via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> >>\n> >> Instead of using the hardcoded # , use the user-defined comment_line_char.\n> >> Adds a test to prevent regressions.\n> >>\n> >> Tony Tung (2):\n> >>    sequencer: remove use of comment character\n> >>    sequencer: fix remaining hardcoded comment char\n> >\n> > The second commit message seems to suggest that the two commits should\n> > just be squashed; there's no explicit or even implicit reason provided\n> > for why the two small patches are logically independent.  After\n> > reading them carefully, and digging through the particular changes\n> > being made and what part of the code they touch, I think I can guess\n> > at a potential reason, but I feel like I'm crossing into the territory\n> > of mind reading trying to articulate that reason.  (Besides, my\n> > rationale would argue that the two patches should be split\n> > differently.)  Perhaps a comment could be added, to either the second\n> > commit message or the cover letter, to explain that better?\n> >\n> > More importantly, though, I think the second commit message is\n> > actually wrong.  Before and after applying this series:\n> >\n> > $ git grep -c -e '\".*#' -e \"'#'\" -- sequencer.c\n> > sequencer.c:16\n> >\n> > $ b4 am c9f4ff34dbdb7ba221e4203bb6551b80948dc71d.1698728953.git.gitgitgadget@gmail.com\n> > $ git am ./v2_20231031_gitgitgadget_sequencer_remove_use_of_hardcoded_comment_char.mbx\n> >\n> > $ git grep -c -e '\".*#' -e \"'#'\" -- sequencer.c\n> > sequencer.c:12\n>\n> As far as I can see those remaining instances are all to do with the '#'\n> that separates a merge subject line from its parents. I don't think we\n> need to complicate things anymore by respecting core.commentchar there\n> as the '#' is not denoting a commented line, it is being used as an\n> intra-line separator instead.\n\nAh, that might be jogging my memory slightly.  I had a patch to put a\ncomment before the one-line commit summaries in the TODO list\n(https://github.com/git/git/commit/f1ae608477e010b96557d6fc87eed9f3f39b905e).\nI think I at some point noticed comment_line_char, and went to switch\nto it, probably also switching the mid-line comment char for merges,\nand then noticed the potential for breakage due to the manual parsing\nof those.\n\nAnyway, I trust your analysis, but I believe some of that analysis\nbelongs in the relevant commit messages if we push forward with these\nchanges.\n\n> > Granted, four of those lines are code comments, but that still leaves\n> > 8 hard coded references to '#' in the code at the end (i.e. the\n> > majority are still left), meaning your second patch doesn't do what\n> > its subject line claims.\n> >\n> > And, most important of all is still the first patch.  As I stated\n> > elsewhere in this thread (at\n> > CABPp-BFY7m_g+sT131_Ubxqo5FsHGKOPMng7=90_0-+xCS9NEQ@mail.gmail.com):\n> >\n> > \"\"\"\n> > I think supporting comment_line_char for the TODO file provides no\n> > value, and I think the easier fix would be undoing the uses of\n> > comment_line_char relative to the TODO file (perhaps also leaving\n> > in-code comments to the effect that comment_line_char just doesn't\n> > apply to the TODO file).\n>\n> I agree that I don't see much point in respecting core.commentchar in\n> the TODO file as unlike a commit message a legitimate non-commented line\n> will never begin with '#'. Unfortunately I think we're committed to\n> respecting it - see 180bad3d10f (rebase -i: respect core.commentchar,\n> 2013-02-11)\n\nThanks for digging up the old commit and the explicit mention of the\nTODO file.  Kind of disappointing.  While I can't imagine anything\nthat would actually break by reverting this, it's not worth it at this\npoint.\n\n> > [...]\n> > I feel quite differently about patches that make COMMIT_EDITMSG\n> > handling use comment_line_char more consistently since that code\n> > simply writes the file without re-parsing it; although fixing\n> > everything would be best, even fixing some of them to use\n> > comment_line_char would be welcome.  I think the first two hunks of\n> > your second patch happen to fall into this category, so if those were\n> > split out, then I'd say those are good partial solutions.\n>\n> I think splitting the changes so that we have one patch that fixes the\n> TODO file generation and another that fixes the commit message\n> generation for fixup commands would be best.\n\nThat would seem more logical to me.\n"},{"id":"484284","messageId":"xmqqlebit6fx.fsf@gitster.g","threadId":"60446","inReplyTo":"10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] sequencer: remove use of comment character","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-01T04:59:14Z","receivedAt":"2023-11-01T04:59:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Tony Tung via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Subject: Re: [PATCH v2 1/2] sequencer: remove use of comment character\n\nThe patch does not seem to be doing that, though.  It may have\nremoved '#' in \"# Ref\", but still uses comment_line_char, so it does\nnot remove use at all (and we do not want to, of course).\n\n\"use the core.commentchar consistently\"\n\n> From: Tony Tung <tonytung@merly.org>\n>\n> Instead of using the hardcoded `# `, use the\n> user-defined comment_line_char.  Adds a test\n> to prevent regressions.\n\nOverly short lines.\n\nThe readers cannot tell where in the output the hardcoded # appears\nwith the above description. I am guessing that it is in comments in\nthe sequencer/todo file that mark commits that are at the tip of\nbranches that are checked out, but there may be more specific\ncircumstances in which the comment is used, like \"when rebase -i is\nused with the --update-refs option\", if so that also need to be told\nto the readers.\n\nDescribe the condition well enough so that readers can easily see\nthe defect the patch attempts to fix.\n\n> -\t\t\tstrbuf_addf(ctx->buf, \"# Ref %s checked out at '%s'\\n\",\n> -\t\t\t\t    decoration->name, path);\n> +\t\t\tstrbuf_commented_addf(ctx->buf, comment_line_char,\n> +\t\t\t\t\t      \"Ref %s checked out at '%s'\\n\",\n> +\t\t\t\t\t      decoration->name, path);\n\nOK.\n"}]}