threads / patch / 61580

patchformat-patch: assume --cover-letter for diff in multi-patch series

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

## tl;dr

21 messages between Jun 3, 2024 and Jun 7, 2024. Diffs are folded; open one to read it.

replies: 20people: 3as markdown or json

Rubén Justo· Jun 3, 2024, 22:49 UTC · lore

If either `--interdiff` or `--range-diff` is specified without `--cover-letter`, we'll abort if it would result in a multi-patch series being generated. Because the cover-letter is needed to give the diff text in a multi-patch series.

Considering that `format-patch` generates a multi-patch as needed, let's adopt a similar "cover as necessary" approach when using `--interdiff` or `--range-diff`.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/log.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
Show changes to builtin/log.c +8 −4
diff --git a/builtin/log.c b/builtin/log.c
index c8ce0c0d88..56101672f8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2286,8 +2286,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		rev.total = total + start_number - 1;
 
 	if (idiff_prev.nr) {
-		if (!cover_letter && total != 1)
-			die(_("--interdiff requires --cover-letter or single patch"));
+		if (!cover_letter && total != 1) {
+			warning(_("--interdiff implies --cover-letter for multi-patch series"));
+			cover_letter = 1;
+		}
 		rev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];
 		rev.idiff_oid2 = get_commit_tree_oid(list[0]);
 		rev.idiff_title = diff_title(&idiff_title, reroll_count,
@@ -2301,8 +2303,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		die(_("the option '%s' requires '%s'"), "--creation-factor", "--range-diff");
 
 	if (rdiff_prev) {
-		if (!cover_letter && total != 1)
-			die(_("--range-diff requires --cover-letter or single patch"));
+		if (!cover_letter && total != 1) {
+			warning(_("--range-diff implies --cover-letter for multi-patch series"));
+			cover_letter = 1;
+		}
 
 		infer_range_diff_ranges(&rdiff1, &rdiff2, rdiff_prev,
 					origin, list[0]);

-- 
2.45.2.405.gf8e6085128
Patrick Steinhardt· Jun 4, 2024, 08:02 UTC · re: Rubén Justo · lore

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

On Tue, Jun 04, 2024 at 12:49:35AM +0200, Rubén Justo wrote:
Show 8 quoted lines
> If either `--interdiff` or `--range-diff` is specified without
> `--cover-letter`, we'll abort if it would result in a multi-patch series
> being generated.  Because the cover-letter is needed to give the diff
> text in a multi-patch series.
> 
> Considering that `format-patch` generates a multi-patch as needed, let's
> adopt a similar "cover as necessary" approach when using `--interdiff`
> or `--range-diff`.
What does git-format-patch(1) do right now in this situation?

In any case, this change should probably have a test or two to demonstrate that it works as advertised.

Thanks!
Patrick
Junio C Hamano· Jun 4, 2024, 17:32 UTC · re: Patrick Steinhardt · lore

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

Patrick Steinhardt <ps@pks.im> writes:
Show 14 quoted lines
> On Tue, Jun 04, 2024 at 12:49:35AM +0200, Rubén Justo wrote:
>> If either `--interdiff` or `--range-diff` is specified without
>> `--cover-letter`, we'll abort if it would result in a multi-patch series
>> being generated.  Because the cover-letter is needed to give the diff
>> text in a multi-patch series.
>> 
>> Considering that `format-patch` generates a multi-patch as needed, let's
>> adopt a similar "cover as necessary" approach when using `--interdiff`
>> or `--range-diff`.
>
> What does git-format-patch(1) do right now in this situation?
>
> In any case, this change should probably have a test or two to
> demonstrate that it works as advertised.

Yes. I think the existing tests for giving --interdiff to a single patch series serves as the "it does not trigger when it shouldn't" side of the test, so a positive "it does what it claims to do" test should be sufficient.

Thanks.
Rubén Justo· Jun 5, 2024, 18:01 UTC · re: Rubén Justo · lore

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

When we deal with a multi-patch series in git-format-patch(1), if we see `--interdiff` or `--range-diff` but no `--cover-letter`, we return with an error, saying:

    fatal: --range-diff requires --cover-letter or single patch
or:
    fatal: --interdiff requires --cover-letter or single patch

This makes sense because the cover-letter is where we place the diff from the previous version.

However, considering that `format-patch` generates a multi-patch as needed, let's adopt a similar "cover as necessary" approach when using `--interdiff` or `--range-diff`.

Therefore, relax the requirement for an explicit `--cover-letter` in a multi-patch series when the user says `--iterdiff` or `--range-diff`.

Still, if only to return the error, respect "format.coverLetter=no" and `--no-cover-letter`.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

This is a hopefully more curated version that better explains the current situation and adds a couple of tests.

Thanks!
 builtin/log.c           | 2 ++
 t/t3206-range-diff.sh   | 6 ++++++
 t/t4014-format-patch.sh | 6 ++++++
 3 files changed, 14 insertions(+)
Show changes to 3 files +14 −0

builtin/log.c, t/t3206-range-diff.sh, t/t4014-format-patch.sh

diff --git a/builtin/log.c b/builtin/log.c
index c8ce0c0d88..8032909d4f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter == -1) {
 		if (config_cover_letter == COVER_AUTO)
 			cover_letter = (total > 1);
+		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
+			cover_letter = (config_cover_letter != COVER_OFF);
 		else
 			cover_letter = (config_cover_letter == COVER_ON);
 	}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 7b05bf3961..5af155805d 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -545,6 +545,12 @@ do
 	'
 done
 
+test_expect_success "format-patch --range-diff, implicit --cover-letter" '
+	git format-patch -v2 --range-diff=topic main..unmodified &&
+	test_when_finished "rm v2-000?-*" &&
+	test_grep "^Range-diff against v1:$" v2-0000-*
+'
+
 test_expect_success 'format-patch --range-diff as commentary' '
 	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
 	test_when_finished "rm 0001-*" &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba85b582c5..c844fbfe47 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2492,6 +2492,12 @@ test_expect_success 'interdiff: solo-patch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '
+	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

-- 
2.45.2.410.g52d620e86a
Junio C Hamano· Jun 5, 2024, 18:17 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 13 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c
> index c8ce0c0d88..8032909d4f 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (cover_letter == -1) {
>  		if (config_cover_letter == COVER_AUTO)
>  			cover_letter = (total > 1);
> +		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
> +			cover_letter = (config_cover_letter != COVER_OFF);
>  		else
>  			cover_letter = (config_cover_letter == COVER_ON);
>  	}

Interesting. So those who really really hate cover letters can set the configuration explicitly to 'off' and giving an --interdiff option would still have the sanity check kick in. Makes sense.

Show 11 quoted lines
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 7b05bf3961..5af155805d 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -545,6 +545,12 @@ do
>  	'
>  done
>  
> +test_expect_success "format-patch --range-diff, implicit --cover-letter" '
> +	git format-patch -v2 --range-diff=topic main..unmodified &&
> +	test_when_finished "rm v2-000?-*" &&
I was about to make the follwoing:
    Swap these two.  Arrange things we are going to create to be
    removed at end, and then start creating them.  That way, we will
    clean them up even if we fail after creating some but before the
    end of the command.

but this one is littered with exiting breakage, so I'll let it pass. Somebody will later go in and fix them all (#leftoverbits).

> +	test_grep "^Range-diff against v1:$" v2-0000-*
> +'

Isn't the name of the cover letter file always cover-letter.patch unless you configure a custom --suffix (which is not the case here)?

Show 13 quoted lines
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index ba85b582c5..c844fbfe47 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -2492,6 +2492,12 @@ test_expect_success 'interdiff: solo-patch' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '
> +	git format-patch --interdiff=boop~2 -2 -v23 &&
> +	test_grep "^Interdiff against v22:$" v23-0000-cover-letter.patch &&
> +	test_cmp expect actual
> +'
OK.
>  test_expect_success 'format-patch does not respect diff.noprefix' '
>  	git -c diff.noprefix format-patch -1 --stdout >actual &&
>  	grep "^--- a/blorp" actual
Looking good.
Junio C Hamano· Jun 5, 2024, 18:58 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index c8ce0c0d88..8032909d4f 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>>  	if (cover_letter == -1) {
>>  		if (config_cover_letter == COVER_AUTO)
>>  			cover_letter = (total > 1);
>> +		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
>> +			cover_letter = (config_cover_letter != COVER_OFF);
>>  		else
>>  			cover_letter = (config_cover_letter == COVER_ON);
>>  	}
>
> Interesting.  So those who really really hate cover letters can set
> the configuration explicitly to 'off' and giving an --interdiff
> option would still have the sanity check kick in.  Makes sense.
This is not covered by the added tests, is it?

We need to test this case: the user asks for --interdiff but at the same time refuses with --no-cover-letter (or its config equivalent) to create a cover letter.

As I said already, everything else looked OK in this patch.
Thanks.
Rubén Justo· Jun 5, 2024, 20:27 UTC · re: Rubén Justo · lore

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

When we deal with a multi-patch series in git-format-patch(1), if we see `--interdiff` or `--range-diff` but no `--cover-letter`, we return with an error, saying:

    fatal: --range-diff requires --cover-letter or single patch
or:
    fatal: --interdiff requires --cover-letter or single patch

This makes sense because the cover-letter is where we place the diff from the previous version.

However, considering that `format-patch` generates a multi-patch as needed, let's adopt a similar "cover as necessary" approach when using `--interdiff` or `--range-diff`.

Therefore, relax the requirement for an explicit `--cover-letter` in a multi-patch series when the user says `--iterdiff` or `--range-diff`.

Still, if only to return the error, respect "format.coverLetter=no" and `--no-cover-letter`.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Range-diff against v2:
1:  ff67f24022 ! 1:  8dc5f16d83 format-patch: assume --cover-letter for diff in multi-patch series
    @@ t/t3206-range-diff.sh: do
      done
      
     +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-*
    ++	test_grep "^Range-diff against v1:$" v2-0000-cover-letter.patch
     +'
     +
      test_expect_success 'format-patch --range-diff as commentary' '
    @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '
      '
      
     +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
 builtin/log.c           |  2 ++
 t/t3206-range-diff.sh   | 10 ++++++++++
 t/t4014-format-patch.sh | 10 ++++++++++
 3 files changed, 22 insertions(+)
Show changes to 3 files +22 −0

builtin/log.c, t/t3206-range-diff.sh, t/t4014-format-patch.sh

diff --git a/builtin/log.c b/builtin/log.c
index c8ce0c0d88..8032909d4f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter == -1) {
 		if (config_cover_letter == COVER_AUTO)
 			cover_letter = (total > 1);
+		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
+			cover_letter = (config_cover_letter != COVER_OFF);
 		else
 			cover_letter = (config_cover_letter == COVER_ON);
 	}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 7b05bf3961..4a597466a2 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -545,6 +545,16 @@ do
 	'
 done
 
