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

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

[Rubén Justo]

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

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

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

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.

Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

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

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

[Rubén Justo]

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

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

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

[Junio C Hamano]

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.

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

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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

[Rubén Justo]

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

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

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> +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?

> 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

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

[Rubén Justo]

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

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

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

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

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

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

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

[Rubén Justo]

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.

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

[Rubén Justo]

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

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

[Rubén Justo]

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

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

[Junio C Hamano]

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

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

[Rubén Justo]

On Fri, Jun 07, 2024 at 10:14:10AM -0700, Junio C Hamano wrote:
> 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?

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.

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

[Junio C Hamano]

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

;-)

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

[Rubén Justo]

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

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

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

[Rubén Justo]

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

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

[Rubén Justo]

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

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

[Rubén Justo]

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

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

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

[Junio C Hamano]

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.