On Sat, Oct 19, 2024 at 12:04 PM Kristoffer Haugsbakk <kristofferhaugsbakk@xxxxxxxxxxxx> 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
