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

[Junio C Hamano <gitster@pobox.com>]

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.