Better suggestions when git-am(1) fails

Better suggestions when git-am(1) fails

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2358 bytes --]

Hi,

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.  Now I know that the fix is to use -p0, but the first times
it wasn't obvious.  And still I forget about -p0 sometimes and it's
hard to find in the manual pages.  I think it would be good to suggest
using it when such an error appears.


$ git am -s patches/\[PATCH\ 1_2\]\ CONTRIBUTING\:\ Fix\ typo\,\ there\ is\ one\ active\ maintainer\ -\ Rodrigo\ Campos\ \<rodrigo@sdfg.com.ar\>\ -\ 2023-03-08\ 1622.eml
Applying: CONTRIBUTING: Fix typo, there is one active maintainer
error: git diff header lacks filename information when removing 1 leading pathname component (line 9)
Patch failed at 0001 CONTRIBUTING: Fix typo, there is one active maintainer
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
alx@asus5775:~/src/linux/man-pages/man-pages/main$ man git-am
alx@asus5775:~/src/linux/man-pages/man-pages/main$ git am --show-current-patch=diff
---
 CONTRIBUTING | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git CONTRIBUTING CONTRIBUTING
index 3b4408108..3bb671eca 100644
--- CONTRIBUTING
+++ CONTRIBUTING
@@ -8,7 +8,7 @@ Description
    Mailing list
        The main discussions regarding development of the project, patches,
        bugs, news, doubts, etc. happen on the mailing list.  To send an email
-       to the project, send it to both maintainers and CC the mailing list:
+       to the project, send it to Alejandro and CC the mailing list:
 
            To: Alejandro Colomar <alx@kernel.org>
            Cc: <linux-man@vger.kernel.org>
-- 
2.39.2


git(1) could recommend using `-p` to sort this out.  It could go
further and check which level would be needed for the patch to apply,
but at the very least it could tell the user which option it wants
to look for in the documentation.

Does this make sense to you?  I usually find git-am error messages
quite uninformative.


Cheers,

Alex


-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Better suggestions when git-am(1) fails

[Jeff King]

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/

> Now I know that the fix is to use -p0, but the first times it wasn't
> obvious.  And still I forget about -p0 sometimes and it's hard to find
> in the manual pages.  I think it would be good to suggest using it
> when such an error appears.

I agree it may be reasonable to have "git am" be more helpful on the
receiving side. Hopefully if format-patch is changed then you wouldn't
see the situation as often, but it could still happen.

-Peff

Re: Better suggestions when git-am(1) fails

[Jeff King]

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

[PATCH 1/5] diff: factor out src/dst prefix setup

[Jeff King]

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 *);
 
-- 
2.40.0.rc2.537.g928a61c97db

[PATCH 2/5] t4013: add tests for diff prefix options

[Jeff King]

We don't have any specific test coverage of diff's various prefix
options. We do incidentally invoke them in a few places, but it's worth
having a more thorough set of tests that covers all of the effects we
expect to see, and that the options kick in at the appropriate times.

This will be especially useful as the next patch adds more options.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t4013-diff-various.sh | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dfcf3a0aaae..0bc69579898 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -616,4 +616,36 @@ test_expect_success 'diff -I<regex>: detect malformed regex' '
 	test_i18ngrep "invalid regex given to -I: " error
 '
 
+# check_prefix <patch> <src> <dst>
+# check only lines with paths to avoid dependency on exact oid/contents
+check_prefix () {
+	grep -E '^(diff|---|\+\+\+) ' "$1" >actual.paths &&
+	cat >expect <<-EOF &&
+	diff --git $2 $3
+	--- $2
+	+++ $3
+	EOF
+	test_cmp expect actual.paths
+}
+
+test_expect_success 'diff-files does not respect diff.noprefix' '
+	git -c diff.noprefix diff-files -p >actual &&
+	check_prefix actual a/file0 b/file0
+'
+
+test_expect_success 'diff-files respects --no-prefix' '
+	git diff-files -p --no-prefix >actual &&
+	check_prefix actual file0 file0
+'
+
+test_expect_success 'diff respects diff.noprefix' '
+	git -c diff.noprefix diff >actual &&
+	check_prefix actual file0 file0
+'
+
+test_expect_success 'diff respects diff.mnemonicprefix' '
+	git -c diff.mnemonicprefix diff >actual &&
+	check_prefix actual i/file0 w/file0
+'
+
 test_done
-- 
2.40.0.rc2.537.g928a61c97db

[PATCH 3/5] diff: add --default-prefix option

[Jeff King]

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
-- 
2.40.0.rc2.537.g928a61c97db

[PATCH 4/5] format-patch: do not respect diff.noprefix

[Jeff King]

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
-- 
2.40.0.rc2.537.g928a61c97db

[PATCH 5/5] format-patch: add format.noprefix option

[Jeff King]

The previous commit dropped support for diff.noprefix in format-patch.
While this will do the right thing in most cases (where sending patches
without a prefix was an accidental side effect of the sender preferring
to see their local patches without prefixes), it left no good option for
a project or workflow where you really do want to send patches without
prefixes. You'd be stuck using "--no-prefix" for every invocation.

So let's add a config option specific to format-patch that enables this
behavior. That gives people who have such a workflow a way to get what
they want, but makes it hard to accidentally trigger it.

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.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/config/format.txt |  7 +++++++
 builtin/log.c                   |  8 ++++++++
 t/t4014-format-patch.sh         | 11 +++++++++++
 3 files changed, 26 insertions(+)

diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index 73678d88a1d..8cf6f00d936 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -144,3 +144,10 @@ will only show notes from `refs/notes/bar`.
 format.mboxrd::
 	A boolean value which enables the robust "mboxrd" format when
 	`--stdout` is in use to escape "^>+From " lines.
+
+format.noprefix::
+	If set, do not show any source or destination prefix in patches.
+	This is equivalent to the `diff.noprefix` option used by `git
+	diff` (but which is not respected by `format-patch`). Note that
+	by setting this, the receiver of any patches you generate will
+	have to apply them using the `-p0` option.
diff --git a/builtin/log.c b/builtin/log.c
index eaf511aab86..b1f59062f40 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -56,6 +56,7 @@ static int stdout_mboxrd;
 static const char *fmt_patch_subject_prefix = "PATCH";
 static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
 static const char *fmt_pretty;
