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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Oct 19, 2024, 00:38 UTC
Message-ID: <ZxL_oAXN5KQ4FVMc@tapette.crustytoothpaste.net>
In-Reply-To: <pull.1817.git.1729296853800.gitgitgadget@gmail.com>

On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote:

Show 12 quoted lines

> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
> 
> Notes can be added to a commit using the -m (message),
> -C (copy a note from a blob object) or
> -F (read the note from a file) options.
> 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 editted
> after the messages have been provided through -[mF].

I don't use the notes feature, but I definitely see how this is valuable there much as it is for `git commit`.

Show 38 quoted lines

> diff --git a/builtin/notes.c b/builtin/notes.c
> index 8c26e455269..02cdfdf1c9d 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)
>  		OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"),
>  			N_("reuse and edit specified note object"), PARSE_OPT_NONEG,
>  			parse_reedit_arg),
> +		OPT_BOOL('e', "edit", &d.use_editor,
> +			N_("edit note message in editor")),
>  		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
>  			N_("reuse specified note object"), PARSE_OPT_NONEG,
>  			parse_reuse_arg),
> @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  		OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"),
>  			N_("reuse specified note object"), PARSE_OPT_NONEG,
>  			parse_reuse_arg),
> +		OPT_BOOL('e', "edit", &d.use_editor,
> +			N_("edit note message in editor")),
>  		OPT_BOOL(0, "allow-empty", &allow_empty,
>  			N_("allow storing empty note")),
>  		OPT_CALLBACK_F(0, "separator", &separator,
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 99137fb2357..7f45a324faa 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' '
>  	git notes remove HEAD
>  '
>  
> +test_expect_success '"git notes add" with -m/-F invokes the editor with -e' '
> +	test_commit 19th &&
> +	GIT_EDITOR="true" git notes add -m "note message" -e &&
> +	git notes remove HEAD &&
> +	echo "message from file" >file_1 &&
> +	GIT_EDITOR="true" git notes add -F file_1 -e &&
> +	git notes remove HEAD
> +'

Maybe I don't understand what this is supposed to be testing (and if so, please correct me), but how are we verifying that the editor is being invoked? If we're invoking `true`, then that doesn't change the message in any way, so if we suddenly stopped invoking the editor, I don't think this would fail.

Maybe we could use something else as `GIT_EDITOR` instead. For example, if we did `GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e` (with an appropriate PERL prerequisite), then we could test that the message after the fact was "message from editor", which would help us verify that both the `-F` and `-e` options were being honoured. (Similar things can be said about the tests you added below this as well.)

I suggest Perl here because `sed -i` is nonstandard and not portable, but you could also set up a fake editor script as in t7004, which would avoid the need for the Perl dependency by using `sed` with a temporary file. That might be more palatable to the project at large, but I'd be fine with either approach.

Do you think there are any cases where testing the `--no-edit` functionality might be helpful? For example, is `git notes edit` ever useful to invoke with such an option, like one might do with `git commit amend`? (This isn't rhetorical, since the notes code is one of the areas of Git I'm least familiar with, so I ask both because I'm curious and if you think it's a useful thing to do, then tests might be a good idea.)

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA