Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks
From: Junio C Hamano <hidden>
Date: 2022-08-16 01:13:40
"Phillip Wood via GitGitGadget" [off-list ref] writes:
quoted hunk
From: Phillip Wood <redacted> 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 <redacted> Signed-off-by: Phillip Wood <redacted> --- 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, thediff --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".
quoted hunk
@@ -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() ...
quoted hunk
@@ -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.