| Message ID | 9cd4c372ee4b3e5ba45c66a43ad0edaf52f0eed9.1660576283.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point | expand |
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > As --keep-base does not rebase the branch it is confusing if it > removes commits that have been cherry-picked to the upstream branch. > As --reapply-cherry-picks is not supported by the "apply" backend this > commit ensures that cherry-picks are reapplied by forcing the upstream > commit to match the onto commit unless --no-reapply-cherry-picks is > given. > > Reported-by: Philippe Blain <levraiphilippeblain@gmail.com> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > Documentation/git-rebase.txt | 2 +- > builtin/rebase.c | 15 ++++++++++++++- > t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++ > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 080658c8710..dc0c6c54e27 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. > merge base of `<upstream>` and `<branch>`. Running > `git rebase --keep-base <upstream> <branch>` is equivalent to > running > - `git rebase --onto <upstream>...<branch> <upstream> <branch>`. > + `git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`. > + > This option is useful in the case where one is developing a feature on > top of an upstream branch. While the feature is being worked on, the > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 86ea731ca3a..b6b3e00e3b1 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1181,6 +1181,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > prepare_repo_settings(the_repository); > the_repository->settings.command_requires_full_index = 0; > > + options.reapply_cherry_picks = -1; > options.allow_empty_message = 1; > git_config(rebase_config, &options); > /* options.gpg_sign_opt will be either "-S" or NULL */ > @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.root) > die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root"); > } > + /* > + * --keep-base defaults to --reapply-cherry-picks as it is confusing if > + * commits disappear when using this option. > + */ > + if (options.reapply_cherry_picks < 0) > + options.reapply_cherry_picks = keep_base; It makes me wonder if an explicit "--no-reapply-cherry-picks" makes sense in combination with "--keep-base". If that happens, we do not take this "By default, reapply is enabled with keep-base". > @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.empty != EMPTY_UNSPECIFIED) > imply_merge(&options, "--empty"); > > - if (options.reapply_cherry_picks) > + /* > + * --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"); Interesting. The idea is that we shouldn't care how much progress (which may include cherry-picks) the upstream side made, and it is no use to compare the commits between the F (fork point) and O (our tip) against the commits between updated U (upstream) and F (fork point) to notice that X' is a cherry-pick from our X. o---X---o---O (our work) / ----F----o----o----o----X'----U (upstream) So almost ignoring U (except for obviously figure out F, possibly, for the purpose of keep-base) is an effective way to keep X on our history, and when it happens, we do not have to explicitly pass the "--reapply" option to underlying rebase machinery. Makes sense. If an explicit "--no-reapply-cherry-picks" with "--keep-base" is given, we still skip this and do not call imply_merge() ... > @@ -1680,6 +1691,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > } > if (keep_base) { > oidcpy(&merge_base, &options.onto->object.oid); > + if (options.reapply_cherry_picks) > + options.upstream = options.onto; ... but this is also skipped in such a case. I do not offhand know if the combination makes practical sense, but this should allow the combination to "work". OK. Thanks.
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.root) > die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root"); > } > + /* > + * --keep-base defaults to --reapply-cherry-picks as it is confusing if > + * commits disappear when using this option. > + */ > + if (options.reapply_cherry_picks < 0) > + options.reapply_cherry_picks = keep_base; Here, we set options.reapply_cherry_picks to 1 if keep_base is 1, but... > @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.empty != EMPTY_UNSPECIFIED) > imply_merge(&options, "--empty"); > > - if (options.reapply_cherry_picks) > + /* > + * --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 we implement --reapply-cherry-picks by altering upstream (and hence not need to rely on the "merge" backend), shouldn't we suppress the setting of options.reapply_cherry_picks too?
Hi Phillip, Le 2022-08-15 à 11:11, Phillip Wood via GitGitGadget a écrit : > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > As --keep-base does not rebase the branch it is confusing if it > removes commits that have been cherry-picked to the upstream branch. > As --reapply-cherry-picks is not supported by the "apply" backend this > commit ensures that cherry-picks are reapplied by forcing the upstream > commit to match the onto commit unless --no-reapply-cherry-picks is > given. > > Reported-by: Philippe Blain <levraiphilippeblain@gmail.com> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Thanks a lot for picking this up ! I still think that it's a good idea to do that. At least now since 767a4ca648 (sequencer: advise if skipping cherry-picked commit, 2021-08-30) we get a warning, but I think changing the default for '--keep-base' is even better. > --- > Documentation/git-rebase.txt | 2 +- > builtin/rebase.c | 15 ++++++++++++++- > t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++ > 3 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 080658c8710..dc0c6c54e27 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. > merge base of `<upstream>` and `<branch>`. Running > `git rebase --keep-base <upstream> <branch>` is equivalent to > running > - `git rebase --onto <upstream>...<branch> <upstream> <branch>`. > + `git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`. > + > This option is useful in the case where one is developing a feature on > top of an upstream branch. While the feature is being worked on, the I think the doc should explicitely mention "This implies `--reapply-cherry-picks`", in addition to your changes. Cheers, Philippe.
Hi Jonathan On 24/08/2022 23:09, Jonathan Tan wrote: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: >> @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> if (options.root) >> die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root"); >> } >> + /* >> + * --keep-base defaults to --reapply-cherry-picks as it is confusing if >> + * commits disappear when using this option. >> + */ >> + if (options.reapply_cherry_picks < 0) >> + options.reapply_cherry_picks = keep_base; > > Here, we set options.reapply_cherry_picks to 1 if keep_base is 1, but... > >> @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) >> if (options.empty != EMPTY_UNSPECIFIED) >> imply_merge(&options, "--empty"); >> >> - if (options.reapply_cherry_picks) >> + /* >> + * --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 we implement --reapply-cherry-picks by altering upstream (and > hence not need to rely on the "merge" backend), shouldn't we suppress > the setting of options.reapply_cherry_picks too? I'm not quite sure what you're suggesting. We only alter upstream if options.reapply_cherry_picks is true which happens below this hunk, are you saying we should clear options.reapply_cherry_picks when we change upstream? I'm not sure that has any practical effect as the left hand side of options.upstream...options.orig_head will be empty so when we do the revision walk to generate the todo list. Best Wishes Phillip
Hi Philippe On 25/08/2022 01:29, Philippe Blain wrote: >> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt >> index 080658c8710..dc0c6c54e27 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. >> merge base of `<upstream>` and `<branch>`. Running >> `git rebase --keep-base <upstream> <branch>` is equivalent to >> running >> - `git rebase --onto <upstream>...<branch> <upstream> <branch>`. >> + `git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`. >> + >> This option is useful in the case where one is developing a feature on >> top of an upstream branch. While the feature is being worked on, the > > I think the doc should explicitely mention "This implies `--reapply-cherry-picks`", > in addition to your changes. That's a good idea, I've already extended the documentation to mention the effect of --keep-base in the discussion of --reapply-cherry-picks, I'll add an extra sentence to the discussion of --keep-base as well. Thanks Phillip
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 080658c8710..dc0c6c54e27 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -218,7 +218,7 @@ leave out at most one of A and B, in which case it defaults to HEAD. merge base of `<upstream>` and `<branch>`. Running `git rebase --keep-base <upstream> <branch>` is equivalent to running - `git rebase --onto <upstream>...<branch> <upstream> <branch>`. + `git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`. + This option is useful in the case where one is developing a feature on top of an upstream branch. While the feature is being worked on, the diff --git a/builtin/rebase.c b/builtin/rebase.c index 86ea731ca3a..b6b3e00e3b1 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1181,6 +1181,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) prepare_repo_settings(the_repository); the_repository->settings.command_requires_full_index = 0; + options.reapply_cherry_picks = -1; options.allow_empty_message = 1; git_config(rebase_config, &options); /* options.gpg_sign_opt will be either "-S" or NULL */ @@ -1240,6 +1241,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.root) die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root"); } + /* + * --keep-base defaults to --reapply-cherry-picks as it is confusing if + * commits disappear when using this option. + */ + if (options.reapply_cherry_picks < 0) + options.reapply_cherry_picks = keep_base; if (options.root && options.fork_point > 0) die(_("options '%s' and '%s' cannot be used together"), "--root", "--fork-point"); @@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) if (options.empty != EMPTY_UNSPECIFIED) imply_merge(&options, "--empty"); - if (options.reapply_cherry_picks) + /* + * --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) @@ -1680,6 +1691,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } if (keep_base) { oidcpy(&merge_base, &options.onto->object.oid); + if (options.reapply_cherry_picks) + options.upstream = options.onto; } else { fill_merge_base(&options, &merge_base); } diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh index e1fc2dbd48e..e0410bfc2a8 100755 --- a/t/t3416-rebase-onto-threedots.sh +++ b/t/t3416-rebase-onto-threedots.sh @@ -197,6 +197,27 @@ test_expect_success 'rebase -i --keep-base main from side' ' test_must_fail git rebase -i --keep-base main ' +test_expect_success 'rebase --keep-base keeps cherry picks' ' + git checkout -f -B main E && + git cherry-pick F && + ( + set_fake_editor && + EXPECT_COUNT=2 git rebase -i --keep-base HEAD G + ) && + test_cmp_rev HEAD G +' + +test_expect_success 'rebase --keep-base --no-reapply-cherry-picks' ' + git checkout -f -B main E && + git cherry-pick F && + ( + set_fake_editor && + EXPECT_COUNT=1 git rebase -i --keep-base \ + --no-reapply-cherry-picks HEAD G + ) && + test_cmp_rev HEAD^ C +' + # This must be the last test in this file test_expect_success '$EDITOR and friends are unchanged' ' test_editor_unchanged