Hi Alex
On 23/02/2023 05:34, Alex Henrie wrote:
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@xxxxxxxxx>
---
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.
[...]
+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
> [...]
@@ -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
[...]
+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
[...]
+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