+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
+'
+
 test_expect_success 'format-patch --range-diff as commentary' '
 	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
 	test_when_finished "rm 0001-*" &&
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

-- 
2.45.2.410.g52d620e86a
Junio C Hamano· Jun 5, 2024, 20:44 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 9 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. 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?

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
Rubén Justo· Jun 5, 2024, 21:24 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Jun 5, 2024, 21:52 UTC · re: Rubén Justo · lore

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

Rubén Justo <rjusto@gmail.com> writes:
Show 8 quoted lines
> 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?

It is, but what makes it confusing is that the title does not describe what it tests, no? It tests that --no-cover-letter disables implicit cover-letter generation even with the presence of --range-diff.

In the context of t3206-range-diff.sh, we know we are talking about the "range-diff", and mention of "cover-letter" is a hint enough that the "format-patch" is involved, so perhaps titles like

    test_expect_success 'explicit --no-cover-letter defeats implied --cover-letter'
and
    test_expect_success '--cover-letter is implied for multi-patch series'
would be clear enough and convey what the tests are actually doing.
Rubén Justo· Jun 5, 2024, 21:39 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 05, 2024 at 01:44:27PM -0700, Junio C Hamano wrote:
> > +	git format-patch -v2 --range-diff=topic main..unmodified &&
> > +	test_when_finished "rm v2-000?-*" &&
At any rate, I agree with you this is confusing.

