{"thread":{"id":"61541","subject":"[PATCH 0/2] give range-diff at the end of single patch output","startedAt":"2024-05-23T22:50:12Z","lastAt":"2024-05-30T20:05:49Z","messageCount":18,"participants":["Junio C Hamano","Dragan Simic","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"495487","messageId":"20240523225007.2871766-1-gitster@pobox.com","threadId":"61541","inReplyTo":null,"subject":"[PATCH 0/2] give range-diff at the end of single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T22:50:05Z","receivedAt":"2024-05-23T22:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When running \"format-patch\" on a multiple patch series, the output\ncoming from \"--interdiff\" and \"--range-diff\" options is inserted\nafter the \"shortlog\" list of commits and the overall diffstat.\n\nThe idea is that shortlog/diffstat are shorter and with denser\ninformation content, which gives a better overview before the\nreaders dive into more details of range/inter diff.\n\nWhen working on a single patch, however, we stuff the inter/range\ndiff output before the actual patch, next to the diffstat.  This\npushes down the patch text way down with inter/range diff output,\ndistracting readers.\n\nMove the inter/range diff output to the very end of the output,\nafter all the patch text is shown.\n\nThe first patch is a no-op refactoring, the second patch makes the\nactual behaviour change.\n\nJunio C Hamano (2):\n  show_log: factor out interdiff/range-diff generation\n  format-patch: move range/inter diff at the end of a single patch\n    output\n\n log-tree.c              | 89 ++++++++++++++++++++++-------------------\n t/t4014-format-patch.sh | 17 +++++---\n 2 files changed, 59 insertions(+), 47 deletions(-)\n\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495488","messageId":"20240523225007.2871766-2-gitster@pobox.com","threadId":"61541","inReplyTo":"20240523225007.2871766-1-gitster@pobox.com","subject":"[PATCH 1/2] show_log: factor out interdiff/range-diff generation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T22:50:06Z","receivedAt":"2024-05-23T22:50:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The integration of \"git range-diff\" with \"git format-patch\" for a\nsingle patch (i.e., not generating \"range-diff\" into the cover\nletter) hooks into log-tree.c:show_log(), which is responsible for\nwriting the log message out and other stuff.  Essentially,\neverything you see before the diffstat and the patch is generated\nthere.\n\nSplit out the code that spits out the interdiff/range-diff into a\nseparate helper function show_diff_of_diff().  Hopefully this will\nmake it easier to move things around in the output stream in the\nfuture patches.\n\nThis is supposed to be a no-op refactoring.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n log-tree.c | 88 +++++++++++++++++++++++++++++-------------------------\n 1 file changed, 48 insertions(+), 40 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 41416de4e3..e7cd2c491f 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -673,6 +673,53 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)\n \topt->shown_dashes = 1;\n }\n \n+static void show_diff_of_diff(struct rev_info *opt)\n+{\n+\tif (!cmit_fmt_is_mail(opt->commit_format))\n+\t\treturn;\n+\n+\tif (opt->idiff_oid1) {\n+\t\tstruct diff_queue_struct dq;\n+\n+\t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n+\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\n+\t\tnext_commentary_block(opt, NULL);\n+\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->idiff_title);\n+\t\tshow_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,\n+\t\t\t       &opt->diffopt);\n+\n+\t\tmemcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));\n+\t}\n+\n+\tif (opt->rdiff1) {\n+\t\tstruct diff_queue_struct dq;\n+\t\tstruct diff_options opts;\n+\t\tstruct range_diff_options range_diff_opts = {\n+\t\t\t.creation_factor = opt->creation_factor,\n+\t\t\t.dual_color = 1,\n+\t\t\t.diffopt = &opts\n+\t\t};\n+\n+\t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n+\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n+\n+\t\tnext_commentary_block(opt, NULL);\n+\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n+\t\t/*\n+\t\t * Pass minimum required diff-options to range-diff; others\n+\t\t * can be added later if deemed desirable.\n+\t\t */\n+\t\trepo_diff_setup(the_repository, &opts);\n+\t\topts.file = opt->diffopt.file;\n+\t\topts.use_color = opt->diffopt.use_color;\n+\t\tdiff_setup_done(&opts);\n+\t\tshow_range_diff(opt->rdiff1, opt->rdiff2, &range_diff_opts);\n+\n+\t\tmemcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));\n+\t}\n+}\n+\n void show_log(struct rev_info *opt)\n {\n \tstruct strbuf msgbuf = STRBUF_INIT;\n@@ -857,46 +904,7 @@ void show_log(struct rev_info *opt)\n \tfree(ctx.notes_message);\n \tfree(ctx.after_subject);\n \n-\tif (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {\n-\t\tstruct diff_queue_struct dq;\n-\n-\t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n-\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n-\n-\t\tnext_commentary_block(opt, NULL);\n-\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->idiff_title);\n-\t\tshow_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,\n-\t\t\t       &opt->diffopt);\n-\n-\t\tmemcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));\n-\t}\n-\n-\tif (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {\n-\t\tstruct diff_queue_struct dq;\n-\t\tstruct diff_options opts;\n-\t\tstruct range_diff_options range_diff_opts = {\n-\t\t\t.creation_factor = opt->creation_factor,\n-\t\t\t.dual_color = 1,\n-\t\t\t.diffopt = &opts\n-\t\t};\n-\n-\t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n-\t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n-\n-\t\tnext_commentary_block(opt, NULL);\n-\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n-\t\t/*\n-\t\t * Pass minimum required diff-options to range-diff; others\n-\t\t * can be added later if deemed desirable.\n-\t\t */\n-\t\trepo_diff_setup(the_repository, &opts);\n-\t\topts.file = opt->diffopt.file;\n-\t\topts.use_color = opt->diffopt.use_color;\n-\t\tdiff_setup_done(&opts);\n-\t\tshow_range_diff(opt->rdiff1, opt->rdiff2, &range_diff_opts);\n-\n-\t\tmemcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));\n-\t}\n+\tshow_diff_of_diff(opt);\n }\n \n int log_tree_diff_flush(struct rev_info *opt)\n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495489","messageId":"20240523225007.2871766-3-gitster@pobox.com","threadId":"61541","inReplyTo":"20240523225007.2871766-1-gitster@pobox.com","subject":"[PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T22:50:07Z","receivedAt":"2024-05-23T22:50:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When running \"format-patch\" on a multiple patch series, the output\ncoming from \"--interdiff\" and \"--range-diff\" options is inserted\nafter the \"shortlog\" list of commits and the overall diffstat.\n\nThe idea is that shortlog/diffstat are shorter and with denser\ninformation content, which gives a better overview before the\nreaders dive into more details of range/inter diff.\n\nWhen working on a single patch, however, we stuff the inter/range\ndiff output before the actual patch, next to the diffstat.  This\npushes down the patch text way down with inter/range diff output,\ndistracting readers.\n\nMove the inter/range diff output to the very end of the output,\nafter all the patch text is shown.\n\nAs the inter/range diff is no longer part of the commentary block\n(i.e., what comes after the log message and \"---\", but before the\npatch text), stop producing \"---\" in the function that generates\nthem.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n log-tree.c              |  7 +++----\n t/t4014-format-patch.sh | 17 +++++++++++------\n 2 files changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex e7cd2c491f..f28c4d0bb0 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -684,7 +684,6 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n \t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n \n-\t\tnext_commentary_block(opt, NULL);\n \t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->idiff_title);\n \t\tshow_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,\n \t\t\t       &opt->diffopt);\n@@ -704,7 +703,6 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n \t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n \n-\t\tnext_commentary_block(opt, NULL);\n \t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n \t\t/*\n \t\t * Pass minimum required diff-options to range-diff; others\n@@ -903,8 +901,6 @@ void show_log(struct rev_info *opt)\n \tstrbuf_release(&msgbuf);\n \tfree(ctx.notes_message);\n \tfree(ctx.after_subject);\n-\n-\tshow_diff_of_diff(opt);\n }\n \n int log_tree_diff_flush(struct rev_info *opt)\n@@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \topt->loginfo = NULL;\n \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n \topt->diffopt.no_free = no_free;\n+\tif (shown)\n+\t\tshow_diff_of_diff(opt);\n+\n \tdiff_free(&opt->diffopt);\n \treturn shown;\n }\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba85b582c5..c0c5eccb7c 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2482,13 +2482,18 @@ test_expect_success 'interdiff: reroll-count with a integer' '\n '\n \n test_expect_success 'interdiff: solo-patch' '\n-\tcat >expect <<-\\EOF &&\n-\t  +fleep\n-\n-\tEOF\n \tgit format-patch --interdiff=boop~2 -1 boop &&\n-\ttest_grep \"^Interdiff:$\" 0001-fleep.patch &&\n-\tsed \"1,/^  @@ /d; /^$/q\" 0001-fleep.patch >actual &&\n+\n+\t# remove up to the last \"patch\" output line,\n+\t# and remove everything below the signature mark.\n+\tsed -e \"1,/^+fleep\\$/d\" -e \"/^-- /,\\$d\" 0001-fleep.patch >actual &&\n+\n+\t# fabricate Interdiff output.\n+\tgit diff boop~2 boop >inter &&\n+\t{\n+\t\techo \"Interdiff:\" &&\n+\t\tsed -e \"s/^/  /\" inter\n+\t} >expect &&\n \ttest_cmp expect actual\n '\n \n-- \n2.45.1-246-gb9cfe4845c\n\n"},{"id":"495495","messageId":"83436db5996b129032c2869f331980d0@manjaro.org","threadId":"61541","inReplyTo":"20240523225007.2871766-1-gitster@pobox.com","subject":"Re: [PATCH 0/2] give range-diff at the end of single patch output","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-23T23:22:23Z","receivedAt":"2024-05-23T23:22:25Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-05-24 00:50, Junio C Hamano wrote:\n> When running \"format-patch\" on a multiple patch series, the output\n> coming from \"--interdiff\" and \"--range-diff\" options is inserted\n> after the \"shortlog\" list of commits and the overall diffstat.\n> \n> The idea is that shortlog/diffstat are shorter and with denser\n> information content, which gives a better overview before the\n> readers dive into more details of range/inter diff.\n> \n> When working on a single patch, however, we stuff the inter/range\n> diff output before the actual patch, next to the diffstat.  This\n> pushes down the patch text way down with inter/range diff output,\n> distracting readers.\n> \n> Move the inter/range diff output to the very end of the output,\n> after all the patch text is shown.\n\nHmm...  I think this should be made configurable, with the current\nbehavior being the default.  Without that, we could easily disrupt\nmany people's workflows, because the power of \"muscle memory\" is\noften really strong.\n\nIf it were just about moving a few lines up or down, making it\nconfigurable wouldn't make much sense, but with moving this large\nchunks of text...  It's a different story.\n"},{"id":"495496","messageId":"xmqq34q8qfek.fsf@gitster.g","threadId":"61541","inReplyTo":"83436db5996b129032c2869f331980d0@manjaro.org","subject":"Re: [PATCH 0/2] give range-diff at the end of single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-23T23:25:55Z","receivedAt":"2024-05-23T23:26:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> Hmm...  I think this should be made configurable, with the current\n> behavior being the default.  Without that, we could easily disrupt\n> many people's workflows, because the power of \"muscle memory\" is\n> often really strong.\n\nI would view this more like \"Porcelain layers reserve the right to\nchange the behaviour to suit human-end-user needs, without having to\ncomplicate the system with extra configuration knobs.\"\n\nBut if other people want to do a follow-up patch to cleanly add such\na configuration, I would not object to it.  The main desire of this\npatch is *not* to make the option to have range-diff at the end of a\nsingle patch \"series\" available, but to make that the default.\n\nThanks.\n"},{"id":"495499","messageId":"8b08ccd0953aec5a2fcb3a61bc86c32d@manjaro.org","threadId":"61541","inReplyTo":"xmqq34q8qfek.fsf@gitster.g","subject":"Re: [PATCH 0/2] give range-diff at the end of single patch output","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-23T23:35:17Z","receivedAt":"2024-05-23T23:35:19Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-05-24 01:25, Junio C Hamano wrote:\n> Dragan Simic <dsimic@manjaro.org> writes:\n> \n>> Hmm...  I think this should be made configurable, with the current\n>> behavior being the default.  Without that, we could easily disrupt\n>> many people's workflows, because the power of \"muscle memory\" is\n>> often really strong.\n> \n> I would view this more like \"Porcelain layers reserve the right to\n> change the behaviour to suit human-end-user needs, without having to\n> complicate the system with extra configuration knobs.\"\n> \n> But if other people want to do a follow-up patch to cleanly add such\n> a configuration, I would not object to it.  The main desire of this\n> patch is *not* to make the option to have range-diff at the end of a\n> single patch \"series\" available, but to make that the default.\n\nI see.  Personally, I find the range diffs placed _after_ the actual\npatches more pleasant, but I'm just concerned about other people that\nmight not like such an arrangement that much.\n\nTo me, a range diff is like an additional description of the actual \npatch,\nor like a really long footnote, so to me it makes more sense to put it\nat the end of the \"document\".  Sometimes I don't even want or need to \nlook\nat that footnote, so not needing to scroll down is another plus.\n"},{"id":"495509","messageId":"xmqqed9rq2va.fsf@gitster.g","threadId":"61541","inReplyTo":"8b08ccd0953aec5a2fcb3a61bc86c32d@manjaro.org","subject":"Re: [PATCH 0/2] give range-diff at the end of single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T03:56:41Z","receivedAt":"2024-05-24T03:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic <dsimic@manjaro.org> writes:\n\n> To me, a range diff is like an additional description of the\n> actual patch, or like a really long footnote, so to me it makes\n> more sense to put it at the end of the \"document\".  Sometimes I\n> don't even want or need to look at that footnote, so not needing\n> to scroll down is another plus.\n\nThat apparently makes the three of us, counting Patrick [*].\n\nPersonally, for a single-patch \"series\", I find that interdiff is\noften easier to see than the range diff, but that is a separate\nstory.\n\n[Reference]\n * https://lore.kernel.org/git/Zk7UsJjhY_FV2z8C@tanuki/\n"},{"id":"495560","messageId":"ZlB2g5bTuBFz5m5_@tanuki","threadId":"61541","inReplyTo":"20240523225007.2871766-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-24T11:14:11Z","receivedAt":"2024-05-24T11:14:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote:\n[snip]\n> @@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n>  \topt->loginfo = NULL;\n>  \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n>  \topt->diffopt.no_free = no_free;\n> +\tif (shown)\n> +\t\tshow_diff_of_diff(opt);\n\nShouldn't we write the range-diff before `maybe_flush_or_die()`?\n\n>  \tdiff_free(&opt->diffopt);\n>  \treturn shown;\n>  }\n> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n> index ba85b582c5..c0c5eccb7c 100755\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -2482,13 +2482,18 @@ test_expect_success 'interdiff: reroll-count with a integer' '\n>  '\n>  \n>  test_expect_success 'interdiff: solo-patch' '\n> -\tcat >expect <<-\\EOF &&\n> -\t  +fleep\n> -\n> -\tEOF\n>  \tgit format-patch --interdiff=boop~2 -1 boop &&\n> -\ttest_grep \"^Interdiff:$\" 0001-fleep.patch &&\n> -\tsed \"1,/^  @@ /d; /^$/q\" 0001-fleep.patch >actual &&\n> +\n> +\t# remove up to the last \"patch\" output line,\n> +\t# and remove everything below the signature mark.\n> +\tsed -e \"1,/^+fleep\\$/d\" -e \"/^-- /,\\$d\" 0001-fleep.patch >actual &&\n> +\n> +\t# fabricate Interdiff output.\n> +\tgit diff boop~2 boop >inter &&\n> +\t{\n> +\t\techo \"Interdiff:\" &&\n> +\t\tsed -e \"s/^/  /\" inter\n> +\t} >expect &&\n>  \ttest_cmp expect actual\n>  '\n\nDo we also want to have a test that demonstrates the new behaviour for\nrange-diffs?\n\nI also think that there's a bug here. The output from the above command\nis:\n\n    From 15bea9f4ecca544a87b671e6b9aba65a8c8d9667 Mon Sep 17 00:00:00 2001\n    Message-ID: <15bea9f4ecca544a87b671e6b9aba65a8c8d9667.1716549087.git.committer@example.com>\n    From: A U Thor <author@example.com>\n    Date: Thu, 7 Apr 2005 15:38:13 -0700\n    Subject: [PATCH v2] fleep\n    Header1: B E Cipient <rcipient@example.com>\n    To: Someone <someone@out.there>\n    Cc: C E Cipient <rcipient@example.com>\n\n    ---\n     blorp | 2 +-\n     1 file changed, 1 insertion(+), 1 deletion(-)\n\n    diff --git a/blorp b/blorp\n    index 2fa8fca..bb6e81c 100644\n    --- a/blorp\n    +++ b/blorp\n    @@ -1 +1 @@\n    -fnorp\n    +fleep\n    Interdiff against v1:\n      diff --git a/blorp b/blorp\n      new file mode 100644\n      index 0000000..bb6e81c\n      --- /dev/null\n      +++ b/blorp\n      @@ -0,0 +1 @@\n      +fleep\n    --\n    2.45.1.248.g15a88ae3cc.dirty\n\nThe diff is before the separator for the signature, and there is no\nclear delimiter between the actual diff and the interdiff. The reason\nwhy I wanted to check this in the first place is that I didn't find\nexpectations of the test itself clear, so I wanted to double check what\nthe actual output looks like to confirm that it does the right thing.\n\nPatrick\n"},{"id":"495605","messageId":"xmqqo78ukhmk.fsf@gitster.g","threadId":"61541","inReplyTo":"ZlB2g5bTuBFz5m5_@tanuki","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T21:46:43Z","receivedAt":"2024-05-24T21:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote:\n> [snip]\n>> @@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n>>  \topt->loginfo = NULL;\n>>  \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n>>  \topt->diffopt.no_free = no_free;\n>> +\tif (shown)\n>> +\t\tshow_diff_of_diff(opt);\n>\n> Shouldn't we write the range-diff before `maybe_flush_or_die()`?\n\nHmph, perhaps.  That would catch errors from write done in the\nshow_diff_of_diff() helper.\n\n>> +\n>> +\t# remove up to the last \"patch\" output line,\n>> +\t# and remove everything below the signature mark.\n>> +\tsed -e \"1,/^+fleep\\$/d\" -e \"/^-- /,\\$d\" 0001-fleep.patch >actual &&\n>> +\n>> +\t# fabricate Interdiff output.\n>> +\tgit diff boop~2 boop >inter &&\n>> +\t{\n>> +\t\techo \"Interdiff:\" &&\n>> +\t\tsed -e \"s/^/  /\" inter\n>> +\t} >expect &&\n>>  \ttest_cmp expect actual\n>>  '\n>\n> Do we also want to have a test that demonstrates the new behaviour for\n> range-diffs?\n\nI dunno.  From the whitebox point of view I know it appears at the\nsame place, so it does not matter all that much.\n\n> I also think that there's a bug here. The output from the above command\n> is:\n> ...\n>     --- a/blorp\n>     +++ b/blorp\n>     @@ -1 +1 @@\n>     -fnorp\n>     +fleep\n>     Interdiff against v1:\n>       diff --git a/blorp b/blorp\n> ...\n>\n> The diff is before the separator for the signature, and there is no\n> clear delimiter between the actual diff and the interdiff.\n\nEarlier Eric expressed concern about writing this out _after_ the\nmail signature mark \"-- \", so the output deliberately goes before\nit.  There is no need for any marker after the last line of the\npatch.  \"Interdiff against ...\" is a clear enough delimiter.\n\nFWIW, the parsing of patches has always paid attention to the\nlengths recorded in @@ ... @@ hunk headers, and the parser notices\nwhere the run of (\"diff --git a/... b/...\" followed by a patch) ends\nand stops without problems.  On the other hand, if you remove the\nline \"+fleep\" in the above example and try to feed it to \"git\napply\", it would correctly notice that it failed to see the expected\none line of postimage and complains (because it sees \"Interdiff\nagainst...\"  when it expects to see a line that begins with a plus).\n\nSo, I do not see any problem with the output from this cocde at all.\n\nThanks for careful reading.\n\n"},{"id":"495608","messageId":"xmqqed9qke3k.fsf_-_@gitster.g","threadId":"61541","inReplyTo":"20240523225007.2871766-3-gitster@pobox.com","subject":"[PATCH v2 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-24T23:02:55Z","receivedAt":"2024-05-24T23:03:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When running \"format-patch\" on a multiple patch series, the output\ncoming from \"--interdiff\" and \"--range-diff\" options is inserted\nafter the \"shortlog\" list of commits and the overall diffstat.\n\nThe idea is that shortlog/diffstat are shorter and with denser\ninformation content, which gives a better overview before the\nreaders dive into more details of range/inter diff.\n\nWhen working on a single patch, however, we stuff the inter/range\ndiff output before the actual patch, next to the diffstat.  This\npushes down the patch text way down with inter/range diff output,\ndistracting readers.\n\nMove the inter/range diff output to the very end of the output,\nafter all the patch text is shown.\n\nAs the inter/range diff is no longer part of the commentary block\n(i.e., what comes after the log message and \"---\", but before the\npatch text), stop producing \"---\" in the function that generates\nthem.  But to separate it out visually (note: this is not needed\nto help tools like \"git apply\" that pay attention to the hunk\nheaders to figure out the length of the hunks), add an extra blank\nline between the end of the patch text and the inter/range diff.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * The [1/2] patch is not shown as it is unchanged, to pretend that\n   this [2/2] is a \"single patch series\".  This output (except for\n   this commentary) was created with\n\n   $ git format-patch --interdiff=@{24.hours} -1\n\n log-tree.c              | 11 +++++------\n t/t4014-format-patch.sh | 36 ++++++++++++++++++++++++++++++------\n 2 files changed, 35 insertions(+), 12 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex e7cd2c491f..7de744911e 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -684,8 +684,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n \t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n \n-\t\tnext_commentary_block(opt, NULL);\n-\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->idiff_title);\n+\t\tfprintf_ln(opt->diffopt.file, \"\\n%s\", opt->idiff_title);\n \t\tshow_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,\n \t\t\t       &opt->diffopt);\n \n@@ -704,8 +703,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n \t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n \n-\t\tnext_commentary_block(opt, NULL);\n-\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n+\t\tfprintf_ln(opt->diffopt.file, \"\\n%s\", opt->rdiff_title);\n \t\t/*\n \t\t * Pass minimum required diff-options to range-diff; others\n \t\t * can be added later if deemed desirable.\n@@ -903,8 +901,6 @@ void show_log(struct rev_info *opt)\n \tstrbuf_release(&msgbuf);\n \tfree(ctx.notes_message);\n \tfree(ctx.after_subject);\n-\n-\tshow_diff_of_diff(opt);\n }\n \n int log_tree_diff_flush(struct rev_info *opt)\n@@ -1173,9 +1169,12 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n \t}\n \tif (opt->track_linear && !opt->linear && opt->reverse_output_stage)\n \t\tfprintf(opt->diffopt.file, \"\\n%s\\n\", opt->break_bar);\n+\tif (shown)\n+\t\tshow_diff_of_diff(opt);\n \topt->loginfo = NULL;\n \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n \topt->diffopt.no_free = no_free;\n+\n \tdiff_free(&opt->diffopt);\n \treturn shown;\n }\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex ba85b582c5..de039825a9 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -2482,13 +2482,37 @@ test_expect_success 'interdiff: reroll-count with a integer' '\n '\n \n test_expect_success 'interdiff: solo-patch' '\n-\tcat >expect <<-\\EOF &&\n-\t  +fleep\n-\n-\tEOF\n \tgit format-patch --interdiff=boop~2 -1 boop &&\n-\ttest_grep \"^Interdiff:$\" 0001-fleep.patch &&\n-\tsed \"1,/^  @@ /d; /^$/q\" 0001-fleep.patch >actual &&\n+\n+\t# remove up to the last \"patch\" output line,\n+\t# and remove everything below the signature mark.\n+\tsed -e \"1,/^+fleep\\$/d\" -e \"/^-- /,\\$d\" 0001-fleep.patch >actual &&\n+\n+\t# fabricate Interdiff output.\n+\tgit diff boop~2 boop >inter &&\n+\t{\n+\t\techo &&\n+\t\techo \"Interdiff:\" &&\n+\t\tsed -e \"s/^/  /\" inter\n+\t} >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'range-diff: solo-patch' '\n+\tgit format-patch --creation-factor=999 \\\n+\t\t--range-diff=boop~2..boop~1 -1 boop &&\n+\n+\t# remove up to the last \"patch\" output line,\n+\t# and remove everything below the signature mark.\n+\tsed -e \"1,/^+fleep\\$/d\" -e \"/^-- /,\\$d\" 0001-fleep.patch >actual &&\n+\n+\t# fabricate range-diff output.\n+\t{\n+\t\techo &&\n+\t\techo \"Range-diff:\" &&\n+\t\tgit range-diff --creation-factor=999 \\\n+\t\t\tboop~2..boop~1 boop~1..boop\n+\t} >expect &&\n \ttest_cmp expect actual\n '\n \n\nInterdiff:\n  diff --git a/log-tree.c b/log-tree.c\n  index f28c4d0bb0..7de744911e 100644\n  --- a/log-tree.c\n  +++ b/log-tree.c\n  @@ -684,7 +684,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n   \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n   \t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n   \n  -\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->idiff_title);\n  +\t\tfprintf_ln(opt->diffopt.file, \"\\n%s\", opt->idiff_title);\n   \t\tshow_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,\n   \t\t\t       &opt->diffopt);\n   \n  @@ -703,7 +703,7 @@ static void show_diff_of_diff(struct rev_info *opt)\n   \t\tmemcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));\n   \t\tDIFF_QUEUE_CLEAR(&diff_queued_diff);\n   \n  -\t\tfprintf_ln(opt->diffopt.file, \"%s\", opt->rdiff_title);\n  +\t\tfprintf_ln(opt->diffopt.file, \"\\n%s\", opt->rdiff_title);\n   \t\t/*\n   \t\t * Pass minimum required diff-options to range-diff; others\n   \t\t * can be added later if deemed desirable.\n  @@ -1169,11 +1169,11 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)\n   \t}\n   \tif (opt->track_linear && !opt->linear && opt->reverse_output_stage)\n   \t\tfprintf(opt->diffopt.file, \"\\n%s\\n\", opt->break_bar);\n  +\tif (shown)\n  +\t\tshow_diff_of_diff(opt);\n   \topt->loginfo = NULL;\n   \tmaybe_flush_or_die(opt->diffopt.file, \"stdout\");\n   \topt->diffopt.no_free = no_free;\n  -\tif (shown)\n  -\t\tshow_diff_of_diff(opt);\n   \n   \tdiff_free(&opt->diffopt);\n   \treturn shown;\n  diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\n  index c0c5eccb7c..de039825a9 100755\n  --- a/t/t4014-format-patch.sh\n  +++ b/t/t4014-format-patch.sh\n  @@ -2491,12 +2491,31 @@ test_expect_success 'interdiff: solo-patch' '\n   \t# fabricate Interdiff output.\n   \tgit diff boop~2 boop >inter &&\n   \t{\n  +\t\techo &&\n   \t\techo \"Interdiff:\" &&\n   \t\tsed -e \"s/^/  /\" inter\n   \t} >expect &&\n   \ttest_cmp expect actual\n   '\n   \n  +test_expect_success 'range-diff: solo-patch' '\n  +\tgit format-patch --creation-factor=999 \\\n  +\t\t--range-diff=boop~2..boop~1 -1 boop &&\n  +\n  +\t# remove up to the last \"patch\" output line,\n  +\t# and remove everything below the signature mark.\n  +\tsed -e \"1,/^+fleep\\$/d\" -e \"/^-- /,\\$d\" 0001-fleep.patch >actual &&\n  +\n  +\t# fabricate range-diff output.\n  +\t{\n  +\t\techo &&\n  +\t\techo \"Range-diff:\" &&\n  +\t\tgit range-diff --creation-factor=999 \\\n  +\t\t\tboop~2..boop~1 boop~1..boop\n  +\t} >expect &&\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.1-248-g15a88ae3cc\n\n"},{"id":"495663","messageId":"ZlQX0FmIsz2eFgsC@tanuki","threadId":"61541","inReplyTo":"xmqqo78ukhmk.fsf@gitster.g","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-27T05:19:12Z","receivedAt":"2024-05-27T05:19:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 24, 2024 at 02:46:43PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> > On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote:\n> > I also think that there's a bug here. The output from the above command\n> > is:\n> > ...\n> >     --- a/blorp\n> >     +++ b/blorp\n> >     @@ -1 +1 @@\n> >     -fnorp\n> >     +fleep\n> >     Interdiff against v1:\n> >       diff --git a/blorp b/blorp\n> > ...\n> >\n> > The diff is before the separator for the signature, and there is no\n> > clear delimiter between the actual diff and the interdiff.\n> \n> Earlier Eric expressed concern about writing this out _after_ the\n> mail signature mark \"-- \", so the output deliberately goes before\n> it.  There is no need for any marker after the last line of the\n> patch.  \"Interdiff against ...\" is a clear enough delimiter.\n> \n> FWIW, the parsing of patches has always paid attention to the\n> lengths recorded in @@ ... @@ hunk headers, and the parser notices\n> where the run of (\"diff --git a/... b/...\" followed by a patch) ends\n> and stops without problems.  On the other hand, if you remove the\n> line \"+fleep\" in the above example and try to feed it to \"git\n> apply\", it would correctly notice that it failed to see the expected\n> one line of postimage and complains (because it sees \"Interdiff\n> against...\"  when it expects to see a line that begins with a plus).\n> \n> So, I do not see any problem with the output from this cocde at all.\n> \n> Thanks for careful reading.\n\nThe machine can cope alright. But I think that it's way harder to parse\nfor a human if there is no clear visual delimiter between the diff and\nthe interdiff. And \"Interdiff\" isn't quite ideal in my opinion because\nit is text, only, and may be quite easy to miss if it follows a long\ndiff.\n\nThe signature mark may not be ideal here as an indicator. Mail readers\nmay hide signatures, color them differently or other stuff. But I think\nthere should be some indicator here that visually highlights the fact\nthat one section is ending and another section is starting. This could\neither be a newline, or the triple-dashes as we use in other places.\n\nPatrick\n"},{"id":"495692","messageId":"a4f6cf69b5fb189a80f69dcf6d5a8731@manjaro.org","threadId":"61541","inReplyTo":"ZlQX0FmIsz2eFgsC@tanuki","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-27T12:59:21Z","receivedAt":"2024-05-27T12:59:24Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-05-27 07:19, Patrick Steinhardt wrote:\n> On Fri, May 24, 2024 at 02:46:43PM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n>> > On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote:\n>> > I also think that there's a bug here. The output from the above command\n>> > is:\n>> > ...\n>> >     --- a/blorp\n>> >     +++ b/blorp\n>> >     @@ -1 +1 @@\n>> >     -fnorp\n>> >     +fleep\n>> >     Interdiff against v1:\n>> >       diff --git a/blorp b/blorp\n>> > ...\n>> >\n>> > The diff is before the separator for the signature, and there is no\n>> > clear delimiter between the actual diff and the interdiff.\n>> \n>> Earlier Eric expressed concern about writing this out _after_ the\n>> mail signature mark \"-- \", so the output deliberately goes before\n>> it.  There is no need for any marker after the last line of the\n>> patch.  \"Interdiff against ...\" is a clear enough delimiter.\n>> \n>> FWIW, the parsing of patches has always paid attention to the\n>> lengths recorded in @@ ... @@ hunk headers, and the parser notices\n>> where the run of (\"diff --git a/... b/...\" followed by a patch) ends\n>> and stops without problems.  On the other hand, if you remove the\n>> line \"+fleep\" in the above example and try to feed it to \"git\n>> apply\", it would correctly notice that it failed to see the expected\n>> one line of postimage and complains (because it sees \"Interdiff\n>> against...\"  when it expects to see a line that begins with a plus).\n>> \n>> So, I do not see any problem with the output from this cocde at all.\n>> \n>> Thanks for careful reading.\n> \n> The machine can cope alright. But I think that it's way harder to parse\n> for a human if there is no clear visual delimiter between the diff and\n> the interdiff. And \"Interdiff\" isn't quite ideal in my opinion because\n> it is text, only, and may be quite easy to miss if it follows a long\n> diff.\n> \n> The signature mark may not be ideal here as an indicator. Mail readers\n> may hide signatures, color them differently or other stuff. But I think\n> there should be some indicator here that visually highlights the fact\n> that one section is ending and another section is starting. This could\n> either be a newline, or the triple-dashes as we use in other places.\n\nI agree about the need for having a distinctive separator.\n"},{"id":"495700","messageId":"xmqqsey39mmt.fsf@gitster.g","threadId":"61541","inReplyTo":"ZlQX0FmIsz2eFgsC@tanuki","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-27T17:43:06Z","receivedAt":"2024-05-27T17:43:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> The machine can cope alright. But I think that it's way harder to parse\n> for a human if there is no clear visual delimiter between the diff and\n> the interdiff. And \"Interdiff\" isn't quite ideal in my opinion because\n> it is text, only, and may be quite easy to miss if it follows a long\n> diff.\n\nApparently our messages crossed.   See <xmqqed9qke3k.fsf_-_@gitster.g>\nthat takes advantage of the fact that \"the machine can cope alright\"\nwith an extra blank line ;-).  The message is its own demonstration.\n\nThanks.\n\n"},{"id":"495751","messageId":"ZlXbxzFOJ8gVv7r5@tanuki","threadId":"61541","inReplyTo":"xmqqsey39mmt.fsf@gitster.g","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-28T13:27:35Z","receivedAt":"2024-05-28T13:27:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, May 27, 2024 at 10:43:06AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > The machine can cope alright. But I think that it's way harder to parse\n> > for a human if there is no clear visual delimiter between the diff and\n> > the interdiff. And \"Interdiff\" isn't quite ideal in my opinion because\n> > it is text, only, and may be quite easy to miss if it follows a long\n> > diff.\n> \n> Apparently our messages crossed.   See <xmqqed9qke3k.fsf_-_@gitster.g>\n> that takes advantage of the fact that \"the machine can cope alright\"\n> with an extra blank line ;-).  The message is its own demonstration.\n> \n> Thanks.\n\nYeah, that's definitely better. Whether it's preferable over having it\nafter the signature separator I don't know. I personally liked that\nversion better, but can totally see why others may not like it.\n\nHm. By now I've gotten a bit indifferent, to be honest. I'm not a 100%\nsure whether it's an improvement or not, but I don't have a strong\nopinion either way.\n\nPatrick\n"},{"id":"495761","messageId":"xmqqv82x6fto.fsf@gitster.g","threadId":"61541","inReplyTo":"ZlXbxzFOJ8gVv7r5@tanuki","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-28T16:50:43Z","receivedAt":"2024-05-28T16:50:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Yeah, that's definitely better. Whether it's preferable over having it\n> after the signature separator I don't know. I personally liked that\n> version better, but can totally see why others may not like it.\n\nI do not think anybody posted a version that writes inter/range diff\nater the signature mark.\n\n> Hm. By now I've gotten a bit indifferent, to be honest. I'm not a 100%\n> sure whether it's an improvement or not, but I don't have a strong\n> opinion either way.\n\nI am not sure what two you are comparing.  The current version with\ninter/range diff that is before the diffstat and the proposed one\nthat places inter/range diff after the main patch?  Between them, I\ndo have a strong preference.\n\nOr placing the inter/range diff after the main patch, before or\nafter the signature mark?  As a reader of such a patch, I do not\nhave strong preference myself, either, but the signature mark is a\nconvention, established and honored for more than a few decades, to\nsay \"no interesting contents come after this line\".  I do not think\nof a strong reason to go against that convention.\n\nWe certainly could use the \"---\" after the main patch before we add\nthe inter/range diff.  I had such a version but its output looked\nrather ugly.  Because the inter/range diff output are designed to be\nvery distinct from the usual patch, I'd say something as innocuous\nas an extra blank line would be a good choice.\n\nThanks.\n\n"},{"id":"495774","messageId":"Zla-GJ6NpSNNVDXq@tanuki","threadId":"61541","inReplyTo":"xmqqv82x6fto.fsf@gitster.g","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-29T05:33:12Z","receivedAt":"2024-05-29T05:33:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, May 28, 2024 at 09:50:43AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > Yeah, that's definitely better. Whether it's preferable over having it\n> > after the signature separator I don't know. I personally liked that\n> > version better, but can totally see why others may not like it.\n> \n> I do not think anybody posted a version that writes inter/range diff\n> ater the signature mark.\n\nNo, I'm talking about the version that you hand crafted initially and\nthat kicked off this topic.\n\n> > Hm. By now I've gotten a bit indifferent, to be honest. I'm not a 100%\n> > sure whether it's an improvement or not, but I don't have a strong\n> > opinion either way.\n> \n> I am not sure what two you are comparing.  The current version with\n> inter/range diff that is before the diffstat and the proposed one\n> that places inter/range diff after the main patch?  Between them, I\n> do have a strong preference.\n\nYeah, that's the one I'm talking about.\n\n> Or placing the inter/range diff after the main patch, before or\n> after the signature mark?  As a reader of such a patch, I do not\n> have strong preference myself, either, but the signature mark is a\n> convention, established and honored for more than a few decades, to\n> say \"no interesting contents come after this line\".  I do not think\n> of a strong reason to go against that convention.\n\nWell, agreed. I liked it because it rendered nicely for me, but as I\nsaid, I can very much understand why others are not so thrilled.\n\n> We certainly could use the \"---\" after the main patch before we add\n> the inter/range diff.  I had such a version but its output looked\n> rather ugly.  Because the inter/range diff output are designed to be\n> very distinct from the usual patch, I'd say something as innocuous\n> as an extra blank line would be a good choice.\n\nFair enough. In any case, I think the result looks fine with the extra\nblank line. I just don't have a strong preference between the old and\nnew formats by now. If you or others feel strongly I don't mind at all\nif this patch lands.\n\nPatrick\n"},{"id":"495835","messageId":"xmqqplt41yk4.fsf@gitster.g","threadId":"61541","inReplyTo":"Zla-GJ6NpSNNVDXq@tanuki","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-29T14:29:31Z","receivedAt":"2024-05-29T14:29:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Tue, May 28, 2024 at 09:50:43AM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt <ps@pks.im> writes:\n>> \n>> > Yeah, that's definitely better. Whether it's preferable over having it\n>> > after the signature separator I don't know. I personally liked that\n>> > version better, but can totally see why others may not like it.\n>> \n>> I do not think anybody posted a version that writes inter/range diff\n>> ater the signature mark.\n>\n> No, I'm talking about the version that you hand crafted initially and\n> that kicked off this topic.\n\nAh, https://lore.kernel.org/git/xmqqh6ep1pwz.fsf_-_@gitster.g/ I\nforgot all about it already ;-).\n\n> ... I just don't have a strong preference between the old and\n> new formats by now. If you or others feel strongly I don't mind at all\n> if this patch lands.\n\nLet's scrap it then.  I do not think a single-patch topic happens\nall that often anyway.\n\n"},{"id":"495969","messageId":"e01506f4c8129318dbcba553d7c90b13@manjaro.org","threadId":"61541","inReplyTo":"xmqqplt41yk4.fsf@gitster.g","subject":"Re: [PATCH 2/2] format-patch: move range/inter diff at the end of a single patch output","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-05-30T20:05:47Z","receivedAt":"2024-05-30T20:05:49Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-05-29 16:29, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n>> On Tue, May 28, 2024 at 09:50:43AM -0700, Junio C Hamano wrote:\n>>> Patrick Steinhardt <ps@pks.im> writes:\n>>> \n>>> > Yeah, that's definitely better. Whether it's preferable over having it\n>>> > after the signature separator I don't know. I personally liked that\n>>> > version better, but can totally see why others may not like it.\n>>> \n>>> I do not think anybody posted a version that writes inter/range diff\n>>> ater the signature mark.\n>> \n>> No, I'm talking about the version that you hand crafted initially and\n>> that kicked off this topic.\n> \n> Ah, https://lore.kernel.org/git/xmqqh6ep1pwz.fsf_-_@gitster.g/ I\n> forgot all about it already ;-).\n> \n>> ... I just don't have a strong preference between the old and\n>> new formats by now. If you or others feel strongly I don't mind at all\n>> if this patch lands.\n> \n> Let's scrap it then.  I do not think a single-patch topic happens\n> all that often anyway.\n\nHmm.  Actually, I find it logical and I don't think it should be \nscrapped.\nAs I wrote already, I find range diffs as really long footnotes, and\nplacing them at the end of \"documents\" seems like a logical choice to \nme.\n"}]}