Re: [PATCH 2/2] builtin add -p: fix hunk splitting

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 20, 2021, 21:30 UTC
Message-ID: <xmqq4k73ufq7.fsf@gitster.g>
In-Reply-To: <5d5639c2b0474680850b7adbb7c5ec81d124eb50.1640010777.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> To determine whether a hunk can be split a counter is incremented each
> time a context line follows an insertion or deletion. If at the end of
> the hunk the value of this counter is greater than one then the hunk
> can be split into that number of smaller hunks. If the last hunk in a
> file ends with an insertion or deletion then there is no following
> context line and the counter will not be incremented. This case is
> already handled at the end of the loop where counter is incremented if
> the last hunk ended with an insertion or deletion. Unfortunately there
> is no similar check between files (likely because the perl version
> only ever parses one diff at a time).

In other words, the original laid out the code in such a way that such a bug will be impossible, and the rewrite broke it because it rolled both "next file" and "next hunk" into the same loop?

> Fix this by checking if the last
> hunk ended with an insertion or deletion when we see the diff header
> of a new file and extend the existing regression test.

You should be able to explain what end-user visible bug is in a simple single sentence before all of the above.

"The C reimplementation of 'add -p' fails to split a hunk when the hunk ends with addition or deletion without post context line." or something like that.

Show 8 quoted lines

>  		if (starts_with(p, "diff ")) {
> +			if (marker == '-' || marker == '+')
> +				/*
> +				 * Last hunk ended in non-context line (i.e. it
> +				 * appended lines to the file, so there are no
> +				 * trailing context lines).
> +				 */
> +				hunk->splittable_into++;

This looks correct but unsatisfactory. We have the same processing immediately after loop---what is common between them is that this is a process to "conclude" the hunks for the file we have been reading the patch for.

Can we at least make a helper function that identifies what it does clearly by its name, and use it here and after the loop, to clarify what is going on? Then you do not need the 5-line comment there.

		if (starts_with(p, "diff ")) {
+			conclude_file(hunk, marker);

or something like that, perhaps.

Thanks.