Re: [PATCH 2/2] apply: handle assertion failure gracefully

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2017, 20:04 UTC
Message-ID: <xmqqmvd7wgc7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <a5626d97-e644-65b5-2fd3-41ce870f85a6@web.de>

René Scharfe <l.s.r@web.de> writes:

Show 22 quoted lines

>> diff --git a/apply.c b/apply.c
>> index cbf7cc7f2..9219d2737 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
>>  	if (!old_name)
>>  		return 0;
>>
>> -	assert(patch->is_new <= 0);
>
> 5c47f4c6 (builtin-apply: accept patch to an empty file) added that
> line. Its intent was to handle diffs that contain an old name even for
> a file that's created.  Citing from its commit message: "When we
> cannot be sure by parsing the patch that it is not a creation patch,
> we shouldn't complain when if there is no such a file."  Why not stop
> complaining also in case we happen to know for sure that it's a
> creation patch? I.e., why not replace the assert() with:
>
> 	if (patch->is_new == 1)
> 		goto is_new;
>
>>  	previous = previous_patch(state, patch, &status);

When the caller does know is_new is true, old_name must be made/left NULL. That is the invariant this assert is checking to catch an error in the calling code.

Errors in the patches fed as its input are caught by "if we do not know if the patch is to add a new path yet, then declare it is, but if we do know the patch is _NOT_ adding a new path, barf if that path is not there" and other checks in this function, and changing the assert to "if already new, then make it a no-op" defeats the whole point of having an assert (and just removing it is even worse).

Thanks.