+static int format_no_prefix;
 
 static const char * const builtin_log_usage[] = {
 	N_("git log [<options>] [<revision-range>] [[--] <path>...]"),
@@ -1084,6 +1085,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		stdout_mboxrd = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.noprefix")) {
+		format_no_prefix = 1;
+		return 0;
+	}
 
 	/*
 	 * ignore some porcelain config which would otherwise be parsed by
@@ -2002,6 +2007,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
 
+	if (format_no_prefix)
+		diff_set_noprefix(&rev.diffopt);
+
 	if (default_attach) {
 		rev.mime_boundary = default_attach;
 		rev.no_inline = 1;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index f5a41fd47ed..2711fd09ca0 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -2391,4 +2391,15 @@ test_expect_success 'format-patch does not respect diff.noprefix' '
 	grep "^--- a/blorp" actual
 '
 
+test_expect_success 'format-patch respects format.noprefix' '
+	git -c format.noprefix format-patch -1 --stdout >actual &&
+	grep "^--- blorp" actual
+'
+
+test_expect_success 'format-patch --default-prefix overrides format.noprefix' '
+	git -c format.noprefix \
+		format-patch -1 --default-prefix --stdout >actual &&
+	grep "^--- a/blorp" actual
+'
+
 test_done
-- 
2.40.0.rc2.537.g928a61c97db

Re: [PATCH 1/5] diff: factor out src/dst prefix setup

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2689 bytes --]



On 3/9/23 07:07, Jeff King wrote:
> 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>

Acked-by: Alejandro Colomar <alx@kernel.org>

> ---
>  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 *);
>  

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/5] diff: add --default-prefix option

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 3862 bytes --]



On 3/9/23 07:09, Jeff King wrote:
> 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>

Acked-by: Alejandro Colomar <alx@kernel.org>

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

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/5] format-patch: do not respect diff.noprefix

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 3235 bytes --]



On 3/9/23 07:11, Jeff King wrote:
> 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>

Acked-by: Alejandro Colomar <alx@kernel.org>

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

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Better suggestions when git-am(1) fails

[Alejandro Colomar]

[-- Attachment #1.1: Type: text/plain, Size: 2221 bytes --]

Hi Jeff,

On 3/9/23 07:06, Jeff King wrote:
> 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.

Thanks for the rapid patch set :)

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

1, 3, and 4 LGTM.  I'm not used to your tests, so can't really check
what 2 does without further reading, and I'm not sure 5 is useful.

BTW, I'll probably report a few more things I don't like from
git-am(1)'s error reports, whenever I find them again.  ;)

Cheers,

Alex

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

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Better suggestions when git-am(1) fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Re: [PATCH 3/5] diff: add --default-prefix option

[Junio C Hamano]

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

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

I don't either.  We already have src-prefix and dst-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`.

OK.

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

> +	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),

Thanks.

Re: [PATCH 4/5] format-patch: do not respect diff.noprefix

[Junio C Hamano]

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.

But we can declare that we do not care about such projects that do
not honor our -p1 worldview, and I have no objection to this change
if we can have list consensus for us to go in that direction.

Colored patches, by the way, cannot be applied, so perhaps we should
disable ui_config altogether, on the other hand?  I dunno.

Thanks, queued.

Re: [PATCH 5/5] format-patch: add format.noprefix option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I tend to agree that this is closing the barn door after the horse
escaped for projects who did use diff.noprefix because it is their
preference to exchange prefix-free patches.

The other direction we could go is to tie the default p-value
specified centrally for both producer and consumer.  If the project
wants no-prefix patches so much that the contributors use it in
their own diff by setting diff.noprefix in their configuration, the
project would be perfectly happy if format-patch sent prefix-less
patches honoring the configuration, and if apply took prefix-less
patches honoring the configuration, all honoring diff.noprefix.

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.

---
 apply.c       | 5 +++++
 cache.h       | 1 +
 environment.c | 1 +
 3 files changed, 7 insertions(+)

diff --git c/apply.c w/apply.c
index 5cc5479c9c..20645fc9af 100644
--- c/apply.c
+++ w/apply.c
@@ -33,6 +33,7 @@ static void git_apply_config(void)
 {
 	git_config_get_string("apply.whitespace", &apply_default_whitespace);
 	git_config_get_string("apply.ignorewhitespace", &apply_default_ignorewhitespace);
+	git_config_get_int("apply.pValue", &apply_default_p_value);
 	git_config(git_xmerge_config, NULL);
 }
 
@@ -112,6 +113,10 @@ int init_apply_state(struct apply_state *state,
 		return -1;
 	if (apply_default_ignorewhitespace && parse_ignorewhitespace_option(state, apply_default_ignorewhitespace))
 		return -1;
+	if (0 <= apply_default_p_value) {
+		state->p_value = apply_default_p_value;
+		state->p_value_known = 1;
+	}
 	return 0;
 }
 
diff --git c/cache.h w/cache.h
index 12789903e8..05efa9dc52 100644
--- c/cache.h
+++ w/cache.h
@@ -967,6 +967,7 @@ extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
+extern int apply_default_p_value;
 extern char *apply_default_whitespace;
 extern char *apply_default_ignorewhitespace;
 extern const char *git_attributes_file;
diff --git c/environment.c w/environment.c
index 1ee3686fd8..b82ad370b4 100644
--- c/environment.c
+++ w/environment.c
@@ -38,6 +38,7 @@ const char *git_commit_encoding;
 const char *git_log_output_encoding;
 char *apply_default_whitespace;
 char *apply_default_ignorewhitespace;
+int apply_default_p_value = -1; /* unspecified */
 const char *git_attributes_file;
 const char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;

Re: Better suggestions when git-am(1) fails

[Junio C Hamano]

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.

Re: Better suggestions when git-am(1) fails

[Jeff King]

On Thu, Mar 09, 2023 at 08:22:00AM -0800, Junio C Hamano wrote:

> > 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 doubt they would help, because they imply a constant project workflow.
We have seen several reports of "sometimes I get a patch without a
prefix, and it doesn't apply. What's going on?". But I don't think
anybody asked for "my project doesn't use prefixes, and I am tired of
typing -p0".

