Re: [PATCH v4] notes: teach the -e option to edit messages in editor

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 99137fb2357..813dfd8db97 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1567,4 +1567,67 @@ test_expect_success 'empty notes do not invoke the editor' '
>  	git notes remove HEAD
>  '
>  
> +test_expect_success 'git notes add with -m/-F invokes editor with -e' '
> +	test_commit 19th &&
> +	echo "edited" >expect &&
> +	MSG="$(cat expect)" git notes add -m "initial" -e &&
> +	git notes show >actual &&
> +	test_cmp expect actual &&
> +	git notes remove HEAD &&
> +
> +	# Add a note using -F and edit it
> +	echo "initial" >note_file &&
> +	MSG="$(cat expect)" git notes add -F note_file -e &&
> +	git notes show >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '
> +	test_commit 20th &&
> +	cat >expect <<-EOF &&
> +		initial
> +
> +		edited
> +	EOF

Nit: we typically align the contents of HERE docs with `cat`. I'm not a
huge fan of it and had been struggling with it initially, as well, but
coding style is subjective anyway and it's totally fine that one doesn't
agree with everything.

In any case, I don't think this warrants a reroll of this patch, just
to keep it in mind for future patches you may send.

[snip]
> +test_expect_success 'git notes append aborts when editor fails with -e' '
> +	test_commit 22nd &&
> +	echo "foo-file-1" >note_1 &&
> +
> +	# Try to append a note with -F and -e, but make the editor fail
> +	test_env GIT_EDITOR="false" test_must_fail git notes append -F note_1 -e &&
> +
> +	# Verify that no note was added due to editor failure
> +	test_must_fail git notes show
> +'
> +

Nice.

Thanks, this looks good to me overall.

Patrick