Re: [PATCH v3] format-patch: assume --cover-letter for diff in multi-patch series

From: Rubén Justo <rjusto@gmail.com>
Date: Jun 5, 2024, 21:24 UTC
Message-ID: <5aebe520-e540-46b4-a887-af488fe2663a@gmail.com>
In-Reply-To: <xmqqr0dbqfv8.fsf@gitster.g>

On Wed, Jun 05, 2024 at 01:44:27PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> > +test_expect_success "format-patch --range-diff, implicit --cover-letter" '
> > +	test_must_fail git format-patch --no-cover-letter \
> > +		-v2 --range-diff=topic main..unmodified &&
> > +	test_must_fail git -c format.coverLetter=no format-patch \
> > +		-v2 --range-diff=topic main..unmodified &&
> > +	git format-patch -v2 --range-diff=topic main..unmodified &&
> > +	test_when_finished "rm v2-000?-*" &&
> > +	test_grep "^Range-diff against v1:$" v2-0000-cover-letter.patch
> > +'
> 
> Isn't this doing three separate things in a single test?  Unless it
> is the local convention in this script, let's split them to three.

Honestly, I don't have a strong opinion on this. I know, though, there are others who like to pack as much as possible into one test.

I see your point. However, I can also accept that testing in the same test the simple exceptions for the implicit --cover-letter with --range-diff, or --interdiff in the other one below, makes sense.

> If "--no-cover-letter" fails to prevent v2-* files from getting
> created, it would fail without hitting test_when_finished.  v2 was
> already bad enough in that regard, but piling two more things that
> could fail on top is making it even worse, no?

I'm curious, a test like:

test_expect_success "format-patch --range-diff, implicit --cover-letter" '
	test_when_finished "rm v2-000?-*" &&
	test_must_fail git format-patch --no-cover-letter
		-v2 --range-diff=topic main..unmodified

isn't it confusing?

Thanks.

Show 21 quoted lines

> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index ba85b582c5..b96348eebd 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -2492,6 +2492,16 @@ test_expect_success 'interdiff: solo-patch' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '
> > +	test_must_fail git format-patch --no-cover-letter \
> > +		--interdiff=boop~2 -2 -v23 &&
> > +	test_must_fail git -c format.coverLetter=no format-patch \
> > +		--interdiff=boop~2 -2 -v23 &&
> > +	git format-patch --interdiff=boop~2 -2 -v23 &&
> > +	test_grep "^Interdiff against v22:$" v23-0000-cover-letter.patch &&
> > +	test_cmp expect actual
> > +'
> > +
> >  test_expect_success 'format-patch does not respect diff.noprefix' '
> >  	git -c diff.noprefix format-patch -1 --stdout >actual &&
> >  	grep "^--- a/blorp" actual