{"thread":{"id":"62365","subject":"[PATCH] notes: teach the -e option to edit messages in editor","startedAt":"2024-10-19T00:14:17Z","lastAt":"2024-10-23T16:51:39Z","messageCount":22,"participants":["Samuel Adekunle Abraham via GitGitGadget","brian m. carlson","Kristoffer Haugsbakk","Samuel Abraham","Patrick Steinhardt","Taylor Blau","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505480","messageId":"pull.1817.git.1729296853800.gitgitgadget@gmail.com","threadId":"62365","inReplyTo":null,"subject":"[PATCH] notes: teach the -e option to edit messages in editor","fromName":"Samuel Adekunle Abraham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-19T00:14:13Z","receivedAt":"2024-10-19T00:14:17Z","isPatch":true,"sender":{"key":"name:Samuel Adekunle Abraham","avatar":null},"body":"From: Abraham Samuel Adekunle \n\nNotes 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.\nWhen these options are used, Git does not open an editor,\nit simply takes the content provided via these options and\nattaches it to the commit as a note.\n\nImprove flexibility to fine-tune the note before finalizing it\nby allowing the messages to be prefilled in the editor and editted\nafter the messages have been provided through -[mF].\n\nSigned-off-by: Abraham Samuel Adekunle \n---\n [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in\n editor\n \n Notes can be added to a commit using the -m (message), -C (copy a note\n from a blob object) or -F (read the note from a file) options. When\n these options are used, Git does not open an editor, it simply takes the\n content provided via these options and attaches it to the commit as a\n note.\n \n Improve flexibility to fine-tune the note before finalizing it by\n allowing the messages to be prefilled in the editor and edited after\n they have been provided through -[mF].\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1817\n\n builtin/notes.c | 4 ++++\n t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 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 \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reedit_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT_CALLBACK_F(0, \"separator\", &separator,\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 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 \tgit notes remove HEAD\n '\n \n+test_expect_success '\"git notes add\" with -m/-F invokes the editor with -e' '\n+\ttest_commit 19th &&\n+\tGIT_EDITOR=\"true\" git notes add -m \"note message\" -e &&\n+\tgit notes remove HEAD &&\n+\techo \"message from file\" >file_1 &&\n+\tGIT_EDITOR=\"true\" git notes add -F file_1 -e &&\n+\tgit notes remove HEAD\n+'\n+\n+test_expect_success 'git notes append with -m/-F invokes editor with -e' '\n+\ttest_commit 20th &&\n+\tGIT_EDITOR=\"true\" git notes add -m \"initial note\" -e &&\n+\tGIT_EDITOR=\"true\" git notes append -m \"appended note\" -e &&\n+\tgit notes remove HEAD &&\n+\techo \"initial note\" >note_a &&\n+\techo \"appended note\" >note_b &&\n+\tGIT_EDITOR=\"true\" git notes add -F note_a -e &&\n+\tGIT_EDITOR=\"true\" git notes append -F note_b -e &&\n+\tgit notes remove HEAD\n+'\n+\n+test_expect_success 'append note with multiple combinations of -m, -F and -e, invokes editor' '\n+\ttest_commit 21st &&\n+\techo \"foo-file-1\" >note_1 &&\n+\techo \"foo-file-2\" >note_2 &&\n+\tGIT_EDITOR=\"true\" git notes append -F note_1 -m \"message-1\" -F note_2 -m \"message-2\" -e &&\n+\tgit notes remove HEAD\n+'\n+\n test_done\n\nbase-commit: 15030f9556f545b167b1879b877a5d780252dc16\n-- \ngitgitgadget\n"},{"id":"505481","messageId":"ZxL_oAXN5KQ4FVMc@tapette.crustytoothpaste.net","threadId":"62365","inReplyTo":"pull.1817.git.1729296853800.gitgitgadget@gmail.com","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-10-19T00:38:56Z","receivedAt":"2024-10-19T00:38:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"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\nI don't use the notes feature, but I definitely see how this is valuable\nthere 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> \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reedit_arg),\n> +\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n> +\t\t\tN_(\"edit note message in editor\")),\n> \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reuse_arg),\n> @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reuse_arg),\n> +\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n> +\t\t\tN_(\"edit note message in editor\")),\n> \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> \t\t\tN_(\"allow storing empty note\")),\n> \t\tOPT_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> \tgit notes remove HEAD\n> '\n> \n> +test_expect_success '\"git notes add\" with -m/-F invokes the editor with -e' '\n> +\ttest_commit 19th &&\n> +\tGIT_EDITOR=\"true\" git notes add -m \"note message\" -e &&\n> +\tgit notes remove HEAD &&\n> +\techo \"message from file\" >file_1 &&\n> +\tGIT_EDITOR=\"true\" git notes add -F file_1 -e &&\n> +\tgit notes remove HEAD\n> +'\n\nMaybe I don't understand what this is supposed to be testing (and if so,\nplease correct me), but how are we verifying that the editor is being\ninvoked? If we're invoking `true`, then that doesn't change the message\nin any way, so if we suddenly stopped invoking the editor, I don't think\nthis would fail.\n\nMaybe we could use something else as `GIT_EDITOR` instead. For example,\nif 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\nmessage after the fact was \"message from editor\", which would help us\nverify that both the `-F` and `-e` options were being honoured.\n(Similar things can be said about the tests you added below this as\nwell.)\n\nI suggest Perl here because `sed -i` is nonstandard and not portable,\nbut you could also set up a fake editor script as in t7004, which would\navoid the need for the Perl dependency by using `sed` with a temporary\nfile. That might be more palatable to the project at large, but I'd be\nfine with either approach.\n\nDo you think there are any cases where testing the `--no-edit`\nfunctionality might be helpful? For example, is `git notes edit` ever\nuseful to invoke with such an option, like one might do with `git commit\namend`? (This isn't rhetorical, since the notes code is one of the areas\nof Git I'm least familiar with, so I ask both because I'm curious and if\nyou think it's a useful thing to do, then tests might be a good idea.)\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"505492","messageId":"5e41f019-f37f-4860-b672-78b85e24d9bf@app.fastmail.com","threadId":"62365","inReplyTo":"pull.1817.git.1729296853800.gitgitgadget@gmail.com","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-19T10:28:19Z","receivedAt":"2024-10-19T10:28:49Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi Samuel\n\nOn Sat, Oct 19, 2024, at 02:14, 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\nThanks for this work. I think part of the motivation here is to make\ngit-notes(1) act more in line with the conventions from git-commit(1),\nwhich is always nice to see.\n\nIt’s also useful in its own right.\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\nHere you explain how the end-user will benefit from this change. Nice.\n\nIt’s important to explain the background, what is being done, and why it\nis being done. And this commit message does all of that.\n"},{"id":"505493","messageId":"55176c6e-7ac1-46e6-affa-184307a0edf4@app.fastmail.com","threadId":"62365","inReplyTo":"ZxL_oAXN5KQ4FVMc@tapette.crustytoothpaste.net","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-19T11:03:51Z","receivedAt":"2024-10-19T11:04:13Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi brian and Samuel\n\nOn Sat, Oct 19, 2024, at 02:38, brian m. carlson wrote:\n> On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote:\n>> +test_expect_success '\"git notes add\" with -m/-F invokes the editor with -e' '\n>> +\ttest_commit 19th &&\n>> +\tGIT_EDITOR=\"true\" git notes add -m \"note message\" -e &&\n>> +\tgit notes remove HEAD &&\n>> +\techo \"message from file\" >file_1 &&\n>> +\tGIT_EDITOR=\"true\" git notes add -F file_1 -e &&\n>> +\tgit 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\nI also didn’t understand these tests.\n\nThere is this test in this file/test suite which tests the negative\ncase:\n\n test_expect_success 'empty notes do not invoke the editor' '\n test_commit 18th &&\n GIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty &&\n git notes remove HEAD &&\n GIT_EDITOR=\"false\" git notes add -m \"\" --allow-empty &&\n git notes remove HEAD &&\n GIT_EDITOR=\"false\" git notes add -F /dev/null --allow-empty &&\n git notes remove HEAD\n '\n\nAnd this works because the commands would fail if the editor was invoked:\n\n error: there was a problem with the editor 'false'\n\nBut this doesn’t work for `true`.\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\nThis file defines a `fake_editor`:[1]\n\n write_script fake_editor <<\\EOF\n echo \"$MSG\" >\"$1\"\n echo \"$MSG\" >&2\n EOF\n GIT_EDITOR=./fake_editor\n export GIT_EDITOR\n\nAnd it looks like this is how it is used:\n\n test_expect_success 'create notes' '\n MSG=b4 git notes add &&\n test_path_is_missing .git/NOTES_EDITMSG &&\n git ls-tree -r refs/notes/commits >actual &&\n test_line_count = 1 actual &&\n echo b4 >expect &&\n git notes show >actual &&\n test_cmp expect actual &&\n git show HEAD^ &&\n test_must_fail git notes show HEAD^\n '\n\nSo it seems that the new tests here should use the `test_cmp expect\nactual` style.\n\n† 1: The different test files use both `fake_editor`, `fake-editor`,\n and `fakeeditor`.\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\nYes, that is useful (both as a use-case and as a regression test[2]).\ngit-notes(1) is often used to programmatically add metadata:\n\n git show todo:post-applypatch | grep -C5 refs/notes/amlog\n\n(And this non-interactive example is not affected by this change since\n`-e` is required in order to invoke the editor)\n\n† 2: I seem to recall a regression in how git-notes(1) chose to invoke\n the editor or not\n"},{"id":"505495","messageId":"CADYq+fauh0ZBzvtBDLvHGx-=VDu-E+Z0U8MB8iZQxY+siOrw2A@mail.gmail.com","threadId":"62365","inReplyTo":"5e41f019-f37f-4860-b672-78b85e24d9bf@app.fastmail.com","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-19T11:19:17Z","receivedAt":"2024-10-19T11:19:28Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Sat, Oct 19, 2024 at 11:28 AM Kristoffer Haugsbakk\n wrote:\n>\n> Hi Samuel\n>\n> On Sat, Oct 19, 2024, at 02:14, 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> Thanks for this work. I think part of the motivation here is to make\n> git-notes(1) act more in line with the conventions from git-commit(1),\n> which is always nice to see.\n>\n> It’s also useful in its own right.\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> Here you explain how the end-user will benefit from this change. Nice.\n>\n> It’s important to explain the background, what is being done, and why it\n> is being done. And this commit message does all of that.\n\nHello Kristoffer,\nThank you very much for your response.\n"},{"id":"505496","messageId":"CADYq+fY=f5tKJJb0wfmGwTNzzqkMnBNzFZedUxuHhuPNNSgNfQ@mail.gmail.com","threadId":"62365","inReplyTo":"55176c6e-7ac1-46e6-affa-184307a0edf4@app.fastmail.com","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-19T11:24:55Z","receivedAt":"2024-10-19T11:25:06Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Sat, Oct 19, 2024 at 12:04 PM Kristoffer Haugsbakk\n wrote:\n>\n> Hi brian and Samuel\n>\n> On Sat, Oct 19, 2024, at 02:38, brian m. carlson wrote:\n> > On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote:\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>\n> I also didn’t understand these tests.\n>\n> There is this test in this file/test suite which tests the negative\n> case:\n>\n> test_expect_success 'empty notes do not invoke the editor' '\n> test_commit 18th &&\n> GIT_EDITOR=\"false\" git notes add -C \"$empty_blob\" --allow-empty &&\n> git notes remove HEAD &&\n> GIT_EDITOR=\"false\" git notes add -m \"\" --allow-empty &&\n> git notes remove HEAD &&\n> GIT_EDITOR=\"false\" git notes add -F /dev/null --allow-empty &&\n> git notes remove HEAD\n> '\n>\nThank you Kristoffer,\n\nYes incorrectly used this as a reference and I have however look\ndeeper into the implementation\nof the write_scripts function and the fake_editor file for better understanding.\n\n> And this works because the commands would fail if the editor was invoked:\n>\n> error: there was a problem with the editor 'false'\n>\n> But this doesn’t work for `true`.\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>\n> This file defines a `fake_editor`:[1]\n>\n> write_script fake_editor <<\\EOF\n> echo \"$MSG\" >\"$1\"\n> echo \"$MSG\" >&2\n> EOF\n> GIT_EDITOR=./fake_editor\n> export GIT_EDITOR\n>\n> And it looks like this is how it is used:\n>\n> test_expect_success 'create notes' '\n> MSG=b4 git notes add &&\n> test_path_is_missing .git/NOTES_EDITMSG &&\n> git ls-tree -r refs/notes/commits >actual &&\n> test_line_count = 1 actual &&\n> echo b4 >expect &&\n> git notes show >actual &&\n> test_cmp expect actual &&\n> git show HEAD^ &&\n> test_must_fail git notes show HEAD^\n> '\n>\n> So it seems that the new tests here should use the `test_cmp expect\n> actual` style.\n\nThank you very much for the guide.\nI will correct them and send a modified patch.\n\n>\n> † 1: The different test files use both `fake_editor`, `fake-editor`,\n> and `fakeeditor`.\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> Yes, that is useful (both as a use-case and as a regression test[2]).\n> git-notes(1) is often used to programmatically add metadata:\n>\n> git show todo:post-applypatch | grep -C5 refs/notes/amlog\n>\n> (And this non-interactive example is not affected by this change since\n> `-e` is required in order to invoke the editor)\n>\n> † 2: I seem to recall a regression in how git-notes(1) chose to invoke\n> the editor or not\n"},{"id":"505497","messageId":"0e393f0e-aa4b-479f-bedf-f4dad2256d02@app.fastmail.com","threadId":"62365","inReplyTo":"pull.1817.git.1729296853800.gitgitgadget@gmail.com","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-19T11:36:32Z","receivedAt":"2024-10-19T11:37:32Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Sat, Oct 19, 2024, at 02:14, Samuel Adekunle Abraham via GitGitGadget wrote:\n> builtin/notes.c | 4 ++++\n> t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++\n> 2 files changed, 33 insertions(+)\n\nThe documentation should be updated:\n\n Documentation/git-notes.txt\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\n> char *prefix)\n> \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reedit_arg),\n> +\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n> +\t\t\tN_(\"edit note message in editor\")),\n\nThe `add` subcommand does what I expect it to after some testing.\n\n> \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reuse_arg),\n> @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv,\n> const char *prefix)\n> \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reuse_arg),\n> +\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n> +\t\t\tN_(\"edit note message in editor\")),\n> \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> \t\t\tN_(\"allow storing empty note\")),\n> \t\tOPT_CALLBACK_F(0, \"separator\", &separator,\n\nLikewise for the `append` subcommand.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"505498","messageId":"CADYq+fYCAJx-FsPnn+RpTTFZ=hWucyC-gj=tq=d+7R2YiYrKdw@mail.gmail.com","threadId":"62365","inReplyTo":"0e393f0e-aa4b-479f-bedf-f4dad2256d02@app.fastmail.com","subject":"Re: [PATCH] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-19T11:58:41Z","receivedAt":"2024-10-19T11:58:51Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Sat, Oct 19, 2024 at 12:37 PM Kristoffer Haugsbakk\n wrote:\n>\n> On Sat, Oct 19, 2024, at 02:14, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > builtin/notes.c | 4 ++++\n> > t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++\n> > 2 files changed, 33 insertions(+)\n>\n> The documentation should be updated:\n>\n> Documentation/git-notes.txt\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\n> > 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>\n> The `add` subcommand does what I expect it to after some testing.\n>\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,\n> > 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>\n> Likewise for the `append` subcommand.\n>\n> --\n> Kristoffer Haugsbakk\n>\nOkay, I will do that. Thank you very much, Kristoffer.\n"},{"id":"505525","messageId":"pull.1817.v2.git.1729382580491.gitgitgadget@gmail.com","threadId":"62365","inReplyTo":"pull.1817.git.1729296853800.gitgitgadget@gmail.com","subject":"[PATCH v2] notes: teach the -e option to edit messages in editor","fromName":"Samuel Adekunle Abraham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-20T00:03:00Z","receivedAt":"2024-10-20T00:03:04Z","isPatch":true,"sender":{"key":"name:Samuel Adekunle Abraham","avatar":null},"body":"From: Abraham Samuel Adekunle \n\nNotes 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.\nWhen these options are used, Git does not open an editor,\nit simply takes the content provided via these options and\nattaches it to the commit as a note.\n\nImprove flexibility to fine-tune the note before finalizing it\nby allowing the messages to be prefilled in the editor and editted\nafter the messages have been provided through -[mF].\n\nSigned-off-by: Abraham Samuel Adekunle \n---\n [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in\n editor\n \n Notes can be added to a commit using the -m (message), -C (copy a note\n from a blob object) or -F (read the note from a file) options. When\n these options are used, Git does not open an editor, it simply takes the\n content provided via these options and attaches it to the commit as a\n note.\n \n Improve flexibility to fine-tune the note before finalizing it by\n allowing the messages to be prefilled in the editor and edited after\n they have been provided through -[mF].\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1817\n\nRange-diff vs v1:\n\n 1: 61a6d2dbcb9 ! 1: 55804cd1269 notes: teach the -e option to edit messages in editor\n @@ Commit message\n \n Signed-off-by: Abraham Samuel Adekunle \n \n + ## Documentation/git-notes.txt ##\n +@@ Documentation/git-notes.txt: SYNOPSIS\n + --------\n + [verse]\n + 'git notes' [list []]\n +-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n ++'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n + 'git notes' copy [-f] ( --stdin | [] )\n +-'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n ++'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n + 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n + 'git notes' show []\n + 'git notes' merge [-v | -q] [-s ] \n +@@ Documentation/git-notes.txt: add::\n + \tthe existing notes will be opened in the editor (like the `edit`\n + \tsubcommand). If you specify multiple `-m` and `-F`, a blank\n + \tline will be inserted between the messages. Use the `--separator`\n +-\toption to insert other delimiters.\n ++\toption to insert other delimiters. You can use `-e` to edit and\n ++\tfine-tune the message(s) supplied from `-m` and `-F` options\n ++\tinteractively (using an editor) before adding the note.\n + \n + copy::\n + \tCopy the notes for the first object onto the second object (defaults to\n +@@ Documentation/git-notes.txt: append::\n + \tan existing note, a blank line is added before each new\n + \tmessage as an inter-paragraph separator. The separator can\n + \tbe customized with the `--separator` option.\n ++\tEdit the notes to be appended given by `-m` and `-F` options with\n ++\t`-e` interactively (using an editor) before appending the note.\n + \n + edit::\n + \tEdit the notes for a given object (defaults to HEAD).\n +\n ## builtin/notes.c ##\n +@@\n + static const char *separator = \"\\n\";\n + static const char * const git_notes_usage[] = {\n + \tN_(\"git notes [--ref ] [list []]\"),\n +-\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n ++\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n + \tN_(\"git notes [--ref ] copy [-f] \"),\n +-\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n ++\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n + \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n + \tN_(\"git notes [--ref ] show []\"),\n + \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n @@ builtin/notes.c: static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '\n \tgit notes remove HEAD\n '\n \n -+test_expect_success '\"git notes add\" with -m/-F invokes the editor with -e' '\n ++test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n +\ttest_commit 19th &&\n -+\tGIT_EDITOR=\"true\" git notes add -m \"note message\" -e &&\n ++\tMSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n ++\techo \"Edited notes message\" >expect &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect actual &&\n +\tgit notes remove HEAD &&\n -+\techo \"message from file\" >file_1 &&\n -+\tGIT_EDITOR=\"true\" git notes add -F file_1 -e &&\n -+\tgit notes remove HEAD\n ++\n ++\t# Add a note using -F and edit it\n ++\techo \"Note from file\" >note_file &&\n ++\tMSG=\"Edited note from file\" git notes add -F note_file -e &&\n ++\techo \"Edited note from file\" >expect &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'git notes append with -m/-F invokes editor with -e' '\n ++test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n +\ttest_commit 20th &&\n -+\tGIT_EDITOR=\"true\" git notes add -m \"initial note\" -e &&\n -+\tGIT_EDITOR=\"true\" git notes append -m \"appended note\" -e &&\n ++\tgit notes add -m \"Initial note message\" &&\n ++\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n ++\n ++\t# Verify the note content was appended and edited\n ++\techo \"Initial note message\" >expect &&\n ++\techo \"\" >>expect &&\n ++\techo \"Appended edited note message\" >>expect &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect actual &&\n +\tgit notes remove HEAD &&\n -+\techo \"initial note\" >note_a &&\n -+\techo \"appended note\" >note_b &&\n -+\tGIT_EDITOR=\"true\" git notes add -F note_a -e &&\n -+\tGIT_EDITOR=\"true\" git notes append -F note_b -e &&\n -+\tgit notes remove HEAD\n ++\n ++\t#Append a note using -F and edit it\n ++\techo \"Note from file\" >note_file &&\n ++\tgit notes add -m \"Initial note message\" &&\n ++\tMSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n ++\n ++\t# Verify notes from file has been edited in editor and appended\n ++\techo \"Initial note message\" >expect &&\n ++\techo \"\" >>expect &&\n ++\techo \"Appended edited note from file\" >>expect &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'append note with multiple combinations of -m, -F and -e, invokes editor' '\n ++test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '\n +\ttest_commit 21st &&\n +\techo \"foo-file-1\" >note_1 &&\n +\techo \"foo-file-2\" >note_2 &&\n -+\tGIT_EDITOR=\"true\" git notes append -F note_1 -m \"message-1\" -F note_2 -m \"message-2\" -e &&\n -+\tgit notes remove HEAD\n ++\n ++\tMSG=\"Collapsed edited notes\" git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n ++\n ++\t# Verify that combined messages from file and -m have been edited\n ++\n ++\techo \"Collapsed edited notes\" >expect &&\n ++\tgit notes show >actual &&\n ++\ttest_cmp expect actual\n +'\n +\n test_done\n\n\n Documentation/git-notes.txt | 10 +++++--\n builtin/notes.c | 8 ++++--\n t/t3301-notes.sh | 56 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 69 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex c9221a68cce..d5505a426aa 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,9 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -67,7 +67,9 @@ add::\n \tthe existing notes will be opened in the editor (like the `edit`\n \tsubcommand). If you specify multiple `-m` and `-F`, a blank\n \tline will be inserted between the messages. Use the `--separator`\n-\toption to insert other delimiters.\n+\toption to insert other delimiters. You can use `-e` to edit and\n+\tfine-tune the message(s) supplied from `-m` and `-F` options\n+\tinteractively (using an editor) before adding the note.\n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n@@ -93,6 +95,8 @@ append::\n \tan existing note, a blank line is added before each new\n \tmessage as an inter-paragraph separator. The separator can\n \tbe customized with the `--separator` option.\n+\tEdit the notes to be appended given by `-m` and `-F` options with\n+\t`-e` interactively (using an editor) before appending the note.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8c26e455269..72c8a51cfac 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -32,9 +32,9 @@\n static const char *separator = \"\\n\";\n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n-\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n \tN_(\"git notes [--ref ] copy [-f] \"),\n-\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reedit_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT_CALLBACK_F(0, \"separator\", &separator,\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 99137fb2357..ffa1d21671d 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1567,4 +1567,60 @@ test_expect_success 'empty notes do not invoke the editor' '\n \tgit notes remove HEAD\n '\n \n+test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n+\ttest_commit 19th &&\n+\tMSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n+\techo \"Edited notes message\" >expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove HEAD &&\n+\n+\t# Add a note using -F and edit it\n+\techo \"Note from file\" >note_file &&\n+\tMSG=\"Edited note from file\" git notes add -F note_file -e &&\n+\techo \"Edited note from file\" >expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n+\ttest_commit 20th &&\n+\tgit notes add -m \"Initial note message\" &&\n+\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n+\n+\t# Verify the note content was appended and edited\n+\techo \"Initial note message\" >expect &&\n+\techo \"\" >>expect &&\n+\techo \"Appended edited note message\" >>expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove HEAD &&\n+\n+\t#Append a note using -F and edit it\n+\techo \"Note from file\" >note_file &&\n+\tgit notes add -m \"Initial note message\" &&\n+\tMSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n+\n+\t# Verify notes from file has been edited in editor and appended\n+\techo \"Initial note message\" >expect &&\n+\techo \"\" >>expect &&\n+\techo \"Appended edited note from file\" >>expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '\n+\ttest_commit 21st &&\n+\techo \"foo-file-1\" >note_1 &&\n+\techo \"foo-file-2\" >note_2 &&\n+\n+\tMSG=\"Collapsed edited notes\" git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n+\n+\t# Verify that combined messages from file and -m have been edited\n+\n+\techo \"Collapsed edited notes\" >expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\nbase-commit: 15030f9556f545b167b1879b877a5d780252dc16\n-- \ngitgitgadget\n"},{"id":"505661","messageId":"ZxZByGdre-lup2sO@pks.im","threadId":"62365","inReplyTo":"pull.1817.v2.git.1729382580491.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] notes: teach the -e option to edit messages in editor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-21T11:58:05Z","receivedAt":"2024-10-21T11:58:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Oct 20, 2024 at 12:03:00AM +0000, 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\nNit: this would read a bit better if this was a bulleted list, I think.\nE.g.:\n\n Notes can be added to a commit using:\n\n - \"-m\" to provide a message on the command line.\n - -C to copy a note from a blob object.\n - -F to read the note from a file.\n\n When these options are used, ...\n\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\ns/editted/edited\n\n> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> index c9221a68cce..d5505a426aa 100644\n> --- a/Documentation/git-notes.txt\n> +++ b/Documentation/git-notes.txt\n> @@ -9,9 +9,9 @@ SYNOPSIS\n> --------\n> [verse]\n> 'git notes' [list []]\n> -'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n> +'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n> 'git notes' copy [-f] ( --stdin | [] )\n> -'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n> +'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n> 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n> 'git notes' show []\n> 'git notes' merge [-v | -q] [-s ] \n\nNit: I'd move the `[-e]` before [] so that -F, -C and -m are all\nclose together.\n\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 8c26e455269..72c8a51cfac 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> \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reedit_arg),\n> +\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n> +\t\t\tN_(\"edit note message in editor\")),\n> \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reuse_arg),\n> @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n> \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n> \t\t\tparse_reuse_arg),\n> +\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n> +\t\t\tN_(\"edit note message in editor\")),\n> \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n> \t\t\tN_(\"allow storing empty note\")),\n> \t\tOPT_CALLBACK_F(0, \"separator\", &separator,\n\nNice.\n\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 99137fb2357..ffa1d21671d 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -1567,4 +1567,60 @@ test_expect_success 'empty notes do not invoke the editor' '\n> \tgit notes remove HEAD\n> '\n> \n> +test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n> +\ttest_commit 19th &&\n> +\tMSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n> +\techo \"Edited notes message\" >expect &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit notes remove HEAD &&\n> +\n> +\t# Add a note using -F and edit it\n> +\techo \"Note from file\" >note_file &&\n> +\tMSG=\"Edited note from file\" git notes add -F note_file -e &&\n> +\techo \"Edited note from file\" >expect &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nI was surprised at first why the MSG ended up in the commit message. But\nthe setup of t3301 writes a fake editor that listens to this environment\nvariable, so this looks good to me.\n\n> +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n> +\ttest_commit 20th &&\n> +\tgit notes add -m \"Initial note message\" &&\n> +\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n> +\n> +\t# Verify the note content was appended and edited\n> +\techo \"Initial note message\" >expect &&\n> +\techo \"\" >>expect &&\n> +\techo \"Appended edited note message\" >>expect &&\n\nWhen you want to write multiple lines we typically use HERE docs. E.g.:\n\n cat >expect <<-EOF &&\n Initial note message\n\n Appended edited note message\n EOF\n\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit notes remove HEAD &&\n> +\n> +\t#Append a note using -F and edit it\n\nThere should be a space after the \"#\" here.\n\n> +\techo \"Note from file\" >note_file &&\n> +\tgit notes add -m \"Initial note message\" &&\n> +\tMSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n> +\n> +\t# Verify notes from file has been edited in editor and appended\n> +\techo \"Initial note message\" >expect &&\n> +\techo \"\" >>expect &&\n> +\techo \"Appended edited note from file\" >>expect &&\n\nSame comment here.\n\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '\n> +\ttest_commit 21st &&\n> +\techo \"foo-file-1\" >note_1 &&\n> +\techo \"foo-file-2\" >note_2 &&\n> +\n> +\tMSG=\"Collapsed edited notes\" git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n> +\n> +\t# Verify that combined messages from file and -m have been edited\n> +\n> +\techo \"Collapsed edited notes\" >expect &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\nIt would be nice to have another test that uses EDITOR=false to\ndemonstrate that we abort when the editor returns an error.\n\nOther than that this patch looks good to me, thanks a lot!\n\nPatrick\n"},{"id":"505673","messageId":"CADYq+fZ0A-6Odqoe0Yk+DHx13gc84Uk4RVagL7kMoY6+O9kB=Q@mail.gmail.com","threadId":"62365","inReplyTo":"ZxZByGdre-lup2sO@pks.im","subject":"Re: [PATCH v2] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-21T12:37:22Z","receivedAt":"2024-10-21T12:37:33Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Mon, Oct 21, 2024 at 12:58 PM Patrick Steinhardt wrote:\n>\n> On Sun, Oct 20, 2024 at 12:03:00AM +0000, 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>\n> Nit: this would read a bit better if this was a bulleted list, I think.\n> E.g.:\n>\n> Notes can be added to a commit using:\n>\n> - \"-m\" to provide a message on the command line.\n> - -C to copy a note from a blob object.\n> - -F to read the note from a file.\n>\n> When these options are used, ...\n\nThank you Patrick. Noted.\n>\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>\n> s/editted/edited\n\nOkay.\n>\n> > diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\n> > index c9221a68cce..d5505a426aa 100644\n> > --- a/Documentation/git-notes.txt\n> > +++ b/Documentation/git-notes.txt\n> > @@ -9,9 +9,9 @@ SYNOPSIS\n> > --------\n> > [verse]\n> > 'git notes' [list []]\n> > -'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n> > +'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n> > 'git notes' copy [-f] ( --stdin | [] )\n> > -'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n> > +'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n> > 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n> > 'git notes' show []\n> > 'git notes' merge [-v | -q] [-s ] \n>\n> Nit: I'd move the `[-e]` before [] so that -F, -C and -m are all\n> close together.\n>\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > index 8c26e455269..72c8a51cfac 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>\n> Nice.\n>\n> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> > index 99137fb2357..ffa1d21671d 100755\n> > --- a/t/t3301-notes.sh\n> > +++ b/t/t3301-notes.sh\n> > @@ -1567,4 +1567,60 @@ 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 editor with -e' '\n> > + test_commit 19th &&\n> > + MSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n> > + echo \"Edited notes message\" >expect &&\n> > + git notes show >actual &&\n> > + test_cmp expect actual &&\n> > + git notes remove HEAD &&\n> > +\n> > + # Add a note using -F and edit it\n> > + echo \"Note from file\" >note_file &&\n> > + MSG=\"Edited note from file\" git notes add -F note_file -e &&\n> > + echo \"Edited note from file\" >expect &&\n> > + git notes show >actual &&\n> > + test_cmp expect actual\n> > +'\n>\n> I was surprised at first why the MSG ended up in the commit message. But\n> the setup of t3301 writes a fake editor that listens to this environment\n> variable, so this looks good to me.\n>\n> > +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n> > + test_commit 20th &&\n> > + git notes add -m \"Initial note message\" &&\n> > + MSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n> > +\n> > + # Verify the note content was appended and edited\n> > + echo \"Initial note message\" >expect &&\n> > + echo \"\" >>expect &&\n> > + echo \"Appended edited note message\" >>expect &&\n>\n> When you want to write multiple lines we typically use HERE docs. E.g.:\n>\n> cat >expect <<-EOF &&\n> Initial note message\n>\n> Appended edited note message\n> EOF\n>\n> > + git notes show >actual &&\n> > + test_cmp expect actual &&\n> > + git notes remove HEAD &&\n> > +\n> > + #Append a note using -F and edit it\n>\n> There should be a space after the \"#\" here.\n>\n> > + echo \"Note from file\" >note_file &&\n> > + git notes add -m \"Initial note message\" &&\n> > + MSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n> > +\n> > + # Verify notes from file has been edited in editor and appended\n> > + echo \"Initial note message\" >expect &&\n> > + echo \"\" >>expect &&\n> > + echo \"Appended edited note from file\" >>expect &&\n>\n> Same comment here.\n>\n> > + git notes show >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '\n> > + test_commit 21st &&\n> > + echo \"foo-file-1\" >note_1 &&\n> > + echo \"foo-file-2\" >note_2 &&\n> > +\n> > + MSG=\"Collapsed edited notes\" git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n> > +\n> > + # Verify that combined messages from file and -m have been edited\n> > +\n> > + echo \"Collapsed edited notes\" >expect &&\n> > + git notes show >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n\n> > test_done\n>\n> It would be nice to have another test that uses EDITOR=false to\n> demonstrate that we abort when the editor returns an error.\n>\n\nThanks for the review, I will effect the changes.\n> Other than that this patch looks good to me, thanks a lot!\n\nYou're welcome, Patrick.\n>\n> Patrick\n"},{"id":"505698","messageId":"pull.1817.v3.git.1729521495497.gitgitgadget@gmail.com","threadId":"62365","inReplyTo":"pull.1817.v2.git.1729382580491.gitgitgadget@gmail.com","subject":"[PATCH v3] notes: teach the -e option to edit messages in editor","fromName":"Samuel Adekunle Abraham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-21T14:38:15Z","receivedAt":"2024-10-21T14:38:19Z","isPatch":true,"sender":{"key":"name:Samuel Adekunle Abraham","avatar":null},"body":"From: Abraham Samuel Adekunle \n\nNotes can be added to a commit using:\n\t- \"-m\" to provide a message on the command line.\n\t- -C to copy a note from a blob object.\n\t- -F to read the note from a file.\nWhen these options are used, Git does not open an editor,\nit simply takes the content provided via these options and\nattaches it to the commit as a note.\n\nImprove flexibility to fine-tune the note before finalizing it\nby allowing the messages to be prefilled in the editor and edited\nafter the messages have been provided through -[mF].\n\nSigned-off-by: Abraham Samuel Adekunle \n---\n [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in\n editor\n \n Notes can be added to a commit using the -m (message), -C (copy a note\n from a blob object) or -F (read the note from a file) options. When\n these options are used, Git does not open an editor, it simply takes the\n content provided via these options and attaches it to the commit as a\n note.\n \n Improve flexibility to fine-tune the note before finalizing it by\n allowing the messages to be prefilled in the editor and edited after\n they have been provided through -[mF].\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1817\n\nRange-diff vs v2:\n\n 1: 55804cd1269 ! 1: 8f80c61ec0d notes: teach the -e option to edit messages in editor\n @@ Metadata\n ## Commit message ##\n notes: teach the -e option to edit messages in editor\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 + Notes can be added to a commit using:\n + - \"-m\" to provide a message on the command line.\n + - -C to copy a note from a blob object.\n + - -F to read the note from a file.\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 + by allowing the messages to be prefilled in the editor and edited\n after the messages have been provided through -[mF].\n \n Signed-off-by: Abraham Samuel Adekunle \n @@ Documentation/git-notes.txt: SYNOPSIS\n [verse]\n 'git notes' [list []]\n -'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n -+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n ++'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [-e] []\n 'git notes' copy [-f] ( --stdin | [] )\n -'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n -+'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [] [-e]\n ++'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [-e] []\n 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '\n +\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n +\n +\t# Verify the note content was appended and edited\n -+\techo \"Initial note message\" >expect &&\n -+\techo \"\" >>expect &&\n -+\techo \"Appended edited note message\" >>expect &&\n ++\n ++\tcat >expect <<-EOF &&\n ++\t\tInitial note message\n ++\n ++\t\tAppended edited note message\n ++\tEOF\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual &&\n +\tgit notes remove HEAD &&\n +\n -+\t#Append a note using -F and edit it\n ++\t# Append a note using -F and edit it\n +\techo \"Note from file\" >note_file &&\n +\tgit notes add -m \"Initial note message\" &&\n +\tMSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n +\n +\t# Verify notes from file has been edited in editor and appended\n -+\techo \"Initial note message\" >expect &&\n -+\techo \"\" >>expect &&\n -+\techo \"Appended edited note from file\" >>expect &&\n ++\tcat >expect <<-EOF &&\n ++\t\tInitial note message\n ++\n ++\t\tAppended edited note from file\n ++\tEOF\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n ++test_expect_success 'git notes append aborts when editor fails with -e' '\n ++\ttest_commit 22nd &&\n ++\techo \"foo-file-1\" >note_1 &&\n ++\n ++\t# Try to append a note with -F and -e, but make the editor fail\n ++\ttest_env GIT_EDITOR=\"false\" test_must_fail git notes append -F note_1 -e &&\n ++\n ++\t# Verify that no note was added due to editor failure\n ++\ttest_must_fail git notes show\n ++'\n +\n test_done\n\n\n Documentation/git-notes.txt | 10 ++++--\n builtin/notes.c | 8 +++--\n t/t3301-notes.sh | 71 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 84 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex c9221a68cce..84022f99d76 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,9 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [-e] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [-e] []\n 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -67,7 +67,9 @@ add::\n \tthe existing notes will be opened in the editor (like the `edit`\n \tsubcommand). If you specify multiple `-m` and `-F`, a blank\n \tline will be inserted between the messages. Use the `--separator`\n-\toption to insert other delimiters.\n+\toption to insert other delimiters. You can use `-e` to edit and\n+\tfine-tune the message(s) supplied from `-m` and `-F` options\n+\tinteractively (using an editor) before adding the note.\n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n@@ -93,6 +95,8 @@ append::\n \tan existing note, a blank line is added before each new\n \tmessage as an inter-paragraph separator. The separator can\n \tbe customized with the `--separator` option.\n+\tEdit the notes to be appended given by `-m` and `-F` options with\n+\t`-e` interactively (using an editor) before appending the note.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8c26e455269..72c8a51cfac 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -32,9 +32,9 @@\n static const char *separator = \"\\n\";\n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n-\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n \tN_(\"git notes [--ref ] copy [-f] \"),\n-\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reedit_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT_CALLBACK_F(0, \"separator\", &separator,\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 99137fb2357..2827f592c66 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1567,4 +1567,75 @@ test_expect_success 'empty notes do not invoke the editor' '\n \tgit notes remove HEAD\n '\n \n+test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n+\ttest_commit 19th &&\n+\tMSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n+\techo \"Edited notes message\" >expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove HEAD &&\n+\n+\t# Add a note using -F and edit it\n+\techo \"Note from file\" >note_file &&\n+\tMSG=\"Edited note from file\" git notes add -F note_file -e &&\n+\techo \"Edited note from file\" >expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n+\ttest_commit 20th &&\n+\tgit notes add -m \"Initial note message\" &&\n+\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n+\n+\t# Verify the note content was appended and edited\n+\n+\tcat >expect <<-EOF &&\n+\t\tInitial note message\n+\n+\t\tAppended edited note message\n+\tEOF\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove HEAD &&\n+\n+\t# Append a note using -F and edit it\n+\techo \"Note from file\" >note_file &&\n+\tgit notes add -m \"Initial note message\" &&\n+\tMSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n+\n+\t# Verify notes from file has been edited in editor and appended\n+\tcat >expect <<-EOF &&\n+\t\tInitial note message\n+\n+\t\tAppended edited note from file\n+\tEOF\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '\n+\ttest_commit 21st &&\n+\techo \"foo-file-1\" >note_1 &&\n+\techo \"foo-file-2\" >note_2 &&\n+\n+\tMSG=\"Collapsed edited notes\" git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n+\n+\t# Verify that combined messages from file and -m have been edited\n+\n+\techo \"Collapsed edited notes\" >expect &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'git notes append aborts when editor fails with -e' '\n+\ttest_commit 22nd &&\n+\techo \"foo-file-1\" >note_1 &&\n+\n+\t# Try to append a note with -F and -e, but make the editor fail\n+\ttest_env GIT_EDITOR=\"false\" test_must_fail git notes append -F note_1 -e &&\n+\n+\t# Verify that no note was added due to editor failure\n+\ttest_must_fail git notes show\n+'\n+\n test_done\n\nbase-commit: 15030f9556f545b167b1879b877a5d780252dc16\n-- \ngitgitgadget\n"},{"id":"505713","messageId":"ZxaG67wuvjOXc5kr@nand.local","threadId":"62365","inReplyTo":"pull.1817.v3.git.1729521495497.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] notes: teach the -e option to edit messages in editor","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T16:52:59Z","receivedAt":"2024-10-21T16:53:01Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> From: Abraham Samuel Adekunle \n>\n> Notes can be added to a commit using:\n> \t- \"-m\" to provide a message on the command line.\n> \t- -C to copy a note from a blob object.\n> \t- -F to read the note from a file.\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 edited\n> after the messages have been provided through -[mF].\n>\n> Signed-off-by: Abraham Samuel Adekunle \n\nNicely described, this commit message is looking good. Let's take a look\nat the patch below...\n\n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 99137fb2357..2827f592c66 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -1567,4 +1567,75 @@ test_expect_success 'empty notes do not invoke the editor' '\n> \tgit notes remove HEAD\n> '\n>\n> +test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n> +\ttest_commit 19th &&\n> +\tMSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n> +\techo \"Edited notes message\" >expect &&\n\nVery nice use of the fake_editor script here.\n\nIt is a little cumbersome to repeat the same message in MSG= and when\npopulating the 'expect' file. Perhaps instead this could be written as:\n\n echo \"edited notes message\" >expect &&\n MSG=\"$(cat expect)\" git notes -add -m \"initial\" -e\n\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit notes remove HEAD &&\n> +\n> +\t# Add a note using -F and edit it\n> +\techo \"Note from file\" >note_file &&\n> +\tMSG=\"Edited note from file\" git notes add -F note_file -e &&\n> +\techo \"Edited note from file\" >expect &&\n\nSame \"note\" here. ;-).\n\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n> +\ttest_commit 20th &&\n> +\tgit notes add -m \"Initial note message\" &&\n> +\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n\nIt's fine to use shorter values for -m and $MSG here. I think \"appended\"\nand \"edited\" would be fine for each, respectively.\n\nBesides applying those suggestions throughout the patch's new tests\n(including the ones that I didn't explicitly comment on here), I think\nthat this should be looking good after another round. Thanks for working\non it.\n\nThanks,\nTaylor\n"},{"id":"505715","messageId":"CADYq+fYERH=9_ZXq4JiwjE2_piP93u2=-9ii63SCA0r9p2==Ag@mail.gmail.com","threadId":"62365","inReplyTo":"ZxaG67wuvjOXc5kr@nand.local","subject":"Re: [PATCH v3] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-21T17:12:34Z","receivedAt":"2024-10-21T17:12:45Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Mon, Oct 21, 2024 at 5:53 PM Taylor Blau wrote:\n>\n> On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > From: Abraham Samuel Adekunle \n> >\n> > Notes can be added to a commit using:\n> > - \"-m\" to provide a message on the command line.\n> > - -C to copy a note from a blob object.\n> > - -F to read the note from a file.\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 edited\n> > after the messages have been provided through -[mF].\n> >\n> > Signed-off-by: Abraham Samuel Adekunle \n>\n> Nicely described, this commit message is looking good. Let's take a look\n> at the patch below...\n>\n> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> > index 99137fb2357..2827f592c66 100755\n> > --- a/t/t3301-notes.sh\n> > +++ b/t/t3301-notes.sh\n> > @@ -1567,4 +1567,75 @@ 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 editor with -e' '\n> > + test_commit 19th &&\n> > + MSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n> > + echo \"Edited notes message\" >expect &&\n>\n> Very nice use of the fake_editor script here.\n>\n> It is a little cumbersome to repeat the same message in MSG= and when\n> populating the 'expect' file. Perhaps instead this could be written as:\n>\n> echo \"edited notes message\" >expect &&\n> MSG=\"$(cat expect)\" git notes -add -m \"initial\" -e\n\nConcise! :-). Thank you very much Taylor.\n>\n> > + git notes show >actual &&\n> > + test_cmp expect actual &&\n> > + git notes remove HEAD &&\n> > +\n> > + # Add a note using -F and edit it\n> > + echo \"Note from file\" >note_file &&\n> > + MSG=\"Edited note from file\" git notes add -F note_file -e &&\n> > + echo \"Edited note from file\" >expect &&\n>\n> Same \"note\" here. ;-).\n>\n> > + git notes show >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n> > + test_commit 20th &&\n> > + git notes add -m \"Initial note message\" &&\n> > + MSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n>\n> It's fine to use shorter values for -m and $MSG here. I think \"appended\"\n> and \"edited\" would be fine for each, respectively.\n>\nOkay. Noted\n> Besides applying those suggestions throughout the patch's new tests\n> (including the ones that I didn't explicitly comment on here), I think\n> that this should be looking good after another round. Thanks for working\n> on it.\nThank you very much,\nAbraham Samuel\n>\n> Thanks,\n> Taylor\n"},{"id":"505719","messageId":"pull.1817.v4.git.1729534340786.gitgitgadget@gmail.com","threadId":"62365","inReplyTo":"pull.1817.v3.git.1729521495497.gitgitgadget@gmail.com","subject":"[PATCH v4] notes: teach the -e option to edit messages in editor","fromName":"Samuel Adekunle Abraham via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-10-21T18:12:20Z","receivedAt":"2024-10-21T18:12:24Z","isPatch":true,"sender":{"key":"name:Samuel Adekunle Abraham","avatar":null},"body":"From: Abraham Samuel Adekunle \n\nNotes can be added to a commit using:\n\t- \"-m\" to provide a message on the command line.\n\t- -C to copy a note from a blob object.\n\t- -F to read the note from a file.\nWhen these options are used, Git does not open an editor,\nit simply takes the content provided via these options and\nattaches it to the commit as a note.\n\nImprove flexibility to fine-tune the note before finalizing it\nby allowing the messages to be prefilled in the editor and edited\nafter the messages have been provided through -[mF].\n\nSigned-off-by: Abraham Samuel Adekunle \n---\n [Outreachy][RFC/PATCH] notes: teach the -e option to edit messages in\n editor\n \n Notes can be added to a commit using the -m (message), -C (copy a note\n from a blob object) or -F (read the note from a file) options. When\n these options are used, Git does not open an editor, it simply takes the\n content provided via these options and attaches it to the commit as a\n note.\n \n Improve flexibility to fine-tune the note before finalizing it by\n allowing the messages to be prefilled in the editor and edited after\n they have been provided through -[mF].\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1817%2Fdevdekunle%2Fnotes_add_e_option-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1817/devdekunle/notes_add_e_option-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1817\n\nRange-diff vs v3:\n\n 1: 8f80c61ec0d ! 1: 4ecf79e0766 notes: teach the -e option to edit messages in editor\n @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '\n \n +test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n +\ttest_commit 19th &&\n -+\tMSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n -+\techo \"Edited notes message\" >expect &&\n ++\techo \"edited\" >expect &&\n ++\tMSG=\"$(cat expect)\" git notes add -m \"initial\" -e &&\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual &&\n +\tgit notes remove HEAD &&\n +\n +\t# Add a note using -F and edit it\n -+\techo \"Note from file\" >note_file &&\n -+\tMSG=\"Edited note from file\" git notes add -F note_file -e &&\n -+\techo \"Edited note from file\" >expect &&\n ++\techo \"initial\" >note_file &&\n ++\tMSG=\"$(cat expect)\" git notes add -F note_file -e &&\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n +\n +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n +\ttest_commit 20th &&\n -+\tgit notes add -m \"Initial note message\" &&\n -+\tMSG=\"Appended edited note message\" git notes append -m \"New appended note\" -e &&\n -+\n -+\t# Verify the note content was appended and edited\n -+\n +\tcat >expect <<-EOF &&\n -+\t\tInitial note message\n ++\t\tinitial\n +\n -+\t\tAppended edited note message\n ++\t\tedited\n +\tEOF\n ++\tgit notes add -m \"initial\" &&\n ++\tMSG=\"edited\" git notes append -m \"appended\" -e &&\n ++\n ++\t# Verify the note content was appended and edited\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual &&\n +\tgit notes remove HEAD &&\n +\n +\t# Append a note using -F and edit it\n -+\techo \"Note from file\" >note_file &&\n -+\tgit notes add -m \"Initial note message\" &&\n -+\tMSG=\"Appended edited note from file\" git notes append -F note_file -e &&\n ++\techo \"note from file\" >note_file &&\n ++\tgit notes add -m \"initial\" &&\n ++\tMSG=\"edited\" git notes append -F note_file -e &&\n +\n +\t# Verify notes from file has been edited in editor and appended\n -+\tcat >expect <<-EOF &&\n -+\t\tInitial note message\n -+\n -+\t\tAppended edited note from file\n -+\tEOF\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n @@ t/t3301-notes.sh: test_expect_success 'empty notes do not invoke the editor' '\n +\ttest_commit 21st &&\n +\techo \"foo-file-1\" >note_1 &&\n +\techo \"foo-file-2\" >note_2 &&\n ++\techo \"edited\" >expect &&\n +\n -+\tMSG=\"Collapsed edited notes\" git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n ++\tMSG=$(cat expect) git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n +\n +\t# Verify that combined messages from file and -m have been edited\n -+\n -+\techo \"Collapsed edited notes\" >expect &&\n +\tgit notes show >actual &&\n +\ttest_cmp expect actual\n +'\n\n\n Documentation/git-notes.txt | 10 ++++--\n builtin/notes.c | 8 +++--\n t/t3301-notes.sh | 63 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 76 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt\nindex c9221a68cce..84022f99d76 100644\n--- a/Documentation/git-notes.txt\n+++ b/Documentation/git-notes.txt\n@@ -9,9 +9,9 @@ SYNOPSIS\n --------\n [verse]\n 'git notes' [list []]\n-'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' add [-f] [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [-e] []\n 'git notes' copy [-f] ( --stdin | [] )\n-'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] []\n+'git notes' append [--allow-empty] [--[no-]separator | --separator=] [--[no-]stripspace] [-F | -m | (-c | -C) ] [-e] []\n 'git notes' edit [--allow-empty] [] [--[no-]stripspace]\n 'git notes' show []\n 'git notes' merge [-v | -q] [-s ] \n@@ -67,7 +67,9 @@ add::\n \tthe existing notes will be opened in the editor (like the `edit`\n \tsubcommand). If you specify multiple `-m` and `-F`, a blank\n \tline will be inserted between the messages. Use the `--separator`\n-\toption to insert other delimiters.\n+\toption to insert other delimiters. You can use `-e` to edit and\n+\tfine-tune the message(s) supplied from `-m` and `-F` options\n+\tinteractively (using an editor) before adding the note.\n \n copy::\n \tCopy the notes for the first object onto the second object (defaults to\n@@ -93,6 +95,8 @@ append::\n \tan existing note, a blank line is added before each new\n \tmessage as an inter-paragraph separator. The separator can\n \tbe customized with the `--separator` option.\n+\tEdit the notes to be appended given by `-m` and `-F` options with\n+\t`-e` interactively (using an editor) before appending the note.\n \n edit::\n \tEdit the notes for a given object (defaults to HEAD).\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 8c26e455269..72c8a51cfac 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -32,9 +32,9 @@\n static const char *separator = \"\\n\";\n static const char * const git_notes_usage[] = {\n \tN_(\"git notes [--ref ] [list []]\"),\n-\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] add [-f] [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n \tN_(\"git notes [--ref ] copy [-f] \"),\n-\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] []\"),\n+\tN_(\"git notes [--ref ] append [--allow-empty] [--[no-]separator|--separator=] [--[no-]stripspace] [-m | -F | (-c | -C) ] [] [-e]\"),\n \tN_(\"git notes [--ref ] edit [--allow-empty] []\"),\n \tN_(\"git notes [--ref ] show []\"),\n \tN_(\"git notes [--ref ] merge [-v | -q] [-s ] \"),\n@@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('c', \"reedit-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse and edit specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reedit_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n@@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n \t\tOPT_CALLBACK_F('C', \"reuse-message\", &d, N_(\"object\"),\n \t\t\tN_(\"reuse specified note object\"), PARSE_OPT_NONEG,\n \t\t\tparse_reuse_arg),\n+\t\tOPT_BOOL('e', \"edit\", &d.use_editor,\n+\t\t\tN_(\"edit note message in editor\")),\n \t\tOPT_BOOL(0, \"allow-empty\", &allow_empty,\n \t\t\tN_(\"allow storing empty note\")),\n \t\tOPT_CALLBACK_F(0, \"separator\", &separator,\ndiff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\nindex 99137fb2357..813dfd8db97 100755\n--- a/t/t3301-notes.sh\n+++ b/t/t3301-notes.sh\n@@ -1567,4 +1567,67 @@ test_expect_success 'empty notes do not invoke the editor' '\n \tgit notes remove HEAD\n '\n \n+test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n+\ttest_commit 19th &&\n+\techo \"edited\" >expect &&\n+\tMSG=\"$(cat expect)\" git notes add -m \"initial\" -e &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove HEAD &&\n+\n+\t# Add a note using -F and edit it\n+\techo \"initial\" >note_file &&\n+\tMSG=\"$(cat expect)\" git notes add -F note_file -e &&\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n+\ttest_commit 20th &&\n+\tcat >expect <<-EOF &&\n+\t\tinitial\n+\n+\t\tedited\n+\tEOF\n+\tgit notes add -m \"initial\" &&\n+\tMSG=\"edited\" git notes append -m \"appended\" -e &&\n+\n+\t# Verify the note content was appended and edited\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual &&\n+\tgit notes remove HEAD &&\n+\n+\t# Append a note using -F and edit it\n+\techo \"note from file\" >note_file &&\n+\tgit notes add -m \"initial\" &&\n+\tMSG=\"edited\" git notes append -F note_file -e &&\n+\n+\t# Verify notes from file has been edited in editor and appended\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git notes with a combination of -m, -F and -e invokes editor' '\n+\ttest_commit 21st &&\n+\techo \"foo-file-1\" >note_1 &&\n+\techo \"foo-file-2\" >note_2 &&\n+\techo \"edited\" >expect &&\n+\n+\tMSG=$(cat expect) git notes append -F note_1 -m \"message-1\" -F note_2 -e &&\n+\n+\t# Verify that combined messages from file and -m have been edited\n+\tgit notes show >actual &&\n+\ttest_cmp expect actual\n+'\n+test_expect_success 'git notes append aborts when editor fails with -e' '\n+\ttest_commit 22nd &&\n+\techo \"foo-file-1\" >note_1 &&\n+\n+\t# Try to append a note with -F and -e, but make the editor fail\n+\ttest_env GIT_EDITOR=\"false\" test_must_fail git notes append -F note_1 -e &&\n+\n+\t# Verify that no note was added due to editor failure\n+\ttest_must_fail git notes show\n+'\n+\n test_done\n\nbase-commit: 15030f9556f545b167b1879b877a5d780252dc16\n-- \ngitgitgadget\n"},{"id":"505720","messageId":"CAPig+cSb-2c1kUfOQ0Qzdh7ZXfSeF2FoboSvwYUPfkXJ=56BZw@mail.gmail.com","threadId":"62365","inReplyTo":"ZxZByGdre-lup2sO@pks.im","subject":"Re: [PATCH v2] notes: teach the -e option to edit messages in editor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-21T18:22:52Z","receivedAt":"2024-10-21T18:23:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 21, 2024 at 8:00 AM Patrick Steinhardt wrote:\n> On Sun, Oct 20, 2024 at 12:03:00AM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > + echo \"Initial note message\" >expect &&\n> > + echo \"\" >>expect &&\n> > + echo \"Appended edited note message\" >>expect &&\n>\n> When you want to write multiple lines we typically use HERE docs. E.g.:\n>\n> cat >expect <<-EOF &&\n> Initial note message\n>\n> Appended edited note message\n> EOF\n\nNit: Since there are no variable interpolations inside the heredoc\nbody and we don't expect any, we'd normally indicate that by using\n\\EOF rather than EOF:\n\n cat >expect <<-\\EOF &&\n"},{"id":"505721","messageId":"CAPig+cSw_F97nBzO3Z7t2Zrv5TZwGnYiQLhpq2iKgLfxhhxvfQ@mail.gmail.com","threadId":"62365","inReplyTo":"ZxaG67wuvjOXc5kr@nand.local","subject":"Re: [PATCH v3] notes: teach the -e option to edit messages in editor","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-10-21T18:28:05Z","receivedAt":"2024-10-21T18:28:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 21, 2024 at 12:53 PM Taylor Blau wrote:\n> On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > + MSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n> > + echo \"Edited notes message\" >expect &&\n>\n> Very nice use of the fake_editor script here.\n>\n> It is a little cumbersome to repeat the same message in MSG= and when\n> populating the 'expect' file. Perhaps instead this could be written as:\n>\n> echo \"edited notes message\" >expect &&\n> MSG=\"$(cat expect)\" git notes -add -m \"initial\" -e\n\nThis suggested rewrite feels unusually roundabout and increases\ncognitive load for readers who now have to trace the message flow from\nscript to file and back into script, and to consider how the loss of\ntrailing newline caused by use of $(...) impacts the behavior. It also\nwastes a subprocess (which can be expensive on some platforms, such as\nWindows). If we're really concerned about this minor duplication of\nthe message, we can instead do this:\n\n msg=\"edited notes message\" &&\n echo \"$msg\" >expect &&\n MSG=\"$msg\" git notes -add -m \"initial\" -e\n"},{"id":"505735","messageId":"Zxaw+pUMT4juAePD@nand.local","threadId":"62365","inReplyTo":"CAPig+cSw_F97nBzO3Z7t2Zrv5TZwGnYiQLhpq2iKgLfxhhxvfQ@mail.gmail.com","subject":"Re: [PATCH v3] notes: teach the -e option to edit messages in editor","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T19:52:26Z","receivedAt":"2024-10-21T19:52:29Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 02:28:05PM -0400, Eric Sunshine wrote:\n> On Mon, Oct 21, 2024 at 12:53 PM Taylor Blau wrote:\n> > On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > > + MSG=\"Edited notes message\" git notes add -m \"Initial notes message\" -e &&\n> > > + echo \"Edited notes message\" >expect &&\n> >\n> > Very nice use of the fake_editor script here.\n> >\n> > It is a little cumbersome to repeat the same message in MSG= and when\n> > populating the 'expect' file. Perhaps instead this could be written as:\n> >\n> > echo \"edited notes message\" >expect &&\n> > MSG=\"$(cat expect)\" git notes -add -m \"initial\" -e\n>\n> This suggested rewrite feels unusually roundabout and increases\n> cognitive load for readers who now have to trace the message flow from\n> script to file and back into script, and to consider how the loss of\n> trailing newline caused by use of $(...) impacts the behavior. It also\n> wastes a subprocess (which can be expensive on some platforms, such as\n> Windows). If we're really concerned about this minor duplication of\n> the message, we can instead do this:\n>\n> msg=\"edited notes message\" &&\n> echo \"$msg\" >expect &&\n> MSG=\"$msg\" git notes -add -m \"initial\" -e\n\nI am not sure I agree that the suggested version is clearer. The way I\nread mine is that we are writing what we expect to see in \"expect\", and\nthen setting up MSG to be the same thing.\n\nI definitely do not feel strongly between the two and would rather avoid\nnitpicking something as trivial as this when compared to the rest of the\npatch, especially considering that I would be equally happy with your\nversion.\n\nI think the whole thing is a little bit of a moot point, though, because\nby far the thing that is least clear here is that setting MSG has the\nside-effect of modifying the behavior of the fake-editor that we set up\nearlier in the script. So I don't know that optimizing for clarity in\nhow we setup and write to the \"expect\" file is the right thing to spend\nour time on.\n\nThanks,\nTaylor\n"},{"id":"505736","messageId":"ZxaxHmIP0iJZfvHr@nand.local","threadId":"62365","inReplyTo":"pull.1817.v4.git.1729534340786.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] notes: teach the -e option to edit messages in editor","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T19:53:02Z","receivedAt":"2024-10-21T19:53:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> From: Abraham Samuel Adekunle \n>\n> Notes can be added to a commit using:\n> \t- \"-m\" to provide a message on the command line.\n> \t- -C to copy a note from a blob object.\n> \t- -F to read the note from a file.\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 edited\n> after the messages have been provided through -[mF].\n>\n> Signed-off-by: Abraham Samuel Adekunle \n\nThanks, will queue.\n\nThanks,\nTaylor\n"},{"id":"505781","messageId":"CADYq+fbHKf=YZvLYnWPyqctFeunz0dPxt_aw3t-J2Zd_-vpMfA@mail.gmail.com","threadId":"62365","inReplyTo":"ZxaxHmIP0iJZfvHr@nand.local","subject":"Re: [PATCH v4] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-21T21:44:15Z","receivedAt":"2024-10-21T21:44:26Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Mon, 21 Oct 2024, 20:53 Taylor Blau, wrote:\n>\n> On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > From: Abraham Samuel Adekunle \n> >\n> > Notes can be added to a commit using:\n> > - \"-m\" to provide a message on the command line.\n> > - -C to copy a note from a blob object.\n> > - -F to read the note from a file.\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 edited\n> > after the messages have been provided through -[mF].\n> >\n> > Signed-off-by: Abraham Samuel Adekunle \n>\n> Thanks, will queue.\n>\n\nThank you, Taylor, my Outreachy mentors Patrick and Phillip, Junio,\nand all the maintainers for your time and patience in reviewing\nmy patches. It has been a good learning period. Thanks once again.\nAbraham Samuel\n>\n> Thanks,\n> Taylor\n"},{"id":"505895","messageId":"ZxiUTbmGpeK3KnOx@pks.im","threadId":"62365","inReplyTo":"pull.1817.v4.git.1729534340786.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] notes: teach the -e option to edit messages in editor","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-23T06:14:41Z","receivedAt":"2024-10-23T06:14:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> From: Abraham Samuel Adekunle \n> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> index 99137fb2357..813dfd8db97 100755\n> --- a/t/t3301-notes.sh\n> +++ b/t/t3301-notes.sh\n> @@ -1567,4 +1567,67 @@ test_expect_success 'empty notes do not invoke the editor' '\n> \tgit notes remove HEAD\n> '\n> \n> +test_expect_success 'git notes add with -m/-F invokes editor with -e' '\n> +\ttest_commit 19th &&\n> +\techo \"edited\" >expect &&\n> +\tMSG=\"$(cat expect)\" git notes add -m \"initial\" -e &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit notes remove HEAD &&\n> +\n> +\t# Add a note using -F and edit it\n> +\techo \"initial\" >note_file &&\n> +\tMSG=\"$(cat expect)\" git notes add -F note_file -e &&\n> +\tgit notes show >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n> +\ttest_commit 20th &&\n> +\tcat >expect <<-EOF &&\n> +\t\tinitial\n> +\n> +\t\tedited\n> +\tEOF\n\nNit: we typically align the contents of HERE docs with `cat`. I'm not a\nhuge fan of it and had been struggling with it initially, as well, but\ncoding style is subjective anyway and it's totally fine that one doesn't\nagree with everything.\n\nIn any case, I don't think this warrants a reroll of this patch, just\nto keep it in mind for future patches you may send.\n\n[snip]\n> +test_expect_success 'git notes append aborts when editor fails with -e' '\n> +\ttest_commit 22nd &&\n> +\techo \"foo-file-1\" >note_1 &&\n> +\n> +\t# Try to append a note with -F and -e, but make the editor fail\n> +\ttest_env GIT_EDITOR=\"false\" test_must_fail git notes append -F note_1 -e &&\n> +\n> +\t# Verify that no note was added due to editor failure\n> +\ttest_must_fail git notes show\n> +'\n> +\n\nNice.\n\nThanks, this looks good to me overall.\n\nPatrick\n"},{"id":"505950","messageId":"CADYq+fbb6e0z9V41gurNps53RMxOeZgO++c0HSwc8BMPpgakyA@mail.gmail.com","threadId":"62365","inReplyTo":"ZxiUTbmGpeK3KnOx@pks.im","subject":"Re: [PATCH v4] notes: teach the -e option to edit messages in editor","fromName":"Samuel Abraham","fromEmail":"abrahamadekunle50@gmail.com","sentAt":"2024-10-23T16:51:28Z","receivedAt":"2024-10-23T16:51:39Z","isPatch":true,"sender":{"key":"abrahamadekunle50@gmail.com","avatar":"https://avatars.githubusercontent.com/u/110066922?v=4"},"body":"On Wed, Oct 23, 2024 at 7:14 AM Patrick Steinhardt wrote:\n>\n> On Mon, Oct 21, 2024 at 06:12:20PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote:\n> > From: Abraham Samuel Adekunle \n> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh\n> > index 99137fb2357..813dfd8db97 100755\n> > --- a/t/t3301-notes.sh\n> > +++ b/t/t3301-notes.sh\n> > @@ -1567,4 +1567,67 @@ 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 editor with -e' '\n> > + test_commit 19th &&\n> > + echo \"edited\" >expect &&\n> > + MSG=\"$(cat expect)\" git notes add -m \"initial\" -e &&\n> > + git notes show >actual &&\n> > + test_cmp expect actual &&\n> > + git notes remove HEAD &&\n> > +\n> > + # Add a note using -F and edit it\n> > + echo \"initial\" >note_file &&\n> > + MSG=\"$(cat expect)\" git notes add -F note_file -e &&\n> > + git notes show >actual &&\n> > + test_cmp expect actual\n> > +'\n> > +\n> > +test_expect_success 'git notes append with -m/-F invokes the editor with -e' '\n> > + test_commit 20th &&\n> > + cat >expect <<-EOF &&\n> > + initial\n> > +\n> > + edited\n> > + EOF\n>\n> Nit: we typically align the contents of HERE docs with `cat`. I'm not a\n> huge fan of it and had been struggling with it initially, as well, but\n> coding style is subjective anyway and it's totally fine that one doesn't\n> agree with everything.\n>\n> In any case, I don't think this warrants a reroll of this patch, just\n> to keep it in mind for future patches you may send.\n>\n\nOkay, I will keep that in mind. Thanks\n> [snip]\n> > +test_expect_success 'git notes append aborts when editor fails with -e' '\n> > + test_commit 22nd &&\n> > + echo \"foo-file-1\" >note_1 &&\n> > +\n> > + # Try to append a note with -F and -e, but make the editor fail\n> > + test_env GIT_EDITOR=\"false\" test_must_fail git notes append -F note_1 -e &&\n> > +\n> > + # Verify that no note was added due to editor failure\n> > + test_must_fail git notes show\n> > +'\n> > +\n>\n> Nice.\n>\n> Thanks, this looks good to me overall.\n>\n> Patrick\n\nThank you very much, I really appreciate.\nAbraham Samuel.\n"}]}