[Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor

[devdekunle]

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 edited after they have been provided through -[mF].

cc: "brian m. carlson" [email protected]

[devdekunle]

/submit

[gitgitgadget]

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1817/devdekunle/notes_add_e_option-v1

To fetch this version to local tag pr-1817/devdekunle/notes_add_e_option-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1817/devdekunle/notes_add_e_option-v1

[gitgitgadget]

On the Git mailing list, "brian m. carlson" wrote (reply to this):

On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <[email protected]>
> 
> 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`.

> 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

[gitgitgadget]

User "brian m. carlson" <[email protected]> has been added to the cc: list.