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

From: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Date: Oct 19, 2024, 11:03 UTC
Message-ID: <55176c6e-7ac1-46e6-affa-184307a0edf4@app.fastmail.com>
In-Reply-To: <ZxL_oAXN5KQ4FVMc@tapette.crustytoothpaste.net>

Hi brian and Samuel

On Sat, Oct 19, 2024, at 02:38, brian m. carlson wrote:

Show 15 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

> 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