| Message ID | cb6b6d54-959f-477d-83e5-027c81ae85de@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] format-patch: assume --cover-letter for diff in multi-patch series | expand |
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
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
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 <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.
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
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(+)