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

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2024, 21:46 UTC
Message-ID: <xmqqo78ukhmk.fsf@gitster.g>
In-Reply-To: <ZlB2g5bTuBFz5m5_@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 10 quoted lines

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

Show 16 quoted lines

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

Show 14 quoted lines

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