From: David Disseldorp <ddiss@suse.de>
Date: Thu, 25 Jul 2024 16:03:22 GMT
Subject: Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add
Message-ID: <20240725180322.5b6d3a0d.ddiss@suse.de>
In-Reply-To: <5fdccb84-1e60-477e-99d9-b22e3ef83f54@app.fastmail.com>

On Thu, 25 Jul 2024 17:52:25 +0200, Kristoffer Haugsbakk wrote:

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

Okay, will fix in a subsequent version. 

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

Makes sense, thanks for the feedback.