On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote:
> From: Abraham Samuel Adekunle <abrahamadekunle50@xxxxxxxxx>
>
> 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
Attachment:
signature.asc
Description: PGP signature
