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
