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

On Sat, Oct 19, 2024 at 1:43 AM gitgitgadget[bot]
[off-list ref] wrote:

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:

quoted

From: Abraham Samuel Adekunle <redacted>

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`.

quoted

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.

Hello Brian,
Thanks for your review and feedback.
Yes, I realized the tests do not show a way to determine that the
editor changed the message.
Thank you your observation and pointing out to me.

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.)

Okay this is duly noted.

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.

I will look into this and also take a close look at the functions
which define the implementation
for the GIT_EDITOR which are the write_script with fake_editor as file.

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: [off-list ref]