add -p: obey diff.noprefix option if set

Commit Message

Marcel Partap March 4, 2023, 12:39 p.m. UTC

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

Signed-off-by: Marcel Partap <[e-mail]>
---
 add-patch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.38.1

Comments

Jeff King March 6, 2023, 8:40 a.m. UTC | #1

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/[e-mail]/

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

Phillip Wood March 6, 2023, 9:39 a.m. UTC | #2

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

Jeff King March 6, 2023, 10:31 a.m. UTC | #3

On Mon, Mar 06, 2023 at 09:39:07AM +0000, Phillip Wood wrote:

> > 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.

Yeah, that was exactly the kind of refactoring I had in mind (but I
didn't work on it, even as a "maybe something like this" patch).

I agree it's the better approach.

> >   	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

Whoops, yes.

-Peff

Patch

diff --git add-patch.c add-patch.c
index a86a92e164..520faae9cb 100644
--- add-patch.c
+++ add-patch.c
@@ -1,4 +1,5 @@ 
 #include "cache.h"
+#include "config.h"
 #include "add-interactive.h"
 #include "strbuf.h"
 #include "run-command.h"
@@ -404,11 +405,13 @@  static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
 	size_t file_diff_alloc = 0, i, color_arg_index;
 	struct file_diff *file_diff = NULL;
 	struct hunk *hunk = NULL;
-	int res;
+	int res, noprefix;

 	strvec_pushv(&args, s->mode->diff_cmd);
 	if (diff_algorithm)
 		strvec_pushf(&args, "--diff-algorithm=%s", diff_algorithm);
+	if (!git_config_get_bool("diff.noprefix", &noprefix) && noprefix)
+		strvec_pushf(&args, "--no-prefix");
 	if (s->revision) {
 		struct object_id oid;
 		strvec_push(&args,