Thread (1 message) 1 message, 1 author, 2022-08-16

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, 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".
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help