| Message ID | pull.1817.git.1729296853800.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | notes: teach the -e option to edit messages in editor | expand |
On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote: > From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> > > Notes can be added to a commit using the -m (message), > -C (copy a note from a blob object) or > -F (read the note from a file) options. > When these options are used, Git does not open an editor, > it simply takes the content provided via these options and > attaches it to the commit as a note. > > Improve flexibility to fine-tune the note before finalizing it > by allowing the messages to be prefilled in the editor and editted > after the messages have been provided through -[mF]. I don't use the notes feature, but I definitely see how this is valuable there much as it is for `git commit`. > diff --git a/builtin/notes.c b/builtin/notes.c > index 8c26e455269..02cdfdf1c9d 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix) > OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"), > N_("reuse and edit specified note object"), PARSE_OPT_NONEG, > parse_reedit_arg), > + OPT_BOOL('e', "edit", &d.use_editor, > + N_("edit note message in editor")), > OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), > N_("reuse specified note object"), PARSE_OPT_NONEG, > parse_reuse_arg), > @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) > OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), > N_("reuse specified note object"), PARSE_OPT_NONEG, > parse_reuse_arg), > + OPT_BOOL('e', "edit", &d.use_editor, > + N_("edit note message in editor")), > OPT_BOOL(0, "allow-empty", &allow_empty, > N_("allow storing empty note")), > OPT_CALLBACK_F(0, "separator", &separator, > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh > index 99137fb2357..7f45a324faa 100755 > --- a/t/t3301-notes.sh > +++ b/t/t3301-notes.sh > @@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' ' > git notes remove HEAD > ' > > +test_expect_success '"git notes add" with -m/-F invokes the editor with -e' ' > + test_commit 19th && > + GIT_EDITOR="true" git notes add -m "note message" -e && > + git notes remove HEAD && > + echo "message from file" >file_1 && > + GIT_EDITOR="true" git notes add -F file_1 -e && > + git notes remove HEAD > +' Maybe I don't understand what this is supposed to be testing (and if so, please correct me), but how are we verifying that the editor is being invoked? If we're invoking `true`, then that doesn't change the message in any way, so if we suddenly stopped invoking the editor, I don't think this would fail. Maybe we could use something else as `GIT_EDITOR` instead. For example, if we did `GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e` (with an appropriate PERL prerequisite), then we could test that the message after the fact was "message from editor", which would help us verify that both the `-F` and `-e` options were being honoured. (Similar things can be said about the tests you added below this as well.) I suggest Perl here because `sed -i` is nonstandard and not portable, but you could also set up a fake editor script as in t7004, which would avoid the need for the Perl dependency by using `sed` with a temporary file. That might be more palatable to the project at large, but I'd be fine with either approach. Do you think there are any cases where testing the `--no-edit` functionality might be helpful? For example, is `git notes edit` ever useful to invoke with such an option, like one might do with `git commit amend`? (This isn't rhetorical, since the notes code is one of the areas of Git I'm least familiar with, so I ask both because I'm curious and if you think it's a useful thing to do, then tests might be a good idea.)
Hi Samuel On Sat, Oct 19, 2024, at 02:14, Samuel Adekunle Abraham via GitGitGadget wrote: > From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> > > Notes can be added to a commit using the -m (message), > -C (copy a note from a blob object) or > -F (read the note from a file) options. > When these options are used, Git does not open an editor, > it simply takes the content provided via these options and > attaches it to the commit as a note. Thanks for this work. I think part of the motivation here is to make git-notes(1) act more in line with the conventions from git-commit(1), which is always nice to see. It’s also useful in its own right. > Improve flexibility to fine-tune the note before finalizing it > by allowing the messages to be prefilled in the editor and editted > after the messages have been provided through -[mF]. Here you explain how the end-user will benefit from this change. Nice. It’s important to explain the background, what is being done, and why it is being done. And this commit message does all of that.
Hi brian and Samuel On Sat, Oct 19, 2024, at 02:38, brian m. carlson wrote: > On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote: >> +test_expect_success '"git notes add" with -m/-F invokes the editor with -e' ' >> + test_commit 19th && >> + GIT_EDITOR="true" git notes add -m "note message" -e && >> + git notes remove HEAD && >> + echo "message from file" >file_1 && >> + GIT_EDITOR="true" git notes add -F file_1 -e && >> + git notes remove HEAD >> +' > > Maybe I don't understand what this is supposed to be testing (and if so, > please correct me), but how are we verifying that the editor is being > invoked? If we're invoking `true`, then that doesn't change the message > in any way, so if we suddenly stopped invoking the editor, I don't think > this would fail. I also didn’t understand these tests. There is this test in this file/test suite which tests the negative case: test_expect_success 'empty notes do not invoke the editor' ' test_commit 18th && GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty && git notes remove HEAD && GIT_EDITOR="false" git notes add -m "" --allow-empty && git notes remove HEAD && GIT_EDITOR="false" git notes add -F /dev/null --allow-empty && git notes remove HEAD ' And this works because the commands would fail if the editor was invoked: error: there was a problem with the editor 'false' But this doesn’t work for `true`. > Maybe we could use something else as `GIT_EDITOR` instead. For example, > if we did `GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e` > (with an appropriate PERL prerequisite), then we could test that the > message after the fact was "message from editor", which would help us > verify that both the `-F` and `-e` options were being honoured. > (Similar things can be said about the tests you added below this as > well.) This file defines a `fake_editor`:[1] write_script fake_editor <<\EOF echo "$MSG" >"$1" echo "$MSG" >&2 EOF GIT_EDITOR=./fake_editor export GIT_EDITOR And it looks like this is how it is used: test_expect_success 'create notes' ' MSG=b4 git notes add && test_path_is_missing .git/NOTES_EDITMSG && git ls-tree -r refs/notes/commits >actual && test_line_count = 1 actual && echo b4 >expect && git notes show >actual && test_cmp expect actual && git show HEAD^ && test_must_fail git notes show HEAD^ ' So it seems that the new tests here should use the `test_cmp expect actual` style. † 1: The different test files use both `fake_editor`, `fake-editor`, and `fakeeditor`. > Do you think there are any cases where testing the `--no-edit` > functionality might be helpful? For example, is `git notes edit` ever > useful to invoke with such an option, like one might do with `git commit > amend`? (This isn't rhetorical, since the notes code is one of the areas > of Git I'm least familiar with, so I ask both because I'm curious and if > you think it's a useful thing to do, then tests might be a good idea.) Yes, that is useful (both as a use-case and as a regression test[2]). git-notes(1) is often used to programmatically add metadata: git show todo:post-applypatch | grep -C5 refs/notes/amlog (And this non-interactive example is not affected by this change since `-e` is required in order to invoke the editor) † 2: I seem to recall a regression in how git-notes(1) chose to invoke the editor or not
On Sat, Oct 19, 2024 at 11:28 AM Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com> wrote: > > Hi Samuel > > On Sat, Oct 19, 2024, at 02:14, Samuel Adekunle Abraham via GitGitGadget wrote: > > From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com> > > > > Notes can be added to a commit using the -m (message), > > -C (copy a note from a blob object) or > > -F (read the note from a file) options. > > When these options are used, Git does not open an editor, > > it simply takes the content provided via these options and > > attaches it to the commit as a note. > > Thanks for this work. I think part of the motivation here is to make > git-notes(1) act more in line with the conventions from git-commit(1), > which is always nice to see. > > It’s also useful in its own right. > > > Improve flexibility to fine-tune the note before finalizing it > > by allowing the messages to be prefilled in the editor and editted > > after the messages have been provided through -[mF]. > > Here you explain how the end-user will benefit from this change. Nice. > > It’s important to explain the background, what is being done, and why it > is being done. And this commit message does all of that. Hello Kristoffer, Thank you very much for your response.
On Sat, Oct 19, 2024 at 12:04 PM Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com> wrote: > > Hi brian and Samuel > > On Sat, Oct 19, 2024, at 02:38, brian m. carlson wrote: > > On 2024-10-19 at 00:14:13, Samuel Adekunle Abraham via GitGitGadget wrote: > >> +test_expect_success '"git notes add" with -m/-F invokes the editor with -e' ' > >> + test_commit 19th && > >> + GIT_EDITOR="true" git notes add -m "note message" -e && > >> + git notes remove HEAD && > >> + echo "message from file" >file_1 && > >> + GIT_EDITOR="true" git notes add -F file_1 -e && > >> + git notes remove HEAD > >> +' > > > > Maybe I don't understand what this is supposed to be testing (and if so, > > please correct me), but how are we verifying that the editor is being > > invoked? If we're invoking `true`, then that doesn't change the message > > in any way, so if we suddenly stopped invoking the editor, I don't think > > this would fail. > > I also didn’t understand these tests. > > There is this test in this file/test suite which tests the negative > case: > > test_expect_success 'empty notes do not invoke the editor' ' > test_commit 18th && > GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty && > git notes remove HEAD && > GIT_EDITOR="false" git notes add -m "" --allow-empty && > git notes remove HEAD && > GIT_EDITOR="false" git notes add -F /dev/null --allow-empty && > git notes remove HEAD > ' > Thank you Kristoffer, Yes incorrectly used this as a reference and I have however look deeper into the implementation of the write_scripts function and the fake_editor file for better understanding. > And this works because the commands would fail if the editor was invoked: > > error: there was a problem with the editor 'false' > > But this doesn’t work for `true`. > > > Maybe we could use something else as `GIT_EDITOR` instead. For example, > > if we did `GIT_EDITOR="perl -pi -e s/file/editor/" git notes add -F file_1 -e` > > (with an appropriate PERL prerequisite), then we could test that the > > message after the fact was "message from editor", which would help us > > verify that both the `-F` and `-e` options were being honoured. > > (Similar things can be said about the tests you added below this as > > well.) > > This file defines a `fake_editor`:[1] > > write_script fake_editor <<\EOF > echo "$MSG" >"$1" > echo "$MSG" >&2 > EOF > GIT_EDITOR=./fake_editor > export GIT_EDITOR > > And it looks like this is how it is used: > > test_expect_success 'create notes' ' > MSG=b4 git notes add && > test_path_is_missing .git/NOTES_EDITMSG && > git ls-tree -r refs/notes/commits >actual && > test_line_count = 1 actual && > echo b4 >expect && > git notes show >actual && > test_cmp expect actual && > git show HEAD^ && > test_must_fail git notes show HEAD^ > ' > > So it seems that the new tests here should use the `test_cmp expect > actual` style. Thank you very much for the guide. I will correct them and send a modified patch. > > † 1: The different test files use both `fake_editor`, `fake-editor`, > and `fakeeditor`. > > > Do you think there are any cases where testing the `--no-edit` > > functionality might be helpful? For example, is `git notes edit` ever > > useful to invoke with such an option, like one might do with `git commit > > amend`? (This isn't rhetorical, since the notes code is one of the areas > > of Git I'm least familiar with, so I ask both because I'm curious and if > > you think it's a useful thing to do, then tests might be a good idea.) > > Yes, that is useful (both as a use-case and as a regression test[2]). > git-notes(1) is often used to programmatically add metadata: > > git show todo:post-applypatch | grep -C5 refs/notes/amlog > > (And this non-interactive example is not affected by this change since > `-e` is required in order to invoke the editor) > > † 2: I seem to recall a regression in how git-notes(1) chose to invoke > the editor or not
On Sat, Oct 19, 2024, at 02:14, Samuel Adekunle Abraham via GitGitGadget wrote: > builtin/notes.c | 4 ++++ > t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) The documentation should be updated: Documentation/git-notes.txt > diff --git a/builtin/notes.c b/builtin/notes.c > index 8c26e455269..02cdfdf1c9d 100644 > --- a/builtin/notes.c > +++ b/builtin/notes.c > @@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const > char *prefix) > OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"), > N_("reuse and edit specified note object"), PARSE_OPT_NONEG, > parse_reedit_arg), > + OPT_BOOL('e', "edit", &d.use_editor, > + N_("edit note message in editor")), The `add` subcommand does what I expect it to after some testing. > OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), > N_("reuse specified note object"), PARSE_OPT_NONEG, > parse_reuse_arg), > @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, > const char *prefix) > OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), > N_("reuse specified note object"), PARSE_OPT_NONEG, > parse_reuse_arg), > + OPT_BOOL('e', "edit", &d.use_editor, > + N_("edit note message in editor")), > OPT_BOOL(0, "allow-empty", &allow_empty, > N_("allow storing empty note")), > OPT_CALLBACK_F(0, "separator", &separator, Likewise for the `append` subcommand.
On Sat, Oct 19, 2024 at 12:37 PM Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com> wrote: > > On Sat, Oct 19, 2024, at 02:14, Samuel Adekunle Abraham via GitGitGadget wrote: > > builtin/notes.c | 4 ++++ > > t/t3301-notes.sh | 29 +++++++++++++++++++++++++++++ > > 2 files changed, 33 insertions(+) > > The documentation should be updated: > > Documentation/git-notes.txt > > > diff --git a/builtin/notes.c b/builtin/notes.c > > index 8c26e455269..02cdfdf1c9d 100644 > > --- a/builtin/notes.c > > +++ b/builtin/notes.c > > @@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const > > char *prefix) > > OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"), > > N_("reuse and edit specified note object"), PARSE_OPT_NONEG, > > parse_reedit_arg), > > + OPT_BOOL('e', "edit", &d.use_editor, > > + N_("edit note message in editor")), > > The `add` subcommand does what I expect it to after some testing. > > > OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), > > N_("reuse specified note object"), PARSE_OPT_NONEG, > > parse_reuse_arg), > > @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, > > const char *prefix) > > OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), > > N_("reuse specified note object"), PARSE_OPT_NONEG, > > parse_reuse_arg), > > + OPT_BOOL('e', "edit", &d.use_editor, > > + N_("edit note message in editor")), > > OPT_BOOL(0, "allow-empty", &allow_empty, > > N_("allow storing empty note")), > > OPT_CALLBACK_F(0, "separator", &separator, > > Likewise for the `append` subcommand. > > -- > Kristoffer Haugsbakk > Okay, I will do that. Thank you very much, Kristoffer.
diff --git a/builtin/notes.c b/builtin/notes.c index 8c26e455269..02cdfdf1c9d 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -489,6 +489,8 @@ static int add(int argc, const char **argv, const char *prefix) OPT_CALLBACK_F('c', "reedit-message", &d, N_("object"), N_("reuse and edit specified note object"), PARSE_OPT_NONEG, parse_reedit_arg), + OPT_BOOL('e', "edit", &d.use_editor, + N_("edit note message in editor")), OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), N_("reuse specified note object"), PARSE_OPT_NONEG, parse_reuse_arg), @@ -667,6 +669,8 @@ static int append_edit(int argc, const char **argv, const char *prefix) OPT_CALLBACK_F('C', "reuse-message", &d, N_("object"), N_("reuse specified note object"), PARSE_OPT_NONEG, parse_reuse_arg), + OPT_BOOL('e', "edit", &d.use_editor, + N_("edit note message in editor")), OPT_BOOL(0, "allow-empty", &allow_empty, N_("allow storing empty note")), OPT_CALLBACK_F(0, "separator", &separator, diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh index 99137fb2357..7f45a324faa 100755 --- a/t/t3301-notes.sh +++ b/t/t3301-notes.sh @@ -1567,4 +1567,33 @@ test_expect_success 'empty notes do not invoke the editor' ' git notes remove HEAD ' +test_expect_success '"git notes add" with -m/-F invokes the editor with -e' ' + test_commit 19th && + GIT_EDITOR="true" git notes add -m "note message" -e && + git notes remove HEAD && + echo "message from file" >file_1 && + GIT_EDITOR="true" git notes add -F file_1 -e && + git notes remove HEAD +' + +test_expect_success 'git notes append with -m/-F invokes editor with -e' ' + test_commit 20th && + GIT_EDITOR="true" git notes add -m "initial note" -e && + GIT_EDITOR="true" git notes append -m "appended note" -e && + git notes remove HEAD && + echo "initial note" >note_a && + echo "appended note" >note_b && + GIT_EDITOR="true" git notes add -F note_a -e && + GIT_EDITOR="true" git notes append -F note_b -e && + git notes remove HEAD +' + +test_expect_success 'append note with multiple combinations of -m, -F and -e, invokes editor' ' + test_commit 21st && + echo "foo-file-1" >note_1 && + echo "foo-file-2" >note_2 && + GIT_EDITOR="true" git notes append -F note_1 -m "message-1" -F note_2 -m "message-2" -e && + git notes remove HEAD +' + test_done