| Message ID | 20240523225007.2871766-3-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | give range-diff at the end of single patch output | expand |
On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote: [snip] > @@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit) > opt->loginfo = NULL; > maybe_flush_or_die(opt->diffopt.file, "stdout"); > opt->diffopt.no_free = no_free; > + if (shown) > + show_diff_of_diff(opt); Shouldn't we write the range-diff before `maybe_flush_or_die()`? > diff_free(&opt->diffopt); > return shown; > } > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh > index ba85b582c5..c0c5eccb7c 100755 > --- a/t/t4014-format-patch.sh > +++ b/t/t4014-format-patch.sh > @@ -2482,13 +2482,18 @@ test_expect_success 'interdiff: reroll-count with a integer' ' > ' > > test_expect_success 'interdiff: solo-patch' ' > - cat >expect <<-\EOF && > - +fleep > - > - EOF > git format-patch --interdiff=boop~2 -1 boop && > - test_grep "^Interdiff:$" 0001-fleep.patch && > - sed "1,/^ @@ /d; /^$/q" 0001-fleep.patch >actual && > + > + # remove up to the last "patch" output line, > + # and remove everything below the signature mark. > + sed -e "1,/^+fleep\$/d" -e "/^-- /,\$d" 0001-fleep.patch >actual && > + > + # fabricate Interdiff output. > + git diff boop~2 boop >inter && > + { > + echo "Interdiff:" && > + sed -e "s/^/ /" inter > + } >expect && > test_cmp expect actual > ' Do we also want to have a test that demonstrates the new behaviour for range-diffs? I also think that there's a bug here. The output from the above command is: From 15bea9f4ecca544a87b671e6b9aba65a8c8d9667 Mon Sep 17 00:00:00 2001 Message-ID: <15bea9f4ecca544a87b671e6b9aba65a8c8d9667.1716549087.git.committer@example.com> From: A U Thor <author@example.com> Date: Thu, 7 Apr 2005 15:38:13 -0700 Subject: [PATCH v2] fleep Header1: B E Cipient <rcipient@example.com> To: Someone <someone@out.there> Cc: C E Cipient <rcipient@example.com> --- blorp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blorp b/blorp index 2fa8fca..bb6e81c 100644 --- a/blorp +++ b/blorp @@ -1 +1 @@ -fnorp +fleep Interdiff against v1: diff --git a/blorp b/blorp new file mode 100644 index 0000000..bb6e81c --- /dev/null +++ b/blorp @@ -0,0 +1 @@ +fleep -- 2.45.1.248.g15a88ae3cc.dirty The diff is before the separator for the signature, and there is no clear delimiter between the actual diff and the interdiff. The reason why I wanted to check this in the first place is that I didn't find expectations of the test itself clear, so I wanted to double check what the actual output looks like to confirm that it does the right thing. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote: > [snip] >> @@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit) >> opt->loginfo = NULL; >> maybe_flush_or_die(opt->diffopt.file, "stdout"); >> opt->diffopt.no_free = no_free; >> + if (shown) >> + show_diff_of_diff(opt); > > Shouldn't we write the range-diff before `maybe_flush_or_die()`? Hmph, perhaps. That would catch errors from write done in the show_diff_of_diff() helper. >> + >> + # remove up to the last "patch" output line, >> + # and remove everything below the signature mark. >> + sed -e "1,/^+fleep\$/d" -e "/^-- /,\$d" 0001-fleep.patch >actual && >> + >> + # fabricate Interdiff output. >> + git diff boop~2 boop >inter && >> + { >> + echo "Interdiff:" && >> + sed -e "s/^/ /" inter >> + } >expect && >> test_cmp expect actual >> ' > > Do we also want to have a test that demonstrates the new behaviour for > range-diffs? I dunno. From the whitebox point of view I know it appears at the same place, so it does not matter all that much. > I also think that there's a bug here. The output from the above command > is: > ... > --- a/blorp > +++ b/blorp > @@ -1 +1 @@ > -fnorp > +fleep > Interdiff against v1: > diff --git a/blorp b/blorp > ... > > The diff is before the separator for the signature, and there is no > clear delimiter between the actual diff and the interdiff. Earlier Eric expressed concern about writing this out _after_ the mail signature mark "-- ", so the output deliberately goes before it. There is no need for any marker after the last line of the patch. "Interdiff against ..." is a clear enough delimiter. FWIW, the parsing of patches has always paid attention to the lengths recorded in @@ ... @@ hunk headers, and the parser notices where the run of ("diff --git a/... b/..." followed by a patch) ends and stops without problems. On the other hand, if you remove the line "+fleep" in the above example and try to feed it to "git apply", it would correctly notice that it failed to see the expected one line of postimage and complains (because it sees "Interdiff against..." when it expects to see a line that begins with a plus). So, I do not see any problem with the output from this cocde at all. Thanks for careful reading.
On Fri, May 24, 2024 at 02:46:43PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote: > > I also think that there's a bug here. The output from the above command > > is: > > ... > > --- a/blorp > > +++ b/blorp > > @@ -1 +1 @@ > > -fnorp > > +fleep > > Interdiff against v1: > > diff --git a/blorp b/blorp > > ... > > > > The diff is before the separator for the signature, and there is no > > clear delimiter between the actual diff and the interdiff. > > Earlier Eric expressed concern about writing this out _after_ the > mail signature mark "-- ", so the output deliberately goes before > it. There is no need for any marker after the last line of the > patch. "Interdiff against ..." is a clear enough delimiter. > > FWIW, the parsing of patches has always paid attention to the > lengths recorded in @@ ... @@ hunk headers, and the parser notices > where the run of ("diff --git a/... b/..." followed by a patch) ends > and stops without problems. On the other hand, if you remove the > line "+fleep" in the above example and try to feed it to "git > apply", it would correctly notice that it failed to see the expected > one line of postimage and complains (because it sees "Interdiff > against..." when it expects to see a line that begins with a plus). > > So, I do not see any problem with the output from this cocde at all. > > Thanks for careful reading. The machine can cope alright. But I think that it's way harder to parse for a human if there is no clear visual delimiter between the diff and the interdiff. And "Interdiff" isn't quite ideal in my opinion because it is text, only, and may be quite easy to miss if it follows a long diff. The signature mark may not be ideal here as an indicator. Mail readers may hide signatures, color them differently or other stuff. But I think there should be some indicator here that visually highlights the fact that one section is ending and another section is starting. This could either be a newline, or the triple-dashes as we use in other places. Patrick
On 2024-05-27 07:19, Patrick Steinhardt wrote: > On Fri, May 24, 2024 at 02:46:43PM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> > On Thu, May 23, 2024 at 03:50:07PM -0700, Junio C Hamano wrote: >> > I also think that there's a bug here. The output from the above command >> > is: >> > ... >> > --- a/blorp >> > +++ b/blorp >> > @@ -1 +1 @@ >> > -fnorp >> > +fleep >> > Interdiff against v1: >> > diff --git a/blorp b/blorp >> > ... >> > >> > The diff is before the separator for the signature, and there is no >> > clear delimiter between the actual diff and the interdiff. >> >> Earlier Eric expressed concern about writing this out _after_ the >> mail signature mark "-- ", so the output deliberately goes before >> it. There is no need for any marker after the last line of the >> patch. "Interdiff against ..." is a clear enough delimiter. >> >> FWIW, the parsing of patches has always paid attention to the >> lengths recorded in @@ ... @@ hunk headers, and the parser notices >> where the run of ("diff --git a/... b/..." followed by a patch) ends >> and stops without problems. On the other hand, if you remove the >> line "+fleep" in the above example and try to feed it to "git >> apply", it would correctly notice that it failed to see the expected >> one line of postimage and complains (because it sees "Interdiff >> against..." when it expects to see a line that begins with a plus). >> >> So, I do not see any problem with the output from this cocde at all. >> >> Thanks for careful reading. > > The machine can cope alright. But I think that it's way harder to parse > for a human if there is no clear visual delimiter between the diff and > the interdiff. And "Interdiff" isn't quite ideal in my opinion because > it is text, only, and may be quite easy to miss if it follows a long > diff. > > The signature mark may not be ideal here as an indicator. Mail readers > may hide signatures, color them differently or other stuff. But I think > there should be some indicator here that visually highlights the fact > that one section is ending and another section is starting. This could > either be a newline, or the triple-dashes as we use in other places. I agree about the need for having a distinctive separator.
Patrick Steinhardt <ps@pks.im> writes: > The machine can cope alright. But I think that it's way harder to parse > for a human if there is no clear visual delimiter between the diff and > the interdiff. And "Interdiff" isn't quite ideal in my opinion because > it is text, only, and may be quite easy to miss if it follows a long > diff. Apparently our messages crossed. See <xmqqed9qke3k.fsf_-_@gitster.g> that takes advantage of the fact that "the machine can cope alright" with an extra blank line ;-). The message is its own demonstration. Thanks.
On Mon, May 27, 2024 at 10:43:06AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > The machine can cope alright. But I think that it's way harder to parse > > for a human if there is no clear visual delimiter between the diff and > > the interdiff. And "Interdiff" isn't quite ideal in my opinion because > > it is text, only, and may be quite easy to miss if it follows a long > > diff. > > Apparently our messages crossed. See <xmqqed9qke3k.fsf_-_@gitster.g> > that takes advantage of the fact that "the machine can cope alright" > with an extra blank line ;-). The message is its own demonstration. > > Thanks. Yeah, that's definitely better. Whether it's preferable over having it after the signature separator I don't know. I personally liked that version better, but can totally see why others may not like it. Hm. By now I've gotten a bit indifferent, to be honest. I'm not a 100% sure whether it's an improvement or not, but I don't have a strong opinion either way. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Yeah, that's definitely better. Whether it's preferable over having it > after the signature separator I don't know. I personally liked that > version better, but can totally see why others may not like it. I do not think anybody posted a version that writes inter/range diff ater the signature mark. > Hm. By now I've gotten a bit indifferent, to be honest. I'm not a 100% > sure whether it's an improvement or not, but I don't have a strong > opinion either way. I am not sure what two you are comparing. The current version with inter/range diff that is before the diffstat and the proposed one that places inter/range diff after the main patch? Between them, I do have a strong preference. Or placing the inter/range diff after the main patch, before or after the signature mark? As a reader of such a patch, I do not have strong preference myself, either, but the signature mark is a convention, established and honored for more than a few decades, to say "no interesting contents come after this line". I do not think of a strong reason to go against that convention. We certainly could use the "---" after the main patch before we add the inter/range diff. I had such a version but its output looked rather ugly. Because the inter/range diff output are designed to be very distinct from the usual patch, I'd say something as innocuous as an extra blank line would be a good choice. Thanks.
On Tue, May 28, 2024 at 09:50:43AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Yeah, that's definitely better. Whether it's preferable over having it > > after the signature separator I don't know. I personally liked that > > version better, but can totally see why others may not like it. > > I do not think anybody posted a version that writes inter/range diff > ater the signature mark. No, I'm talking about the version that you hand crafted initially and that kicked off this topic. > > Hm. By now I've gotten a bit indifferent, to be honest. I'm not a 100% > > sure whether it's an improvement or not, but I don't have a strong > > opinion either way. > > I am not sure what two you are comparing. The current version with > inter/range diff that is before the diffstat and the proposed one > that places inter/range diff after the main patch? Between them, I > do have a strong preference. Yeah, that's the one I'm talking about. > Or placing the inter/range diff after the main patch, before or > after the signature mark? As a reader of such a patch, I do not > have strong preference myself, either, but the signature mark is a > convention, established and honored for more than a few decades, to > say "no interesting contents come after this line". I do not think > of a strong reason to go against that convention. Well, agreed. I liked it because it rendered nicely for me, but as I said, I can very much understand why others are not so thrilled. > We certainly could use the "---" after the main patch before we add > the inter/range diff. I had such a version but its output looked > rather ugly. Because the inter/range diff output are designed to be > very distinct from the usual patch, I'd say something as innocuous > as an extra blank line would be a good choice. Fair enough. In any case, I think the result looks fine with the extra blank line. I just don't have a strong preference between the old and new formats by now. If you or others feel strongly I don't mind at all if this patch lands. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Tue, May 28, 2024 at 09:50:43AM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> > Yeah, that's definitely better. Whether it's preferable over having it >> > after the signature separator I don't know. I personally liked that >> > version better, but can totally see why others may not like it. >> >> I do not think anybody posted a version that writes inter/range diff >> ater the signature mark. > > No, I'm talking about the version that you hand crafted initially and > that kicked off this topic. Ah, https://lore.kernel.org/git/xmqqh6ep1pwz.fsf_-_@gitster.g/ I forgot all about it already ;-). > ... I just don't have a strong preference between the old and > new formats by now. If you or others feel strongly I don't mind at all > if this patch lands. Let's scrap it then. I do not think a single-patch topic happens all that often anyway.
On 2024-05-29 16:29, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > >> On Tue, May 28, 2024 at 09:50:43AM -0700, Junio C Hamano wrote: >>> Patrick Steinhardt <ps@pks.im> writes: >>> >>> > Yeah, that's definitely better. Whether it's preferable over having it >>> > after the signature separator I don't know. I personally liked that >>> > version better, but can totally see why others may not like it. >>> >>> I do not think anybody posted a version that writes inter/range diff >>> ater the signature mark. >> >> No, I'm talking about the version that you hand crafted initially and >> that kicked off this topic. > > Ah, https://lore.kernel.org/git/xmqqh6ep1pwz.fsf_-_@gitster.g/ I > forgot all about it already ;-). > >> ... I just don't have a strong preference between the old and >> new formats by now. If you or others feel strongly I don't mind at all >> if this patch lands. > > Let's scrap it then. I do not think a single-patch topic happens > all that often anyway. Hmm. Actually, I find it logical and I don't think it should be scrapped. As I wrote already, I find range diffs as really long footnotes, and placing them at the end of "documents" seems like a logical choice to me.
diff output before the actual patch, next to the diffstat. This pushes down the patch text way down with inter/range diff output, distracting readers. Move the inter/range diff output to the very end of the output, after all the patch text is shown. As the inter/range diff is no longer part of the commentary block (i.e., what comes after the log message and "---", but before the patch text), stop producing "---" in the function that generates them. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- log-tree.c | 7 +++---- t/t4014-format-patch.sh | 17 +++++++++++------ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/log-tree.c b/log-tree.c index e7cd2c491f..f28c4d0bb0 100644 --- a/log-tree.c +++ b/log-tree.c @@ -684,7 +684,6 @@ static void show_diff_of_diff(struct rev_info *opt) memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); DIFF_QUEUE_CLEAR(&diff_queued_diff); - next_commentary_block(opt, NULL); fprintf_ln(opt->diffopt.file, "%s", opt->idiff_title); show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2, &opt->diffopt); @@ -704,7 +703,6 @@ static void show_diff_of_diff(struct rev_info *opt) memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff)); DIFF_QUEUE_CLEAR(&diff_queued_diff); - next_commentary_block(opt, NULL); fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title); /* * Pass minimum required diff-options to range-diff; others @@ -903,8 +901,6 @@ void show_log(struct rev_info *opt) strbuf_release(&msgbuf); free(ctx.notes_message); free(ctx.after_subject); - - show_diff_of_diff(opt); } int log_tree_diff_flush(struct rev_info *opt) @@ -1176,6 +1172,9 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit) opt->loginfo = NULL; maybe_flush_or_die(opt->diffopt.file, "stdout"); opt->diffopt.no_free = no_free; + if (shown) + show_diff_of_diff(opt); + diff_free(&opt->diffopt); return shown; } diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index ba85b582c5..c0c5eccb7c 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -2482,13 +2482,18 @@ test_expect_success 'interdiff: reroll-count with a integer' ' ' test_expect_success 'interdiff: solo-patch' ' - cat >expect <<-\EOF && - +fleep - - EOF git format-patch --interdiff=boop~2 -1 boop && - test_grep "^Interdiff:$" 0001-fleep.patch && - sed "1,/^ @@ /d; /^$/q" 0001-fleep.patch >actual && + + # remove up to the last "patch" output line, + # and remove everything below the signature mark. + sed -e "1,/^+fleep\$/d" -e "/^-- /,\$d" 0001-fleep.patch >actual && + + # fabricate Interdiff output. + git diff boop~2 boop >inter && + { + echo "Interdiff:" && + sed -e "s/^/ /" inter + } >expect && test_cmp expect actual '