Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Jul 25, 2024, 15:52 UTC
Message-ID: <5fdccb84-1e60-477e-99d9-b22e3ef83f54@app.fastmail.com>
In-Reply-To: <20240725144548.3434-2-ddiss@suse.de>

Hi

On Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote:

> 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> changed note_data.given logic such that it's no longer set if a zero
> length file or blob object is provided.

This project uses the `git show -s --pretty=reference` format:

    90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>'
    option, 2023-05-27)

Show 20 quoted lines

>  t/t3301-notes.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 536bd11ff4..c0dbacc161 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by
> git log' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'empty notes do not invoke the editor' '
> +	test_commit 18th &&
> +	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
> +'
> +
>  test_done
> --
> 2.43.0

This test fails, obviously. Maybe you can reorder the patches so that both two patches pass the test suite?

Introducing a regression test in one patch and then fixing the bug (and making the test pass) in the next patch is a style that some prefer. But I have received feedback before that it’s best to avoid that.

-- 
Kristoffer Haugsbakk