We had a report about --update-refs being ignored when --whitespace=fix was passed, confusing an end user. These were already marked as incompatible in the manual, but the code didn't check for the incompatibility and provide an error to the user. Folks brought up other flags tangentially when reviewing an earlier round of this series, and I found we had more that were missing checks, and that we were missing some testcases, and that the documentation was wrong on some of the relevant options. So, I ended up getting lots of little fixes to straighten these all out. Left out of this series: * If an option like rebase.autosquash or rebase.updateRefs are selected and the user specifies e.g. --whitespace=fix, we should either (a) tailor the error message better to point out that it's a config option that is incompatible with their command line flag, or (b) implement --whitespace=fix for the merge backend so we can just deprecate and eventually remove the apply backend[1]. [1] See "longer term goal" at https://lore.kernel.org/git/xmqqa78d2qmk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/ Changes since v2: * Remove the extra patch and reword the comment in t3422 more thoroughly. * Add tests for other flag incompatibilities that were missing * Add code that was missing for checking various flag incompatibilities * Fix incorrect claims in the documentation around incompatible options * A few other miscellaneous fixups noticed while doing all the above Changes since v1: * Add a patch nuking the -C option to rebase (fixes confusion around the comment in t3422 and acknowledges the fact that the option is totally and utterly useless and always has been. It literally never affects the results of a rebase.) Signed-off-by: Elijah Newren newren@xxxxxxxxx Elijah Newren (7): rebase: mark --update-refs as requiring the merge backend rebase: flag --apply and --merge as incompatible rebase: remove --allow-empty-message from incompatible opts rebase: fix docs about incompatibilities with --root rebase: add coverage of other incompatible options rebase: clarify the OPT_CMDMODE incompatibilities rebase: fix formatting of rebase --reapply-cherry-picks option in docs Documentation/git-rebase.txt | 73 +++++++++++++------------- builtin/rebase.c | 36 +++++++++---- t/t3422-rebase-incompatible-options.sh | 42 +++++++++++++-- 3 files changed, 101 insertions(+), 50 deletions(-) base-commit: 221222b278e713054e65cbbbcb2b1ac85483ea89 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1466 Range-diff vs v2: 2: 2e44d0b7e57 ! 1: 9089834adea rebase: mark --update-refs as requiring the merge backend @@ Commit message --update-refs is built in terms of the sequencer, which requires the merge backend. It was already marked as incompatible with the apply backend in the git-rebase manual, but the code didn't check for this - incompatibility and warn the user. Check and warn now. + incompatibility and warn the user. Check and error now. - While at it, fix a typo in t3422...and fix some misleading wording (all - useful options other than --whitespace=fix have long since been - implemented in the merge backend). + While at it, fix a typo in t3422...and fix some misleading wording + (most options which used to be am-specific have since been implemented + in the merge backend as well). Signed-off-by: Elijah Newren <newren@xxxxxxxxx> + ## Documentation/git-rebase.txt ## +@@ Documentation/git-rebase.txt: start would be overridden by the presence of + + + If the configuration variable `rebase.updateRefs` is set, then this option + can be used to override and disable this setting. +++ ++See also INCOMPATIBLE OPTIONS below. + + INCOMPATIBLE OPTIONS + -------------------- + ## builtin/rebase.c ## @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) } @@ t/t3422-rebase-incompatible-options.sh: test_expect_success 'setup' ' -# --merge nor --interactive (nor any options that imply those two) use -# git-am, using them together will result in flags like --whitespace=fix -# being ignored. Make sure rebase warns the user and aborts instead. -+# Rebase has a useful option, --whitespace=fix, which is actually -+# built in terms of flags to git-am. Since neither --merge nor -+# --interactive (nor any options that imply those two) use git-am, -+# using them together will result in --whitespace=fix being ignored. -+# Make sure rebase warns the user and aborts instead. ++# Rebase has a couple options which are specific to the apply backend, ++# and several options which are specific to the merge backend. Flags ++# from the different sets cannot work together, and we do not want to ++# just ignore one of the sets of flags. Make sure rebase warns the ++# user and aborts instead. # test_rebase_am_only () { 1: a0f8f5fac1c ! 2: a8b5a0e4fb0 rebase: remove completely useless -C option @@ Metadata Author: Elijah Newren <newren@xxxxxxxxx> ## Commit message ## - rebase: remove completely useless -C option + rebase: flag --apply and --merge as incompatible - The `-C` option to rebase was introduced with 67dad687ad ("add -C[NUM] - to git-am", 2007-02-08). Based on feedback on the patch, the author - added the -C option not just to git-am but also to git-rebase. But did - it such that the option was just passed through to git-am (which passes - it along to git-apply), with no corresponding option to format-patch. - - As per the git-apply documentation for the `-C` option: - Ensure at least <n> lines of surrounding context match...When fewer - lines of surrounding context exist they all must match. - - The fact that format-patch was not passed a -U option to increase the - number of context lines meant that there would still only be 3 lines of - context to match on. So, anyone attempting to use this option in - git-rebase would just be spinning their wheels and wasting time. I was - one such user a number of years ago. - - Since this option can at best waste users time and is nothing more than - a confusing no-op, and has never been anything other than a confusing - no-op, and no one has ever bothered to create a testcase for it that - goes beyond option parsing, simply excise the option. + Previously, we flagged options which implied --apply as being + incompatible with options which implied --merge. But if both options + were given explicitly, then we didn't flag the incompatibility. The + same is true with --apply and --interactive. Add the check, and add + some testcases to verify these are also caught. Signed-off-by: Elijah Newren <newren@xxxxxxxxx> - ## Documentation/git-rebase.txt ## -@@ Documentation/git-rebase.txt: include::rerere-options.txt[] - Allows the pre-rebase hook to run, which is the default. This option can - be used to override `--no-verify`. See also linkgit:githooks[5]. + ## builtin/rebase.c ## +@@ builtin/rebase.c: static int parse_opt_am(const struct option *opt, const char *arg, int unset) + BUG_ON_OPT_NEG(unset); + BUG_ON_OPT_ARG(arg); ---C<n>:: -- Ensure at least `<n>` lines of surrounding context match before -- and after each change. When fewer lines of surrounding -- context exist they all must match. By default no context is -- ever ignored. Implies `--apply`. --+ --See also INCOMPATIBLE OPTIONS below. -- - --no-ff:: - --force-rebase:: - -f:: -@@ Documentation/git-rebase.txt: The following options: ++ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY) ++ die(_("apply options and merge options cannot be used together")); ++ + opts->type = REBASE_APPLY; - * --apply - * --whitespace -- * -C + return 0; +@@ builtin/rebase.c: static int parse_opt_merge(const struct option *opt, const char *arg, int unset) + BUG_ON_OPT_NEG(unset); + BUG_ON_OPT_ARG(arg); - are incompatible with the following options: +- if (!is_merge(opts)) +- opts->type = REBASE_MERGE; ++ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE) ++ die(_("apply options and merge options cannot be used together")); ++ ++ opts->type = REBASE_MERGE; - - ## builtin/rebase.c ## -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) - N_("ignore author date and use current date")), - OPT_HIDDEN_BOOL(0, "ignore-date", &options.ignore_date, - N_("synonym of --reset-author-date")), -- OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"), -- N_("passed to 'git apply'"), 0), - OPT_BOOL(0, "ignore-whitespace", &ignore_whitespace, - N_("ignore changes in whitespace")), - OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts, -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) - if (!strcmp(option, "--whitespace=fix") || - !strcmp(option, "--whitespace=strip")) - allow_preemptive_ff = 0; -- else if (skip_prefix(option, "-C", &p)) { -- while (*p) -- if (!isdigit(*(p++))) -- die(_("switch `C' expects a " -- "numerical value")); -- } else if (skip_prefix(option, "--whitespace=", &p)) { -+ else if (skip_prefix(option, "--whitespace=", &p)) { - if (*p && strcmp(p, "warn") && strcmp(p, "nowarn") && - strcmp(p, "error") && strcmp(p, "error-all")) - die("Invalid whitespace option: '%s'", p); - - ## t/t3406-rebase-message.sh ## -@@ t/t3406-rebase-message.sh: test_expect_success 'rebase --onto outputs the invalid ref' ' - test_i18ngrep "invalid-ref" err - ' + return 0; + } +@@ builtin/rebase.c: static int parse_opt_interactive(const struct option *opt, const char *arg, + BUG_ON_OPT_NEG(unset); + BUG_ON_OPT_ARG(arg); + ++ if (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE) ++ die(_("apply options and merge options cannot be used together")); ++ + opts->type = REBASE_MERGE; + opts->flags |= REBASE_INTERACTIVE_EXPLICIT; --test_expect_success 'error out early upon -C<n> or --whitespace=<bad>' ' -- test_must_fail git rebase -Cnot-a-number HEAD 2>err && -- test_i18ngrep "numerical value" err && -- test_must_fail git rebase --whitespace=bad HEAD 2>err && -- test_i18ngrep "Invalid whitespace option" err --' -- - write_reflog_expect () { - if test $mode = --apply - then ## t/t3422-rebase-incompatible-options.sh ## @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () { + } ++# Check options which imply --apply test_rebase_am_only --whitespace=fix --test_rebase_am_only -C4 + test_rebase_am_only -C4 ++# Also check an explicit --apply ++test_rebase_am_only --apply test_done -: ----------- > 3: f4fbfd40d45 rebase: remove --allow-empty-message from incompatible opts -: ----------- > 4: a1e61ac8f21 rebase: fix docs about incompatibilities with --root -: ----------- > 5: 48c40c0dda0 rebase: add coverage of other incompatible options -: ----------- > 6: 8664cce6cf7 rebase: clarify the OPT_CMDMODE incompatibilities -: ----------- > 7: 0e8b06163f2 rebase: fix formatting of rebase --reapply-cherry-picks option in docs -- gitgitgadget
