[PATCH] rebase: use correct base for --keep-base when a branch is given

[PATCH] rebase: use correct base for --keep-base when a branch is given

[Alex Henrie]

--keep-base rebases onto the merge base of the given upstream and the
current HEAD regardless of whether a branch is given. This is contrary
to the documentation and to the option's intended purpose. Instead,
rebase onto the merge base of the given upstream and the given branch.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/git-rebase.txt     |  5 +--
 builtin/rebase.c                 | 55 ++++++++++++++++----------------
 t/t3416-rebase-onto-threedots.sh | 33 +++++++++++++++++++
 3 files changed, 64 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9da4647061..262fb01aec 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -215,9 +215,10 @@ 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
+	merge base of <upstream> and <branch>. Running
 	'git rebase --keep-base <upstream> <branch>' is equivalent to
-	running 'git rebase --onto <upstream>... <upstream>'.
+	running
+	'git rebase --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 27fde7bf28..7f3bffc0a2 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1583,33 +1583,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.upstream_arg = "--root";
 	}
 
-	/* Make sure the branch to rebase onto is valid. */
-	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) {
-			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 {
-		options.onto =
-			lookup_commit_reference_by_name(options.onto_name);
-		if (!options.onto)
-			die(_("Does not point to a valid commit '%s'"),
-				options.onto_name);
-	}
-
 	/*
 	 * If the branch to rebase is given, that is the branch we will rebase
 	 * branch_name -- branch/commit being rebased, or
@@ -1659,6 +1632,34 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else
 		BUG("unexpected number of arguments left to parse");
 
+	/* Make sure the branch to rebase onto is valid. */
+	if (keep_base) {
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, options.upstream_name);
+		strbuf_addstr(&buf, "...");
+		strbuf_addstr(&buf, branch_name);
+		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) {
+			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 {
+		options.onto =
+			lookup_commit_reference_by_name(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
 	if (options.fork_point > 0) {
 		struct commit *head =
 			lookup_commit_reference(the_repository,
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3716a42e81..d1db528e25 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -129,6 +129,22 @@ test_expect_success 'rebase --keep-base main from topic' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase --keep-base main topic from main' '
+	git reset --hard &&
+	git checkout topic &&
+	git reset --hard G &&
+	git checkout main &&
+
+	git rebase --keep-base main topic &&
+	git rev-parse C >base.expect &&
+	git merge-base main HEAD >base.actual &&
+	test_cmp base.expect base.actual &&
+
+	git rev-parse HEAD~2 >actual &&
+	git rev-parse C^0 >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase --keep-base main from side' '
 	git reset --hard &&
 	git checkout side &&
@@ -153,6 +169,23 @@ test_expect_success 'rebase -i --keep-base main from topic' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase -i --keep-base main topic from main' '
+	git reset --hard &&
+	git checkout topic &&
+	git reset --hard G &&
+	git checkout main &&
+
+	set_fake_editor &&
+	EXPECT_COUNT=2 git rebase -i --keep-base main topic &&
+	git rev-parse C >base.expect &&
+	git merge-base main HEAD >base.actual &&
+	test_cmp base.expect base.actual &&
+
+	git rev-parse HEAD~2 >actual &&
+	git rev-parse C^0 >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase -i --keep-base main from side' '
 	git reset --hard &&
 	git checkout side &&
-- 
2.35.3

Re: [PATCH] rebase: use correct base for --keep-base when a branch is given

[Junio C Hamano]

Alex Henrie <alexhenrie24@gmail.com> writes:

> -	/* Make sure the branch to rebase onto is valid. */

So, we used to ...

> -	if (keep_base) {
> -		strbuf_reset(&buf);
> -		strbuf_addstr(&buf, options.upstream_name);
> -		strbuf_addstr(&buf, "...");
> -		options.onto_name = xstrdup(buf.buf);

... store "<upstream>..." in onto_name ...

> -	} else if (!options.onto_name)
> -		options.onto_name = options.upstream_name;

... or used <upstream> as onto_name, and then ...

> -	if (strstr(options.onto_name, "...")) {

... immediately used the "..." as a cue to compute the merge base
and ...

> -		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);

... used the "<upstream>..." as a label, and the merge base as the
"onto" commit.

> -	} else {
> -		options.onto =
> -			lookup_commit_reference_by_name(options.onto_name);
> -		if (!options.onto)
> -			die(_("Does not point to a valid commit '%s'"),
> -				options.onto_name);
> -	}
> -

But that was done before we even examined the optinal "this one, not
the current branch, is to be rebased" argument.  So it all works by
assuming the current branch is what is being rebase, which clearly
is wrong.

It is surprising that a basic and trivial bug was with us forever.
I wonder if the addition of --keep-base was faulty and the feature
was broken from the beginning, or there was an unrelated change that
broke the interaction between --keep-base and branch-to-be-rebased?

    ... goes and looks ...
    
    Between 640f9cd5 (Merge branch 'dl/rebase-i-keep-base',
    2019-09-30) which was where "--keep-base" was introduced, and
    today's codebase, there are many unrelated changes to
    builtin/rebase.c but this part is essentially unchanged.  Before
    the option was introduced, it didn't matter if the computation
    of "onto" came before or after the next part that is not shown
    in this patch, because "..." could have come only from the
    end-user input and there the end-user must have given the branch
    to be rebased on the right hand side of "..." if that is what
    they meant.  So, the feature was broken from the moment the
    "--keep-base" option was introduced.

Now we got rid of all the above, so we need to be careful that we do
not depend on options.onto_name and options.onto in the part that
you did not touch, from here to the precontext of the next hunk.

And I looked for onto and onto_name, these strings do not appear
from this point until the next hunk where we see an added line, so
we are not breaking that part of the code by removing this block of
code from here and moving it down.

>  	/*
>  	 * If the branch to rebase is given, that is the branch we will rebase
>  	 * branch_name -- branch/commit being rebased, or
> @@ -1659,6 +1632,34 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	} else
>  		BUG("unexpected number of arguments left to parse");
>  
> +	/* Make sure the branch to rebase onto is valid. */

And now we do what the removed block wanted to do here, but we do so
in a different context.  We know know which branch gets rebased in
the branch_name variable, so we do not use "<upstream>..." notation
to imply "the current branch" on the RHS; we can be explicit.

> +	if (keep_base) {
> +		strbuf_reset(&buf);
> +		strbuf_addstr(&buf, options.upstream_name);
> +		strbuf_addstr(&buf, "...");
> +		strbuf_addstr(&buf, branch_name);

... and that is what happens here, which is good.

The rest of this new block is a literal copy from the old code
removed above, which is as expected.

> +		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) {
> +			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 {
> +		options.onto =
> +			lookup_commit_reference_by_name(options.onto_name);
> +		if (!options.onto)
> +			die(_("Does not point to a valid commit '%s'"),
> +				options.onto_name);
> +	}
> +
>  	if (options.fork_point > 0) {
>  		struct commit *head =
>  			lookup_commit_reference(the_repository,



> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index 3716a42e81..d1db528e25 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -129,6 +129,22 @@ test_expect_success 'rebase --keep-base main from topic' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'rebase --keep-base main topic from main' '
> +	git reset --hard &&

Clearing whatever cruft the last test left is good, but ...

> +	git checkout topic &&
> +	git reset --hard G &&
> +	git checkout main &&

it would be more efficient to just do

	git checkout main &&
	git branch -f topic G &&

no?  Instead of rewriting the working tree three times, you only
need to do so once here.

Re: [PATCH] rebase: use correct base for --keep-base when a branch is given

[Alex Henrie]

On Wed, Apr 20, 2022 at 2:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> It is surprising that a basic and trivial bug was with us forever.

I was also surprised. Like you, I dug through the mailing list
archives and my impression was that the code review was mostly
concerned about the case where there are multiple merge bases. No one
seemed to have read the man page closely enough to notice that 'git
rebase --keep-base <upstream> <branch>' cannot be equivalent to 'git
rebase --onto <upstream>... <upstream>' because the latter doesn't
have <branch> at all. Seeing that discrepancy is what prompted me to
try out what actually happens when the branch is specified on the
command line, and then I realized that it was more than just a
documentation error.

> > +test_expect_success 'rebase --keep-base main topic from main' '
> > +     git reset --hard &&
>
> Clearing whatever cruft the last test left is good, but ...
>
> > +     git checkout topic &&
> > +     git reset --hard G &&
> > +     git checkout main &&
>
> it would be more efficient to just do
>
>         git checkout main &&
>         git branch -f topic G &&
>
> no?  Instead of rewriting the working tree three times, you only
> need to do so once here.

Sure. I'll make that change.

-Alex

[PATCH v2] rebase: use correct base for --keep-base when a branch is given

[Alex Henrie]

--keep-base rebases onto the merge base of the given upstream and the
current HEAD regardless of whether a branch is given. This is contrary
to the documentation and to the option's intended purpose. Instead,
rebase onto the merge base of the given upstream and the given branch.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
v2: clear previous test cruft more efficiently
---
 Documentation/git-rebase.txt     |  5 +--
 builtin/rebase.c                 | 55 ++++++++++++++++----------------
 t/t3416-rebase-onto-threedots.sh | 29 +++++++++++++++++
 3 files changed, 60 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9da4647061..262fb01aec 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -215,9 +215,10 @@ 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
+	merge base of <upstream> and <branch>. Running
 	'git rebase --keep-base <upstream> <branch>' is equivalent to
-	running 'git rebase --onto <upstream>... <upstream>'.
+	running
+	'git rebase --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 27fde7bf28..7f3bffc0a2 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1583,33 +1583,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		options.upstream_arg = "--root";
 	}
 
-	/* Make sure the branch to rebase onto is valid. */
-	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) {
-			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 {
-		options.onto =
-			lookup_commit_reference_by_name(options.onto_name);
-		if (!options.onto)
-			die(_("Does not point to a valid commit '%s'"),
-				options.onto_name);
-	}
-
 	/*
 	 * If the branch to rebase is given, that is the branch we will rebase
 	 * branch_name -- branch/commit being rebased, or
@@ -1659,6 +1632,34 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} else
 		BUG("unexpected number of arguments left to parse");
 
+	/* Make sure the branch to rebase onto is valid. */
+	if (keep_base) {
+		strbuf_reset(&buf);
+		strbuf_addstr(&buf, options.upstream_name);
+		strbuf_addstr(&buf, "...");
+		strbuf_addstr(&buf, branch_name);
+		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) {
+			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 {
+		options.onto =
+			lookup_commit_reference_by_name(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
 	if (options.fork_point > 0) {
 		struct commit *head =
 			lookup_commit_reference(the_repository,
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3716a42e81..3e04802cb0 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -129,6 +129,20 @@ test_expect_success 'rebase --keep-base main from topic' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase --keep-base main topic from main' '
+	git checkout main &&
+	git branch -f topic G &&
+
+	git rebase --keep-base main topic &&
+	git rev-parse C >base.expect &&
+	git merge-base main HEAD >base.actual &&
+	test_cmp base.expect base.actual &&
+
+	git rev-parse HEAD~2 >actual &&
+	git rev-parse C^0 >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase --keep-base main from side' '
 	git reset --hard &&
 	git checkout side &&
@@ -153,6 +167,21 @@ test_expect_success 'rebase -i --keep-base main from topic' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase -i --keep-base main topic from main' '
+	git checkout main &&
+	git branch -f topic G &&
+
+	set_fake_editor &&
+	EXPECT_COUNT=2 git rebase -i --keep-base main topic &&
+	git rev-parse C >base.expect &&
+	git merge-base main HEAD >base.actual &&
+	test_cmp base.expect base.actual &&
+
+	git rev-parse HEAD~2 >actual &&
+	git rev-parse C^0 >expect &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase -i --keep-base main from side' '
 	git reset --hard &&
 	git checkout side &&
-- 
2.36.0