I'll address this and the other similar ones in t3206, in a preparation patch within this series.

However, I'll refrain for two or three days before sending a new iteration.

Thanks.
Rubén Justo· Jun 7, 2024, 16:29 UTC · re: Rubén Justo · lore

[PATCH v4 0/2] format-patch: assume --cover-letter for diff in multi-patch series

This iteration incorporates the changes to the tests suggested in the reviews from the previous iteration.

The main change is to split the tests proposed in the previous iteration into several separate tests; separating the functionality that checks when `--cover-letter` is implicitly assumed, from the tests that check when this implicit assumption is avoided.

The new patch in the series, [1/2], is a preparation patch that reorders the way `test_when_finished` is used in t4014, making it more reasonable and logical.

Thanks.
Rubén Justo (2):
  t4014: cleanups in a few tests
  format-patch: assume --cover-letter for diff in multi-patch series
 builtin/log.c           |  2 ++
 t/t3206-range-diff.sh   | 14 ++++++++++++++
 t/t4014-format-patch.sh | 25 ++++++++++++++++++++-----
 3 files changed, 36 insertions(+), 5 deletions(-)
Range-diff against v3:
-:  ---------- > 1:  678bae2e42 t4014: cleanups in a few tests
1:  78aeff9016 ! 2:  d1e9f8561b format-patch: assume --cover-letter for diff in multi-patch series
    @@ t/t3206-range-diff.sh: do
      	'
      done
      
    -+test_expect_success "format-patch --range-diff, implicit --cover-letter" '
    ++test_expect_success "--range-diff implies --cover-letter for multi-patch series" '
    ++	test_when_finished "rm -f v2-000?-*" &&
    ++	git format-patch -v2 --range-diff=topic main..unmodified &&
    ++	test_grep "^Range-diff against v1:$" v2-0000-cover-letter.patch
    ++'
    ++
    ++test_expect_success "explicit --no-cover-letter defeats implied --cover-letter" '
    ++	test_when_finished "rm -f v2-000?-*" &&
     +	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
    ++		-v2 --range-diff=topic main..unmodified
     +'
     +
      test_expect_success 'format-patch --range-diff as commentary' '
    @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '
      '
      
     +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 &&
    ++	test_when_finished "rm -f v23-0*.patch" &&
     +	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 'interdiff: explicit --no-cover-letter defeats implied --cover-letter' '
    ++	test_when_finished "rm -f v23-0*.patch" &&
    ++	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
    ++'
     +
      test_expect_success 'format-patch does not respect diff.noprefix' '
      	git -c diff.noprefix format-patch -1 --stdout >actual &&
-- 
2.45.2.23.gd1e9f8561b
Rubén Justo· Jun 7, 2024, 16:30 UTC · re: Rubén Justo · lore

[PATCH v4 1/2] t4014: cleanups in a few tests

Arrange things we are going to create to be removed at end, and then start creating them. That way, we will clean them up even if we fail after creating some but before the end of the command.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t4014-format-patch.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to t/t4014-format-patch.sh +5 −5
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e37a1411ee..5fb5250df4 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '
 '
 
 test_expect_success 'format-patch notes output control' '
+	test_when_finished "git notes remove HEAD" &&
 	git notes add -m "notes config message" HEAD &&
-	test_when_finished git notes remove HEAD &&
 
 	git format-patch -1 --stdout >out &&
 	! grep "notes config message" out &&
@@ -848,10 +848,10 @@ test_expect_success 'format-patch notes output control' '
 '
 
 test_expect_success 'format-patch with multiple notes refs' '
+	test_when_finished "git notes --ref note1 remove HEAD;
+			    git notes --ref note2 remove HEAD" &&
 	git notes --ref note1 add -m "this is note 1" HEAD &&
-	test_when_finished git notes --ref note1 remove HEAD &&
 	git notes --ref note2 add -m "this is note 2" HEAD &&
-	test_when_finished git notes --ref note2 remove HEAD &&
 
 	git format-patch -1 --stdout >out &&
 	! grep "this is note 1" out &&
@@ -892,10 +892,10 @@ test_expect_success 'format-patch with multiple notes refs' '
 test_expect_success 'format-patch with multiple notes refs in config' '
 	test_when_finished "test_unconfig format.notes" &&
 
+	test_when_finished "git notes --ref note1 remove HEAD;
+			    git notes --ref note2 remove HEAD" &&
 	git notes --ref note1 add -m "this is note 1" HEAD &&
-	test_when_finished git notes --ref note1 remove HEAD &&
 	git notes --ref note2 add -m "this is note 2" HEAD &&
-	test_when_finished git notes --ref note2 remove HEAD &&
 
 	git config format.notes note1 &&
 	git format-patch -1 --stdout >out &&

