rebase: learn --keep-base

From: Denton Liu <[email protected]>

A common scenario is if a user is working on a topic branch and they wish to make some changes to intermediate commits or autosquash, they would run something such as

	git rebase -i --onto master... master

in order to preserve the merge base. This is useful when contributing a patch series to the Git mailing list, one often starts on top of the current 'master'. However, while developing the patches, 'master' is also developed further and it is sometimes not the bst idea to keep rebasing on top of 'master', but to keep the base commit as-is.

Alternatively, a user wishing to test individual commits in a topic branch without changing anything may run

git rebase -x ./test.sh master... master

Since rebasing onto the merge base of the branch and the upstream is such a common case, introduce the --keep-base option as a shortcut.

This allows us to rewrite the above as

	git rebase -i --keep-base master

and

	git rebase -x ./test.sh --keep-base master

respectively.

Add tests to ensure --keep-base works correctly in the normal case and fails when there are multiple merge bases, both in regular and interactive mode. Also, test to make sure conflicting options cause rebase to fail. While we're adding test cases, add a missing set_fake_editor call to 'rebase -i --onto master...side'.

While we're documenting the --keep-base option, change an instance of "merge-base" to "merge base", which is the consistent spelling.

---
 Documentation/git-rebase.txt           | 30 ++++++++++++--
 builtin/rebase.c                       | 32 ++++++++++++---
 contrib/completion/git-completion.bash |  2 +-
 t/t3416-rebase-onto-threedots.sh       | 57 ++++++++++++++++++++++++++
 t/t3431-rebase-fork-point.sh           |  4 ++
 t/t3432-rebase-fast-forward.sh         | 11 +++++
 6 files changed, 126 insertions(+), 10 deletions(-)

index f5e6ae3907..fafb7b4eb3 100644
@@ -8,8 +8,8 @@ git-rebase - Reapply commits on top of another base tip
 SYNOPSIS
 --------
 [verse]
-'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]
-	[<upstream> [<branch>]]
+'git rebase' [-i | --interactive] [<options>] [--exec <cmd>]
+	[--onto <newbase> | --keep-base] [<upstream> [<branch>]]
 'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]
 	--root [<branch>]
 'git rebase' --continue | --skip | --abort | --quit | --edit-todo | --show-current-patch

@@ -217,6 +217,24 @@ As a special case, you may use "A\...B" as a shortcut for the
 merge base of A and B if there is exactly one merge base. You can
 leave out at most one of A and B, in which case it defaults to HEAD.
 
+--keep-base::
+	Set the starting point at which to create the new commits to the
+	merge base of <upstream> <branch>. Running
+	'git rebase --keep-base <upstream> <branch>' is equivalent to
+	running 'git rebase --onto <upstream>... <upstream>'.
++
+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
+upstream branch may advance and it may not be the best idea to keep
+rebasing on top of the upstream but to keep the base commit as-is.
++
+Although both this option and --fork-point find the merge base between
+<upstream> and <branch>, this option uses the merge base as the _starting
+point_ on which new commits will be created, whereas --fork-point uses
+the merge base to determine the _set of commits_ which will be rebased.
++
+See also INCOMPATIBLE OPTIONS below.
+
 <upstream>::
 	Upstream branch to compare against.  May be any valid commit,
 	not just an existing branch name. Defaults to the configured

@@ -369,6 +387,10 @@ ends up being empty, the <upstream> will be used as a fallback.
 +
 If either <upstream> or --root is given on the command line, then the
 default is `--no-fork-point`, otherwise the default is `--fork-point`.
++
+If your branch was based on <upstream> but <upstream> was rewound and
+your branch contains commits which were dropped, this option can be used
+with `--keep-base` in order to drop those commits from your branch.
 
 --ignore-whitespace::
 --whitespace=<option>::

@@ -545,6 +567,8 @@ In addition, the following pairs of options are incompatible:
  * --preserve-merges and --rebase-merges
  * --rebase-merges and --strategy
  * --rebase-merges and --strategy-option
+ * --keep-base and --onto
+ * --keep-base and --root
 
 BEHAVIORAL DIFFERENCES
 -----------------------

