[prev in list] [next in list] [prev in thread] [next in thread]
List: git
Subject: Re: [PATCH] add -p: obey diff.noprefix option if set
From: Phillip Wood <phillip.wood123 () gmail ! com>
Date: 2023-03-06 9:39:07
Message-ID: 0ba2f495-892c-3e27-a32c-9f136e86fc26 () dunelm ! org ! uk
[Download RAW message or body]
On 06/03/2023 08:40, Jeff King wrote:
> 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.
> 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
> 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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic