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

[Taylor Blau <me@ttaylorr.com>]

On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
>
> Notes can be added to a commit using:
> 	- "-m" to provide a message on the command line.
> 	- -C to copy a note from a blob object.
> 	- -F to read the note from a file.
> When these options are used, Git does not open an editor,
> it simply takes the content provided via these options and
> attaches it to the commit as a note.
>
> Improve flexibility to fine-tune the note before finalizing it
> by allowing the messages to be prefilled in the editor and edited
> after the messages have been provided through -[mF].
>
> Signed-off-by: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>

Nicely described, this commit message is looking good. Let's take a look
at the patch below...

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 99137fb2357..2827f592c66 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1567,4 +1567,75 @@ 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 &&
> +	MSG="Edited notes message" git notes add -m "Initial notes message" -e &&
> +	echo "Edited notes message" >expect &&

Very nice use of the fake_editor script here.

It is a little cumbersome to repeat the same message in MSG= and when
populating the 'expect' file. Perhaps instead this could be written as:

    echo "edited notes message" >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 "Note from file" >note_file &&
> +	MSG="Edited note from file" git notes add -F note_file -e &&
> +	echo "Edited note from file" >expect &&

Same "note" here. ;-).

> +	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 &&
> +	git notes add -m "Initial note message" &&
> +	MSG="Appended edited note message" git notes append -m "New appended note" -e &&

It's fine to use shorter values for -m and $MSG here. I think "appended"
and "edited" would be fine for each, respectively.

Besides applying those suggestions throughout the patch's new tests
(including the ones that I didn't explicitly comment on here), I think
that this should be looking good after another round. Thanks for working
on it.

Thanks,
Taylor