Re: [gitgitgadget/git] [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor (PR #1817)

>
> 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 <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`.
>
> > 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
>
> —
> Reply to this email directly, view it on GitHub, or unsubscribe.
> You are receiving this because you authored the thread.Message ID: <gitgitgadget/git/pull/1817/c2423410614@github.com>