-- 
2.45.2.23.gd1e9f8561b
Junio C Hamano· Jun 7, 2024, 17:14 UTC · re: Rubén Justo · lore

Re: [PATCH v4 1/2] t4014: cleanups in a few tests

Rubén Justo <rjusto@gmail.com> writes:
Show 20 quoted lines
> Arrange things we are going to create to be removed at end, and then
> start creating them.  That way, we will clean them up even if we fail
> after creating some but before the end of the command.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  t/t4014-format-patch.sh | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index e37a1411ee..5fb5250df4 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '
>  '
>  
>  test_expect_success 'format-patch notes output control' '
> +	test_when_finished "git notes remove HEAD" &&
>  	git notes add -m "notes config message" HEAD &&
> -	test_when_finished git notes remove HEAD &&

If "notes add" fails to create a note for HEAD, test_when_finished would notice that it cannot remove a note from HEAD, wouldn't it? If you do

                ! grep "notes config message" out &&
                git format-patch -1 --stdout --no-notes --notes >out &&
        -	grep "notes config message" out
        +	grep "notes config message" out &&
        +	git notes remove HEAD
         '

at the end of this passing test to remove the note from HEAD (so that when-finished handler has nothing to remove), and run "sh t4014-format-patch.sh -i -v", this test piece 4014.70 fails with

	...
            notes config message
        Removing note for object HEAD
        Object HEAD has no note
        not ok 70 - format-patch notes output control

A failure in the when-finished handler is noticed (which we might argue is a misfeature), and that is why it is a good idea to write

	test_when_finished 'rm -f cruft-that-may-be-created' &&
	do what might create cruft-that-may-be-created
with "-f".
A standard trick can be found in the output of
	$ git grep 'finished.*|| *:' t/
Thanks.
Rubén Justo· Jun 7, 2024, 17:38 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 1/2] t4014: cleanups in a few tests

On Fri, Jun 07, 2024 at 10:14:10AM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Arrange things we are going to create to be removed at end, and then
> > start creating them.  That way, we will clean them up even if we fail
> > after creating some but before the end of the command.
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  t/t4014-format-patch.sh | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index e37a1411ee..5fb5250df4 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '
> >  '
> >  
> >  test_expect_success 'format-patch notes output control' '
> > +	test_when_finished "git notes remove HEAD" &&
> >  	git notes add -m "notes config message" HEAD &&
> > -	test_when_finished git notes remove HEAD &&
> 
> If "notes add" fails to create a note for HEAD, test_when_finished
> would notice that it cannot remove a note from HEAD, wouldn't it?
Yep.  Something like this, no?
Show changes to t/t4014-format-patch.sh +3 −3
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index de9e8455b3..1088c435e0 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -820,7 +820,7 @@ test_expect_success 'format-patch --notes --signoff' '
 '
 
 test_expect_success 'format-patch notes output control' '
-       test_when_finished "git notes remove HEAD" &&
+       test_when_finished "git notes remove HEAD || :" &&
        git notes add -m "notes config message" HEAD &&
 
        git format-patch -1 --stdout >out &&
@@ -849,7 +849,7 @@ test_expect_success 'format-patch notes output control' '
 
 test_expect_success 'format-patch with multiple notes refs' '
        test_when_finished "git notes --ref note1 remove HEAD;
-                           git notes --ref note2 remove HEAD" &&
+                           git notes --ref note2 remove HEAD || :" &&
        git notes --ref note1 add -m "this is note 1" HEAD &&
        git notes --ref note2 add -m "this is note 2" HEAD &&
 
@@ -893,7 +893,7 @@ test_expect_success 'format-patch with multiple notes refs in config' '
        test_when_finished "test_unconfig format.notes" &&
 
        test_when_finished "git notes --ref note1 remove HEAD;
-                           git notes --ref note2 remove HEAD" &&
+                           git notes --ref note2 remove HEAD || :" &&
        git notes --ref note1 add -m "this is note 1" HEAD &&
        git notes --ref note2 add -m "this is note 2" HEAD &&