The more interesting case to me is that the receiver _isn't_ using Git.
They are using "patch" or similar, and they expect senders to send them
patches without prefixes. And there, diff.noprefix is doing what they
want. But I have to wonder if these hypothetical maintainers exist:

  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.

  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.

All that said, if "apply" and "am" could automatically figure out and
handle "-p0" patches, that would be a useful way to help people. I'm
just hesitant because it probably involves some heuristics. E.g., we get
"foo/bar", realize that "bar" doesn't exist, but "foo/bar" does. Except
that fails if a project does have "bar". And so on.

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

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.

-Peff

Re: [PATCH 3/5] diff: add --default-prefix option

[Jeff King]

On Thu, Mar 09, 2023 at 08:31:37AM -0800, Junio C Hamano wrote:

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

Doh. How did I write this whole patch series without remembering the
existence of those options?

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

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

We don't strictly have to do it. It's a cross-check that the correct
flags were set in the options struct, and serves as documentation both
for the human and the compiler (via -Wunused-parameter) that yes, it
really is correct to take "unset" and not look at it. We could just as
easily mark unset with "UNUSED", but I consider the extra run-time check
a bonus.

I do admit that in a one-off callback like this, it is not accomplishing
much. It's much more useful for generic ones like parse_opt_commit(),
that may be triggered from many places. I do wish there was a better way
to make sure they matched at compile-time, but I can't think of one.

-Peff

Re: [PATCH 4/5] format-patch: do not respect diff.noprefix

[Jeff King]

On Thu, Mar 09, 2023 at 08:41:29AM -0800, Junio C Hamano wrote:

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

I think the escape hatch there is patch 5, where the sender just sets
the new variable to say "no, really, I actually want to send patches
without a prefix".

I had originally thought to squash them together to help explain that
better, but I wasn't 100% sure we'd want format.noprefix.

> But we can declare that we do not care about such projects that do
> not honor our -p1 worldview, and I have no objection to this change
> if we can have list consensus for us to go in that direction.

Yeah, I would very much like to hear from others on the list, especially
anybody who does have a "-p0" workflow.

> Colored patches, by the way, cannot be applied, so perhaps we should
> disable ui_config altogether, on the other hand?  I dunno.

Yeah, color is a bit weird there. We auto-disable it when the patch
isn't going to stdout or a pager, so it's mostly a non-issue. I think
more interesting cases are ones like diff.algorithm, diff.context, etc,
where they don't break the diff, but we don't quite consider them
vanilla enough for plumbing.

I do wonder about diff.relative, which may or may not cause confusion on
the receiving end, depending on what you're trying to achieve (are you
sending a patch for somebody else's git repo, or did you make a git repo
yourself and want to send a diff of some subset).

Also diff.submodule, but the implications of submodule-via-format-patch
are too scary for me to even contemplate. ;)

-Peff

Re: [PATCH 5/5] format-patch: add format.noprefix option

[Jeff King]

On Thu, Mar 09, 2023 at 09:00:01AM -0800, Junio C Hamano wrote:

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

I don't mind this at all for the sake of completeness, and your patch
looks reasonable to me. I mostly just wouldn't bother if there's not a
demonstrated need (and again, this is something people could be asking
for _now_, because of the way diff.noprefix works, and nobody has done
so).

-Peff

Re: Better suggestions when git-am(1) fails

[Jeff King]

On Thu, Mar 09, 2023 at 01:53:55PM -0800, Junio C Hamano wrote:

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

Thanks for looking at them. Let's see if we get any other comments on
the direction, and then I may re-roll. Even if we don't do 4 or 5, I
think the extra tests are worth adding. Either way I'd probably drop 3
(in favor of --src-prefix) and squash its tests into 2. Patch 1 isn't
worthwhile if we don't do 3-5, since we wouldn't be adding any new
callers of the helpers.

If we do proceed, I'd suggest trying to cook in 'next' for a long time
to get comment. Though I think both you and I are pessimistic that we
get a wide variety of user testing that way.

-Peff

Re: Better suggestions when git-am(1) fails

[Junio C Hamano]

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.

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

