From 6799aadfdf484135476aaf74f5d2eb825d9f00e8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Mar 2023 01:07:06 -0500 Subject: [PATCH 1/6] diff: factor out src/dst prefix setup 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 Signed-off-by: Junio C Hamano --- diff.c | 19 ++++++++++++++----- diff.h | 2 ++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index 469e18aed2..750d1b1a6c 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 8d770b1d57..2af10bc585 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 *); From 7c03d0db8807d303540297432455adfa1c45b05e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Mar 2023 01:07:45 -0500 Subject: [PATCH 2/6] t4013: add tests for diff prefix options 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 Signed-off-by: Junio C Hamano --- 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 dfcf3a0aaa..0bc6957989 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -616,4 +616,36 @@ test_expect_success 'diff -I: detect malformed regex' ' test_i18ngrep "invalid regex given to -I: " error ' +# check_prefix +# 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 From b39a5697296659c344cd0a286b51303fd5375fab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Mar 2023 01:09:54 -0500 Subject: [PATCH 3/6] diff: add --default-prefix option 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 Signed-off-by: Junio C Hamano --- 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 7d73e976d9..08ab86189a 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=:: Prepend an additional prefix to every line of output. diff --git a/diff.c b/diff.c index 750d1b1a6c..b322e319ff 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 0bc6957989..5de1d19075 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 From c169af8f7ab521cc47b59f104db78847e324a3cb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Mar 2023 01:11:49 -0500 Subject: [PATCH 4/6] format-patch: do not respect diff.noprefix 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 Signed-off-by: Junio C Hamano --- 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 a70fba198f..eaf511aab8 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 f3313b8c58..f5a41fd47e 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 From 8d5213decff887b2d950b732e37b7abad6f8d450 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Mar 2023 01:12:37 -0500 Subject: [PATCH 5/6] format-patch: add format.noprefix option 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 Signed-off-by: Junio C Hamano --- 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 73678d88a1..8cf6f00d93 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 eaf511aab8..b1f59062f4 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 [] [] [[--] ...]"), @@ -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 f5a41fd47e..2711fd09ca 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 From ab89575387c02ea024163256826ad1c6dd2e4247 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 13 Mar 2023 15:54:11 -0400 Subject: [PATCH 6/6] 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 Signed-off-by: Junio C Hamano --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 6635f10d52..a47dfd45ef 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)