{"thread":{"id":"61580","subject":"[PATCH] format-patch: assume --cover-letter for diff in multi-patch series","startedAt":"2024-06-03T22:49:38Z","lastAt":"2024-06-07T21:10:51Z","messageCount":21,"participants":["Rubén Justo","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"496194","messageId":"6269eed5-f1ff-43f3-9249-d6a0f1852a6c@gmail.com","threadId":"61580","inReplyTo":null,"subject":"[PATCH] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-03T22:49:35Z","receivedAt":"2024-06-03T22:49:38Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"If either `--interdiff` or `--range-diff` is specified without\n`--cover-letter`, we'll abort if it would result in a multi-patch series\nbeing generated. Because the cover-letter is needed to give the diff\ntext in a multi-patch series.\n\nConsidering that `format-patch` generates a multi-patch as needed, let's\nadopt a similar \"cover as necessary\" approach when using `--interdiff`\nor `--range-diff`.\n\nSigned-off-by: Rubén Justo \n---\n builtin/log.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c8ce0c0d88..56101672f8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2286,8 +2286,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\trev.total = total + start_number - 1;\n \n \tif (idiff_prev.nr) {\n-\t\tif (!cover_letter && total != 1)\n-\t\t\tdie(_(\"--interdiff requires --cover-letter or single patch\"));\n+\t\tif (!cover_letter && total != 1) {\n+\t\t\twarning(_(\"--interdiff implies --cover-letter for multi-patch series\"));\n+\t\t\tcover_letter = 1;\n+\t\t}\n \t\trev.idiff_oid1 = &idiff_prev.oid[idiff_prev.nr - 1];\n \t\trev.idiff_oid2 = get_commit_tree_oid(list[0]);\n \t\trev.idiff_title = diff_title(&idiff_title, reroll_count,\n@@ -2301,8 +2303,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"the option '%s' requires '%s'\"), \"--creation-factor\", \"--range-diff\");\n \n \tif (rdiff_prev) {\n-\t\tif (!cover_letter && total != 1)\n-\t\t\tdie(_(\"--range-diff requires --cover-letter or single patch\"));\n+\t\tif (!cover_letter && total != 1) {\n+\t\t\twarning(_(\"--range-diff implies --cover-letter for multi-patch series\"));\n+\t\t\tcover_letter = 1;\n+\t\t}\n \n \t\tinfer_range_diff_ranges(&rdiff1, &rdiff2, rdiff_prev,\n \t\t\t\t\torigin, list[0]);\n-- \n2.45.2.405.gf8e6085128\n"},{"id":"496209","messageId":"Zl7J_Xr6Z7Ot6Hlk@framework","threadId":"61580","inReplyTo":"6269eed5-f1ff-43f3-9249-d6a0f1852a6c@gmail.com","subject":"Re: [PATCH] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-06-04T08:02:05Z","receivedAt":"2024-06-04T08:02:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 04, 2024 at 12:49:35AM +0200, Rubén Justo wrote:\n> If either `--interdiff` or `--range-diff` is specified without\n> `--cover-letter`, we'll abort if it would result in a multi-patch series\n> being generated. Because the cover-letter is needed to give the diff\n> text in a multi-patch series.\n> \n> Considering that `format-patch` generates a multi-patch as needed, let's\n> adopt a similar \"cover as necessary\" approach when using `--interdiff`\n> or `--range-diff`.\n\nWhat does git-format-patch(1) do right now in this situation?\n\nIn any case, this change should probably have a test or two to\ndemonstrate that it works as advertised.\n\nThanks!\n\nPatrick\n"},{"id":"496292","messageId":"xmqqplswwr41.fsf@gitster.g","threadId":"61580","inReplyTo":"Zl7J_Xr6Z7Ot6Hlk@framework","subject":"Re: [PATCH] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-04T17:32:46Z","receivedAt":"2024-06-04T17:32:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Jun 04, 2024 at 12:49:35AM +0200, Rubén Justo wrote:\n>> If either `--interdiff` or `--range-diff` is specified without\n>> `--cover-letter`, we'll abort if it would result in a multi-patch series\n>> being generated. Because the cover-letter is needed to give the diff\n>> text in a multi-patch series.\n>> \n>> Considering that `format-patch` generates a multi-patch as needed, let's\n>> adopt a similar \"cover as necessary\" approach when using `--interdiff`\n>> or `--range-diff`.\n>\n> What does git-format-patch(1) do right now in this situation?\n>\n> In any case, this change should probably have a test or two to\n> demonstrate that it works as advertised.\n\nYes. I think the existing tests for giving --interdiff to a single\npatch series serves as the \"it does not trigger when it shouldn't\"\nside of the test, so a positive \"it does what it claims to do\" test\nshould be sufficient.\n\nThanks.\n"},{"id":"496389","messageId":"14365d68-ed04-44fe-823b-a3959626684e@gmail.com","threadId":"61580","inReplyTo":"6269eed5-f1ff-43f3-9249-d6a0f1852a6c@gmail.com","subject":"Re: [PATCH] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-05T18:01:21Z","receivedAt":"2024-06-05T18:01:24Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we deal with a multi-patch series in git-format-patch(1), if we see\n`--interdiff` or `--range-diff` but no `--cover-letter`, we return with\nan error, saying:\n\n fatal: --range-diff requires --cover-letter or single patch\n\nor:\n\n fatal: --interdiff requires --cover-letter or single patch\n\nThis makes sense because the cover-letter is where we place the diff\nfrom the previous version.\n\nHowever, considering that `format-patch` generates a multi-patch as\nneeded, let's adopt a similar \"cover as necessary\" approach when using\n`--interdiff` or `--range-diff`.\n\nTherefore, relax the requirement for an explicit `--cover-letter` in a\nmulti-patch series when the user says `--iterdiff` or `--range-diff`.\n\nStill, if only to return the error, respect \"format.coverLetter=no\" and\n`--no-cover-letter`.\n\nSigned-off-by: Rubén Justo \n---\n\nThis is a hopefully more curated version that better explains the\ncurrent situation and adds a couple of tests.\n\nThanks!\n\n\n builtin/log.c | 2 ++\n t/t3206-range-diff.sh | 6 ++++++\n t/t4014-format-patch.sh | 6 ++++++\n 3 files changed, 14 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c8ce0c0d88..8032909d4f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter == -1) {\n \t\tif (config_cover_letter == COVER_AUTO)\n \t\t\tcover_letter = (total > 1);\n+\t\telse if ((idiff_prev.nr || rdiff_prev) && (total > 1))\n+\t\t\tcover_letter = (config_cover_letter != COVER_OFF);\n \t\telse\n \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n \t}\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 7b05bf3961..5af155805d 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -545,6 +545,12 @@ do\n \t'\n done\n \n+test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n+\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n+\ttest_when_finished \"rm v2-000?-*\" &&\n+\ttest_grep \"^Range-diff against v1:$\" v2-0000-*\n+'\n+\n test_expect_success 'format-patch --range-diff as commentary' '\n \tgit format-patch --range-diff=HEAD~1 HEAD~1 >actual &&\n \ttest_when_finished \"rm 0001-*\" &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba85b582c5..c844fbfe47 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2492,6 +2492,12 @@ test_expect_success 'interdiff: solo-patch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n+\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n+\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'format-patch does not respect diff.noprefix' '\n \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n \tgrep \"^--- a/blorp\" actual\n-- \n2.45.2.410.g52d620e86a\n\n"},{"id":"496391","messageId":"xmqqbk4fs185.fsf@gitster.g","threadId":"61580","inReplyTo":"14365d68-ed04-44fe-823b-a3959626684e@gmail.com","subject":"Re: [PATCH] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T18:17:46Z","receivedAt":"2024-06-05T18:17:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> diff --git a/builtin/log.c b/builtin/log.c\n> index c8ce0c0d88..8032909d4f 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \tif (cover_letter == -1) {\n> \t\tif (config_cover_letter == COVER_AUTO)\n> \t\t\tcover_letter = (total > 1);\n> +\t\telse if ((idiff_prev.nr || rdiff_prev) && (total > 1))\n> +\t\t\tcover_letter = (config_cover_letter != COVER_OFF);\n> \t\telse\n> \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n> \t}\n\nInteresting. So those who really really hate cover letters can set\nthe configuration explicitly to 'off' and giving an --interdiff\noption would still have the sanity check kick in. Makes sense.\n\n> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\n> index 7b05bf3961..5af155805d 100755\n> --- a/t/t3206-range-diff.sh\n> +++ b/t/t3206-range-diff.sh\n> @@ -545,6 +545,12 @@ do\n> \t'\n> done\n> \n> +test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n> +\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n> +\ttest_when_finished \"rm v2-000?-*\" &&\n\nI was about to make the follwoing:\n\n Swap these two. Arrange things we are going to create to be\n removed at end, and then start creating them. That way, we will\n clean them up even if we fail after creating some but before the\n end of the command.\n\nbut this one is littered with exiting breakage, so I'll let it pass.\nSomebody will later go in and fix them all (#leftoverbits).\n\n> +\ttest_grep \"^Range-diff against v1:$\" v2-0000-*\n> +'\n\nIsn't the name of the cover letter file always cover-letter.patch\nunless you configure a custom --suffix (which is not the case here)?\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index ba85b582c5..c844fbfe47 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -2492,6 +2492,12 @@ test_expect_success 'interdiff: solo-patch' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n> +\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n> +\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n> +\ttest_cmp expect actual\n> +'\n\nOK.\n\n> test_expect_success 'format-patch does not respect diff.noprefix' '\n> \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n> \tgrep \"^--- a/blorp\" actual\n\nLooking good.\n"},{"id":"496394","messageId":"xmqqy17jqkre.fsf@gitster.g","threadId":"61580","inReplyTo":"xmqqbk4fs185.fsf@gitster.g","subject":"Re: [PATCH] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T18:58:45Z","receivedAt":"2024-06-05T18:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Rubén Justo writes:\n>\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index c8ce0c0d88..8032909d4f 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n>> @@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n>> \tif (cover_letter == -1) {\n>> \t\tif (config_cover_letter == COVER_AUTO)\n>> \t\t\tcover_letter = (total > 1);\n>> +\t\telse if ((idiff_prev.nr || rdiff_prev) && (total > 1))\n>> +\t\t\tcover_letter = (config_cover_letter != COVER_OFF);\n>> \t\telse\n>> \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n>> \t}\n>\n> Interesting. So those who really really hate cover letters can set\n> the configuration explicitly to 'off' and giving an --interdiff\n> option would still have the sanity check kick in. Makes sense.\n\nThis is not covered by the added tests, is it?\n\nWe need to test this case: the user asks for --interdiff but at the\nsame time refuses with --no-cover-letter (or its config equivalent)\nto create a cover letter.\n\nAs I said already, everything else looked OK in this patch.\n\nThanks.\n"},{"id":"496395","messageId":"cb6b6d54-959f-477d-83e5-027c81ae85de@gmail.com","threadId":"61580","inReplyTo":"14365d68-ed04-44fe-823b-a3959626684e@gmail.com","subject":"[PATCH v3] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-05T20:27:41Z","receivedAt":"2024-06-05T20:27:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we deal with a multi-patch series in git-format-patch(1), if we see\n`--interdiff` or `--range-diff` but no `--cover-letter`, we return with\nan error, saying:\n\n fatal: --range-diff requires --cover-letter or single patch\n\nor:\n\n fatal: --interdiff requires --cover-letter or single patch\n\nThis makes sense because the cover-letter is where we place the diff\nfrom the previous version.\n\nHowever, considering that `format-patch` generates a multi-patch as\nneeded, let's adopt a similar \"cover as necessary\" approach when using\n`--interdiff` or `--range-diff`.\n\nTherefore, relax the requirement for an explicit `--cover-letter` in a\nmulti-patch series when the user says `--iterdiff` or `--range-diff`.\n\nStill, if only to return the error, respect \"format.coverLetter=no\" and\n`--no-cover-letter`.\n\nSigned-off-by: Rubén Justo \n---\n\nRange-diff against v2:\n1: ff67f24022 ! 1: 8dc5f16d83 format-patch: assume --cover-letter for diff in multi-patch series\n @@ t/t3206-range-diff.sh: do\n done\n \n +test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n ++\ttest_must_fail git format-patch --no-cover-letter \\\n ++\t\t-v2 --range-diff=topic main..unmodified &&\n ++\ttest_must_fail git -c format.coverLetter=no format-patch \\\n ++\t\t-v2 --range-diff=topic main..unmodified &&\n +\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n +\ttest_when_finished \"rm v2-000?-*\" &&\n -+\ttest_grep \"^Range-diff against v1:$\" v2-0000-*\n ++\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n +'\n +\n test_expect_success 'format-patch --range-diff as commentary' '\n @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '\n '\n \n +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n ++\ttest_must_fail git format-patch --no-cover-letter \\\n ++\t\t--interdiff=boop~2 -2 -v23 &&\n ++\ttest_must_fail git -c format.coverLetter=no format-patch \\\n ++\t\t--interdiff=boop~2 -2 -v23 &&\n +\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n +\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n +\ttest_cmp expect actual\n\n builtin/log.c | 2 ++\n t/t3206-range-diff.sh | 10 ++++++++++\n t/t4014-format-patch.sh | 10 ++++++++++\n 3 files changed, 22 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c8ce0c0d88..8032909d4f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2277,6 +2277,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter == -1) {\n \t\tif (config_cover_letter == COVER_AUTO)\n \t\t\tcover_letter = (total > 1);\n+\t\telse if ((idiff_prev.nr || rdiff_prev) && (total > 1))\n+\t\t\tcover_letter = (config_cover_letter != COVER_OFF);\n \t\telse\n \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n \t}\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 7b05bf3961..4a597466a2 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -545,6 +545,16 @@ do\n \t'\n done\n \n+test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n+\ttest_must_fail git format-patch --no-cover-letter \\\n+\t\t-v2 --range-diff=topic main..unmodified &&\n+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n+\t\t-v2 --range-diff=topic main..unmodified &&\n+\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n+\ttest_when_finished \"rm v2-000?-*\" &&\n+\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n+'\n+\n test_expect_success 'format-patch --range-diff as commentary' '\n \tgit format-patch --range-diff=HEAD~1 HEAD~1 >actual &&\n \ttest_when_finished \"rm 0001-*\" &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba85b582c5..b96348eebd 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2492,6 +2492,16 @@ test_expect_success 'interdiff: solo-patch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n+\ttest_must_fail git format-patch --no-cover-letter \\\n+\t\t--interdiff=boop~2 -2 -v23 &&\n+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n+\t\t--interdiff=boop~2 -2 -v23 &&\n+\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n+\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'format-patch does not respect diff.noprefix' '\n \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n \tgrep \"^--- a/blorp\" actual\n-- \n2.45.2.410.g52d620e86a\n"},{"id":"496396","messageId":"xmqqr0dbqfv8.fsf@gitster.g","threadId":"61580","inReplyTo":"cb6b6d54-959f-477d-83e5-027c81ae85de@gmail.com","subject":"Re: [PATCH v3] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T20:44:27Z","receivedAt":"2024-06-05T20:44:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> +test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n> +\ttest_must_fail git format-patch --no-cover-letter \\\n> +\t\t-v2 --range-diff=topic main..unmodified &&\n> +\ttest_must_fail git -c format.coverLetter=no format-patch \\\n> +\t\t-v2 --range-diff=topic main..unmodified &&\n> +\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n> +\ttest_when_finished \"rm v2-000?-*\" &&\n> +\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n> +'\n\nIsn't this doing three separate things in a single test? Unless it\nis the local convention in this script, let's split them to three.\nIf \"--no-cover-letter\" fails to prevent v2-* files from getting\ncreated, it would fail without hitting test_when_finished. v2 was\nalready bad enough in that regard, but piling two more things that\ncould fail on top is making it even worse, no?\n\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index ba85b582c5..b96348eebd 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -2492,6 +2492,16 @@ test_expect_success 'interdiff: solo-patch' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n> +\ttest_must_fail git format-patch --no-cover-letter \\\n> +\t\t--interdiff=boop~2 -2 -v23 &&\n> +\ttest_must_fail git -c format.coverLetter=no format-patch \\\n> +\t\t--interdiff=boop~2 -2 -v23 &&\n> +\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n> +\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'format-patch does not respect diff.noprefix' '\n> \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n> \tgrep \"^--- a/blorp\" actual\n"},{"id":"496398","messageId":"5aebe520-e540-46b4-a887-af488fe2663a@gmail.com","threadId":"61580","inReplyTo":"xmqqr0dbqfv8.fsf@gitster.g","subject":"Re: [PATCH v3] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-05T21:24:53Z","receivedAt":"2024-06-05T21:24:56Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jun 05, 2024 at 01:44:27PM -0700, Junio C Hamano wrote:\n\n> > +test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n> > +\ttest_must_fail git format-patch --no-cover-letter \\\n> > +\t\t-v2 --range-diff=topic main..unmodified &&\n> > +\ttest_must_fail git -c format.coverLetter=no format-patch \\\n> > +\t\t-v2 --range-diff=topic main..unmodified &&\n> > +\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n> > +\ttest_when_finished \"rm v2-000?-*\" &&\n> > +\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n> > +'\n> \n> Isn't this doing three separate things in a single test? Unless it\n> is the local convention in this script, let's split them to three.\n\nHonestly, I don't have a strong opinion on this. I know, though, there\nare others who like to pack as much as possible into one test.\n\nI see your point. However, I can also accept that testing in the same\ntest the simple exceptions for the implicit --cover-letter with\n--range-diff, or --interdiff in the other one below, makes sense.\n\n> If \"--no-cover-letter\" fails to prevent v2-* files from getting\n> created, it would fail without hitting test_when_finished. v2 was\n> already bad enough in that regard, but piling two more things that\n> could fail on top is making it even worse, no?\n\nI'm curious, a test like: \n\ntest_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n\ttest_when_finished \"rm v2-000?-*\" &&\n\ttest_must_fail git format-patch --no-cover-letter\n\t\t-v2 --range-diff=topic main..unmodified\n\nisn't it confusing?\n\nThanks.\n\n> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> > index ba85b582c5..b96348eebd 100755\n> > --- a/t/t4014-format-patch.sh\n> > +++ b/t/t4014-format-patch.sh\n> > @@ -2492,6 +2492,16 @@ test_expect_success 'interdiff: solo-patch' '\n> > \ttest_cmp expect actual\n> > '\n> > \n> > +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n> > +\ttest_must_fail git format-patch --no-cover-letter \\\n> > +\t\t--interdiff=boop~2 -2 -v23 &&\n> > +\ttest_must_fail git -c format.coverLetter=no format-patch \\\n> > +\t\t--interdiff=boop~2 -2 -v23 &&\n> > +\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n> > +\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n> > +\ttest_cmp expect actual\n> > +'\n> > +\n> > test_expect_success 'format-patch does not respect diff.noprefix' '\n> > \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n> > \tgrep \"^--- a/blorp\" actual\n"},{"id":"496400","messageId":"054e2032-8d09-491a-bc33-309fb20fa9bc@gmail.com","threadId":"61580","inReplyTo":"xmqqr0dbqfv8.fsf@gitster.g","subject":"Re: [PATCH v3] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-05T21:39:28Z","receivedAt":"2024-06-05T21:39:31Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Wed, Jun 05, 2024 at 01:44:27PM -0700, Junio C Hamano wrote:\n\n> > +\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n> > +\ttest_when_finished \"rm v2-000?-*\" &&\n\nAt any rate, I agree with you this is confusing.\n\nI'll address this and the other similar ones in t3206, in a\npreparation patch within this series.\n\nHowever, I'll refrain for two or three days before sending a new\niteration.\n\nThanks.\n"},{"id":"496402","messageId":"xmqqv82noy5m.fsf@gitster.g","threadId":"61580","inReplyTo":"5aebe520-e540-46b4-a887-af488fe2663a@gmail.com","subject":"Re: [PATCH v3] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-05T21:52:21Z","receivedAt":"2024-06-05T21:52:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> I'm curious, a test like: \n>\n> test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n> \ttest_when_finished \"rm v2-000?-*\" &&\n> \ttest_must_fail git format-patch --no-cover-letter\n> \t\t-v2 --range-diff=topic main..unmodified\n>\n> isn't it confusing?\n\nIt is, but what makes it confusing is that the title does not\ndescribe what it tests, no? It tests that --no-cover-letter\ndisables implicit cover-letter generation even with the presence of\n--range-diff.\n\nIn the context of t3206-range-diff.sh, we know we are talking about\nthe \"range-diff\", and mention of \"cover-letter\" is a hint enough\nthat the \"format-patch\" is involved, so perhaps titles like\n\n test_expect_success 'explicit --no-cover-letter defeats implied --cover-letter'\n\nand\n\n test_expect_success '--cover-letter is implied for multi-patch series'\n\nwould be clear enough and convey what the tests are actually doing.\n"},{"id":"496651","messageId":"9f520828-f87e-49b1-aa4b-c00ec6bb0133@gmail.com","threadId":"61580","inReplyTo":"cb6b6d54-959f-477d-83e5-027c81ae85de@gmail.com","subject":"[PATCH v4 0/2] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T16:29:06Z","receivedAt":"2024-06-07T16:29:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This iteration incorporates the changes to the tests suggested in the\nreviews from the previous iteration.\n\nThe main change is to split the tests proposed in the previous iteration\ninto several separate tests; separating the functionality that checks\nwhen `--cover-letter` is implicitly assumed, from the tests that check\nwhen this implicit assumption is avoided.\n\nThe new patch in the series, [1/2], is a preparation patch that reorders\nthe way `test_when_finished` is used in t4014, making it more reasonable\nand logical.\n\nThanks.\n\nRubén Justo (2):\n t4014: cleanups in a few tests\n format-patch: assume --cover-letter for diff in multi-patch series\n\n builtin/log.c | 2 ++\n t/t3206-range-diff.sh | 14 ++++++++++++++\n t/t4014-format-patch.sh | 25 ++++++++++++++++++++-----\n 3 files changed, 36 insertions(+), 5 deletions(-)\n\nRange-diff against v3:\n-: ---------- > 1: 678bae2e42 t4014: cleanups in a few tests\n1: 78aeff9016 ! 2: d1e9f8561b format-patch: assume --cover-letter for diff in multi-patch series\n @@ t/t3206-range-diff.sh: do\n \t'\n done\n \n -+test_expect_success \"format-patch --range-diff, implicit --cover-letter\" '\n ++test_expect_success \"--range-diff implies --cover-letter for multi-patch series\" '\n ++\ttest_when_finished \"rm -f v2-000?-*\" &&\n ++\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n ++\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n ++'\n ++\n ++test_expect_success \"explicit --no-cover-letter defeats implied --cover-letter\" '\n ++\ttest_when_finished \"rm -f v2-000?-*\" &&\n +\ttest_must_fail git format-patch --no-cover-letter \\\n +\t\t-v2 --range-diff=topic main..unmodified &&\n +\ttest_must_fail git -c format.coverLetter=no format-patch \\\n -+\t\t-v2 --range-diff=topic main..unmodified &&\n -+\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n -+\ttest_when_finished \"rm v2-000?-*\" &&\n -+\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n ++\t\t-v2 --range-diff=topic main..unmodified\n +'\n +\n test_expect_success 'format-patch --range-diff as commentary' '\n @@ t/t4014-format-patch.sh: test_expect_success 'interdiff: solo-patch' '\n '\n \n +test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n -+\ttest_must_fail git format-patch --no-cover-letter \\\n -+\t\t--interdiff=boop~2 -2 -v23 &&\n -+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n -+\t\t--interdiff=boop~2 -2 -v23 &&\n ++\ttest_when_finished \"rm -f v23-0*.patch\" &&\n +\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n +\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'interdiff: explicit --no-cover-letter defeats implied --cover-letter' '\n ++\ttest_when_finished \"rm -f v23-0*.patch\" &&\n ++\ttest_must_fail git format-patch --no-cover-letter \\\n ++\t\t--interdiff=boop~2 -2 -v23 &&\n ++\ttest_must_fail git -c format.coverLetter=no format-patch \\\n ++\t\t--interdiff=boop~2 -2 -v23\n ++'\n +\n test_expect_success 'format-patch does not respect diff.noprefix' '\n \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n-- \n2.45.2.23.gd1e9f8561b\n"},{"id":"496652","messageId":"20b95372-12cf-49bd-b1b7-dc069e7c86dd@gmail.com","threadId":"61580","inReplyTo":"9f520828-f87e-49b1-aa4b-c00ec6bb0133@gmail.com","subject":"[PATCH v4 1/2] t4014: cleanups in a few tests","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T16:30:17Z","receivedAt":"2024-06-07T16:30:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Arrange things we are going to create to be removed at end, and then\nstart creating them. That way, we will clean them up even if we fail\nafter creating some but before the end of the command.\n\nSigned-off-by: Rubén Justo \n---\n t/t4014-format-patch.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex e37a1411ee..5fb5250df4 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '\n '\n \n test_expect_success 'format-patch notes output control' '\n+\ttest_when_finished \"git notes remove HEAD\" &&\n \tgit notes add -m \"notes config message\" HEAD &&\n-\ttest_when_finished git notes remove HEAD &&\n \n \tgit format-patch -1 --stdout >out &&\n \t! grep \"notes config message\" out &&\n@@ -848,10 +848,10 @@ test_expect_success 'format-patch notes output control' '\n '\n \n test_expect_success 'format-patch with multiple notes refs' '\n+\ttest_when_finished \"git notes --ref note1 remove HEAD;\n+\t\t\t git notes --ref note2 remove HEAD\" &&\n \tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n-\ttest_when_finished git notes --ref note1 remove HEAD &&\n \tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n-\ttest_when_finished git notes --ref note2 remove HEAD &&\n \n \tgit format-patch -1 --stdout >out &&\n \t! grep \"this is note 1\" out &&\n@@ -892,10 +892,10 @@ test_expect_success 'format-patch with multiple notes refs' '\n test_expect_success 'format-patch with multiple notes refs in config' '\n \ttest_when_finished \"test_unconfig format.notes\" &&\n \n+\ttest_when_finished \"git notes --ref note1 remove HEAD;\n+\t\t\t git notes --ref note2 remove HEAD\" &&\n \tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n-\ttest_when_finished git notes --ref note1 remove HEAD &&\n \tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n-\ttest_when_finished git notes --ref note2 remove HEAD &&\n \n \tgit config format.notes note1 &&\n \tgit format-patch -1 --stdout >out &&\n-- \n2.45.2.23.gd1e9f8561b\n"},{"id":"496653","messageId":"bbe775aa-1dae-4919-acdf-27051ea74076@gmail.com","threadId":"61580","inReplyTo":"9f520828-f87e-49b1-aa4b-c00ec6bb0133@gmail.com","subject":"[PATCH v4 2/2] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T16:30:32Z","receivedAt":"2024-06-07T16:30:34Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we deal with a multi-patch series in git-format-patch(1), if we see\n`--interdiff` or `--range-diff` but no `--cover-letter`, we return with\nan error, saying:\n\n fatal: --range-diff requires --cover-letter or single patch\n\nor:\n\n fatal: --interdiff requires --cover-letter or single patch\n\nThis makes sense because the cover-letter is where we place the diff\nfrom the previous version.\n\nHowever, considering that `format-patch` generates a multi-patch as\nneeded, let's adopt a similar \"cover as necessary\" approach when using\n`--interdiff` or `--range-diff`.\n\nTherefore, relax the requirement for an explicit `--cover-letter` in a\nmulti-patch series when the user says `--iterdiff` or `--range-diff`.\n\nStill, if only to return the error, respect \"format.coverLetter=no\" and\n`--no-cover-letter`.\n\nSigned-off-by: Rubén Justo \n---\n builtin/log.c | 2 ++\n t/t3206-range-diff.sh | 14 ++++++++++++++\n t/t4014-format-patch.sh | 15 +++++++++++++++\n 3 files changed, 31 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e9..d61cdbf304 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2255,6 +2255,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter == -1) {\n \t\tif (config_cover_letter == COVER_AUTO)\n \t\t\tcover_letter = (total > 1);\n+\t\telse if ((idiff_prev.nr || rdiff_prev) && (total > 1))\n+\t\t\tcover_letter = (config_cover_letter != COVER_OFF);\n \t\telse\n \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n \t}\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 7b05bf3961..a767c3520e 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -545,6 +545,20 @@ do\n \t'\n done\n \n+test_expect_success \"--range-diff implies --cover-letter for multi-patch series\" '\n+\ttest_when_finished \"rm -f v2-000?-*\" &&\n+\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n+\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n+'\n+\n+test_expect_success \"explicit --no-cover-letter defeats implied --cover-letter\" '\n+\ttest_when_finished \"rm -f v2-000?-*\" &&\n+\ttest_must_fail git format-patch --no-cover-letter \\\n+\t\t-v2 --range-diff=topic main..unmodified &&\n+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n+\t\t-v2 --range-diff=topic main..unmodified\n+'\n+\n test_expect_success 'format-patch --range-diff as commentary' '\n \tgit format-patch --range-diff=HEAD~1 HEAD~1 >actual &&\n \ttest_when_finished \"rm 0001-*\" &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 5fb5250df4..de9e8455b3 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2445,6 +2445,21 @@ test_expect_success 'interdiff: solo-patch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n+\ttest_when_finished \"rm -f v23-0*.patch\" &&\n+\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n+\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'interdiff: explicit --no-cover-letter defeats implied --cover-letter' '\n+\ttest_when_finished \"rm -f v23-0*.patch\" &&\n+\ttest_must_fail git format-patch --no-cover-letter \\\n+\t\t--interdiff=boop~2 -2 -v23 &&\n+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n+\t\t--interdiff=boop~2 -2 -v23\n+'\n+\n test_expect_success 'format-patch does not respect diff.noprefix' '\n \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n \tgrep \"^--- a/blorp\" actual\n-- \n2.45.2.23.gd1e9f8561b\n"},{"id":"496658","messageId":"xmqqed98ekv1.fsf@gitster.g","threadId":"61580","inReplyTo":"20b95372-12cf-49bd-b1b7-dc069e7c86dd@gmail.com","subject":"Re: [PATCH v4 1/2] t4014: cleanups in a few tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T17:14:10Z","receivedAt":"2024-06-07T17:14:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Arrange things we are going to create to be removed at end, and then\n> start creating them. That way, we will clean them up even if we fail\n> after creating some but before the end of the command.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> t/t4014-format-patch.sh | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index e37a1411ee..5fb5250df4 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '\n> '\n> \n> test_expect_success 'format-patch notes output control' '\n> +\ttest_when_finished \"git notes remove HEAD\" &&\n> \tgit notes add -m \"notes config message\" HEAD &&\n> -\ttest_when_finished git notes remove HEAD &&\n\nIf \"notes add\" fails to create a note for HEAD, test_when_finished\nwould notice that it cannot remove a note from HEAD, wouldn't it?\nIf you do\n\n ! grep \"notes config message\" out &&\n git format-patch -1 --stdout --no-notes --notes >out &&\n -\tgrep \"notes config message\" out\n +\tgrep \"notes config message\" out &&\n +\tgit notes remove HEAD\n '\n\nat the end of this passing test to remove the note from HEAD (so\nthat when-finished handler has nothing to remove), and run \"sh\nt4014-format-patch.sh -i -v\", this test piece 4014.70 fails with\n\n\t...\n notes config message\n Removing note for object HEAD\n Object HEAD has no note\n not ok 70 - format-patch notes output control\n\nA failure in the when-finished handler is noticed (which we might\nargue is a misfeature), and that is why it is a good idea to write\n\n\ttest_when_finished 'rm -f cruft-that-may-be-created' &&\n\tdo what might create cruft-that-may-be-created\n\nwith \"-f\".\n\nA standard trick can be found in the output of\n\n\t$ git grep 'finished.*|| *:' t/\n\nThanks.\n"},{"id":"496660","messageId":"de9d8f38-6e4c-43d4-acc4-a38e860787a7@gmail.com","threadId":"61580","inReplyTo":"xmqqed98ekv1.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] t4014: cleanups in a few tests","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T17:38:25Z","receivedAt":"2024-06-07T17:38:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Jun 07, 2024 at 10:14:10AM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > Arrange things we are going to create to be removed at end, and then\n> > start creating them. That way, we will clean them up even if we fail\n> > after creating some but before the end of the command.\n> >\n> > Signed-off-by: Rubén Justo \n> > ---\n> > t/t4014-format-patch.sh | 10 +++++-----\n> > 1 file changed, 5 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> > index e37a1411ee..5fb5250df4 100755\n> > --- a/t/t4014-format-patch.sh\n> > +++ b/t/t4014-format-patch.sh\n> > @@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '\n> > '\n> > \n> > test_expect_success 'format-patch notes output control' '\n> > +\ttest_when_finished \"git notes remove HEAD\" &&\n> > \tgit notes add -m \"notes config message\" HEAD &&\n> > -\ttest_when_finished git notes remove HEAD &&\n> \n> If \"notes add\" fails to create a note for HEAD, test_when_finished\n> would notice that it cannot remove a note from HEAD, wouldn't it?\n\nYep. Something like this, no?\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex de9e8455b3..1088c435e0 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -820,7 +820,7 @@ test_expect_success 'format-patch --notes --signoff' '\n '\n \n test_expect_success 'format-patch notes output control' '\n- test_when_finished \"git notes remove HEAD\" &&\n+ test_when_finished \"git notes remove HEAD || :\" &&\n git notes add -m \"notes config message\" HEAD &&\n \n git format-patch -1 --stdout >out &&\n@@ -849,7 +849,7 @@ test_expect_success 'format-patch notes output control' '\n \n test_expect_success 'format-patch with multiple notes refs' '\n test_when_finished \"git notes --ref note1 remove HEAD;\n- git notes --ref note2 remove HEAD\" &&\n+ git notes --ref note2 remove HEAD || :\" &&\n git notes --ref note1 add -m \"this is note 1\" HEAD &&\n git notes --ref note2 add -m \"this is note 2\" HEAD &&\n \n@@ -893,7 +893,7 @@ test_expect_success 'format-patch with multiple notes refs in config' '\n test_when_finished \"test_unconfig format.notes\" &&\n \n test_when_finished \"git notes --ref note1 remove HEAD;\n- git notes --ref note2 remove HEAD\" &&\n+ git notes --ref note2 remove HEAD || :\" &&\n git notes --ref note1 add -m \"this is note 1\" HEAD &&\n git notes --ref note2 add -m \"this is note 2\" HEAD &&\n\n> If you do\n> \n> ! grep \"notes config message\" out &&\n> git format-patch -1 --stdout --no-notes --notes >out &&\n> -\tgrep \"notes config message\" out\n> +\tgrep \"notes config message\" out &&\n> +\tgit notes remove HEAD\n> '\n> \n> at the end of this passing test to remove the note from HEAD (so\n> that when-finished handler has nothing to remove), and run \"sh\n> t4014-format-patch.sh -i -v\", this test piece 4014.70 fails with\n> \n> \t...\n> notes config message\n> Removing note for object HEAD\n> Object HEAD has no note\n> not ok 70 - format-patch notes output control\n> \n> A failure in the when-finished handler is noticed (which we might\n> argue is a misfeature)\n\nDropping it doesn't seem like something to be strongly opposed to :-)\n\n> , and that is why it is a good idea to write\n> \n> \ttest_when_finished 'rm -f cruft-that-may-be-created' &&\n> \tdo what might create cruft-that-may-be-created\n> \n> with \"-f\".\n> \n> A standard trick can be found in the output of\n> \n> \t$ git grep 'finished.*|| *:' t/\n> \n> Thanks.\n\nThank you.\n"},{"id":"496667","messageId":"xmqqa5jwd1i1.fsf@gitster.g","threadId":"61580","inReplyTo":"de9d8f38-6e4c-43d4-acc4-a38e860787a7@gmail.com","subject":"Re: [PATCH v4 1/2] t4014: cleanups in a few tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T18:57:42Z","receivedAt":"2024-06-07T18:57:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> If \"notes add\" fails to create a note for HEAD, test_when_finished\n>> would notice that it cannot remove a note from HEAD, wouldn't it?\n>\n> Yep. Something like this, no?\n\nThat's following the \"grep for them\" advice ;-)\n\n>> A failure in the when-finished handler is noticed (which we might\n>> argue is a misfeature)\n>\n> Dropping it doesn't seem like something to be strongly opposed to :-)\n\nIt does protect us from careless test writers. At least, when we\nsee the care has been taken to make sure the \"clean-up\" tasks covers\nboth cases where the main test did or failed to create the cruft to\nbe removed, that assures us that the test writers were thinking it\nthrough.\n\nBut of course, those who blindly cut and paste the \"|| :\" pattern\nwould fool such protection measure X-<.\n\n;-)\n"},{"id":"496673","messageId":"91014071-13f2-46d3-aae7-75c8ea036786@gmail.com","threadId":"61580","inReplyTo":"9f520828-f87e-49b1-aa4b-c00ec6bb0133@gmail.com","subject":"[PATCH v5 0/2] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T20:52:53Z","receivedAt":"2024-06-07T20:52:56Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This iteration fixes some tests introduced in the previous iteration.\n\n\nRubén Justo (2):\n t4014: cleanups in a few tests\n format-patch: assume --cover-letter for diff in multi-patch series\n\n builtin/log.c | 2 ++\n t/t3206-range-diff.sh | 14 ++++++++++++++\n t/t4014-format-patch.sh | 25 ++++++++++++++++++++-----\n 3 files changed, 36 insertions(+), 5 deletions(-)\n\nAnd here is the result of a `--range-diff` from the previous iteration,\nthat implicitly produced the current cover-letter :-)\n\nRange-diff against v4:\n1: 678bae2e42 ! 1: 1dbfce39d9 t4014: cleanups in a few tests\n @@ t/t4014-format-patch.sh: test_expect_success 'format-patch --notes --signoff' '\n '\n \n test_expect_success 'format-patch notes output control' '\n -+\ttest_when_finished \"git notes remove HEAD\" &&\n ++\ttest_when_finished \"git notes remove HEAD || :\" &&\n \tgit notes add -m \"notes config message\" HEAD &&\n -\ttest_when_finished git notes remove HEAD &&\n \n @@ t/t4014-format-patch.sh: test_expect_success 'format-patch notes output control'\n \n test_expect_success 'format-patch with multiple notes refs' '\n +\ttest_when_finished \"git notes --ref note1 remove HEAD;\n -+\t\t\t git notes --ref note2 remove HEAD\" &&\n ++\t\t\t git notes --ref note2 remove HEAD || :\" &&\n \tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n -\ttest_when_finished git notes --ref note1 remove HEAD &&\n \tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n @@ t/t4014-format-patch.sh: test_expect_success 'format-patch with multiple notes r\n \ttest_when_finished \"test_unconfig format.notes\" &&\n \n +\ttest_when_finished \"git notes --ref note1 remove HEAD;\n -+\t\t\t git notes --ref note2 remove HEAD\" &&\n ++\t\t\t git notes --ref note2 remove HEAD || :\" &&\n \tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n -\ttest_when_finished git notes --ref note1 remove HEAD &&\n \tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n2: 7d3afe14a7 = 2: fa22af3ed5 format-patch: assume --cover-letter for diff in multi-patch series\n-- \n2.45.2.23.gd1e9f8561b\n"},{"id":"496674","messageId":"07323810-69b0-4171-b775-77a97d29cc35@gmail.com","threadId":"61580","inReplyTo":"91014071-13f2-46d3-aae7-75c8ea036786@gmail.com","subject":"[PATCH v5 1/2] t4014: cleanups in a few tests","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T20:55:10Z","receivedAt":"2024-06-07T20:55:13Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Arrange things we are going to create to be removed at end, and then\nstart creating them. That way, we will clean them up even if we fail\nafter creating some but before the end of the command.\n\nSigned-off-by: Rubén Justo \n---\n t/t4014-format-patch.sh | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex e37a1411ee..a252c8fbf1 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '\n '\n \n test_expect_success 'format-patch notes output control' '\n+\ttest_when_finished \"git notes remove HEAD || :\" &&\n \tgit notes add -m \"notes config message\" HEAD &&\n-\ttest_when_finished git notes remove HEAD &&\n \n \tgit format-patch -1 --stdout >out &&\n \t! grep \"notes config message\" out &&\n@@ -848,10 +848,10 @@ test_expect_success 'format-patch notes output control' '\n '\n \n test_expect_success 'format-patch with multiple notes refs' '\n+\ttest_when_finished \"git notes --ref note1 remove HEAD;\n+\t\t\t git notes --ref note2 remove HEAD || :\" &&\n \tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n-\ttest_when_finished git notes --ref note1 remove HEAD &&\n \tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n-\ttest_when_finished git notes --ref note2 remove HEAD &&\n \n \tgit format-patch -1 --stdout >out &&\n \t! grep \"this is note 1\" out &&\n@@ -892,10 +892,10 @@ test_expect_success 'format-patch with multiple notes refs' '\n test_expect_success 'format-patch with multiple notes refs in config' '\n \ttest_when_finished \"test_unconfig format.notes\" &&\n \n+\ttest_when_finished \"git notes --ref note1 remove HEAD;\n+\t\t\t git notes --ref note2 remove HEAD || :\" &&\n \tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n-\ttest_when_finished git notes --ref note1 remove HEAD &&\n \tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n-\ttest_when_finished git notes --ref note2 remove HEAD &&\n \n \tgit config format.notes note1 &&\n \tgit format-patch -1 --stdout >out &&\n-- \n2.45.2.23.gd1e9f8561b\n"},{"id":"496675","messageId":"23bac6b9-1bb1-43fa-be49-c77711b0ece8@gmail.com","threadId":"61580","inReplyTo":"91014071-13f2-46d3-aae7-75c8ea036786@gmail.com","subject":"[PATCH v5 2/2] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-06-07T20:55:21Z","receivedAt":"2024-06-07T20:55:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we deal with a multi-patch series in git-format-patch(1), if we see\n`--interdiff` or `--range-diff` but no `--cover-letter`, we return with\nan error, saying:\n\n fatal: --range-diff requires --cover-letter or single patch\n\nor:\n\n fatal: --interdiff requires --cover-letter or single patch\n\nThis makes sense because the cover-letter is where we place the diff\nfrom the previous version.\n\nHowever, considering that `format-patch` generates a multi-patch as\nneeded, let's adopt a similar \"cover as necessary\" approach when using\n`--interdiff` or `--range-diff`.\n\nTherefore, relax the requirement for an explicit `--cover-letter` in a\nmulti-patch series when the user says `--iterdiff` or `--range-diff`.\n\nStill, if only to return the error, respect \"format.coverLetter=no\" and\n`--no-cover-letter`.\n\nSigned-off-by: Rubén Justo \n---\n builtin/log.c | 2 ++\n t/t3206-range-diff.sh | 14 ++++++++++++++\n t/t4014-format-patch.sh | 15 +++++++++++++++\n 3 files changed, 31 insertions(+)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex c0a8bb95e9..d61cdbf304 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -2255,6 +2255,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tif (cover_letter == -1) {\n \t\tif (config_cover_letter == COVER_AUTO)\n \t\t\tcover_letter = (total > 1);\n+\t\telse if ((idiff_prev.nr || rdiff_prev) && (total > 1))\n+\t\t\tcover_letter = (config_cover_letter != COVER_OFF);\n \t\telse\n \t\t\tcover_letter = (config_cover_letter == COVER_ON);\n \t}\ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex 7b05bf3961..a767c3520e 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -545,6 +545,20 @@ do\n \t'\n done\n \n+test_expect_success \"--range-diff implies --cover-letter for multi-patch series\" '\n+\ttest_when_finished \"rm -f v2-000?-*\" &&\n+\tgit format-patch -v2 --range-diff=topic main..unmodified &&\n+\ttest_grep \"^Range-diff against v1:$\" v2-0000-cover-letter.patch\n+'\n+\n+test_expect_success \"explicit --no-cover-letter defeats implied --cover-letter\" '\n+\ttest_when_finished \"rm -f v2-000?-*\" &&\n+\ttest_must_fail git format-patch --no-cover-letter \\\n+\t\t-v2 --range-diff=topic main..unmodified &&\n+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n+\t\t-v2 --range-diff=topic main..unmodified\n+'\n+\n test_expect_success 'format-patch --range-diff as commentary' '\n \tgit format-patch --range-diff=HEAD~1 HEAD~1 >actual &&\n \ttest_when_finished \"rm 0001-*\" &&\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex a252c8fbf1..1088c435e0 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2445,6 +2445,21 @@ test_expect_success 'interdiff: solo-patch' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'interdiff: multi-patch, implicit --cover-letter' '\n+\ttest_when_finished \"rm -f v23-0*.patch\" &&\n+\tgit format-patch --interdiff=boop~2 -2 -v23 &&\n+\ttest_grep \"^Interdiff against v22:$\" v23-0000-cover-letter.patch &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'interdiff: explicit --no-cover-letter defeats implied --cover-letter' '\n+\ttest_when_finished \"rm -f v23-0*.patch\" &&\n+\ttest_must_fail git format-patch --no-cover-letter \\\n+\t\t--interdiff=boop~2 -2 -v23 &&\n+\ttest_must_fail git -c format.coverLetter=no format-patch \\\n+\t\t--interdiff=boop~2 -2 -v23\n+'\n+\n test_expect_success 'format-patch does not respect diff.noprefix' '\n \tgit -c diff.noprefix format-patch -1 --stdout >actual &&\n \tgrep \"^--- a/blorp\" actual\n-- \n2.45.2.23.gd1e9f8561b\n"},{"id":"496676","messageId":"xmqqsexoe9wp.fsf@gitster.g","threadId":"61580","inReplyTo":"91014071-13f2-46d3-aae7-75c8ea036786@gmail.com","subject":"Re: [PATCH v5 0/2] format-patch: assume --cover-letter for diff in multi-patch series","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-07T21:10:46Z","receivedAt":"2024-06-07T21:10:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> This iteration fixes some tests introduced in the previous iteration.\n\nLooking good. Let's mark it for 'next' and merge it down unless\nsomebody finds other issues.\n\nThanks.\n"}]}