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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]