[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:       Jeff King <peff () peff ! net>
Date:       2023-03-06 8:40:45
Message-ID: ZAWnDUkgO5clf6qu () coredump ! intra ! peff ! net
[Download RAW message or body]

On Sat, Mar 04, 2023 at 01:39:00PM +0100, Marcel Partap wrote:

> If the user has set the diff.noprefix option, he likely will expect
> this display setting to also apply when interactively adding hunks.

I think it's reasonable for the interactive display to respect the
configured preferences here. But unfortunately, it's not quite as simple
as your patch.

> diff --git add-patch.c add-patch.c
> index a86a92e164..520faae9cb 100644

A semi-aside: I note that this patch was also generated with
diff.noprefix. It has to be applied with "git am -p0" (and anybody
receiving it has to know to do that).

The "aside" part is that this is (IMHO) a bug or at least a misfeature
in format-patch. Looks like it has come up a few times recently, too
(though AFAICT it has been this way since the option was added in 2010):

  https://lore.kernel.org/git/xmqqr1auvs7m.fsf@gitster.g/

  https://lore.kernel.org/git/CAAHpriMPdahH2xbrrQbeCJPYpLhr6tuvT6xsG3nACmskKF1v2w@mail.gmail.com/

The not-aside part is that this same problem is important for what your
patch is trying to do. ;)

If we generate the diff with "--no-prefix", then it has to be applied
with "-p0". But your patch touches only the generation side, so it
doesn't work at all:

  $ echo foo >>Makefile
  $ ./git -c diff.noprefix add -p
  diff --git Makefile Makefile
  [...etc...]
  +foo
  (1/1) Stage this hunk [y,n,q,a,d,e,?]? y
  error: git diff header lacks filename information when removing 1 leading pathname component (line 5)
  error: 'git apply' failed

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:

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");
 	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]