Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Nov 13, 2024, 14:48 UTC
Message-ID: <4c623fcf-01dd-4056-80c1-b3c860ab7f87@gmail.com>
In-Reply-To: <xmqqbjyk2b70.fsf@gitster.g>

On 13/11/2024 01:07, Junio C Hamano wrote:

Show 21 quoted lines

> kristofferhaugsbakk@fastmail.com writes:
> 
>> +test_expect_success 'git revert --reference with core.commentChar' '
>> +	test_when_finished "git reset --hard to-ident" &&
>> +	git checkout --detach to-ident &&
>> +	git -c core.commentChar=% revert \
>> +		--edit --reference HEAD &&
>> +	git log -1 --format=%B HEAD >actual &&
>> +	printf "This reverts commit $(git show -s \
>> + 		--pretty=reference HEAD^).\n\n" \
>> +		>expect &&
>> +	test_cmp expect actual
>> +'
> 
> I guess this fails by leaving the "# *** SAY WHY" in the resulting
> message, because the stripspace wants to see '%' to start commented
> out lines to be stripped?  If we inspect with this test what the
> temporary file we give to the editor looks like to make sure that
> '%' is used for commenting, that would be a more direct test, but
> without going that far, at least can we have a comment describing
> how this is expected to fail without the fix?

For me something like

	GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \
		--edit --reference HEAD &&
	test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \
		  actual

Would be a more convincing test as it actually checks that the user sees the line that we expect strbuf_stripspace() to remove from the final message. If we want to check the commit message as well that's fine but I'm not sure its necessary. (if we do we should use test_commit_message like patch 3)

Best Wishes

Phillip