| Message ID | 20230221055805.210951-4-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] rebase: document the --no-rebase-merges option | expand |
Hi Alex On 21/02/2023 05:58, 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. Thanks for improving the commit message, this seems like a reasonable change. > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > Documentation/config/rebase.txt | 3 ++ > Documentation/git-rebase.txt | 3 +- > builtin/rebase.c | 11 +++++ > t/t3430-rebase-merges.sh | 81 +++++++++++++++++++++++++++++++++ > 4 files changed, 97 insertions(+), 1 deletion(-) > > diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt > index f19bd0e040..d956ec4441 100644 > --- a/Documentation/config/rebase.txt > +++ b/Documentation/config/rebase.txt > @@ -67,3 +67,6 @@ rebase.rescheduleFailedExec:: > > rebase.forkPoint:: > If set to false set `--no-fork-point` option by default. > + > +rebase.merges:: > + Default value of `--rebase-merges` option. Below I see this takes a boolean in addition to [no-]rebase-cousins, it would be nice to document that, especially what "true" means. > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index c98784a0d2..b02f9cbb8c 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -537,7 +537,8 @@ See also INCOMPATIBLE OPTIONS below. > by recreating the merge commits. Any resolved merge conflicts or > manual amendments in these merge commits will have to be > resolved/re-applied manually. `--no-rebase-merges` can be used to > - countermand a previous `--rebase-merges`. > + countermand both the `rebase.merges` config option and a previous > + `--rebase-merges`. > + > By default, or when `no-rebase-cousins` was specified, commits which do not > have `<upstream>` as direct ancestor will keep their original branch point, > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 0a8366f30f..35f3837f43 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -829,6 +829,17 @@ static int rebase_config(const char *var, const char *value, void *data) > return 0; > } > > + if (!strcmp(var, "rebase.merges")) { > + const char *rebase_merges; > + if (!git_config_string(&rebase_merges, var, value) && > + rebase_merges && *rebase_merges) { rebase_merges is guaranteed to be non-NULL if git_config_string returns 0. > + opts->rebase_merges = git_parse_maybe_bool(rebase_merges); > + if (opts->rebase_merges < 0) > + parse_merges_value(opts, rebase_merges); > + } > + return 0; > + } I think this leaks rebase_merges as git_config_string() returns a copy despite taking a "const char*". If we're adding a new config option that is incompatible with the apply backend we should add some logic to error out if the config is set and any of the apply options are given - see eddfcd8ece (rebase: provide better error message for apply options vs. merge config, 2023-01-25) > if (!strcmp(var, "rebase.backend")) { > return git_config_string(&opts->default_backend, var, value); > } > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index b8ba323dbc..4a7193d501 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -299,6 +299,86 @@ test_expect_success '--rebase-merges="" is invalid syntax' ' > test_cmp expect actual > ' > > +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' ' > + git config rebase.merges "" && test_config is generally preferred as it clears the value at the end of the test. Then you don't need the final hunk of this patch. > + git checkout -b config-merges-blank E && > + git rebase C && > + test_cmp_graph C.. <<-\EOF > + * B > + * D > + o C > + EOF > +' > + > +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' ' > + git config rebase.merges rebase-cousins && > + git checkout -b config-rebase-cousins main && > + git rebase HEAD^ && > + test_cmp_graph HEAD^.. <<-\EOF > + * Merge the topic branch '\''onebranch'\'' > + |\ > + | * D > + | * G > + |/ > + o H > + EOF > +' > + > +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' ' > + git config rebase.merges no-rebase-cousins && > + git checkout -b override-config-no-rebase-cousins E && > + git rebase --no-rebase-merges C && > + test_cmp_graph C.. <<-\EOF > + * B > + * D > + o C > + EOF > +' > + > +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' ' > + git config rebase.merges rebase-cousins && > + git checkout -b override-config-rebase-cousins main && > + git rebase --rebase-merges=no-rebase-cousins HEAD^ && > + test_cmp_graph HEAD^.. <<-\EOF > + * Merge the topic branch '\''onebranch'\'' > + |\ > + | * D > + | * G > + o | H > + |/ > + o A > + EOF > +' > + > +test_expect_success '--rebase-merges overrides rebase.merges=false' ' > + git config rebase.merges false && > + git checkout -b override-config-merges-false main && > + git rebase --rebase-merges HEAD^ && > + test_cmp_graph HEAD^.. <<-\EOF > + * Merge the topic branch '\''onebranch'\'' > + |\ > + | * D > + | * G > + o | H > + |/ > + o A > + EOF > +' > + > +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' ' > + git config rebase.merges rebase-cousins && > + git checkout -b no-override-config-rebase-cousins main && > + git rebase --rebase-merges HEAD^ && > + test_cmp_graph HEAD^.. <<-\EOF > + * Merge the topic branch '\''onebranch'\'' > + |\ > + | * D > + | * G > + |/ > + o H > + EOF > +' Thanks for the tests, I think they provide good coverage of the various combinations of config and commandline settings Best Wishes Phillip > test_expect_success 'refs/rewritten/* is worktree-local' ' > git worktree add wt && > cat >wt/script-from-scratch <<-\EOF && > @@ -409,6 +489,7 @@ test_expect_success 'a "merge" into a root commit is a fast-forward' ' > ' > > test_expect_success 'A root commit can be a cousin, treat it that way' ' > + git config --unset rebase.merges && > git checkout --orphan khnum && > test_commit yama && > git checkout -b asherah main &&
On Tue, Feb 21, 2023 at 3:46 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 21/02/2023 05:58, Alex Henrie wrote: > > +rebase.merges:: > > + Default value of `--rebase-merges` option. > > Below I see this takes a boolean in addition to [no-]rebase-cousins, it > would be nice to document that, especially what "true" means. Good point. I'll add another sentence explaining that "true" is equivalent to --rebase-merges without arguments and "false" is equivalent to --no-rebase-merges. > > + if (!strcmp(var, "rebase.merges")) { > > + const char *rebase_merges; > > + if (!git_config_string(&rebase_merges, var, value) && > > + rebase_merges && *rebase_merges) { > > rebase_merges is guaranteed to be non-NULL if git_config_string returns 0. > > > + opts->rebase_merges = git_parse_maybe_bool(rebase_merges); > > + if (opts->rebase_merges < 0) > > + parse_merges_value(opts, rebase_merges); > > + } > > + return 0; > > + } > > I think this leaks rebase_merges as git_config_string() returns a copy > despite taking a "const char*". Come to think of it, I don't think we need git_config_string at all here. As far as I can tell, it should be fine to just check (value && *value) and not duplicate the string in memory. > > +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' ' > > + git config rebase.merges "" && > > test_config is generally preferred as it clears the value at the end of > the test. Then you don't need the final hunk of this patch. I've used test_config before but I forgot about it. Thanks for the reminder. -Alex
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt index f19bd0e040..d956ec4441 100644 --- a/Documentation/config/rebase.txt +++ b/Documentation/config/rebase.txt @@ -67,3 +67,6 @@ rebase.rescheduleFailedExec:: rebase.forkPoint:: If set to false set `--no-fork-point` option by default. + +rebase.merges:: + Default value of `--rebase-merges` option. diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index c98784a0d2..b02f9cbb8c 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -537,7 +537,8 @@ See also INCOMPATIBLE OPTIONS below. by recreating the merge commits. Any resolved merge conflicts or manual amendments in these merge commits will have to be resolved/re-applied manually. `--no-rebase-merges` can be used to - countermand a previous `--rebase-merges`. + countermand both the `rebase.merges` config option and a previous + `--rebase-merges`. + By default, or when `no-rebase-cousins` was specified, commits which do not have `<upstream>` as direct ancestor will keep their original branch point, diff --git a/builtin/rebase.c b/builtin/rebase.c index 0a8366f30f..35f3837f43 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -829,6 +829,17 @@ static int rebase_config(const char *var, const char *value, void *data) return 0; } + if (!strcmp(var, "rebase.merges")) { + const char *rebase_merges; + if (!git_config_string(&rebase_merges, var, value) && + rebase_merges && *rebase_merges) { + opts->rebase_merges = git_parse_maybe_bool(rebase_merges); + if (opts->rebase_merges < 0) + parse_merges_value(opts, rebase_merges); + } + return 0; + } + if (!strcmp(var, "rebase.backend")) { return git_config_string(&opts->default_backend, var, value); } diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index b8ba323dbc..4a7193d501 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -299,6 +299,86 @@ test_expect_success '--rebase-merges="" is invalid syntax' ' test_cmp expect actual ' +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' ' + git config rebase.merges "" && + git checkout -b config-merges-blank E && + git rebase C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' ' + git config rebase.merges rebase-cousins && + git checkout -b config-rebase-cousins main && + git rebase HEAD^ && + test_cmp_graph HEAD^.. <<-\EOF + * Merge the topic branch '\''onebranch'\'' + |\ + | * D + | * G + |/ + o H + EOF +' + +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' ' + git config rebase.merges no-rebase-cousins && + git checkout -b override-config-no-rebase-cousins E && + git rebase --no-rebase-merges C && + test_cmp_graph C.. <<-\EOF + * B + * D + o C + EOF +' + +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' ' + git config rebase.merges rebase-cousins && + git checkout -b override-config-rebase-cousins main && + git rebase --rebase-merges=no-rebase-cousins HEAD^ && + test_cmp_graph HEAD^.. <<-\EOF + * Merge the topic branch '\''onebranch'\'' + |\ + | * D + | * G + o | H + |/ + o A + EOF +' + +test_expect_success '--rebase-merges overrides rebase.merges=false' ' + git config rebase.merges false && + git checkout -b override-config-merges-false main && + git rebase --rebase-merges HEAD^ && + test_cmp_graph HEAD^.. <<-\EOF + * Merge the topic branch '\''onebranch'\'' + |\ + | * D + | * G + o | H + |/ + o A + EOF +' + +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' ' + git config rebase.merges rebase-cousins && + git checkout -b no-override-config-rebase-cousins main && + git rebase --rebase-merges HEAD^ && + test_cmp_graph HEAD^.. <<-\EOF + * Merge the topic branch '\''onebranch'\'' + |\ + | * D + | * G + |/ + o H + EOF +' + test_expect_success 'refs/rewritten/* is worktree-local' ' git worktree add wt && cat >wt/script-from-scratch <<-\EOF && @@ -409,6 +489,7 @@ test_expect_success 'a "merge" into a root commit is a fast-forward' ' ' test_expect_success 'A root commit can be a cousin, treat it that way' ' + git config --unset rebase.merges && git checkout --orphan khnum && test_commit yama && git checkout -b asherah main &&
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 | 3 ++ Documentation/git-rebase.txt | 3 +- builtin/rebase.c | 11 +++++ t/t3430-rebase-merges.sh | 81 +++++++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+), 1 deletion(-)