| Message ID | 20221127093721.31012-4-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | diff-merges: more features | expand |
Sergey Organov <sorganov@gmail.com> writes:
> + if (set_func != NULL) {
Please write it like so:
if (set_func) {
I am not reviewing any new feature topic during -rc period (yet),
but this triggered CI failure at the tip of 'seen', so.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> + if (set_func != NULL) { > > Please write it like so: > > if (set_func) { OK, will do. > > I am not reviewing any new feature topic during -rc period (yet), > but this triggered CI failure at the tip of 'seen', so. Thanks! Do we now have tool for auto-check for these issues? I still use one from Linux kernel, and it didn't object to this form. -- Sergey Organov
On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote: > > Force specified log format for -c, --cc, and --remerge-diff options > instead of their respective formats. The override is useful when some > external tool hard-codes diff for merges format option. > > Using any of the above options twice or more will get back the > original meaning of the option no matter what configuration says. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > Documentation/config/log.txt | 11 +++++++++++ > builtin/log.c | 2 ++ > diff-merges.c | 32 ++++++++++++++++++++++++++------ > diff-merges.h | 2 ++ > t/t4013-diff-various.sh | 18 ++++++++++++++++++ > t/t9902-completion.sh | 3 +++ > 6 files changed, 62 insertions(+), 6 deletions(-) > > diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt > index 265a57312e58..7452c7fad638 100644 > --- a/Documentation/config/log.txt > +++ b/Documentation/config/log.txt > @@ -43,6 +43,17 @@ log.diffMergesHide:: > log.diffMerges-m-imply-p:: > `true` enables implication of `-p` by `-m`. > > +log.diffMergesForce:: > + Use specified log format for -c, --cc, and --remerge-diff > + options instead of their respective formats when the option > + appears on the command line one time. See `--diff-merges` in > + linkgit:git-log[1] for allowed values. Using 'off' or 'none' > + disables the override (default). > ++ > +The override is useful when external tool hard-codes one of the above > +options. Using any of these options two (or more) times will get back > +the original meaning of the options. I didn't quite understand your intent here from this explanation. When you pointed out to Junio that you wanted to override magit's hard-coded `git log --cc` and turn it into `git log -m -p`, then it suddenly made more sense. And the two or more times I guess is your escape hatch to allow users to say "I *really* do want this other format, so `git log --cc --cc` will get it for me.". Maybe something like: Override -c, --cc, --remerge-diff options and use the specified diff-generation scheme for merges instead. However, this config setting can in turn be overridden by specifying an alternate option multiple times (e.g. `git log --cc --cc`). Overriding the diff-generation scheme for merges can be useful when an external tool has a hard-coded command line it calls such as `git log --cc`. See `--diff-merges` in linkgit:git-log[1] for allowed values. Using 'off' or 'none' disables the override (default). However: * This feels like we're trying to workaround bugs or inflexibility in other tools with code in Git. This feels like a slippery slope issue and/or fixing the wrong tool. * Why is this just for -c, --cc, and --remerge-diff, and not for also overriding -m? It seems odd that one would be left out, especially since tools are more likely to have hard-coded it than --remerge-diff, given that -m has been around for a long time and --remerge-diff is new. > + > log.follow:: > If `true`, `git log` will act as if the `--follow` option was used when > a single <path> is given. This has the same limitations as `--follow`, [...] > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index 1789dd6063c5..8a90d2dac360 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' ' > test_cmp expected actual > ' > > +test_expect_success 'git config log.diffMergesForce has proper effect' ' > + git log -m -p master >result && > + process_diffs result >expected && > + test_config log.diffMergesForce on && I think the default for `on` is bad; it made sense at the time, but I think we have a better option now. Perhaps we switch to it, perhaps we don't, but if there's _any_ chance at all we change the default for "on" (which I think there definitely is), then you should really use the option that matches the actual mode you are using rather than a synonym for it; doing so future-proofs this testcase. > + git log --cc master >result && > + process_diffs result >actual && > + test_cmp expected actual > +' > + > +test_expect_success 'git config log.diffMergesForce override by duplicate' ' > + git log --cc master >result && > + process_diffs result >expected && > + test_config log.diffMergesForce on && Matters less here, but just in case "--cc" were to become the default, it'd be nice to explicitly use something else like separate here. > + git log --cc --cc master >result && > + process_diffs result >actual && > + test_cmp expected actual > +'
Sergey Organov <sorganov@gmail.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> + if (set_func != NULL) { >> >> Please write it like so: >> >> if (set_func) { > > OK, will do. > >> >> I am not reviewing any new feature topic during -rc period (yet), >> but this triggered CI failure at the tip of 'seen', so. > > Thanks! Do we now have tool for auto-check for these issues? I still use > one from Linux kernel, and it didn't object to this form. I noticed it when I pushed to GitHub, which ran the CI ;-) If you have your own fork at GitHub of https://github.com/git/git/, I think preparing a pull request against it triggers the CI.
On Wed, Nov 30 2022, Junio C Hamano wrote: > Sergey Organov <sorganov@gmail.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Sergey Organov <sorganov@gmail.com> writes: >>> >>>> + if (set_func != NULL) { >>> >>> Please write it like so: >>> >>> if (set_func) { >> >> OK, will do. >> >>> >>> I am not reviewing any new feature topic during -rc period (yet), >>> but this triggered CI failure at the tip of 'seen', so. >> >> Thanks! Do we now have tool for auto-check for these issues? I still use >> one from Linux kernel, and it didn't object to this form. > > I noticed it when I pushed to GitHub, which ran the CI ;-) > > If you have your own fork at GitHub of https://github.com/git/git/, I > think preparing a pull request against it triggers the CI. ...in this case though there's no reason to wait for the glacially slow GitHub CI. You just need spatch installed and: make coccicheck Sergey: If you've hacked on the (I'm assuming linux) kernel you likely have that installed already, they use it too (being the canonical and I believe original use-case for it).
On Mon, Nov 28, 2022 at 05:44:29PM +0300, Sergey Organov wrote: > >> + if (set_func != NULL) { > > > > Please write it like so: > > > > if (set_func) { >[...] > Thanks! Do we now have tool for auto-check for these issues? I still use > one from Linux kernel, and it didn't object to this form. Running "make coccicheck" will find this, but of course you need to have coccinelle installed. Note that if it finds anything, "make" won't report an error. You have to check for non-empty files in contrib/coccinelle/*.patch. -Peff
Elijah Newren <newren@gmail.com> writes: > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Force specified log format for -c, --cc, and --remerge-diff options >> instead of their respective formats. The override is useful when some >> external tool hard-codes diff for merges format option. >> >> Using any of the above options twice or more will get back the >> original meaning of the option no matter what configuration says. >> >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> Documentation/config/log.txt | 11 +++++++++++ >> builtin/log.c | 2 ++ >> diff-merges.c | 32 ++++++++++++++++++++++++++------ >> diff-merges.h | 2 ++ >> t/t4013-diff-various.sh | 18 ++++++++++++++++++ >> t/t9902-completion.sh | 3 +++ >> 6 files changed, 62 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt >> index 265a57312e58..7452c7fad638 100644 >> --- a/Documentation/config/log.txt >> +++ b/Documentation/config/log.txt >> @@ -43,6 +43,17 @@ log.diffMergesHide:: >> log.diffMerges-m-imply-p:: >> `true` enables implication of `-p` by `-m`. >> >> +log.diffMergesForce:: >> + Use specified log format for -c, --cc, and --remerge-diff >> + options instead of their respective formats when the option >> + appears on the command line one time. See `--diff-merges` in >> + linkgit:git-log[1] for allowed values. Using 'off' or 'none' >> + disables the override (default). >> ++ >> +The override is useful when external tool hard-codes one of the above >> +options. Using any of these options two (or more) times will get back >> +the original meaning of the options. > > I didn't quite understand your intent here from this explanation. > When you pointed out to Junio that you wanted to override magit's > hard-coded `git log --cc` and turn it into `git log -m -p`, then it > suddenly made more sense. And the two or more times I guess is your > escape hatch to allow users to say "I *really* do want this other > format, so `git log --cc --cc` will get it for me.". > > Maybe something like: > > Override -c, --cc, --remerge-diff options and use the specified > diff-generation scheme for merges instead. However, this config > setting can in turn be overridden by specifying an alternate option > multiple times (e.g. `git log --cc --cc`). Overriding the > diff-generation scheme for merges can be useful when an external tool > has a hard-coded command line it calls such as `git log --cc`. See > `--diff-merges` in linkgit:git-log[1] for allowed values. Using 'off' > or 'none' disables the override (default). Thanks for suggestion, I'll take this into consideration should we agree to actually let this feature in. > > However: > * This feels like we're trying to workaround bugs or inflexibility > in other tools with code in Git. This feels like a slippery slope > issue and/or fixing the wrong tool. Yep, that's why I said in my another answer to Junio that I won't insist on it if you guys object, even though it does look useful for me. > * Why is this just for -c, --cc, and --remerge-diff, and not for > also overriding -m? It seems odd that one would be left out, > especially since tools are more likely to have hard-coded it than > --remerge-diff, given that -m has been around for a long time and > --remerge-diff is new. '-m' is rather the first one that got an override support, see 'log.diffMerges'. [As for --remerge-diff, as a side note, I'd call it something like --rd for short, as we have --diff-merges=remerge anyway. And then I'll think about adding --pd (pure-diff) or --fpd (first-parent-diff) ;-)] > >> + >> log.follow:: >> If `true`, `git log` will act as if the `--follow` option was used when >> a single <path> is given. This has the same limitations as `--follow`, > [...] >> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh >> index 1789dd6063c5..8a90d2dac360 100755 >> --- a/t/t4013-diff-various.sh >> +++ b/t/t4013-diff-various.sh >> @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' ' >> test_cmp expected actual >> ' >> >> +test_expect_success 'git config log.diffMergesForce has proper effect' ' >> + git log -m -p master >result && >> + process_diffs result >expected && >> + test_config log.diffMergesForce on && > > I think the default for `on` is bad; it made sense at the time, but I > think we have a better option now. We probably disagree about what a better option actually is, but the point is valid anyway. > Perhaps we switch to it, perhaps we don't, but if there's _any_ chance > at all we change the default for "on" (which I think there definitely > is), then you should really use the option that matches the actual > mode you are using rather than a synonym for it; doing so > future-proofs this testcase. Yep, agreed. Thanks for the catch! > >> + git log --cc master >result && >> + process_diffs result >actual && >> + test_cmp expected actual >> +' >> + >> +test_expect_success 'git config log.diffMergesForce override by duplicate' ' >> + git log --cc master >result && >> + process_diffs result >expected && >> + test_config log.diffMergesForce on && > > Matters less here, but just in case "--cc" were to become the default, > it'd be nice to explicitly use something else like separate here. Yes, thanks! > >> + git log --cc --cc master >result && >> + process_diffs result >actual && >> + test_cmp expected actual >> +' -- Sergey Organov
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Nov 30 2022, Junio C Hamano wrote: > >> Sergey Organov <sorganov@gmail.com> writes: >> >>> Junio C Hamano <gitster@pobox.com> writes: >>> >>>> Sergey Organov <sorganov@gmail.com> writes: >>>> >>>>> + if (set_func != NULL) { >>>> >>>> Please write it like so: >>>> >>>> if (set_func) { >>> >>> OK, will do. >>> >>>> >>>> I am not reviewing any new feature topic during -rc period (yet), >>>> but this triggered CI failure at the tip of 'seen', so. >>> >>> Thanks! Do we now have tool for auto-check for these issues? I still use >>> one from Linux kernel, and it didn't object to this form. >> >> I noticed it when I pushed to GitHub, which ran the CI ;-) >> >> If you have your own fork at GitHub of https://github.com/git/git/, I >> think preparing a pull request against it triggers the CI. > > ...in this case though there's no reason to wait for the glacially slow > GitHub CI. You just need spatch installed and: > > make coccicheck > > Sergey: If you've hacked on the (I'm assuming linux) kernel you likely > have that installed already, they use it too (being the canonical and I > believe original use-case for it). Thanks a lot! I'll definitely give it a try even though I didn't use it while hacking on the Linux kernel. -- Sergey Organov
Jeff King <peff@peff.net> writes: > On Mon, Nov 28, 2022 at 05:44:29PM +0300, Sergey Organov wrote: > >> >> + if (set_func != NULL) { >> > >> > Please write it like so: >> > >> > if (set_func) { >>[...] >> Thanks! Do we now have tool for auto-check for these issues? I still use >> one from Linux kernel, and it didn't object to this form. > > Running "make coccicheck" will find this, but of course you need to have > coccinelle installed. Note that if it finds anything, "make" won't > report an error. You have to check for non-empty files in > contrib/coccinelle/*.patch. Thank you! I'll give it a try. -- Sergey Organov
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> If you have your own fork at GitHub of https://github.com/git/git/, I >> think preparing a pull request against it triggers the CI. > > ...in this case though there's no reason to wait for the glacially slow > GitHub CI. You just need spatch installed and: > > make coccicheck And you have to wait for the glacially slow coccicheck run locally, though ;-).
diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt index 265a57312e58..7452c7fad638 100644 --- a/Documentation/config/log.txt +++ b/Documentation/config/log.txt @@ -43,6 +43,17 @@ log.diffMergesHide:: log.diffMerges-m-imply-p:: `true` enables implication of `-p` by `-m`. +log.diffMergesForce:: + Use specified log format for -c, --cc, and --remerge-diff + options instead of their respective formats when the option + appears on the command line one time. See `--diff-merges` in + linkgit:git-log[1] for allowed values. Using 'off' or 'none' + disables the override (default). ++ +The override is useful when external tool hard-codes one of the above +options. Using any of these options two (or more) times will get back +the original meaning of the options. + log.follow:: If `true`, `git log` will act as if the `--follow` option was used when a single <path> is given. This has the same limitations as `--follow`, diff --git a/builtin/log.c b/builtin/log.c index 332b5e478cc5..1e8d0a2545a9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -585,6 +585,8 @@ static int git_log_config(const char *var, const char *value, void *cb) return diff_merges_hide_config(git_config_bool(var, value)); if (!strcmp(var, "log.diffmerges-m-imply-p")) return diff_merges_m_imply_p_config(git_config_bool(var, value)); + if (!strcmp(var, "log.diffmergesforce")) + return diff_merges_force_config(value); if (!strcmp(var, "log.showroot")) { default_show_root = git_config_bool(var, value); return 0; diff --git a/diff-merges.c b/diff-merges.c index 1fbf476d378e..cedd7652bf42 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -6,6 +6,7 @@ typedef void (*diff_merges_setup_func_t)(struct rev_info *); static void set_separate(struct rev_info *revs); static diff_merges_setup_func_t set_to_default = set_separate; +static diff_merges_setup_func_t force_func = NULL; static int suppress_m_parsing; static int hide = 0; static int m_imply_p = 0; @@ -150,6 +151,21 @@ int diff_merges_m_imply_p_config(int on) return 0; } +int diff_merges_force_config(const char *value) +{ + diff_merges_setup_func_t func = func_by_opt(value); + + if (!func) + return -1; + + if (func == set_none) + force_func = NULL; + else if (func != set_hide && func != set_no_hide) + force_func = func; + + return 0; +} + void diff_merges_suppress_m_parsing(void) { suppress_m_parsing = 1; @@ -160,20 +176,18 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) int argcount = 1; const char *optarg; const char *arg = argv[0]; + diff_merges_setup_func_t set_func = NULL; if (!suppress_m_parsing && !strcmp(arg, "-m")) { set_to_default(revs); set_hide(revs); revs->merges_imply_patch = m_imply_p; } else if (!strcmp(arg, "-c")) { - set_combined(revs); - revs->merges_imply_patch = 1; + set_func = set_combined; } else if (!strcmp(arg, "--cc")) { - set_dense_combined(revs); - revs->merges_imply_patch = 1; + set_func = set_dense_combined; } else if (!strcmp(arg, "--remerge-diff")) { - set_remerge_diff(revs); - revs->merges_imply_patch = 1; + set_func = set_remerge_diff; } else if (!strcmp(arg, "--no-diff-merges")) { set_none(revs); } else if (!strcmp(arg, "--combined-all-paths")) { @@ -183,6 +197,12 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv) } else return 0; + if (set_func != NULL) { + (force_func ? force_func : set_func)(revs); + force_func = NULL; + revs->merges_imply_patch = 1; + } + revs->explicit_diff_merges = 1; return argcount; } diff --git a/diff-merges.h b/diff-merges.h index 9f0b3901fe82..6ef0cc87bb2a 100644 --- a/diff-merges.h +++ b/diff-merges.h @@ -15,6 +15,8 @@ int diff_merges_hide_config(int hide); int diff_merges_m_imply_p_config(int on); +int diff_merges_force_config(const char *value); + void diff_merges_suppress_m_parsing(void); int diff_merges_parse_opts(struct rev_info *revs, const char **argv); diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index 1789dd6063c5..8a90d2dac360 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -557,6 +557,24 @@ test_expect_success 'git config log.diffMerges-m-imply-p has proper effect' ' test_cmp expected actual ' +test_expect_success 'git config log.diffMergesForce has proper effect' ' + git log -m -p master >result && + process_diffs result >expected && + test_config log.diffMergesForce on && + git log --cc master >result && + process_diffs result >actual && + test_cmp expected actual +' + +test_expect_success 'git config log.diffMergesForce override by duplicate' ' + git log --cc master >result && + process_diffs result >expected && + test_config log.diffMergesForce on && + git log --cc --cc master >result && + process_diffs result >actual && + test_cmp expected actual +' + # -m in "git diff-index" means "match missing", that differs # from its meaning in "git diff". Let's check it in diff-index. # The line in the output for removed file should disappear when diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 26a7e4ff877c..411e08b2fa1b 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -2498,6 +2498,7 @@ test_expect_success 'git config - variable name' ' log.decorate Z log.diffMerges Z log.diffMergesHide Z + log.diffMergesForce Z log.diffMerges-m-imply-p Z EOF ' @@ -2528,6 +2529,7 @@ test_expect_success 'git -c - variable name' ' log.decorate=Z log.diffMerges=Z log.diffMergesHide=Z + log.diffMergesForce=Z log.diffMerges-m-imply-p=Z EOF ' @@ -2552,6 +2554,7 @@ test_expect_success 'git clone --config= - variable name' ' log.decorate=Z log.diffMerges=Z log.diffMergesHide=Z + log.diffMergesForce=Z log.diffMerges-m-imply-p=Z EOF '
Force specified log format for -c, --cc, and --remerge-diff options instead of their respective formats. The override is useful when some external tool hard-codes diff for merges format option. Using any of the above options twice or more will get back the original meaning of the option no matter what configuration says. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- Documentation/config/log.txt | 11 +++++++++++ builtin/log.c | 2 ++ diff-merges.c | 32 ++++++++++++++++++++++++++------ diff-merges.h | 2 ++ t/t4013-diff-various.sh | 18 ++++++++++++++++++ t/t9902-completion.sh | 3 +++ 6 files changed, 62 insertions(+), 6 deletions(-)