sequencer: remove use of comment character

From: Tony Tung <[email protected]>

Instead of using the hardcoded # , use the
user-defined comment_line_char. Adds a test
to prevent regressions.

Quoted 5 lines · Tony Tung via GitGitGadget

From: Tony Tung <[email protected]>

Instead of using the hardcoded # , use the
user-defined comment_line_char. Adds a test
to prevent regressions.

Good spotting.

Two observations.

(1) There are a few more places that need similar treatment in the same file; you may want to fix them all while at it.

(2) The second argument to strbuf_commented_addf() is always the comment_line_char global variable, not just inside this file but all callers across the codebase. We probably should drop it and have the strbuf_commented_addf() helper itself refer to the global. That way, if we ever want to change the global variable reference to something else (e.g. function call), we only have to touch a single place.

The latter is meant as #leftoverbits and will be a lot wider clean-up that we may want to do long after this patch hits out codebase. The "other places" I spotted for the former are the following, but needs to be taken with a huge grain of salt, as it has not even been compile tested.

Thanks.

 sequencer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

sequencer.c +2 -2

index d584cac8ed..33208b1660 100644
@@ -1893,8 +1893,8 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
        size_t orig_msg_len;
        int i = 1;
 
-       strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
-       strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
+       strbuf_addf(&buf1, comment_line_char, "%s\n", _(first_commit_msg_str));
+       strbuf_addf(&buf2, comment_line_char, "%s\n", _(skip_first_commit_msg_str));
        s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
        while (s) {
                const char *next;

I thought the point of the comment_line_char was so that commit messages could have lines starting with '#'. That rationale doesn't apply to the TODO list generation or parsing, and I'm not sure if we want to add the same complexity there.

Quoted 4 lines · Elijah Newren

I thought the point of the comment_line_char was so that commit messages could have lines starting with '#'. That rationale doesn't apply to the TODO list generation or parsing, and I'm not sure if we want to add the same complexity there.

Thanks for a healthy dose of sanity. I noticed existing use of comment_line_char everywhere in sequencer.c and assumed we would want to be consistent, but you are right to point out that they are all about the COMMIT_EDITMSG kind of thing, and not about what appears in "sequencer/todo”.

Quoted 4 lines · Elijah Newren

I thought the point of the comment_line_char was so that commit messages could have lines starting with '#'. That rationale doesn't apply to the TODO list generation or parsing, and I'm not sure if we want to add the same complexity there.

Thanks for a healthy dose of sanity. I noticed existing use of comment_line_char everywhere in sequencer.c and assumed we would want to be consistent, but you are right to point out that they are all about the COMMIT_EDITMSG kind of thing, and not about what appears in "sequencer/todo”.

Quoted 4 lines · Elijah Newren

I thought the point of the comment_line_char was so that commit messages could have lines starting with '#'. That rationale doesn't apply to the TODO list generation or parsing, and I'm not sure if we want to add the same complexity there.

Thanks for a healthy dose of sanity. I noticed existing use of comment_line_char everywhere in sequencer.c and assumed we would want to be consistent, but you are right to point out that they are all about the COMMIT_EDITMSG kind of thing, and not about what appears in "sequencer/todo".

Quoted 6 lines · Junio C Hamano

Quoted 4 lines · Elijah Newren

I thought the point of the comment_line_char was so that commit messages could have lines starting with '#'. That rationale doesn't apply to the TODO list generation or parsing, and I'm not sure if we want to add the same complexity there.

Earlier I said

Quoted 5 lines

Thanks for a healthy dose of sanity. I noticed existing use of comment_line_char everywhere in sequencer.c and assumed we would want to be consistent, but you are right to point out that they are all about the COMMIT_EDITMSG kind of thing, and not about what appears in "sequencer/todo".

but with something as simple as

    $ git -c core.commentchar='@' rebase -i master seen^2

I can see that the references to comment_line_char in sequencer.c are about the commented lines after the list of insn in the generated sequencer/todo file, so even though the rationale does not apply, isn't this already "broken" in the current code anyway?

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

Hi,

Quoted 7 lines · Tony Tung via GitGitGadget

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.

From: Tony Tung <[email protected]>

Signed-off-by Tony Tung <[email protected]>

---
  sequencer.c | 18 +++++++++++++-----
  1 file changed, 13 insertions(+), 5 deletions(-)

sequencer.c +0 -0

index 8c6666d5e43..bbadc3fb710 100644

sequencer.c +3 -2

  	s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
  	while (s) {
  		const char *next;

@@ -2269,8 +2275,9 @@ static int do_pick_commit(struct repository *r,
  		next = parent;
  		next_label = msg.parent_label;
  		if (opts->commit_use_reference) {
-			strbuf_addstr(&msgbuf,
-				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+			strbuf_commented_addf(&msgbuf,
+					      "%s",
+					      "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
  		} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
  			   /*
  			    * We don't touch pre-existing repeated reverts, because

From: Tony Tung <[email protected]>

Instead of using the hardcoded # , use the
user-defined comment_line_char. Adds a test
to prevent regressions.

Signed-off-by Tony Tung <[email protected]>

---
  sequencer.c                   |  5 +++--
  t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++
  2 files changed, 42 insertions(+), 2 deletions(-)

sequencer.c +3 -2

index d584cac8ed9..8c6666d5e43 100644
@@ -6082,8 +6082,9 @@ static int add_decorations_to_list(const struct commit *commit,
  		/* If the branch is checked out, then leave a comment instead. */
  		if ((path = branch_checked_out(decoration->name))) {
  			item->command = TODO_COMMENT;
-			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
-				    decoration->name, path);
+			strbuf_commented_addf(ctx->buf, comment_line_char,
+					      "Ref %s checked out at '%s'\n",
+					      decoration->name, path);
  		} else {
  			struct string_list_item *sti;
  			item->command = TODO_UPDATE_REF;

t/t3404-rebase-interactive.sh +0 -0

index 8ea2bf13026..076dca87871 100755

t/t3404-rebase-interactive.sh +39 -0

+test_expect_success '--update-refs works with core.commentChar' '
+	git checkout -b update-refs-with-commentchar no-conflict-branch &&
+	test_config core.commentChar : &&
+	git branch -f base HEAD~4 &&
+	git branch -f first HEAD~3 &&
+	git branch -f second HEAD~3 &&
+	git branch -f third HEAD~1 &&
+	git commit --allow-empty --fixup=third &&
+	git branch -f is-not-reordered &&
+	git commit --allow-empty --fixup=HEAD~4 &&
+	git branch -f shared-tip &&
+	git checkout update-refs &&
+	(
+		write_script fake-editor.sh <<-\EOF &&
+		grep "^[^:]" "$1"
+		exit 1
+		EOF
+		test_set_editor "$(pwd)/fake-editor.sh" &&
+
+		cat >expect <<-EOF &&
+		pick $(git log -1 --format=%h J) J
+		fixup $(git log -1 --format=%h update-refs) fixup! J : empty
+		update-ref refs/heads/second
+		update-ref refs/heads/first
+		pick $(git log -1 --format=%h K) K
+		pick $(git log -1 --format=%h L) L
+		fixup $(git log -1 --format=%h is-not-reordered) fixup! L : empty
+		update-ref refs/heads/third
+		pick $(git log -1 --format=%h M) M
+		update-ref refs/heads/no-conflict-branch
+		update-ref refs/heads/is-not-reordered
+		update-ref refs/heads/update-refs-with-commentchar
+		EOF
+
+		test_must_fail git rebase -i --autosquash --update-refs primary shared-tip >todo &&
+		test_cmp expect todo
+	)
+'
+
  test_expect_success '--update-refs adds commands with --rebase-merges' '
  	git checkout -b update-refs-with-merge no-conflict-branch &&
  	git branch -f base HEAD~4 &&

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.

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 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.

Hi Elijah

Quoted 32 lines · Elijah Newren

Hi,

Quoted 7 lines · Tony Tung via GitGitGadget

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

Quoted 2 lines

$ 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.

Quoted 9 lines

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):

Quoted 6 lines

"""
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)

Quoted 8 lines

[...]
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.

Subject: Re: [PATCH v2 1/2] sequencer: remove use of comment character

From: Tony Tung <[email protected]>

Instead of using the hardcoded # , use the
user-defined comment_line_char. Adds a test
to prevent regressions.