Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jan 23, 2023, 20:08 UTC
- Message-ID
- <759fb313-ce88-4eb7-96c0-4adeb75ca9f9@dunelm.org.uk>
- In-Reply-To
- <5e4851e611ee18112bd71939ee900e02a8d590c5.1674367961.git.gitgitgadget@gmail.com>
Hi Elijah
On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:
Show 11 quoted lines
> From: Elijah Newren <newren@gmail.com> > > The git-rebase manual noted several sets of incompatible options, but > we were missing tests for a few of these. Further, we were missing > code checks for some of these, which could result in command line > options being silently ignored. > > Also, note that adding a check for autosquash means that using > --whitespace=fix together with the config setting rebase.autosquash=true > will trigger an error. A subsequent commit will improve the error > message.
Thanks for updating the commit message and for the new commits at the end of the series.
Show 23 quoted lines
> Signed-off-by: Elijah Newren <newren@gmail.com> > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.fork_point < 0) > options.fork_point = 0; > } > + /* > + * The apply backend does not support --[no-]reapply-cherry-picks. > + * The behavior it implements by default is equivalent to > + * --no-reapply-cherry-picks (due to passing --cherry-picks to > + * format-patch), but --keep-base alters the upstream such that no > + * cherry-picks can be found (effectively making it act like > + * --reapply-cherry-picks). > + * > + * Now, if the user does specify --[no-]reapply-cherry-picks, but > + * does so in such a way that options.reapply_cherry_picks == > + * keep_base, then the behavior they get will match what they > + * expect despite options.reapply_cherry_picks being ignored. We > + * could just allow the flag in that case, but it seems better to > + * just alert the user that they've specified a flag that the > + * backend ignores. > + */
I'm a bit confused by this. --keep-base works with either --reapply-cherry-picks (which is the default if --keep-base is given) or --no-reapply-cherry-picks. Just below this hunk we have
if (options.reapply_cherry_picks < 0) options.reapply_cherry_picks = keep_base;
So we only set options.reapply_cherry_picks to match keep_base if the user did not specify -[-no]-reapply-cherry-picks on the commandline.
Best Wishes
Phillip
Show 74 quoted lines
> + if (options.reapply_cherry_picks >= 0)
> + imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" :
> + "--no-reapply-cherry-picks");
> +
> /*
> * --keep-base defaults to --reapply-cherry-picks to avoid losing
> * commits when using this option.
> @@ -1406,13 +1426,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.empty != EMPTY_UNSPECIFIED)
> imply_merge(&options, "--empty");
>
> - /*
> - * --keep-base implements --reapply-cherry-picks by altering upstream so
> - * it works with both backends.
> - */
> - if (options.reapply_cherry_picks && !keep_base)
> - imply_merge(&options, "--reapply-cherry-picks");
> -
> if (gpg_sign)
> options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
>
> @@ -1503,6 +1516,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> if (options.update_refs)
> imply_merge(&options, "--update-refs");
>
> + if (options.autosquash)
> + imply_merge(&options, "--autosquash");
> +
> if (options.type == REBASE_UNSPECIFIED) {
> if (!strcmp(options.default_backend, "merge"))
> imply_merge(&options, "--merge");
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index f86274990b0..6a17b571ec7 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -50,6 +50,11 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --strategy-option=ours A
> "
>
> + test_expect_success "$opt incompatible with --autosquash" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --autosquash A
> + "
> +
> test_expect_success "$opt incompatible with --interactive" "
> git checkout B^0 &&
> test_must_fail git rebase $opt --interactive A
> @@ -60,6 +65,26 @@ test_rebase_am_only () {
> test_must_fail git rebase $opt --exec 'true' A
> "
>
> + test_expect_success "$opt incompatible with --keep-empty" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --keep-empty A
> + "
> +
> + test_expect_success "$opt incompatible with --empty=..." "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --empty=ask A
> + "
> +
> + test_expect_success "$opt incompatible with --no-reapply-cherry-picks" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --no-reapply-cherry-picks A
> + "
> +
> + test_expect_success "$opt incompatible with --reapply-cherry-picks" "
> + git checkout B^0 &&
> + test_must_fail git rebase $opt --reapply-cherry-picks A
> + "
> +
> test_expect_success "$opt incompatible with --update-refs" "
> git checkout B^0 &&
> test_must_fail git rebase $opt --update-refs A