| Message ID | 20240311023217.GA2345739@quokka (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | diff: add diff.srcprefix and diff.dstprefix option support | expand |
Peter Hutterer <peter.hutterer@who-t.net> writes: > Subject: Re: [PATCH] diff: add diff.srcprefix and diff.dstprefix option support "option support" -> "configuration variables" > The git option equivalent to --src-prefix and --dst-prefix. > Both of these are of lower precedence than the diff.noprefix and > diff.mnemonicprefix option. I think it will become simpler to sell and explain if you do not mention these options, and instead say that we are tweaking the default prefixes used when none of the other options are used, something like: Allow the default prefixes "a/" and "b/" to be tweaked by diff.srcprefix and diff.dstprefix configuration variables. > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > --- > +diff.srcprefix:: > + If set, 'git diff' uses this source prefix. Add "Defaults to 'a/'", perhaps? > @@ -3429,6 +3437,14 @@ void diff_set_default_prefix(struct diff_options *options) > options->b_prefix = "b/"; > } > > +void diff_set_custom_prefix(struct diff_options *options, const char *src_prefix, const char *dst_prefix) > +{ > + if (src_prefix) > + options->a_prefix = src_prefix; > + if (dst_prefix) > + options->b_prefix = dst_prefix; > +} > + > struct userdiff_driver *get_textconv(struct repository *r, > struct diff_filespec *one) > { > @@ -4736,6 +4752,8 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) > diff_set_noprefix(options); > } else if (!diff_mnemonic_prefix) { > diff_set_default_prefix(options); > + if (diff_src_prefix || diff_dst_prefix) > + diff_set_custom_prefix(options, diff_src_prefix, diff_dst_prefix); > } This feels somewhat roundabout way to do this. Instead of touching this part at all, and not adding diff_set_custom_prefix() function, how about just patching diff_set_default_prefix()? The function does not even have to be public and there is no need to touch the header file. Here is how I would simplify the code change part if I were doing this patch. It seems to pass t4013 (including your additional ones). Thanks. diff.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git i/diff.c w/diff.c index e50def4538..4439b1a958 100644 --- i/diff.c +++ w/diff.c @@ -62,6 +62,8 @@ static const char *diff_order_file_cfg; int diff_auto_refresh_index = 1; static int diff_mnemonic_prefix; static int diff_no_prefix; +static const char *diff_src_prefix = "a/"; +static const char *diff_dst_prefix = "b/"; static int diff_relative; static int diff_stat_name_width; static int diff_stat_graph_width; @@ -408,6 +410,12 @@ int git_diff_ui_config(const char *var, const char *value, diff_no_prefix = git_config_bool(var, value); return 0; } + if (!strcmp(var, "diff.srcprefix")) { + return git_config_string(&diff_src_prefix, var, value); + } + if (!strcmp(var, "diff.dstprefix")) { + return git_config_string(&diff_dst_prefix, var, value); + } if (!strcmp(var, "diff.relative")) { diff_relative = git_config_bool(var, value); return 0; @@ -3425,8 +3433,8 @@ void diff_set_noprefix(struct diff_options *options) void diff_set_default_prefix(struct diff_options *options) { - options->a_prefix = "a/"; - options->b_prefix = "b/"; + options->a_prefix = diff_src_prefix; + options->b_prefix = diff_dst_prefix; } struct userdiff_driver *get_textconv(struct repository *r,
diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt index 6c7e09a1ef5e..888632955b30 100644 --- a/Documentation/config/diff.txt +++ b/Documentation/config/diff.txt @@ -111,6 +111,12 @@ diff.mnemonicPrefix:: diff.noprefix:: If set, 'git diff' does not show any source or destination prefix. +diff.srcprefix:: + If set, 'git diff' uses this source prefix. + +diff.dstprefix:: + If set, 'git diff' uses this destination prefix. + diff.relative:: If set to 'true', 'git diff' does not show changes outside of the directory and show pathnames relative to the current directory. diff --git a/diff.c b/diff.c index e50def45383e..52a476737def 100644 --- a/diff.c +++ b/diff.c @@ -62,6 +62,8 @@ static const char *diff_order_file_cfg; int diff_auto_refresh_index = 1; static int diff_mnemonic_prefix; static int diff_no_prefix; +static const char *diff_src_prefix; +static const char *diff_dst_prefix; static int diff_relative; static int diff_stat_name_width; static int diff_stat_graph_width; @@ -408,6 +410,12 @@ int git_diff_ui_config(const char *var, const char *value, diff_no_prefix = git_config_bool(var, value); return 0; } + if (!strcmp(var, "diff.srcprefix")) { + return git_config_string(&diff_src_prefix, var, value); + } + if (!strcmp(var, "diff.dstprefix")) { + return git_config_string(&diff_dst_prefix, var, value); + } if (!strcmp(var, "diff.relative")) { diff_relative = git_config_bool(var, value); return 0; @@ -3429,6 +3437,14 @@ void diff_set_default_prefix(struct diff_options *options) options->b_prefix = "b/"; } +void diff_set_custom_prefix(struct diff_options *options, const char *src_prefix, const char *dst_prefix) +{ + if (src_prefix) + options->a_prefix = src_prefix; + if (dst_prefix) + options->b_prefix = dst_prefix; +} + struct userdiff_driver *get_textconv(struct repository *r, struct diff_filespec *one) { @@ -4736,6 +4752,8 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) diff_set_noprefix(options); } else if (!diff_mnemonic_prefix) { diff_set_default_prefix(options); + if (diff_src_prefix || diff_dst_prefix) + diff_set_custom_prefix(options, diff_src_prefix, diff_dst_prefix); } options->color_moved = diff_color_moved_default; diff --git a/diff.h b/diff.h index 66bd8aeb2936..ab4dd5ec70f3 100644 --- a/diff.h +++ b/diff.h @@ -499,6 +499,7 @@ 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); +void diff_set_custom_prefix(struct diff_options *options, const char *src_prefix, const char *dst_prefix); int diff_can_quit_early(struct diff_options *); diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 1e3b2dbea484..86834186fdba 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -663,6 +663,26 @@ test_expect_success 'diff --default-prefix overrides diff.mnemonicprefix' ' check_prefix actual a/file0 b/file0 ' +test_expect_success 'diff respects diff.srcprefix' ' + git -c diff.srcprefix=x/ diff >actual && + check_prefix actual x/file0 b/file0 +' + +test_expect_success 'diff respects diff.dstprefix' ' + git -c diff.dstprefix=y/ diff >actual && + check_prefix actual a/file0 y/file0 +' + +test_expect_success 'diff src/dstprefix ignored with diff.noprefix' ' + git -c diff.dstprefix=y/ -c diff.srcprefix=x/ -c diff.noprefix diff >actual && + check_prefix actual file0 file0 +' + +test_expect_success 'diff src/dstprefix ignored with diff.mnemonicprefix' ' + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ -c diff.mnemonicprefix diff >actual && + check_prefix actual i/file0 w/file0 +' + test_expect_success 'diff --no-renames cannot be abbreviated' ' test_expect_code 129 git diff --no-rename >actual 2>error && test_must_be_empty actual &&
The git option equivalent to --src-prefix and --dst-prefix. Both of these are of lower precedence than the diff.noprefix and diff.mnemonicprefix option. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> --- As David already mentioned [1] my main motivation here is to use a prefix of "./" because that is patch -p1 compatible *and* supports double-click highlighting of the file path in most terminals. The current approach of a/ and b/ fail at the latter and diff.noprefix fails at the former. [1] https://lore.kernel.org/git/f80aaf4a-ffea-48e6-b279-c3b7a6a53996@ixit.cz/ Documentation/config/diff.txt | 6 ++++++ diff.c | 18 ++++++++++++++++++ diff.h | 1 + t/t4013-diff-various.sh | 20 ++++++++++++++++++++ 4 files changed, 45 insertions(+)