| Message ID | 20230225180325.796624-3-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: add a config option for --rebase-merges | expand |
Alex Henrie <alexhenrie24@gmail.com> writes: > 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. [...] > 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=no-rebase-cousins " > + "instead.")); > else if (!strcmp("rebase-cousins", rebase_merges)) > options.rebase_cousins = 1; > else if (strcmp("no-rebase-cousins", rebase_merges)) As I mentioned in my review of patch 3/3, I think we might be better served by saying that --rebase-merges="" is a synonym of --rebase-merges (aka give me a sane default) instead of giving a specific value like "no-rebase-cousins". This would be give us leeway to change the default behavior in the future.
Hi Alex On 25/02/2023 18:03, Alex Henrie wrote: > 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. I think deprecating this rather than making it an error is a good idea. I don't think we need the test though. The test suite is incredibly slow to run on windows (I'd heard people complain about it but it was not until I tried running it myself I realized just how diabolically slow it really is) and so it is important to balance adding tests to catch regression vs test run time. Tests that catch bugs in the rebase implementation are useful, ones like this just make everything slower which makes it harder to catch real regressions. Best Wishes Phillip > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> > --- > builtin/rebase.c | 8 ++++++-- > t/t3430-rebase-merges.sh | 5 +++++ > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 6635f10d52..ccc13200b5 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=no-rebase-cousins " > + "instead.")); > else if (!strcmp("rebase-cousins", rebase_merges)) > options.rebase_cousins = 1; > else if (strcmp("no-rebase-cousins", rebase_merges)) > diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh > index d46d9545f2..f50fbf1390 100755 > --- a/t/t3430-rebase-merges.sh > +++ b/t/t3430-rebase-merges.sh > @@ -278,6 +278,11 @@ 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 &&
> - ; /* 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=no-rebase-cousins " > + "instead.")); Just a small nit on the warning message. It should describe to the user what behavior is happening here and presumably the user wanted `--rebase-merges` with no argument. For example, "Defaulting to --rebase-merges instead."
diff --git a/builtin/rebase.c b/builtin/rebase.c index 6635f10d52..ccc13200b5 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=no-rebase-cousins " + "instead.")); else if (!strcmp("rebase-cousins", rebase_merges)) options.rebase_cousins = 1; else if (strcmp("no-rebase-cousins", rebase_merges)) diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index d46d9545f2..f50fbf1390 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -278,6 +278,11 @@ 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 &&
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. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- builtin/rebase.c | 8 ++++++-- t/t3430-rebase-merges.sh | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-)