From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 31 Oct 2023 11:43:20 GMT
Subject: Re: [PATCH v2 1/2] sequencer: remove use of comment character
Message-ID: <e37d2943-3cb3-4cf9-9f69-26fb42bf1027@gmail.com>
In-Reply-To: <10598a56d64f5c2b4d8d05d7e7b09a18ef254f88.1698728953.git.gitgitgadget@gmail.com>

Hi Tony

On 31/10/2023 05:09, Tony Tung via GitGitGadget wrote:
> From: Tony Tung <tonytung@merly.org>
> 
> Instead of using the hardcoded `# `, use the
> user-defined comment_line_char.  Adds a test
> to prevent regressions.

Well spotted and thanks for fixing this. Normally we wrap the commit 
message at ~72 chars.

> Signed-off-by: Tony Tung <tonytung@merly.org>
> ---
>   sequencer.c                   |  5 +++--
>   t/t3404-rebase-interactive.sh | 39 +++++++++++++++++++++++++++++++++++
>   2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed9..8c6666d5e43 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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;
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 8ea2bf13026..076dca87871 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1839,6 +1839,45 @@ test_expect_success '--update-refs adds label and update-ref commands' '
>   	)
>   '

Thank you for taking the time to add a test. I think it could be 
simplified though as all we really need to do is check that the expected 
comment is present in the todo list. Something like (untested)

test_expect_success '--update-refs works with core.commentChar' '
	git worktree add new-branch &&
	test_when_finished "git worktree remove new-branch" &&
	test_config core.commentchar : &&
	write_script fake-editor.sh <<-\EOF &&
	grep "^: Ref refs/heads/new-branch checked out at .*new-branch" "$1" &&
	# no need to rebase
	>"$1"
	EOF
	(
		test_set_editor "$(pwd)/fake-editor.sh" &&
		git rebase -i --update-refs HEAD^
	)
'

Best Wishes

Phillip

> +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 &&