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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 19, 2022, 20:01 UTC
Message-ID: <47e7b23a-56a3-b533-63ed-a854503b59ed@gmail.com>
In-Reply-To: <xmqqv8yovlc2.fsf@gitster.g>

Hi Junio

On 12/01/2022 18:51, Junio C Hamano wrote:

Show 47 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I'm not sure I want to go with your extra changes. I've left some
>> comments on them below
>>
>>> @@ -488,12 +499,12 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>>>   	else if (starts_with(p, "@@ ") ||
>>>   		 (hunk == &file_diff->head &&
>>>   		  (skip_prefix(p, "deleted file", &deleted)))) {
>>> -		if (marker == '-' || marker == '+')
>>> -			/*
>>> -			 * Should not happen; previous hunk did not end
>>> -			 * in a context line? Handle it anyway.
>>> -			 */
>>> +			hunk->splittable_into++;
>>> +		/*
>>> +		 * Should not increment "splittable_into";
>>> +		 * previous hunk did not end in a context
>>> +		 * line? Handle it anyway.
>>> +		 */
>>> +		complete_file(marker, &hunk->splittable_into);
>>>    		ALLOC_GROW_BY(file_diff->hunk, file_diff->hunk_nr, 1,
>>>   			   file_diff->hunk_alloc);
>>
>> I deliberately left this alone as I think we should probably make this
>> BUG() out instead of silently accepting an invalid diff.
> 
> As we are reading our own output, I agree that such a data error is
> a BUG().
> 
> In any case, a helper to see if the file ended without post-context
> is one thing, and a helper that specify what happens after we are
> done with a single file, before we move on top the next file or
> after processing the last file, is another thing.  The latter may be
> able to make use of the former, but the latter may want to do more
> than that in the future.
> 
> As complete_file() is about finalizing the processing we have done
> to the current file, it should be used for that purpose, and nothing
> else, I think the hunk I see at
> https://github.com/git/git/commit/c082176f8c5a1fc1c8b2a93991ca28fd63aae73a
> (reproduced below) is simply a nonsense.
> 
> Stepping back a bit, though, is this helper really finalizing the
> current file, or is it finalizing the current hunk?  If it were the
> latter, then its use in the hunk I called "nonsense" above actually
> makes perfect sense.

Even if the helper is finalizing the current hunk then I think that "nonsense" hunk would still wrong as it would be calling finalize_hunk() on _every_ context line in the hunk rather than just being called once to finalize the hunk. We could call the function something like update_splittable() but then we'd need to explain why we were calling that function at the start of a diff and at the end of the loop.

Show 5 quoted lines

> There may not be anything other than finalizing
> the last hunk when we see the end of a file right now, so we may not
> need to add a finalize_file() helper right now, and when we need to
> do something more than finalizing the last hunk, we may need to capture
> the distinction by adding one.

Yes, if you're happy lets leave this series as it is

Best Wishes

Phillip

Show 16 quoted lines

> diff --git i/add-patch.c w/add-patch.c
> index 89ffda32b2..6094290c86 100644
> --- i/add-patch.c
> +++ w/add-patch.c
> @@ -578,8 +578,8 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>   			    (int)(eol - (plain->buf + file_diff->head.start)),
>   			    plain->buf + file_diff->head.start);
>   
> -		if ((marker == '-' || marker == '+') && *p == ' ')
> -			hunk->splittable_into++;
> +		if (*p == ' ')
> +			complete_file(marker, &hunk->splittable_into);
>   		if (marker && *p != '\\')
>   			marker = *p;
>   
>