* [PATCH 1/5] diff: factor out src/dst prefix setup
2023-03-09 6:06 ` Jeff King
@ 2023-03-09 6:07 ` Jeff King
2023-03-09 10:50 ` Alejandro Colomar
2023-03-09 6:07 ` [PATCH 2/5] t4013: add tests for diff prefix options Jeff King
` (5 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Jeff King @ 2023-03-09 6:07 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 1/5] diff: factor out src/dst prefix setup
2023-03-09 6:07 ` [PATCH 1/5] diff: factor out src/dst prefix setup Jeff King
@ 2023-03-09 10:50 ` Alejandro Colomar
0 siblings, 0 replies; 30+ messages in thread
From: Alejandro Colomar @ 2023-03-09 10:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 2/5] t4013: add tests for diff prefix options
2023-03-09 6:06 ` Jeff King
2023-03-09 6:07 ` [PATCH 1/5] diff: factor out src/dst prefix setup Jeff King
@ 2023-03-09 6:07 ` Jeff King
2023-03-09 6:09 ` [PATCH 3/5] diff: add --default-prefix option Jeff King
` (4 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: Jeff King @ 2023-03-09 6:07 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 3/5] diff: add --default-prefix option
2023-03-09 6:06 ` Jeff King
2023-03-09 6:07 ` [PATCH 1/5] diff: factor out src/dst prefix setup Jeff King
2023-03-09 6:07 ` [PATCH 2/5] t4013: add tests for diff prefix options Jeff King
@ 2023-03-09 6:09 ` Jeff King
2023-03-09 10:51 ` Alejandro Colomar
2023-03-09 16:31 ` Junio C Hamano
2023-03-09 6:11 ` [PATCH 4/5] format-patch: do not respect diff.noprefix Jeff King
` (3 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: Jeff King @ 2023-03-09 6:09 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-09 6:09 ` [PATCH 3/5] diff: add --default-prefix option Jeff King
@ 2023-03-09 10:51 ` Alejandro Colomar
2023-03-09 16:31 ` Junio C Hamano
1 sibling, 0 replies; 30+ messages in thread
From: Alejandro Colomar @ 2023-03-09 10:51 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-09 6:09 ` [PATCH 3/5] diff: add --default-prefix option Jeff King
2023-03-09 10:51 ` Alejandro Colomar
@ 2023-03-09 16:31 ` Junio C Hamano
2023-03-10 9:44 ` Jeff King
1 sibling, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-03-09 16:31 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-09 16:31 ` Junio C Hamano
@ 2023-03-10 9:44 ` Jeff King
2023-03-10 17:04 ` Junio C Hamano
0 siblings, 1 reply; 30+ messages in thread
From: Jeff King @ 2023-03-10 9:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alejandro Colomar, Git Mailing List
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-10 9:44 ` Jeff King
@ 2023-03-10 17:04 ` Junio C Hamano
2023-03-13 16:43 ` Jeff King
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-03-10 17:04 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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".
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-10 17:04 ` Junio C Hamano
@ 2023-03-13 16:43 ` Jeff King
2023-03-13 17:17 ` Junio C Hamano
2023-03-13 17:31 ` Junio C Hamano
0 siblings, 2 replies; 30+ messages in thread
From: Jeff King @ 2023-03-13 16:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alejandro Colomar, Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-13 16:43 ` Jeff King
@ 2023-03-13 17:17 ` Junio C Hamano
2023-03-13 17:31 ` Junio C Hamano
1 sibling, 0 replies; 30+ messages in thread
From: Junio C Hamano @ 2023-03-13 17:17 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-13 16:43 ` Jeff King
2023-03-13 17:17 ` Junio C Hamano
@ 2023-03-13 17:31 ` Junio C Hamano
2023-03-13 19:54 ` Jeff King
1 sibling, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-03-13 17:31 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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".
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/5] diff: add --default-prefix option
2023-03-13 17:31 ` Junio C Hamano
@ 2023-03-13 19:54 ` Jeff King
0 siblings, 0 replies; 30+ messages in thread
From: Jeff King @ 2023-03-13 19:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alejandro Colomar, Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 4/5] format-patch: do not respect diff.noprefix
2023-03-09 6:06 ` Jeff King
` (2 preceding siblings ...)
2023-03-09 6:09 ` [PATCH 3/5] diff: add --default-prefix option Jeff King
@ 2023-03-09 6:11 ` Jeff King
2023-03-09 10:53 ` Alejandro Colomar
2023-03-09 16:41 ` Junio C Hamano
2023-03-09 6:12 ` [PATCH 5/5] format-patch: add format.noprefix option Jeff King
` (2 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: Jeff King @ 2023-03-09 6:11 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 4/5] format-patch: do not respect diff.noprefix
2023-03-09 6:11 ` [PATCH 4/5] format-patch: do not respect diff.noprefix Jeff King
@ 2023-03-09 10:53 ` Alejandro Colomar
2023-03-09 16:41 ` Junio C Hamano
1 sibling, 0 replies; 30+ messages in thread
From: Alejandro Colomar @ 2023-03-09 10:53 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/5] format-patch: do not respect diff.noprefix
2023-03-09 6:11 ` [PATCH 4/5] format-patch: do not respect diff.noprefix Jeff King
2023-03-09 10:53 ` Alejandro Colomar
@ 2023-03-09 16:41 ` Junio C Hamano
2023-03-10 9:49 ` Jeff King
1 sibling, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-03-09 16:41 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 4/5] format-patch: do not respect diff.noprefix
2023-03-09 16:41 ` Junio C Hamano
@ 2023-03-10 9:49 ` Jeff King
0 siblings, 0 replies; 30+ messages in thread
From: Jeff King @ 2023-03-10 9:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alejandro Colomar, Git Mailing List
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 5/5] format-patch: add format.noprefix option
2023-03-09 6:06 ` Jeff King
` (3 preceding siblings ...)
2023-03-09 6:11 ` [PATCH 4/5] format-patch: do not respect diff.noprefix Jeff King
@ 2023-03-09 6:12 ` Jeff King
2023-03-09 17:00 ` Junio C Hamano
2023-03-09 10:58 ` Better suggestions when git-am(1) fails Alejandro Colomar
2023-03-09 21:53 ` Junio C Hamano
6 siblings, 1 reply; 30+ messages in thread
From: Jeff King @ 2023-03-09 6:12 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Git Mailing List
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 5/5] format-patch: add format.noprefix option
2023-03-09 6:12 ` [PATCH 5/5] format-patch: add format.noprefix option Jeff King
@ 2023-03-09 17:00 ` Junio C Hamano
2023-03-10 9:51 ` Jeff King
0 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-03-09 17:00 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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;
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 5/5] format-patch: add format.noprefix option
2023-03-09 17:00 ` Junio C Hamano
@ 2023-03-10 9:51 ` Jeff King
0 siblings, 0 replies; 30+ messages in thread
From: Jeff King @ 2023-03-10 9:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alejandro Colomar, Git Mailing List
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Better suggestions when git-am(1) fails
2023-03-09 6:06 ` Jeff King
` (4 preceding siblings ...)
2023-03-09 6:12 ` [PATCH 5/5] format-patch: add format.noprefix option Jeff King
@ 2023-03-09 10:58 ` Alejandro Colomar
2023-03-09 21:53 ` Junio C Hamano
6 siblings, 0 replies; 30+ messages in thread
From: Alejandro Colomar @ 2023-03-09 10:58 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
[-- 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 --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Better suggestions when git-am(1) fails
2023-03-09 6:06 ` Jeff King
` (5 preceding siblings ...)
2023-03-09 10:58 ` Better suggestions when git-am(1) fails Alejandro Colomar
@ 2023-03-09 21:53 ` Junio C Hamano
2023-03-10 9:54 ` Jeff King
6 siblings, 1 reply; 30+ messages in thread
From: Junio C Hamano @ 2023-03-09 21:53 UTC (permalink / raw)
To: Jeff King; +Cc: Alejandro Colomar, Git Mailing List
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.
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Better suggestions when git-am(1) fails
2023-03-09 21:53 ` Junio C Hamano
@ 2023-03-10 9:54 ` Jeff King
0 siblings, 0 replies; 30+ messages in thread
From: Jeff King @ 2023-03-10 9:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alejandro Colomar, Git Mailing List
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
^ permalink raw reply [flat|nested] 30+ messages in thread