Re: [PATCH] add -p: obey diff.noprefix option if set

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 6, 2023, 09:39 UTC
Message-ID: <0ba2f495-892c-3e27-a32c-9f136e86fc26@dunelm.org.uk>
In-Reply-To: <ZAWnDUkgO5clf6qu@coredump.intra.peff.net>

On 06/03/2023 08:40, Jeff King wrote:

Show 15 quoted lines

> On Sat, Mar 04, 2023 at 01:39:00PM +0100, Marcel Partap wrote:
> There are two options, I think.
> 
> One is that we have a similar issue with color. To handle that, we
> generate the diff twice, once with color and once without. We could
> probably do the same thing here, by sticking the "--no-prefix" part with
> the color setup. Though it turns out to be a little tricky to do because
> of the way the code is written, and IIRC there are probably some corner
> cases lurking (e.g., after splitting, I think we'll try to re-colorize
> the diff headers ourselves).
> 
> The second is to just remember that we set noprefix and to add the
> matching "-p0". Unfortunately we have to do so in a few places, but it's
> not _too_ bad (and possibly some refactoring could make it less ugly).
> Something like:

I think that is the better approach. Looking at how we handle diff.algorithm we should maybe add a "noprefix" member to "struct add_i_state" and initialize it in init_add_i_state() (which is in add-interactive.c). That way we're consistent with the existing code and we don't need to keep calling git_config_get_bool() whenever we want the value of diff.noPrefix.

Show 18 quoted lines

> diff --git a/add-patch.c b/add-patch.c
> index 520faae9cba..6e5390621c0 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -1189,13 +1189,16 @@ static int run_apply_check(struct add_p_state *s,
>   			   struct file_diff *file_diff)
>   {
>   	struct child_process cp = CHILD_PROCESS_INIT;
> +	int noprefix;
>   
>   	strbuf_reset(&s->buf);
>   	reassemble_patch(s, file_diff, 1, &s->buf);
>   
>   	setup_child_process(s, &cp,
>   			    "apply", "--check", NULL);
>   	strvec_pushv(&cp.args, s->mode->apply_check_args);
> +	if (!git_config_get_bool("diff.noprefix", &noprefix) && noprefix)
> +		strvec_pushf(&cp.args, "-p1");

I think you meant "-p0" here

Best Wishes

Phillip

Show 24 quoted lines

>   	if (pipe_command(&cp, s->buf.buf, s->buf.len, NULL, 0, NULL, 0))
>   		return error(_("'git apply --cached' failed"));
>   
> @@ -1695,7 +1698,10 @@ static int patch_update_file(struct add_p_state *s,
>   			apply_for_checkout(s, &s->buf,
>   					   s->mode->is_reverse);
>   		else {
> +			int noprefix;
>   			setup_child_process(s, &cp, "apply", NULL);
> +			if (!git_config_get_bool("diff.noprefix", &noprefix) && noprefix)
> +				strvec_pushf(&cp.args, "-p0");
>   			strvec_pushv(&cp.args, s->mode->apply_args);
>   			if (pipe_command(&cp, s->buf.buf, s->buf.len,
>   					 NULL, 0, NULL, 0))
> 
>>   add-patch.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> We'd probably want at least one test using "add -p" with diff.noprefix
> (probably in t3701). That would demonstrate that the feature works, as
> well as protect it from future regressions (the test suite doesn't fail
> even with your broken patch because no test sets noprefix).
> 
> -Peff