Re: [PATCH] notes: teach the -e option to edit messages in editor

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