[PATCH 0/2] give range-diff at the end of single patch output

[PATCH 0/2] give range-diff at the end of single patch output

[Junio C Hamano]

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
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.

The first patch is a no-op refactoring, the second patch makes the
actual behaviour change.

Junio C Hamano (2):
  show_log: factor out interdiff/range-diff generation
  format-patch: move range/inter diff at the end of a single patch
    output

 log-tree.c              | 89 ++++++++++++++++++++++-------------------
 t/t4014-format-patch.sh | 17 +++++---
 2 files changed, 59 insertions(+), 47 deletions(-)

-- 
2.45.1-246-gb9cfe4845c

[PATCH 1/2] show_log: factor out interdiff/range-diff generation

[Junio C Hamano]

The integration of "git range-diff" with "git format-patch" for a
single patch (i.e., not generating "range-diff" into the cover
letter) hooks into log-tree.c:show_log(), which is responsible for
writing the log message out and other stuff.  Essentially,
everything you see before the diffstat and the patch is generated
there.

Split out the code that spits out the interdiff/range-diff into a
separate helper function show_diff_of_diff().  Hopefully this will
make it easier to move things around in the output stream in the
future patches.

This is supposed to be a no-op refactoring.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 log-tree.c | 88 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 40 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 41416de4e3..e7cd2c491f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -673,6 +673,53 @@ static void next_commentary_block(struct rev_info *opt, struct strbuf *sb)
 	opt->shown_dashes = 1;
 }
 
+static void show_diff_of_diff(struct rev_info *opt)
+{
+	if (!cmit_fmt_is_mail(opt->commit_format))
+		return;
+
+	if (opt->idiff_oid1) {
+		struct diff_queue_struct dq;
+
+		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);
+
+		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
+	}
+
+	if (opt->rdiff1) {
+		struct diff_queue_struct dq;
+		struct diff_options opts;
+		struct range_diff_options range_diff_opts = {
+			.creation_factor = opt->creation_factor,
+			.dual_color = 1,
+			.diffopt = &opts
+		};
+
+		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
+		 * can be added later if deemed desirable.
+		 */
+		repo_diff_setup(the_repository, &opts);
+		opts.file = opt->diffopt.file;
+		opts.use_color = opt->diffopt.use_color;
+		diff_setup_done(&opts);
+		show_range_diff(opt->rdiff1, opt->rdiff2, &range_diff_opts);
+
+		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
+	}
+}
+
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
@@ -857,46 +904,7 @@ void show_log(struct rev_info *opt)
 	free(ctx.notes_message);
 	free(ctx.after_subject);
 
-	if (cmit_fmt_is_mail(ctx.fmt) && opt->idiff_oid1) {
-		struct diff_queue_struct dq;
-
-		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);
-
-		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
-	}
-
-	if (cmit_fmt_is_mail(ctx.fmt) && opt->rdiff1) {
-		struct diff_queue_struct dq;
-		struct diff_options opts;
-		struct range_diff_options range_diff_opts = {
-			.creation_factor = opt->creation_factor,
-			.dual_color = 1,
-			.diffopt = &opts
-		};
-
-		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
-		 * can be added later if deemed desirable.
-		 */
-		repo_diff_setup(the_repository, &opts);
-		opts.file = opt->diffopt.file;
-		opts.use_color = opt->diffopt.use_color;
-		diff_setup_done(&opts);
-		show_range_diff(opt->rdiff1, opt->rdiff2, &range_diff_opts);
-
-		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
-	}
+	show_diff_of_diff(opt);
 }
 
 int log_tree_diff_flush(struct rev_info *opt)
-- 
2.45.1-246-gb9cfe4845c

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

[Junio C Hamano]

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
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
 '
 
-- 
2.45.1-246-gb9cfe4845c

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2779 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dragan Simic]

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.

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

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.

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2124 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Junio C Hamano]

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.

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

[Dragan Simic]

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.

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

[Junio C Hamano]

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
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.  But to separate it out visually (note: this is not needed
to help tools like "git apply" that pay attention to the hunk
headers to figure out the length of the hunks), add an extra blank
line between the end of the patch text and the inter/range diff.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The [1/2] patch is not shown as it is unchanged, to pretend that
   this [2/2] is a "single patch series".  This output (except for
   this commentary) was created with

   $ git format-patch --interdiff=@{24.hours} -1

 log-tree.c              | 11 +++++------
 t/t4014-format-patch.sh | 36 ++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index e7cd2c491f..7de744911e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -684,8 +684,7 @@ 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);
+		fprintf_ln(opt->diffopt.file, "\n%s", opt->idiff_title);
 		show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,
 			       &opt->diffopt);
 
