diff mbox series

[2/2] format-patch: move range/inter diff at the end of a single patch output

Message ID 20240523225007.2871766-3-gitster@pobox.com (mailing list archive)
State New, archived
Headers show
Series give range-diff at the end of single patch output | expand

Commit Message

Junio C Hamano May 23, 2024, 10:50 p.m. UTC
When running "format-patch" on a multiple patch series, the output
coming from "--interdiff" and "--range-diff" options is inserted
after the "shortlog" list of commits and the overall diffstat.

The idea is that shortlog/diffstat are shorter and with denser
information content, which gives a better overview before the
readers dive into more details of range/inter diff.

When working on a single patch, however, we stuff the inter/range

Comments

Patrick Steinhardt May 24, 2024, 11:14 a.m. UTC | #1
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
Junio C Hamano May 24, 2024, 9:46 p.m. UTC | #2
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.
Patrick Steinhardt May 27, 2024, 5:19 a.m. UTC | #3
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
Dragan Simic May 27, 2024, 12:59 p.m. UTC | #4
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.
Junio C Hamano May 27, 2024, 5:43 p.m. UTC | #5
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.
Patrick Steinhardt May 28, 2024, 1:27 p.m. UTC | #6
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
Junio C Hamano May 28, 2024, 4:50 p.m. UTC | #7
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.
Patrick Steinhardt May 29, 2024, 5:33 a.m. UTC | #8
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
Junio C Hamano May 29, 2024, 2:29 p.m. UTC | #9
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.
Dragan Simic May 30, 2024, 8:05 p.m. UTC | #10
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 mbox series

Patch

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
 '