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

[Junio C Hamano <gitster@pobox.com>]

kristofferhaugsbakk@fastmail.com writes:

> @@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r,
>  		next = parent;
>  		next_label = msg.parent_label;
>  		if (opts->commit_use_reference) {
> -			strbuf_addstr(&ctx->message,
> -				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> +			strbuf_commented_addf(&ctx->message, comment_line_str,
> +				"*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");

With the switch to "commented_addf", we'd terminate this line with
LF here, which means ...

> @@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r,
>  			   !starts_with(orig_subject, "Revert \"")) {
>  			strbuf_addstr(&ctx->message, "Reapply \"");
>  			strbuf_addstr(&ctx->message, orig_subject);
> +			strbuf_addstr(&ctx->message, "\n");
>  		} else {
>  			strbuf_addstr(&ctx->message, "Revert \"");
>  			strbuf_addstr(&ctx->message, msg.subject);
> -			strbuf_addstr(&ctx->message, "\"");
> +			strbuf_addstr(&ctx->message, "\"\n");

... we'd want to terminate the line in these two other if/else if/else
arms for symmetry, so that ...

>  		}
> -		strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
> +		strbuf_addstr(&ctx->message, "\nThis reverts commit ");

... we can lose the termination of the previous line from here.

Makes sense.

> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 411027fb58c..26d3cabb608 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -228,6 +228,18 @@ test_expect_success 'identification of reverted commit (--reference)' '
>  	test_cmp expect actual
>  '
>  
> +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?

Thanks.