@@ -869,7 +893,7 @@ NOTE: While an "easy case recovery" sometimes appears to be successful
       --interactive` will be **resurrected**!
 
 The idea is to manually tell 'git rebase' "where the old 'subsystem'
-ended and your 'topic' began", that is, what the old merge-base
+ended and your 'topic' began", that is, what the old merge base
 between them was.  You will have to find a way to name the last commit
 of the old 'subsystem', for example:
 

index 5f9beda46b..ae6b9b42b8 100644
@@ -27,8 +27,8 @@
 #include "branch.h"
 
 static char const * const builtin_rebase_usage[] = {
-	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
-		"[<upstream>] [<branch>]"),
+	N_("git rebase [-i] [options] [--exec <cmd>] "
+		"[--onto <newbase> | --keep-base] [<upstream> [<branch>]]"),
 	N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
 		"--root [<branch>]"),
 	N_("git rebase --continue | --abort | --skip | --edit-todo"),

@@ -1022,6 +1022,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	};
 	const char *branch_name;
 	int ret, flags, total_argc, in_progress = 0;
+	int keep_base = 0;
 	int ok_to_skip_pre_rebase = 0;
 	struct strbuf msg = STRBUF_INIT;
 	struct strbuf revisions = STRBUF_INIT;

@@ -1055,6 +1056,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
 			   N_("rebase onto given branch instead of upstream")),
+		OPT_BOOL(0, "keep-base", &keep_base,
+			 N_("use the merge-base of upstream and branch as the current base")),
 		OPT_BOOL(0, "no-verify", &ok_to_skip_pre_rebase,
 			 N_("allow pre-rebase hook to run")),
 		OPT_NEGBIT('q', "quiet", &options.flags,

@@ -1214,6 +1217,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		warning(_("git rebase --preserve-merges is deprecated. "
 			  "Use --rebase-merges instead."));
 
+	if (keep_base) {
+		if (options.onto_name)
+			die(_("cannot combine '--keep-base' with '--onto'"));
+		if (options.root)
+			die(_("cannot combine '--keep-base' with '--root'"));
+	}
+
 	if (action != NO_ACTION && !in_progress)
 		die(_("No rebase in progress?"));
 	setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);

@@ -1538,12 +1548,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Make sure the branch to rebase onto is valid. */
-	if (!options.onto_name)
+	if (keep_base) {
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, options.upstream_name);
+		strbuf_addstr(&buf, "...");
+		options.onto_name = xstrdup(buf.buf);
+	} else if (!options.onto_name)
 		options.onto_name = options.upstream_name;
 	if (strstr(options.onto_name, "...")) {
-		if (get_oid_mb(options.onto_name, &merge_base) < 0)
-			die(_("'%s': need exactly one merge base"),
-			    options.onto_name);
+		if (get_oid_mb(options.onto_name, &merge_base) < 0) {
+			if (keep_base)
+				die(_("'%s': need exactly one merge base with branch"),
+				    options.upstream_name);
+			else
+				die(_("'%s': need exactly one merge base"),
+				    options.onto_name);
+		}
 		options.onto = lookup_commit_or_die(&merge_base,
 						    options.onto_name);
 	} else {

index 3eefbabdb1..fe3768c8c6 100644
@@ -2028,7 +2028,7 @@ _git_rebase ()
 			--autosquash --no-autosquash
 			--fork-point --no-fork-point
 			--autostash --no-autostash
-			--verify --no-verify
+			--verify --no-verify --keep-base
 			--keep-empty --root --force-rebase --no-ff
 			--rerere-autoupdate
 			--exec

index 2d5c6e641e..78851b9a2a 100755
@@ -43,11 +43,15 @@ test_rebase () {
 
 test_rebase 'G F E D B A'
 test_rebase 'G F D B A' --onto D
+test_rebase 'G F B A' --keep-base
 test_rebase 'G F C E D B A' --no-fork-point
 test_rebase 'G F C D B A' --no-fork-point --onto D
+test_rebase 'G F C B A' --no-fork-point --keep-base
 test_rebase 'G F E D B A' --fork-point refs/heads/master
 test_rebase 'G F D B A' --fork-point --onto D refs/heads/master
+test_rebase 'G F B A' --fork-point --keep-base refs/heads/master
 test_rebase 'G F C E D B A' refs/heads/master
 test_rebase 'G F C D B A' --onto D refs/heads/master
+test_rebase 'G F C B A' --keep-base refs/heads/master
 
 test_done

index 8f4996e476..e8a9bf42b6 100755
@@ -75,11 +75,15 @@ test_rebase_same_head success noop same success noop-force same master
 test_rebase_same_head success noop same success noop-force diff --onto B B
 test_rebase_same_head success noop same success noop-force diff --onto B... B
 test_rebase_same_head success noop same success noop-force same --onto master... master
+test_rebase_same_head success noop same success noop-force same --keep-base master
+test_rebase_same_head success noop same success noop-force same --keep-base
 test_rebase_same_head success noop same success noop-force same --no-fork-point
+test_rebase_same_head success noop same success noop-force same --keep-base --no-fork-point
 test_rebase_same_head success noop same success work same --fork-point master
 test_rebase_same_head success noop same success work diff --fork-point --onto B B
 test_rebase_same_head success noop same success work diff --fork-point --onto B... B
 test_rebase_same_head success noop same success work same --fork-point --onto master... master
+test_rebase_same_head success noop same success work same --keep-base --keep-base master
 
 test_expect_success 'add work same  to side' '
 	test_commit E

@@ -91,11 +95,15 @@ test_rebase_same_head success noop same success noop-force same master
 test_rebase_same_head success noop same success noop-force diff --onto B B
 test_rebase_same_head success noop same success noop-force diff --onto B... B
 test_rebase_same_head success noop same success noop-force same --onto master... master
+test_rebase_same_head success noop same success noop-force same --keep-base master
+test_rebase_same_head success noop same success noop-force same --keep-base
 test_rebase_same_head success noop same success noop-force same --no-fork-point
+test_rebase_same_head success noop same success noop-force same --keep-base --no-fork-point
 test_rebase_same_head success noop same success work same --fork-point master
 test_rebase_same_head success noop same success work diff --fork-point --onto B B
 test_rebase_same_head success noop same success work diff --fork-point --onto B... B
 test_rebase_same_head success noop same success work same --fork-point --onto master... master
+test_rebase_same_head success noop same success work same --fork-point --keep-base master
 
 test_expect_success 'add work same  to upstream' '
 	git checkout master &&

@@ -107,8 +115,11 @@ changes='our and their changes'
 test_rebase_same_head success noop same success noop-force diff --onto B B
 test_rebase_same_head success noop same success noop-force diff --onto B... B
 test_rebase_same_head success noop same success work diff --onto master... master
+test_rebase_same_head success noop same success work diff --keep-base master
+test_rebase_same_head success noop same success work diff --keep-base
 test_rebase_same_head failure work same success work diff --fork-point --onto B B
 test_rebase_same_head failure work same success work diff --fork-point --onto B... B
 test_rebase_same_head success noop same success work diff --fork-point --onto master... master
+test_rebase_same_head success noop same success work diff --fork-point --keep-base master
 
 test_done

2.21.0.1020.gf2820cf01a

Add tests rebasing a linear branch topology to linear rebase tests added in 2aad7cace2 ("add simple tests of consistency across rebase types", 2013-06-06).

These tests are duplicates of two surrounding tests that do the same with tags pointing to the same objects. Right now there's no change in behavior being introduced, but as we'll see in a subsequent change rebase can have different behaviors when working implicitly with remote tracking branches.

While I'm at it add a --fork-point test, strictly speaking this is redundant to the existing '' test, as no argument to rebase implies --fork-point. But now it's easier to grep for tests that explicitly stress --fork-point.

---
 t/t3421-rebase-topology-linear.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

index 7274dca40b..b847064f91 100755
@@ -31,6 +31,16 @@ test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
 
+test_expect_success 'setup branches and remote tracking' '
+	git tag -l >tags &&
+	for tag in $(cat tags)
+	do
+		git branch branch-$tag $tag || return 1
+	done &&
+	git remote add origin "file://$PWD" &&
+	git fetch origin
+'
+
 test_run_rebase () {
 	result=$1
 	shift

@@ -57,10 +67,28 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
+test_run_rebase success --fork-point
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase failure -p
 
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* -f rewrites even if remote upstream is an ancestor" "
+		reset_rebase &&
+		git rebase $* -f branch-b branch-e &&
+		! test_cmp_rev branch-e origin/branch-e &&
+		test_cmp_rev branch-b HEAD~2 &&
+		test_linear_range 'd e' branch-b..
+	"
+}
+test_run_rebase success ''
+test_run_rebase success --fork-point
+test_run_rebase success -m
+test_run_rebase success -i
+test_have_prereq !REBASE_P || test_run_rebase success -p
+
 test_run_rebase () {
 	result=$1
 	shift

@@ -71,6 +99,7 @@ test_run_rebase () {
 	"
 }
 test_run_rebase success ''
+test_run_rebase success --fork-point
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p

2.21.0.1020.gf2820cf01a

Fix a regression introduced in 4f21454b55 ("merge-base: handle --fork-point without reflog", 2016-10-12).

Before that change having a linear history on top of an upstream master would with --fork-point (aka argument-less rebase) tell us there was nothing to be done:

    $ git rebase
    Current branch master is up to date.

After that change "rebase" will always redundantly find that it has work to do (it doesn't):

    $ git rebase
    First, rewinding head to replay your work on top of it...
    Applying: [...]

Whereas equivalently running:

    $ git rebase @{upstream}
    $ git rebase $(git merge-base --fork-point @{u})

Gives us the old behavior of doing nothing.

Now, why did we have this regression? Fully digging into it yields an interesting combination of causes:

Way back in 1308c17b3e ("Allow rebase to run if upstream is completely merged", 2007-07-04) "rebase" learned to not do this redundant work when asked to rebase on a commit that was already an ancestor of the current commit.

Then in 1e0dacdbdb ("rebase: omit patch-identical commits with --fork-point", 2014-07-16) a rebase bug was fixed for a case where the history to be rebased was divergent by entirely skipping the 2007-era logic if --fork-point was provided.

But here's the critical thing, *only* if the --fork-point was divergent. At that time "git merge-base --fork-point A B" would return nothing if the two commits weren't divergent.

Then in 4f21454b55 ("merge-base: handle --fork-point without reflog", 2016-10-12) which introduced the regression being fixed here, a bug fix for "git merge-base --fork-point" being run stand-alone by proxy broke this use-case git-rebase.sh was relying on, since it was still assuming that if we didn't have divergent history we'd have no output.

Finally, when "rebase" was rewritten in C a combination of 9a48a615b4 ("builtin rebase: try to fast forward when possible", 2018-09-04), 103148aad8 ("merge-base --fork-point: extract libified function", 2018-09-04) and 92d0d74e8d ("builtin rebase: support fork-point option", 2018-09-04) faithfully re-implemented the then-buggy behavior.

So let's fix this. It's easy enough, we just stop explicitly excluding --fork-point from the can_fast_forward(...) test we're doing, which as discussed above is faithfully ported over from buggy shellscript-era logic.

I'm not bothering to fix this in the legacy rebase mode. As discussed in 9aea5e9286 ("rebase: fix regression in rebase.useBuiltin=false test mode", 2019-02-13) it'll be going away shortly after 2.21.0 lands.

---
 t/t3421-rebase-topology-linear.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

index b847064f91..1754537789 100755
@@ -55,6 +55,21 @@ test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
 
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* is no-op if remote upstream is an ancestor" "
+		reset_rebase &&
+		GIT_TEST_REBASE_USE_BUILTIN=true git rebase $* branch-b branch-e &&
+		test_cmp_rev e HEAD
+	"
+}
+test_run_rebase success ''
+test_run_rebase success --fork-point
+test_run_rebase success -m
+test_run_rebase success -i
+test_have_prereq !REBASE_P || test_run_rebase success -p
+
 test_run_rebase () {
 	result=$1
 	shift

2.21.0.1020.gf2820cf01a

The can_fast_forward() function is potentially much more expensive than is_interactive() since it e.g. might need to call is_linear_history().

So reversing the two looks like an obvious improvement, but doing so reveals a previously hidden caveat: We need the can_fast_forward() function to populate data used later, namely the "merge_bases" variable. This has been the case since it was added in 9a48a615b4 ("builtin rebase: try to fast forward when possible", 2018-09-04).

So let's refactor it into two functions. Now we'll always call populate_merge_bases(), and then only call can_fast_forward() if is_interactive() is false, making this both faster in pathological cases, and more importantly easier to follow.

---
 builtin/rebase.c | 41 +++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

index ae6b9b42b8..cb5d7fcb53 100644
@@ -878,24 +878,30 @@ static int is_linear_history(struct commit *from, struct commit *to)
 	return 1;
 }
 
-static int can_fast_forward(struct commit *onto, struct commit *upstream,
+static void populate_merge_bases(struct commit *head, struct commit *onto,
+				 struct commit_list *merge_bases,
+				 struct object_id *merge_base)
+{
+	merge_bases = get_merge_bases(onto, head);
+	if (!merge_bases || merge_bases->next) {
+		oidcpy(merge_base, &null_oid);
+		return;
+	}
+	oidcpy(merge_base, &merge_bases->item->object.oid);
+}
+
+static int can_fast_forward(struct commit *head,
+			    struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct object_id *head_oid, struct object_id *merge_base)
+			    struct object_id *head_oid,
+			    struct commit_list *merge_bases,
+			    struct object_id *merge_base)
 {
-	struct commit *head = lookup_commit(the_repository, head_oid);
-	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
 	if (!head)
 		goto done;
 
-	merge_bases = get_merge_bases(onto, head);
-	if (!merge_bases || merge_bases->next) {
-		oidcpy(merge_base, &null_oid);
-		goto done;
-	}
-
-	oidcpy(merge_base, &merge_bases->item->object.oid);
 	if (!oideq(merge_base, &onto->object.oid))
 		goto done;
 

@@ -1154,6 +1160,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 	int i;
+	struct commit *head_commit;
+	struct commit_list *merge_bases = NULL;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_rebase_usage,

@@ -1703,9 +1711,14 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * with new commits recreated by replaying their changes. This
 	 * optimization must not be done if this is an interactive rebase.
 	 */
-	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    &options.orig_head, &merge_base) &&
-	    !is_interactive(&options)) {
+	head_commit = lookup_commit(the_repository, &options.orig_head);
+	if (head_commit)
+		populate_merge_bases(head_commit, options.onto, merge_bases,
+				     &merge_base);
+	if (!is_interactive(&options) &&
+	    can_fast_forward(head_commit, options.onto, options.upstream,
+			     options.restrict_revision, &options.orig_head,
+			     merge_bases, &merge_base)) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {

2.21.0.1020.gf2820cf01a

Hi Ævar,

True.

We might want to mention, though, what exactly that this "pathological case" is: rebasing commits onto an unrelated history.

The is_interactive() test gets tricky around --rebase-merges. Let's split our use of it for the purposes of whether we should try fast-forwarding a rebase into a utility function to prepare for adding more logic to that specific codepath.

---
 builtin/rebase.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

index cb5d7fcb53..167d4fcf67 100644
@@ -890,6 +890,11 @@ static void populate_merge_bases(struct commit *head, struct commit *onto,
 	oidcpy(merge_base, &merge_bases->item->object.oid);
 }
 
+static int should_fast_forward(struct rebase_options *opts)
+{
+	return !is_interactive(opts);
+}
+
 static int can_fast_forward(struct commit *head,
 			    struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,

@@ -1715,7 +1720,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (head_commit)
 		populate_merge_bases(head_commit, options.onto, merge_bases,
 				     &merge_base);
-	if (!is_interactive(&options) &&
+	if (should_fast_forward(&options) &&
 	    can_fast_forward(head_commit, options.onto, options.upstream,
 			     options.restrict_revision, &options.orig_head,
 			     merge_bases, &merge_base)) {

2.21.0.1020.gf2820cf01a

This seems to work, needs more tests etc...

---
 builtin/rebase.c               | 6 ++++++
 t/t3432-rebase-fast-forward.sh | 7 +++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

index 167d4fcf67..de1c5cacb8 100644
@@ -892,6 +892,12 @@ static void populate_merge_bases(struct commit *head, struct commit *onto,
 
 static int should_fast_forward(struct rebase_options *opts)
 {
+	if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) {
+		if (opts->rebase_merges)
+			return 1;
+		if (opts->type == REBASE_PRESERVE_MERGES)
+			return 1;
+	}
 	return !is_interactive(opts);
 }
 

index e8a9bf42b6..d3e1815057 100755
@@ -44,12 +44,13 @@ test_rebase_same_head_ () {
 	test_expect_$status "git rebase$flag $* with $changes is $what with $cmp HEAD" "
 		oldhead=\$(git rev-parse HEAD) &&
 		test_when_finished 'git reset --hard \$oldhead' &&
-		git rebase$flag $* >stdout &&
+		git rebase$flag $* >stdout 2>stderr &&
 		if test $what = work
 		then
 			# Must check this case first, for 'is up to
 			# date, rebase forced[...]rewinding head' cases
-			test_i18ngrep 'rewinding head' stdout
+			test_i18ngrep 'rewinding head' stdout ||
+			test_i18ngrep 'is up to date, rebase forced' stdout
 		elif test $what = noop
 		then
 			test_i18ngrep 'is up to date' stdout &&

@@ -79,6 +80,8 @@ test_rebase_same_head success noop same success noop-force same --keep-base mast
 test_rebase_same_head success noop same success noop-force same --keep-base
 test_rebase_same_head success noop same success noop-force same --no-fork-point
 test_rebase_same_head success noop same success noop-force same --keep-base --no-fork-point
+test_rebase_same_head success noop same success noop-force same --preserve-merges
+test_rebase_same_head success noop same success noop-force same --rebase-merges
 test_rebase_same_head success noop same success work same --fork-point master
 test_rebase_same_head success noop same success work diff --fork-point --onto B B
 test_rebase_same_head success noop same success work diff --fork-point --onto B... B

2.21.0.1020.gf2820cf01a

Hi Ævar,

I can see how it works, but it is a bit limited, and at the same time overzealous.

The reason why we do not enter the fast-forwarding block in the interactive case would appear to me to be that the interactive rebase *might* want to avoid fast-forwarding e.g. with --force-rebase.

However, that is not the only instance where we must not simply
fast-forward: Think --exec. There might be others, too. (I just saw that --signoff sets FORCE_REBASE, but --exec does not, so you cannot even use the FORCE_REBASE flag as indicator.)

Since git rebase -rx "make -j8" is something I use myself, this here patch would therefore break *my* workflow.

This is the "overzealous" part. Now for the "limited" part:

Let's take a step back and ask how the interactive rebase handles these potentially fast-forwarding cases? Via skip_unnecessary_picks().

And that function is ill-prepared for rebasing merges (I specifically do *not* think about --preserve-merges at this point, for all I care, it is already deprecated *and* dropped).

Even if this function *was* well-prepared for rebasing merges, I think that would miss the mark. Take this todo list, for example:

label onto

# Branch dscho
reset onto
pick a123 first
label dscho

# Branch avar
reset onto
pick b789 second
label avar

reset onto
merge -C c124 dscho
merge -C d314 avar

Two branches, both one patch deep, both merged, one after the other. Now, if you insert pick abc zeroth before the first pick, obviously the first branch would no longer be skippable, but the second one totally would be!

This is the "limited" part.

To remedy this, I think what we would need is code in pick_commits(), right where TODO_RESET is handled (or more toward the beginning of that function), that would:

- parse the argument (this is currently done in do_reset() and would have to be refactored out) and pretend that it is HEAD,

- look at the following command: if it is

- a pick, and if its parent agrees with HEAD, pretend that the pick was actually a reset, update the pretended HEAD and keep looking at the next command,

- a merge, and if its option was -C <orig-merge> (not lower-case -c!), and if its parent agrees with HEAD, and if its merge head(s) agree with the original merge commit's (if any), pretend that it was actually a reset <orig-merge>, update the pretended HEAD and keep looking at the next command,

- a label, perform it, but with the pretended HEAD, and keep looking for the next command,

- a reset, update the done and git-rebase-todo files and start the entire spiel from the top,

- otherwise perform the reset.

- all while skipping, this code would need to take care of updating the done and git-rebase-todo files,

- if a reset is necessary, and if it fails, the done and git-rebase-todo files should *not* be updated, but the original reset should be re-scheduled, and

- since this adds quite a bit of code, it should probably be done in a separate function.

Instead of marking this as a left-over bit (which I would either forget, or whose status would be hard to track), I decided to open a ticket:

https://github.com/gitgitgadget/git/issues/209

(I opened GitGitGadget's issues for exactly this kind of use case, because I recently tried to find some useful left-over bits as easy project starters, and even *I* found it super hard to find those, let alone figure out whether they are being/have been addressed already, a mailing list is just not a good bug tracker, even if it is still better than trying to report a bug on Twitter, where I could not even have written this paragraph in a single Tweet.)

Ciao,
Dscho

Late in replying to this, have been off-list. This also applies for your v4.

The current version you have still doesn't explain the "Why would we redundantly rebase every time in this case..." question I had in
https://public-inbox.org/git/[email protected]/

I *think* it's closer to "it was easier to implement this in terms of --onto, which happens to behave that way now" than "it must work this way for --keep-base", which is fair enough.

Although I see when I forward-port my POC patch from that E-Mail that one test fails now, which is good, that wasn't the case before, but it looks like that might be testing something else than just the lazy behavior.

It would be good to know in terms of commit message or (better) explicit tests so that if we teach these various rebase modes the same lazyness --fork-point uses in the future it's clear if that's OK or not.

Correct, the reason why --keep-base was not lazy initially was because "--onto did it that way". You are correct in that --keep-base should be lazily rebasing so I changed --onto's behaviour in 3/4 because it would also benefit from laziness. Thus, now --keep-base lazily rebases because --onto also does.

The test fails because the patch disables fork_point if --keep-base is set. So, with the patch applied, C is rebased even though it is excluded when fork_point is set.

Sorry, could you please clarify what you mean by the "lazyness --fork-point uses"? I don't understand what laziness is introduced by using --fork-point. Also, are the tests in t3432 not sufficient for testing fast-forwarding (aka lazy) behaviour?

Thanks,

Denton

Late reply, sorry. I mean if I do e.g. on git.git:

git checkout -b avar/test 041f5ea1cf

I.e. branch of Junio's "The third batch" (we're now on the 4th) and then:

    git branch --set-upstream-to origin/master

And commit a dummy change, I'm now:

On branch avar/test
Your branch and 'origin/master' have diverged, and have 1 and 33 different commits each, respectively.

Then I run a --keep-base rebase twice:

    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --keep-base; git rev-parse HEAD
    fc3e916c5f foo
    041f5ea1cf The third batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    b10e672074dfee6b6e8b2901c9bb49f856a13708
    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --keep-base; git rev-parse HEAD
    b10e672074 foo
    041f5ea1cf The third batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    fd3029e73b89f5a799653ff17199d00f2a6ee2e2

I.e. I'll keep on getting a new commit, even though by any criteria I can think of for this type of case there's no work to do. I.e. we're already based on 041f5ea1cf, no need to rebase again.

Of course this is also currently the case with --fork-point without argument, which'll settle on (note fourth, not third):

    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --fork-point; git rev-parse HEAD
    85a1861cec foo
    e35b8cb8e2 The fourth batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    c6046e97af29d71bf6270080acf188c095e0cb7c
    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --fork-point; git rev-parse HEAD
    c6046e97af foo
    e35b8cb8e2 The fourth batch
    First, rewinding head to replay your work on top of it...
    Applying: foo
    672d22d58e9aa9b6a28054531c21e1f1b598b013

Whereas a --keep-base in *this* case should be identical to:

    $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase $(git merge-base --fork-point origin/master HEAD); git rev-parse HEAD
    fc3e916c5f foo
    041f5ea1cf The third batch
    Current branch avar/test is up to date.
    fc3e916c5fc707cfc83f28b3faca81046306b305

The reason I'm asking is that as noted in the thread starting at
https://public-inbox.org/git/[email protected]/
we used to in *some* cases do the same for:

    rebase $(git merge-base --fork-point origin/master HEAD)

As for just:

    rebase --fork-point

I.e. say "Current branch is up-to-date". I'm planning to loop back to that and fix it for the --fork-point case. So I was wondering if you could think of a reason for why --keep-base couldn't also do the same, or if there was some intrinsic difference I'm missing.

Anyway. Don't worry about it. When I poke at it it'll likely shake out of the respective tests.

I was just wondering if you having looked at many of the same things recently could understand my ramblings and knew if this was a case of "yup, we could add a bit more to that can_fast_forward(...) case and make it lazy too", or "no, --keep-base is special for reasons you're missing...".