| Message ID | 48c40c0dda00eeb8b9bdc5ba9372b46964eea14a.1674266126.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase: fix several code/testing/documentation issues around flag incompatibilities | expand |
Hi Elijah On 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote: > 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. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/rebase.c | 21 ++++++++++++++------- > t/t3422-rebase-incompatible-options.sh | 20 ++++++++++++++++++++ > 2 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 2a5e0e8a7a0..6dcdb59bb02 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.fork_point < 0) > options.fork_point = 0; > } > + /* > + * reapply_cherry_picks is slightly weird. It starts out with a > + * value of -1. It will be assigned a value of keep_base below and > + * then handled specially. The apply backend is hardcoded to > + * behave like reapply_cherry_picks==1, I think it is hard coded to 0 not 1. We generate the patches with git format-patch --right-only $upstream...$head so cherry-picks will not be reapplied. I'm hardly an impartial observer but I think the current check is correct. We support --whitespace=fix --no-reapply-cherry-picks --whitespace=fix --keep-base --reapply-cherry-picks but not --whitespace=fix --reapply-cherry-picks > @@ -1525,6 +1529,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"); Thanks for adding this, it maybe merits a mention in the commit message though as it is a change in behavior for users who have rebase.autosquash set and try to use the apply backend. Best Wishes Phillip > 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..c830025470f 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,21 @@ 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 --update-refs" " > git checkout B^0 && > test_must_fail git rebase $opt --update-refs A
On 21/01/2023 15:20, Phillip Wood wrote: >> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, >> const char *prefix) >> if (options.fork_point < 0) >> options.fork_point = 0; >> } >> + /* >> + * reapply_cherry_picks is slightly weird. It starts out with a >> + * value of -1. It will be assigned a value of keep_base below and >> + * then handled specially. The apply backend is hardcoded to >> + * behave like reapply_cherry_picks==1, > > I think it is hard coded to 0 not 1. We generate the patches with > > git format-patch --right-only $upstream...$head Sorry I somhow managed to omit --cherry-pick from that, it should have been git format-patch --right-only --cherry-pick $upstream...$head Phillip > so cherry-picks will not be reapplied. I'm hardly an impartial observer > but I think the current check is correct. We support > > --whitespace=fix --no-reapply-cherry-picks > --whitespace=fix --keep-base --reapply-cherry-picks > > but not > > --whitespace=fix --reapply-cherry-picks > >> @@ -1525,6 +1529,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"); > > Thanks for adding this, it maybe merits a mention in the commit message > though as it is a change in behavior for users who have > rebase.autosquash set and try to use the apply backend. > > Best Wishes > > Phillip > >> 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..c830025470f 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,21 @@ 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 --update-refs" " >> git checkout B^0 && >> test_must_fail git rebase $opt --update-refs A
On Sat, Jan 21, 2023 at 11:25 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 21/01/2023 15:20, Phillip Wood wrote: > >> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, > >> const char *prefix) > >> if (options.fork_point < 0) > >> options.fork_point = 0; > >> } > >> + /* > >> + * reapply_cherry_picks is slightly weird. It starts out with a > >> + * value of -1. It will be assigned a value of keep_base below and > >> + * then handled specially. The apply backend is hardcoded to > >> + * behave like reapply_cherry_picks==1, > > > > I think it is hard coded to 0 not 1. We generate the patches with > > > > git format-patch --right-only $upstream...$head > > Sorry I somhow managed to omit --cherry-pick from that, it should have been > > git format-patch --right-only --cherry-pick $upstream...$head > > Phillip Oh, indeed, I was reading wrong. Thanks for the correction. I still think there's something to fix up here, which I'll include in my re-roll. > > so cherry-picks will not be reapplied. I'm hardly an impartial observer > > but I think the current check is correct. We support > > > > --whitespace=fix --no-reapply-cherry-picks > > --whitespace=fix --keep-base --reapply-cherry-picks > > > > but not > > > > --whitespace=fix --reapply-cherry-picks Right, nor do we support --whitespace=fix --keep-base --no-reapply-cherry-picks (If you try it, you'll notice that the code accepts those flags and starts the rebase pretending everything is fine, but it silently ignores the --no-reapply-cherry-picks flag.) Stepping back a bit, though, the apply backend doesn't support toggling --[no-]reapply-cherry-picks at all. It hard codes the behavior (in a way dependent upon --keep-base). So, if the user passes --[no-]reapply-cherry-picks and we don't error out, we are just going to ignore whatever they passed and do whatever hardcoded thing is baked into the algorithm. While the user can pass the flag in a way that happens to match what is baked into the apply backend, I'd still say it's wrong for them to specify it since we will just ignore it. Doing so is likely to result in the user later toggling the flag and being surprised that they get an error when it used to be accepted, or seeing that it only sometimes gets accepted. Anyway, I'll update this patch to document this a bit more clearly in a code comment and add an improved check.
diff --git a/builtin/rebase.c b/builtin/rebase.c index 2a5e0e8a7a0..6dcdb59bb02 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.fork_point < 0) options.fork_point = 0; } + /* + * reapply_cherry_picks is slightly weird. It starts out with a + * value of -1. It will be assigned a value of keep_base below and + * then handled specially. The apply backend is hardcoded to + * behave like reapply_cherry_picks==1, so if it has that value, we + * can just ignore the flag with the apply backend. Thus, we only + * really need to throw an error and require the merge backend if + * reapply_cherry_picks==0. + */ + if (options.reapply_cherry_picks == 0) + imply_merge(&options, "--no-reapply-cherry-picks"); /* * --keep-base defaults to --reapply-cherry-picks to avoid losing * commits when using this option. @@ -1420,13 +1431,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); @@ -1525,6 +1529,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..c830025470f 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,21 @@ 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 --update-refs" " git checkout B^0 && test_must_fail git rebase $opt --update-refs A