Better suggestions when git-am(1) fails

> On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote:
> 
> > I had the following error already a few times, when some contributors,
> > for some reason unknown to me, remove the leading path components from
> > the patch.
> 
> The reason is probably that they have set diff.noprefix in their config,
> and git-format-patch respects that. Which is arguably a bug. There's a
> little discussion in this message, along with references to some
> previous discussions:
> 
>   https://lore.kernel.org/git/ZAWnDUkgO5clf6qu@coredump.intra.peff.net/

> We directly manipulate diffopt's a_prefix and b_prefix to set up either
> the default "a/foo" prefix or the "--no-prefix" variant. Although this
> is only a few lines, it's worth pulling these into their own functions.
> That lets us avoid one repetition already in this patch, but will also
> give us a cleaner interface for callers which want to tweak this
> setting.
> 
> Signed-off-by: Jeff King <peff@peff.net>

> ---
>  diff.c | 19 ++++++++++++++-----
>  diff.h |  2 ++
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 469e18aed20..750d1b1a6c3 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3374,6 +3374,17 @@ void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const
>  		options->b_prefix = b;
>  }
>  
> +void diff_set_noprefix(struct diff_options *options)
> +{
> +	options->a_prefix = options->b_prefix = "";
> +}
> +
> +void diff_set_default_prefix(struct diff_options *options)
> +{
> +	options->a_prefix = "a/";
> +	options->b_prefix = "b/";
> +}
> +
>  struct userdiff_driver *get_textconv(struct repository *r,
>  				     struct diff_filespec *one)
>  {
> @@ -4674,10 +4685,9 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
>  		options->flags.ignore_untracked_in_submodules = 1;
>  
>  	if (diff_no_prefix) {
> -		options->a_prefix = options->b_prefix = "";
> +		diff_set_noprefix(options);
>  	} else if (!diff_mnemonic_prefix) {
> -		options->a_prefix = "a/";
> -		options->b_prefix = "b/";
> +		diff_set_default_prefix(options);
>  	}
>  
>  	options->color_moved = diff_color_moved_default;
> @@ -5261,8 +5271,7 @@ static int diff_opt_no_prefix(const struct option *opt,
>  
>  	BUG_ON_OPT_NEG(unset);
>  	BUG_ON_OPT_ARG(optarg);
> -	options->a_prefix = "";
> -	options->b_prefix = "";
> +	diff_set_noprefix(options);
>  	return 0;
>  }
>  
> diff --git a/diff.h b/diff.h
> index 8d770b1d579..2af10bc5851 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -497,6 +497,8 @@ void diff_tree_combined(const struct object_id *oid, const struct oid_array *par
>  void diff_tree_combined_merge(const struct commit *commit, struct rev_info *rev);
>  
>  void diff_set_mnemonic_prefix(struct diff_options *options, const char *a, const char *b);
> +void diff_set_noprefix(struct diff_options *options);
> +void diff_set_default_prefix(struct diff_options *options);
>  
>  int diff_can_quit_early(struct diff_options *);
>  

> You can change the output of prefixes with diff.noprefix and
> diff.mnemonicprefix, but there's no easy way to override them from the
> command-line. We do have "--no-prefix", but there's no way to get back
> to the default prefix. So let's add an option to do that.
> 
> Signed-off-by: Jeff King <peff@peff.net>

> ---
> This isn't strictly necessary for the series, but it seemed like a gap.
> You can always do:
> 
>   git -c diff.noprefix=false -c diff.mnemonicprefix=false ...
> 
> but that's rather a mouthful.
> 
> Note that there isn't a command-line equivalent for mnemonicprefix,
> either. I don't think it's worth adding unless somebody really wants it.
> 
>  Documentation/diff-options.txt |  5 +++++
>  diff.c                         | 14 ++++++++++++++
>  t/t4013-diff-various.sh        | 10 ++++++++++
>  3 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 7d73e976d99..08ab86189a7 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -852,6 +852,11 @@ endif::git-format-patch[]
>  --no-prefix::
>  	Do not show any source or destination prefix.
>  
> +--default-prefix::
> +	Use the default source and destination prefixes ("a/" and "b/").
> +	This is usually the default already, but may be used to override
> +	config such as `diff.noprefix`.
> +
>  --line-prefix=<prefix>::
>  	Prepend an additional prefix to every line of output.
>  
> diff --git a/diff.c b/diff.c
> index 750d1b1a6c3..b322e319ff3 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -5275,6 +5275,17 @@ static int diff_opt_no_prefix(const struct option *opt,
>  	return 0;
>  }
>  
> +static int diff_opt_default_prefix(const struct option *opt,
> +				   const char *optarg, int unset)
> +{
> +	struct diff_options *options = opt->value;
> +
> +	BUG_ON_OPT_NEG(unset);
> +	BUG_ON_OPT_ARG(optarg);
> +	diff_set_default_prefix(options);
> +	return 0;
> +}
> +
>  static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
>  					     const struct option *opt,
>  					     const char *arg, int unset)
> @@ -5564,6 +5575,9 @@ struct option *add_diff_options(const struct option *opts,
>  		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
>  			       N_("do not show any source or destination prefix"),
>  			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
> +		OPT_CALLBACK_F(0, "default-prefix", options, NULL,
> +			       N_("use default prefixes a/ and b/"),
> +			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_default_prefix),
>  		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
>  			      N_("show context between diff hunks up to the specified number of lines"),
>  			      PARSE_OPT_NONEG),
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 0bc69579898..5de1d190759 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -643,9 +643,19 @@ test_expect_success 'diff respects diff.noprefix' '
>  	check_prefix actual file0 file0
>  '
>  
> +test_expect_success 'diff --default-prefix overrides diff.noprefix' '
> +	git -c diff.noprefix diff --default-prefix >actual &&
> +	check_prefix actual a/file0 b/file0
> +'
> +
>  test_expect_success 'diff respects diff.mnemonicprefix' '
>  	git -c diff.mnemonicprefix diff >actual &&
>  	check_prefix actual i/file0 w/file0
>  '
>  
> +test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' '
> +	git -c diff.mnemonicprefix diff --default-prefix >actual &&
> +	check_prefix actual a/file0 b/file0
> +'
> +
>  test_done

> This isn't strictly necessary for the series, but it seemed like a gap.
> You can always do:
>
>   git -c diff.noprefix=false -c diff.mnemonicprefix=false ...
>
> but that's rather a mouthful.

> +static int diff_opt_default_prefix(const struct option *opt,
> +				   const char *optarg, int unset)
> +{
> +	struct diff_options *options = opt->value;
> +
> +	BUG_ON_OPT_NEG(unset);
> +	BUG_ON_OPT_ARG(optarg);

> +	diff_set_default_prefix(options);
> +	return 0;
> +}
> +
>  static enum parse_opt_result diff_opt_output(struct parse_opt_ctx_t *ctx,
>  					     const struct option *opt,
>  					     const char *arg, int unset)
> @@ -5564,6 +5575,9 @@ struct option *add_diff_options(const struct option *opts,
>  		OPT_CALLBACK_F(0, "no-prefix", options, NULL,
>  			       N_("do not show any source or destination prefix"),
>  			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_no_prefix),
> +		OPT_CALLBACK_F(0, "default-prefix", options, NULL,
> +			       N_("use default prefixes a/ and b/"),
> +			       PARSE_OPT_NONEG | PARSE_OPT_NOARG, diff_opt_default_prefix),
>  		OPT_INTEGER_F(0, "inter-hunk-context", &options->interhunkcontext,
>  			      N_("show context between diff hunks up to the specified number of lines"),
>  			      PARSE_OPT_NONEG),

> Jeff King <peff@peff.net> writes:
> 
> > This isn't strictly necessary for the series, but it seemed like a gap.
> > You can always do:
> >
> >   git -c diff.noprefix=false -c diff.mnemonicprefix=false ...
> >
> > but that's rather a mouthful.
> 
> or "git diff --src-prefix=a/ --dst-prefix=b/"

> > +static int diff_opt_default_prefix(const struct option *opt,
> > +				   const char *optarg, int unset)
> > +{
> > +	struct diff_options *options = opt->value;
> > +
> > +	BUG_ON_OPT_NEG(unset);
> > +	BUG_ON_OPT_ARG(optarg);
> 
> OK.  It is a bit unsatisfactory that we already said this does not
> take negative form or any argument in the option[] array, and still
> have to do this, but that is completely outside the topic of this
> series.

> While it is not _quite_ the same thing to say "use prefixes a/ and b/"
> versus "countermand any config and use the default", it is close enough
> that I am tempted to say this patch should be scrapped. I mostly just
> wanted to have a way to counter format.noprefix, if we are going to
> endorse it as a concept (whether by adding it, or saying "no, respecting
> diff.noprefix is not a bug").
>
> (If we do scrap it, I'd probably fold the extra tests into the previous
> commit, but using --src-prefix, etc).

> Jeff King <peff@peff.net> writes:
> 
> > While it is not _quite_ the same thing to say "use prefixes a/ and b/"
> > versus "countermand any config and use the default", it is close enough
> > that I am tempted to say this patch should be scrapped. I mostly just
> > wanted to have a way to counter format.noprefix, if we are going to
> > endorse it as a concept (whether by adding it, or saying "no, respecting
> > diff.noprefix is not a bug").
> >
> > (If we do scrap it, I'd probably fold the extra tests into the previous
> > commit, but using --src-prefix, etc).
> 
> I would very much like to keep this one; if we can find a shorter
> name that would be even sweeter.

> Sure, but how do we send that message? I guess if we leave diff.noprefix
> as it is and add a new format.patch (which preempts diff.noprefix only
> for format-patch), then people will still accidentally send patches
> without prefixes, but at least there is an "out" for the maintainer
> receiving them to say "don't do that; please set format.patch".

> By the way, we might also want something like this:
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index dd31d5ab91e..5b7b908b66b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -661,7 +661,7 @@ static int run_am(struct rebase_options *opts)
>  	format_patch.git_cmd = 1;
>  	strvec_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
>  		     "--full-index", "--cherry-pick", "--right-only",
> -		     "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
> +		     "--default-prefix", "--no-renames",
>  		     "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
>  		     "--no-base", NULL);
>  	if (opts->git_format_patch_opt.len)
>
> which uses --src-prefix to (you may have guessed it!) counteract
> diff.noprefix in the user's config. (It would still be necessary even
> with my series because the user might have set format.patch).

> > -		     "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
> > +		     "--default-prefix", "--no-renames",
> [...]
> Of course, nothing breaks without the above one-liner so it is
> somewhere between a "Meh" and a "clean-up we should do before we
> forget".

> The output of format-patch respects diff.noprefix, but this usually ends
> up being a hassle for people receiving the patch, as they have to
> manually specify "-p0" in order to apply it.
> 
> I don't think there was any specific intention for it to behave this
> way. The noprefix option is handled by git_diff_ui_config(), and
> format-patch exists in a gray area between plumbing and porcelain.
> People do look at the output, and we'd expect it to colorize things,
> respect their choice of algorithm, and so on. But this particular option
> creates problems for the receiver (in theory so does diff.mnemonicprefix,
> but since we are always formatting commits, the mnemonic prefixes will
> always be "a/" and "b/").
> 
> So let's disable it. The slight downsides are:
> 
>   - people who have set diff.noprefix presumably like to see their
>     patches without prefixes. If they use format-patch to review their
>     series, they'll see prefixes. On the other hand, it is probably a
>     good idea for them to look at what will actually get sent out.
> 
>     We could try to play games here with "is stdout a tty", as we do for
>     color. But that's not a completely reliable signal, and it's
>     probably not worth the trouble. If you want to see the patch with
>     the usual bells and whistles, then you are better off using "git
>     log" or "git show".
> 
>   - if a project really does have a workflow that likes prefix-less
>     patches, and the receiver is prepared to use "-p0", then the sender
>     now has to manually say "--no-prefix" for each format-patch
>     invocation. That doesn't seem _too_ terrible given that the receiver
>     has to manually say "-p0" for each git-am invocation.
> 
> Signed-off-by: Jeff King <peff@peff.net>

> ---
>  builtin/log.c           | 9 +++++++++
>  t/t4014-format-patch.sh | 5 +++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index a70fba198f9..eaf511aab86 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1085,6 +1085,15 @@ static int git_format_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
> +	/*
> +	 * ignore some porcelain config which would otherwise be parsed by
> +	 * git_diff_ui_config(), via git_log_config(); we can't just avoid
> +	 * diff_ui_config completely, because we do care about some ui options
> +	 * like color.
> +	 */
> +	if (!strcmp(var, "diff.noprefix"))
> +		return 0;
> +
>  	return git_log_config(var, value, cb);
>  }
>  
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index f3313b8c58f..f5a41fd47ed 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -2386,4 +2386,9 @@ test_expect_success 'interdiff: solo-patch' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'format-patch does not respect diff.noprefix' '
> +	git -c diff.noprefix format-patch -1 --stdout >actual &&
> +	grep "^--- a/blorp" actual
> +'
> +
>  test_done

>   - if a project really does have a workflow that likes prefix-less
>     patches, and the receiver is prepared to use "-p0", then the sender
>     now has to manually say "--no-prefix" for each format-patch
>     invocation. That doesn't seem _too_ terrible given that the receiver
>     has to manually say "-p0" for each git-am invocation.

> Jeff King <peff@peff.net> writes:
> 
> >   - if a project really does have a workflow that likes prefix-less
> >     patches, and the receiver is prepared to use "-p0", then the sender
> >     now has to manually say "--no-prefix" for each format-patch
> >     invocation. That doesn't seem _too_ terrible given that the receiver
> >     has to manually say "-p0" for each git-am invocation.
> 
> It does seem very terrible if any existing projects do use the
> workflow, as their receivers need to change their workflow, though.

> A more backwards-compatible way of doing the transition would be to have
> format.noprefix default to diff.noprefix when it's not set. But that
> doesn't really help the "accidental" problem; people would have to
> manually set format.noprefix=false. And it's unlikely that anybody
> really wants format.noprefix=true in the first place. I'm adding it here
> mostly as an escape hatch, not because anybody has expressed any
> interest in it.

> But that is the other extreme.
> 
> I wonder if the consumer side should be made configurable for
> completeness, though.
> 
> Here is how "apply.pValue" configuration variable would look like.
> Projects whose members want diff.noprefix set can standardise on
> using that and then receiving end configured to match with this.

> On Wed, Mar 08, 2023 at 10:17:11PM -0500, Jeff King wrote:
> 
>> On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote:
>>
>>> I had the following error already a few times, when some contributors,
>>> for some reason unknown to me, remove the leading path components from
>>> the patch.
>>
>> The reason is probably that they have set diff.noprefix in their config,
>> and git-format-patch respects that. Which is arguably a bug. There's a
>> little discussion in this message, along with references to some
>> previous discussions:
>>
>>   https://lore.kernel.org/git/ZAWnDUkgO5clf6qu@coredump.intra.peff.net/
> 
> So here's a patch series which I think should help with the sending
> side. Most of it is just filling in gaps in the code and tests for
> current features. Patch 4 is the actual change. Patch 5 adds an
> equivalent option just for format-patch. I'm not convinced anybody
> really wants it (which is why I split it out), but it's probably worth
> doing just in case.

> 
>   [1/5]: diff: factor out src/dst prefix setup
>   [2/5]: t4013: add tests for diff prefix options
>   [3/5]: diff: add --default-prefix option
>   [4/5]: format-patch: do not respect diff.noprefix
>   [5/5]: format-patch: add format.noprefix option

> 
>  Documentation/config/format.txt |  7 ++++++
>  Documentation/diff-options.txt  |  5 ++++
>  builtin/log.c                   | 17 +++++++++++++
>  diff.c                          | 33 ++++++++++++++++++++++----
>  diff.h                          |  2 ++
>  t/t4013-diff-various.sh         | 42 +++++++++++++++++++++++++++++++++
>  t/t4014-format-patch.sh         | 16 +++++++++++++
>  7 files changed, 117 insertions(+), 5 deletions(-)
> 
> -Peff

> So here's a patch series which I think should help with the sending
> side. Most of it is just filling in gaps in the code and tests for
> current features. Patch 4 is the actual change. Patch 5 adds an
> equivalent option just for format-patch. I'm not convinced anybody
> really wants it (which is why I split it out), but it's probably worth
> doing just in case.
>
>   [1/5]: diff: factor out src/dst prefix setup
>   [2/5]: t4013: add tests for diff prefix options
>   [3/5]: diff: add --default-prefix option
>   [4/5]: format-patch: do not respect diff.noprefix
>   [5/5]: format-patch: add format.noprefix option

> Jeff King <peff@peff.net> writes:
> 
> > So here's a patch series which I think should help with the sending
> > side. Most of it is just filling in gaps in the code and tests for
> > current features. Patch 4 is the actual change. Patch 5 adds an
> > equivalent option just for format-patch. I'm not convinced anybody
> > really wants it (which is why I split it out), but it's probably worth
> > doing just in case.
> >
> >   [1/5]: diff: factor out src/dst prefix setup
> >   [2/5]: t4013: add tests for diff prefix options
> >   [3/5]: diff: add --default-prefix option
> >   [4/5]: format-patch: do not respect diff.noprefix
> >   [5/5]: format-patch: add format.noprefix option
> 
> I've reviewed these five changes, and while I am not 100% sold to
> the idea that we should force our -p1 worldview to those who choose
> to use diff.noprefix for whatever reason, I think these patches
> describe what they want to do and implement it in a very readable
> way.
> 
> Thanks.  Queued.

> On Wed, Mar 08, 2023 at 09:15:53PM +0100, Alejandro Colomar wrote:
>
>> I had the following error already a few times, when some contributors,
>> for some reason unknown to me, remove the leading path components from
>> the patch.
>
> The reason is probably that they have set diff.noprefix in their config,
> and git-format-patch respects that. Which is arguably a bug.

> > The reason is probably that they have set diff.noprefix in their config,
> > and git-format-patch respects that. Which is arguably a bug.
> 
> FWIW, I've always considered it a feature to help projects that
> prefer their patches in -p0 form.  Of course, Git optimized itself
> for the usecase we consider the optimum, i.e. using a/ and b/ prefix
> on the diff generation side, while stripping them with -p1 on the
> applying side.
> 
> I wonder apply.plevel or am.plevel would be a good way to help them
> further?

> I am not sure making format-patch _ignore_ diff.src/dst_prefix is a
> good approach.  If we were wiser, we may not have introduced the
> diff.noprefix option, made sure diff.src/dstprefix to be always a
> single level, and kept -p<n> on the application side as an escape
> hatch only to deal with non-Git generated patches.  The opportunity
> to simplify the world that way however we missed 15 years ago X-<.

>   2. It feels weird that a maintainer who isn't using Git would expect a
>      lot of contributions from folks who are. And even weirder, that
>      they would insist that all of the folks sending patches set
>      diff.noprefix.
>
> So I won't say it's not possible (especially in some closed community).
> But I'm skeptical.

> Yeah, I am as always a little concerned that one person's fix is another
> one's regression. But it really just seems to that on balance people set
> diff.noprefix with no thought at all to how it would affect format-patch
> (in fact, I'd guess 99% of Git users do not use format-patch at all).
> And then they are surprised (or worse, the receiver is surprised) when
> it doesn't work.

> Jeff King <peff@peff.net> writes:
> 
> >   1. I feel like "-p1" was pretty standard even before Git. You'd
> >      extract two copies of the tarball, one into "foo-1.2.3" and one
> >      into "foo-1.2.3.orig", and then "diff -Nru" between them to send a
> >      patch.
> 
> I would too, but then we wouldn't have accepted the request to add
> .noprefix configuration; I do not recall where it came from.

> On Fri, Mar 10, 2023 at 08:28:55AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >   1. I feel like "-p1" was pretty standard even before Git. You'd
>> >      extract two copies of the tarball, one into "foo-1.2.3" and one
>> >      into "foo-1.2.3.orig", and then "diff -Nru" between them to send a
>> >      patch.
>> 
>> I would too, but then we wouldn't have accepted the request to add
>> .noprefix configuration; I do not recall where it came from.
>
> I always thought it was an aesthetic thing for humans viewing diffs (and
> likewise mnemonicprefix). The original thread doesn't give much
> motivation, though:
>
>   https://lore.kernel.org/git/1272852221-14927-1-git-send-email-eli@cloudera.com/