@@ -704,8 +703,7 @@ 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);
+		fprintf_ln(opt->diffopt.file, "\n%s", opt->rdiff_title);
 		/*
 		 * Pass minimum required diff-options to range-diff; others
 		 * can be added later if deemed desirable.
@@ -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)
@@ -1173,9 +1169,12 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
 	}
 	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
 		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
+	if (shown)
+		show_diff_of_diff(opt);
 	opt->loginfo = NULL;
 	maybe_flush_or_die(opt->diffopt.file, "stdout");
 	opt->diffopt.no_free = no_free;
+
 	diff_free(&opt->diffopt);
 	return shown;
 }
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba85b582c5..de039825a9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2482,13 +2482,37 @@ 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 &&
+		echo "Interdiff:" &&
+		sed -e "s/^/  /" inter
+	} >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'range-diff: solo-patch' '
+	git format-patch --creation-factor=999 \
+		--range-diff=boop~2..boop~1 -1 boop &&
+
+	# 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 range-diff output.
+	{
+		echo &&
+		echo "Range-diff:" &&
+		git range-diff --creation-factor=999 \
+			boop~2..boop~1 boop~1..boop
+	} >expect &&
 	test_cmp expect actual
 '
 

Interdiff:
  diff --git a/log-tree.c b/log-tree.c
  index f28c4d0bb0..7de744911e 100644
  --- a/log-tree.c
  +++ b/log-tree.c
  @@ -684,7 +684,7 @@ 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);
   
  -		fprintf_ln(opt->diffopt.file, "%s", opt->idiff_title);
  +		fprintf_ln(opt->diffopt.file, "\n%s", opt->idiff_title);
   		show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,
   			       &opt->diffopt);
   
  @@ -703,7 +703,7 @@ 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);
   
  -		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
  +		fprintf_ln(opt->diffopt.file, "\n%s", opt->rdiff_title);
   		/*
   		 * Pass minimum required diff-options to range-diff; others
   		 * can be added later if deemed desirable.
  @@ -1169,11 +1169,11 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)
   	}
   	if (opt->track_linear && !opt->linear && opt->reverse_output_stage)
   		fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);
  +	if (shown)
  +		show_diff_of_diff(opt);
   	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 c0c5eccb7c..de039825a9 100755
  --- a/t/t4014-format-patch.sh
  +++ b/t/t4014-format-patch.sh
  @@ -2491,12 +2491,31 @@ test_expect_success 'interdiff: solo-patch' '
   	# fabricate Interdiff output.
   	git diff boop~2 boop >inter &&
   	{
  +		echo &&
   		echo "Interdiff:" &&
   		sed -e "s/^/  /" inter
   	} >expect &&
   	test_cmp expect actual
   '
   
  +test_expect_success 'range-diff: solo-patch' '
  +	git format-patch --creation-factor=999 \
  +		--range-diff=boop~2..boop~1 -1 boop &&
  +
  +	# 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 range-diff output.
  +	{
  +		echo &&
  +		echo "Range-diff:" &&
  +		git range-diff --creation-factor=999 \
  +			boop~2..boop~1 boop~1..boop
  +	} >expect &&
  +	test_cmp expect actual
  +'
  +
   test_expect_success 'format-patch does not respect diff.noprefix' '
   	git -c diff.noprefix format-patch -1 --stdout >actual &&
   	grep "^--- a/blorp" actual
-- 
2.45.1-248-g15a88ae3cc

Re: [PATCH 0/2] give range-diff at the end of single patch output

[Dragan Simic]

On 2024-05-24 00:50, Junio C Hamano wrote:
> 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
> 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.

Hmm...  I think this should be made configurable, with the current
behavior being the default.  Without that, we could easily disrupt
many people's workflows, because the power of "muscle memory" is
often really strong.

If it were just about moving a few lines up or down, making it
configurable wouldn't make much sense, but with moving this large
chunks of text...  It's a different story.

Re: [PATCH 0/2] give range-diff at the end of single patch output

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Hmm...  I think this should be made configurable, with the current
> behavior being the default.  Without that, we could easily disrupt
> many people's workflows, because the power of "muscle memory" is
> often really strong.

I would view this more like "Porcelain layers reserve the right to
change the behaviour to suit human-end-user needs, without having to
complicate the system with extra configuration knobs."

But if other people want to do a follow-up patch to cleanly add such
a configuration, I would not object to it.  The main desire of this
patch is *not* to make the option to have range-diff at the end of a
single patch "series" available, but to make that the default.

Thanks.

Re: [PATCH 0/2] give range-diff at the end of single patch output

[Dragan Simic]

On 2024-05-24 01:25, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> Hmm...  I think this should be made configurable, with the current
>> behavior being the default.  Without that, we could easily disrupt
>> many people's workflows, because the power of "muscle memory" is
>> often really strong.
> 
> I would view this more like "Porcelain layers reserve the right to
> change the behaviour to suit human-end-user needs, without having to
> complicate the system with extra configuration knobs."
> 
> But if other people want to do a follow-up patch to cleanly add such
> a configuration, I would not object to it.  The main desire of this
> patch is *not* to make the option to have range-diff at the end of a
> single patch "series" available, but to make that the default.

I see.  Personally, I find the range diffs placed _after_ the actual
patches more pleasant, but I'm just concerned about other people that
might not like such an arrangement that much.

To me, a range diff is like an additional description of the actual 
patch,
or like a really long footnote, so to me it makes more sense to put it
at the end of the "document".  Sometimes I don't even want or need to 
look
at that footnote, so not needing to scroll down is another plus.

Re: [PATCH 0/2] give range-diff at the end of single patch output

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> To me, a range diff is like an additional description of the
> actual patch, or like a really long footnote, so to me it makes
> more sense to put it at the end of the "document".  Sometimes I
> don't even want or need to look at that footnote, so not needing
> to scroll down is another plus.

That apparently makes the three of us, counting Patrick [*].

Personally, for a single-patch "series", I find that interdiff is
often easier to see than the range diff, but that is a separate
story.

[Reference]
 * https://lore.kernel.org/git/Zk7UsJjhY_FV2z8C@tanuki/