> If you do
> 
>                 ! grep "notes config message" out &&
>                 git format-patch -1 --stdout --no-notes --notes >out &&
>         -	grep "notes config message" out
>         +	grep "notes config message" out &&
>         +	git notes remove HEAD
>          '
> 
> at the end of this passing test to remove the note from HEAD (so
> that when-finished handler has nothing to remove), and run "sh
> t4014-format-patch.sh -i -v", this test piece 4014.70 fails with
> 
> 	...
>             notes config message
>         Removing note for object HEAD
>         Object HEAD has no note
>         not ok 70 - format-patch notes output control
> 
> A failure in the when-finished handler is noticed (which we might
> argue is a misfeature)

Dropping it doesn't seem like something to be strongly opposed to :-)

> , and that is why it is a good idea to write
> 
> 	test_when_finished 'rm -f cruft-that-may-be-created' &&
> 	do what might create cruft-that-may-be-created
> 
> with "-f".
> 
> A standard trick can be found in the output of
> 
> 	$ git grep 'finished.*|| *:' t/
> 
> Thanks.

Thank you.
Junio C Hamano· Jun 7, 2024, 18:57 UTC · re: Rubén Justo · lore

Re: [PATCH v4 1/2] t4014: cleanups in a few tests

Rubén Justo <rjusto@gmail.com> writes:
>> If "notes add" fails to create a note for HEAD, test_when_finished
>> would notice that it cannot remove a note from HEAD, wouldn't it?
>
> Yep.  Something like this, no?
That's following the "grep for them" advice ;-)
>> A failure in the when-finished handler is noticed (which we might
>> argue is a misfeature)
>
> Dropping it doesn't seem like something to be strongly opposed to :-)

It does protect us from careless test writers. At least, when we see the care has been taken to make sure the "clean-up" tasks covers both cases where the main test did or failed to create the cruft to be removed, that assures us that the test writers were thinking it through.

But of course, those who blindly cut and paste the "|| :" pattern would fool such protection measure X-<.

;-)
Rubén Justo· Jun 7, 2024, 16:30 UTC · re: Rubén Justo · lore

[PATCH v4 2/2] format-patch: assume --cover-letter for diff in multi-patch series

When we deal with a multi-patch series in git-format-patch(1), if we see `--interdiff` or `--range-diff` but no `--cover-letter`, we return with an error, saying:

    fatal: --range-diff requires --cover-letter or single patch
or:
    fatal: --interdiff requires --cover-letter or single patch

This makes sense because the cover-letter is where we place the diff from the previous version.

However, considering that `format-patch` generates a multi-patch as needed, let's adopt a similar "cover as necessary" approach when using `--interdiff` or `--range-diff`.

Therefore, relax the requirement for an explicit `--cover-letter` in a multi-patch series when the user says `--iterdiff` or `--range-diff`.

Still, if only to return the error, respect "format.coverLetter=no" and `--no-cover-letter`.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/log.c           |  2 ++
 t/t3206-range-diff.sh   | 14 ++++++++++++++
 t/t4014-format-patch.sh | 15 +++++++++++++++
 3 files changed, 31 insertions(+)
Show changes to 3 files +31 −0

builtin/log.c, t/t3206-range-diff.sh, t/t4014-format-patch.sh

diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e9..d61cdbf304 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2255,6 +2255,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter == -1) {
 		if (config_cover_letter == COVER_AUTO)
 			cover_letter = (total > 1);
+		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
+			cover_letter = (config_cover_letter != COVER_OFF);
 		else
 			cover_letter = (config_cover_letter == COVER_ON);
 	}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 7b05bf3961..a767c3520e 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -545,6 +545,20 @@ do
 	'
 done
 
+test_expect_success "--range-diff implies --cover-letter for multi-patch series" '
+	test_when_finished "rm -f v2-000?-*" &&
+	git format-patch -v2 --range-diff=topic main..unmodified &&
+	test_grep "^Range-diff against v1:$" v2-0000-cover-letter.patch
+'
+
+test_expect_success "explicit --no-cover-letter defeats implied --cover-letter" '
+	test_when_finished "rm -f v2-000?-*" &&
+	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
+'
+
 test_expect_success 'format-patch --range-diff as commentary' '
 	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
 	test_when_finished "rm 0001-*" &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 5fb5250df4..de9e8455b3 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2445,6 +2445,21 @@ test_expect_success 'interdiff: solo-patch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '
