Re: [PATCH v2 4/4] rebase: add a config option for --rebase-merges
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.
Show 19 quoted lines
> 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.
Show 26 quoted lines
> 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.
Show 6 quoted lines
> + 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)
Show 13 quoted lines
> 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.
Show 77 quoted lines
> + 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
Show 11 quoted lines
> 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 &&
Alex Henrie