* [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
@ 2023-03-05 5:07 ` Alex Henrie
2023-03-08 22:25 ` Sergey Organov
2023-03-05 5:07 ` [PATCH v6 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
` (5 subsequent siblings)
6 siblings, 1 reply; 96+ messages in thread
From: Alex Henrie @ 2023-03-05 5:07 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
As far as I can tell, --no-rebase-merges has always worked, but has
never been documented. It is especially important to document it before
a rebase.rebaseMerges option is introduced so that users know how to
override the config option on the command line. It's also important to
clarify that --rebase-merges without an argument is not the same as
--no-rebase-merges and not passing --rebase-merges is not the same as
passing --rebase-merges=no-rebase-cousins.
A test case is necessary to make sure that --no-rebase-merges keeps
working after its code is refactored in the following patches of this
series. The test case is a little contrived: It's unlikely that a user
would type both --rebase-merges and --no-rebase-merges at the same time.
However, if an alias is defined which includes --rebase-merges, the user
might decide to add --no-rebase-merges to countermand that part of the
alias but leave alone other flags set by the alias.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/git-rebase.txt | 18 +++++++++++-------
t/t3430-rebase-merges.sh | 10 ++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9a295bcee4..4e57a87624 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -529,20 +529,24 @@ See also INCOMPATIBLE OPTIONS below.
-r::
--rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
+--no-rebase-merges::
By default, a rebase will simply drop merge commits from the todo
list, and put the rebased commits into a single, linear branch.
With `--rebase-merges`, the rebase will instead try to preserve
the branching structure within the commits that are to be rebased,
by recreating the merge commits. Any resolved merge conflicts or
manual amendments in these merge commits will have to be
- resolved/re-applied manually.
+ resolved/re-applied manually. `--no-rebase-merges` can be used to
+ countermand 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,
-i.e. commits that would be excluded by linkgit:git-log[1]'s
-`--ancestry-path` option will keep their original ancestry by default. If
-the `rebase-cousins` mode is turned on, such commits are instead rebased
-onto `<upstream>` (or `<onto>`, if specified).
+When rebasing merges, there are two modes: `rebase-cousins` and
+`no-rebase-cousins`. If the mode is not specified, it defaults to
+`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
+`<upstream>` as direct ancestor will keep their original branch point, i.e.
+commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
+option will keep their original ancestry by default. In `rebase-cousins` mode,
+such commits are instead rebased onto `<upstream>` (or `<onto>`, if
+specified).
+
It is currently only possible to recreate the merge commits using the
`ort` merge strategy; different merge strategies can be used only via
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index fa2a06c19f..d46d9545f2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -250,6 +250,16 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
EOF
'
+test_expect_success '--no-rebase-merges countermands --rebase-merges' '
+ git checkout -b no-rebase-merges E &&
+ git rebase --rebase-merges --no-rebase-merges C &&
+ test_cmp_graph C.. <<-\EOF
+ * B
+ * D
+ o C
+ EOF
+'
+
test_expect_success 'do not rebase cousins unless asked for' '
git checkout -b cousins main &&
before="$(git rev-parse --verify HEAD)" &&
--
2.39.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges
2023-03-05 5:07 ` [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
@ 2023-03-08 22:25 ` Sergey Organov
0 siblings, 0 replies; 96+ messages in thread
From: Sergey Organov @ 2023-03-08 22:25 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
chooglen, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
[...]
> +When rebasing merges, there are two modes: `rebase-cousins` and
> +`no-rebase-cousins`. If the mode is not specified, it defaults to
> +`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
> +`<upstream>` as direct ancestor will keep their original branch point, i.e.
I realize this is in fact unchanged from the original, so is not exactly
material of these series, but what is the meaning of "direct ancestor"?
Is it just "parent"?
> +commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
> +option will keep their original ancestry by default.
Excluded when --ancestry-path is applied to what commit, exactly? To the
commit being considered for rebase, or to the <upstream>, or to the
'fork_point'?
Please notice that rebase claims to operate either on <upstream>..HEAD or
'fork_point'..HEAD range, and --ancestry-path without arguments applies
to the left commit of the range when used in "git log".
Looks like some clarifications are needed here, even though maybe not in
these series?
Thanks,
-- Sergey Organov
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v6 2/3] rebase: deprecate --rebase-merges=""
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-05 5:07 ` [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
@ 2023-03-05 5:07 ` Alex Henrie
2023-03-07 14:59 ` Phillip Wood
2023-03-05 5:07 ` [PATCH v6 3/3] rebase: add a config option for --rebase-merges Alex Henrie
` (4 subsequent siblings)
6 siblings, 1 reply; 96+ messages in thread
From: Alex Henrie @ 2023-03-05 5:07 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
empty string argument) has been an undocumented synonym of
--rebase-merges without an argument. Deprecate that syntax to avoid
confusion when a rebase.rebaseMerges config option is introduced, where
rebase.rebaseMerges="" will be equivalent to --no-rebase-merges.
It is not likely that anyone is actually using this syntax, but just in
case, deprecate the empty string argument instead of dropping support
for it immediately.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
builtin/rebase.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6635f10d52..c36ddc0050 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)""},
+ PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1438,7 +1438,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (rebase_merges) {
if (!*rebase_merges)
- ; /* default mode; do nothing */
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
else if (!strcmp("rebase-cousins", rebase_merges))
options.rebase_cousins = 1;
else if (strcmp("no-rebase-cousins", rebase_merges))
--
2.39.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v6 2/3] rebase: deprecate --rebase-merges=""
2023-03-05 5:07 ` [PATCH v6 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
@ 2023-03-07 14:59 ` Phillip Wood
0 siblings, 0 replies; 96+ messages in thread
From: Phillip Wood @ 2023-03-07 14:59 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, chooglen, calvinwan,
jonathantanmy
Hi Alex
On 05/03/2023 05:07, Alex Henrie wrote:
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6635f10d52..c36ddc0050 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> {OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
> N_("mode"),
> N_("try to rebase merges instead of skipping them"),
> - PARSE_OPT_OPTARG, NULL, (intptr_t)""},
> + PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
I've just realized there is a subtle behavior change here. If the user
passes "--rebase-merges=rebase-cousins --rebase-merges" we used to
rebase cousins but now we wont. The next patch goes back to the old
behavior so I don't think we need to worry about it.
Best Wishes
Phillip
> OPT_BOOL(0, "fork-point", &options.fork_point,
> N_("use 'merge-base --fork-point' to refine upstream")),
> OPT_STRING('s', "strategy", &options.strategy,
> @@ -1438,7 +1438,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
> if (rebase_merges) {
> if (!*rebase_merges)
> - ; /* default mode; do nothing */
> + warning(_("--rebase-merges with an empty string "
> + "argument is deprecated and will stop "
> + "working in a future version of Git. Use "
> + "--rebase-merges without an argument "
> + "instead, which does the same thing."));
> else if (!strcmp("rebase-cousins", rebase_merges))
> options.rebase_cousins = 1;
> else if (strcmp("no-rebase-cousins", rebase_merges))
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-05 5:07 ` [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-05 5:07 ` [PATCH v6 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
@ 2023-03-05 5:07 ` Alex Henrie
2023-03-07 14:56 ` Phillip Wood
2023-03-08 0:02 ` Glen Choo
2023-03-05 12:22 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Sergey Organov
` (3 subsequent siblings)
6 siblings, 2 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-05 5:07 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
The purpose of the new option is to accommodate users who would like
--rebase-merges to be on by default and to facilitate turning on
--rebase-merges by default without configuration in a future version of
Git.
Name the new option rebase.rebaseMerges, even though it is a little
redundant, for consistency with the name of the command line option and
to be clear when scrolling through values in the [rebase] section of
.gitconfig.
In the future, the default rebase-merges mode may change from
no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
to the nonspecific value "true" for users who do not need or want to
care about the default changing in the future. Similarly, for users who
have --rebase-merges in an alias and want to get the future behavior
now, use the specific rebase-merges mode from the config if a specific
mode is not given on the command line.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/config/rebase.txt | 11 ++++
Documentation/git-rebase.txt | 17 +++---
builtin/rebase.c | 79 ++++++++++++++++++--------
t/t3422-rebase-incompatible-options.sh | 17 ++++++
t/t3430-rebase-merges.sh | 68 ++++++++++++++++++++++
5 files changed, 161 insertions(+), 31 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f19bd0e040..f7d3218b1d 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -67,3 +67,14 @@ rebase.rescheduleFailedExec::
rebase.forkPoint::
If set to false set `--no-fork-point` option by default.
+
+rebase.rebaseMerges::
+ 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.rebaseMerges=false`
+ configuration, but the absence of a specific rebase-merges mode on the
+ command line does not counteract a specific mode set in the configuration.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 4e57a87624..6ec86c9c6e 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -537,16 +537,17 @@ 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.rebaseMerges` config option and a previous
+ `--rebase-merges`.
+
When rebasing merges, there are two modes: `rebase-cousins` and
-`no-rebase-cousins`. If the mode is not specified, it defaults to
-`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
-`<upstream>` as direct ancestor will keep their original branch point, i.e.
-commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
-option will keep their original ancestry by default. In `rebase-cousins` mode,
-such commits are instead rebased onto `<upstream>` (or `<onto>`, if
-specified).
+`no-rebase-cousins`. If the mode is not specified on the command line or in
+the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
+In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
+ancestor will keep their original branch point, i.e. commits that would be
+excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
+original ancestry by default. In `rebase-cousins` mode, such commits are
+instead rebased onto `<upstream>` (or `<onto>`, if specified).
+
It is currently only possible to recreate the merge commits using the
`ort` merge strategy; different merge strategies can be used only via
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c36ddc0050..04f815e3d0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -123,6 +123,7 @@ struct rebase_options {
int fork_point;
int update_refs;
int config_autosquash;
+ int config_rebase_merges;
int config_update_refs;
};
@@ -140,6 +141,8 @@ struct rebase_options {
.allow_empty_message = 1, \
.autosquash = -1, \
.config_autosquash = -1, \
+ .rebase_merges = -1, \
+ .config_rebase_merges = -1, \
.update_refs = -1, \
.config_update_refs = -1, \
}
@@ -771,6 +774,16 @@ static int run_specific_rebase(struct rebase_options *opts)
return status ? -1 : 0;
}
+static void parse_rebase_merges_value(struct rebase_options *options, const char *value)
+{
+ if (!strcmp("no-rebase-cousins", value))
+ options->rebase_cousins = 0;
+ else if (!strcmp("rebase-cousins", value))
+ options->rebase_cousins = 1;
+ else
+ die(_("Unknown rebase-merges mode: %s"), value);
+}
+
static int rebase_config(const char *var, const char *value, void *data)
{
struct rebase_options *opts = data;
@@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
return 0;
}
+ if (!strcmp(var, "rebase.rebasemerges")) {
+ opts->config_rebase_merges = git_parse_maybe_bool(value);
+ if (opts->config_rebase_merges < 0) {
+ opts->config_rebase_merges = 1;
+ parse_rebase_merges_value(opts, value);
+ }
+ return 0;
+ }
+
if (!strcmp(var, "rebase.updaterefs")) {
opts->config_update_refs = git_config_bool(var, value);
return 0;
@@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
return 0;
}
+static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
+{
+ struct rebase_options *options = opt->value;
+
+ options->rebase_merges = !unset;
+
+ if (arg) {
+ if (!*arg) {
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
+ return 0;
+ }
+ parse_rebase_merges_value(options, arg);
+ }
+
+ return 0;
+}
+
static void NORETURN error_on_missing_default_upstream(void)
{
struct branch *current_branch = branch_get(NULL);
@@ -1035,7 +1078,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
struct object_id branch_base;
int ignore_whitespace = 0;
const char *gpg_sign = NULL;
- const char *rebase_merges = NULL;
struct string_list strategy_options = STRING_LIST_INIT_NODUP;
struct object_id squash_onto;
char *squash_onto_name = NULL;
@@ -1137,10 +1179,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
&options.allow_empty_message,
N_("allow rebasing commits with empty messages"),
PARSE_OPT_HIDDEN),
- {OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
- N_("mode"),
+ OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
+ PARSE_OPT_OPTARG, parse_opt_rebase_merges),
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1436,21 +1477,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.exec.nr)
imply_merge(&options, "--exec");
- if (rebase_merges) {
- if (!*rebase_merges)
- warning(_("--rebase-merges with an empty string "
- "argument is deprecated and will stop "
- "working in a future version of Git. Use "
- "--rebase-merges without an argument "
- "instead, which does the same thing."));
- else 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;
- imply_merge(&options, "--rebase-merges");
- }
-
if (options.type == REBASE_APPLY) {
if (ignore_whitespace)
strvec_push(&options.git_am_opts,
@@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
if (i >= 0 || options.type == REBASE_APPLY) {
- if (is_merge(&options))
- die(_("apply options and merge options "
- "cannot be used together"));
- else if (options.autosquash == -1 && options.config_autosquash == 1)
+ if (options.autosquash == -1 && options.config_autosquash == 1)
die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash"));
+ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
+ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
+ else if (is_merge(&options))
+ die(_("apply options and merge options "
+ "cannot be used together"));
else
options.type = REBASE_APPLY;
}
@@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.update_refs = (options.update_refs >= 0) ? options.update_refs :
((options.config_update_refs >= 0) ? options.config_update_refs : 0);
+ if (options.rebase_merges == 1)
+ imply_merge(&options, "--rebase-merges");
+ options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
+ ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
+
if (options.autosquash == 1)
imply_merge(&options, "--autosquash");
options.autosquash = (options.autosquash >= 0) ? options.autosquash :
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 4711b37a28..2eba00bdf5 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -85,6 +85,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --reapply-cherry-picks A
"
+ test_expect_success "$opt incompatible with --rebase-merges" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --rebase-merges A
+ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
test_must_fail git rebase $opt --update-refs A
@@ -101,6 +106,12 @@ test_rebase_am_only () {
grep -e --no-autosquash err
"
+ test_expect_success "$opt incompatible with rebase.rebaseMerges" "
+ git checkout B^0 &&
+ test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
+ grep -e --no-rebase-merges err
+ "
+
test_expect_success "$opt incompatible with rebase.updateRefs" "
git checkout B^0 &&
test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
@@ -113,6 +124,12 @@ test_rebase_am_only () {
git -c rebase.autosquash=true rebase --no-autosquash $opt A
"
+ test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
+ test_when_finished \"git reset --hard B^0\" &&
+ git checkout B^0 &&
+ git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
+ "
+
test_expect_success "$opt okay with overridden rebase.updateRefs" "
test_when_finished \"git reset --hard B^0\" &&
git checkout B^0 &&
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index d46d9545f2..aa75e192d1 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' '
EOF
'
+test_expect_success '--rebase-merges="" is deprecated' '
+ git rebase --rebase-merges="" HEAD^ 2>actual &&
+ grep deprecated actual
+'
+
+test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' '
+ test_config rebase.rebaseMerges 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.rebaseMerges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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.rebaseMerges=false' '
+ test_config rebase.rebaseMerges false &&
+ git checkout -b override-config-merges-false E &&
+ before="$(git rev-parse --verify HEAD)" &&
+ test_tick &&
+ git rebase --rebase-merges C &&
+ test_cmp_rev HEAD $before
+'
+
+test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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 &&
--
2.39.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-03-07 14:56 ` Phillip Wood
2023-03-07 18:32 ` Junio C Hamano
2023-03-08 0:09 ` Glen Choo
2023-03-08 0:02 ` Glen Choo
1 sibling, 2 replies; 96+ messages in thread
From: Phillip Wood @ 2023-03-07 14:56 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, chooglen, calvinwan,
jonathantanmy
Hi Alex
On 05/03/2023 05:07, 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 turning on
> --rebase-merges by default without configuration in a future version of
> Git.
>
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.
>
> In the future, the default rebase-merges mode may change from
> no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
> to the nonspecific value "true" for users who do not need or want to
> care about the default changing in the future. Similarly, for users who
> have --rebase-merges in an alias and want to get the future behavior
> now, use the specific rebase-merges mode from the config if a specific
> mode is not given on the command line.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> Documentation/config/rebase.txt | 11 ++++
> Documentation/git-rebase.txt | 17 +++---
> builtin/rebase.c | 79 ++++++++++++++++++--------
> t/t3422-rebase-incompatible-options.sh | 17 ++++++
> t/t3430-rebase-merges.sh | 68 ++++++++++++++++++++++
> 5 files changed, 161 insertions(+), 31 deletions(-)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..f7d3218b1d 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,14 @@ rebase.rescheduleFailedExec::
>
> rebase.forkPoint::
> If set to false set `--no-fork-point` option by default.
> +
> +rebase.rebaseMerges::
> + 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,
This is a bit picky but how can rebase.rebaseMerges=true be equivalent
to --rebase-merges without an argument when the behavior of
--rebase-merges without an argument depends on the value of
rebase.rebaseMerges?
> 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.rebaseMerges=false`
> + configuration, but the absence of a specific rebase-merges mode on the
> + command line does not counteract a specific mode set in the configuration.
I may not agree the with the design choice but the documentation here
and below is very clear about the behavior of --rebase-merges without an
argument which is good.
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 4e57a87624..6ec86c9c6e 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -537,16 +537,17 @@ 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.rebaseMerges` config option and a previous
> + `--rebase-merges`.
> +
> When rebasing merges, there are two modes: `rebase-cousins` and
> -`no-rebase-cousins`. If the mode is not specified, it defaults to
> -`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
> -`<upstream>` as direct ancestor will keep their original branch point, i.e.
> -commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
> -option will keep their original ancestry by default. In `rebase-cousins` mode,
> -such commits are instead rebased onto `<upstream>` (or `<onto>`, if
> -specified).
> +`no-rebase-cousins`. If the mode is not specified on the command line or in
> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
> +In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
> +ancestor will keep their original branch point, i.e. commits that would be
> +excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
> +original ancestry by default. In `rebase-cousins` mode, such commits are
> +instead rebased onto `<upstream>` (or `<onto>`, if specified).
>[...]
> static int rebase_config(const char *var, const char *value, void *data)
> {
> struct rebase_options *opts = data;
> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
> return 0;
> }
>
> + if (!strcmp(var, "rebase.rebasemerges")) {
> + opts->config_rebase_merges = git_parse_maybe_bool(value);
> + if (opts->config_rebase_merges < 0) {
> + opts->config_rebase_merges = 1;
> + parse_rebase_merges_value(opts, value);
> + }
I think we need
} else {
opts->rebase_cousins = 0;
}
here. Otherwise if rebase.rebaseMerges is set twice we wont follow the
usual "last config wins" convention. For example
[rebase]
rebaseMerges=rebase-cousins
rebaseMerges=true
will result in us unexpectedly rebasing cousins
The rest of the patch looks fine
Best Wishes
Phillip
> + return 0;
> + }
> +
> if (!strcmp(var, "rebase.updaterefs")) {
> opts->config_update_refs = git_config_bool(var, value);
> return 0;
> @@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
> return 0;
> }
>
> +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
> +{
> + struct rebase_options *options = opt->value;
> +
> + options->rebase_merges = !unset;
> +
> + if (arg) {
> + if (!*arg) {
> + warning(_("--rebase-merges with an empty string "
> + "argument is deprecated and will stop "
> + "working in a future version of Git. Use "
> + "--rebase-merges without an argument "
> + "instead, which does the same thing."));
> + return 0;
> + }
> + parse_rebase_merges_value(options, arg);
> + }
> +
> + return 0;
> +}
> +
> static void NORETURN error_on_missing_default_upstream(void)
> {
> struct branch *current_branch = branch_get(NULL);
> @@ -1035,7 +1078,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> struct object_id branch_base;
> int ignore_whitespace = 0;
> const char *gpg_sign = NULL;
> - const char *rebase_merges = NULL;
> struct string_list strategy_options = STRING_LIST_INIT_NODUP;
> struct object_id squash_onto;
> char *squash_onto_name = NULL;
> @@ -1137,10 +1179,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> &options.allow_empty_message,
> N_("allow rebasing commits with empty messages"),
> PARSE_OPT_HIDDEN),
> - {OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
> - N_("mode"),
> + OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
> N_("try to rebase merges instead of skipping them"),
> - PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
> + PARSE_OPT_OPTARG, parse_opt_rebase_merges),
> OPT_BOOL(0, "fork-point", &options.fork_point,
> N_("use 'merge-base --fork-point' to refine upstream")),
> OPT_STRING('s', "strategy", &options.strategy,
> @@ -1436,21 +1477,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.exec.nr)
> imply_merge(&options, "--exec");
>
> - if (rebase_merges) {
> - if (!*rebase_merges)
> - warning(_("--rebase-merges with an empty string "
> - "argument is deprecated and will stop "
> - "working in a future version of Git. Use "
> - "--rebase-merges without an argument "
> - "instead, which does the same thing."));
> - else 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;
> - imply_merge(&options, "--rebase-merges");
> - }
> -
> if (options.type == REBASE_APPLY) {
> if (ignore_whitespace)
> strvec_push(&options.git_am_opts,
> @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> break;
>
> if (i >= 0 || options.type == REBASE_APPLY) {
> - if (is_merge(&options))
> - die(_("apply options and merge options "
> - "cannot be used together"));
> - else if (options.autosquash == -1 && options.config_autosquash == 1)
> + if (options.autosquash == -1 && options.config_autosquash == 1)
> die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash"));
> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> + die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
> else if (options.update_refs == -1 && options.config_update_refs == 1)
> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
> + else if (is_merge(&options))
> + die(_("apply options and merge options "
> + "cannot be used together"));
> else
> options.type = REBASE_APPLY;
> }
> @@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> options.update_refs = (options.update_refs >= 0) ? options.update_refs :
> ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
>
> + if (options.rebase_merges == 1)
> + imply_merge(&options, "--rebase-merges");
> + options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
> + ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
> +
> if (options.autosquash == 1)
> imply_merge(&options, "--autosquash");
> options.autosquash = (options.autosquash >= 0) ? options.autosquash :
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 4711b37a28..2eba00bdf5 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -85,6 +85,11 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --reapply-cherry-picks A
> "
>
> + test_expect_success "$opt incompatible with --rebase-merges" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --rebase-merges A
> + "
> +
> test_expect_success "$opt incompatible with --update-refs" "
> git checkout B^0 &&
> test_must_fail git rebase $opt --update-refs A
> @@ -101,6 +106,12 @@ test_rebase_am_only () {
> grep -e --no-autosquash err
> "
>
> + test_expect_success "$opt incompatible with rebase.rebaseMerges" "
> + git checkout B^0 &&
> + test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
> + grep -e --no-rebase-merges err
> + "
> +
> test_expect_success "$opt incompatible with rebase.updateRefs" "
> git checkout B^0 &&
> test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
> @@ -113,6 +124,12 @@ test_rebase_am_only () {
> git -c rebase.autosquash=true rebase --no-autosquash $opt A
> "
>
> + test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
> + test_when_finished \"git reset --hard B^0\" &&
> + git checkout B^0 &&
> + git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
> + "
> +
> test_expect_success "$opt okay with overridden rebase.updateRefs" "
> test_when_finished \"git reset --hard B^0\" &&
> git checkout B^0 &&
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index d46d9545f2..aa75e192d1 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' '
> EOF
> '
>
> +test_expect_success '--rebase-merges="" is deprecated' '
> + git rebase --rebase-merges="" HEAD^ 2>actual &&
> + grep deprecated actual
> +'
> +
> +test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> + test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' '
> + test_config rebase.rebaseMerges 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.rebaseMerges=rebase-cousins' '
> + test_config rebase.rebaseMerges 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.rebaseMerges=false' '
> + test_config rebase.rebaseMerges false &&
> + git checkout -b override-config-merges-false E &&
> + before="$(git rev-parse --verify HEAD)" &&
> + test_tick &&
> + git rebase --rebase-merges C &&
> + test_cmp_rev HEAD $before
> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
> + test_config rebase.rebaseMerges 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 &&
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-07 14:56 ` Phillip Wood
@ 2023-03-07 18:32 ` Junio C Hamano
2023-03-12 21:01 ` Alex Henrie
2023-03-08 0:09 ` Glen Choo
1 sibling, 1 reply; 96+ messages in thread
From: Junio C Hamano @ 2023-03-07 18:32 UTC (permalink / raw)
To: Phillip Wood
Cc: Alex Henrie, git, tao, newren, Johannes.Schindelin, sorganov,
chooglen, calvinwan, jonathantanmy
Phillip Wood <phillip.wood123@gmail.com> writes:
>> +rebase.rebaseMerges::
>> + 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,
>
> This is a bit picky but how can rebase.rebaseMerges=true be equivalent
> to --rebase-merges without an argument when the behavior of
> --rebase-merges without an argument depends on the value of
> rebase.rebaseMerges?
True. I think the configuration is meant to give (when set to
something other than Boolean) the default value to the option
"--rebase-merges" that is given without value, so setting to false
should be a no-op (a command line option would override it if given,
and if there is no command line option, --rebase-merges is not used
by default), setting it to a specific value between cousin choices
would give --rebase-merges=<value> unless --no-rebase-merges is
given, but setting it to true here makes the result undefined,
unless the built-in default between cousin choices is described
here.
"Setting to true is equivalent to setting to no-rebase-cousins" and
"Setting to false is a no-op but accepted only for completeness",
perhaps?
>> 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.rebaseMerges=false`
>> + configuration, but the absence of a specific rebase-merges mode on the
>> + command line does not counteract a specific mode set in the configuration.
>
> I may not agree the with the design choice but the documentation here
> and below is very clear about the behavior of --rebase-merges without
> an argument which is good.
Is it? rebase.rebaseMerges=true does not give "a specific mode set
in the configuration", so it still is unclear what --rebase-merges
should do in that case. Unless what it means to set it to true is
described, as you pointed out above, that is.
>> +`no-rebase-cousins`. If the mode is not specified on the command line or in
>> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
This side could describe what setting it to "true" means, but it is
a separate page so it would be more friendly to readers to cover it
on both pages.
> I think we need
>
> } else {
> opts->rebase_cousins = 0;
> }
>
> here. Otherwise if rebase.rebaseMerges is set twice we wont follow the
> usual "last config wins" convention. For example
>
> [rebase]
> rebaseMerges=rebase-cousins
> rebaseMerges=true
>
> will result in us unexpectedly rebasing cousins
Thanks for a careful review.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-07 18:32 ` Junio C Hamano
@ 2023-03-12 21:01 ` Alex Henrie
0 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-12 21:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood, git, tao, newren, Johannes.Schindelin, sorganov,
chooglen, calvinwan, jonathantanmy
On Tue, Mar 7, 2023 at 11:32 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> +rebase.rebaseMerges::
> >> + 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,
> >
> > This is a bit picky but how can rebase.rebaseMerges=true be equivalent
> > to --rebase-merges without an argument when the behavior of
> > --rebase-merges without an argument depends on the value of
> > rebase.rebaseMerges?
>
> True. I think the configuration is meant to give (when set to
> something other than Boolean) the default value to the option
> "--rebase-merges" that is given without value, so setting to false
> should be a no-op (a command line option would override it if given,
> and if there is no command line option, --rebase-merges is not used
> by default), setting it to a specific value between cousin choices
> would give --rebase-merges=<value> unless --no-rebase-merges is
> given, but setting it to true here makes the result undefined,
> unless the built-in default between cousin choices is described
> here.
>
> "Setting to true is equivalent to setting to no-rebase-cousins" and
> "Setting to false is a no-op but accepted only for completeness",
> perhaps?
A false in the local configuration overrides a true in the global
configuration, so I wouldn't call "false" a no-op. But it would be
fine to say that "true" is equivalent to no-rebase-cousins (and then
change the documentation for "true" if and when the default changes in
the future).
> >> +`no-rebase-cousins`. If the mode is not specified on the command line or in
> >> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
>
> This side could describe what setting it to "true" means, but it is
> a separate page so it would be more friendly to readers to cover it
> on both pages.
The longer the documentation is, the less likely people are to read
any of it. I don't really want to repeat the detailed explanation that
is in git-config.txt, but I see that other places in git-rebase.txt
link to git-config.txt, and having a link seems like a good idea. Fair
enough?
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-07 14:56 ` Phillip Wood
2023-03-07 18:32 ` Junio C Hamano
@ 2023-03-08 0:09 ` Glen Choo
1 sibling, 0 replies; 96+ messages in thread
From: Glen Choo @ 2023-03-08 0:09 UTC (permalink / raw)
To: phillip.wood, Alex Henrie, git, tao, gitster, newren,
phillip.wood123, Johannes.Schindelin, sorganov, calvinwan,
jonathantanmy
Phillip Wood <phillip.wood123@gmail.com> writes:
>> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
>> return 0;
>> }
>>
>> + if (!strcmp(var, "rebase.rebasemerges")) {
>> + opts->config_rebase_merges = git_parse_maybe_bool(value);
>> + if (opts->config_rebase_merges < 0) {
>> + opts->config_rebase_merges = 1;
>> + parse_rebase_merges_value(opts, value);
>> + }
>
> I think we need
>
> } else {
> opts->rebase_cousins = 0;
> }
>
> here. Otherwise if rebase.rebaseMerges is set twice we wont follow the
> usual "last config wins" convention. For example
>
> [rebase]
> rebaseMerges=rebase-cousins
> rebaseMerges=true
>
> will result in us unexpectedly rebasing cousins
Ah, good catch.
An alternative might be to use the "lookup" functions (e.g.
repo_config_get_maybe_bool()), which already implement "last one wins"
semantics. It's a bigger change, but it may be easier for future readers
to understand. I don't have a strong opinion on which is the 'better'
approach.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-07 14:56 ` Phillip Wood
@ 2023-03-08 0:02 ` Glen Choo
2023-03-12 21:03 ` Alex Henrie
2023-03-15 2:52 ` Alex Henrie
1 sibling, 2 replies; 96+ messages in thread
From: Glen Choo @ 2023-03-08 0:02 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, calvinwan, jonathantanmy
Cc: Alex Henrie
Alex Henrie <alexhenrie24@gmail.com> writes:
> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate turning on
> --rebase-merges by default without configuration in a future version of
> Git.
>
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.
This rationale makes sense to me.
> In the future, the default rebase-merges mode may change from
> no-rebase-cousins to rebase-cousins.
I suspect a more likely future would be that the default is to rebase
'evil' merges instead of trying to recreate merge commits, but of
course, the important thing is that we promote the default, not what the
default will be...
> Support setting rebase.rebaseMerges
> to the nonspecific value "true" for users who do not need or want to
> care about the default changing in the future. Similarly, for users who
> have --rebase-merges in an alias and want to get the future behavior
> now, use the specific rebase-merges mode from the config if a specific
> mode is not given on the command line.
so this rationale makes sense to me too :)
> @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' '
> EOF
> '
>
> +test_expect_success '--rebase-merges="" is deprecated' '
> + git rebase --rebase-merges="" HEAD^ 2>actual &&
> + grep deprecated actual
> +'
I believe this used to be on 2/3, i.e.
https://lore.kernel.org/git/20230225180325.796624-3-alexhenrie24@gmail.com/
but your cover letter suggests that it was removed. Mechanical error?
The rest of changes look good (besides what others have spotted).
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-08 0:02 ` Glen Choo
@ 2023-03-12 21:03 ` Alex Henrie
2023-03-15 2:52 ` Alex Henrie
1 sibling, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-12 21:03 UTC (permalink / raw)
To: Glen Choo
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, calvinwan, jonathantanmy
On Tue, Mar 7, 2023 at 5:02 PM Glen Choo <chooglen@google.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
> > +test_expect_success '--rebase-merges="" is deprecated' '
> > + git rebase --rebase-merges="" HEAD^ 2>actual &&
> > + grep deprecated actual
> > +'
>
> I believe this used to be on 2/3, i.e.
>
> https://lore.kernel.org/git/20230225180325.796624-3-alexhenrie24@gmail.com/
>
> but your cover letter suggests that it was removed. Mechanical error?
I removed it from the second patch but accidentally added it back in
the third patch. Thanks for catching that.
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-08 0:02 ` Glen Choo
2023-03-12 21:03 ` Alex Henrie
@ 2023-03-15 2:52 ` Alex Henrie
2023-03-16 17:32 ` Glen Choo
1 sibling, 1 reply; 96+ messages in thread
From: Alex Henrie @ 2023-03-15 2:52 UTC (permalink / raw)
To: Glen Choo
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, calvinwan, jonathantanmy
On Tue, Mar 7, 2023 at 5:02 PM Glen Choo <chooglen@google.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
> > In the future, the default rebase-merges mode may change from
> > no-rebase-cousins to rebase-cousins.
>
> I suspect a more likely future would be that the default is to rebase
> 'evil' merges instead of trying to recreate merge commits, but of
> course, the important thing is that we promote the default, not what the
> default will be...
Glen, do you have any more thoughts? At this point, the only thing
that's keeping me from implementing Phillip's request to make
--rebase-merges without an argument clobber rebase.rebaseMerges is
your suspicion that we might need to change the default rebase-merges
mode in the future, and I assume that we would want to use the
rebase.rebaseMerges config option to facilitate the transition.
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-15 2:52 ` Alex Henrie
@ 2023-03-16 17:32 ` Glen Choo
2023-03-16 18:11 ` Felipe Contreras
2023-03-16 20:27 ` Alex Henrie
0 siblings, 2 replies; 96+ messages in thread
From: Glen Choo @ 2023-03-16 17:32 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
>> > In the future, the default rebase-merges mode may change from
>> > no-rebase-cousins to rebase-cousins.
>>
>> I suspect a more likely future would be that the default is to rebase
>> 'evil' merges instead of trying to recreate merge commits, but of
>> course, the important thing is that we promote the default, not what the
>> default will be...
>
> Glen, do you have any more thoughts? At this point, the only thing
> that's keeping me from implementing Phillip's request to make
> --rebase-merges without an argument clobber rebase.rebaseMerges is
> your suspicion that we might need to change the default rebase-merges
> mode in the future, and I assume that we would want to use the
> rebase.rebaseMerges config option to facilitate the transition.
(Sorry for the late reply)
Ah, I don't really have more thoughts on the matter. I am fairly
confident that we would _like_ to change the default to rebase 'evil'
merges, but I don't know how likely that will be.
Perhaps it would help to enumerate the rules to see if it is too
confusing or not?
The behaviors we can tweak are:
- Whether to rebase merges or not (true, false, specified mode, or
default)
- What mode to use when rebasing merges (specified mode or default)
And the sources are either CLI or config, with CLI always overriding
config.
Should we rebase a merge?
- If neither CLI or config tells us whether or not to rebase a merge,
default to "don't rebase merges".
- If one of CLI or config tells us whether or not to rebase a merge,
respect it.
- If both CLI or config tell us whether or not to rebase a merge,
respect CLI and ignore config.
What mode should we use?
- If neither CLI or config tells us what mode to use, default to
"no-rebase-cousins" (or whatever default we decide).
- If one of CLI or config tells us what mode to use, respect it.
- If both CLI or config tell us what mode to use, respect CLI and ignore
config.
If users cleanly separate the two concepts, I think it is quite clear.
(I'm not advocating for this approach, but) e.g. if we pretend that each
behavior were configured separately, like:
--[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
I don't think there would be any confusion. (Having --rebase-merges-mode
be a no-op without --rebase-merges is probably even more confusing to
users, plus this would break backwards compatibility, so I don't think
this is a good idea at all.)
Your doc patch explains the rules pretty clearly, but perhaps it doesn't
explain this mental model clearly enough, hence the confusion. If we
don't find a good way to communicate this (I think it is clear, but
other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
suggestion to have "--rebase-merges" override
"rebase.rebaseMerges='specific-mode'".
>
> -Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 17:32 ` Glen Choo
@ 2023-03-16 18:11 ` Felipe Contreras
2023-03-16 22:46 ` Glen Choo
2023-03-16 20:27 ` Alex Henrie
1 sibling, 1 reply; 96+ messages in thread
From: Felipe Contreras @ 2023-03-16 18:11 UTC (permalink / raw)
To: Glen Choo
Cc: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, calvinwan, jonathantanmy
On Thu, Mar 16, 2023 at 11:57 AM Glen Choo <chooglen@google.com> wrote:
> If users cleanly separate the two concepts, I think it is quite clear.
> (I'm not advocating for this approach, but) e.g. if we pretend that each
> behavior were configured separately, like:
>
> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
>
> I don't think there would be any confusion.
Not being conversant with these options I agree the above isn't confusing.
> (Having --rebase-merges-mode
> be a no-op without --rebase-merges is probably even more confusing to
> users, plus this would break backwards compatibility, so I don't think
> this is a good idea at all.)
I don't find it confusing. And how would it break backwards
compatibility if --rebase-merges-mode doesn't exist now?
--
Felipe Contreras
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 18:11 ` Felipe Contreras
@ 2023-03-16 22:46 ` Glen Choo
2023-03-16 23:48 ` Felipe Contreras
0 siblings, 1 reply; 96+ messages in thread
From: Glen Choo @ 2023-03-16 22:46 UTC (permalink / raw)
To: Felipe Contreras
Cc: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, calvinwan, jonathantanmy
Felipe Contreras <felipe.contreras@gmail.com> writes:
>> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
>>
>> I don't think there would be any confusion.
>> [...]
>> (Having --rebase-merges-mode
>> be a no-op without --rebase-merges is probably even more confusing to
>> users, plus this would break backwards compatibility, so I don't think
>> this is a good idea at all.)
>
> I don't find it confusing. And how would it break backwards
> compatibility if --rebase-merges-mode doesn't exist now?
I meant that for the above example to work, we would need to have
`--[no-]rebase-merges` as a boolean that says whether we rebase merges at
all, and `--rebase-merges-mode=[whatever]` would tell us what mode to
use _if_ we were rebasing merges. This means that
`--rebase-merges=not-a-boolean` would become invalid.
We were in this position before, actually, with `git log -p -m`. The
conclusion on a recent series [1] is that having such a no-op flag was
probably a mistake (and unfortunately, one that is extremely hard to fix
due to backwards compatibility requirements), so introducing more no-op
flags is probably a bad idea :)
[1] https://lore.kernel.org/git/871qn5pyez.fsf@osv.gnss.ru
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 22:46 ` Glen Choo
@ 2023-03-16 23:48 ` Felipe Contreras
0 siblings, 0 replies; 96+ messages in thread
From: Felipe Contreras @ 2023-03-16 23:48 UTC (permalink / raw)
To: Glen Choo
Cc: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, calvinwan, jonathantanmy
On Thu, Mar 16, 2023 at 4:46 PM Glen Choo <chooglen@google.com> wrote:
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
> >> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
> >>
> >> I don't think there would be any confusion.
> >> [...]
> >> (Having --rebase-merges-mode
> >> be a no-op without --rebase-merges is probably even more confusing to
> >> users, plus this would break backwards compatibility, so I don't think
> >> this is a good idea at all.)
> >
> > I don't find it confusing. And how would it break backwards
> > compatibility if --rebase-merges-mode doesn't exist now?
>
> I meant that for the above example to work, we would need to have
> `--[no-]rebase-merges` as a boolean that says whether we rebase merges at
> all, and `--rebase-merges-mode=[whatever]` would tell us what mode to
> use _if_ we were rebasing merges.
No, we don't need --no-rebase-merges for --rebase-merges-mode to work.
It would be nice, but not necessary.
> This means that
> `--rebase-merges=not-a-boolean` would become invalid.
No, it would not become invalid, that's yet another decision to make.
`--rebase-merges=mode` could become synonymous with `--rebase-merges
--rebase-merges-mode=mode`. A shortcut.
Because it's redundant it could be deprecated in the future, but not
necessarily invalid.
> We were in this position before, actually, with `git log -p -m`. The
> conclusion on a recent series [1] is that having such a no-op flag was
> probably a mistake (and unfortunately, one that is extremely hard to fix
> due to backwards compatibility requirements),
No, `git log -m` was a mistake *only* if -p isn't turned on as well,
which is an interface wart that could be fixed today.
> so introducing more no-op flags is probably a bad idea :)
Whether --rebase-merges-mode turns on --rebase-merges by default is a
decision that could be made in the future. Just like `git log -m`
turning on `-p` by default is a decision that could be made in the
future (and probably should).
---
Either way: introducing a new option cannot possibly break backwards
compatibility.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 17:32 ` Glen Choo
2023-03-16 18:11 ` Felipe Contreras
@ 2023-03-16 20:27 ` Alex Henrie
2023-03-16 22:39 ` Glen Choo
` (2 more replies)
1 sibling, 3 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-16 20:27 UTC (permalink / raw)
To: Glen Choo
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, calvinwan, jonathantanmy
On Thu, Mar 16, 2023 at 11:32 AM Glen Choo <chooglen@google.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> >> > In the future, the default rebase-merges mode may change from
> >> > no-rebase-cousins to rebase-cousins.
> >>
> >> I suspect a more likely future would be that the default is to rebase
> >> 'evil' merges instead of trying to recreate merge commits, but of
> >> course, the important thing is that we promote the default, not what the
> >> default will be...
> >
> > Glen, do you have any more thoughts? At this point, the only thing
> > that's keeping me from implementing Phillip's request to make
> > --rebase-merges without an argument clobber rebase.rebaseMerges is
> > your suspicion that we might need to change the default rebase-merges
> > mode in the future, and I assume that we would want to use the
> > rebase.rebaseMerges config option to facilitate the transition.
>
> (Sorry for the late reply)
>
> Ah, I don't really have more thoughts on the matter. I am fairly
> confident that we would _like_ to change the default to rebase 'evil'
> merges, but I don't know how likely that will be.
>
> Perhaps it would help to enumerate the rules to see if it is too
> confusing or not?
>
> The behaviors we can tweak are:
>
> - Whether to rebase merges or not (true, false, specified mode, or
> default)
> - What mode to use when rebasing merges (specified mode or default)
>
> And the sources are either CLI or config, with CLI always overriding
> config.
>
> Should we rebase a merge?
>
> - If neither CLI or config tells us whether or not to rebase a merge,
> default to "don't rebase merges".
> - If one of CLI or config tells us whether or not to rebase a merge,
> respect it.
> - If both CLI or config tell us whether or not to rebase a merge,
> respect CLI and ignore config.
>
> What mode should we use?
>
> - If neither CLI or config tells us what mode to use, default to
> "no-rebase-cousins" (or whatever default we decide).
> - If one of CLI or config tells us what mode to use, respect it.
> - If both CLI or config tell us what mode to use, respect CLI and ignore
> config.
>
> If users cleanly separate the two concepts, I think it is quite clear.
> (I'm not advocating for this approach, but) e.g. if we pretend that each
> behavior were configured separately, like:
>
> --[no-]rebase-merges [--rebase-merges-mode=(rebase-cousins|no-rebase-cousins)]
>
> I don't think there would be any confusion. (Having --rebase-merges-mode
> be a no-op without --rebase-merges is probably even more confusing to
> users, plus this would break backwards compatibility, so I don't think
> this is a good idea at all.)
>
> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
> explain this mental model clearly enough, hence the confusion. If we
> don't find a good way to communicate this (I think it is clear, but
> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
> suggestion to have "--rebase-merges" override
> "rebase.rebaseMerges='specific-mode'".
I got the impression that everyone, including Phillip,[1] already
agrees that the proposed documentation is clear about the interaction
between the config option and the command line option. However, it is
a little weird when you consider that other flags with optional
arguments, like `git branch --track`, unconditionally override their
corresponding config options.[2]
Let me ask a different but related question: If we add a
rebase-evil-merges mode, do you think that would be orthogonal to the
rebase-cousins mode?
-Alex
[1] https://lore.kernel.org/git/7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk/
[2] https://lore.kernel.org/git/5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk/
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 20:27 ` Alex Henrie
@ 2023-03-16 22:39 ` Glen Choo
2023-03-18 5:59 ` Alex Henrie
2023-03-24 15:05 ` Johannes Schindelin
2023-03-25 16:59 ` Sergey Organov
2 siblings, 1 reply; 96+ messages in thread
From: Glen Choo @ 2023-03-16 22:39 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
>> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
>> explain this mental model clearly enough, hence the confusion. If we
>> don't find a good way to communicate this (I think it is clear, but
>> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
>> suggestion to have "--rebase-merges" override
>> "rebase.rebaseMerges='specific-mode'".
>
> I got the impression that everyone, including Phillip,[1] already
> agrees that the proposed documentation is clear about the interaction
> between the config option and the command line option. However, it is
> a little weird when you consider that other flags with optional
> arguments, like `git branch --track`, unconditionally override their
> corresponding config options.[2]
Ah, I didn't consider other options like `git branch --track`. I haven't
looked into what is the norm, but I think we should follow it (whatever
it is).
If other reviewers have a strong idea of what this norm is, I am happy
to defer to them. If not, I can look into it given some time.
> Let me ask a different but related question: If we add a
> rebase-evil-merges mode, do you think that would be orthogonal to the
> rebase-cousins mode?
I am not an expert on this, so perhaps my opinion isn't that important
;)
My understanding is that `[no-]rebase-cousins` affects which commits get
rebased and onto where, whereas `rebase-evil-merges` would affect how
the merge commits are generated (by rebasing the evil or by simply
recreating the merges). From that perspective, it seems like yes, the
two would be orthogonal.
Hm. Maybe this means that we'd be introducing a new option, and that my
hunch that we would change the default to `rebase-evil-merges` is more
wrong than I expected.
Though I guess this doesn't really affects what we do with the CLI
options _now_, since the discussion is about what we do about defaults,
and what the default is is quite immaterial.
>
> -Alex
>
> [1] https://lore.kernel.org/git/7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk/
> [2] https://lore.kernel.org/git/5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk/
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 22:39 ` Glen Choo
@ 2023-03-18 5:59 ` Alex Henrie
0 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-18 5:59 UTC (permalink / raw)
To: Glen Choo
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, calvinwan, jonathantanmy
On Thu, Mar 16, 2023 at 4:39 PM Glen Choo <chooglen@google.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> >> Your doc patch explains the rules pretty clearly, but perhaps it doesn't
> >> explain this mental model clearly enough, hence the confusion. If we
> >> don't find a good way to communicate this (I think it is clear, but
> >> other reviewers seem yet unconvinced), I wouldn't mind taking Phillip's
> >> suggestion to have "--rebase-merges" override
> >> "rebase.rebaseMerges='specific-mode'".
> >
> > I got the impression that everyone, including Phillip,[1] already
> > agrees that the proposed documentation is clear about the interaction
> > between the config option and the command line option. However, it is
> > a little weird when you consider that other flags with optional
> > arguments, like `git branch --track`, unconditionally override their
> > corresponding config options.[2]
>
> Ah, I didn't consider other options like `git branch --track`. I haven't
> looked into what is the norm, but I think we should follow it (whatever
> it is).
>
> If other reviewers have a strong idea of what this norm is, I am happy
> to defer to them. If not, I can look into it given some time.
>
> > Let me ask a different but related question: If we add a
> > rebase-evil-merges mode, do you think that would be orthogonal to the
> > rebase-cousins mode?
>
> I am not an expert on this, so perhaps my opinion isn't that important
> ;)
>
> My understanding is that `[no-]rebase-cousins` affects which commits get
> rebased and onto where, whereas `rebase-evil-merges` would affect how
> the merge commits are generated (by rebasing the evil or by simply
> recreating the merges). From that perspective, it seems like yes, the
> two would be orthogonal.
>
> Hm. Maybe this means that we'd be introducing a new option, and that my
> hunch that we would change the default to `rebase-evil-merges` is more
> wrong than I expected.
>
> Though I guess this doesn't really affects what we do with the CLI
> options _now_, since the discussion is about what we do about defaults,
> and what the default is is quite immaterial.
I looked through the code and documentation and found 12 good examples
of command line flags with an optional argument that always override
their corresponding config options whether or not the optional
argument is given:
git -c branch.autoSetupMerge=inherit branch --track mynewbranch
git -c commit.gpgSign=mykey commit --gpg-sign
git -c core.sharedRepository=0666 init --shared .
git -c diff.ignoreSubmodules=untracked diff --ignore-submodules
git -c diff.submodule=diff diff --submodule
git -c format.attach=myboundary format-patch --attach HEAD^
git -c format.from='Dang Git <dang@example.org>' format-patch --from HEAD^
git -c format.thread=deep format-patch --thread HEAD~3
git -c gc.pruneExpire=1.week.ago gc --prune
git -c log.decorate=full log --decorate
git -c merge.log=1 merge --log mytopicbranch
git -c pull.rebase=interactive pull --rebase
I found 6 other similar examples where the config option is a
yes/no/auto trilean, but I don't think those are good examples because
it makes total sense for a command line flag without an argument to
override a nonspecific "auto" value in the configuration:
git -c color.diff=auto diff --color | less
git -c color.grep=auto grep --color . | less
git -c color.showbranch=auto show-branch --color master mytopicbranch | less
git -c color.ui=auto log --color | less
git -c fetch.recurseSubmodules=on-demand fetch --recurse-submodules
git -c push.gpgSign=if-asked push --signed
I found 1 good example where the config option does make a difference
if the optional argument is omitted:
git -c diff.colorMoved=plain diff --color-moved
And I found 1 other similar example, but it's not good a example
because the config option doesn't do anything unless the command line
flag is specified:
git -c diff.dirstat=lines diff --dirstat
Given 12 good examples versus 1 good counterexample, I would say that
the established convention is for the command line flag to always
override the config option. Please let me know if there are other
examples that I missed.
It's worth noting that the documentation for `git format-patch
--thread` explicitly says that format.thread takes precedence over
--thread without an argument, but that is not correct: When I tested
it, the command line argument always took precedence.
Another problem is that the documentation for `git pack-objects` does
not even mention that --unpack-unreachable has an optional argument,
and it says that the argument to --cruft-expiration is required when
it is not.
I'm not going to fix all the documentation problems, at least not
right now. However, in order to follow the established convention for
flags with optional arguments, and because we probably aren't going to
use rebase.rebaseMerges to facilitate a future change of defaults, I
will redo the patch so that --rebase-merges without an argument takes
precedence over rebase.rebaseMerges.
Thanks for the feedback. I feel more confident about the way forward now.
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 20:27 ` Alex Henrie
2023-03-16 22:39 ` Glen Choo
@ 2023-03-24 15:05 ` Johannes Schindelin
2023-03-25 16:59 ` Sergey Organov
2 siblings, 0 replies; 96+ messages in thread
From: Johannes Schindelin @ 2023-03-24 15:05 UTC (permalink / raw)
To: Alex Henrie
Cc: Glen Choo, git, tao, gitster, newren, phillip.wood123, sorganov,
calvinwan, jonathantanmy
Hi Alex,
On Thu, 16 Mar 2023, Alex Henrie wrote:
> If we add a rebase-evil-merges mode, do you think that would be
> orthogonal to the rebase-cousins mode?
Those two concepts are very much orthogonal.
The rebase-evil-merges mode needs to change the way the `merge` command
operates, from "forget everything but the commit message of the original
merge commit" to "do try to figure out what amendments were made to the
original merge and replay them".
Whether cousins are rebased or not has everything to do with the `reset`
command, though.
Elsewhere in the thread, you mentioned a suspicion that `rebase-cousins`
should become the default. However, that would be a radical shift from the
spirit of the `git rebase [-r] <onto>` command: If `<onto>` is reachable
from `HEAD`, the idea is that an unchanged rebase script will produce the
identical commit topology. This is in line with a regular (non-`-r`)
rebase as long as there are no merge commits between `<onto>` and `HEAD`.
I am not sure that such a radical shift could ever be an easy sell.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 3/3] rebase: add a config option for --rebase-merges
2023-03-16 20:27 ` Alex Henrie
2023-03-16 22:39 ` Glen Choo
2023-03-24 15:05 ` Johannes Schindelin
@ 2023-03-25 16:59 ` Sergey Organov
2 siblings, 0 replies; 96+ messages in thread
From: Sergey Organov @ 2023-03-25 16:59 UTC (permalink / raw)
To: Alex Henrie
Cc: Glen Choo, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
[...]
> Let me ask a different but related question: If we add a
> rebase-evil-merges mode, do you think that would be orthogonal to the
> rebase-cousins mode?
This is the question that I considered as well. To me they look
orthogonal, and I'm afraid we will have hard times extending
--rebase-merges gracefully in a backward-compatible manner.
The set:
--rebase-merges=remerge
--rebase-merges=rebase
--rebase-merges=off
looks natural to me, but "cousins" that affect what is to be rebased (as
opposed to how, if at all), will apparently get in the way. Besides, I
recently ran into similar problem with --diff-merges (when|how)
dichotomy, and there was no elegant solution found, even though a
working one has been implemented, and then there were some discussions
around.
Also, I'm not quite sure that these "cousins" options make no sense when
we in fact flatten history (--no-rebase-merges case). To me it looks
like there should have been separate --[no-]rebase-cousins option,
provided we do need this choice in the first place, but I definitely
might be wrong here as well.
As a side note, it sounds both unfair and confusing to use
"rebase-evil-merges" term to describe a mode that will actually rebase
(all the) merges, as neither a merge needs to be 'evil' to be the object
of rebase operation, nor dedicated mode is necessarily needed to rebase
specifically 'evil' merges (whatever they actually are.)
Thanks,
-- Sergey Organov
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
` (2 preceding siblings ...)
2023-03-05 5:07 ` [PATCH v6 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-03-05 12:22 ` Sergey Organov
2023-03-05 21:33 ` Alex Henrie
2023-03-06 16:24 ` Phillip Wood
` (2 subsequent siblings)
6 siblings, 1 reply; 96+ messages in thread
From: Sergey Organov @ 2023-03-05 12:22 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
chooglen, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
[...]
> - Rephrase the warning about --rebase-merges="" to recommend
> --rebase-merges without an argument instead, and clarify that that
> will do the same thing
Not a strong objection to the series as they are, but, provided options
with optional arguments are considered to be a bad practice in general
(unless something had recently changed), I'd like to vote for adding:
--rebase-merges=on (≡ --reabase-merges="")
and for suggesting to use that one instead of --rebase-merges="" rather
than naked --rebase-merges.
It'd be best to actually fix --rebase-merges to always have an argument,
the more so as we have '-r' shortcut, but as this is unlikely to fly due
to backward compatibility considerations, this way we would at least
avoid promoting bad practices further.
If accepted, please consider to introduce
--rebase-merges=off (≡ --no-rebase-merges)
as well for completeness.
BTW, that's how we settled upon in the implementation of --diff-merges,
so these two will then be mutually consistent.
Thanks,
-- Sergey Organov
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 12:22 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Sergey Organov
@ 2023-03-05 21:33 ` Alex Henrie
2023-03-05 22:54 ` Sergey Organov
0 siblings, 1 reply; 96+ messages in thread
From: Alex Henrie @ 2023-03-05 21:33 UTC (permalink / raw)
To: Sergey Organov
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
chooglen, calvinwan, jonathantanmy
On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@gmail.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> [...]
>
> > - Rephrase the warning about --rebase-merges="" to recommend
> > --rebase-merges without an argument instead, and clarify that that
> > will do the same thing
>
> Not a strong objection to the series as they are, but, provided options
> with optional arguments are considered to be a bad practice in general
> (unless something had recently changed), I'd like to vote for adding:
>
> --rebase-merges=on (≡ --reabase-merges="")
>
> and for suggesting to use that one instead of --rebase-merges="" rather
> than naked --rebase-merges.
>
> It'd be best to actually fix --rebase-merges to always have an argument,
> the more so as we have '-r' shortcut, but as this is unlikely to fly due
> to backward compatibility considerations, this way we would at least
> avoid promoting bad practices further.
>
> If accepted, please consider to introduce
>
> --rebase-merges=off (≡ --no-rebase-merges)
>
> as well for completeness.
>
> BTW, that's how we settled upon in the implementation of --diff-merges,
> so these two will then be mutually consistent.
I am curious as to why you say that flags with optional arguments are
considered bad practice. I don't see an advantage to adding
--rebase-merges=on and then telling users that they ought to always
type the extra three characters, even if we were designing the feature
from scratch. As it is, we're certainly not going to drop support for
--no-rebase-merges or --rebase-merges without an argument, so it seems
fine to advertise them to users. And if we do want to add support for
passing a boolean value as an argument to --rebase-merges, that can be
done after the rebase.rebaseMerges config option is added; it's not a
prerequisite.
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 21:33 ` Alex Henrie
@ 2023-03-05 22:54 ` Sergey Organov
2023-03-06 0:02 ` Alex Henrie
2023-03-06 17:19 ` Junio C Hamano
0 siblings, 2 replies; 96+ messages in thread
From: Sergey Organov @ 2023-03-05 22:54 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
chooglen, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
> On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Alex Henrie <alexhenrie24@gmail.com> writes:
>>
>> [...]
>>
>> > - Rephrase the warning about --rebase-merges="" to recommend
>> > --rebase-merges without an argument instead, and clarify that that
>> > will do the same thing
>>
>> Not a strong objection to the series as they are, but, provided options
>> with optional arguments are considered to be a bad practice in general
>> (unless something had recently changed), I'd like to vote for adding:
>>
>> --rebase-merges=on (≡ --reabase-merges="")
>>
>> and for suggesting to use that one instead of --rebase-merges="" rather
>> than naked --rebase-merges.
>>
>> It'd be best to actually fix --rebase-merges to always have an argument,
>> the more so as we have '-r' shortcut, but as this is unlikely to fly due
>> to backward compatibility considerations, this way we would at least
>> avoid promoting bad practices further.
>>
>> If accepted, please consider to introduce
>>
>> --rebase-merges=off (≡ --no-rebase-merges)
>>
>> as well for completeness.
>>
>> BTW, that's how we settled upon in the implementation of --diff-merges,
>> so these two will then be mutually consistent.
>
> I am curious as to why you say that flags with optional arguments are
> considered bad practice.
As far as I can tell, the core problem with such options is that generic
options parsing code can't tell if in "--option value" "value" is an
argument to "--option", or separate argument, that in turn may lead to
very surprising behaviors and bugs.
I believe it's a common knowledge here. If I'm wrong, then the rest
simply doesn't make sense.
> I don't see an advantage to adding --rebase-merges=on and then telling
> users that they ought to always type the extra three characters, even
> if we were designing the feature from scratch.
The advantage is that we avoid optional arguments. That said, there is
'-r' that is 13 characters shorter than --rebase-merges, so another
option is to advertise that, provided you are still opposed to
"--rebase-merges=on".
> As it is, we're certainly not going to drop support for
> --no-rebase-merges
--no-rebase-merges is fine, as it has no optional arguments
> or --rebase-merges without an argument,
Yep, but that's a pity and the whole point of my comment: if we can't
fix it, at least don't promote it.
> so it seems fine to advertise them to users.
--no-rebase-merges is fine, but then you don't advertise it anyway.
> And if we do want to add support for passing a boolean value as an
> argument to --rebase-merges, that can be done after the
> rebase.rebaseMerges config option is added; it's not a prerequisite.
Yep, that's why I said at the beginning that this is not an objection to
the series as they are, rather a nitpick.
Thanks,
-- Sergey
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 22:54 ` Sergey Organov
@ 2023-03-06 0:02 ` Alex Henrie
2023-03-06 13:23 ` Sergey Organov
2023-03-06 19:08 ` Junio C Hamano
2023-03-06 17:19 ` Junio C Hamano
1 sibling, 2 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-06 0:02 UTC (permalink / raw)
To: Sergey Organov
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
chooglen, calvinwan, jonathantanmy
On Sun, Mar 5, 2023 at 3:54 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Alex Henrie <alexhenrie24@gmail.com> writes:
> >>
> >> [...]
> >>
> >> > - Rephrase the warning about --rebase-merges="" to recommend
> >> > --rebase-merges without an argument instead, and clarify that that
> >> > will do the same thing
> >>
> >> Not a strong objection to the series as they are, but, provided options
> >> with optional arguments are considered to be a bad practice in general
> >> (unless something had recently changed), I'd like to vote for adding:
> >>
> >> --rebase-merges=on (≡ --reabase-merges="")
> >>
> >> and for suggesting to use that one instead of --rebase-merges="" rather
> >> than naked --rebase-merges.
> >>
> >> It'd be best to actually fix --rebase-merges to always have an argument,
> >> the more so as we have '-r' shortcut, but as this is unlikely to fly due
> >> to backward compatibility considerations, this way we would at least
> >> avoid promoting bad practices further.
> >>
> >> If accepted, please consider to introduce
> >>
> >> --rebase-merges=off (≡ --no-rebase-merges)
> >>
> >> as well for completeness.
> >>
> >> BTW, that's how we settled upon in the implementation of --diff-merges,
> >> so these two will then be mutually consistent.
> >
> > I am curious as to why you say that flags with optional arguments are
> > considered bad practice.
>
> As far as I can tell, the core problem with such options is that generic
> options parsing code can't tell if in "--option value" "value" is an
> argument to "--option", or separate argument, that in turn may lead to
> very surprising behaviors and bugs.
Interesting, thank you for clarifying. I just tried it and it appears
that --rebase-merges requires an equals sign when it has an argument.
For example:
$ git rebase --rebase-merges=rebase-cousins
Current branch master is up to date.
$ git rebase --rebase-merges rebase-cousins
fatal: invalid upstream 'rebase-cousins'
So there is no ambiguity because if an argument to a flag is optional,
an equals sign is required.
Conversely, because the argument to --diff-merges is required, the
equals sign is optional.
I can see the argument (no pun intended) for avoiding optional
arguments to flags because it is confusing that '=' is required for
optional arguments but not for mandatory arguments, and it's easy to
forget which arguments are mandatory and which are optional.
However, --rebase-merges already has an optional argument. If we make
it look more like --diff-merges by accepting the value "on", it could
actually be even more confusing because users would be more likely to
try "--rebase-merges on", thinking that that will work because
"--diff-merges on" works.
Given the current situation, users are just going to have to learn
that in Git, optional arguments to flags must be preceded by an equals
sign. Maybe someday Git will make breaking changes to get rid of
optional arguments to flags, but for better or for worse, that day
will not come soon :-/
> > As it is, we're certainly not going to drop support for
> > --no-rebase-merges
>
> --no-rebase-merges is fine, as it has no optional arguments
>
> > or --rebase-merges without an argument,
>
> Yep, but that's a pity and the whole point of my comment: if we can't
> fix it, at least don't promote it.
>
> > so it seems fine to advertise them to users.
>
> --no-rebase-merges is fine, but then you don't advertise it anyway.
I am not sure what you mean by this. The first patch of the series
adds documentation and a test for --no-rebase-merges, so I am
advertising it. Or are you saying that I /should/ advertise neither
--no-rebase-merges nor --rebase-merges without an argument, because
you think --rebase-merges=off and --rebase-merges=on would be better?
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-06 0:02 ` Alex Henrie
@ 2023-03-06 13:23 ` Sergey Organov
2023-03-06 19:08 ` Junio C Hamano
1 sibling, 0 replies; 96+ messages in thread
From: Sergey Organov @ 2023-03-06 13:23 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
chooglen, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
[...]
>> > so it seems fine to advertise them to users.
>>
>> --no-rebase-merges is fine, but then you don't advertise it anyway.
>
> I am not sure what you mean by this. The first patch of the series
> adds documentation and a test for --no-rebase-merges, so I am
> advertising it.
Ah, yes, you do it there, and that's fine with me.
I meant only the part where you suggest --rebase-merges instead of
--rebase-merges="". I have no nitpicks about any other parts of the
series.
> Or are you saying that I /should/ advertise neither --no-rebase-merges
> nor --rebase-merges without an argument, because you think
> --rebase-merges=off and --rebase-merges=on would be better?
No, --no-rebase-merges is fine as far as I'm concerned.
Thanks,
-- Sergey
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-06 0:02 ` Alex Henrie
2023-03-06 13:23 ` Sergey Organov
@ 2023-03-06 19:08 ` Junio C Hamano
1 sibling, 0 replies; 96+ messages in thread
From: Junio C Hamano @ 2023-03-06 19:08 UTC (permalink / raw)
To: Alex Henrie
Cc: Sergey Organov, git, tao, newren, phillip.wood123,
Johannes.Schindelin, chooglen, calvinwan, jonathantanmy
Alex Henrie <alexhenrie24@gmail.com> writes:
> Interesting, thank you for clarifying. I just tried it and it appears
> that --rebase-merges requires an equals sign when it has an argument.
> For example:
>
> $ git rebase --rebase-merges=rebase-cousins
> Current branch master is up to date.
>
> $ git rebase --rebase-merges rebase-cousins
> fatal: invalid upstream 'rebase-cousins'
>
> So there is no ambiguity because if an argument to a flag is optional,
> an equals sign is required.
Exactly.
It is not a good excuse that users can express something
unambiguously to the machinery once they know they need to use '=',
when they are so accustomed to giving values to ordinary options
without. This is why options with optional value is considered a
bad UI element, because the way "--opt val" is interpreted for them
is different from everybody else. And it burdens the users by
forcing them to _know_ which ones are with optional value.
Since it is an existing UI breakage, as long as the series is not
making it worse or harder to fix in the future, it is fine, though.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 22:54 ` Sergey Organov
2023-03-06 0:02 ` Alex Henrie
@ 2023-03-06 17:19 ` Junio C Hamano
1 sibling, 0 replies; 96+ messages in thread
From: Junio C Hamano @ 2023-03-06 17:19 UTC (permalink / raw)
To: Sergey Organov
Cc: Alex Henrie, git, tao, newren, phillip.wood123,
Johannes.Schindelin, chooglen, calvinwan, jonathantanmy
Sergey Organov <sorganov@gmail.com> writes:
>> I am curious as to why you say that flags with optional arguments are
>> considered bad practice.
>
> As far as I can tell, the core problem with such options is that generic
> options parsing code can't tell if in "--option value" "value" is an
> argument to "--option", or separate argument, that in turn may lead to
> very surprising behaviors and bugs.
Another one and half reasons are
* Can there be two or more such options used on a single command
line? Unless we limit the command line and say "such an option
can appear only at the end of options and without any non option
arguments" (the latter is what you said above), we'd end up with
a system that cannot be explained to casual users.
* What about short-form of option? Is "-abc" asking for three
options "-a", "-b", and "-c"? Or is it "-a" option that takes
optional argument "bc"?
There are some advices we give in "git help cli" to our users, which
we shouldn't have to have given if we rejected flags with optional
arguments in our commands.
Thanks.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
` (3 preceding siblings ...)
2023-03-05 12:22 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Sergey Organov
@ 2023-03-06 16:24 ` Phillip Wood
2023-03-06 17:36 ` Alex Henrie
2023-03-08 0:13 ` Glen Choo
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
6 siblings, 1 reply; 96+ messages in thread
From: Phillip Wood @ 2023-03-06 16:24 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, chooglen, calvinwan,
jonathantanmy
Hi Alex
On 05/03/2023 05:07, Alex Henrie wrote:
> This patch series introduces a rebase.rebaseMerges config option to
> accommodate users who would like --rebase-merges to be on by default and
> to facilitate turning on --rebase-merges by default without
> configuration in a future version of Git. It also cleans up and
> documents the behavior of the --rebase-merges command line option to
> avoid confusion about how the config option and the command line option
> interact.
>
> Changes from v5:
> - Add commit message note about --no-rebase-merges having always worked
> - Add commit message note about the test for --no-rebase-merges being
> somewhat contrived
> - Rephrase the documentation to avoid using the phrase "By default" with
> two different meanings, and in so doing clarify that --rebase-merges
> without an argument is not the same as --no-rebase-merges and not
> passing --rebase-merges is not the same as passing
> --rebase-merges=no-rebase-cousins
> - Add commit message note about keeping --rebase-merges="" for now out
> of an abundance of caution
> - Rephrase the warning about --rebase-merges="" to recommend
> --rebase-merges without an argument instead, and clarify that that
> will do the same thing
> - Remove the test for --rebase-merges=""
> - Rename the config option from rebase.merges to rebase.rebaseMerges and
> explain why in the commit message
> - Add commit message note about why "true" is a valid option for
> rebase.rebaseMerges and why --rebase-merges without an argument does
> not clobber the mode specified in rebase.rebaseMerges
> - Rephrase the documentation to clarify that --rebase-merges without an
> argument does not clobber the mode specified in rebase.rebaseMerges
> - Add another test for incompatible options
>
> Suggestions on v5 not incorporated in v6:
Thanks for adding this, it is really helpful to see what did not change
as well as what did change. It is also helpful to briefly explain why
you disagree with the suggestions so others can understand why you
decided not to make these changes.
> - Make --rebase-merges without an argument clobber the mode specified in
> rebase.rebaseMerges
I'm still confused as to why we want the config value to change the
behavior of --rebase-merges. Is it for "git pull --rebase=merges"? If
that's the case then I think a better approach is for pull to parse
rebase.merges and pass the appropriate argument to rebase. That way we
don't break the expectation that command line arguments override config
values.
> - Remove the test for --rebase-merges=no-rebase-cousins overriding
> rebase.rebaseMerges=rebase-cousins
> - In the tests, check the graph itself instead of checking that the
> graph has not changed by checking that the commit hash has not changed
I'm not sure what value the existing test has if it only checks that
HEAD is unchanged after the rebase. It could be unchanged because the
rebase fast-forwarded or because it did nothing.
Best Wishes
Phillip
> Thanks to Glen, Calvin, Junio, Jonathan, Elijah, and Phillip for your
> feedback on v5. As before, if you feel strongly about one of the
> suggestions that I have not incorporated into v6, or if you see
> something else amiss, let's continue the discussion.
>
> Alex Henrie (3):
> rebase: add documentation and test for --no-rebase-merges
> rebase: deprecate --rebase-merges=""
> rebase: add a config option for --rebase-merges
>
> Documentation/config/rebase.txt | 11 ++++
> Documentation/git-rebase.txt | 19 ++++---
> builtin/rebase.c | 75 ++++++++++++++++++-------
> t/t3422-rebase-incompatible-options.sh | 17 ++++++
> t/t3430-rebase-merges.sh | 78 ++++++++++++++++++++++++++
> 5 files changed, 174 insertions(+), 26 deletions(-)
>
> Range-diff against v5:
> 1: 76e38ef9f8 ! 1: bf08c03ba7 rebase: add documentation and test for --no-rebase-merges
> @@ Metadata
> ## Commit message ##
> rebase: add documentation and test for --no-rebase-merges
>
> + As far as I can tell, --no-rebase-merges has always worked, but has
> + never been documented. It is especially important to document it before
> + a rebase.rebaseMerges option is introduced so that users know how to
> + override the config option on the command line. It's also important to
> + clarify that --rebase-merges without an argument is not the same as
> + --no-rebase-merges and not passing --rebase-merges is not the same as
> + passing --rebase-merges=no-rebase-cousins.
> +
> + A test case is necessary to make sure that --no-rebase-merges keeps
> + working after its code is refactored in the following patches of this
> + series. The test case is a little contrived: It's unlikely that a user
> + would type both --rebase-merges and --no-rebase-merges at the same time.
> + However, if an alias is defined which includes --rebase-merges, the user
> + might decide to add --no-rebase-merges to countermand that part of the
> + alias but leave alone other flags set by the alias.
> +
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>
> ## Documentation/git-rebase.txt ##
> @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
> + resolved/re-applied manually. `--no-rebase-merges` can be used to
> + countermand 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,
> +-By default, or when `no-rebase-cousins` was specified, commits which do not
> +-have `<upstream>` as direct ancestor will keep their original branch point,
> +-i.e. commits that would be excluded by linkgit:git-log[1]'s
> +-`--ancestry-path` option will keep their original ancestry by default. If
> +-the `rebase-cousins` mode is turned on, such commits are instead rebased
> +-onto `<upstream>` (or `<onto>`, if specified).
> ++When rebasing merges, there are two modes: `rebase-cousins` and
> ++`no-rebase-cousins`. If the mode is not specified, it defaults to
> ++`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
> ++`<upstream>` as direct ancestor will keep their original branch point, i.e.
> ++commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
> ++option will keep their original ancestry by default. In `rebase-cousins` mode,
> ++such commits are instead rebased onto `<upstream>` (or `<onto>`, if
> ++specified).
> + +
> + It is currently only possible to recreate the merge commits using the
> + `ort` merge strategy; different merge strategies can be used only via
>
> ## t/t3430-rebase-merges.sh ##
> @@ t/t3430-rebase-merges.sh: test_expect_success 'with a branch tip that was cherry-picked already' '
> 2: c6099e6dee ! 2: 26f98b8400 rebase: deprecate --rebase-merges=""
> @@ Commit message
>
> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
> empty string argument) has been an undocumented synonym of
> - --rebase-merges=no-rebase-cousins. Deprecate that syntax to avoid
> - confusion when a rebase.merges config option is introduced, where
> - rebase.merges="" will be equivalent to --no-rebase-merges.
> + --rebase-merges without an argument. Deprecate that syntax to avoid
> + confusion when a rebase.rebaseMerges config option is introduced, where
> + rebase.rebaseMerges="" will be equivalent to --no-rebase-merges.
> +
> + It is not likely that anyone is actually using this syntax, but just in
> + case, deprecate the empty string argument instead of dropping support
> + for it immediately.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> + warning(_("--rebase-merges with an empty string "
> + "argument is deprecated and will stop "
> + "working in a future version of Git. Use "
> -+ "--rebase-merges=no-rebase-cousins "
> -+ "instead."));
> ++ "--rebase-merges without an argument "
> ++ "instead, which does the same thing."));
> else if (!strcmp("rebase-cousins", rebase_merges))
> options.rebase_cousins = 1;
> else if (strcmp("no-rebase-cousins", rebase_merges))
> -
> - ## t/t3430-rebase-merges.sh ##
> -@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless asked for' '
> - EOF
> - '
> -
> -+test_expect_success '--rebase-merges="" is deprecated' '
> -+ git rebase --rebase-merges="" HEAD^ 2>actual &&
> -+ grep deprecated actual
> -+'
> -+
> - test_expect_success 'refs/rewritten/* is worktree-local' '
> - git worktree add wt &&
> - cat >wt/script-from-scratch <<-\EOF &&
> 3: 95cba9588c ! 3: 402365256c rebase: add a config option for --rebase-merges
> @@ Commit message
> rebase: add a config option for --rebase-merges
>
> 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.
> + --rebase-merges to be on by default and to facilitate turning on
> + --rebase-merges by default without configuration in a future version of
> + Git.
> +
> + Name the new option rebase.rebaseMerges, even though it is a little
> + redundant, for consistency with the name of the command line option and
> + to be clear when scrolling through values in the [rebase] section of
> + .gitconfig.
> +
> + In the future, the default rebase-merges mode may change from
> + no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
> + to the nonspecific value "true" for users who do not need or want to
> + care about the default changing in the future. Similarly, for users who
> + have --rebase-merges in an alias and want to get the future behavior
> + now, use the specific rebase-merges mode from the config if a specific
> + mode is not given on the command line.
>
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
>
> @@ Documentation/config/rebase.txt: rebase.rescheduleFailedExec::
> rebase.forkPoint::
> If set to false set `--no-fork-point` option by default.
> +
> -+rebase.merges::
> ++rebase.rebaseMerges::
> + 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`.
> ++ command line without an argument overrides a `rebase.rebaseMerges=false`
> ++ configuration, but the absence of a specific rebase-merges mode on the
> ++ command line does not counteract a specific mode set in the configuration.
>
> ## Documentation/git-rebase.txt ##
> @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
> @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
> 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
> ++ countermand both the `rebase.rebaseMerges` 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,
> + When rebasing merges, there are two modes: `rebase-cousins` and
> +-`no-rebase-cousins`. If the mode is not specified, it defaults to
> +-`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
> +-`<upstream>` as direct ancestor will keep their original branch point, i.e.
> +-commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
> +-option will keep their original ancestry by default. In `rebase-cousins` mode,
> +-such commits are instead rebased onto `<upstream>` (or `<onto>`, if
> +-specified).
> ++`no-rebase-cousins`. If the mode is not specified on the command line or in
> ++the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
> ++In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
> ++ancestor will keep their original branch point, i.e. commits that would be
> ++excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
> ++original ancestry by default. In `rebase-cousins` mode, such commits are
> ++instead rebased onto `<upstream>` (or `<onto>`, if specified).
> + +
> + It is currently only possible to recreate the merge commits using the
> + `ort` merge strategy; different merge strategies can be used only via
>
> ## builtin/rebase.c ##
> @@ builtin/rebase.c: struct rebase_options {
> @@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
> return 0;
> }
>
> -+ if (!strcmp(var, "rebase.merges")) {
> ++ if (!strcmp(var, "rebase.rebasemerges")) {
> + opts->config_rebase_merges = git_parse_maybe_bool(value);
> + if (opts->config_rebase_merges < 0) {
> + opts->config_rebase_merges = 1;
> @@ builtin/rebase.c: static int parse_opt_empty(const struct option *opt, const cha
> + warning(_("--rebase-merges with an empty string "
> + "argument is deprecated and will stop "
> + "working in a future version of Git. Use "
> -+ "--rebase-merges=no-rebase-cousins "
> -+ "instead."));
> -+ arg = "no-rebase-cousins";
> ++ "--rebase-merges without an argument "
> ++ "instead, which does the same thing."));
> ++ return 0;
> + }
> + parse_rebase_merges_value(options, arg);
> + }
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> - warning(_("--rebase-merges with an empty string "
> - "argument is deprecated and will stop "
> - "working in a future version of Git. Use "
> -- "--rebase-merges=no-rebase-cousins "
> -- "instead."));
> +- "--rebase-merges without an argument "
> +- "instead, which does the same thing."));
> - else if (!strcmp("rebase-cousins", rebase_merges))
> - options.rebase_cousins = 1;
> - else if (strcmp("no-rebase-cousins", rebase_merges))
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> + if (options.autosquash == -1 && options.config_autosquash == 1)
> die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash"));
> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> -+ die(_("apply options are incompatible with rebase.merges. Consider adding --no-rebase-merges"));
> ++ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
> else if (options.update_refs == -1 && options.config_update_refs == 1)
> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
> + else if (is_merge(&options))
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> options.autosquash = (options.autosquash >= 0) ? options.autosquash :
>
> ## t/t3422-rebase-incompatible-options.sh ##
> +@@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
> + test_must_fail git rebase $opt --reapply-cherry-picks A
> + "
> +
> ++ test_expect_success "$opt incompatible with --rebase-merges" "
> ++ git checkout B^0 &&
> ++ test_must_fail git rebase $opt --rebase-merges A
> ++ "
> ++
> + test_expect_success "$opt incompatible with --update-refs" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --update-refs A
> @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
> grep -e --no-autosquash err
> "
>
> -+ test_expect_success "$opt incompatible with rebase.merges" "
> ++ test_expect_success "$opt incompatible with rebase.rebaseMerges" "
> + git checkout B^0 &&
> -+ test_must_fail git -c rebase.merges=true rebase $opt A 2>err &&
> ++ test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
> + grep -e --no-rebase-merges err
> + "
> +
> @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
> git -c rebase.autosquash=true rebase --no-autosquash $opt A
> "
>
> -+ test_expect_success "$opt okay with overridden rebase.merges" "
> ++ test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
> + test_when_finished \"git reset --hard B^0\" &&
> + git checkout B^0 &&
> -+ git -c rebase.merges=true rebase --no-rebase-merges $opt A
> ++ git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
> + "
> +
> test_expect_success "$opt okay with overridden rebase.updateRefs" "
> @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {
> git checkout B^0 &&
>
> ## t/t3430-rebase-merges.sh ##
> -@@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated' '
> - grep deprecated actual
> +@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless asked for' '
> + EOF
> '
>
> -+test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> -+ test_config rebase.merges rebase-cousins &&
> ++test_expect_success '--rebase-merges="" is deprecated' '
> ++ git rebase --rebase-merges="" HEAD^ 2>actual &&
> ++ grep deprecated actual
> ++'
> ++
> ++test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> ++ test_config rebase.rebaseMerges rebase-cousins &&
> + git checkout -b config-rebase-cousins main &&
> + git rebase HEAD^ &&
> + test_cmp_graph HEAD^.. <<-\EOF
> @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
> + EOF
> +'
> +
> -+test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
> -+ test_config rebase.merges no-rebase-cousins &&
> ++test_expect_success '--no-rebase-merges overrides rebase.rebaseMerges=no-rebase-cousins' '
> ++ test_config rebase.rebaseMerges no-rebase-cousins &&
> + git checkout -b override-config-no-rebase-cousins E &&
> + git rebase --no-rebase-merges C &&
> + test_cmp_graph C.. <<-\EOF
> @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
> + EOF
> +'
> +
> -+test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
> -+ test_config rebase.merges rebase-cousins &&
> ++test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.rebaseMerges=rebase-cousins' '
> ++ test_config rebase.rebaseMerges rebase-cousins &&
> + git checkout -b override-config-rebase-cousins main &&
> + git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> + test_cmp_graph HEAD^.. <<-\EOF
> @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
> + EOF
> +'
> +
> -+test_expect_success '--rebase-merges overrides rebase.merges=false' '
> -+ test_config rebase.merges false &&
> ++test_expect_success '--rebase-merges overrides rebase.rebaseMerges=false' '
> ++ test_config rebase.rebaseMerges false &&
> + git checkout -b override-config-merges-false E &&
> + before="$(git rev-parse --verify HEAD)" &&
> + test_tick &&
> @@ t/t3430-rebase-merges.sh: test_expect_success '--rebase-merges="" is deprecated'
> + test_cmp_rev HEAD $before
> +'
> +
> -+test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
> -+ test_config rebase.merges rebase-cousins &&
> ++test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
> ++ test_config rebase.rebaseMerges rebase-cousins &&
> + git checkout -b no-override-config-rebase-cousins main &&
> + git rebase --rebase-merges HEAD^ &&
> + test_cmp_graph HEAD^.. <<-\EOF
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-06 16:24 ` Phillip Wood
@ 2023-03-06 17:36 ` Alex Henrie
2023-03-07 15:07 ` Phillip Wood
0 siblings, 1 reply; 96+ messages in thread
From: Alex Henrie @ 2023-03-06 17:36 UTC (permalink / raw)
To: phillip.wood
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
On Mon, Mar 6, 2023 at 9:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 05/03/2023 05:07, Alex Henrie wrote:
> > Suggestions on v5 not incorporated in v6:
>
> Thanks for adding this, it is really helpful to see what did not change
> as well as what did change. It is also helpful to briefly explain why
> you disagree with the suggestions so others can understand why you
> decided not to make these changes.
>
> > - Make --rebase-merges without an argument clobber the mode specified in
> > rebase.rebaseMerges
>
> I'm still confused as to why we want the config value to change the
> behavior of --rebase-merges. Is it for "git pull --rebase=merges"? If
> that's the case then I think a better approach is for pull to parse
> rebase.merges and pass the appropriate argument to rebase. That way we
> don't break the expectation that command line arguments override config
> values.
>
> > - Remove the test for --rebase-merges=no-rebase-cousins overriding
> > rebase.rebaseMerges=rebase-cousins
> > - In the tests, check the graph itself instead of checking that the
> > graph has not changed by checking that the commit hash has not changed
>
> I'm not sure what value the existing test has if it only checks that
> HEAD is unchanged after the rebase. It could be unchanged because the
> rebase fast-forwarded or because it did nothing.
Please have a look at the email I sent before sending v6:
https://lore.kernel.org/git/CAMMLpeRfD+81fsEtvKFvVepPpwYm0_-AD=QHMwhoc+LtiXpavw@mail.gmail.com/
In that email I tried to explain why I didn't incorporate your three
suggestions on v5. Please let me know if it still isn't clear.
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-06 17:36 ` Alex Henrie
@ 2023-03-07 15:07 ` Phillip Wood
0 siblings, 0 replies; 96+ messages in thread
From: Phillip Wood @ 2023-03-07 15:07 UTC (permalink / raw)
To: Alex Henrie
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Hi Alex
On 06/03/2023 17:36, Alex Henrie wrote:
> On Mon, Mar 6, 2023 at 9:24 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> Please have a look at the email I sent before sending v6:
> https://lore.kernel.org/git/CAMMLpeRfD+81fsEtvKFvVepPpwYm0_-AD=QHMwhoc+LtiXpavw@mail.gmail.com/
Thanks for the link, I'd missed that email.
> In that email I tried to explain why I didn't incorporate your three
> suggestions on v5. Please let me know if it still isn't clear.
I'll take a look in the next couple of days and let you know
Thanks
Phillip
> -Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
` (4 preceding siblings ...)
2023-03-06 16:24 ` Phillip Wood
@ 2023-03-08 0:13 ` Glen Choo
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
6 siblings, 0 replies; 96+ messages in thread
From: Glen Choo @ 2023-03-08 0:13 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, calvinwan, jonathantanmy
Cc: Alex Henrie
Alex Henrie <alexhenrie24@gmail.com> writes:
> Changes from v5:
> - Add commit message note about --no-rebase-merges having always worked
> - Add commit message note about the test for --no-rebase-merges being
> somewhat contrived
> - Rephrase the documentation to avoid using the phrase "By default" with
> two different meanings, and in so doing clarify that --rebase-merges
> without an argument is not the same as --no-rebase-merges and not
> passing --rebase-merges is not the same as passing
> --rebase-merges=no-rebase-cousins
> - Add commit message note about keeping --rebase-merges="" for now out
> of an abundance of caution
> - Rephrase the warning about --rebase-merges="" to recommend
> --rebase-merges without an argument instead, and clarify that that
> will do the same thing
> - Remove the test for --rebase-merges=""
> - Rename the config option from rebase.merges to rebase.rebaseMerges and
> explain why in the commit message
> - Add commit message note about why "true" is a valid option for
> rebase.rebaseMerges and why --rebase-merges without an argument does
> not clobber the mode specified in rebase.rebaseMerges
> - Rephrase the documentation to clarify that --rebase-merges without an
> argument does not clobber the mode specified in rebase.rebaseMerges
> - Add another test for incompatible options
This version addresses all of the concerns I had with the previous
version. Thanks!
Besides the concerns that other reviewers raised and a possible
mechanical error, I don't have any outstanding concerns. I'd be happy
to see this merged when those are resolved :)
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v7 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
` (5 preceding siblings ...)
2023-03-08 0:13 ` Glen Choo
@ 2023-03-12 21:04 ` Alex Henrie
2023-03-12 21:04 ` [PATCH v7 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
` (3 more replies)
6 siblings, 4 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-12 21:04 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
This patch series introduces a rebase.rebaseMerges config option to
accommodate users who would like --rebase-merges to be on by default and
to facilitate turning on --rebase-merges by default without
configuration in a future version of Git. It also cleans up and
documents the behavior of the --rebase-merges command line option to
avoid confusion about how the config option and the command line option
interact.
Changes from v6:
- Don't say that the default rebase-merges mode will likely change to
rebase-cousins (although it might change to something else)
- In git-config.txt, say that rebase.rebaseMerges=true is equivalent
to --rebase-merges=no-rebase-cousins
- Add a link from the revised paragraph in git-rebase.txt to the
corresponding new section in git-config.txt
- Clear rebase_cousins if rebase.rebaseMerges is set to true after
having been set to rebase-cousins or no-rebase-cousins
- Actually remove the test for --rebase-merges=""
- Remove the test for --rebase-merges=no-rebase-cousins overriding
rebase.rebaseMerges=rebase-cousins
Suggestions on v6 not incorporated in v7:
- Make --rebase-merges without an argument clobber the mode specified in
rebase.rebaseMerges
- In the tests, pass --force and check the graph itself or the reflog
instead of checking that the graph has not changed by checking that
the commit hash has not changed
- Add --rebase-merges=on as a synonym of
--rebase-merges=no-rebase-cousins and --rebase-merges=off as a synonym
of --no-rebase-merges
- Rewrite the documentation for rebase-cousins to more clearly explain
the difference between rebase-cousins and no-rebase-cousins
Thanks to Phillip, Junio, Glen, and Sergey for your feedback on v6.
Alex Henrie (3):
rebase: add documentation and test for --no-rebase-merges
rebase: deprecate --rebase-merges=""
rebase: add a config option for --rebase-merges
Documentation/config/rebase.txt | 11 ++++
Documentation/git-rebase.txt | 20 ++++---
builtin/rebase.c | 76 +++++++++++++++++++-------
t/t3422-rebase-incompatible-options.sh | 17 ++++++
t/t3430-rebase-merges.sh | 58 ++++++++++++++++++++
5 files changed, 156 insertions(+), 26 deletions(-)
Range-diff against v6:
1: bf08c03ba7 = 1: 3aee0c2277 rebase: add documentation and test for --no-rebase-merges
2: 26f98b8400 = 2: e57843d8b5 rebase: deprecate --rebase-merges=""
3: 402365256c ! 3: b0c1a4dcb2 rebase: add a config option for --rebase-merges
@@ Commit message
.gitconfig.
In the future, the default rebase-merges mode may change from
- no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
- to the nonspecific value "true" for users who do not need or want to
- care about the default changing in the future. Similarly, for users who
- have --rebase-merges in an alias and want to get the future behavior
- now, use the specific rebase-merges mode from the config if a specific
- mode is not given on the command line.
+ no-rebase-cousins to some other mode that doesn't exist yet. Support
+ setting rebase.rebaseMerges to the nonspecific value "true" for users
+ who do not need or want to care about the default changing in the
+ future. Similarly, for users who have --rebase-merges in an alias and
+ want to get the future behavior now, use the specific rebase-merges mode
+ from the config if a specific mode is not given on the command line.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
@@ Documentation/config/rebase.txt: rebase.rescheduleFailedExec::
+rebase.rebaseMerges::
+ 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
++ true or to `no-rebase-cousins` is equivalent to
++ `--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
++ equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
++ equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
+ command line without an argument overrides a `rebase.rebaseMerges=false`
+ configuration, but the absence of a specific rebase-merges mode on the
+ command line does not counteract a specific mode set in the configuration.
@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
-such commits are instead rebased onto `<upstream>` (or `<onto>`, if
-specified).
+`no-rebase-cousins`. If the mode is not specified on the command line or in
-+the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
-+In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
++the `rebase.rebaseMerges` config option (see linkgit:git-config[1] or
++"CONFIGURATION" below), it defaults to `no-rebase-cousins`. In
++`no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
+ancestor will keep their original branch point, i.e. commits that would be
+excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
+original ancestry by default. In `rebase-cousins` mode, such commits are
@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
+ if (opts->config_rebase_merges < 0) {
+ opts->config_rebase_merges = 1;
+ parse_rebase_merges_value(opts, value);
-+ }
++ } else
++ opts->rebase_cousins = 0;
+ return 0;
+ }
+
@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless aske
EOF
'
-+test_expect_success '--rebase-merges="" is deprecated' '
-+ git rebase --rebase-merges="" HEAD^ 2>actual &&
-+ grep deprecated actual
-+'
-+
+test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
+ test_config rebase.rebaseMerges rebase-cousins &&
+ git checkout -b config-rebase-cousins main &&
@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless aske
+ EOF
+'
+
-+test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.rebaseMerges=rebase-cousins' '
-+ test_config rebase.rebaseMerges 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.rebaseMerges=false' '
+ test_config rebase.rebaseMerges false &&
+ git checkout -b override-config-merges-false E &&
--
2.39.2
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v7 1/3] rebase: add documentation and test for --no-rebase-merges
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
@ 2023-03-12 21:04 ` Alex Henrie
2023-03-12 21:04 ` [PATCH v7 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
` (2 subsequent siblings)
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-12 21:04 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
As far as I can tell, --no-rebase-merges has always worked, but has
never been documented. It is especially important to document it before
a rebase.rebaseMerges option is introduced so that users know how to
override the config option on the command line. It's also important to
clarify that --rebase-merges without an argument is not the same as
--no-rebase-merges and not passing --rebase-merges is not the same as
passing --rebase-merges=no-rebase-cousins.
A test case is necessary to make sure that --no-rebase-merges keeps
working after its code is refactored in the following patches of this
series. The test case is a little contrived: It's unlikely that a user
would type both --rebase-merges and --no-rebase-merges at the same time.
However, if an alias is defined which includes --rebase-merges, the user
might decide to add --no-rebase-merges to countermand that part of the
alias but leave alone other flags set by the alias.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/git-rebase.txt | 18 +++++++++++-------
t/t3430-rebase-merges.sh | 10 ++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9a295bcee4..4e57a87624 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -529,20 +529,24 @@ See also INCOMPATIBLE OPTIONS below.
-r::
--rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
+--no-rebase-merges::
By default, a rebase will simply drop merge commits from the todo
list, and put the rebased commits into a single, linear branch.
With `--rebase-merges`, the rebase will instead try to preserve
the branching structure within the commits that are to be rebased,
by recreating the merge commits. Any resolved merge conflicts or
manual amendments in these merge commits will have to be
- resolved/re-applied manually.
+ resolved/re-applied manually. `--no-rebase-merges` can be used to
+ countermand 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,
-i.e. commits that would be excluded by linkgit:git-log[1]'s
-`--ancestry-path` option will keep their original ancestry by default. If
-the `rebase-cousins` mode is turned on, such commits are instead rebased
-onto `<upstream>` (or `<onto>`, if specified).
+When rebasing merges, there are two modes: `rebase-cousins` and
+`no-rebase-cousins`. If the mode is not specified, it defaults to
+`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
+`<upstream>` as direct ancestor will keep their original branch point, i.e.
+commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
+option will keep their original ancestry by default. In `rebase-cousins` mode,
+such commits are instead rebased onto `<upstream>` (or `<onto>`, if
+specified).
+
It is currently only possible to recreate the merge commits using the
`ort` merge strategy; different merge strategies can be used only via
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index fa2a06c19f..d46d9545f2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -250,6 +250,16 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
EOF
'
+test_expect_success '--no-rebase-merges countermands --rebase-merges' '
+ git checkout -b no-rebase-merges E &&
+ git rebase --rebase-merges --no-rebase-merges C &&
+ test_cmp_graph C.. <<-\EOF
+ * B
+ * D
+ o C
+ EOF
+'
+
test_expect_success 'do not rebase cousins unless asked for' '
git checkout -b cousins main &&
before="$(git rev-parse --verify HEAD)" &&
--
2.39.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v7 2/3] rebase: deprecate --rebase-merges=""
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
2023-03-12 21:04 ` [PATCH v7 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
@ 2023-03-12 21:04 ` Alex Henrie
2023-03-12 21:04 ` [PATCH v7 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-12 21:04 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
empty string argument) has been an undocumented synonym of
--rebase-merges without an argument. Deprecate that syntax to avoid
confusion when a rebase.rebaseMerges config option is introduced, where
rebase.rebaseMerges="" will be equivalent to --no-rebase-merges.
It is not likely that anyone is actually using this syntax, but just in
case, deprecate the empty string argument instead of dropping support
for it immediately.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
builtin/rebase.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6635f10d52..c36ddc0050 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)""},
+ PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1438,7 +1438,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (rebase_merges) {
if (!*rebase_merges)
- ; /* default mode; do nothing */
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
else if (!strcmp("rebase-cousins", rebase_merges))
options.rebase_cousins = 1;
else if (strcmp("no-rebase-cousins", rebase_merges))
--
2.39.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v7 3/3] rebase: add a config option for --rebase-merges
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
2023-03-12 21:04 ` [PATCH v7 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-12 21:04 ` [PATCH v7 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
@ 2023-03-12 21:04 ` Alex Henrie
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-12 21:04 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy
Cc: Alex Henrie
The purpose of the new option is to accommodate users who would like
--rebase-merges to be on by default and to facilitate turning on
--rebase-merges by default without configuration in a future version of
Git.
Name the new option rebase.rebaseMerges, even though it is a little
redundant, for consistency with the name of the command line option and
to be clear when scrolling through values in the [rebase] section of
.gitconfig.
In the future, the default rebase-merges mode may change from
no-rebase-cousins to some other mode that doesn't exist yet. Support
setting rebase.rebaseMerges to the nonspecific value "true" for users
who do not need or want to care about the default changing in the
future. Similarly, for users who have --rebase-merges in an alias and
want to get the future behavior now, use the specific rebase-merges mode
from the config if a specific mode is not given on the command line.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/config/rebase.txt | 11 ++++
Documentation/git-rebase.txt | 18 +++---
builtin/rebase.c | 80 ++++++++++++++++++--------
t/t3422-rebase-incompatible-options.sh | 17 ++++++
t/t3430-rebase-merges.sh | 48 ++++++++++++++++
5 files changed, 143 insertions(+), 31 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f19bd0e040..98e8f193d4 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -67,3 +67,14 @@ rebase.rescheduleFailedExec::
rebase.forkPoint::
If set to false set `--no-fork-point` option by default.
+
+rebase.rebaseMerges::
+ Whether and how to set the `--rebase-merges` option by default. Can
+ be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
+ true or to `no-rebase-cousins` is equivalent to
+ `--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
+ equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
+ equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
+ command line without an argument overrides a `rebase.rebaseMerges=false`
+ configuration, but the absence of a specific rebase-merges mode on the
+ command line does not counteract a specific mode set in the configuration.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 4e57a87624..7ff02f474b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -537,16 +537,18 @@ 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.rebaseMerges` config option and a previous
+ `--rebase-merges`.
+
When rebasing merges, there are two modes: `rebase-cousins` and
-`no-rebase-cousins`. If the mode is not specified, it defaults to
-`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
-`<upstream>` as direct ancestor will keep their original branch point, i.e.
-commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
-option will keep their original ancestry by default. In `rebase-cousins` mode,
-such commits are instead rebased onto `<upstream>` (or `<onto>`, if
-specified).
+`no-rebase-cousins`. If the mode is not specified on the command line or in
+the `rebase.rebaseMerges` config option (see linkgit:git-config[1] or
+"CONFIGURATION" below), it defaults to `no-rebase-cousins`. In
+`no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
+ancestor will keep their original branch point, i.e. commits that would be
+excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
+original ancestry by default. In `rebase-cousins` mode, such commits are
+instead rebased onto `<upstream>` (or `<onto>`, if specified).
+
It is currently only possible to recreate the merge commits using the
`ort` merge strategy; different merge strategies can be used only via
diff --git a/builtin/rebase.c b/builtin/rebase.c
index c36ddc0050..593db9e53f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -123,6 +123,7 @@ struct rebase_options {
int fork_point;
int update_refs;
int config_autosquash;
+ int config_rebase_merges;
int config_update_refs;
};
@@ -140,6 +141,8 @@ struct rebase_options {
.allow_empty_message = 1, \
.autosquash = -1, \
.config_autosquash = -1, \
+ .rebase_merges = -1, \
+ .config_rebase_merges = -1, \
.update_refs = -1, \
.config_update_refs = -1, \
}
@@ -771,6 +774,16 @@ static int run_specific_rebase(struct rebase_options *opts)
return status ? -1 : 0;
}
+static void parse_rebase_merges_value(struct rebase_options *options, const char *value)
+{
+ if (!strcmp("no-rebase-cousins", value))
+ options->rebase_cousins = 0;
+ else if (!strcmp("rebase-cousins", value))
+ options->rebase_cousins = 1;
+ else
+ die(_("Unknown rebase-merges mode: %s"), value);
+}
+
static int rebase_config(const char *var, const char *value, void *data)
{
struct rebase_options *opts = data;
@@ -800,6 +813,16 @@ static int rebase_config(const char *var, const char *value, void *data)
return 0;
}
+ if (!strcmp(var, "rebase.rebasemerges")) {
+ opts->config_rebase_merges = git_parse_maybe_bool(value);
+ if (opts->config_rebase_merges < 0) {
+ opts->config_rebase_merges = 1;
+ parse_rebase_merges_value(opts, value);
+ } else
+ opts->rebase_cousins = 0;
+ return 0;
+ }
+
if (!strcmp(var, "rebase.updaterefs")) {
opts->config_update_refs = git_config_bool(var, value);
return 0;
@@ -980,6 +1003,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
return 0;
}
+static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
+{
+ struct rebase_options *options = opt->value;
+
+ options->rebase_merges = !unset;
+
+ if (arg) {
+ if (!*arg) {
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
+ return 0;
+ }
+ parse_rebase_merges_value(options, arg);
+ }
+
+ return 0;
+}
+
static void NORETURN error_on_missing_default_upstream(void)
{
struct branch *current_branch = branch_get(NULL);
@@ -1035,7 +1079,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
struct object_id branch_base;
int ignore_whitespace = 0;
const char *gpg_sign = NULL;
- const char *rebase_merges = NULL;
struct string_list strategy_options = STRING_LIST_INIT_NODUP;
struct object_id squash_onto;
char *squash_onto_name = NULL;
@@ -1137,10 +1180,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
&options.allow_empty_message,
N_("allow rebasing commits with empty messages"),
PARSE_OPT_HIDDEN),
- {OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
- N_("mode"),
+ OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
+ PARSE_OPT_OPTARG, parse_opt_rebase_merges),
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1436,21 +1478,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.exec.nr)
imply_merge(&options, "--exec");
- if (rebase_merges) {
- if (!*rebase_merges)
- warning(_("--rebase-merges with an empty string "
- "argument is deprecated and will stop "
- "working in a future version of Git. Use "
- "--rebase-merges without an argument "
- "instead, which does the same thing."));
- else 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;
- imply_merge(&options, "--rebase-merges");
- }
-
if (options.type == REBASE_APPLY) {
if (ignore_whitespace)
strvec_push(&options.git_am_opts,
@@ -1513,13 +1540,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
if (i >= 0 || options.type == REBASE_APPLY) {
- if (is_merge(&options))
- die(_("apply options and merge options "
- "cannot be used together"));
- else if (options.autosquash == -1 && options.config_autosquash == 1)
+ if (options.autosquash == -1 && options.config_autosquash == 1)
die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash"));
+ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
+ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
+ else if (is_merge(&options))
+ die(_("apply options and merge options "
+ "cannot be used together"));
else
options.type = REBASE_APPLY;
}
@@ -1530,6 +1559,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.update_refs = (options.update_refs >= 0) ? options.update_refs :
((options.config_update_refs >= 0) ? options.config_update_refs : 0);
+ if (options.rebase_merges == 1)
+ imply_merge(&options, "--rebase-merges");
+ options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
+ ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
+
if (options.autosquash == 1)
imply_merge(&options, "--autosquash");
options.autosquash = (options.autosquash >= 0) ? options.autosquash :
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 4711b37a28..2eba00bdf5 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -85,6 +85,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --reapply-cherry-picks A
"
+ test_expect_success "$opt incompatible with --rebase-merges" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --rebase-merges A
+ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
test_must_fail git rebase $opt --update-refs A
@@ -101,6 +106,12 @@ test_rebase_am_only () {
grep -e --no-autosquash err
"
+ test_expect_success "$opt incompatible with rebase.rebaseMerges" "
+ git checkout B^0 &&
+ test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
+ grep -e --no-rebase-merges err
+ "
+
test_expect_success "$opt incompatible with rebase.updateRefs" "
git checkout B^0 &&
test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
@@ -113,6 +124,12 @@ test_rebase_am_only () {
git -c rebase.autosquash=true rebase --no-autosquash $opt A
"
+ test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
+ test_when_finished \"git reset --hard B^0\" &&
+ git checkout B^0 &&
+ git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
+ "
+
test_expect_success "$opt okay with overridden rebase.updateRefs" "
test_when_finished \"git reset --hard B^0\" &&
git checkout B^0 &&
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index d46d9545f2..a8d9297224 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -278,6 +278,54 @@ test_expect_success 'do not rebase cousins unless asked for' '
EOF
'
+test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' '
+ test_config rebase.rebaseMerges 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 overrides rebase.rebaseMerges=false' '
+ test_config rebase.rebaseMerges false &&
+ git checkout -b override-config-merges-false E &&
+ before="$(git rev-parse --verify HEAD)" &&
+ test_tick &&
+ git rebase --rebase-merges C &&
+ test_cmp_rev HEAD $before
+'
+
+test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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 &&
--
2.39.2
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v8 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
` (2 preceding siblings ...)
2023-03-12 21:04 ` [PATCH v7 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-03-20 5:59 ` Alex Henrie
2023-03-20 5:59 ` [PATCH v8 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
` (3 more replies)
3 siblings, 4 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-20 5:59 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
This patch series introduces a rebase.rebaseMerges config option to
accommodate users who would like --rebase-merges to be on by default and
to facilitate turning on --rebase-merges by default without
configuration in a future version of Git. It also cleans up and
documents the behavior of the --rebase-merges command line option to
avoid confusion about how the config option and the command line option
interact.
Changes from v7:
- Make --rebase-merges without an argument clobber the mode specified in
rebase.rebaseMerges
- Replace the test for --rebase-merges not overriding
rebase.rebaseMerges=rebase-cousins and the test for --rebase-merges
overriding rebase.rebaseMerges=false with a single test that
--rebase-merges does override rebase.rebaseMerges=rebase-cousins
Thanks to Phillip, Felipe, Junio, and Glen for your feedback on v7.
Alex Henrie (3):
rebase: add documentation and test for --no-rebase-merges
rebase: deprecate --rebase-merges=""
rebase: add a config option for --rebase-merges
Documentation/config/rebase.txt | 10 ++++
Documentation/git-rebase.txt | 19 ++++---
builtin/rebase.c | 77 +++++++++++++++++++-------
t/t3422-rebase-incompatible-options.sh | 17 ++++++
t/t3430-rebase-merges.sh | 44 +++++++++++++++
5 files changed, 141 insertions(+), 26 deletions(-)
Range-diff against v7:
1: 3aee0c2277 = 1: 09fb7c1b74 rebase: add documentation and test for --no-rebase-merges
2: e57843d8b5 = 2: a846716a4a rebase: deprecate --rebase-merges=""
3: b0c1a4dcb2 ! 3: b12a3610ba rebase: add a config option for --rebase-merges
@@ Commit message
to be clear when scrolling through values in the [rebase] section of
.gitconfig.
- In the future, the default rebase-merges mode may change from
- no-rebase-cousins to some other mode that doesn't exist yet. Support
- setting rebase.rebaseMerges to the nonspecific value "true" for users
- who do not need or want to care about the default changing in the
- future. Similarly, for users who have --rebase-merges in an alias and
- want to get the future behavior now, use the specific rebase-merges mode
- from the config if a specific mode is not given on the command line.
+ Support setting rebase.rebaseMerges to the nonspecific value "true" for
+ users who don't need to or don't want to learn about the difference
+ between rebase-cousins and no-rebase-cousins.
+
+ Make --rebase-merges without an argument on the command line override
+ any value of rebase.rebaseMerges in the configuration, for consistency
+ with other command line flags with optional arguments that have an
+ associated config option.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
@@ Documentation/config/rebase.txt: rebase.rescheduleFailedExec::
+ `--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
+ equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
+ equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
-+ command line without an argument overrides a `rebase.rebaseMerges=false`
-+ configuration, but the absence of a specific rebase-merges mode on the
-+ command line does not counteract a specific mode set in the configuration.
++ command line, with or without an argument, overrides any
++ `rebase.rebaseMerges` configuration.
## Documentation/git-rebase.txt ##
@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
+ `--rebase-merges`.
+
When rebasing merges, there are two modes: `rebase-cousins` and
--`no-rebase-cousins`. If the mode is not specified, it defaults to
--`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
--`<upstream>` as direct ancestor will keep their original branch point, i.e.
--commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
--option will keep their original ancestry by default. In `rebase-cousins` mode,
--such commits are instead rebased onto `<upstream>` (or `<onto>`, if
--specified).
-+`no-rebase-cousins`. If the mode is not specified on the command line or in
-+the `rebase.rebaseMerges` config option (see linkgit:git-config[1] or
-+"CONFIGURATION" below), it defaults to `no-rebase-cousins`. In
-+`no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
-+ancestor will keep their original branch point, i.e. commits that would be
-+excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
-+original ancestry by default. In `rebase-cousins` mode, such commits are
-+instead rebased onto `<upstream>` (or `<onto>`, if specified).
- +
- It is currently only possible to recreate the merge commits using the
- `ort` merge strategy; different merge strategies can be used only via
+ `no-rebase-cousins`. If the mode is not specified, it defaults to
## builtin/rebase.c ##
@@ builtin/rebase.c: struct rebase_options {
@@ builtin/rebase.c: static int parse_opt_empty(const struct option *opt, const cha
+ struct rebase_options *options = opt->value;
+
+ options->rebase_merges = !unset;
++ options->rebase_cousins = 0;
+
+ if (arg) {
+ if (!*arg) {
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
- "cannot be used together"));
- else if (options.autosquash == -1 && options.config_autosquash == 1)
+ if (options.autosquash == -1 && options.config_autosquash == 1)
- die(_("apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash"));
+ die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
+ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
+ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
@@ t/t3430-rebase-merges.sh: test_expect_success 'do not rebase cousins unless aske
+ EOF
+'
+
-+test_expect_success '--rebase-merges overrides rebase.rebaseMerges=false' '
-+ test_config rebase.rebaseMerges false &&
-+ git checkout -b override-config-merges-false E &&
++test_expect_success '--rebase-merges overrides rebase.rebaseMerges=rebase-cousins' '
++ test_config rebase.rebaseMerges rebase-cousins &&
++ git checkout -b override-config-rebase-cousins E &&
+ before="$(git rev-parse --verify HEAD)" &&
+ test_tick &&
+ git rebase --rebase-merges C &&
+ test_cmp_rev HEAD $before
+'
-+
-+test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
-+ test_config rebase.rebaseMerges 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 &&
--
2.40.0
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v8 1/3] rebase: add documentation and test for --no-rebase-merges
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
@ 2023-03-20 5:59 ` Alex Henrie
2023-03-20 5:59 ` [PATCH v8 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
` (2 subsequent siblings)
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-20 5:59 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
As far as I can tell, --no-rebase-merges has always worked, but has
never been documented. It is especially important to document it before
a rebase.rebaseMerges option is introduced so that users know how to
override the config option on the command line. It's also important to
clarify that --rebase-merges without an argument is not the same as
--no-rebase-merges and not passing --rebase-merges is not the same as
passing --rebase-merges=no-rebase-cousins.
A test case is necessary to make sure that --no-rebase-merges keeps
working after its code is refactored in the following patches of this
series. The test case is a little contrived: It's unlikely that a user
would type both --rebase-merges and --no-rebase-merges at the same time.
However, if an alias is defined which includes --rebase-merges, the user
might decide to add --no-rebase-merges to countermand that part of the
alias but leave alone other flags set by the alias.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/git-rebase.txt | 18 +++++++++++-------
t/t3430-rebase-merges.sh | 10 ++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9a295bcee4..4e57a87624 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -529,20 +529,24 @@ See also INCOMPATIBLE OPTIONS below.
-r::
--rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
+--no-rebase-merges::
By default, a rebase will simply drop merge commits from the todo
list, and put the rebased commits into a single, linear branch.
With `--rebase-merges`, the rebase will instead try to preserve
the branching structure within the commits that are to be rebased,
by recreating the merge commits. Any resolved merge conflicts or
manual amendments in these merge commits will have to be
- resolved/re-applied manually.
+ resolved/re-applied manually. `--no-rebase-merges` can be used to
+ countermand 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,
-i.e. commits that would be excluded by linkgit:git-log[1]'s
-`--ancestry-path` option will keep their original ancestry by default. If
-the `rebase-cousins` mode is turned on, such commits are instead rebased
-onto `<upstream>` (or `<onto>`, if specified).
+When rebasing merges, there are two modes: `rebase-cousins` and
+`no-rebase-cousins`. If the mode is not specified, it defaults to
+`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
+`<upstream>` as direct ancestor will keep their original branch point, i.e.
+commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
+option will keep their original ancestry by default. In `rebase-cousins` mode,
+such commits are instead rebased onto `<upstream>` (or `<onto>`, if
+specified).
+
It is currently only possible to recreate the merge commits using the
`ort` merge strategy; different merge strategies can be used only via
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index fa2a06c19f..d46d9545f2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -250,6 +250,16 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
EOF
'
+test_expect_success '--no-rebase-merges countermands --rebase-merges' '
+ git checkout -b no-rebase-merges E &&
+ git rebase --rebase-merges --no-rebase-merges C &&
+ test_cmp_graph C.. <<-\EOF
+ * B
+ * D
+ o C
+ EOF
+'
+
test_expect_success 'do not rebase cousins unless asked for' '
git checkout -b cousins main &&
before="$(git rev-parse --verify HEAD)" &&
--
2.40.0
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v8 2/3] rebase: deprecate --rebase-merges=""
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-20 5:59 ` [PATCH v8 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
@ 2023-03-20 5:59 ` Alex Henrie
2023-03-20 5:59 ` [PATCH v8 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-20 5:59 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
empty string argument) has been an undocumented synonym of
--rebase-merges without an argument. Deprecate that syntax to avoid
confusion when a rebase.rebaseMerges config option is introduced, where
rebase.rebaseMerges="" will be equivalent to --no-rebase-merges.
It is not likely that anyone is actually using this syntax, but just in
case, deprecate the empty string argument instead of dropping support
for it immediately.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
builtin/rebase.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index dd31d5ab91..4b3f29a449 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1141,7 +1141,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)""},
+ PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1439,7 +1439,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (rebase_merges) {
if (!*rebase_merges)
- ; /* default mode; do nothing */
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
else if (!strcmp("rebase-cousins", rebase_merges))
options.rebase_cousins = 1;
else if (strcmp("no-rebase-cousins", rebase_merges))
--
2.40.0
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v8 3/3] rebase: add a config option for --rebase-merges
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-20 5:59 ` [PATCH v8 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-20 5:59 ` [PATCH v8 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
@ 2023-03-20 5:59 ` Alex Henrie
2023-03-22 16:54 ` Phillip Wood
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
3 siblings, 1 reply; 96+ messages in thread
From: Alex Henrie @ 2023-03-20 5:59 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
The purpose of the new option is to accommodate users who would like
--rebase-merges to be on by default and to facilitate turning on
--rebase-merges by default without configuration in a future version of
Git.
Name the new option rebase.rebaseMerges, even though it is a little
redundant, for consistency with the name of the command line option and
to be clear when scrolling through values in the [rebase] section of
.gitconfig.
Support setting rebase.rebaseMerges to the nonspecific value "true" for
users who don't need to or don't want to learn about the difference
between rebase-cousins and no-rebase-cousins.
Make --rebase-merges without an argument on the command line override
any value of rebase.rebaseMerges in the configuration, for consistency
with other command line flags with optional arguments that have an
associated config option.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/config/rebase.txt | 10 ++++
Documentation/git-rebase.txt | 3 +-
builtin/rebase.c | 81 ++++++++++++++++++--------
t/t3422-rebase-incompatible-options.sh | 17 ++++++
t/t3430-rebase-merges.sh | 34 +++++++++++
5 files changed, 121 insertions(+), 24 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f19bd0e040..afaf6dad99 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.rebaseMerges::
+ Whether and how to set the `--rebase-merges` option by default. Can
+ be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
+ true or to `no-rebase-cousins` is equivalent to
+ `--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
+ equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
+ equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
+ command line, with or without an argument, overrides any
+ `rebase.rebaseMerges` configuration.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 4e57a87624..e7b39ad244 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.rebaseMerges` config option and a previous
+ `--rebase-merges`.
+
When rebasing merges, there are two modes: `rebase-cousins` and
`no-rebase-cousins`. If the mode is not specified, it defaults to
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 4b3f29a449..fd284e24ab 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -124,6 +124,7 @@ struct rebase_options {
int fork_point;
int update_refs;
int config_autosquash;
+ int config_rebase_merges;
int config_update_refs;
};
@@ -141,6 +142,8 @@ struct rebase_options {
.allow_empty_message = 1, \
.autosquash = -1, \
.config_autosquash = -1, \
+ .rebase_merges = -1, \
+ .config_rebase_merges = -1, \
.update_refs = -1, \
.config_update_refs = -1, \
}
@@ -772,6 +775,16 @@ static int run_specific_rebase(struct rebase_options *opts)
return status ? -1 : 0;
}
+static void parse_rebase_merges_value(struct rebase_options *options, const char *value)
+{
+ if (!strcmp("no-rebase-cousins", value))
+ options->rebase_cousins = 0;
+ else if (!strcmp("rebase-cousins", value))
+ options->rebase_cousins = 1;
+ else
+ die(_("Unknown rebase-merges mode: %s"), value);
+}
+
static int rebase_config(const char *var, const char *value, void *data)
{
struct rebase_options *opts = data;
@@ -801,6 +814,16 @@ static int rebase_config(const char *var, const char *value, void *data)
return 0;
}
+ if (!strcmp(var, "rebase.rebasemerges")) {
+ opts->config_rebase_merges = git_parse_maybe_bool(value);
+ if (opts->config_rebase_merges < 0) {
+ opts->config_rebase_merges = 1;
+ parse_rebase_merges_value(opts, value);
+ } else
+ opts->rebase_cousins = 0;
+ return 0;
+ }
+
if (!strcmp(var, "rebase.updaterefs")) {
opts->config_update_refs = git_config_bool(var, value);
return 0;
@@ -981,6 +1004,28 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
return 0;
}
+static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
+{
+ struct rebase_options *options = opt->value;
+
+ options->rebase_merges = !unset;
+ options->rebase_cousins = 0;
+
+ if (arg) {
+ if (!*arg) {
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
+ return 0;
+ }
+ parse_rebase_merges_value(options, arg);
+ }
+
+ return 0;
+}
+
static void NORETURN error_on_missing_default_upstream(void)
{
struct branch *current_branch = branch_get(NULL);
@@ -1036,7 +1081,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
struct object_id branch_base;
int ignore_whitespace = 0;
const char *gpg_sign = NULL;
- const char *rebase_merges = NULL;
struct string_list strategy_options = STRING_LIST_INIT_NODUP;
struct object_id squash_onto;
char *squash_onto_name = NULL;
@@ -1138,10 +1182,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
&options.allow_empty_message,
N_("allow rebasing commits with empty messages"),
PARSE_OPT_HIDDEN),
- {OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
- N_("mode"),
+ OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
+ PARSE_OPT_OPTARG, parse_opt_rebase_merges),
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1437,21 +1480,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.exec.nr)
imply_merge(&options, "--exec");
- if (rebase_merges) {
- if (!*rebase_merges)
- warning(_("--rebase-merges with an empty string "
- "argument is deprecated and will stop "
- "working in a future version of Git. Use "
- "--rebase-merges without an argument "
- "instead, which does the same thing."));
- else 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;
- imply_merge(&options, "--rebase-merges");
- }
-
if (options.type == REBASE_APPLY) {
if (ignore_whitespace)
strvec_push(&options.git_am_opts,
@@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
break;
if (i >= 0 || options.type == REBASE_APPLY) {
- if (is_merge(&options))
- die(_("apply options and merge options "
- "cannot be used together"));
- else if (options.autosquash == -1 && options.config_autosquash == 1)
+ if (options.autosquash == -1 && options.config_autosquash == 1)
die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
+ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
+ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
+ else if (is_merge(&options))
+ die(_("apply options and merge options "
+ "cannot be used together"));
else
options.type = REBASE_APPLY;
}
@@ -1531,6 +1561,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.update_refs = (options.update_refs >= 0) ? options.update_refs :
((options.config_update_refs >= 0) ? options.config_update_refs : 0);
+ if (options.rebase_merges == 1)
+ imply_merge(&options, "--rebase-merges");
+ options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
+ ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
+
if (options.autosquash == 1)
imply_merge(&options, "--autosquash");
options.autosquash = (options.autosquash >= 0) ? options.autosquash :
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 4711b37a28..2eba00bdf5 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -85,6 +85,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --reapply-cherry-picks A
"
+ test_expect_success "$opt incompatible with --rebase-merges" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --rebase-merges A
+ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
test_must_fail git rebase $opt --update-refs A
@@ -101,6 +106,12 @@ test_rebase_am_only () {
grep -e --no-autosquash err
"
+ test_expect_success "$opt incompatible with rebase.rebaseMerges" "
+ git checkout B^0 &&
+ test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
+ grep -e --no-rebase-merges err
+ "
+
test_expect_success "$opt incompatible with rebase.updateRefs" "
git checkout B^0 &&
test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
@@ -113,6 +124,12 @@ test_rebase_am_only () {
git -c rebase.autosquash=true rebase --no-autosquash $opt A
"
+ test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
+ test_when_finished \"git reset --hard B^0\" &&
+ git checkout B^0 &&
+ git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
+ "
+
test_expect_success "$opt okay with overridden rebase.updateRefs" "
test_when_finished \"git reset --hard B^0\" &&
git checkout B^0 &&
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index d46d9545f2..f03599c63b 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -278,6 +278,40 @@ test_expect_success 'do not rebase cousins unless asked for' '
EOF
'
+test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' '
+ test_config rebase.rebaseMerges 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 overrides rebase.rebaseMerges=rebase-cousins' '
+ test_config rebase.rebaseMerges rebase-cousins &&
+ git checkout -b override-config-rebase-cousins E &&
+ before="$(git rev-parse --verify HEAD)" &&
+ test_tick &&
+ git rebase --rebase-merges C &&
+ test_cmp_rev HEAD $before
+'
+
test_expect_success 'refs/rewritten/* is worktree-local' '
git worktree add wt &&
cat >wt/script-from-scratch <<-\EOF &&
--
2.40.0
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges
2023-03-20 5:59 ` [PATCH v8 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-03-22 16:54 ` Phillip Wood
2023-03-23 18:45 ` Junio C Hamano
2023-03-25 5:21 ` Alex Henrie
0 siblings, 2 replies; 96+ messages in thread
From: Phillip Wood @ 2023-03-22 16:54 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, chooglen, calvinwan,
jonathantanmy, felipe.contreras
Hi Alex
On 20/03/2023 05:59, 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 turning on
> --rebase-merges by default without configuration in a future version of
> Git.
>
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.
>
> Support setting rebase.rebaseMerges to the nonspecific value "true" for
> users who don't need to or don't want to learn about the difference
> between rebase-cousins and no-rebase-cousins.
>
> Make --rebase-merges without an argument on the command line override
> any value of rebase.rebaseMerges in the configuration, for consistency
> with other command line flags with optional arguments that have an
> associated config option.
Hurray! Thanks for re-rolling. I've left a couple of comments below, I
had hoped this would be the final version but I've realized that the way
you've rearranged the error handling for apply and merge options is not
a good idea (see below). Apart from that I'm happy.
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
> Documentation/config/rebase.txt | 10 ++++
> Documentation/git-rebase.txt | 3 +-
> builtin/rebase.c | 81 ++++++++++++++++++--------
> t/t3422-rebase-incompatible-options.sh | 17 ++++++
> t/t3430-rebase-merges.sh | 34 +++++++++++
> 5 files changed, 121 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..afaf6dad99 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.rebaseMerges::
> + Whether and how to set the `--rebase-merges` option by default. Can
> + be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> + true or to `no-rebase-cousins` is equivalent to
> + `--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
> + equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
> + equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> + command line, with or without an argument, overrides any
> + `rebase.rebaseMerges` configuration.
Sounds good
> [...]
> + if (!strcmp(var, "rebase.rebasemerges")) {
> + opts->config_rebase_merges = git_parse_maybe_bool(value);
> + if (opts->config_rebase_merges < 0) {
> + opts->config_rebase_merges = 1;
> + parse_rebase_merges_value(opts, value);
> + } else
> + opts->rebase_cousins = 0;
The "else" clause should have braces because the "if" cause requires
them (see Documentation/CodingGuidelines). I don't think it is worth
re-rolling just for this though.
> [...]
> +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
> +{
> + struct rebase_options *options = opt->value;
> +
> + options->rebase_merges = !unset;
> + options->rebase_cousins = 0;
This makes --rebase-merges without an option override
rebase.rebaseMerges - good.
> [...]
> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> break;
>
> if (i >= 0 || options.type == REBASE_APPLY) {
> - if (is_merge(&options))
> - die(_("apply options and merge options "
> - "cannot be used together"));
This isn't a new change but having thought about it I'm not sure moving
this code is a good idea. If the user runs
git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
then instead of getting an message saying that they cannot use apply and
merge options together they'll get a message suggesting they pass
--no-update-refs which wont fix the problem for them.
> - else if (options.autosquash == -1 && options.config_autosquash == 1)
> + if (options.autosquash == -1 && options.config_autosquash == 1)
> die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> + die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
> else if (options.update_refs == -1 && options.config_update_refs == 1)
> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
> + else if (is_merge(&options))
> + die(_("apply options and merge options "
> + "cannot be used together"));
> else
> options.type = REBASE_APPLY;
> }
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges
2023-03-22 16:54 ` Phillip Wood
@ 2023-03-23 18:45 ` Junio C Hamano
2023-03-24 14:52 ` Phillip Wood
2023-03-25 5:23 ` Alex Henrie
2023-03-25 5:21 ` Alex Henrie
1 sibling, 2 replies; 96+ messages in thread
From: Junio C Hamano @ 2023-03-23 18:45 UTC (permalink / raw)
To: Phillip Wood
Cc: Alex Henrie, git, tao, newren, Johannes.Schindelin, sorganov,
chooglen, calvinwan, jonathantanmy, felipe.contreras
Phillip Wood <phillip.wood123@gmail.com> writes:
>> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>> break;
>> if (i >= 0 || options.type == REBASE_APPLY) {
>> - if (is_merge(&options))
>> - die(_("apply options and merge options "
>> - "cannot be used together"));
>
> This isn't a new change but having thought about it I'm not sure
> moving this code is a good idea. If the user runs
>
> git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
>
> then instead of getting an message saying that they cannot use apply
> and merge options together they'll get a message suggesting they pass
> --no-update-refs which wont fix the problem for them.
Hmph. Instead of dying here, ...
>> + if (options.autosquash == -1 && options.config_autosquash == 1)
>> die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
>> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>> + die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
>> else if (options.update_refs == -1 && options.config_update_refs == 1)
>> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
... we get caught here, and the next one is not triggered.
>> + else if (is_merge(&options))
>> + die(_("apply options and merge options "
>> + "cannot be used together"));
>> else
>> options.type = REBASE_APPLY;
What's the reason why "cannot be used together" is moved to the last
in the chain?
The first two new conditions in this chain try to catch an
invocation with some apply-specific command line option
(e.g. "--whitespace=fix") when used with configuration variables
specific to the merge-backend (e.g. "rebase.merges") and suggest
overriding the configuration from the command line, and I suspect
that the motivation behind this change is that their error messages
are more specific than the generic "apply and merge do not mix".
But are these two new errors and hints universally a good suggestion
to make? I actually think a command line option forcing us to use
the apply backend should simply silently ignore (aka "override")
configuration variables that take effect only when we are using the
merge-backend. "git rebase --whitespace=fix --rebase-merges",
giving both from the command line, is making conflicting and
unsatisfyable request, and it is worth giving an error message.
But if you configure rebase.autosquash only because you want to it
to be in effect for your invocations of "git rebase -i", you
shouldn't have to override it from the command line when you say
"git rebase" or "git rebase --whitespace=fix", should you?
Thanks.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges
2023-03-23 18:45 ` Junio C Hamano
@ 2023-03-24 14:52 ` Phillip Wood
2023-03-25 5:23 ` Alex Henrie
1 sibling, 0 replies; 96+ messages in thread
From: Phillip Wood @ 2023-03-24 14:52 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood
Cc: Alex Henrie, git, tao, newren, Johannes.Schindelin, sorganov,
chooglen, calvinwan, jonathantanmy, felipe.contreras
Hi Junio
On 23/03/2023 18:45, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>>> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>> break;
>>> if (i >= 0 || options.type == REBASE_APPLY) {
>>> - if (is_merge(&options))
>>> - die(_("apply options and merge options "
>>> - "cannot be used together"));
>>
>> This isn't a new change but having thought about it I'm not sure
>> moving this code is a good idea. If the user runs
>>
>> git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
>>
>> then instead of getting an message saying that they cannot use apply
>> and merge options together they'll get a message suggesting they pass
>> --no-update-refs which wont fix the problem for them.
>
> Hmph. Instead of dying here, ...
>
>>> + if (options.autosquash == -1 && options.config_autosquash == 1)
>>> die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
>>> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
>>> + die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
>>> else if (options.update_refs == -1 && options.config_update_refs == 1)
>>> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
>
> ... we get caught here, and the next one is not triggered.
>
>>> + else if (is_merge(&options))
>>> + die(_("apply options and merge options "
>>> + "cannot be used together"));
>>> else
>>> options.type = REBASE_APPLY;
>
> What's the reason why "cannot be used together" is moved to the last
> in the chain?
>
> The first two new conditions in this chain try to catch an
> invocation with some apply-specific command line option
> (e.g. "--whitespace=fix") when used with configuration variables
> specific to the merge-backend (e.g. "rebase.merges") and suggest
> overriding the configuration from the command line, and I suspect
> that the motivation behind this change is that their error messages
> are more specific than the generic "apply and merge do not mix".
>
> But are these two new errors and hints universally a good suggestion
> to make? I actually think a command line option forcing us to use
> the apply backend should simply silently ignore (aka "override")
> configuration variables that take effect only when we are using the
> merge-backend. "git rebase --whitespace=fix --rebase-merges",
> giving both from the command line, is making conflicting and
> unsatisfyable request, and it is worth giving an error message.
>
> But if you configure rebase.autosquash only because you want to it
> to be in effect for your invocations of "git rebase -i", you
> shouldn't have to override it from the command line when you say
> "git rebase" or "git rebase --whitespace=fix", should you?
I think there are two conflicting viewpoints here which depend on what
one thinks the user wants when they set these configuration variables.
As I understand it Elijah's thinking was that if the user set
rebase.autosquash they'd be surprised when their fixup commits were not
squashed by
git rebase --whitespace=fix
and that's why his patch series changed things to error out.
The other point of view is that the user expects that these
configuration variables to apply only when they are using the "merge"
backend and so we should not error out.
Personally I lean a little more towards the latter but I don't feel that
strongly about it and can see an argument for having either behavior. I
do think we should leave the ordering alone though so the user gets a
useful error message in the case of "git rebase --exec 'make test'
--whitespace=fix"
Best Wishes
Phillip
> Thanks.
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges
2023-03-23 18:45 ` Junio C Hamano
2023-03-24 14:52 ` Phillip Wood
@ 2023-03-25 5:23 ` Alex Henrie
1 sibling, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-25 5:23 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood, git, tao, newren, Johannes.Schindelin, sorganov,
chooglen, calvinwan, jonathantanmy, felipe.contreras
On Thu, Mar 23, 2023 at 12:45 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> @@ -1514,13 +1542,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >> break;
> >> if (i >= 0 || options.type == REBASE_APPLY) {
> >> - if (is_merge(&options))
> >> - die(_("apply options and merge options "
> >> - "cannot be used together"));
> >
> > This isn't a new change but having thought about it I'm not sure
> > moving this code is a good idea. If the user runs
> >
> > git -c rebase.updateRefs=true rebase --whitespace=fix --exec "make test"
> >
> > then instead of getting an message saying that they cannot use apply
> > and merge options together they'll get a message suggesting they pass
> > --no-update-refs which wont fix the problem for them.
>
> Hmph. Instead of dying here, ...
>
> >> + if (options.autosquash == -1 && options.config_autosquash == 1)
> >> die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
> >> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> >> + die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
> >> else if (options.update_refs == -1 && options.config_update_refs == 1)
> >> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
>
> ... we get caught here, and the next one is not triggered.
>
> >> + else if (is_merge(&options))
> >> + die(_("apply options and merge options "
> >> + "cannot be used together"));
> >> else
> >> options.type = REBASE_APPLY;
>
> What's the reason why "cannot be used together" is moved to the last
> in the chain?
>
> The first two new conditions in this chain try to catch an
> invocation with some apply-specific command line option
> (e.g. "--whitespace=fix") when used with configuration variables
> specific to the merge-backend (e.g. "rebase.merges") and suggest
> overriding the configuration from the command line, and I suspect
> that the motivation behind this change is that their error messages
> are more specific than the generic "apply and merge do not mix".
Phillip specifically asked for `git -c rebase.rebaseMerges=true rebase
--whitespace=fix` to print "error: apply options are incompatible with
rebase.rebaseMerges. Consider adding --no-rebase-merges".[1] I could
have sworn that "if (is_merge(&options))" had to be after "if
(options.rebase_merges == -1 && options.config_rebase_merges == 1)" to
make that happen, but now it works without that change. I must have
been debugging with some intermediate version that still had
'imply_merge(&options, "--rebase-merges")' before this if chain. I'll
send a v9 that puts "if (is_merge(&options))" back at the top.
Thanks for your attention to detail!
-Alex
[1] https://lore.kernel.org/git/7ba8a92d-8c94-5226-5416-6ed3a8e2e389@dunelm.org.uk/
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v8 3/3] rebase: add a config option for --rebase-merges
2023-03-22 16:54 ` Phillip Wood
2023-03-23 18:45 ` Junio C Hamano
@ 2023-03-25 5:21 ` Alex Henrie
1 sibling, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-25 5:21 UTC (permalink / raw)
To: phillip.wood
Cc: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
On Wed, Mar 22, 2023 at 10:54 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> Hurray! Thanks for re-rolling.
Thanks for making sure that we got the UI right!
> On 20/03/2023 05:59, Alex Henrie wrote:
> > + if (!strcmp(var, "rebase.rebasemerges")) {
> > + opts->config_rebase_merges = git_parse_maybe_bool(value);
> > + if (opts->config_rebase_merges < 0) {
> > + opts->config_rebase_merges = 1;
> > + parse_rebase_merges_value(opts, value);
> > + } else
> > + opts->rebase_cousins = 0;
>
> The "else" clause should have braces because the "if" cause requires
> them (see Documentation/CodingGuidelines). I don't think it is worth
> re-rolling just for this though.
Thanks for pointing out that documentation. I'm going to make a v9
anyway, and I'll add the braces then. By the way, actions speak louder
than words: While writing the patch, I found several examples of the
braces being omitted in cases like this in other places in rebase.c,
so I assumed that that was the preferred style here. If you want to
encourage people to follow the CodingGuidelines document, the best way
would be to clean up the existing code to conform to it.
-Alex
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v9 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
` (2 preceding siblings ...)
2023-03-20 5:59 ` [PATCH v8 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-03-26 3:06 ` Alex Henrie
2023-03-26 3:06 ` [PATCH v9 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
` (3 more replies)
3 siblings, 4 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-26 3:06 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
This patch series introduces a rebase.rebaseMerges config option to
accommodate users who would like --rebase-merges to be on by default and
to facilitate turning on --rebase-merges by default without
configuration in a future version of Git. It also cleans up and
documents the behavior of the --rebase-merges command line option to
avoid confusion about how the config option and the command line option
interact.
Changes from v8:
- Add braces around one-line else clause
- Remove unnecessary change to error message priority
Thanks to Phillip, Junio, Johannes and Sergey for your feedback on v8.
Alex Henrie (3):
rebase: add documentation and test for --no-rebase-merges
rebase: deprecate --rebase-merges=""
rebase: add a config option for --rebase-merges
Documentation/config/rebase.txt | 10 ++++
Documentation/git-rebase.txt | 19 ++++---
builtin/rebase.c | 70 ++++++++++++++++++++------
t/t3422-rebase-incompatible-options.sh | 17 +++++++
t/t3430-rebase-merges.sh | 44 ++++++++++++++++
5 files changed, 138 insertions(+), 22 deletions(-)
Range-diff against v8:
1: 09fb7c1b74 = 1: a22b9d0da2 rebase: add documentation and test for --no-rebase-merges
2: a846716a4a = 2: 112fee4833 rebase: deprecate --rebase-merges=""
3: b12a3610ba ! 3: 868899cd6d rebase: add a config option for --rebase-merges
@@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
+ if (opts->config_rebase_merges < 0) {
+ opts->config_rebase_merges = 1;
+ parse_rebase_merges_value(opts, value);
-+ } else
++ } else {
+ opts->rebase_cousins = 0;
++ }
+ return 0;
+ }
+
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
if (ignore_whitespace)
strvec_push(&options.git_am_opts,
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
- break;
-
- if (i >= 0 || options.type == REBASE_APPLY) {
-- if (is_merge(&options))
-- die(_("apply options and merge options "
-- "cannot be used together"));
-- else if (options.autosquash == -1 && options.config_autosquash == 1)
-+ if (options.autosquash == -1 && options.config_autosquash == 1)
+ "cannot be used together"));
+ else if (options.autosquash == -1 && options.config_autosquash == 1)
die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
+ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
+ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
-+ else if (is_merge(&options))
-+ die(_("apply options and merge options "
-+ "cannot be used together"));
else
- options.type = REBASE_APPLY;
- }
@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
options.update_refs = (options.update_refs >= 0) ? options.update_refs :
((options.config_update_refs >= 0) ? options.config_update_refs : 0);
--
2.40.0
^ permalink raw reply [flat|nested] 96+ messages in thread
* [PATCH v9 1/3] rebase: add documentation and test for --no-rebase-merges
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
@ 2023-03-26 3:06 ` Alex Henrie
2023-03-26 3:06 ` [PATCH v9 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
` (2 subsequent siblings)
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-26 3:06 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
As far as I can tell, --no-rebase-merges has always worked, but has
never been documented. It is especially important to document it before
a rebase.rebaseMerges option is introduced so that users know how to
override the config option on the command line. It's also important to
clarify that --rebase-merges without an argument is not the same as
--no-rebase-merges and not passing --rebase-merges is not the same as
passing --rebase-merges=no-rebase-cousins.
A test case is necessary to make sure that --no-rebase-merges keeps
working after its code is refactored in the following patches of this
series. The test case is a little contrived: It's unlikely that a user
would type both --rebase-merges and --no-rebase-merges at the same time.
However, if an alias is defined which includes --rebase-merges, the user
might decide to add --no-rebase-merges to countermand that part of the
alias but leave alone other flags set by the alias.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/git-rebase.txt | 18 +++++++++++-------
t/t3430-rebase-merges.sh | 10 ++++++++++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9a295bcee4..4e57a87624 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -529,20 +529,24 @@ See also INCOMPATIBLE OPTIONS below.
-r::
--rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
+--no-rebase-merges::
By default, a rebase will simply drop merge commits from the todo
list, and put the rebased commits into a single, linear branch.
With `--rebase-merges`, the rebase will instead try to preserve
the branching structure within the commits that are to be rebased,
by recreating the merge commits. Any resolved merge conflicts or
manual amendments in these merge commits will have to be
- resolved/re-applied manually.
+ resolved/re-applied manually. `--no-rebase-merges` can be used to
+ countermand 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,
-i.e. commits that would be excluded by linkgit:git-log[1]'s
-`--ancestry-path` option will keep their original ancestry by default. If
-the `rebase-cousins` mode is turned on, such commits are instead rebased
-onto `<upstream>` (or `<onto>`, if specified).
+When rebasing merges, there are two modes: `rebase-cousins` and
+`no-rebase-cousins`. If the mode is not specified, it defaults to
+`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
+`<upstream>` as direct ancestor will keep their original branch point, i.e.
+commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
+option will keep their original ancestry by default. In `rebase-cousins` mode,
+such commits are instead rebased onto `<upstream>` (or `<onto>`, if
+specified).
+
It is currently only possible to recreate the merge commits using the
`ort` merge strategy; different merge strategies can be used only via
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index fa2a06c19f..d46d9545f2 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -250,6 +250,16 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
EOF
'
+test_expect_success '--no-rebase-merges countermands --rebase-merges' '
+ git checkout -b no-rebase-merges E &&
+ git rebase --rebase-merges --no-rebase-merges C &&
+ test_cmp_graph C.. <<-\EOF
+ * B
+ * D
+ o C
+ EOF
+'
+
test_expect_success 'do not rebase cousins unless asked for' '
git checkout -b cousins main &&
before="$(git rev-parse --verify HEAD)" &&
--
2.40.0
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v9 2/3] rebase: deprecate --rebase-merges=""
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-26 3:06 ` [PATCH v9 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
@ 2023-03-26 3:06 ` Alex Henrie
2023-03-26 3:06 ` [PATCH v9 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-26 15:12 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Phillip Wood
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-26 3:06 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
The unusual syntax --rebase-merges="" (that is, --rebase-merges with an
empty string argument) has been an undocumented synonym of
--rebase-merges without an argument. Deprecate that syntax to avoid
confusion when a rebase.rebaseMerges config option is introduced, where
rebase.rebaseMerges="" will be equivalent to --no-rebase-merges.
It is not likely that anyone is actually using this syntax, but just in
case, deprecate the empty string argument instead of dropping support
for it immediately.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
builtin/rebase.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 5b7b908b66..9448f7d87f 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1141,7 +1141,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)""},
+ PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1439,7 +1439,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (rebase_merges) {
if (!*rebase_merges)
- ; /* default mode; do nothing */
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
else if (!strcmp("rebase-cousins", rebase_merges))
options.rebase_cousins = 1;
else if (strcmp("no-rebase-cousins", rebase_merges))
--
2.40.0
^ permalink raw reply related [flat|nested] 96+ messages in thread
* [PATCH v9 3/3] rebase: add a config option for --rebase-merges
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-26 3:06 ` [PATCH v9 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-26 3:06 ` [PATCH v9 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
@ 2023-03-26 3:06 ` Alex Henrie
2023-03-26 15:12 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Phillip Wood
3 siblings, 0 replies; 96+ messages in thread
From: Alex Henrie @ 2023-03-26 3:06 UTC (permalink / raw)
To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin,
sorganov, chooglen, calvinwan, jonathantanmy, felipe.contreras
Cc: Alex Henrie
The purpose of the new option is to accommodate users who would like
--rebase-merges to be on by default and to facilitate turning on
--rebase-merges by default without configuration in a future version of
Git.
Name the new option rebase.rebaseMerges, even though it is a little
redundant, for consistency with the name of the command line option and
to be clear when scrolling through values in the [rebase] section of
.gitconfig.
Support setting rebase.rebaseMerges to the nonspecific value "true" for
users who don't need to or don't want to learn about the difference
between rebase-cousins and no-rebase-cousins.
Make --rebase-merges without an argument on the command line override
any value of rebase.rebaseMerges in the configuration, for consistency
with other command line flags with optional arguments that have an
associated config option.
Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
Documentation/config/rebase.txt | 10 ++++
Documentation/git-rebase.txt | 3 +-
builtin/rebase.c | 74 +++++++++++++++++++-------
t/t3422-rebase-incompatible-options.sh | 17 ++++++
t/t3430-rebase-merges.sh | 34 ++++++++++++
5 files changed, 118 insertions(+), 20 deletions(-)
diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f19bd0e040..afaf6dad99 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.rebaseMerges::
+ Whether and how to set the `--rebase-merges` option by default. Can
+ be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
+ true or to `no-rebase-cousins` is equivalent to
+ `--rebase-merges=no-rebase-cousins`, setting to `rebase-cousins` is
+ equivalent to `--rebase-merges=rebase-cousins`, and setting to false is
+ equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
+ command line, with or without an argument, overrides any
+ `rebase.rebaseMerges` configuration.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 4e57a87624..e7b39ad244 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.rebaseMerges` config option and a previous
+ `--rebase-merges`.
+
When rebasing merges, there are two modes: `rebase-cousins` and
`no-rebase-cousins`. If the mode is not specified, it defaults to
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 9448f7d87f..beebeb8f52 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -124,6 +124,7 @@ struct rebase_options {
int fork_point;
int update_refs;
int config_autosquash;
+ int config_rebase_merges;
int config_update_refs;
};
@@ -141,6 +142,8 @@ struct rebase_options {
.allow_empty_message = 1, \
.autosquash = -1, \
.config_autosquash = -1, \
+ .rebase_merges = -1, \
+ .config_rebase_merges = -1, \
.update_refs = -1, \
.config_update_refs = -1, \
}
@@ -772,6 +775,16 @@ static int run_specific_rebase(struct rebase_options *opts)
return status ? -1 : 0;
}
+static void parse_rebase_merges_value(struct rebase_options *options, const char *value)
+{
+ if (!strcmp("no-rebase-cousins", value))
+ options->rebase_cousins = 0;
+ else if (!strcmp("rebase-cousins", value))
+ options->rebase_cousins = 1;
+ else
+ die(_("Unknown rebase-merges mode: %s"), value);
+}
+
static int rebase_config(const char *var, const char *value, void *data)
{
struct rebase_options *opts = data;
@@ -801,6 +814,17 @@ static int rebase_config(const char *var, const char *value, void *data)
return 0;
}
+ if (!strcmp(var, "rebase.rebasemerges")) {
+ opts->config_rebase_merges = git_parse_maybe_bool(value);
+ if (opts->config_rebase_merges < 0) {
+ opts->config_rebase_merges = 1;
+ parse_rebase_merges_value(opts, value);
+ } else {
+ opts->rebase_cousins = 0;
+ }
+ return 0;
+ }
+
if (!strcmp(var, "rebase.updaterefs")) {
opts->config_update_refs = git_config_bool(var, value);
return 0;
@@ -981,6 +1005,28 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
return 0;
}
+static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
+{
+ struct rebase_options *options = opt->value;
+
+ options->rebase_merges = !unset;
+ options->rebase_cousins = 0;
+
+ if (arg) {
+ if (!*arg) {
+ warning(_("--rebase-merges with an empty string "
+ "argument is deprecated and will stop "
+ "working in a future version of Git. Use "
+ "--rebase-merges without an argument "
+ "instead, which does the same thing."));
+ return 0;
+ }
+ parse_rebase_merges_value(options, arg);
+ }
+
+ return 0;
+}
+
static void NORETURN error_on_missing_default_upstream(void)
{
struct branch *current_branch = branch_get(NULL);
@@ -1036,7 +1082,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
struct object_id branch_base;
int ignore_whitespace = 0;
const char *gpg_sign = NULL;
- const char *rebase_merges = NULL;
struct string_list strategy_options = STRING_LIST_INIT_NODUP;
struct object_id squash_onto;
char *squash_onto_name = NULL;
@@ -1138,10 +1183,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
&options.allow_empty_message,
N_("allow rebasing commits with empty messages"),
PARSE_OPT_HIDDEN),
- {OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
- N_("mode"),
+ OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
N_("try to rebase merges instead of skipping them"),
- PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
+ PARSE_OPT_OPTARG, parse_opt_rebase_merges),
OPT_BOOL(0, "fork-point", &options.fork_point,
N_("use 'merge-base --fork-point' to refine upstream")),
OPT_STRING('s', "strategy", &options.strategy,
@@ -1437,21 +1481,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (options.exec.nr)
imply_merge(&options, "--exec");
- if (rebase_merges) {
- if (!*rebase_merges)
- warning(_("--rebase-merges with an empty string "
- "argument is deprecated and will stop "
- "working in a future version of Git. Use "
- "--rebase-merges without an argument "
- "instead, which does the same thing."));
- else 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;
- imply_merge(&options, "--rebase-merges");
- }
-
if (options.type == REBASE_APPLY) {
if (ignore_whitespace)
strvec_push(&options.git_am_opts,
@@ -1519,6 +1548,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
"cannot be used together"));
else if (options.autosquash == -1 && options.config_autosquash == 1)
die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
+ else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
+ die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
else if (options.update_refs == -1 && options.config_update_refs == 1)
die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
else
@@ -1531,6 +1562,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
options.update_refs = (options.update_refs >= 0) ? options.update_refs :
((options.config_update_refs >= 0) ? options.config_update_refs : 0);
+ if (options.rebase_merges == 1)
+ imply_merge(&options, "--rebase-merges");
+ options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
+ ((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
+
if (options.autosquash == 1)
imply_merge(&options, "--autosquash");
options.autosquash = (options.autosquash >= 0) ? options.autosquash :
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 4711b37a28..2eba00bdf5 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -85,6 +85,11 @@ test_rebase_am_only () {
test_must_fail git rebase $opt --reapply-cherry-picks A
"
+ test_expect_success "$opt incompatible with --rebase-merges" "
+ git checkout B^0 &&
+ test_must_fail git rebase $opt --rebase-merges A
+ "
+
test_expect_success "$opt incompatible with --update-refs" "
git checkout B^0 &&
test_must_fail git rebase $opt --update-refs A
@@ -101,6 +106,12 @@ test_rebase_am_only () {
grep -e --no-autosquash err
"
+ test_expect_success "$opt incompatible with rebase.rebaseMerges" "
+ git checkout B^0 &&
+ test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
+ grep -e --no-rebase-merges err
+ "
+
test_expect_success "$opt incompatible with rebase.updateRefs" "
git checkout B^0 &&
test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
@@ -113,6 +124,12 @@ test_rebase_am_only () {
git -c rebase.autosquash=true rebase --no-autosquash $opt A
"
+ test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
+ test_when_finished \"git reset --hard B^0\" &&
+ git checkout B^0 &&
+ git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
+ "
+
test_expect_success "$opt okay with overridden rebase.updateRefs" "
test_when_finished \"git reset --hard B^0\" &&
git checkout B^0 &&
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index d46d9545f2..f03599c63b 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -278,6 +278,40 @@ test_expect_success 'do not rebase cousins unless asked for' '
EOF
'
+test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
+ test_config rebase.rebaseMerges 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.rebaseMerges=no-rebase-cousins' '
+ test_config rebase.rebaseMerges 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 overrides rebase.rebaseMerges=rebase-cousins' '
+ test_config rebase.rebaseMerges rebase-cousins &&
+ git checkout -b override-config-rebase-cousins E &&
+ before="$(git rev-parse --verify HEAD)" &&
+ test_tick &&
+ git rebase --rebase-merges C &&
+ test_cmp_rev HEAD $before
+'
+
test_expect_success 'refs/rewritten/* is worktree-local' '
git worktree add wt &&
cat >wt/script-from-scratch <<-\EOF &&
--
2.40.0
^ permalink raw reply related [flat|nested] 96+ messages in thread
* Re: [PATCH v9 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
` (2 preceding siblings ...)
2023-03-26 3:06 ` [PATCH v9 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-03-26 15:12 ` Phillip Wood
2023-03-27 16:33 ` Junio C Hamano
3 siblings, 1 reply; 96+ messages in thread
From: Phillip Wood @ 2023-03-26 15:12 UTC (permalink / raw)
To: Alex Henrie, git, tao, gitster, newren, phillip.wood123,
Johannes.Schindelin, sorganov, chooglen, calvinwan,
jonathantanmy, felipe.contreras
Hi Alex
On 26/03/2023 04:06, Alex Henrie wrote:
> This patch series introduces a rebase.rebaseMerges config option to
> accommodate users who would like --rebase-merges to be on by default and
> to facilitate turning on --rebase-merges by default without
> configuration in a future version of Git. It also cleans up and
> documents the behavior of the --rebase-merges command line option to
> avoid confusion about how the config option and the command line option
> interact.
>
> Changes from v8:
> - Add braces around one-line else clause
> - Remove unnecessary change to error message priority
The range-diff looks good to me. This iteration addresses all of my
outstanding concerns.
Thanks
Phillip
> Thanks to Phillip, Junio, Johannes and Sergey for your feedback on v8.
>
> Alex Henrie (3):
> rebase: add documentation and test for --no-rebase-merges
> rebase: deprecate --rebase-merges=""
> rebase: add a config option for --rebase-merges
>
> Documentation/config/rebase.txt | 10 ++++
> Documentation/git-rebase.txt | 19 ++++---
> builtin/rebase.c | 70 ++++++++++++++++++++------
> t/t3422-rebase-incompatible-options.sh | 17 +++++++
> t/t3430-rebase-merges.sh | 44 ++++++++++++++++
> 5 files changed, 138 insertions(+), 22 deletions(-)
>
> Range-diff against v8:
> 1: 09fb7c1b74 = 1: a22b9d0da2 rebase: add documentation and test for --no-rebase-merges
> 2: a846716a4a = 2: 112fee4833 rebase: deprecate --rebase-merges=""
> 3: b12a3610ba ! 3: 868899cd6d rebase: add a config option for --rebase-merges
> @@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
> + if (opts->config_rebase_merges < 0) {
> + opts->config_rebase_merges = 1;
> + parse_rebase_merges_value(opts, value);
> -+ } else
> ++ } else {
> + opts->rebase_cousins = 0;
> ++ }
> + return 0;
> + }
> +
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
> if (ignore_whitespace)
> strvec_push(&options.git_am_opts,
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
> - break;
> -
> - if (i >= 0 || options.type == REBASE_APPLY) {
> -- if (is_merge(&options))
> -- die(_("apply options and merge options "
> -- "cannot be used together"));
> -- else if (options.autosquash == -1 && options.config_autosquash == 1)
> -+ if (options.autosquash == -1 && options.config_autosquash == 1)
> + "cannot be used together"));
> + else if (options.autosquash == -1 && options.config_autosquash == 1)
> die(_("apply options are incompatible with rebase.autoSquash. Consider adding --no-autosquash"));
> + else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> + die(_("apply options are incompatible with rebase.rebaseMerges. Consider adding --no-rebase-merges"));
> else if (options.update_refs == -1 && options.config_update_refs == 1)
> die(_("apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs"));
> -+ else if (is_merge(&options))
> -+ die(_("apply options and merge options "
> -+ "cannot be used together"));
> else
> - options.type = REBASE_APPLY;
> - }
> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
> options.update_refs = (options.update_refs >= 0) ? options.update_refs :
> ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
^ permalink raw reply [flat|nested] 96+ messages in thread
* Re: [PATCH v9 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges
2023-03-26 15:12 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Phillip Wood
@ 2023-03-27 16:33 ` Junio C Hamano
0 siblings, 0 replies; 96+ messages in thread
From: Junio C Hamano @ 2023-03-27 16:33 UTC (permalink / raw)
To: Phillip Wood
Cc: Alex Henrie, git, tao, newren, Johannes.Schindelin, sorganov,
chooglen, calvinwan, jonathantanmy, felipe.contreras
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Alex
>
> On 26/03/2023 04:06, Alex Henrie wrote:
>> This patch series introduces a rebase.rebaseMerges config option to
>> accommodate users who would like --rebase-merges to be on by default and
>> to facilitate turning on --rebase-merges by default without
>> configuration in a future version of Git. It also cleans up and
>> documents the behavior of the --rebase-merges command line option to
>> avoid confusion about how the config option and the command line option
>> interact.
>> Changes from v8:
>> - Add braces around one-line else clause
>> - Remove unnecessary change to error message priority
>
> The range-diff looks good to me. This iteration addresses all of my
> outstanding concerns.
Thanks, both. Let's mark the topic as ready for 'next', then.
^ permalink raw reply [flat|nested] 96+ messages in thread