+	test_when_finished "rm -f v23-0*.patch" &&
+	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 'interdiff: explicit --no-cover-letter defeats implied --cover-letter' '
+	test_when_finished "rm -f v23-0*.patch" &&
+	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
+'
+
 test_expect_success 'format-patch does not respect diff.noprefix' '
 	git -c diff.noprefix format-patch -1 --stdout >actual &&
 	grep "^--- a/blorp" actual

-- 
2.45.2.23.gd1e9f8561b
Rubén Justo· Jun 7, 2024, 20:52 UTC · re: Rubén Justo · lore

[PATCH v5 0/2] format-patch: assume --cover-letter for diff in multi-patch series

This iteration fixes some tests introduced in the previous iteration.
Rubén Justo (2):
  t4014: cleanups in a few tests
  format-patch: assume --cover-letter for diff in multi-patch series
 builtin/log.c           |  2 ++
 t/t3206-range-diff.sh   | 14 ++++++++++++++
 t/t4014-format-patch.sh | 25 ++++++++++++++++++++-----
 3 files changed, 36 insertions(+), 5 deletions(-)

And here is the result of a `--range-diff` from the previous iteration, that implicitly produced the current cover-letter :-)

Range-diff against v4:
1:  678bae2e42 ! 1:  1dbfce39d9 t4014: cleanups in a few tests
    @@ t/t4014-format-patch.sh: test_expect_success 'format-patch --notes --signoff' '
      '
      
      test_expect_success 'format-patch notes output control' '
    -+	test_when_finished "git notes remove HEAD" &&
    ++	test_when_finished "git notes remove HEAD || :" &&
      	git notes add -m "notes config message" HEAD &&
     -	test_when_finished git notes remove HEAD &&
      
    @@ t/t4014-format-patch.sh: test_expect_success 'format-patch notes output control'
      
      test_expect_success 'format-patch with multiple notes refs' '
     +	test_when_finished "git notes --ref note1 remove HEAD;
    -+			    git notes --ref note2 remove HEAD" &&
    ++			    git notes --ref note2 remove HEAD || :" &&
      	git notes --ref note1 add -m "this is note 1" HEAD &&
     -	test_when_finished git notes --ref note1 remove HEAD &&
      	git notes --ref note2 add -m "this is note 2" HEAD &&
    @@ t/t4014-format-patch.sh: test_expect_success 'format-patch with multiple notes r
      	test_when_finished "test_unconfig format.notes" &&
      
     +	test_when_finished "git notes --ref note1 remove HEAD;
    -+			    git notes --ref note2 remove HEAD" &&
    ++			    git notes --ref note2 remove HEAD || :" &&
      	git notes --ref note1 add -m "this is note 1" HEAD &&
     -	test_when_finished git notes --ref note1 remove HEAD &&
      	git notes --ref note2 add -m "this is note 2" HEAD &&
2:  7d3afe14a7 = 2:  fa22af3ed5 format-patch: assume --cover-letter for diff in multi-patch series
-- 
2.45.2.23.gd1e9f8561b
Rubén Justo· Jun 7, 2024, 20:55 UTC · re: Rubén Justo · lore

[PATCH v5 1/2] t4014: cleanups in a few tests

Arrange things we are going to create to be removed at end, and then start creating them. That way, we will clean them up even if we fail after creating some but before the end of the command.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t4014-format-patch.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to t/t4014-format-patch.sh +5 −5
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index e37a1411ee..a252c8fbf1 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '
 '
 
 test_expect_success 'format-patch notes output control' '
+	test_when_finished "git notes remove HEAD || :" &&
 	git notes add -m "notes config message" HEAD &&
-	test_when_finished git notes remove HEAD &&
 
 	git format-patch -1 --stdout >out &&
 	! grep "notes config message" out &&
@@ -848,10 +848,10 @@ test_expect_success 'format-patch notes output control' '
 '
 
 test_expect_success 'format-patch with multiple notes refs' '
+	test_when_finished "git notes --ref note1 remove HEAD;
+			    git notes --ref note2 remove HEAD || :" &&
 	git notes --ref note1 add -m "this is note 1" HEAD &&
-	test_when_finished git notes --ref note1 remove HEAD &&
 	git notes --ref note2 add -m "this is note 2" HEAD &&
-	test_when_finished git notes --ref note2 remove HEAD &&
 
 	git format-patch -1 --stdout >out &&
 	! grep "this is note 1" out &&
