{"thread":{"id":"62367","subject":"Re: [gitgitgadget/git] [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor (PR #1817)","startedAt":"2024-10-19T11:17:01Z","lastAt":"2024-10-19T11:17:01Z","messageCount":1,"participants":["Samuel Abraham"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505494","messageId":"CADYq+fYkGrbUWYmPVPQgAPqcnL-o8+Cq534-dKFHYLKV_y_Ttg@mail.gmail.com","threadId":"62367","inReplyTo":"gitgitgadget/git/pull/1817/c2423410614@github.com","subject":"Re: [gitgitgadget/git] [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in editor (PR #1817)","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-19T11:16:49Z","receivedAt":"2024-10-19T11:17:01Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Sat, Oct 19, 2024 at 1:43 AM gitgitgadget[bot]\n wrote:\n>\n> On the Git mailing list, \"brian m. carlson\" wrote (reply to this):\n>\n> On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > From: Abraham Samuel Adekunle \n> >\n> > Notes can be added to a commit using the -m (message),\n> > -C (copy a note from a blob object) or\n> > -F (read the note from a file) options.\n> > When these options are used, Git does not open an editor,\n> > it simply takes the content provided via these options and\n> > attaches it to the commit as a note.\n> >\n> > Improve flexibility to fine-tune the note before finalizing it\n> > by allowing the messages to be prefilled in the editor and editted\n> > after the messages have been provided through -[mF].\n>\n> I don't use the notes feature, but I definitely see how this is valuable\n> there much as it is for `git commit`.\n>\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index 8c26e455269..02cdfdf1c9d 100644\n> > --- a/builtin/notes.c\n> > +++ b/builtin/notes.c\n> > @@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)\n> > OPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n> > N_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n> > parse_reedit_arg),\n> > + OPT_BOOL('e', \"edit\", &d.use_editor,\n> > + N_(\"edit note message in editor\")),\n> > OPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> > N_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> > parse_reuse_arg),\n> > @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> > OPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> > N_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> > parse_reuse_arg),\n> > + OPT_BOOL('e', \"edit\", &d.use_editor,\n> > + N_(\"edit note message in editor\")),\n> > OPT_BOOL(0, \"allow-empty\", &allow_empty,\n> > N_(\"allow storing empty note\")),\n> > OPT_CALLBACK_F(0, \"separator\", &separator,\n> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> > index 99137fb2357..7f45a324faa 100755\n> > --- a/t/t3301-notes.sh\n> > +++ b/t/t3301-notes.sh\n> > @@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' '\n> > git notes remove HEAD\n> > '\n> >\n> > +test_expect_success '\"git notes add\" with -m/-F invokes the editor with -e' '\n> > + test_commit 19th &&\n> > + GIT_EDITOR=\"true\" git notes add -m \"note message\" -e &&\n> > + git notes remove HEAD &&\n> > + echo \"message from file\" >file_1 &&\n> > + GIT_EDITOR=\"true\" git notes add -F file_1 -e &&\n> > + git notes remove HEAD\n> > +'\n>\n> Maybe I don't understand what this is supposed to be testing (and if so,\n> please correct me), but how are we verifying that the editor is being\n> invoked? If we're invoking `true`, then that doesn't change the message\n> in any way, so if we suddenly stopped invoking the editor, I don't think\n> this would fail.\n>\nHello Brian,\nThanks for your review and feedback.\nYes, I realized the tests do not show a way to determine that the\neditor changed the message.\nThank you your observation and pointing out to me.\n\n> Maybe we could use something else as `GIT_EDITOR` instead. For example,\n> if we did `GIT_EDITOR=\"perl -pi -e s/file/editor/\" git notes add -F file_1 -e`\n> (with an appropriate PERL prerequisite), then we could test that the\n> message after the fact was \"message from editor\", which would help us\n> verify that both the `-F` and `-e` options were being honoured.\n> (Similar things can be said about the tests you added below this as\n> well.)\n\nOkay this is duly noted.\n\n>\n> I suggest Perl here because `sed -i` is nonstandard and not portable,\n> but you could also set up a fake editor script as in t7004, which would\n> avoid the need for the Perl dependency by using `sed` with a temporary\n> file. That might be more palatable to the project at large, but I'd be\n> fine with either approach.\n>\n\nI will look into this and also take a close look at the functions\nwhich define the implementation\nfor the GIT_EDITOR which are the write_script with fake_editor as file.\n\n> Do you think there are any cases where testing the `--no-edit`\n> functionality might be helpful? For example, is `git notes edit` ever\n> useful to invoke with such an option, like one might do with `git commit\n> amend`? (This isn't rhetorical, since the notes code is one of the areas\n> of Git I'm least familiar with, so I ask both because I'm curious and if\n> you think it's a useful thing to do, then tests might be a good idea.)\n> --\n> brian m. carlson (they/them or he/him)\n> Toronto, Ontario, CA\n>\n> —\n> Reply to this email directly, view it on GitHub, or unsubscribe.\n> You are receiving this because you authored the thread.Message ID: \n"}]}