Re: [PATCH] apply: tell user location of corrupted patch file

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 5, 2019, 22:51 UTC
Message-ID: <xmqq4l0m7qmv.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <xmqqv9t37fsw.fsf@gitster-ct.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 14 quoted lines

>>  		if (len <= 0) {
>>  			free(fragment);
>> -			return error(_("corrupt patch at line %d"), state->linenr);
>> +			return error(_("corrupt patch at %s:%d"), state->patch_input_file, state->linenr);
>>  		}
>
> Do not forget that you can run "git apply" and feed the patch from
> its standard input, e.g.
>
> 	$ git apply <patchfile
> 	$ git show -R | git apply
>
> Make sure state->patch_input_file is a reasonable string before
> considering this.

I think what the patch does is safe in this case; callsites of apply_patch(), which sets the .patch_input_file field, pass the string "<stdin>", so you'd say

	error: corrupt patch at <stdin>:43

We lost the word "line" in the message, but it would be picked up rather quickly by users that colon + integer is a line number, so I think it is OK.

Show 14 quoted lines

> Also, if you have a mbox file
>
> 	$ cd sub/direc/tory
> 	$ git am -s /var/tmp/mbox
>
> The "git apply" process thatis run inside "git am" would be running
> at the top level of the working tree, so state->patch_input_file may
> say ".git/rebase-apply/patch" (i.e. relative pathname) that is not
> relative to where the end user is in.  I personally do not thinkg it
> matters too much, but some people may complain.
>
> Other than that, looks good.  I am kind-of surprised that there is
> only one place that we report an unusable input with a line number.
> Nicely found.

I still do not know if we have a relative-path problem, how severe it would be if there is, or if it is fixable if we wanted to and how, though.

Thanks.