The scenario I would find more likely is a project established long
before we were popular wants to keep using -p0 even after switching
to use Git.

> All that said, if "apply" and "am" could automatically figure out
> and handle "-p0" patches, that would be a useful way to help
> people.  I'm just hesitant because it probably involves some heuristics.

I am not all that interested in that direction, for exactly the same
reason as I are heditant. Such a tool that outsmarts users will
eventually bite them.

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

For these 99% users, if format-patch paid attention to their
diff.noprefix and used -p0, the world would become even more
interesting place.  I am not sure this particular cure is an
overall win.  And as you mentioned elsewhere, a change that is
deliberately designed to be breaking like this does not become
much safer by cooking in 'next', which is another sad thing.

Re: [PATCH 3/5] diff: add --default-prefix option

[Junio C Hamano]

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.

I am wondering if we can keep the current behaviour instead and send
a message: "if you do not want your everyday diff not to have
prefixes, fine, go set diff.noprefix, but if you do not like that
format-patch also gives a no-prefix patches with that configuration,
or at times you may want your 'git show' to show the standard
prefix, you can countermand your diff.noprefix configuration".

Re: Better suggestions when git-am(1) fails

[Jeff King]

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/

That is not really important as what the option has grown to be used
for, of course, but it's another data point (or lack thereof in this
case).

-Peff

Re: [PATCH 3/5] diff: add --default-prefix option

[Jeff King]

On Fri, Mar 10, 2023 at 09:04:21AM -0800, Junio C Hamano wrote:

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

OK. I don't mind keeping it, if you think it's useful (and certainly it
doesn't hurt). I couldn't come up with a better name, but suggestions
are welcome.

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

> I am wondering if we can keep the current behaviour instead and send
> a message: "if you do not want your everyday diff not to have
> prefixes, fine, go set diff.noprefix, but if you do not like that
> format-patch also gives a no-prefix patches with that configuration,
> or at times you may want your 'git show' to show the standard
> prefix, you can countermand your diff.noprefix configuration".

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

I was hoping to avoid having the accident happen in the first place, but
if we're not willing to change how diff.noprefix works now, then I think
that's our runner-up.

It would be pretty easy to rework the series (drop patch 4, and tweak
patch 5 to be a yes/no/unset tri-state, plus a new test to check the
fallback behavior).

-Peff

Re: Better suggestions when git-am(1) fails

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Interesting.

Comparison with mnemonicprefix is a bit unfair, as it does not break
other tools, though ;-).

Re: [PATCH 3/5] diff: add --default-prefix option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I actually was hoping that it would be enough if the message were
"please unset diff.noprefix---in this project the convention is
to use -p1 patches, so get used to seeing a/ and b/ prefixes".

Even if a project wants -p0, the same approach would almost work,
but it would need apply.pValue support to help the receiving end.

But as we already concluded, let's cook the current 5-patch series
in 'next' and see what happens.

Thanks.

Re: [PATCH 3/5] diff: add --default-prefix option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Oh, you grepped around ;-)?  Good find.

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

Re: [PATCH 3/5] diff: add --default-prefix option

[Jeff King]

On Mon, Mar 13, 2023 at 10:31:04AM -0700, Junio C Hamano wrote:

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

So here it is as a patch, if you want to throw it on top of
jk/format-patch-ignore-noprefix.

I have to admit that after writing the relatively weak argument in the
commit message, it does feel a bit like churn. So I am also OK to just
leave it as-is.

-- >8 --
Subject: rebase: prefer --default-prefix to --{src,dst}-prefix for format-patch

When git-rebase invokes format-patch, it wants to make sure we use the
normal prefixes, and are not confused by diff.noprefix or similar. When
this was added in 5b220a6876f (Add --src/dst-prefix to git-formt-patch
in git-rebase.sh, 2010-09-09), we only had --src-prefix and --dst-prefix
to do so, which requires re-specifying the prefixes we expect to see.
These days we can say what we want more directly: just use the defaults.

This is a minor cleanup that should have no behavior change, but
hopefully the result expresses more clearly what the code is trying to
accomplish.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6635f10d529..a47dfd45efd 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -660,7 +660,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)
-- 
2.40.0.572.gc7ee6f1a071