@@ -892,10 +892,10 @@ test_expect_success 'format-patch with multiple notes refs' '
 test_expect_success 'format-patch with multiple notes refs in config' '
 	test_when_finished "test_unconfig format.notes" &&
 
+	test_when_finished "git notes --ref note1 remove HEAD;
+			    git notes --ref note2 remove HEAD || :" &&
 	git notes --ref note1 add -m "this is note 1" HEAD &&
-	test_when_finished git notes --ref note1 remove HEAD &&
 	git notes --ref note2 add -m "this is note 2" HEAD &&
-	test_when_finished git notes --ref note2 remove HEAD &&
 
 	git config format.notes note1 &&
 	git format-patch -1 --stdout >out &&

-- 
2.45.2.23.gd1e9f8561b
Rubén Justo· Jun 7, 2024, 20:55 UTC · re: Rubén Justo · lore

[PATCH v5 2/2] format-patch: assume --cover-letter for diff in multi-patch series

When we deal with a multi-patch series in git-format-patch(1), if we see `--interdiff` or `--range-diff` but no `--cover-letter`, we return with an error, saying:

    fatal: --range-diff requires --cover-letter or single patch
or:
    fatal: --interdiff requires --cover-letter or single patch

This makes sense because the cover-letter is where we place the diff from the previous version.

However, considering that `format-patch` generates a multi-patch as needed, let's adopt a similar "cover as necessary" approach when using `--interdiff` or `--range-diff`.

Therefore, relax the requirement for an explicit `--cover-letter` in a multi-patch series when the user says `--iterdiff` or `--range-diff`.

Still, if only to return the error, respect "format.coverLetter=no" and `--no-cover-letter`.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/log.c           |  2 ++
 t/t3206-range-diff.sh   | 14 ++++++++++++++
 t/t4014-format-patch.sh | 15 +++++++++++++++
 3 files changed, 31 insertions(+)
Show changes to 3 files +31 −0

builtin/log.c, t/t3206-range-diff.sh, t/t4014-format-patch.sh

diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e9..d61cdbf304 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2255,6 +2255,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter == -1) {
 		if (config_cover_letter == COVER_AUTO)
 			cover_letter = (total > 1);
+		else if ((idiff_prev.nr || rdiff_prev) && (total > 1))
+			cover_letter = (config_cover_letter != COVER_OFF);
 		else
 			cover_letter = (config_cover_letter == COVER_ON);
 	}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 7b05bf3961..a767c3520e 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -545,6 +545,20 @@ do
 	'
 done
 
+test_expect_success "--range-diff implies --cover-letter for multi-patch series" '
+	test_when_finished "rm -f v2-000?-*" &&
+	git format-patch -v2 --range-diff=topic main..unmodified &&
+	test_grep "^Range-diff against v1:$" v2-0000-cover-letter.patch
+'
+
+test_expect_success "explicit --no-cover-letter defeats implied --cover-letter" '
+	test_when_finished "rm -f v2-000?-*" &&
+	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
+'
+
 test_expect_success 'format-patch --range-diff as commentary' '
 	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
 	test_when_finished "rm 0001-*" &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index a252c8fbf1..1088c435e0 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2445,6 +2445,21 @@ test_expect_success 'interdiff: solo-patch' '
 	test_cmp expect actual
 '
 
+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '
+	test_when_finished "rm -f v23-0*.patch" &&
+	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 'interdiff: explicit --no-cover-letter defeats implied --cover-letter' '
+	test_when_finished "rm -f v23-0*.patch" &&
+	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
+'
+
 test_expect_success 'format-patch does not respect diff.noprefix' '
 	git -c diff.noprefix format-patch -1 --stdout >actual &&
 	grep "^--- a/blorp" actual

-- 
2.45.2.23.gd1e9f8561b
Junio C Hamano· Jun 7, 2024, 21:10 UTC · re: Rubén Justo · lore

Re: [PATCH v5 0/2] format-patch: assume --cover-letter for diff in multi-patch series

Rubén Justo <rjusto@gmail.com> writes:
> This iteration fixes some tests introduced in the previous iteration.

Looking good. Let's mark it for 'next' and merge it down unless somebody finds other issues.

Thanks.

← back to recent threads