notes: teach the -e option to edit messages in editor (PR #1817)

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

Quoted 9 lines · Samuel Adekunle Abraham via GitGitGadget

From: Abraham Samuel Adekunle <[email protected]>

Notes can be added to a commit using the -m (message),

(quoted diff) +0 -2

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

Quoted 3 lines

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 30 lines

builtin/notes.c +4 -0

index 8c26e455269..02cdfdf1c9d 100644
@@ -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,

t/t3301-notes.sh +0 -0

index 99137fb2357..7f45a324faa 100755
@@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' '
  git notes remove HEAD
 '
 

t/t3301-notes.sh test_expect_success 'empty notes do not invoke the editor' ' · line 1577

Quoted 8 lines

t/t3301-notes.sh +8 -0

+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/[email protected]>