Re: [PATCH v4 3/3] rebase: add a config option for --rebase-merges
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Feb 24, 2023, 14:55 UTC
- Message-ID
- <7ba8a92d-8c94-5226-5416-6ed3a8e2e389@dunelm.org.uk>
- In-Reply-To
- <20230223053410.644503-3-alexhenrie24@gmail.com>
Hi Alex
On 23/02/2023 05:34, Alex Henrie wrote:
Show 31 quoted lines
> The purpose of the new option is to accommodate users who would like > --rebase-merges to be on by default and to facilitate possibly turning > on --rebase-merges by default without configuration in a future version > of Git. > > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > Documentation/config/rebase.txt | 10 ++++ > Documentation/git-rebase.txt | 3 +- > builtin/rebase.c | 47 ++++++++++++---- > t/t3430-rebase-merges.sh | 96 +++++++++++++++++++++++++++++++++ > 4 files changed, 144 insertions(+), 12 deletions(-) > > diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt > index f19bd0e040..308baa9dbb 100644 > --- a/Documentation/config/rebase.txt > +++ b/Documentation/config/rebase.txt > @@ -67,3 +67,13 @@ rebase.rescheduleFailedExec:: > > rebase.forkPoint:: > If set to false set `--no-fork-point` option by default. > + > +rebase.merges:: > + Whether and how to set the `--rebase-merges` option by default. Can > + be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to > + true is equivalent to `--rebase-merges` without an argument, setting to > + `rebase-cousins` or `no-rebase-cousins` is equivalent to > + `--rebase-merges` with that value as its argument, and setting to false > + is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the > + command line without an argument overrides a `rebase.merges=false` > + configuration but does not override other values of `rebase.merge`.
Thanks for updating this, it is much clearer what the different settings mean now. I not sure if having the config setting override the default when the user passes --rebase-merges without an argument is a good idea.
Show 14 quoted lines
> [...]
> +static void parse_merges_value(struct rebase_options *options, const char *value)
> +{
> + if (value) {
> + if (!strcmp("no-rebase-cousins", value))
> + options->rebase_cousins = 0;
> + else if (!strcmp("rebase-cousins", value))
> + options->rebase_cousins = 1;
> + else
> + die(_("Unknown mode: %s"), value);
> + }
> +
> + options->rebase_merges = 1;
> +}It's a shame we seem to have grown yet another callback since v2, I'm not sure we should need to add quite so much code just to support a new config option
> [...]
Show 13 quoted lines
> @@ -1436,14 +1467,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.exec.nr)
> imply_merge(&options, "--exec");
>
> - if (rebase_merges) {
> - if (!strcmp("rebase-cousins", rebase_merges))
> - options.rebase_cousins = 1;
> - else if (strcmp("no-rebase-cousins", rebase_merges))
> - die(_("Unknown mode: %s"), rebase_merges);
> - options.rebase_merges = 1;
> + if (options.rebase_merges)
> imply_merge(&options, "--rebase-merges");
> - }As I have said before I really think this patch needs to follow the lead of eddfcd8ece (rebase: provide better error message for apply options vs. merge config, 2023-01-25) and provide an error message if --rebase-merges is enabled by rebase.merges and the user provides a command line option that requires the apply backend. So
git -c rebase.merges=true rebase --whitespace=fix
would result in
error: apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges
Show 5 quoted lines
> [...] > +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' ' > + test_config rebase.merges rebase-cousins && > + git checkout -b no-override-config-rebase-cousins main && > + git rebase --rebase-merges HEAD^ &&
I think this behavior is confusing for users and will break scripts that quite reasonably assume --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins
Show 22 quoted lines
> [...] > +test_expect_success 'local rebase.merges=false overrides global rebase.merges=true' ' > + test_config_global rebase.merges true && > + test_config rebase.merges false && > + git checkout -b override-global-config E && > + git rebase C && > + test_cmp_graph C.. <<-\EOF > + * B > + * D > + o C > + EOF > +' > + > +test_expect_success 'local rebase.merges="" does not override global rebase.merges=true' ' > + test_config_global rebase.merges no-rebase-cousins && > + test_config rebase.merges "" && > + git checkout -b no-override-global-config E && > + before="$(git rev-parse --verify HEAD)" && > + test_tick && > + git rebase C && > + test_cmp_rev HEAD $before > +'
These two tests seem to be testing the config subsystem rather than this patch. As Dscho has pointed out it is important to get a balance between test coverage and test run time. I think these two tests can definitely be dropped.
Best Wishes
Phillip