| Message ID | 20240312231559.GA116605@quokka (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] diff: add diff.srcPrefix and diff.dstPrefix configuration variables | expand |
Hello Peter, On 2024-03-13 00:15, Peter Hutterer wrote: > Allow the default prefixes "a/" and "b/" to be tweaked by the > diff.srcprefix and diff.dstprefix configuration variables. The only thing that's left is to update the patch description to use camel case. :) > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > --- > Changes to v2; > - doc: change to camelcase diff.srcPrefix/diff.dstPrefix for > consistency with diff.mnemonicPrefix and most other options > - git diff --default-prefix forces a/ and b/ regardless of configured > prefix, see the 'diff_opt_default_prefix' hunk in the patch below. > > The latter may be slightly controversial but: there are scripts out > there that rely on the a/ and b/ prefix (came across one last night). > With a custom prefix those scripts will break, having an option that > forces the a/ and b/ prefix helps. Plus the man page explicitly says: > Use the default source and destination prefixes ("a/" and "b/"). > So let's stick with that behaviour then. > > Documentation/config/diff.txt | 6 ++++++ > diff.c | 14 ++++++++++++-- > t/t4013-diff-various.sh | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/diff.txt > b/Documentation/config/diff.txt > index 6c7e09a1ef5e..afc23d7723b6 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. Defaults to 'a/'. > + > +diff.dstPrefix:: > + If set, 'git diff' uses this destination prefix. Defaults to 'b/'. > + > 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..108c1875775d 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 = "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, > @@ -5362,6 +5370,8 @@ static int diff_opt_default_prefix(const struct > option *opt, > > BUG_ON_OPT_NEG(unset); > BUG_ON_OPT_ARG(optarg); > + diff_src_prefix = "a/"; > + diff_dst_prefix = "b/"; > diff_set_default_prefix(options); > return 0; > } > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 1e3b2dbea484..e75f9f7d4cb2 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -663,6 +663,41 @@ 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-prefix overrides diff.srcprefix' ' > + git -c diff.srcprefix=z/ diff --src-prefix=z/ >actual && > + check_prefix actual z/file0 b/file0 > +' > + > +test_expect_success 'diff --dst-prefix overrides diff.dstprefix' ' > + git -c diff.dstprefix=y/ diff --dst-prefix=z/ >actual && > + check_prefix actual a/file0 z/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 src/dstprefix ignored with --default-prefix' > ' > + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ diff --default-prefix > >actual && > + check_prefix actual a/file0 b/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 &&
On 2024-03-13 03:15, Dragan Simic wrote: > On 2024-03-13 00:15, Peter Hutterer wrote: >> Allow the default prefixes "a/" and "b/" to be tweaked by the >> diff.srcprefix and diff.dstprefix configuration variables. > > The only thing that's left is to update the patch description > to use camel case. :) I've spotted some inconsistencies in the way camel case is already used in some of the "diff.*" configuration option names, so I went ahead and fixed those. I'll send the patches after I figure out how to best split the changes into patches for easier review, because there are quite a few small changes. >> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >> --- >> Changes to v2; >> - doc: change to camelcase diff.srcPrefix/diff.dstPrefix for >> consistency with diff.mnemonicPrefix and most other options >> - git diff --default-prefix forces a/ and b/ regardless of configured >> prefix, see the 'diff_opt_default_prefix' hunk in the patch below. >> >> The latter may be slightly controversial but: there are scripts out >> there that rely on the a/ and b/ prefix (came across one last night). >> With a custom prefix those scripts will break, having an option that >> forces the a/ and b/ prefix helps. Plus the man page explicitly says: >> Use the default source and destination prefixes ("a/" and "b/"). >> So let's stick with that behaviour then. >> >> Documentation/config/diff.txt | 6 ++++++ >> diff.c | 14 ++++++++++++-- >> t/t4013-diff-various.sh | 35 >> +++++++++++++++++++++++++++++++++++ >> 3 files changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/config/diff.txt >> b/Documentation/config/diff.txt >> index 6c7e09a1ef5e..afc23d7723b6 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. Defaults to 'a/'. >> + >> +diff.dstPrefix:: >> + If set, 'git diff' uses this destination prefix. Defaults to 'b/'. >> + >> 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..108c1875775d 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 = "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, >> @@ -5362,6 +5370,8 @@ static int diff_opt_default_prefix(const struct >> option *opt, >> >> BUG_ON_OPT_NEG(unset); >> BUG_ON_OPT_ARG(optarg); >> + diff_src_prefix = "a/"; >> + diff_dst_prefix = "b/"; >> diff_set_default_prefix(options); >> return 0; >> } >> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >> index 1e3b2dbea484..e75f9f7d4cb2 100755 >> --- a/t/t4013-diff-various.sh >> +++ b/t/t4013-diff-various.sh >> @@ -663,6 +663,41 @@ 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-prefix overrides diff.srcprefix' ' >> + git -c diff.srcprefix=z/ diff --src-prefix=z/ >actual && >> + check_prefix actual z/file0 b/file0 >> +' >> + >> +test_expect_success 'diff --dst-prefix overrides diff.dstprefix' ' >> + git -c diff.dstprefix=y/ diff --dst-prefix=z/ >actual && >> + check_prefix actual a/file0 z/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 src/dstprefix ignored with >> --default-prefix' ' >> + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ diff --default-prefix >> >actual && >> + check_prefix actual a/file0 b/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 &&
Hi Peter On 12/03/2024 23:15, Peter Hutterer wrote: > Allow the default prefixes "a/" and "b/" to be tweaked by the > diff.srcprefix and diff.dstprefix configuration variables. > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> > --- > Changes to v2; > - doc: change to camelcase diff.srcPrefix/diff.dstPrefix for > consistency with diff.mnemonicPrefix and most other options > - git diff --default-prefix forces a/ and b/ regardless of configured > prefix, see the 'diff_opt_default_prefix' hunk in the patch below. > > The latter may be slightly controversial but: there are scripts out > there that rely on the a/ and b/ prefix (came across one last night). > With a custom prefix those scripts will break, having an option that > forces the a/ and b/ prefix helps. Plus the man page explicitly says: > Use the default source and destination prefixes ("a/" and "b/"). > So let's stick with that behaviour then. As I understand it the purpose of --default-prefix is to override all the prefix related config settings so this seems like a very sensible choice. Best Wishes Phillip > Documentation/config/diff.txt | 6 ++++++ > diff.c | 14 ++++++++++++-- > t/t4013-diff-various.sh | 35 +++++++++++++++++++++++++++++++++++ > 3 files changed, 53 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt > index 6c7e09a1ef5e..afc23d7723b6 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. Defaults to 'a/'. > + > +diff.dstPrefix:: > + If set, 'git diff' uses this destination prefix. Defaults to 'b/'. > + > 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..108c1875775d 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 = "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, > @@ -5362,6 +5370,8 @@ static int diff_opt_default_prefix(const struct option *opt, > > BUG_ON_OPT_NEG(unset); > BUG_ON_OPT_ARG(optarg); > + diff_src_prefix = "a/"; > + diff_dst_prefix = "b/"; > diff_set_default_prefix(options); > return 0; > } > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 1e3b2dbea484..e75f9f7d4cb2 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -663,6 +663,41 @@ 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-prefix overrides diff.srcprefix' ' > + git -c diff.srcprefix=z/ diff --src-prefix=z/ >actual && > + check_prefix actual z/file0 b/file0 > +' > + > +test_expect_success 'diff --dst-prefix overrides diff.dstprefix' ' > + git -c diff.dstprefix=y/ diff --dst-prefix=z/ >actual && > + check_prefix actual a/file0 z/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 src/dstprefix ignored with --default-prefix' ' > + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ diff --default-prefix >actual && > + check_prefix actual a/file0 b/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 &&
On 13/03/2024 02:15, Dragan Simic wrote: > Hello Peter, > > On 2024-03-13 00:15, Peter Hutterer wrote: >> Allow the default prefixes "a/" and "b/" to be tweaked by the >> diff.srcprefix and diff.dstprefix configuration variables. > > The only thing that's left is to update the patch description > to use camel case. :) If that's the only change that's required I don't think we should be asking for a re-roll. We do place a high value on good commit messages in this project but I don't think it is reasonable to require a re-roll for a purely aesthetic change. Best Wishes Phillip >> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >> --- >> Changes to v2; >> - doc: change to camelcase diff.srcPrefix/diff.dstPrefix for >> consistency with diff.mnemonicPrefix and most other options >> - git diff --default-prefix forces a/ and b/ regardless of configured >> prefix, see the 'diff_opt_default_prefix' hunk in the patch below. >> >> The latter may be slightly controversial but: there are scripts out >> there that rely on the a/ and b/ prefix (came across one last night). >> With a custom prefix those scripts will break, having an option that >> forces the a/ and b/ prefix helps. Plus the man page explicitly says: >> Use the default source and destination prefixes ("a/" and "b/"). >> So let's stick with that behaviour then. >> >> Documentation/config/diff.txt | 6 ++++++ >> diff.c | 14 ++++++++++++-- >> t/t4013-diff-various.sh | 35 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 53 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/config/diff.txt >> b/Documentation/config/diff.txt >> index 6c7e09a1ef5e..afc23d7723b6 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. Defaults to 'a/'. >> + >> +diff.dstPrefix:: >> + If set, 'git diff' uses this destination prefix. Defaults to 'b/'. >> + >> 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..108c1875775d 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 = "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, >> @@ -5362,6 +5370,8 @@ static int diff_opt_default_prefix(const struct >> option *opt, >> >> BUG_ON_OPT_NEG(unset); >> BUG_ON_OPT_ARG(optarg); >> + diff_src_prefix = "a/"; >> + diff_dst_prefix = "b/"; >> diff_set_default_prefix(options); >> return 0; >> } >> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >> index 1e3b2dbea484..e75f9f7d4cb2 100755 >> --- a/t/t4013-diff-various.sh >> +++ b/t/t4013-diff-various.sh >> @@ -663,6 +663,41 @@ 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-prefix overrides diff.srcprefix' ' >> + git -c diff.srcprefix=z/ diff --src-prefix=z/ >actual && >> + check_prefix actual z/file0 b/file0 >> +' >> + >> +test_expect_success 'diff --dst-prefix overrides diff.dstprefix' ' >> + git -c diff.dstprefix=y/ diff --dst-prefix=z/ >actual && >> + check_prefix actual a/file0 z/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 src/dstprefix ignored with --default-prefix' ' >> + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ diff >> --default-prefix >actual && >> + check_prefix actual a/file0 b/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 && >
Hello Phillip, On 2024-03-13 16:06, Phillip Wood wrote: > On 13/03/2024 02:15, Dragan Simic wrote: >> Hello Peter, >> >> On 2024-03-13 00:15, Peter Hutterer wrote: >>> Allow the default prefixes "a/" and "b/" to be tweaked by the >>> diff.srcprefix and diff.dstprefix configuration variables. >> >> The only thing that's left is to update the patch description >> to use camel case. :) > > If that's the only change that's required I don't think we should be > asking for a re-roll. We do place a high value on good commit messages > in this project but I don't think it is reasonable to require a > re-roll for a purely aesthetic change. Well, I required nothing, I just noted it. Such a small change can also be performed by Junio while applying the patch. >>> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> >>> --- >>> Changes to v2; >>> - doc: change to camelcase diff.srcPrefix/diff.dstPrefix for >>> consistency with diff.mnemonicPrefix and most other options >>> - git diff --default-prefix forces a/ and b/ regardless of configured >>> prefix, see the 'diff_opt_default_prefix' hunk in the patch below. >>> >>> The latter may be slightly controversial but: there are scripts out >>> there that rely on the a/ and b/ prefix (came across one last night). >>> With a custom prefix those scripts will break, having an option that >>> forces the a/ and b/ prefix helps. Plus the man page explicitly says: >>> Use the default source and destination prefixes ("a/" and "b/"). >>> So let's stick with that behaviour then. >>> >>> Documentation/config/diff.txt | 6 ++++++ >>> diff.c | 14 ++++++++++++-- >>> t/t4013-diff-various.sh | 35 >>> +++++++++++++++++++++++++++++++++++ >>> 3 files changed, 53 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/config/diff.txt >>> b/Documentation/config/diff.txt >>> index 6c7e09a1ef5e..afc23d7723b6 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. Defaults to 'a/'. >>> + >>> +diff.dstPrefix:: >>> + If set, 'git diff' uses this destination prefix. Defaults to >>> 'b/'. >>> + >>> 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..108c1875775d 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 = "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, >>> @@ -5362,6 +5370,8 @@ static int diff_opt_default_prefix(const struct >>> option *opt, >>> >>> BUG_ON_OPT_NEG(unset); >>> BUG_ON_OPT_ARG(optarg); >>> + diff_src_prefix = "a/"; >>> + diff_dst_prefix = "b/"; >>> diff_set_default_prefix(options); >>> return 0; >>> } >>> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >>> index 1e3b2dbea484..e75f9f7d4cb2 100755 >>> --- a/t/t4013-diff-various.sh >>> +++ b/t/t4013-diff-various.sh >>> @@ -663,6 +663,41 @@ 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-prefix overrides diff.srcprefix' ' >>> + git -c diff.srcprefix=z/ diff --src-prefix=z/ >actual && >>> + check_prefix actual z/file0 b/file0 >>> +' >>> + >>> +test_expect_success 'diff --dst-prefix overrides diff.dstprefix' ' >>> + git -c diff.dstprefix=y/ diff --dst-prefix=z/ >actual && >>> + check_prefix actual a/file0 z/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 src/dstprefix ignored with >>> --default-prefix' ' >>> + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ diff >>> --default-prefix >actual && >>> + check_prefix actual a/file0 b/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 && >>
Dragan Simic <dsimic@manjaro.org> writes: > Well, I required nothing, I just noted it. Such a small change can > also be performed by Junio while applying the patch. Doing so is trivial. Having to remember doing so when there are many other patches in flight is a burden. Don't put things on my plate when you do not have to. Thanks.
On 2024-03-13 16:24, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> Well, I required nothing, I just noted it. Such a small change can >> also be performed by Junio while applying the patch. > > Doing so is trivial. Having to remember doing so when there are > many other patches in flight is a burden. Don't put things on my > plate when you do not have to. You're right, there are more important things. Sorry.
Phillip Wood <phillip.wood123@gmail.com> writes: >> With a custom prefix those scripts will break, having an option that >> forces the a/ and b/ prefix helps. Plus the man page explicitly says: >> Use the default source and destination prefixes ("a/" and "b/"). >> So let's stick with that behaviour then. > > As I understand it the purpose of --default-prefix is to override all > the prefix related config settings so this seems like a very sensible > choice. It would be nice to update the description of '--default-prefix' so that nobody has to say "As I understand it" anymore ;-) As we are selling .{src,dst}Prefix as a thing that sets the default prefix, we'd need to break the loop somehow, and "hardcoded" below is my attempt to do so, but I am not sure if I succeeded. Documentation/diff-options.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt index aaaff0d46f..62eaa46d84 100644 --- c/Documentation/diff-options.txt +++ w/Documentation/diff-options.txt @@ -864,9 +864,10 @@ endif::git-format-patch[] 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`. + Use the hardcoded default source and destination prefixes + ("a/" and "b/"). This is designed to be used to override + configuration variables such as `diff.noprefix` and + `diff.srcPrefix`. --line-prefix=<prefix>:: Prepend an additional prefix to every line of output.
On 13/03/2024 15:29, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > >>> With a custom prefix those scripts will break, having an option that >>> forces the a/ and b/ prefix helps. Plus the man page explicitly says: >>> Use the default source and destination prefixes ("a/" and "b/"). >>> So let's stick with that behaviour then. >> >> As I understand it the purpose of --default-prefix is to override all >> the prefix related config settings so this seems like a very sensible >> choice. > > It would be nice to update the description of '--default-prefix' so > that nobody has to say "As I understand it" anymore ;-) That's a good idea > As we are selling .{src,dst}Prefix as a thing that sets the default > prefix, we'd need to break the loop somehow, and "hardcoded" below > is my attempt to do so, but I am not sure if I succeeded. > > Documentation/diff-options.txt | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git c/Documentation/diff-options.txt w/Documentation/diff-options.txt > index aaaff0d46f..62eaa46d84 100644 > --- c/Documentation/diff-options.txt > +++ w/Documentation/diff-options.txt > @@ -864,9 +864,10 @@ endif::git-format-patch[] > 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`. > + Use the hardcoded default source and destination prefixes > + ("a/" and "b/"). This is designed to be used to override > + configuration variables such as `diff.noprefix` and > + `diff.srcPrefix`. That looks clear to me. I think the only other config variable that affects the prefix is "diff.mnemonicPrefix" so if we're going to update the description to mention "diff.srcPrefix" maybe we should mention that one as well or just say something like "This is designed to be used to override the configuration variables `diff.*Prefix`.". Best Wishes Phillip > --line-prefix=<prefix>:: > Prepend an additional prefix to every line of output.
Phillip Wood <phillip.wood123@gmail.com> writes: > mention that one as well or just say something like "This is designed > to be used to override the configuration variables `diff.*Prefix`.". Excellent. Thanks. Peter, do you want to wrap things up with an updated patch (no rush and no obligation to do so---we just want to know if it will happen, in which case we will just wait, and otherwise somebody else will do that)?
On 2024-03-13 16:29, Junio C Hamano wrote: > Documentation/diff-options.txt | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git c/Documentation/diff-options.txt > w/Documentation/diff-options.txt > index aaaff0d46f..62eaa46d84 100644 > --- c/Documentation/diff-options.txt > +++ w/Documentation/diff-options.txt > @@ -864,9 +864,10 @@ endif::git-format-patch[] > 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`. > + Use the hardcoded default source and destination prefixes > + ("a/" and "b/"). This is designed to be used to override > + configuration variables such as `diff.noprefix` and > + `diff.srcPrefix`. How about this instead: Ignore any configuration variables that control source and destination prefixes, which includes `diff.noPrefix`, `diff.srcPrefix` and `diff.dstPrefix` (see `git-config`(1)), and use the default prefixes `a/` and `b/`. > --line-prefix=<prefix>:: > Prepend an additional prefix to every line of output.
On Wed, Mar 13, 2024 at 10:55:50AM -0700, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > > mention that one as well or just say something like "This is designed > > to be used to override the configuration variables `diff.*Prefix`.". > > Excellent. > > Thanks. Peter, do you want to wrap things up with an updated patch > (no rush and no obligation to do so---we just want to know if it > will happen, in which case we will just wait, and otherwise somebody > else will do that)? Happy to send out another patch (tomorrow, assuming the discussion has settled fully). Cheers, Peter
diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt index 6c7e09a1ef5e..afc23d7723b6 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. Defaults to 'a/'. + +diff.dstPrefix:: + If set, 'git diff' uses this destination prefix. Defaults to 'b/'. + 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..108c1875775d 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 = "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, @@ -5362,6 +5370,8 @@ static int diff_opt_default_prefix(const struct option *opt, BUG_ON_OPT_NEG(unset); BUG_ON_OPT_ARG(optarg); + diff_src_prefix = "a/"; + diff_dst_prefix = "b/"; diff_set_default_prefix(options); return 0; } diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 1e3b2dbea484..e75f9f7d4cb2 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -663,6 +663,41 @@ 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-prefix overrides diff.srcprefix' ' + git -c diff.srcprefix=z/ diff --src-prefix=z/ >actual && + check_prefix actual z/file0 b/file0 +' + +test_expect_success 'diff --dst-prefix overrides diff.dstprefix' ' + git -c diff.dstprefix=y/ diff --dst-prefix=z/ >actual && + check_prefix actual a/file0 z/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 src/dstprefix ignored with --default-prefix' ' + git -c diff.dstprefix=x/ -c diff.srcprefix=y/ diff --default-prefix >actual && + check_prefix actual a/file0 b/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 &&
Allow the default prefixes "a/" and "b/" to be tweaked by the diff.srcprefix and diff.dstprefix configuration variables. Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net> --- Changes to v2; - doc: change to camelcase diff.srcPrefix/diff.dstPrefix for consistency with diff.mnemonicPrefix and most other options - git diff --default-prefix forces a/ and b/ regardless of configured prefix, see the 'diff_opt_default_prefix' hunk in the patch below. The latter may be slightly controversial but: there are scripts out there that rely on the a/ and b/ prefix (came across one last night). With a custom prefix those scripts will break, having an option that forces the a/ and b/ prefix helps. Plus the man page explicitly says: Use the default source and destination prefixes ("a/" and "b/"). So let's stick with that behaviour then. Documentation/config/diff.txt | 6 ++++++ diff.c | 14 ++++++++++++-- t/t4013-diff-various.sh | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 2 deletions(-)