| Message ID | 6269eed5-f1ff-43f3-9249-d6a0f1852a6c@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | format-patch: assume --cover-letter for diff in multi-patch series | expand |
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
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.
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
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.
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.
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]);
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(-)