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@xxxxxxxxx>
---
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
