[PATCH 0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[PATCH 0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[Phillip Wood via GitGitGadget]

A while a go Philippe reported [1] that he was surprised 'git rebase
--keep-base' removed commits that had been cherry-picked upstream even
though to branch was not being rebased. I think it is also surprising if
'--keep-base' changes the base of the branch without '--fork-point' being
explicitly given on the command line. This series therefore changes the
default behavior of '--keep-base' to imply '--reapply-cherry-picks' and
'--no-fork-point' so that the base of the branch is unchanged and no commits
are removed.

The first 3 patches are preparatory cleanups and refactorings, the last 2
patches then implement the new defaults for --keep-base.

[1]
https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Phillip Wood (5):
  t3416: set $EDITOR in subshell
  rebase: store orig_head as a commit
  rebase: factor out merge_base calculation
  rebase --keep-base: imply --reapply-cherry-picks
  rebase --keep-base: imply --no-fork-point

 Documentation/git-rebase.txt     |   2 +-
 builtin/rebase.c                 | 116 +++++++++++++++++++------------
 t/t3416-rebase-onto-threedots.sh |  52 +++++++++++---
 t/t3431-rebase-fork-point.sh     |   2 +-
 4 files changed, 114 insertions(+), 58 deletions(-)


base-commit: afa70145a25e81faa685dc0b465e52b45d2444bd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1323%2Fphillipwood%2Fwip%2Frebase--keep-base-tweaks-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1323/phillipwood/wip/rebase--keep-base-tweaks-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1323
-- 
gitgitgadget

[PATCH 1/5] t3416: set $EDITOR in subshell

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As $EDITOR is exported setting it in one test affects all subsequent
tests. Avoid this by always setting it in a subshell and remove a
couple of unnecessary call to set_fake_editor.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3e04802cb00..e1fc2dbd48e 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -79,8 +79,10 @@ test_expect_success 'rebase -i --onto main...topic' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main...topic F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main...topic F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -90,8 +92,10 @@ test_expect_success 'rebase -i --onto main...' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main... F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main... F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --onto main...side J
 '
 
@@ -156,8 +159,10 @@ test_expect_success 'rebase -i --keep-base main from topic' '
 	git checkout topic &&
 	git reset --hard G &&
 
-	set_fake_editor &&
-	EXPECT_COUNT=2 git rebase -i --keep-base main &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=2 git rebase -i --keep-base main
+	) &&
 	git rev-parse C >base.expect &&
 	git merge-base main HEAD >base.actual &&
 	test_cmp base.expect base.actual &&
@@ -171,8 +176,10 @@ 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 &&
+	(
+		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 &&
@@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --keep-base main
 '
 
+# This must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 1/5] t3416: set $EDITOR in subshell

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As $EDITOR is exported setting it in one test affects all subsequent
> tests. Avoid this by always setting it in a subshell and remove a
> couple of unnecessary call to set_fake_editor.

Unnecessary because it reuses the one that was established in the
previous test [1]?  Or unnecessary because we know "rebase -i" would
fail even before it gets to the point of asking an editor to tweak
the todo sequence [2]?  Or something else?

If [1], it makes us wonder what happens when an earlier test gets
skipped.  If [2], it makes us wonder what happens when "rebase -i"
fails to fail as expected (does the test correctly diagnose it as a
new breakage in "rebase -i"?).

> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
>  	git checkout side &&
>  	git reset --hard K &&
>  
> -	set_fake_editor &&
>  	test_must_fail git rebase -i --onto main...side J
>  '

This is one of the "removing" instances.

> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
>  	git checkout side &&
>  	git reset --hard K &&
>  
> -	set_fake_editor &&
>  	test_must_fail git rebase -i --keep-base main
>  '

And this is the other one.

Thanks.

Re: [PATCH 1/5] t3416: set $EDITOR in subshell

[Phillip Wood]

Hi Junio

On 15/08/2022 17:53, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> As $EDITOR is exported setting it in one test affects all subsequent
>> tests. Avoid this by always setting it in a subshell and remove a
>> couple of unnecessary call to set_fake_editor.
> 
> Unnecessary because it reuses the one that was established in the
> previous test [1]?  Or unnecessary because we know "rebase -i" would
> fail even before it gets to the point of asking an editor to tweak
> the todo sequence [2]?  Or something else?

I meant unnecessary as the editor does not change the todo list, but [2] 
also applies.

> If [1], it makes us wonder what happens when an earlier test gets
> skipped.  If [2], it makes us wonder what happens when "rebase -i"
> fails to fail as expected (does the test correctly diagnose it as a
> new breakage in "rebase -i"?).

I think those tests could be tightened up, I'll add a new patch that 
renames them to describe what they are testing (that we fail if there is 
more than one merge base) and greps for the expected error message.

Best Wishes

Phillip

>> @@ -102,7 +106,6 @@ test_expect_success 'rebase -i --onto main...side' '
>>   	git checkout side &&
>>   	git reset --hard K &&
>>   
>> -	set_fake_editor &&
>>   	test_must_fail git rebase -i --onto main...side J
>>   '
> 
> This is one of the "removing" instances.
> 
>> @@ -187,8 +194,12 @@ test_expect_success 'rebase -i --keep-base main from side' '
>>   	git checkout side &&
>>   	git reset --hard K &&
>>   
>> -	set_fake_editor &&
>>   	test_must_fail git rebase -i --keep-base main
>>   '
> 
> And this is the other one.
> 
> Thanks.

Re: [PATCH 1/5] t3416: set $EDITOR in subshell

[Jonathan Tan]

Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Junio
> 
> On 15/08/2022 17:53, Junio C Hamano wrote:
> > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > 
> >> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >>
> >> As $EDITOR is exported setting it in one test affects all subsequent
> >> tests. Avoid this by always setting it in a subshell and remove a
> >> couple of unnecessary call to set_fake_editor.
> > 
> > Unnecessary because it reuses the one that was established in the
> > previous test [1]?  Or unnecessary because we know "rebase -i" would
> > fail even before it gets to the point of asking an editor to tweak
> > the todo sequence [2]?  Or something else?
> 
> I meant unnecessary as the editor does not change the todo list, but [2] 
> also applies.

Maybe this is moot with the other changes you're planning, but even if
the editor doesn't change the todo list, it's still necessary, right? At
the very least, we need to suppress the default interactive editor and
replace it with one that just reuses the input file without any
modification.

Re: [PATCH 1/5] t3416: set $EDITOR in subshell

[Phillip Wood]

Hi Jonathan

On 24/08/2022 23:28, Jonathan Tan wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> Hi Junio
>>
>> On 15/08/2022 17:53, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>
>>>> As $EDITOR is exported setting it in one test affects all subsequent
>>>> tests. Avoid this by always setting it in a subshell and remove a
>>>> couple of unnecessary call to set_fake_editor.
>>>
>>> Unnecessary because it reuses the one that was established in the
>>> previous test [1]?  Or unnecessary because we know "rebase -i" would
>>> fail even before it gets to the point of asking an editor to tweak
>>> the todo sequence [2]?  Or something else?
>>
>> I meant unnecessary as the editor does not change the todo list, but [2]
>> also applies.
> 
> Maybe this is moot with the other changes you're planning, but even if
> the editor doesn't change the todo list, it's still necessary, right? At
> the very least, we need to suppress the default interactive editor and
> replace it with one that just reuses the input file without any
> modification.

The default GIT_EDITOR when running the test suite is ":" and 
GIT_SEQUENCE_EDITOR and sequence.editor are unset so we don't need to 
set an editor in the tests unless we want to change the todo list.

Best Wishes

Phillip

[PATCH 2/5] rebase: store orig_head as a commit

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Using a struct commit rather than a struct oid to hold orig_head means
that we error out straight away if branch being rebased does not point
to a commit. It also simplifies the code than handles finding the
merge base and fork point as it not longer has to convert from an oid
to a commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 62 ++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..6cf9c95f4e1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -68,7 +68,7 @@ struct rebase_options {
 	const char *upstream_name;
 	const char *upstream_arg;
 	char *head_name;
-	struct object_id orig_head;
+	struct commit *orig_head;
 	struct commit *onto;
 	const char *onto_name;
 	const char *revisions;
@@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	struct replay_opts replay = get_replay_opts(opts);
 	struct string_list commands = STRING_LIST_INIT_DUP;
 
-	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
+	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
 		return -1;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head)) {
+			     opts->onto, &opts->orig_head->object.oid)) {
 		free(revisions);
 		free(shortrevisions);
 
@@ -298,9 +298,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		split_exec_commands(opts->cmd, &commands);
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
-			&opts->orig_head, &commands, opts->autosquash,
-			opts->update_refs,
-			&todo_list);
+			&opts->orig_head->object.oid, &commands,
+			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
 	string_list_clear(&commands, 0);
@@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid(buf.buf, &opts->orig_head))
+	opts->orig_head = lookup_commit_reference_by_name(buf.buf);
+	if (!opts->orig_head)
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
@@ -517,7 +517,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	write_file(state_dir_path("onto", opts), "%s",
 		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
 	write_file(state_dir_path("orig-head", opts), "%s",
-		   oid_to_hex(&opts->orig_head));
+		   oid_to_hex(&opts->orig_head->object.oid));
 	if (!(opts->flags & REBASE_NO_QUIET))
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
@@ -646,7 +646,7 @@ static int run_am(struct rebase_options *opts)
 			       /* this is now equivalent to !opts->upstream */
 			       &opts->onto->object.oid :
 			       &opts->upstream->object.oid),
-		    oid_to_hex(&opts->orig_head));
+		    oid_to_hex(&opts->orig_head->object.oid));
 
 	rebased_patches = xstrdup(git_path("rebased-patches"));
 	format_patch.out = open(rebased_patches,
@@ -680,7 +680,7 @@ static int run_am(struct rebase_options *opts)
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		ropts.oid = &opts->orig_head;
+		ropts.oid = &opts->orig_head->object.oid;
 		ropts.branch = opts->head_name;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		reset_head(the_repository, &ropts);
@@ -833,7 +833,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	ropts.oid = &options->orig_head;
+	ropts.oid = &options->orig_head->object.oid;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	if (!ropts.branch)
@@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct object_id *head_oid, struct object_id *merge_base)
+			    struct commit *head, 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());
@@ -1312,13 +1308,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		ropts.oid = &options.orig_head;
+		ropts.oid = &options.orig_head->object.oid;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
-			    oid_to_hex(&options.orig_head));
+			    oid_to_hex(&options.orig_head->object.oid));
 		remove_branch_state(the_repository, 0);
 		ret = finish_rebase(&options);
 		goto cleanup;
@@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		/* Is it a local branch? */
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "refs/heads/%s", branch_name);
-		if (!read_ref(buf.buf, &options.orig_head)) {
+		options.orig_head = lookup_commit_reference_by_name(buf.buf);
+		if (options.orig_head) {
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
 		/* If not is it a valid ref (branch or commit)? */
 		} else {
-			struct commit *commit =
+			options.orig_head =
 				lookup_commit_reference_by_name(branch_name);
-			if (!commit)
+			if (!options.orig_head)
 				die(_("no such branch/commit '%s'"),
 				    branch_name);
-			oidcpy(&options.orig_head, &commit->object.oid);
 			options.head_name = NULL;
 		}
 	} else if (argc == 0) {
@@ -1639,8 +1635,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			FREE_AND_NULL(options.head_name);
 			branch_name = "HEAD";
 		}
-		if (get_oid("HEAD", &options.orig_head))
-			die(_("Could not resolve HEAD to a revision"));
+		options.orig_head = lookup_commit_reference_by_name("HEAD");
+		if (!options.orig_head)
+			die(_("Could not resolve HEAD to a commit"));
 	} else
 		BUG("unexpected number of arguments left to parse");
 
@@ -1672,13 +1669,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				options.onto_name);
 	}
 
-	if (options.fork_point > 0) {
-		struct commit *head =
-			lookup_commit_reference(the_repository,
-						&options.orig_head);
+	if (options.fork_point > 0)
 		options.restrict_revision =
-			get_fork_point(options.upstream_name, head);
-	}
+			get_fork_point(options.upstream_name, options.orig_head);
+
 
 	if (repo_read_index(the_repository) < 0)
 		die(_("could not read index"));
@@ -1708,7 +1702,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    &options.orig_head, &merge_base) &&
+		    options.orig_head, &merge_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1785,7 +1779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
-	ropts.orig_head = &options.orig_head,
+	ropts.orig_head = &options.orig_head->object.oid,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;
@@ -1799,7 +1793,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head)) {
+	if (oideq(&merge_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
@@ -1820,7 +1814,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		    (options.restrict_revision ?
 		     oid_to_hex(&options.restrict_revision->object.oid) :
 		     oid_to_hex(&options.upstream->object.oid)),
-		    oid_to_hex(&options.orig_head));
+		    oid_to_hex(&options.orig_head->object.oid));
 
 	options.revisions = revisions.buf;
 
-- 
gitgitgadget

Re: [PATCH 2/5] rebase: store orig_head as a commit

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a struct commit rather than a struct oid to hold orig_head means
> that we error out straight away if branch being rebased does not point
> to a commit. It also simplifies the code than handles finding the
> merge base and fork point as it not longer has to convert from an oid
> to a commit.

As lookup_commit_reference_by_name() eventually calls into deref_tag(),
a command like

    git rebase -i maint $(git rev-parse v2.37.2)

would presumably still work, which is good.

Re: [PATCH 2/5] rebase: store orig_head as a commit

[Johannes Schindelin]

Hi Phillip,

On Mon, 15 Aug 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a struct commit rather than a struct oid to hold orig_head means
> that we error out straight away if branch being rebased does not point
> to a commit. It also simplifies the code than handles finding the
> merge base and fork point as it not longer has to convert from an oid
> to a commit.

Very nice!

The diff is necessarily "chatty", therefore it is especially good that you
separated out this concern into its own, easily-reviewable commit.

Thank you,
Dscho

Re: [PATCH 2/5] rebase: store orig_head as a commit

[Elijah Newren]

On Mon, Aug 15, 2022 at 8:14 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Using a struct commit rather than a struct oid to hold orig_head means
> that we error out straight away if branch being rebased does not point
> to a commit. It also simplifies the code than handles finding the

s/than/that/ ?


> merge base and fork point as it not longer has to convert from an oid
> to a commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 62 ++++++++++++++++++++++--------------------------
>  1 file changed, 28 insertions(+), 34 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 56e4214b441..6cf9c95f4e1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -68,7 +68,7 @@ struct rebase_options {
>         const char *upstream_name;
>         const char *upstream_arg;
>         char *head_name;
> -       struct object_id orig_head;
> +       struct commit *orig_head;
>         struct commit *onto;
>         const char *onto_name;
>         const char *revisions;
> @@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>         struct replay_opts replay = get_replay_opts(opts);
>         struct string_list commands = STRING_LIST_INIT_DUP;
>
> -       if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
> +       if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
>                                 &revisions, &shortrevisions))
>                 return -1;
>
>         if (init_basic_state(&replay,
>                              opts->head_name ? opts->head_name : "detached HEAD",
> -                            opts->onto, &opts->orig_head)) {
> +                            opts->onto, &opts->orig_head->object.oid)) {
>                 free(revisions);
>                 free(shortrevisions);
>
> @@ -298,9 +298,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>                 split_exec_commands(opts->cmd, &commands);
>                 ret = complete_action(the_repository, &replay, flags,
>                         shortrevisions, opts->onto_name, opts->onto,
> -                       &opts->orig_head, &commands, opts->autosquash,
> -                       opts->update_refs,
> -                       &todo_list);
> +                       &opts->orig_head->object.oid, &commands,
> +                       opts->autosquash, opts->update_refs, &todo_list);
>         }
>
>         string_list_clear(&commands, 0);
> @@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
>         } else if (!read_oneliner(&buf, state_dir_path("head", opts),
>                                   READ_ONELINER_WARN_MISSING))
>                 return -1;
> -       if (get_oid(buf.buf, &opts->orig_head))
> +       opts->orig_head = lookup_commit_reference_by_name(buf.buf);
> +       if (!opts->orig_head)
>                 return error(_("invalid orig-head: '%s'"), buf.buf);
>
>         if (file_exists(state_dir_path("quiet", opts)))
> @@ -517,7 +517,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
>         write_file(state_dir_path("onto", opts), "%s",
>                    opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
>         write_file(state_dir_path("orig-head", opts), "%s",
> -                  oid_to_hex(&opts->orig_head));
> +                  oid_to_hex(&opts->orig_head->object.oid));
>         if (!(opts->flags & REBASE_NO_QUIET))
>                 write_file(state_dir_path("quiet", opts), "%s", "");
>         if (opts->flags & REBASE_VERBOSE)
> @@ -646,7 +646,7 @@ static int run_am(struct rebase_options *opts)
>                                /* this is now equivalent to !opts->upstream */
>                                &opts->onto->object.oid :
>                                &opts->upstream->object.oid),
> -                   oid_to_hex(&opts->orig_head));
> +                   oid_to_hex(&opts->orig_head->object.oid));
>
>         rebased_patches = xstrdup(git_path("rebased-patches"));
>         format_patch.out = open(rebased_patches,
> @@ -680,7 +680,7 @@ static int run_am(struct rebase_options *opts)
>                 free(rebased_patches);
>                 strvec_clear(&am.args);
>
> -               ropts.oid = &opts->orig_head;
> +               ropts.oid = &opts->orig_head->object.oid;
>                 ropts.branch = opts->head_name;
>                 ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
>                 reset_head(the_repository, &ropts);
> @@ -833,7 +833,7 @@ static int checkout_up_to_date(struct rebase_options *options)
>         strbuf_addf(&buf, "%s: checkout %s",
>                     getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
>                     options->switch_to);
> -       ropts.oid = &options->orig_head;
> +       ropts.oid = &options->orig_head->object.oid;
>         ropts.branch = options->head_name;
>         ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
>         if (!ropts.branch)
> @@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
>
>  static int can_fast_forward(struct commit *onto, struct commit *upstream,
>                             struct commit *restrict_revision,
> -                           struct object_id *head_oid, struct object_id *merge_base)
> +                           struct commit *head, 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());
> @@ -1312,13 +1308,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
>                 if (read_basic_state(&options))
>                         exit(1);
> -               ropts.oid = &options.orig_head;
> +               ropts.oid = &options.orig_head->object.oid;
>                 ropts.branch = options.head_name;
>                 ropts.flags = RESET_HEAD_HARD;
>                 ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
>                 if (reset_head(the_repository, &ropts) < 0)
>                         die(_("could not move back to %s"),
> -                           oid_to_hex(&options.orig_head));
> +                           oid_to_hex(&options.orig_head->object.oid));
>                 remove_branch_state(the_repository, 0);
>                 ret = finish_rebase(&options);
>                 goto cleanup;
> @@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                 /* Is it a local branch? */
>                 strbuf_reset(&buf);
>                 strbuf_addf(&buf, "refs/heads/%s", branch_name);
> -               if (!read_ref(buf.buf, &options.orig_head)) {
> +               options.orig_head = lookup_commit_reference_by_name(buf.buf);
> +               if (options.orig_head) {
>                         die_if_checked_out(buf.buf, 1);
>                         options.head_name = xstrdup(buf.buf);
>                 /* If not is it a valid ref (branch or commit)? */
>                 } else {
> -                       struct commit *commit =
> +                       options.orig_head =
>                                 lookup_commit_reference_by_name(branch_name);
> -                       if (!commit)
> +                       if (!options.orig_head)
>                                 die(_("no such branch/commit '%s'"),
>                                     branch_name);
> -                       oidcpy(&options.orig_head, &commit->object.oid);
>                         options.head_name = NULL;
>                 }
>         } else if (argc == 0) {
> @@ -1639,8 +1635,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                         FREE_AND_NULL(options.head_name);
>                         branch_name = "HEAD";
>                 }
> -               if (get_oid("HEAD", &options.orig_head))
> -                       die(_("Could not resolve HEAD to a revision"));
> +               options.orig_head = lookup_commit_reference_by_name("HEAD");
> +               if (!options.orig_head)
> +                       die(_("Could not resolve HEAD to a commit"));
>         } else
>                 BUG("unexpected number of arguments left to parse");
>
> @@ -1672,13 +1669,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                                 options.onto_name);
>         }
>
> -       if (options.fork_point > 0) {
> -               struct commit *head =
> -                       lookup_commit_reference(the_repository,
> -                                               &options.orig_head);
> +       if (options.fork_point > 0)
>                 options.restrict_revision =
> -                       get_fork_point(options.upstream_name, head);
> -       }
> +                       get_fork_point(options.upstream_name, options.orig_head);
> +
>
>         if (repo_read_index(the_repository) < 0)
>                 die(_("could not read index"));
> @@ -1708,7 +1702,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>          * call it before checking allow_preemptive_ff.
>          */
>         if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
> -                   &options.orig_head, &merge_base) &&
> +                   options.orig_head, &merge_base) &&
>             allow_preemptive_ff) {
>                 int flag;
>
> @@ -1785,7 +1779,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>         strbuf_addf(&msg, "%s: checkout %s",
>                     getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
>         ropts.oid = &options.onto->object.oid;
> -       ropts.orig_head = &options.orig_head,
> +       ropts.orig_head = &options.orig_head->object.oid,
>         ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
>                         RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
>         ropts.head_msg = msg.buf;
> @@ -1799,7 +1793,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>          * we just fast-forwarded.
>          */
>         strbuf_reset(&msg);
> -       if (oideq(&merge_base, &options.orig_head)) {
> +       if (oideq(&merge_base, &options.orig_head->object.oid)) {
>                 printf(_("Fast-forwarded %s to %s.\n"),
>                         branch_name, options.onto_name);
>                 strbuf_addf(&msg, "rebase finished: %s onto %s",
> @@ -1820,7 +1814,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                     (options.restrict_revision ?
>                      oid_to_hex(&options.restrict_revision->object.oid) :
>                      oid_to_hex(&options.upstream->object.oid)),
> -                   oid_to_hex(&options.orig_head));
> +                   oid_to_hex(&options.orig_head->object.oid));
>
>         options.revisions = revisions.buf;
>
> --
> gitgitgadget
>

[PATCH 3/5] rebase: factor out merge_base calculation

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Separate out calculating the merge base between onto and head from the
check for whether we can fast-forward or not. This means we can skip
the fast-forward checks when the rebase is forced and avoid
calculating the merge-base twice when --keep-base is given.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
Note the unnecessary braces around "if (keep_base)" are added here
reduce the churn on the next commit.
---
 builtin/rebase.c | 35 +++++++++++++++++++++++------------
 1 file changed, 23 insertions(+), 12 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6cf9c95f4e1..86ea731ca3a 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
-	merge_bases = get_merge_bases(onto, head);
-	if (!merge_bases || merge_bases->next) {
-		oidcpy(merge_base, null_oid());
+	if (is_null_oid(merge_base))
 		goto done;
-	}
 
-	oidcpy(merge_base, &merge_bases->item->object.oid);
 	if (!oideq(merge_base, &onto->object.oid))
 		goto done;
 
@@ -902,6 +898,20 @@ done:
 	return res && is_linear_history(onto, head);
 }
 
+static void fill_merge_base(struct rebase_options *options,
+			    struct object_id *merge_base)
+{
+	struct commit_list *merge_bases = NULL;
+
+	merge_bases = get_merge_bases(options->onto, options->orig_head);
+	if (!merge_bases || merge_bases->next)
+		oidcpy(merge_base, null_oid());
+	else
+		oidcpy(merge_base, &merge_bases->item->object.oid);
+
+	free_commit_list(merge_bases);
+}
+
 static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 {
 	struct rebase_options *opts = opt->value;
@@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
 	}
-
+	if (keep_base) {
+		oidcpy(&merge_base, &options.onto->object.oid);
+	} else {
+		fill_merge_base(&options, &merge_base);
+	}
 	if (options.fork_point > 0)
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
@@ -1697,13 +1711,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * Check if we are already based on onto with linear history,
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
-	 *
-	 * Note that can_fast_forward() initializes merge_base, so we have to
-	 * call it before checking allow_preemptive_ff.
 	 */
-	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &merge_base) &&
-	    allow_preemptive_ff) {
+	if (allow_preemptive_ff &&
+	    can_fast_forward(options.onto, options.upstream, options.restrict_revision,
+			     options.orig_head, &merge_base)) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {
-- 
gitgitgadget

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Separate out calculating the merge base between onto and head from the
> check for whether we can fast-forward or not. This means we can skip
> the fast-forward checks when the rebase is forced and avoid
> calculating the merge-base twice when --keep-base is given.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Note the unnecessary braces around "if (keep_base)" are added here
> reduce the churn on the next commit.
> ---
>  builtin/rebase.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6cf9c95f4e1..86ea731ca3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>  	struct commit_list *merge_bases = NULL;
>  	int res = 0;
>  
> -	merge_bases = get_merge_bases(onto, head);
> -	if (!merge_bases || merge_bases->next) {
> -		oidcpy(merge_base, null_oid());
> +	if (is_null_oid(merge_base))
>  		goto done;
> -	}
>  
> -	oidcpy(merge_base, &merge_bases->item->object.oid);
>  	if (!oideq(merge_base, &onto->object.oid))
>  		goto done;

Looking at the change in "git show -W", it seems that this function
no longer touches merge_bases at all, other than initializing it to
NULL at the beginning and then calling free_commit_list() on it at
the end.  Shouldn't it be removed?

> @@ -902,6 +898,20 @@ done:
>  	return res && is_linear_history(onto, head);
>  }
>  
> +static void fill_merge_base(struct rebase_options *options,
> +			    struct object_id *merge_base)
> +{
> +	struct commit_list *merge_bases = NULL;
> +
> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
> +	if (!merge_bases || merge_bases->next)
> +		oidcpy(merge_base, null_oid());
> +	else
> +		oidcpy(merge_base, &merge_bases->item->object.oid);
> +
> +	free_commit_list(merge_bases);
> +}
> +
>  static int parse_opt_am(const struct option *opt, const char *arg, int unset)
>  {
>  	struct rebase_options *opts = opt->value;
> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			die(_("Does not point to a valid commit '%s'"),
>  				options.onto_name);
>  	}
> -
> +	if (keep_base) {
> +		oidcpy(&merge_base, &options.onto->object.oid);
> +	} else {
> +		fill_merge_base(&options, &merge_base);
> +	}

No need for braces around single-statement block on either side.

This is not a new issue introduced by this patch per-se, but
"merge_base" is becoming less and less accurate description of what
this variable really is.  Perhaps it is a good time to rename it?

It is "the base commit to rebuild the history on top of", aka "onto
commit", isn't it?  We often use merge-base between the upstream and
our tip of the history for it, but the variable often does not even
hold the merge-base in it, not because we cannot compute a single
merge-base but because depending on the operation mode we do not
want to use merge-base for further operation using that variable.

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Johannes Schindelin]

Hi Junio,

On Mon, 15 Aug 2022, Junio C Hamano wrote:

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > Separate out calculating the merge base between onto and head from the
> > check for whether we can fast-forward or not. This means we can skip
> > the fast-forward checks when the rebase is forced and avoid
> > calculating the merge-base twice when --keep-base is given.
> >
> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> > ---
> > Note the unnecessary braces around "if (keep_base)" are added here
> > reduce the churn on the next commit.

This note...

> > @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  			die(_("Does not point to a valid commit '%s'"),
> >  				options.onto_name);
> >  	}
> > -
> > +	if (keep_base) {
> > +		oidcpy(&merge_base, &options.onto->object.oid);
> > +	} else {
> > +		fill_merge_base(&options, &merge_base);
> > +	}
>
> No need for braces around single-statement block on either side.

... already addresses this feedback.

Ciao,
Dscho

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Mon, 15 Aug 2022, Junio C Hamano wrote:
>
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
>> >
>> > Separate out calculating the merge base between onto and head from the
>> > check for whether we can fast-forward or not. This means we can skip
>> > the fast-forward checks when the rebase is forced and avoid
>> > calculating the merge-base twice when --keep-base is given.
>> >
>> > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> > ---
>> > Note the unnecessary braces around "if (keep_base)" are added here
>> > reduce the churn on the next commit.
>
> This note...
>
>> > @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>> >  			die(_("Does not point to a valid commit '%s'"),
>> >  				options.onto_name);
>> >  	}
>> > -
>> > +	if (keep_base) {
>> > +		oidcpy(&merge_base, &options.onto->object.oid);
>> > +	} else {
>> > +		fill_merge_base(&options, &merge_base);
>> > +	}
>>
>> No need for braces around single-statement block on either side.
>
> ... already addresses this feedback.

Yeah, but the point is instead of wasting readers' bandwidth with an
additional note, the series can add braces in the step where they
become necessary, i.e. the later step where there is a new statement
in the body of the "if-true" side.

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Phillip Wood]

Hi Junio

On 15/08/2022 18:22, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Separate out calculating the merge base between onto and head from the
>> check for whether we can fast-forward or not. This means we can skip
>> the fast-forward checks when the rebase is forced and avoid
>> calculating the merge-base twice when --keep-base is given.

I should clarify that this is referring to the merge base of onto and 
upstream.

>>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>> Note the unnecessary braces around "if (keep_base)" are added here
>> reduce the churn on the next commit.
>> ---
>>   builtin/rebase.c | 35 +++++++++++++++++++++++------------
>>   1 file changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 6cf9c95f4e1..86ea731ca3a 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>>   	struct commit_list *merge_bases = NULL;
>>   	int res = 0;
>>   
>> -	merge_bases = get_merge_bases(onto, head);
>> -	if (!merge_bases || merge_bases->next) {
>> -		oidcpy(merge_base, null_oid());
>> +	if (is_null_oid(merge_base))
>>   		goto done;
>> -	}
>>   
>> -	oidcpy(merge_base, &merge_bases->item->object.oid);
>>   	if (!oideq(merge_base, &onto->object.oid))
>>   		goto done;
> 
> Looking at the change in "git show -W", it seems that this function
> no longer touches merge_bases at all, other than initializing it to
> NULL at the beginning and then calling free_commit_list() on it at
> the end.  Shouldn't it be removed?

There is still the line

	merge_bases = get_merge_bases(upstream, head);

lower down. I should remove the call to free_commit_list() just above 
that line though as it is no longer needed.

>> @@ -902,6 +898,20 @@ done:
>>   	return res && is_linear_history(onto, head);
>>   }
>>   
>> +static void fill_merge_base(struct rebase_options *options,
>> +			    struct object_id *merge_base)
>> +{
>> +	struct commit_list *merge_bases = NULL;
>> +
>> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
>> +	if (!merge_bases || merge_bases->next)
>> +		oidcpy(merge_base, null_oid());
>> +	else
>> +		oidcpy(merge_base, &merge_bases->item->object.oid);
>> +
>> +	free_commit_list(merge_bases);
>> +}
>> +
>>   static int parse_opt_am(const struct option *opt, const char *arg, int unset)
>>   {
>>   	struct rebase_options *opts = opt->value;
>> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   			die(_("Does not point to a valid commit '%s'"),
>>   				options.onto_name);
>>   	}
>> -
>> +	if (keep_base) {
>> +		oidcpy(&merge_base, &options.onto->object.oid);

This is actually unnecessary as we do

	options.onto = lookup_commit_reference_by_name(options.onto_name);

above when calculating onto.

>> +	} else {
>> +		fill_merge_base(&options, &merge_base);
>> +	}
> 
> No need for braces around single-statement block on either side.
> 
> This is not a new issue introduced by this patch per-se, but
> "merge_base" is becoming less and less accurate description of what
> this variable really is.  Perhaps it is a good time to rename it?

Yes, merge_base is not a very descriptive name as it prompts the 
question "merge base of which commits". I think base_commit or 
branch_base would be better.

> It is "the base commit to rebuild the history on top of", aka "onto
> commit", isn't it?

I think it is the base commit of the branch i.e. we rebase all the 
commits in the range merge_base..branch onto the "onto commit".

> We often use merge-base between the upstream and
> our tip of the history for it,

I'm pretty sure it is always the merge-base of the "onto commit" and our 
tip of the history. When using --keep-base we calculate the "onto 
commit" as the merge base of upstream and our tip of the history which 
makes it look were using that for merge_base but that commit is also the 
merge-base of the "onto commit" and our tip of history.

Best Wishes

Phillip

> but the variable often does not even
> hold the merge-base in it, not because we cannot compute a single
> merge-base but because depending on the operation mode we do not
> want to use merge-base for further operation using that variable.

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>>   -	merge_bases = get_merge_bases(onto, head);
>>> -	if (!merge_bases || merge_bases->next) {
>>> -		oidcpy(merge_base, null_oid());
>>> +	if (is_null_oid(merge_base))
>>>   		goto done;
>>> -	}
>>>   -	oidcpy(merge_base, &merge_bases->item->object.oid);
>>>   	if (!oideq(merge_base, &onto->object.oid))
>>>   		goto done;
>> Looking at the change in "git show -W", it seems that this function
>> no longer touches merge_bases at all, other than initializing it to
>> NULL at the beginning and then calling free_commit_list() on it at
>> the end.  Shouldn't it be removed?
>
> There is still the line
>
> 	merge_bases = get_merge_bases(upstream, head);
>
> lower down. I should remove the call to free_commit_list() just above
> that line though as it is no longer needed.

Yup, that is correct.

Thanks.

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Elijah Newren]

On Mon, Aug 15, 2022 at 8:14 AM Phillip Wood via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Separate out calculating the merge base between onto and head from the
> check for whether we can fast-forward or not. This means we can skip
> the fast-forward checks when the rebase is forced and avoid
> calculating the merge-base twice when --keep-base is given.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> Note the unnecessary braces around "if (keep_base)" are added here
> reduce the churn on the next commit.
> ---
>  builtin/rebase.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6cf9c95f4e1..86ea731ca3a 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>         struct commit_list *merge_bases = NULL;
>         int res = 0;
>
> -       merge_bases = get_merge_bases(onto, head);
> -       if (!merge_bases || merge_bases->next) {
> -               oidcpy(merge_base, null_oid());
> +       if (is_null_oid(merge_base))
>                 goto done;
> -       }
>
> -       oidcpy(merge_base, &merge_bases->item->object.oid);
>         if (!oideq(merge_base, &onto->object.oid))
>                 goto done;
>
> @@ -902,6 +898,20 @@ done:
>         return res && is_linear_history(onto, head);
>  }
>
> +static void fill_merge_base(struct rebase_options *options,
> +                           struct object_id *merge_base)
> +{
> +       struct commit_list *merge_bases = NULL;
> +
> +       merge_bases = get_merge_bases(options->onto, options->orig_head);
> +       if (!merge_bases || merge_bases->next)
> +               oidcpy(merge_base, null_oid());
> +       else
> +               oidcpy(merge_base, &merge_bases->item->object.oid);
> +
> +       free_commit_list(merge_bases);
> +}
> +
>  static int parse_opt_am(const struct option *opt, const char *arg, int unset)
>  {
>         struct rebase_options *opts = opt->value;
> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>                         die(_("Does not point to a valid commit '%s'"),
>                                 options.onto_name);
>         }
> -
> +       if (keep_base) {
> +               oidcpy(&merge_base, &options.onto->object.oid);
> +       } else {
> +               fill_merge_base(&options, &merge_base);
> +       }
>         if (options.fork_point > 0)
>                 options.restrict_revision =
>                         get_fork_point(options.upstream_name, options.orig_head);
> @@ -1697,13 +1711,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>          * Check if we are already based on onto with linear history,
>          * in which case we could fast-forward without replacing the commits
>          * with new commits recreated by replaying their changes.
> -        *
> -        * Note that can_fast_forward() initializes merge_base, so we have to
> -        * call it before checking allow_preemptive_ff.
>          */
> -       if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
> -                   options.orig_head, &merge_base) &&
> -           allow_preemptive_ff) {
> +       if (allow_preemptive_ff &&
> +           can_fast_forward(options.onto, options.upstream, options.restrict_revision,
> +                            options.orig_head, &merge_base)) {

I didn't catch anything new in my review of this patch, but I just
really wanted to say how happy this final hunk makes me.  I hated that
can_fast_forward() had to be called first; thanks for fixing that.


>                 int flag;
>
>                 if (!(options.flags & REBASE_FORCE)) {
> --
> gitgitgadget

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Jonathan Tan]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			die(_("Does not point to a valid commit '%s'"),
>  				options.onto_name);
>  	}
> -
> +	if (keep_base) {
> +		oidcpy(&merge_base, &options.onto->object.oid);
> +	} else {
> +		fill_merge_base(&options, &merge_base);
> +	}
>  	if (options.fork_point > 0)
>  		options.restrict_revision =
>  			get_fork_point(options.upstream_name, options.orig_head);

This patch makes sense, except for this part: why is fill_merge_base()
only called for non-keep_base, but it seemed to be unconditionally
called before? For what it's worth, all tests pass even with this diff:

  -       if (keep_base) {
  -               oidcpy(&merge_base, &options.onto->object.oid);
  -       } else {
  -               fill_merge_base(&options, &merge_base);
  -       }
  +       fill_merge_base(&options, &merge_base);

Re: [PATCH 3/5] rebase: factor out merge_base calculation

[Phillip Wood]

Hi Jonathan

Thanks for taking a look at this series

On 24/08/2022 23:02, Jonathan Tan wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -1668,7 +1678,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   			die(_("Does not point to a valid commit '%s'"),
>>   				options.onto_name);
>>   	}
>> -
>> +	if (keep_base) {
>> +		oidcpy(&merge_base, &options.onto->object.oid);
>> +	} else {
>> +		fill_merge_base(&options, &merge_base);
>> +	}
>>   	if (options.fork_point > 0)
>>   		options.restrict_revision =
>>   			get_fork_point(options.upstream_name, options.orig_head);
> 
> This patch makes sense, except for this part: why is fill_merge_base()
> only called for non-keep_base, but it seemed to be unconditionally
> called before? For what it's worth, all tests pass even with this diff:

It's an optimization, we have already calculated the merge base above in 
the "onto" calculation when --keep-base is given. This is what I meant 
by "avoid calculating the merge-base twice when --keep-base is given" in 
the commit message, maybe I should  try and come up with some clearer 
wording.

Best Wishes

Phillip


>    -       if (keep_base) {
>    -               oidcpy(&merge_base, &options.onto->object.oid);
>    -       } else {
>    -               fill_merge_base(&options, &merge_base);
>    -       }
>    +       fill_merge_base(&options, &merge_base);

[PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

[Phillip Wood via GitGitGadget]

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

Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

[Junio C Hamano]

"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.

Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

[Jonathan Tan]

"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?

Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

[Phillip Wood]

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

Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

[Philippe Blain]

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.

Re: [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks

[Phillip Wood]

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

[PATCH 5/5] rebase --keep-base: imply --no-fork-point

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Given the name of the option it is confusing if --keep-base actually
changes the base of the branch without --fork-point being explicitly
given on the command line.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-rebase.txt | 2 +-
 builtin/rebase.c             | 6 ++++++
 t/t3431-rebase-fork-point.sh | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index dc0c6c54e27..6d62e404268 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 --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
+	`git rebase --reapply-cherry-picks --no-fork-point --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 b6b3e00e3b1..1a8344b890e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1240,6 +1240,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
 		if (options.root)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
+		/*
+		 * --keep-base ignores config.forkPoint as it is confusing if
+		 * the branch base changes when using this option.
+		 */
+		if (options.fork_point < 0)
+			options.fork_point = 0;
 	}
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks as it is confusing if
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 1d0b15380ed..70e81363569 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -50,7 +50,7 @@ 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 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
-- 
gitgitgadget

Re: [PATCH 5/5] rebase --keep-base: imply --no-fork-point

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Given the name of the option it is confusing if --keep-base actually
> changes the base of the branch without --fork-point being explicitly
> given on the command line.

Does it merely "imply"?  As keep-base requests exactly the same base
commit reused from the current history, doesn't fork-point a
competing and conflicting request, i.e. "please compute an
appropriate fork-point by looking at merge base with possibly
rewound tips of upstream branch"?

> +		/*
> +		 * --keep-base ignores config.forkPoint as it is confusing if
> +		 * the branch base changes when using this option.
> +		 */

The comment singles out config.forkPoint (Isn't that "rebase.forkPoint"???)
as "confusing".  Do we ignore rebase.forkPoint when --keep-base is given?
Do we honor --fork-point from the command line when --keep-base is given?

> +		if (options.fork_point < 0)
> +			options.fork_point = 0;

Re: [PATCH 5/5] rebase --keep-base: imply --no-fork-point

[Jonathan Tan]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -1240,6 +1240,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
>  		if (options.root)
>  			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
> +		/*
> +		 * --keep-base ignores config.forkPoint as it is confusing if
> +		 * the branch base changes when using this option.
> +		 */
> +		if (options.fork_point < 0)
> +			options.fork_point = 0;

Hmm..doesn't forkPoint decide the set of commits to be rebased, not the
point at which the new commits are created? If yes, that doesn't seem to
have much to do with --keep-base.

Re: [PATCH 5/5] rebase --keep-base: imply --no-fork-point

[Phillip Wood]

Hi Jonathan

On 24/08/2022 23:18, Jonathan Tan wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> @@ -1240,6 +1240,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
>>   		if (options.root)
>>   			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
>> +		/*
>> +		 * --keep-base ignores config.forkPoint as it is confusing if
>> +		 * the branch base changes when using this option.
>> +		 */
>> +		if (options.fork_point < 0)
>> +			options.fork_point = 0;
> 
> Hmm..doesn't forkPoint decide the set of commits to be rebased, not the
> point at which the new commits are created? If yes, that doesn't seem to
> have much to do with --keep-base.

I think it depends a bit on how you look at it. You're right that 
--fork-point restricts the set of commits that are rebased, but it also 
effectively changes the branch base as it removes commits from the branch.

Best Wishes

Phillip

Re: [PATCH 0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[Johannes Schindelin]

Hi Phillip,

On Mon, 15 Aug 2022, Phillip Wood via GitGitGadget wrote:

> A while a go Philippe reported [1] that he was surprised 'git rebase
> --keep-base' removed commits that had been cherry-picked upstream even
> though to branch was not being rebased.

This has bitten me, too (I did just not get around to reply to Philippe's
mail).

> I think it is also surprising if '--keep-base' changes the base of the
> branch without '--fork-point' being explicitly given on the command
> line. This series therefore changes the default behavior of
> '--keep-base' to imply '--reapply-cherry-picks' and '--no-fork-point' so
> that the base of the branch is unchanged and no commits are removed.

In my mind, `--keep-base` should always have adjusted the `<upstream>` to
point to the base commit, which is what patch 4/5 does.

So I am very much in favor of this patch series.

Junio seems to be on top of the code review, so I'll avoid stepping on his
toes by adding mine ;-)

Ciao,
Dscho

Re: [PATCH 0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[Jonathan Tan]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> A while a go Philippe reported [1] that he was surprised 'git rebase
> --keep-base' removed commits that had been cherry-picked upstream even
> though to branch was not being rebased.
[...]
> [1]
> https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

In the thread at [1] I said that (in [2]) I expected that
--reapply-cherry-picks would be the default, but this implicit
--no-reapply-cherry-picks (in order to automatically exclude certain
commits) is a long-available and long-advertised feature. So I
symphatize with making certain modes of rebase automatically imply
--reapply-cherry-picks, but I think that either all modes (as much as
possible) should imply it, or no mode should imply it.

Having said that, I already looked at the code so I have included my
review comments anyway (in case we do decide to have implied
--reapply-cherry-picks for --keep-base but not for other cases).

As for the --fork-point part, I made a comment in my reply to patch 5.

[2] https://lore.kernel.org/git/20200714035104.1465772-1-jonathantanmy@google.com/

[PATCH v2 0/7] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[Phillip Wood via GitGitGadget]

A while a go Philippe reported [1] that he was surprised 'git rebase
--keep-base' removed commits that had been cherry-picked upstream even
though to branch was not being rebased. I think it is also surprising if
'--keep-base' changes the base of the branch without '--fork-point' being
explicitly given on the command line. This series therefore changes the
default behavior of '--keep-base' to imply '--reapply-cherry-picks' and
'--no-fork-point' so that the base of the branch is unchanged and no commits
are removed.

Thanks to everyone who commented for their reviews, the changes since V1
are:

 * Patch 1: new patch to tighten a couple of existing tests
 * Patch 2: reworded commit message in response to Junio's comments
 * Patch 3: fixed a typo in the commit message spotted by Elijah and tidied
   code formatting
 * Patch 4: new patch to rename a variable suggested by Junio
 * Patch 5: clarified commit message and removed some redundant code spotted
   by Junio
 * Patch 6: improved --reapply-cherry-picks documentation to mention
   --keep-base and vice-versa suggested by Philippe
 * Patch 7: expanded the commit message and documentation in response to
   Junio's comments

[1]
https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Phillip Wood (7):
  t3416: tighten two tests
  t3416: set $EDITOR in subshell
  rebase: store orig_head as a commit
  rebase: rename merge_base to branch_base
  rebase: factor out branch_base calculation
  rebase --keep-base: imply --reapply-cherry-picks
  rebase --keep-base: imply --no-fork-point

 Documentation/git-rebase.txt     |  32 ++++----
 builtin/rebase.c                 | 129 ++++++++++++++++++-------------
 t/t3416-rebase-onto-threedots.sh |  62 +++++++++++----
 t/t3431-rebase-fork-point.sh     |   2 +-
 4 files changed, 142 insertions(+), 83 deletions(-)


base-commit: afa70145a25e81faa685dc0b465e52b45d2444bd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1323%2Fphillipwood%2Fwip%2Frebase--keep-base-tweaks-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1323/phillipwood/wip/rebase--keep-base-tweaks-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1323

Range-diff vs v1:

 -:  ----------- > 1:  12fb0ac6d5d t3416: tighten two tests
 1:  c1d91a2b190 ! 2:  d6f2f716c77 t3416: set $EDITOR in subshell
     @@ Metadata
       ## Commit message ##
          t3416: set $EDITOR in subshell
      
     -    As $EDITOR is exported setting it in one test affects all subsequent
     -    tests. Avoid this by always setting it in a subshell and remove a
     -    couple of unnecessary call to set_fake_editor.
     +    As $EDITOR is exported, setting it in one test affects all subsequent
     +    tests. Avoid this by always setting it in a subshell. Also remove a
     +    couple of unnecessary call to set_fake_editor where the editor does
     +    not change the todo list.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase -i --onto main...'
       	git rev-parse HEAD^1 >actual &&
       	git rev-parse C^0 >expect &&
       	test_cmp expect actual
     -@@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase -i --onto main...side' '
     +@@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase --onto main...side requires a single merge-base' '
       	git checkout side &&
       	git reset --hard K &&
       
      -	set_fake_editor &&
     - 	test_must_fail git rebase -i --onto main...side J
     + 	test_must_fail git rebase -i --onto main...side J 2>err &&
     + 	grep "need exactly one merge base" err
       '
     - 
      @@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase -i --keep-base main from topic' '
       	git checkout topic &&
       	git reset --hard G &&
     @@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase -i --keep-base mai
       	git rev-parse C >base.expect &&
       	git merge-base main HEAD >base.actual &&
       	test_cmp base.expect base.actual &&
     -@@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase -i --keep-base main from side' '
     +@@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase --keep-base requires a single merge base' '
       	git checkout side &&
       	git reset --hard K &&
       
      -	set_fake_editor &&
     - 	test_must_fail git rebase -i --keep-base main
     + 	test_must_fail git rebase -i --keep-base main 2>err &&
     + 	grep "need exactly one merge base with branch" err
       '
       
      +# This must be the last test in this file
 2:  cced4a48360 ! 3:  9daee95d434 rebase: store orig_head as a commit
     @@ Commit message
          rebase: store orig_head as a commit
      
          Using a struct commit rather than a struct oid to hold orig_head means
     -    that we error out straight away if branch being rebased does not point
     -    to a commit. It also simplifies the code than handles finding the
     -    merge base and fork point as it not longer has to convert from an oid
     -    to a commit.
     +    that we error out straight away if the branch being rebased does not
     +    point to a commit. It also simplifies the code that handles finding
     +    the merge base and fork point as it no longer has to convert from an
     +    oid to a commit.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
      -			get_fork_point(options.upstream_name, head);
      -	}
      +			get_fork_point(options.upstream_name, options.orig_head);
     -+
       
       	if (repo_read_index(the_repository) < 0)
       		die(_("could not read index"));
 -:  ----------- > 4:  cca933a5f1d rebase: rename merge_base to branch_base
 3:  019158db9d2 ! 5:  fc45b996d34 rebase: factor out merge_base calculation
     @@ Metadata
      Author: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## Commit message ##
     -    rebase: factor out merge_base calculation
     +    rebase: factor out branch_base calculation
      
     -    Separate out calculating the merge base between onto and head from the
     -    check for whether we can fast-forward or not. This means we can skip
     -    the fast-forward checks when the rebase is forced and avoid
     -    calculating the merge-base twice when --keep-base is given.
     +    Separate out calculating the merge base between 'onto' and 'HEAD' from
     +    the check for whether we can fast-forward or not. This means we can skip
     +    the fast-forward checks when the rebase is forced and avoid calculating
     +    the merge-base between 'HEAD' and 'onto' when --keep-base is given.
      
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     -    ---
     -    Note the unnecessary braces around "if (keep_base)" are added here
     -    reduce the churn on the next commit.
      
       ## builtin/rebase.c ##
      @@ builtin/rebase.c: static int can_fast_forward(struct commit *onto, struct commit *upstream,
     @@ builtin/rebase.c: static int can_fast_forward(struct commit *onto, struct commit
       
      -	merge_bases = get_merge_bases(onto, head);
      -	if (!merge_bases || merge_bases->next) {
     --		oidcpy(merge_base, null_oid());
     -+	if (is_null_oid(merge_base))
     +-		oidcpy(branch_base, null_oid());
     ++	if (is_null_oid(branch_base))
       		goto done;
      -	}
       
     --	oidcpy(merge_base, &merge_bases->item->object.oid);
     - 	if (!oideq(merge_base, &onto->object.oid))
     +-	oidcpy(branch_base, &merge_bases->item->object.oid);
     + 	if (!oideq(branch_base, &onto->object.oid))
       		goto done;
       
     +@@ builtin/rebase.c: static int can_fast_forward(struct commit *onto, struct commit *upstream,
     + 	if (!upstream)
     + 		goto done;
     + 
     +-	free_commit_list(merge_bases);
     + 	merge_bases = get_merge_bases(upstream, head);
     + 	if (!merge_bases || merge_bases->next)
     + 		goto done;
      @@ builtin/rebase.c: done:
       	return res && is_linear_history(onto, head);
       }
       
     -+static void fill_merge_base(struct rebase_options *options,
     -+			    struct object_id *merge_base)
     ++static void fill_branch_base(struct rebase_options *options,
     ++			    struct object_id *branch_base)
      +{
      +	struct commit_list *merge_bases = NULL;
      +
      +	merge_bases = get_merge_bases(options->onto, options->orig_head);
      +	if (!merge_bases || merge_bases->next)
     -+		oidcpy(merge_base, null_oid());
     ++		oidcpy(branch_base, null_oid());
      +	else
     -+		oidcpy(merge_base, &merge_bases->item->object.oid);
     ++		oidcpy(branch_base, &merge_bases->item->object.oid);
      +
      +	free_commit_list(merge_bases);
      +}
     @@ builtin/rebase.c: done:
       {
       	struct rebase_options *opts = opt->value;
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     + 		if (!options.onto)
       			die(_("Does not point to a valid commit '%s'"),
       				options.onto_name);
     ++		fill_branch_base(&options, &branch_base);
       	}
      -
     -+	if (keep_base) {
     -+		oidcpy(&merge_base, &options.onto->object.oid);
     -+	} else {
     -+		fill_merge_base(&options, &merge_base);
     -+	}
       	if (options.fork_point > 0)
       		options.restrict_revision =
       			get_fork_point(options.upstream_name, options.orig_head);
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       	 * in which case we could fast-forward without replacing the commits
       	 * with new commits recreated by replaying their changes.
      -	 *
     --	 * Note that can_fast_forward() initializes merge_base, so we have to
     +-	 * Note that can_fast_forward() initializes branch_base, so we have to
      -	 * call it before checking allow_preemptive_ff.
       	 */
      -	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
     --		    options.orig_head, &merge_base) &&
     +-		    options.orig_head, &branch_base) &&
      -	    allow_preemptive_ff) {
      +	if (allow_preemptive_ff &&
      +	    can_fast_forward(options.onto, options.upstream, options.restrict_revision,
     -+			     options.orig_head, &merge_base)) {
     ++			     options.orig_head, &branch_base)) {
       		int flag;
       
       		if (!(options.flags & REBASE_FORCE)) {
 4:  9cd4c372ee4 ! 6:  faad7eaf0d6 rebase --keep-base: imply --reapply-cherry-picks
     @@ Documentation/git-rebase.txt: leave out at most one of A and B, in which case it
       +
       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.
     ++rebasing on top of the upstream but to keep the base commit as-is. As
     ++the base commit is unchanged this option implies `--reapply-cherry-picks`
     ++to avoid losing commits.
     + +
     + Although both this option and `--fork-point` find the merge base between
     + `<upstream>` and `<branch>`, this option uses the merge base as the _starting
     +@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
     + Note that commits which start empty are kept (unless `--no-keep-empty`
     + is specified), and commits which are clean cherry-picks (as determined
     + by `git log --cherry-mark ...`) are detected and dropped as a
     +-preliminary step (unless `--reapply-cherry-picks` is passed).
     ++preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
     ++passed).
     + +
     + See also INCOMPATIBLE OPTIONS below.
     + 
     +@@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
     + 	upstream changes, the behavior towards them is controlled by
     + 	the `--empty` flag.)
     + +
     +-By default (or if `--no-reapply-cherry-picks` is given), these commits
     +-will be automatically dropped.  Because this necessitates reading all
     +-upstream commits, this can be expensive in repos with a large number
     +-of upstream commits that need to be read.  When using the 'merge'
     +-backend, warnings will be issued for each dropped commit (unless
     +-`--quiet` is given). Advice will also be issued unless
     +-`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
     ++
     ++In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
     ++given), these commits will be automatically dropped.  Because this
     ++necessitates reading all upstream commits, this can be expensive in
     ++repositories with a large number of upstream commits that need to be
     ++read. When using the 'merge' backend, warnings will be issued for each
     ++dropped commit (unless `--quiet` is given). Advice will also be issued
     ++unless `advice.skippedCherryPicks` is set to false (see
     ++linkgit:git-config[1]).
     ++
     + +
     + `--reapply-cherry-picks` allows rebase to forgo reading all upstream
     + commits, potentially improving performance.
      
       ## builtin/rebase.c ##
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       			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.
     ++	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
     ++	 * commits when using this option.
      +	 */
      +	if (options.reapply_cherry_picks < 0)
      +		options.reapply_cherry_picks = keep_base;
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       
       	if (gpg_sign)
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     + 				options.onto_name);
     + 		fill_branch_base(&options, &branch_base);
       	}
     - 	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);
     - 	}
     ++	if (keep_base && options.reapply_cherry_picks)
     ++		options.upstream = options.onto;
     ++
     + 	if (options.fork_point > 0)
     + 		options.restrict_revision =
     + 			get_fork_point(options.upstream_name, options.orig_head);
      
       ## t/t3416-rebase-onto-threedots.sh ##
     -@@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase -i --keep-base main from side' '
     - 	test_must_fail git rebase -i --keep-base main
     +@@ t/t3416-rebase-onto-threedots.sh: test_expect_success 'rebase --keep-base requires a single merge base' '
     + 	grep "need exactly one merge base with branch" err
       '
       
      +test_expect_success 'rebase --keep-base keeps cherry picks' '
 5:  68bcd10949e ! 7:  6410b101d7f rebase --keep-base: imply --no-fork-point
     @@ Commit message
          changes the base of the branch without --fork-point being explicitly
          given on the command line.
      
     +    The combination of --keep-base with an explicit --fork-point is still
     +    supported even though --fork-point means we do not keep the same base
     +    if the upstream branch has been rewound.  We do this in case anyone is
     +    relying on this behavior which is tested in t3431[1]
     +
     +    [1] https://lore.kernel.org/git/20200715032014.GA10818@generichostname/
     +
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## Documentation/git-rebase.txt ##
     @@ Documentation/git-rebase.txt: leave out at most one of A and B, in which case it
       +
       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
     +@@ Documentation/git-rebase.txt: When `--fork-point` is active, 'fork_point' will be used instead of
     + <branch>` command (see linkgit:git-merge-base[1]).  If 'fork_point'
     + ends up being empty, the `<upstream>` will be used as a fallback.
     + +
     +-If `<upstream>` is given on the command line, then the default is
     +-`--no-fork-point`, otherwise the default is `--fork-point`. See also
     +-`rebase.forkpoint` in linkgit:git-config[1].
     ++If `<upstream>` or `--keep-base` is given on the command line, then
     ++the default is `--no-fork-point`, otherwise the default is
     ++`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
     + +
     + If your branch was based on `<upstream>` but `<upstream>` was rewound and
     + your branch contains commits which were dropped, this option can be used
      
       ## builtin/rebase.c ##
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     @@ builtin/rebase.c: 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 ignores config.forkPoint as it is confusing if
     -+		 * the branch base changes when using this option.
     ++		 * --keep-base defaults to --no-fork-point to keep the
     ++		 * base the same.
      +		 */
      +		if (options.fork_point < 0)
      +			options.fork_point = 0;
       	}
       	/*
     - 	 * --keep-base defaults to --reapply-cherry-picks as it is confusing if
     + 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
      
       ## t/t3431-rebase-fork-point.sh ##
      @@ t/t3431-rebase-fork-point.sh: test_rebase () {

-- 
gitgitgadget

[PATCH v2 1/7] t3416: tighten two tests

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a check for the correct error message to the tests that check we
require a single merge base so we can be sure the rebase failed for
the correct reason. Also rename the tests to reflect what they are
testing.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3e04802cb00..dc67d2308f3 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -97,13 +97,14 @@ test_expect_success 'rebase -i --onto main...' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --onto main...side' '
+test_expect_success 'rebase --onto main...side requires a single merge-base' '
 	git reset --hard &&
 	git checkout side &&
 	git reset --hard K &&
 
 	set_fake_editor &&
-	test_must_fail git rebase -i --onto main...side J
+	test_must_fail git rebase -i --onto main...side J 2>err &&
+	grep "need exactly one merge base" err
 '
 
 test_expect_success 'rebase --keep-base --onto incompatible' '
@@ -182,13 +183,14 @@ test_expect_success 'rebase -i --keep-base main topic from main' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --keep-base main from side' '
+test_expect_success 'rebase --keep-base requires a single merge base' '
 	git reset --hard &&
 	git checkout side &&
 	git reset --hard K &&
 
 	set_fake_editor &&
-	test_must_fail git rebase -i --keep-base main
+	test_must_fail git rebase -i --keep-base main 2>err &&
+	grep "need exactly one merge base with branch" err
 '
 
 test_done
-- 
gitgitgadget

Re: [PATCH v2 1/7] t3416: tighten two tests

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Add a check for the correct error message to the tests that check we
> require a single merge base so we can be sure the rebase failed for
> the correct reason. Also rename the tests to reflect what they are
> testing.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---

It is very sensible to ensure that a test that is expected to fail
does fail for the right reason, not by accident.  Looks good.

>  t/t3416-rebase-onto-threedots.sh | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index 3e04802cb00..dc67d2308f3 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -97,13 +97,14 @@ test_expect_success 'rebase -i --onto main...' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success 'rebase -i --onto main...side' '
> +test_expect_success 'rebase --onto main...side requires a single merge-base' '
>  	git reset --hard &&
>  	git checkout side &&
>  	git reset --hard K &&
>  
>  	set_fake_editor &&
> -	test_must_fail git rebase -i --onto main...side J
> +	test_must_fail git rebase -i --onto main...side J 2>err &&
> +	grep "need exactly one merge base" err
>  '
>  
>  test_expect_success 'rebase --keep-base --onto incompatible' '
> @@ -182,13 +183,14 @@ test_expect_success 'rebase -i --keep-base main topic from main' '
>  	test_cmp expect actual
>  '
>  
> -test_expect_success 'rebase -i --keep-base main from side' '
> +test_expect_success 'rebase --keep-base requires a single merge base' '
>  	git reset --hard &&
>  	git checkout side &&
>  	git reset --hard K &&
>  
>  	set_fake_editor &&
> -	test_must_fail git rebase -i --keep-base main
> +	test_must_fail git rebase -i --keep-base main 2>err &&
> +	grep "need exactly one merge base with branch" err
>  '
>  
>  test_done

[PATCH v2 2/7] t3416: set $EDITOR in subshell

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As $EDITOR is exported, setting it in one test affects all subsequent
tests. Avoid this by always setting it in a subshell. Also remove a
couple of unnecessary call to set_fake_editor where the editor does
not change the todo list.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index dc67d2308f3..01eb9513d6c 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -79,8 +79,10 @@ test_expect_success 'rebase -i --onto main...topic' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main...topic F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main...topic F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -90,8 +92,10 @@ test_expect_success 'rebase -i --onto main...' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main... F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main... F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -102,7 +106,6 @@ test_expect_success 'rebase --onto main...side requires a single merge-base' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --onto main...side J 2>err &&
 	grep "need exactly one merge base" err
 '
@@ -157,8 +160,10 @@ test_expect_success 'rebase -i --keep-base main from topic' '
 	git checkout topic &&
 	git reset --hard G &&
 
-	set_fake_editor &&
-	EXPECT_COUNT=2 git rebase -i --keep-base main &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=2 git rebase -i --keep-base main
+	) &&
 	git rev-parse C >base.expect &&
 	git merge-base main HEAD >base.actual &&
 	test_cmp base.expect base.actual &&
@@ -172,8 +177,10 @@ 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 &&
+	(
+		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 &&
@@ -188,9 +195,13 @@ test_expect_success 'rebase --keep-base requires a single merge base' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --keep-base main 2>err &&
 	grep "need exactly one merge base with branch" err
 '
 
+# This must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 2/7] t3416: set $EDITOR in subshell

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As $EDITOR is exported, setting it in one test affects all subsequent
> tests. Avoid this by always setting it in a subshell. Also remove a
> couple of unnecessary call to set_fake_editor where the editor does
> not change the todo list.

True.

The fact that we need to do this makes me wonder if we want a
restore_editor (or a hardcoded "export EDITOR=:") that allows us not
to go to a subshell, as going to a subshell has its downsides (e.g.
we cannot use helpers that rely on being able to modify globals,
like test_commit and test_when_finished), but for now this looks OK.

Thanks.

[PATCH v2 3/7] rebase: store orig_head as a commit

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Using a struct commit rather than a struct oid to hold orig_head means
that we error out straight away if the branch being rebased does not
point to a commit. It also simplifies the code that handles finding
the merge base and fork point as it no longer has to convert from an
oid to a commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 61 +++++++++++++++++++++---------------------------
 1 file changed, 27 insertions(+), 34 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..a3cf1ef5923 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -68,7 +68,7 @@ struct rebase_options {
 	const char *upstream_name;
 	const char *upstream_arg;
 	char *head_name;
-	struct object_id orig_head;
+	struct commit *orig_head;
 	struct commit *onto;
 	const char *onto_name;
 	const char *revisions;
@@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	struct replay_opts replay = get_replay_opts(opts);
 	struct string_list commands = STRING_LIST_INIT_DUP;
 
-	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
+	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
 		return -1;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head)) {
+			     opts->onto, &opts->orig_head->object.oid)) {
 		free(revisions);
 		free(shortrevisions);
 
@@ -298,9 +298,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		split_exec_commands(opts->cmd, &commands);
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
-			&opts->orig_head, &commands, opts->autosquash,
-			opts->update_refs,
-			&todo_list);
+			&opts->orig_head->object.oid, &commands,
+			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
 	string_list_clear(&commands, 0);
@@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid(buf.buf, &opts->orig_head))
+	opts->orig_head = lookup_commit_reference_by_name(buf.buf);
+	if (!opts->orig_head)
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
@@ -517,7 +517,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	write_file(state_dir_path("onto", opts), "%s",
 		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
 	write_file(state_dir_path("orig-head", opts), "%s",
-		   oid_to_hex(&opts->orig_head));
+		   oid_to_hex(&opts->orig_head->object.oid));
 	if (!(opts->flags & REBASE_NO_QUIET))
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
@@ -646,7 +646,7 @@ static int run_am(struct rebase_options *opts)
 			       /* this is now equivalent to !opts->upstream */
 			       &opts->onto->object.oid :
 			       &opts->upstream->object.oid),
-		    oid_to_hex(&opts->orig_head));
+		    oid_to_hex(&opts->orig_head->object.oid));
 
 	rebased_patches = xstrdup(git_path("rebased-patches"));
 	format_patch.out = open(rebased_patches,
@@ -680,7 +680,7 @@ static int run_am(struct rebase_options *opts)
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		ropts.oid = &opts->orig_head;
+		ropts.oid = &opts->orig_head->object.oid;
 		ropts.branch = opts->head_name;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		reset_head(the_repository, &ropts);
@@ -833,7 +833,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	ropts.oid = &options->orig_head;
+	ropts.oid = &options->orig_head->object.oid;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	if (!ropts.branch)
@@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct object_id *head_oid, struct object_id *merge_base)
+			    struct commit *head, 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());
@@ -1312,13 +1308,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		ropts.oid = &options.orig_head;
+		ropts.oid = &options.orig_head->object.oid;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
-			    oid_to_hex(&options.orig_head));
+			    oid_to_hex(&options.orig_head->object.oid));
 		remove_branch_state(the_repository, 0);
 		ret = finish_rebase(&options);
 		goto cleanup;
@@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		/* Is it a local branch? */
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "refs/heads/%s", branch_name);
-		if (!read_ref(buf.buf, &options.orig_head)) {
+		options.orig_head = lookup_commit_reference_by_name(buf.buf);
+		if (options.orig_head) {
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
 		/* If not is it a valid ref (branch or commit)? */
 		} else {
-			struct commit *commit =
+			options.orig_head =
 				lookup_commit_reference_by_name(branch_name);
-			if (!commit)
+			if (!options.orig_head)
 				die(_("no such branch/commit '%s'"),
 				    branch_name);
-			oidcpy(&options.orig_head, &commit->object.oid);
 			options.head_name = NULL;
 		}
 	} else if (argc == 0) {
@@ -1639,8 +1635,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			FREE_AND_NULL(options.head_name);
 			branch_name = "HEAD";
 		}
-		if (get_oid("HEAD", &options.orig_head))
-			die(_("Could not resolve HEAD to a revision"));
+		options.orig_head = lookup_commit_reference_by_name("HEAD");
+		if (!options.orig_head)
+			die(_("Could not resolve HEAD to a commit"));
 	} else
 		BUG("unexpected number of arguments left to parse");
 
@@ -1672,13 +1669,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				options.onto_name);
 	}
 
-	if (options.fork_point > 0) {
-		struct commit *head =
-			lookup_commit_reference(the_repository,
-						&options.orig_head);
+	if (options.fork_point > 0)
 		options.restrict_revision =
-			get_fork_point(options.upstream_name, head);
-	}
+			get_fork_point(options.upstream_name, options.orig_head);
 
 	if (repo_read_index(the_repository) < 0)
 		die(_("could not read index"));
@@ -1708,7 +1701,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    &options.orig_head, &merge_base) &&
+		    options.orig_head, &merge_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1785,7 +1778,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
-	ropts.orig_head = &options.orig_head,
+	ropts.orig_head = &options.orig_head->object.oid,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;
@@ -1799,7 +1792,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head)) {
+	if (oideq(&merge_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
@@ -1820,7 +1813,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		    (options.restrict_revision ?
 		     oid_to_hex(&options.restrict_revision->object.oid) :
 		     oid_to_hex(&options.upstream->object.oid)),
-		    oid_to_hex(&options.orig_head));
+		    oid_to_hex(&options.orig_head->object.oid));
 
 	options.revisions = revisions.buf;
 
-- 
gitgitgadget

Re: [PATCH v2 3/7] rebase: store orig_head as a commit

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 56e4214b441..a3cf1ef5923 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -68,7 +68,7 @@ struct rebase_options {
>  	const char *upstream_name;
>  	const char *upstream_arg;
>  	char *head_name;
> -	struct object_id orig_head;
> +	struct commit *orig_head;

OK.

> @@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>  	struct replay_opts replay = get_replay_opts(opts);
>  	struct string_list commands = STRING_LIST_INIT_DUP;
>  
> -	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
> +	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
>  				&revisions, &shortrevisions))
>  		return -1;

This step has ton of changes like this, which are fallouts from the
change of type of a member that used to be an oid to a full blown
commit object.  It is expected and I will strip all of them from my
quotes.  They all look correct, and otherwise the compiler would
complain ;-).

> @@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
>  	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
>  				  READ_ONELINER_WARN_MISSING))
>  		return -1;
> -	if (get_oid(buf.buf, &opts->orig_head))
> +	opts->orig_head = lookup_commit_reference_by_name(buf.buf);

This is not exactly a new problem, but I noticed it while looking
for more iffy uses of lookup_commit_reference_by_name(), so...

At this point in the codepath, we expect buf.buf has full-hex object
name and nothing else; the original should have used get_oid_hex()
to highlight that fact.  lookup_commit_reference_by_name() allows
object names that are not written as full-hex object name, and it
may get confused if a branch or tag with 40-hex (or 64-hex in a
repository with newhash) name exists.  It would be a more sensible
conversion to use get_oid_hex() to turn buf.buf into an object name
and then use lookup_commit_reference() on it.

> @@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
>  
>  static int can_fast_forward(struct commit *onto, struct commit *upstream,
>  			    struct commit *restrict_revision,
> -			    struct object_id *head_oid, struct object_id *merge_base)
> +			    struct commit *head, 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;
> -

This one benefits from being able to avoid its own lookup_commit()
because the caller already has it in the desired form.

This is not a comment on the new code, but it does make readers
wonder if the conversion changes behaviour.  lookup_commit() takes
an object name and requires it to be a commit object's name, doesn't
it?  If we gave a tag to the program, the old code would have had
the object name of that tag in head_oid and at this point and
lookup_commit() noticed and would have stopped you from fast
forwarding your branch to the tag, which was a good thing.  In the
new code, since we turn the object name we take from the user into a
commit object way before the control reaches this place, we won't
get such an error here, but if we fast-forward to the object, we
will still fast forward to the commit that is pointed by the tag,
so the new behaviour is even better, perhaps?

> @@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		/* Is it a local branch? */
>  		strbuf_reset(&buf);
>  		strbuf_addf(&buf, "refs/heads/%s", branch_name);
> -		if (!read_ref(buf.buf, &options.orig_head)) {
> +		options.orig_head = lookup_commit_reference_by_name(buf.buf);
> +		if (options.orig_head) {
>  			die_if_checked_out(buf.buf, 1);
>  			options.head_name = xstrdup(buf.buf);
>  		/* If not is it a valid ref (branch or commit)? */

This is iffy, or it may be just wrong.

The old code is checking if "refs/heads/$branch_name" is a branch
and does the check.  If you had a branch "refs/heads/A" (whose ref
is at "refs/heads/refs/heads/A") but do not have a branch "A", and
if you fed "A" to this part of the code in buf.buf, then the
original code would not have been fooled by the presence of such a
funny branch.  New code (incorrectly) does because it prefixes
"refs/heads/" to "A" and asks to turn string "refs/heads/A" into a
commit object, triggering the usual ref dwim rules.

We end up setting options.head_name to a wrong thing (in this case,
the user said "A", we turned it into a refname "refs/heads/A" that
does not exist, and set options.orig_head to the commit object
pointed by the ref "refs/heads/refs/heads/A", and we use that commit
as orig_head, but use an incorrect head_name).

I didn't look as carefully as this one, but there may be similarly
iffy uses of lookup_commit_reference_by_name() introduced by this
patch that used to be more strict/exact; they may need to be fixed.

>  		} else {
> -			struct commit *commit =
> +			options.orig_head =
>  				lookup_commit_reference_by_name(branch_name);
> -			if (!commit)
> +			if (!options.orig_head)
>  				die(_("no such branch/commit '%s'"),
>  				    branch_name);
> -			oidcpy(&options.orig_head, &commit->object.oid);
>  			options.head_name = NULL;

This side, which is "this is what happens to a random object name
that is not a branch name", is perfectly fine.

Re: [PATCH v2 3/7] rebase: store orig_head as a commit

[Phillip Wood]

Hi Junio

Thank you for your thoughtful comments

On 07/09/2022 19:12, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 56e4214b441..a3cf1ef5923 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
>>   	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
>>   				  READ_ONELINER_WARN_MISSING))
>>   		return -1;
>> -	if (get_oid(buf.buf, &opts->orig_head))
>> +	opts->orig_head = lookup_commit_reference_by_name(buf.buf);
> 
> This is not exactly a new problem, but I noticed it while looking
> for more iffy uses of lookup_commit_reference_by_name(), so...
> 
> At this point in the codepath, we expect buf.buf has full-hex object
> name and nothing else; the original should have used get_oid_hex()
> to highlight that fact.  lookup_commit_reference_by_name() allows
> object names that are not written as full-hex object name, and it
> may get confused if a branch or tag with 40-hex (or 64-hex in a
> repository with newhash) name exists.  It would be a more sensible
> conversion to use get_oid_hex() to turn buf.buf into an object name
> and then use lookup_commit_reference() on it.

That's a good point, we expect orig_head to be a commit but I don't 
think we have a function that takes an oid and parses it as a commit 
(lookup_commit() just looks in the commit in the repository's parsed 
object hash and returns a newly allocated object if the object is not 
there). lookup_commit_reference() de-references a tags which we don't 
really want to do here if we're being strict but I'm not sure there is 
an easy way to avoid that.

We should probably be stricter when reading 'onto' as well which is also 
using get_oid() rather than get_oid_hex().

>> @@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
>>   
>>   static int can_fast_forward(struct commit *onto, struct commit *upstream,
>>   			    struct commit *restrict_revision,
>> -			    struct object_id *head_oid, struct object_id *merge_base)
>> +			    struct commit *head, 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;
>> -
> 
> This one benefits from being able to avoid its own lookup_commit()
> because the caller already has it in the desired form.
> 
> This is not a comment on the new code, but it does make readers
> wonder if the conversion changes behaviour.  lookup_commit() takes
> an object name and requires it to be a commit object's name, doesn't
> it?  If we gave a tag to the program, the old code would have had
> the object name of that tag in head_oid and at this point and
> lookup_commit() noticed and would have stopped you from fast
> forwarding your branch to the tag, which was a good thing.  In the
> new code, since we turn the object name we take from the user into a
> commit object way before the control reaches this place, we won't
> get such an error here, but if we fast-forward to the object, we
> will still fast forward to the commit that is pointed by the tag,
> so the new behaviour is even better, perhaps?

I don't think head_oid can point to a tag in the original as we will 
have done

	commit = lookup_commit_reference_by_name(branch)
	oidcpy(&options.orig_head, &commit->object.oid)

when parsing the branch name given on the command line. If the user does 
not give a branch name then we use HEAD which should not be a tag.

>> @@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		/* Is it a local branch? */
>>   		strbuf_reset(&buf);
>>   		strbuf_addf(&buf, "refs/heads/%s", branch_name);
>> -		if (!read_ref(buf.buf, &options.orig_head)) {
>> +		options.orig_head = lookup_commit_reference_by_name(buf.buf);
>> +		if (options.orig_head) {
>>   			die_if_checked_out(buf.buf, 1);
>>   			options.head_name = xstrdup(buf.buf);
>>   		/* If not is it a valid ref (branch or commit)? */
> 
> This is iffy, or it may be just wrong.

It's wrong, thanks for pointing that out.

> The old code is checking if "refs/heads/$branch_name" is a branch
> and does the check.  If you had a branch "refs/heads/A" (whose ref
> is at "refs/heads/refs/heads/A") but do not have a branch "A", and
> if you fed "A" to this part of the code in buf.buf, then the
> original code would not have been fooled by the presence of such a
> funny branch.  New code (incorrectly) does because it prefixes
> "refs/heads/" to "A" and asks to turn string "refs/heads/A" into a
> commit object, triggering the usual ref dwim rules.
> 
> We end up setting options.head_name to a wrong thing (in this case,
> the user said "A", we turned it into a refname "refs/heads/A" that
> does not exist, and set options.orig_head to the commit object
> pointed by the ref "refs/heads/refs/heads/A", and we use that commit
> as orig_head, but use an incorrect head_name).
> 
> I didn't look as carefully as this one, but there may be similarly
> iffy uses of lookup_commit_reference_by_name() introduced by this
> patch that used to be more strict/exact; they may need to be fixed.

The only other use added in this patch is

-               if (get_oid("HEAD", &options.orig_head))
-                       die(_("Could not resolve HEAD to a revision"));
+               options.orig_head = lookup_commit_reference_by_name("HEAD");
+               if (!options.orig_head)
+                       die(_("Could not resolve HEAD to a commit"));

So now we will de-reference HEAD to a commit if it points to a tag but I 
don't think that can happen with 'git checkout' and we'll complain if it 
somehow points to a tree or blob.

Thanks for your comments, I'll fix and re-roll

Phillip

[PATCH v2 4/7] rebase: rename merge_base to branch_base

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

merge_base is not a very descriptive name, the variable always holds
the merge-base of 'branch' and 'onto' which is commit at the base of
the branch being rebased so rename it to branch_base.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index a3cf1ef5923..dd5e0e1feb6 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -866,22 +866,22 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct commit *head, struct object_id *merge_base)
+			    struct commit *head, struct object_id *branch_base)
 {
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
 	merge_bases = get_merge_bases(onto, head);
 	if (!merge_bases || merge_bases->next) {
-		oidcpy(merge_base, null_oid());
+		oidcpy(branch_base, null_oid());
 		goto done;
 	}
 
-	oidcpy(merge_base, &merge_bases->item->object.oid);
-	if (!oideq(merge_base, &onto->object.oid))
+	oidcpy(branch_base, &merge_bases->item->object.oid);
+	if (!oideq(branch_base, &onto->object.oid))
 		goto done;
 
-	if (restrict_revision && !oideq(&restrict_revision->object.oid, merge_base))
+	if (restrict_revision && !oideq(&restrict_revision->object.oid, branch_base))
 		goto done;
 
 	if (!upstream)
@@ -1035,7 +1035,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf msg = STRBUF_INIT;
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	struct object_id merge_base;
+	struct object_id branch_base;
 	int ignore_whitespace = 0;
 	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
@@ -1651,7 +1651,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} 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 (get_oid_mb(options.onto_name, &branch_base) < 0) {
 			if (keep_base)
 				die(_("'%s': need exactly one merge base with branch"),
 				    options.upstream_name);
@@ -1659,7 +1659,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				die(_("'%s': need exactly one merge base"),
 				    options.onto_name);
 		}
-		options.onto = lookup_commit_or_die(&merge_base,
+		options.onto = lookup_commit_or_die(&branch_base,
 						    options.onto_name);
 	} else {
 		options.onto =
@@ -1697,11 +1697,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
 	 *
-	 * Note that can_fast_forward() initializes merge_base, so we have to
+	 * Note that can_fast_forward() initializes branch_base, so we have to
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &merge_base) &&
+		    options.orig_head, &branch_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1743,12 +1743,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct diff_options opts;
 
 		if (options.flags & REBASE_VERBOSE) {
-			if (is_null_oid(&merge_base))
+			if (is_null_oid(&branch_base))
 				printf(_("Changes to %s:\n"),
 				       oid_to_hex(&options.onto->object.oid));
 			else
 				printf(_("Changes from %s to %s:\n"),
-				       oid_to_hex(&merge_base),
+				       oid_to_hex(&branch_base),
 				       oid_to_hex(&options.onto->object.oid));
 		}
 
@@ -1760,8 +1760,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 		opts.detect_rename = DIFF_DETECT_RENAME;
 		diff_setup_done(&opts);
-		diff_tree_oid(is_null_oid(&merge_base) ?
-			      the_hash_algo->empty_tree : &merge_base,
+		diff_tree_oid(is_null_oid(&branch_base) ?
+			      the_hash_algo->empty_tree : &branch_base,
 			      &options.onto->object.oid, "", &opts);
 		diffcore_std(&opts);
 		diff_flush(&opts);
@@ -1792,7 +1792,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head->object.oid)) {
+	if (oideq(&branch_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
-- 
gitgitgadget

Re: [PATCH v2 4/7] rebase: rename merge_base to branch_base

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> merge_base is not a very descriptive name, the variable always holds
> the merge-base of 'branch' and 'onto' which is commit at the base of
> the branch being rebased so rename it to branch_base.

OK.  Thanks for making the code easier to follow.

[PATCH v2 5/7] rebase: factor out branch_base calculation

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Separate out calculating the merge base between 'onto' and 'HEAD' from
the check for whether we can fast-forward or not. This means we can skip
the fast-forward checks when the rebase is forced and avoid calculating
the merge-base between 'HEAD' and 'onto' when --keep-base is given.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index dd5e0e1feb6..b5c78ce1fb0 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
-	merge_bases = get_merge_bases(onto, head);
-	if (!merge_bases || merge_bases->next) {
-		oidcpy(branch_base, null_oid());
+	if (is_null_oid(branch_base))
 		goto done;
-	}
 
-	oidcpy(branch_base, &merge_bases->item->object.oid);
 	if (!oideq(branch_base, &onto->object.oid))
 		goto done;
 
@@ -887,7 +883,6 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	if (!upstream)
 		goto done;
 
-	free_commit_list(merge_bases);
 	merge_bases = get_merge_bases(upstream, head);
 	if (!merge_bases || merge_bases->next)
 		goto done;
@@ -902,6 +897,20 @@ done:
 	return res && is_linear_history(onto, head);
 }
 
+static void fill_branch_base(struct rebase_options *options,
+			    struct object_id *branch_base)
+{
+	struct commit_list *merge_bases = NULL;
+
+	merge_bases = get_merge_bases(options->onto, options->orig_head);
+	if (!merge_bases || merge_bases->next)
+		oidcpy(branch_base, null_oid());
+	else
+		oidcpy(branch_base, &merge_bases->item->object.oid);
+
+	free_commit_list(merge_bases);
+}
+
 static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 {
 	struct rebase_options *opts = opt->value;
@@ -1667,8 +1676,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (!options.onto)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
+		fill_branch_base(&options, &branch_base);
 	}
-
 	if (options.fork_point > 0)
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
@@ -1696,13 +1705,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * Check if we are already based on onto with linear history,
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
-	 *
-	 * Note that can_fast_forward() initializes branch_base, so we have to
-	 * call it before checking allow_preemptive_ff.
 	 */
-	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &branch_base) &&
-	    allow_preemptive_ff) {
+	if (allow_preemptive_ff &&
+	    can_fast_forward(options.onto, options.upstream, options.restrict_revision,
+			     options.orig_head, &branch_base)) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {
-- 
gitgitgadget

Re: [PATCH v2 5/7] rebase: factor out branch_base calculation

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index dd5e0e1feb6..b5c78ce1fb0 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>  	struct commit_list *merge_bases = NULL;
>  	int res = 0;
>  
> -	merge_bases = get_merge_bases(onto, head);
> -	if (!merge_bases || merge_bases->next) {
> -		oidcpy(branch_base, null_oid());
> +	if (is_null_oid(branch_base))
>  		goto done;

Naïvely, I would have expected the condition of branch_base being
null to mean "the caller doesn't have branch base specified (like
parsing --onto), so we would need to compute one" and this
"branch_base is null, so leave without doing anything" looked
confusing at first.

After computing options.onto_name (given either with --onto or
falling back to .upstream_name), we make sure branch_base is
correctly filled, either from an explicit A...B notation or a single
committish object name given and converted with a call to
fill_branch_base().  And fill_branch_base() uses null object name as
a signal that it punted due to multiple merge bases.

> +static void fill_branch_base(struct rebase_options *options,
> +			    struct object_id *branch_base)
> +{
> +	struct commit_list *merge_bases = NULL;
> +
> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
> +	if (!merge_bases || merge_bases->next)
> +		oidcpy(branch_base, null_oid());
> +	else
> +		oidcpy(branch_base, &merge_bases->item->object.oid);
> +
> +	free_commit_list(merge_bases);
> +}

So it is doing the same thing as the original, but unlike the
original, it is not immediately obvious why "goto done" is the right
thing to do here.  Perhaps it deserves a comment, e.g.

	if (is_null_oid(branch_base))
		goto done; /* fill_branch_base() already checked and punted */

or something.

Thanks.

[PATCH v2 6/7] rebase --keep-base: imply --reapply-cherry-picks

[Phillip Wood via GitGitGadget]

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     | 26 ++++++++++++++++----------
 builtin/rebase.c                 | 16 +++++++++++++++-
 t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
 3 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 080658c8710..ee6cdd56949 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -218,12 +218,14 @@ 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
 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.
+rebasing on top of the upstream but to keep the base commit as-is. As
+the base commit is unchanged this option implies `--reapply-cherry-picks`
+to avoid losing commits.
 +
 Although both this option and `--fork-point` find the merge base between
 `<upstream>` and `<branch>`, this option uses the merge base as the _starting
@@ -278,7 +280,8 @@ See also INCOMPATIBLE OPTIONS below.
 Note that commits which start empty are kept (unless `--no-keep-empty`
 is specified), and commits which are clean cherry-picks (as determined
 by `git log --cherry-mark ...`) are detected and dropped as a
-preliminary step (unless `--reapply-cherry-picks` is passed).
+preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
+passed).
 +
 See also INCOMPATIBLE OPTIONS below.
 
@@ -311,13 +314,16 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-By default (or if `--no-reapply-cherry-picks` is given), these commits
-will be automatically dropped.  Because this necessitates reading all
-upstream commits, this can be expensive in repos with a large number
-of upstream commits that need to be read.  When using the 'merge'
-backend, warnings will be issued for each dropped commit (unless
-`--quiet` is given). Advice will also be issued unless
-`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
+
+In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
+given), these commits will be automatically dropped.  Because this
+necessitates reading all upstream commits, this can be expensive in
+repositories with a large number of upstream commits that need to be
+read. When using the 'merge' backend, warnings will be issued for each
+dropped commit (unless `--quiet` is given). Advice will also be issued
+unless `advice.skippedCherryPicks` is set to false (see
+linkgit:git-config[1]).
+
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.
diff --git a/builtin/rebase.c b/builtin/rebase.c
index b5c78ce1fb0..204155bb25b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1180,6 +1180,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 */
@@ -1239,6 +1240,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 to avoid losing
+	 * commits 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");
@@ -1415,7 +1422,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)
@@ -1678,6 +1689,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				options.onto_name);
 		fill_branch_base(&options, &branch_base);
 	}
+	if (keep_base && options.reapply_cherry_picks)
+		options.upstream = options.onto;
+
 	if (options.fork_point > 0)
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 01eb9513d6c..ea501f2b42b 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -199,6 +199,27 @@ test_expect_success 'rebase --keep-base requires a single merge base' '
 	grep "need exactly one merge base with branch" err
 '
 
+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
-- 
gitgitgadget

[PATCH v2 7/7] rebase --keep-base: imply --no-fork-point

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Given the name of the option it is confusing if --keep-base actually
changes the base of the branch without --fork-point being explicitly
given on the command line.

The combination of --keep-base with an explicit --fork-point is still
supported even though --fork-point means we do not keep the same base
if the upstream branch has been rewound.  We do this in case anyone is
relying on this behavior which is tested in t3431[1]

[1] https://lore.kernel.org/git/20200715032014.GA10818@generichostname/

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-rebase.txt | 8 ++++----
 builtin/rebase.c             | 6 ++++++
 t/t3431-rebase-fork-point.sh | 2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ee6cdd56949..1e2d543ced9 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 --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
+	`git rebase --reapply-cherry-picks --no-fork-point --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
@@ -452,9 +452,9 @@ When `--fork-point` is active, 'fork_point' will be used instead of
 <branch>` command (see linkgit:git-merge-base[1]).  If 'fork_point'
 ends up being empty, the `<upstream>` will be used as a fallback.
 +
-If `<upstream>` is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`. See also
-`rebase.forkpoint` in linkgit:git-config[1].
+If `<upstream>` or `--keep-base` is given on the command line, then
+the default is `--no-fork-point`, otherwise the default is
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
 +
 If your branch was based on `<upstream>` but `<upstream>` was rewound and
 your branch contains commits which were dropped, this option can be used
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 204155bb25b..3065e6f082b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1239,6 +1239,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
 		if (options.root)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
+		/*
+		 * --keep-base defaults to --no-fork-point to keep the
+		 * base the same.
+		 */
+		if (options.fork_point < 0)
+			options.fork_point = 0;
 	}
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 1d0b15380ed..70e81363569 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -50,7 +50,7 @@ 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 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
-- 
gitgitgadget

Re: [PATCH v2 7/7] rebase --keep-base: imply --no-fork-point

[Denton Liu]

Hi Phillip,

Sorry I haven't been active in Git development lately. I've been busy
with other stuff. I was skimming my emails and this patch caught my eye.

On Wed, Sep 07, 2022 at 02:37:50PM +0000, Phillip Wood via GitGitGadget wrote:
> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> index 1d0b15380ed..70e81363569 100755
> --- a/t/t3431-rebase-fork-point.sh
> +++ b/t/t3431-rebase-fork-point.sh
> @@ -50,7 +50,7 @@ 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 B A' --keep-base

We should add a test here for explicit --fork-point just to ensure that
the behaviour stays the same in case anyone relies on it here.

>  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
> -- 
> gitgitgadget

Re: [PATCH v2 7/7] rebase --keep-base: imply --no-fork-point

[Phillip Wood]

Hi Denton

On 08/09/2022 03:44, Denton Liu wrote:
> Hi Phillip,
> 
> Sorry I haven't been active in Git development lately. I've been busy
> with other stuff. I was skimming my emails and this patch caught my eye.

Thanks for taking a look

> On Wed, Sep 07, 2022 at 02:37:50PM +0000, Phillip Wood via GitGitGadget wrote:
>> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
>> index 1d0b15380ed..70e81363569 100755
>> --- a/t/t3431-rebase-fork-point.sh
>> +++ b/t/t3431-rebase-fork-point.sh
>> @@ -50,7 +50,7 @@ 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 B A' --keep-base
> 
> We should add a test here for explicit --fork-point just to ensure that
> the behaviour stays the same in case anyone relies on it here.

There is an existing test, it does not show up in the patch because it 
is unchanged.

Best Wishes

Phillip

[PATCH v3 0/8] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[Phillip Wood via GitGitGadget]

A while a go Philippe reported [1] that he was surprised 'git rebase
--keep-base' removed commits that had been cherry-picked upstream even
though to branch was not being rebased. I think it is also surprising if
'--keep-base' changes the base of the branch without '--fork-point' being
explicitly given on the command line. This series therefore changes the
default behavior of '--keep-base' to imply '--reapply-cherry-picks' and
'--no-fork-point' so that the base of the branch is unchanged and no commits
are removed.

Thanks to Junio for his comments, the changes since V2 are:

 * Patch 3 new patch to make sure we're reading hex oids from state files
 * Patch 4 restored the call to read_refs() to avoid the dwim behavior of
   lookup_commit_reference_by_name()
 * Patch 6 added a comment to clarify what a null oid branch_base means

Thanks to everyone who commented for their reviews, the changes since V1
are:

 * Patch 1: new patch to tighten a couple of existing tests
 * Patch 2: reworded commit message in response to Junio's comments
 * Patch 3: fixed a typo in the commit message spotted by Elijah and tidied
   code formatting
 * Patch 4: new patch to rename a variable suggested by Junio
 * Patch 5: clarified commit message and removed some redundant code spotted
   by Junio
 * Patch 6: improved --reapply-cherry-picks documentation to mention
   --keep-base and vice-versa suggested by Philippe
 * Patch 7: expanded the commit message and documentation in response to
   Junio's comments

[1]
https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Phillip Wood (8):
  t3416: tighten two tests
  t3416: set $EDITOR in subshell
  rebase: be stricter when reading state files containing oids
  rebase: store orig_head as a commit
  rebase: rename merge_base to branch_base
  rebase: factor out branch_base calculation
  rebase --keep-base: imply --reapply-cherry-picks
  rebase --keep-base: imply --no-fork-point

 Documentation/git-rebase.txt     |  32 ++++---
 builtin/rebase.c                 | 139 ++++++++++++++++++-------------
 t/t3416-rebase-onto-threedots.sh |  62 ++++++++++----
 t/t3431-rebase-fork-point.sh     |   2 +-
 4 files changed, 148 insertions(+), 87 deletions(-)


base-commit: afa70145a25e81faa685dc0b465e52b45d2444bd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1323%2Fphillipwood%2Fwip%2Frebase--keep-base-tweaks-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1323/phillipwood/wip/rebase--keep-base-tweaks-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1323

Range-diff vs v2:

 1:  12fb0ac6d5d = 1:  12fb0ac6d5d t3416: tighten two tests
 2:  d6f2f716c77 = 2:  d6f2f716c77 t3416: set $EDITOR in subshell
 -:  ----------- > 3:  1fd58520253 rebase: be stricter when reading state files containing oids
 3:  9daee95d434 ! 4:  dc056b13ed5 rebase: store orig_head as a commit
     @@ Commit message
          the merge base and fork point as it no longer has to convert from an
          oid to a commit.
      
     +    To avoid changing the behavior of "git rebase <upstream> <branch>" we
     +    keep the existing call to read_ref() and use lookup_commit_reference()
     +    on the oid returned by that rather than calling
     +    lookup_commit_reference_by_name() which applies the ref dwim rules to
     +    its argument. lookup_commit_reference() will dereference tag objects
     +    but we do not expect the branch being rebased to be pointing to a tag
     +    object.
     +
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## builtin/rebase.c ##
     @@ builtin/rebase.c: static int read_basic_state(struct rebase_options *opts)
       	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
       				  READ_ONELINER_WARN_MISSING))
       		return -1;
     --	if (get_oid(buf.buf, &opts->orig_head))
     -+	opts->orig_head = lookup_commit_reference_by_name(buf.buf);
     -+	if (!opts->orig_head)
     +-	if (get_oid_hex(buf.buf, &opts->orig_head))
     ++	if (get_oid_hex(buf.buf, &oid) ||
     ++	    !(opts->orig_head = lookup_commit_reference(the_repository, &oid)))
       		return error(_("invalid orig-head: '%s'"), buf.buf);
       
       	if (file_exists(state_dir_path("quiet", opts)))
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       		ret = finish_rebase(&options);
       		goto cleanup;
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     + 	 */
     + 	if (argc == 1) {
     + 		/* Is it "rebase other branchname" or "rebase other commit"? */
     ++		struct object_id branch_oid;
     + 		branch_name = argv[0];
     + 		options.switch_to = argv[0];
     + 
       		/* Is it a local branch? */
       		strbuf_reset(&buf);
       		strbuf_addf(&buf, "refs/heads/%s", branch_name);
      -		if (!read_ref(buf.buf, &options.orig_head)) {
     -+		options.orig_head = lookup_commit_reference_by_name(buf.buf);
     -+		if (options.orig_head) {
     ++		if (!read_ref(buf.buf, &branch_oid)) {
       			die_if_checked_out(buf.buf, 1);
       			options.head_name = xstrdup(buf.buf);
     ++			options.orig_head =
     ++				lookup_commit_reference(the_repository,
     ++							&branch_oid);
       		/* If not is it a valid ref (branch or commit)? */
       		} else {
      -			struct commit *commit =
      +			options.orig_head =
       				lookup_commit_reference_by_name(branch_name);
      -			if (!commit)
     -+			if (!options.orig_head)
     - 				die(_("no such branch/commit '%s'"),
     - 				    branch_name);
     +-				die(_("no such branch/commit '%s'"),
     +-				    branch_name);
      -			oidcpy(&options.orig_head, &commit->object.oid);
       			options.head_name = NULL;
       		}
     ++		if (!options.orig_head)
     ++			die(_("no such branch/commit '%s'"), branch_name);
       	} else if (argc == 0) {
     + 		/* Do not need to switch branches, we are already on it. */
     + 		options.head_name =
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
       			FREE_AND_NULL(options.head_name);
       			branch_name = "HEAD";
 4:  cca933a5f1d = 5:  00f70c90344 rebase: rename merge_base to branch_base
 5:  fc45b996d34 ! 6:  2efbfc94187 rebase: factor out branch_base calculation
     @@ builtin/rebase.c: static int can_fast_forward(struct commit *onto, struct commit
      -	merge_bases = get_merge_bases(onto, head);
      -	if (!merge_bases || merge_bases->next) {
      -		oidcpy(branch_base, null_oid());
     -+	if (is_null_oid(branch_base))
     - 		goto done;
     +-		goto done;
      -	}
     ++	if (is_null_oid(branch_base))
     ++		goto done; /* fill_branch_base() found multiple merge bases */
       
      -	oidcpy(branch_base, &merge_bases->item->object.oid);
       	if (!oideq(branch_base, &onto->object.oid))
 6:  faad7eaf0d6 = 7:  bc39c76b217 rebase --keep-base: imply --reapply-cherry-picks
 7:  6410b101d7f = 8:  4d0226e1dcc rebase --keep-base: imply --no-fork-point

-- 
gitgitgadget

[PATCH v3 1/8] t3416: tighten two tests

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a check for the correct error message to the tests that check we
require a single merge base so we can be sure the rebase failed for
the correct reason. Also rename the tests to reflect what they are
testing.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3e04802cb00..dc67d2308f3 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -97,13 +97,14 @@ test_expect_success 'rebase -i --onto main...' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --onto main...side' '
+test_expect_success 'rebase --onto main...side requires a single merge-base' '
 	git reset --hard &&
 	git checkout side &&
 	git reset --hard K &&
 
 	set_fake_editor &&
-	test_must_fail git rebase -i --onto main...side J
+	test_must_fail git rebase -i --onto main...side J 2>err &&
+	grep "need exactly one merge base" err
 '
 
 test_expect_success 'rebase --keep-base --onto incompatible' '
@@ -182,13 +183,14 @@ test_expect_success 'rebase -i --keep-base main topic from main' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --keep-base main from side' '
+test_expect_success 'rebase --keep-base requires a single merge base' '
 	git reset --hard &&
 	git checkout side &&
 	git reset --hard K &&
 
 	set_fake_editor &&
-	test_must_fail git rebase -i --keep-base main
+	test_must_fail git rebase -i --keep-base main 2>err &&
+	grep "need exactly one merge base with branch" err
 '
 
 test_done
-- 
gitgitgadget

[PATCH v3 2/8] t3416: set $EDITOR in subshell

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As $EDITOR is exported, setting it in one test affects all subsequent
tests. Avoid this by always setting it in a subshell. Also remove a
couple of unnecessary call to set_fake_editor where the editor does
not change the todo list.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index dc67d2308f3..01eb9513d6c 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -79,8 +79,10 @@ test_expect_success 'rebase -i --onto main...topic' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main...topic F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main...topic F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -90,8 +92,10 @@ test_expect_success 'rebase -i --onto main...' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main... F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main... F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -102,7 +106,6 @@ test_expect_success 'rebase --onto main...side requires a single merge-base' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --onto main...side J 2>err &&
 	grep "need exactly one merge base" err
 '
@@ -157,8 +160,10 @@ test_expect_success 'rebase -i --keep-base main from topic' '
 	git checkout topic &&
 	git reset --hard G &&
 
-	set_fake_editor &&
-	EXPECT_COUNT=2 git rebase -i --keep-base main &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=2 git rebase -i --keep-base main
+	) &&
 	git rev-parse C >base.expect &&
 	git merge-base main HEAD >base.actual &&
 	test_cmp base.expect base.actual &&
@@ -172,8 +177,10 @@ 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 &&
+	(
+		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 &&
@@ -188,9 +195,13 @@ test_expect_success 'rebase --keep-base requires a single merge base' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --keep-base main 2>err &&
 	grep "need exactly one merge base with branch" err
 '
 
+# This must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done
-- 
gitgitgadget

[PATCH v3 3/8] rebase: be stricter when reading state files containing oids

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The state files for 'onto' and 'orig_head' should contain a full hex
oid, change the reading functions from get_oid() to get_oid_hex() to
reflect this.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..76f83a42f49 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -431,7 +431,7 @@ static int read_basic_state(struct rebase_options *opts)
 	opts->head_name = starts_with(head_name.buf, "refs/") ?
 		xstrdup(head_name.buf) : NULL;
 	strbuf_release(&head_name);
-	if (get_oid(buf.buf, &oid))
+	if (get_oid_hex(buf.buf, &oid))
 		return error(_("could not get 'onto': '%s'"), buf.buf);
 	opts->onto = lookup_commit_or_die(&oid, buf.buf);
 
@@ -448,7 +448,7 @@ static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid(buf.buf, &opts->orig_head))
+	if (get_oid_hex(buf.buf, &opts->orig_head))
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
-- 
gitgitgadget

Re: [PATCH v3 3/8] rebase: be stricter when reading state files containing oids

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The state files for 'onto' and 'orig_head' should contain a full hex
> oid, change the reading functions from get_oid() to get_oid_hex() to
> reflect this.

OK.

> -	if (get_oid(buf.buf, &oid))
> +	if (get_oid_hex(buf.buf, &oid))
> ...
>  		return -1;
> -	if (get_oid(buf.buf, &opts->orig_head))
> +	if (get_oid_hex(buf.buf, &opts->orig_head))

Re: [PATCH v3 3/8] rebase: be stricter when reading state files containing oids

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The state files for 'onto' and 'orig_head' should contain a full hex
> oid, change the reading functions from get_oid() to get_oid_hex() to
> reflect this.

This seems sensible, but isn't "full hex oid" the tip of the iceberg in
get_oid() v.s. get_oid_hex() differences? The former allowing
e.g. :-syntax, ^-syntax etc.

> @@ -431,7 +431,7 @@ static int read_basic_state(struct rebase_options *opts)
>  	opts->head_name = starts_with(head_name.buf, "refs/") ?
>  		xstrdup(head_name.buf) : NULL;
>  	strbuf_release(&head_name);
> -	if (get_oid(buf.buf, &oid))
> +	if (get_oid_hex(buf.buf, &oid))

The change looks sensible, maybe a commit message update + assuring
ourselves that we don't care about anyone manually manipulating these
files (e.g. a script with OID abbreviation that would "echo" to these.

>  		return error(_("could not get 'onto': '%s'"), buf.buf);
>  	opts->onto = lookup_commit_or_die(&oid, buf.buf);
>  
> @@ -448,7 +448,7 @@ static int read_basic_state(struct rebase_options *opts)
>  	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
>  				  READ_ONELINER_WARN_MISSING))
>  		return -1;
> -	if (get_oid(buf.buf, &opts->orig_head))
> +	if (get_oid_hex(buf.buf, &opts->orig_head))
>  		return error(_("invalid orig-head: '%s'"), buf.buf);

Not a new issue, but this error() is much more sensible than the above,
we could get "onto", we just didn't like its contents on error(), oh
well...

Re: [PATCH v3 3/8] rebase: be stricter when reading state files containing oids

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
>
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The state files for 'onto' and 'orig_head' should contain a full hex
>> oid, change the reading functions from get_oid() to get_oid_hex() to
>> reflect this.
> ...
>> @@ -431,7 +431,7 @@ static int read_basic_state(struct rebase_options *opts)
>>  	opts->head_name = starts_with(head_name.buf, "refs/") ?
>>  		xstrdup(head_name.buf) : NULL;
>>  	strbuf_release(&head_name);
>> -	if (get_oid(buf.buf, &oid))
>> +	if (get_oid_hex(buf.buf, &oid))
>
> The change looks sensible, maybe a commit message update + assuring
> ourselves that we don't care about anyone manually manipulating these
> files (e.g. a script with OID abbreviation that would "echo" to these.

"should contain" sufficiently conveys that already, but I do not
mind being extra clear, either.

You do not want 6c1221c99975ad3216d82de51ed980fbf327d7f8 to be
interpreted as a branch whose name is the 40-hex that points at a
wrong commit, when it appears on the state file and I think that the
proposed log message gives a reasonable explanation why we do not
want to use get_oid() and instead use get_oid_hex().  If the code
made sure that 40-hex is followed by end-of-line, that would even be
better, but it is not required.

[PATCH v3 4/8] rebase: store orig_head as a commit

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Using a struct commit rather than a struct oid to hold orig_head means
that we error out straight away if the branch being rebased does not
point to a commit. It also simplifies the code that handles finding
the merge base and fork point as it no longer has to convert from an
oid to a commit.

To avoid changing the behavior of "git rebase <upstream> <branch>" we
keep the existing call to read_ref() and use lookup_commit_reference()
on the oid returned by that rather than calling
lookup_commit_reference_by_name() which applies the ref dwim rules to
its argument. lookup_commit_reference() will dereference tag objects
but we do not expect the branch being rebased to be pointing to a tag
object.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 67 ++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 76f83a42f49..7e6ce374c59 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -68,7 +68,7 @@ struct rebase_options {
 	const char *upstream_name;
 	const char *upstream_arg;
 	char *head_name;
-	struct object_id orig_head;
+	struct commit *orig_head;
 	struct commit *onto;
 	const char *onto_name;
 	const char *revisions;
@@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	struct replay_opts replay = get_replay_opts(opts);
 	struct string_list commands = STRING_LIST_INIT_DUP;
 
-	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
+	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
 		return -1;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head)) {
+			     opts->onto, &opts->orig_head->object.oid)) {
 		free(revisions);
 		free(shortrevisions);
 
@@ -298,9 +298,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		split_exec_commands(opts->cmd, &commands);
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
-			&opts->orig_head, &commands, opts->autosquash,
-			opts->update_refs,
-			&todo_list);
+			&opts->orig_head->object.oid, &commands,
+			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
 	string_list_clear(&commands, 0);
@@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid_hex(buf.buf, &opts->orig_head))
+	if (get_oid_hex(buf.buf, &oid) ||
+	    !(opts->orig_head = lookup_commit_reference(the_repository, &oid)))
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
@@ -517,7 +517,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	write_file(state_dir_path("onto", opts), "%s",
 		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
 	write_file(state_dir_path("orig-head", opts), "%s",
-		   oid_to_hex(&opts->orig_head));
+		   oid_to_hex(&opts->orig_head->object.oid));
 	if (!(opts->flags & REBASE_NO_QUIET))
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
@@ -646,7 +646,7 @@ static int run_am(struct rebase_options *opts)
 			       /* this is now equivalent to !opts->upstream */
 			       &opts->onto->object.oid :
 			       &opts->upstream->object.oid),
-		    oid_to_hex(&opts->orig_head));
+		    oid_to_hex(&opts->orig_head->object.oid));
 
 	rebased_patches = xstrdup(git_path("rebased-patches"));
 	format_patch.out = open(rebased_patches,
@@ -680,7 +680,7 @@ static int run_am(struct rebase_options *opts)
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		ropts.oid = &opts->orig_head;
+		ropts.oid = &opts->orig_head->object.oid;
 		ropts.branch = opts->head_name;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		reset_head(the_repository, &ropts);
@@ -833,7 +833,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	ropts.oid = &options->orig_head;
+	ropts.oid = &options->orig_head->object.oid;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	if (!ropts.branch)
@@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct object_id *head_oid, struct object_id *merge_base)
+			    struct commit *head, 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());
@@ -1312,13 +1308,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		ropts.oid = &options.orig_head;
+		ropts.oid = &options.orig_head->object.oid;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
-			    oid_to_hex(&options.orig_head));
+			    oid_to_hex(&options.orig_head->object.oid));
 		remove_branch_state(the_repository, 0);
 		ret = finish_rebase(&options);
 		goto cleanup;
@@ -1604,25 +1600,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 */
 	if (argc == 1) {
 		/* Is it "rebase other branchname" or "rebase other commit"? */
+		struct object_id branch_oid;
 		branch_name = argv[0];
 		options.switch_to = argv[0];
 
 		/* Is it a local branch? */
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "refs/heads/%s", branch_name);
-		if (!read_ref(buf.buf, &options.orig_head)) {
+		if (!read_ref(buf.buf, &branch_oid)) {
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
+			options.orig_head =
+				lookup_commit_reference(the_repository,
+							&branch_oid);
 		/* If not is it a valid ref (branch or commit)? */
 		} else {
-			struct commit *commit =
+			options.orig_head =
 				lookup_commit_reference_by_name(branch_name);
-			if (!commit)
-				die(_("no such branch/commit '%s'"),
-				    branch_name);
-			oidcpy(&options.orig_head, &commit->object.oid);
 			options.head_name = NULL;
 		}
+		if (!options.orig_head)
+			die(_("no such branch/commit '%s'"), branch_name);
 	} else if (argc == 0) {
 		/* Do not need to switch branches, we are already on it. */
 		options.head_name =
@@ -1639,8 +1637,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			FREE_AND_NULL(options.head_name);
 			branch_name = "HEAD";
 		}
-		if (get_oid("HEAD", &options.orig_head))
-			die(_("Could not resolve HEAD to a revision"));
+		options.orig_head = lookup_commit_reference_by_name("HEAD");
+		if (!options.orig_head)
+			die(_("Could not resolve HEAD to a commit"));
 	} else
 		BUG("unexpected number of arguments left to parse");
 
@@ -1672,13 +1671,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				options.onto_name);
 	}
 
-	if (options.fork_point > 0) {
-		struct commit *head =
-			lookup_commit_reference(the_repository,
-						&options.orig_head);
+	if (options.fork_point > 0)
 		options.restrict_revision =
-			get_fork_point(options.upstream_name, head);
-	}
+			get_fork_point(options.upstream_name, options.orig_head);
 
 	if (repo_read_index(the_repository) < 0)
 		die(_("could not read index"));
@@ -1708,7 +1703,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    &options.orig_head, &merge_base) &&
+		    options.orig_head, &merge_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1785,7 +1780,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
-	ropts.orig_head = &options.orig_head,
+	ropts.orig_head = &options.orig_head->object.oid,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;
@@ -1799,7 +1794,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head)) {
+	if (oideq(&merge_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
@@ -1820,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		    (options.restrict_revision ?
 		     oid_to_hex(&options.restrict_revision->object.oid) :
 		     oid_to_hex(&options.upstream->object.oid)),
-		    oid_to_hex(&options.orig_head));
+		    oid_to_hex(&options.orig_head->object.oid));
 
 	options.revisions = revisions.buf;
 
-- 
gitgitgadget

Re: [PATCH v3 4/8] rebase: store orig_head as a commit

[Junio C Hamano]

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> To avoid changing the behavior of "git rebase <upstream> <branch>" we
> keep the existing call to read_ref() and use lookup_commit_reference()
> on the oid returned by that rather than calling
> lookup_commit_reference_by_name() which applies the ref dwim rules to
> its argument.

I agree with the above in that _by_name was an overly broad and
wrong function to call.  But ...

> lookup_commit_reference() will dereference tag objects
> but we do not expect the branch being rebased to be pointing to a tag
> object.

... if we do not expect to see a tag object, why call _reference()
in the first place?  Does lookup_commit() not work for the purpose
of this code?

A bit puzzled...

Re: [PATCH v3 4/8] rebase: store orig_head as a commit

[Phillip Wood]

Hi Junio

On 13/10/2022 17:34, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> To avoid changing the behavior of "git rebase <upstream> <branch>" we
>> keep the existing call to read_ref() and use lookup_commit_reference()
>> on the oid returned by that rather than calling
>> lookup_commit_reference_by_name() which applies the ref dwim rules to
>> its argument.
> 
> I agree with the above in that _by_name was an overly broad and
> wrong function to call.  But ...
> 
>> lookup_commit_reference() will dereference tag objects
>> but we do not expect the branch being rebased to be pointing to a tag
>> object.
> 
> ... if we do not expect to see a tag object, why call _reference()
> in the first place?  Does lookup_commit() not work for the purpose
> of this code?

I was wasn't convinced that the tag dereferencing was the only 
difference between lookup_commit_reference() and lookup_commit(). 
lookup_commit() calls lookup_object() and if the object is not in the 
parsed objects hash table it creates a new commit object with the given 
oid without checking if the object exists. lookup_commit_reference() 
calls parse_object() which handles replacement objects and checks the 
object actually exists.

Best Wishes

Phillip

Re: [PATCH v3 4/8] rebase: store orig_head as a commit

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I was wasn't convinced that the tag dereferencing was the only
> difference between lookup_commit_reference() and
> lookup_commit(). lookup_commit() calls lookup_object() and if the
> object is not in the parsed objects hash table it creates a new commit
> object with the given oid without checking if the object
> exists. lookup_commit_reference() calls parse_object() which handles
> replacement objects and checks the object actually exists.

Yes, I agree that both parse_object() and lookup_commit() are
needed, and the combo is preferred over lookup_commit_reference().

THanks.

[PATCH v3 5/8] rebase: rename merge_base to branch_base

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

merge_base is not a very descriptive name, the variable always holds
the merge-base of 'branch' and 'onto' which is commit at the base of
the branch being rebased so rename it to branch_base.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 7e6ce374c59..cbafcc41e75 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -866,22 +866,22 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct commit *head, struct object_id *merge_base)
+			    struct commit *head, struct object_id *branch_base)
 {
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
 	merge_bases = get_merge_bases(onto, head);
 	if (!merge_bases || merge_bases->next) {
-		oidcpy(merge_base, null_oid());
+		oidcpy(branch_base, null_oid());
 		goto done;
 	}
 
-	oidcpy(merge_base, &merge_bases->item->object.oid);
-	if (!oideq(merge_base, &onto->object.oid))
+	oidcpy(branch_base, &merge_bases->item->object.oid);
+	if (!oideq(branch_base, &onto->object.oid))
 		goto done;
 
-	if (restrict_revision && !oideq(&restrict_revision->object.oid, merge_base))
+	if (restrict_revision && !oideq(&restrict_revision->object.oid, branch_base))
 		goto done;
 
 	if (!upstream)
@@ -1035,7 +1035,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf msg = STRBUF_INIT;
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	struct object_id merge_base;
+	struct object_id branch_base;
 	int ignore_whitespace = 0;
 	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
@@ -1653,7 +1653,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} 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 (get_oid_mb(options.onto_name, &branch_base) < 0) {
 			if (keep_base)
 				die(_("'%s': need exactly one merge base with branch"),
 				    options.upstream_name);
@@ -1661,7 +1661,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				die(_("'%s': need exactly one merge base"),
 				    options.onto_name);
 		}
-		options.onto = lookup_commit_or_die(&merge_base,
+		options.onto = lookup_commit_or_die(&branch_base,
 						    options.onto_name);
 	} else {
 		options.onto =
@@ -1699,11 +1699,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
 	 *
-	 * Note that can_fast_forward() initializes merge_base, so we have to
+	 * Note that can_fast_forward() initializes branch_base, so we have to
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &merge_base) &&
+		    options.orig_head, &branch_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1745,12 +1745,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct diff_options opts;
 
 		if (options.flags & REBASE_VERBOSE) {
-			if (is_null_oid(&merge_base))
+			if (is_null_oid(&branch_base))
 				printf(_("Changes to %s:\n"),
 				       oid_to_hex(&options.onto->object.oid));
 			else
 				printf(_("Changes from %s to %s:\n"),
-				       oid_to_hex(&merge_base),
+				       oid_to_hex(&branch_base),
 				       oid_to_hex(&options.onto->object.oid));
 		}
 
@@ -1762,8 +1762,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 		opts.detect_rename = DIFF_DETECT_RENAME;
 		diff_setup_done(&opts);
-		diff_tree_oid(is_null_oid(&merge_base) ?
-			      the_hash_algo->empty_tree : &merge_base,
+		diff_tree_oid(is_null_oid(&branch_base) ?
+			      the_hash_algo->empty_tree : &branch_base,
 			      &options.onto->object.oid, "", &opts);
 		diffcore_std(&opts);
 		diff_flush(&opts);
@@ -1794,7 +1794,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head->object.oid)) {
+	if (oideq(&branch_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
-- 
gitgitgadget

Re: [PATCH v3 5/8] rebase: rename merge_base to branch_base

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> merge_base is not a very descriptive name, the variable always holds
> the merge-base of 'branch' and 'onto' which is commit at the base of
> the branch being rebased so rename it to branch_base.

To me "branch" means or has heavier implications of "named branch" than
just a merge base, and this command is perfectly happy to work on
commits disconnected from any named branch.

But more to the point, the rebase docs for --onto discuss a "merge
base", so you'd read those, and then encounter this code talking about a
"branch base", and wonder what the difference was...

Re: [PATCH v3 5/8] rebase: rename merge_base to branch_base

[Phillip Wood]

On 13/10/2022 20:16, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> merge_base is not a very descriptive name, the variable always holds
>> the merge-base of 'branch' and 'onto' which is commit at the base of
>> the branch being rebased so rename it to branch_base.
> 
> To me "branch" means or has heavier implications of "named branch" than
> just a merge base, and this command is perfectly happy to work on
> commits disconnected from any named branch.
> > But more to the point, the rebase docs for --onto discuss a "merge
> base", so you'd read those, and then encounter this code talking about a
> "branch base", and wonder what the difference was...

Aren't the docs saying the merge base is the base of the commits (i.e. 
branch) being rebased? I don't think merge_base is a particularly 
helpful name as it doesn't tell us what it is the merge base of and 
branch_base was the best I could come up with. I see what you mean in 
the detached HEAD case, but as the command also works with named 
branches I hope it is fairly obvious what "branch_base" is in the 
detached HEAD case.

Best Wishes

Phillip

Re: [PATCH v3 5/8] rebase: rename merge_base to branch_base

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 17 2022, Phillip Wood wrote:

> On 13/10/2022 20:16, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> merge_base is not a very descriptive name, the variable always holds
>>> the merge-base of 'branch' and 'onto' which is commit at the base of
>>> the branch being rebased so rename it to branch_base.
>> To me "branch" means or has heavier implications of "named branch"
>> than
>> just a merge base, and this command is perfectly happy to work on
>> commits disconnected from any named branch.
>> > But more to the point, the rebase docs for --onto discuss a "merge
>> base", so you'd read those, and then encounter this code talking about a
>> "branch base", and wonder what the difference was...
>
> Aren't the docs saying the merge base is the base of the commits
> (i.e. branch) being rebased? I don't think merge_base is a
> particularly helpful name as it doesn't tell us what it is the merge
> base of and branch_base was the best I could come up with. I see what
> you mean in the detached HEAD case, but as the command also works with
> named branches I hope it is fairly obvious what "branch_base" is in
> the detached HEAD case.

It *optionally* works with a <branch>, but doesn't require one. E.g. try
this on git.git:

	git checkout origin/next
	touch f && git add f && git commit -m"file"
	git rebase --onto origin/master^{} HEAD~

Here we transplant a commit on top of "next" to "master", without either
of those *names* being involved, or their branches, just the
corresponding OIDs/tips.

That will go through e.g. can_fast_forward() which you're modifying
here, and now populate a "branch_base" variable, instead of a
"merge_base".

I know that we conflate the meaning of "branch" somewhat, even in our
own docs. E.g. we sometimes use "branch" and "named branch", but usually
by "branch" we mean "named branch", and otherwise talk about a detached
HEAD, <commit> or "tip".

But in this case it's especially confusing in the post-image, because
"git rebase --onto" explicitly uses an optional "<branch>" to
distinguish the "named branch" case from the case where we're operating
on detached a HEAD, or otherwise don't care about the "<branch>" (except
as generic "restore us to where we were" behavior).

So, if anything I'd think that we'd want something like this in various
places in git-rebase.txt to make the distinction clearer:
	
	diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
	index 9cb8931c7ac..e4700a6e777 100644
	--- a/Documentation/git-rebase.txt
	+++ b/Documentation/git-rebase.txt
	@@ -18,7 +18,7 @@ DESCRIPTION
	 -----------
	 If `<branch>` is specified, `git rebase` will perform an automatic
	 `git switch <branch>` before doing anything else.  Otherwise
	-it remains on the current branch.
	+it remains on the current tip or named branch.
	 
	 If `<upstream>` is not specified, the upstream configured in
	 `branch.<name>.remote` and `branch.<name>.merge` options will be used (see

But your post-image seems to be to make this sort of thing explicitly
more confusing, and e.g. these parts:

	@@ -206,8 +206,8 @@ OPTIONS
	 --onto <newbase>::
	 	Starting point at which to create the new commits. If the
	 	`--onto` option is not specified, the starting point is
	-	`<upstream>`.  May be any valid commit, and not just an
	-	existing branch name.
	+	`<upstream>`.  May be any valid commit, and not just an <-- this
	+	existing branch name. <--- this
	 +
	 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

To sum up why I find this confusing: Reading this from the docs onwards
I'd think (as is the case) that "<branch>" is optional. Then when I read
the code I'd think a "branch_base" is something that *only* had to do
with the "<branch>" case.

But that's not the case, it's just a generic "merge base" in the same
sense that "git merge-base" accepts all of these

	$ git merge-base origin/master origin/next
	d420dda0576340909c3faff364cfbd1485f70376

(These two are equivalent, just demo'ing that we don't need the peel
syntax):

	$ git merge-base $(git rev-parse origin/master) $(git rev-parse origin/next)
	d420dda0576340909c3faff364cfbd1485f70376
	$ git merge-base origin/master^{} origin/next^{}
	d420dda0576340909c3faff364cfbd1485f70376

What *would* make things much clearer is e.g. calling a variable
"branch_merge_base" *if* there is a case where that's a merge base only
for named branches, but I don't know (and didn't look carefully enough)
if you've got such a case or cases here. It just seems like a generic
"merge-base".

Re: [PATCH v3 5/8] rebase: rename merge_base to branch_base

[Phillip Wood]

On 17/10/2022 12:27, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Oct 17 2022, Phillip Wood wrote:
> 
>> On 13/10/2022 20:16, Ævar Arnfjörð Bjarmason wrote:
>>> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
>>>
>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>
>>>> merge_base is not a very descriptive name, the variable always holds
>>>> the merge-base of 'branch' and 'onto' which is commit at the base of
>>>> the branch being rebased so rename it to branch_base.
>>> To me "branch" means or has heavier implications of "named branch"
>>> than
>>> just a merge base, and this command is perfectly happy to work on
>>> commits disconnected from any named branch.
>>>> But more to the point, the rebase docs for --onto discuss a "merge
>>> base", so you'd read those, and then encounter this code talking about a
>>> "branch base", and wonder what the difference was...
>>
>> Aren't the docs saying the merge base is the base of the commits
>> (i.e. branch) being rebased? I don't think merge_base is a
>> particularly helpful name as it doesn't tell us what it is the merge
>> base of and branch_base was the best I could come up with. I see what
>> you mean in the detached HEAD case, but as the command also works with
>> named branches I hope it is fairly obvious what "branch_base" is in
>> the detached HEAD case.
> 
> It *optionally* works with a <branch>, but doesn't require one. E.g. try
> this on git.git:

Maybe I wasn't clear, I was referring to the fact that if HEAD isn't 
detached then it rebases the current branch not about the optional 
<branch> argument. I also think that the docs are for users, they are 
not a guide to the code. With this change if you search for merge_base 
in builtin/rebase.c you still find the part where we calculate the merge 
base. This commit was added in response to a review comment from Junio 
on V1, as far as I know he is happy with it and at this stage I'm 
disinclined to change it.

Best Wishes

Phillip

> 	git checkout origin/next
> 	touch f && git add f && git commit -m"file"
> 	git rebase --onto origin/master^{} HEAD~
> 
> Here we transplant a commit on top of "next" to "master", without either
> of those *names* being involved, or their branches, just the
> corresponding OIDs/tips.
> 
> That will go through e.g. can_fast_forward() which you're modifying
> here, and now populate a "branch_base" variable, instead of a
> "merge_base".
> 
> I know that we conflate the meaning of "branch" somewhat, even in our
> own docs. E.g. we sometimes use "branch" and "named branch", but usually
> by "branch" we mean "named branch", and otherwise talk about a detached
> HEAD, <commit> or "tip".
> 
> But in this case it's especially confusing in the post-image, because
> "git rebase --onto" explicitly uses an optional "<branch>" to
> distinguish the "named branch" case from the case where we're operating
> on detached a HEAD, or otherwise don't care about the "<branch>" (except
> as generic "restore us to where we were" behavior).
> 
> So, if anything I'd think that we'd want something like this in various
> places in git-rebase.txt to make the distinction clearer:
> 	
> 	diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> 	index 9cb8931c7ac..e4700a6e777 100644
> 	--- a/Documentation/git-rebase.txt
> 	+++ b/Documentation/git-rebase.txt
> 	@@ -18,7 +18,7 @@ DESCRIPTION
> 	 -----------
> 	 If `<branch>` is specified, `git rebase` will perform an automatic
> 	 `git switch <branch>` before doing anything else.  Otherwise
> 	-it remains on the current branch.
> 	+it remains on the current tip or named branch.
> 	
> 	 If `<upstream>` is not specified, the upstream configured in
> 	 `branch.<name>.remote` and `branch.<name>.merge` options will be used (see
> 
> But your post-image seems to be to make this sort of thing explicitly
> more confusing, and e.g. these parts:
> 
> 	@@ -206,8 +206,8 @@ OPTIONS
> 	 --onto <newbase>::
> 	 	Starting point at which to create the new commits. If the
> 	 	`--onto` option is not specified, the starting point is
> 	-	`<upstream>`.  May be any valid commit, and not just an
> 	-	existing branch name.
> 	+	`<upstream>`.  May be any valid commit, and not just an <-- this
> 	+	existing branch name. <--- this
> 	 +
> 	 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
> 
> To sum up why I find this confusing: Reading this from the docs onwards
> I'd think (as is the case) that "<branch>" is optional. Then when I read
> the code I'd think a "branch_base" is something that *only* had to do
> with the "<branch>" case.
> 
> But that's not the case, it's just a generic "merge base" in the same
> sense that "git merge-base" accepts all of these
> 
> 	$ git merge-base origin/master origin/next
> 	d420dda0576340909c3faff364cfbd1485f70376
> 
> (These two are equivalent, just demo'ing that we don't need the peel
> syntax):
> 
> 	$ git merge-base $(git rev-parse origin/master) $(git rev-parse origin/next)
> 	d420dda0576340909c3faff364cfbd1485f70376
> 	$ git merge-base origin/master^{} origin/next^{}
> 	d420dda0576340909c3faff364cfbd1485f70376
> 
> What *would* make things much clearer is e.g. calling a variable
> "branch_merge_base" *if* there is a case where that's a merge base only
> for named branches, but I don't know (and didn't look carefully enough)
> if you've got such a case or cases here. It just seems like a generic
> "merge-base".
> 
> 

Re: [PATCH v3 5/8] rebase: rename merge_base to branch_base

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 17 2022, Phillip Wood wrote:

> On 17/10/2022 12:27, Ævar Arnfjörð Bjarmason wrote:
>> On Mon, Oct 17 2022, Phillip Wood wrote:
>> 
>>> On 13/10/2022 20:16, Ævar Arnfjörð Bjarmason wrote:
>>>> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
>>>>
>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>
>>>>> merge_base is not a very descriptive name, the variable always holds
>>>>> the merge-base of 'branch' and 'onto' which is commit at the base of
>>>>> the branch being rebased so rename it to branch_base.
>>>> To me "branch" means or has heavier implications of "named branch"
>>>> than
>>>> just a merge base, and this command is perfectly happy to work on
>>>> commits disconnected from any named branch.
>>>>> But more to the point, the rebase docs for --onto discuss a "merge
>>>> base", so you'd read those, and then encounter this code talking about a
>>>> "branch base", and wonder what the difference was...
>>>
>>> Aren't the docs saying the merge base is the base of the commits
>>> (i.e. branch) being rebased? I don't think merge_base is a
>>> particularly helpful name as it doesn't tell us what it is the merge
>>> base of and branch_base was the best I could come up with. I see what
>>> you mean in the detached HEAD case, but as the command also works with
>>> named branches I hope it is fairly obvious what "branch_base" is in
>>> the detached HEAD case.
>> It *optionally* works with a <branch>, but doesn't require
>> one. E.g. try
>> this on git.git:
>
> Maybe I wasn't clear, I was referring to the fact that if HEAD isn't
> detached then it rebases the current branch not about the optional 
> <branch> argument. I also think that the docs are for users, they are
> not a guide to the code. With this change if you search for merge_base 
> in builtin/rebase.c you still find the part where we calculate the
> merge base. This commit was added in response to a review comment from
> Junio on V1, as far as I know he is happy with it and at this stage
> I'm disinclined to change it.

I was mainly fishing for whether *you* grokked that it wasn't always a
"merge base of a branch" in suggesting the rename. If you do & would
like to keep this it's fine by me.

Having said that.

I hadn't read the original discussion (or maybe I did, and
forgot). Re-reading you're referring to the discussion ending at[1].

I think using "base commit" in this case (your other suggestion) would
be better than either "merge base" or "branch base", just my 0.02.

In that case we could also use it consistently in our docs, and mention
that (in some cases) it's the "merge base", and in others something we
manually provide.

I agree that the docs are "not a guide to the code", and should not be
required to keep the two in sync at all times.

But just do be clear I do think it's a strong signal that the code is
being made more confusing if the variable that holds the "merge base"
now (as discussed in those terms in the docs) is named "branch base",
and we're seemingly unable (or at least I wasn't) to come up with a
corresponding doc change that doesn't start sounding self-contradictory.

I.e. given that the whole point of 5/8 is to come up with a more
descriptive name:

	merge_base is not a very descriptive name, the variable always holds
	the merge-base of 'branch' and 'onto' which is commit at the base of
	the branch being rebased so rename it to branch_base.

Cheers.

1. https://lore.kernel.org/git/c5b01472-7da9-6051-b127-1a8b8ddd2944@gmail.com/

>> 	git checkout origin/next
>> 	touch f && git add f && git commit -m"file"
>> 	git rebase --onto origin/master^{} HEAD~
>> Here we transplant a commit on top of "next" to "master", without
>> either
>> of those *names* being involved, or their branches, just the
>> corresponding OIDs/tips.
>> That will go through e.g. can_fast_forward() which you're modifying
>> here, and now populate a "branch_base" variable, instead of a
>> "merge_base".
>> I know that we conflate the meaning of "branch" somewhat, even in
>> our
>> own docs. E.g. we sometimes use "branch" and "named branch", but usually
>> by "branch" we mean "named branch", and otherwise talk about a detached
>> HEAD, <commit> or "tip".
>> But in this case it's especially confusing in the post-image,
>> because
>> "git rebase --onto" explicitly uses an optional "<branch>" to
>> distinguish the "named branch" case from the case where we're operating
>> on detached a HEAD, or otherwise don't care about the "<branch>" (except
>> as generic "restore us to where we were" behavior).
>> So, if anything I'd think that we'd want something like this in
>> various
>> places in git-rebase.txt to make the distinction clearer:
>> 	
>> 	diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> 	index 9cb8931c7ac..e4700a6e777 100644
>> 	--- a/Documentation/git-rebase.txt
>> 	+++ b/Documentation/git-rebase.txt
>> 	@@ -18,7 +18,7 @@ DESCRIPTION
>> 	 -----------
>> 	 If `<branch>` is specified, `git rebase` will perform an automatic
>> 	 `git switch <branch>` before doing anything else.  Otherwise
>> 	-it remains on the current branch.
>> 	+it remains on the current tip or named branch.
>> 	
>> 	 If `<upstream>` is not specified, the upstream configured in
>> 	 `branch.<name>.remote` and `branch.<name>.merge` options will be used (see
>> But your post-image seems to be to make this sort of thing
>> explicitly
>> more confusing, and e.g. these parts:
>> 	@@ -206,8 +206,8 @@ OPTIONS
>> 	 --onto <newbase>::
>> 	 	Starting point at which to create the new commits. If the
>> 	 	`--onto` option is not specified, the starting point is
>> 	-	`<upstream>`.  May be any valid commit, and not just an
>> 	-	existing branch name.
>> 	+	`<upstream>`.  May be any valid commit, and not just an <-- this
>> 	+	existing branch name. <--- this
>> 	 +
>> 	 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
>> To sum up why I find this confusing: Reading this from the docs
>> onwards
>> I'd think (as is the case) that "<branch>" is optional. Then when I read
>> the code I'd think a "branch_base" is something that *only* had to do
>> with the "<branch>" case.
>> But that's not the case, it's just a generic "merge base" in the
>> same
>> sense that "git merge-base" accepts all of these
>> 	$ git merge-base origin/master origin/next
>> 	d420dda0576340909c3faff364cfbd1485f70376
>> (These two are equivalent, just demo'ing that we don't need the peel
>> syntax):
>> 	$ git merge-base $(git rev-parse origin/master) $(git
>> rev-parse origin/next)
>> 	d420dda0576340909c3faff364cfbd1485f70376
>> 	$ git merge-base origin/master^{} origin/next^{}
>> 	d420dda0576340909c3faff364cfbd1485f70376
>> What *would* make things much clearer is e.g. calling a variable
>> "branch_merge_base" *if* there is a case where that's a merge base only
>> for named branches, but I don't know (and didn't look carefully enough)
>> if you've got such a case or cases here. It just seems like a generic
>> "merge-base".
>> 

Re: [PATCH v3 5/8] rebase: rename merge_base to branch_base

[Phillip Wood]

On 17/10/2022 17:19, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Oct 17 2022, Phillip Wood wrote:
> 
>> On 17/10/2022 12:27, Ævar Arnfjörð Bjarmason wrote:
>>> On Mon, Oct 17 2022, Phillip Wood wrote:
>>>
>>>> On 13/10/2022 20:16, Ævar Arnfjörð Bjarmason wrote:
>>>>> On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:
>>>>>
>>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>>
>>>>>> merge_base is not a very descriptive name, the variable always holds
>>>>>> the merge-base of 'branch' and 'onto' which is commit at the base of
>>>>>> the branch being rebased so rename it to branch_base.
>>>>> To me "branch" means or has heavier implications of "named branch"
>>>>> than
>>>>> just a merge base, and this command is perfectly happy to work on
>>>>> commits disconnected from any named branch.
>>>>>> But more to the point, the rebase docs for --onto discuss a "merge
>>>>> base", so you'd read those, and then encounter this code talking about a
>>>>> "branch base", and wonder what the difference was...
>>>>
>>>> Aren't the docs saying the merge base is the base of the commits
>>>> (i.e. branch) being rebased? I don't think merge_base is a
>>>> particularly helpful name as it doesn't tell us what it is the merge
>>>> base of and branch_base was the best I could come up with. I see what
>>>> you mean in the detached HEAD case, but as the command also works with
>>>> named branches I hope it is fairly obvious what "branch_base" is in
>>>> the detached HEAD case.
>>> It *optionally* works with a <branch>, but doesn't require
>>> one. E.g. try
>>> this on git.git:
>>
>> Maybe I wasn't clear, I was referring to the fact that if HEAD isn't
>> detached then it rebases the current branch not about the optional
>> <branch> argument. I also think that the docs are for users, they are
>> not a guide to the code. With this change if you search for merge_base
>> in builtin/rebase.c you still find the part where we calculate the
>> merge base. This commit was added in response to a review comment from
>> Junio on V1, as far as I know he is happy with it and at this stage
>> I'm disinclined to change it.
> 
> I was mainly fishing for whether *you* grokked that it wasn't always a
> "merge base of a branch" in suggesting the rename. If you do & would
> like to keep this it's fine by me.

Thanks, maybe base_commit would have been clearer but I'm not sure I can 
face re-rolling just for that.

Best Wishes

Phillip

> Having said that.
> 
> I hadn't read the original discussion (or maybe I did, and
> forgot). Re-reading you're referring to the discussion ending at[1].
> 
> I think using "base commit" in this case (your other suggestion) would
> be better than either "merge base" or "branch base", just my 0.02.
> 
> In that case we could also use it consistently in our docs, and mention
> that (in some cases) it's the "merge base", and in others something we
> manually provide.
> 
> I agree that the docs are "not a guide to the code", and should not be
> required to keep the two in sync at all times.
> 
> But just do be clear I do think it's a strong signal that the code is
> being made more confusing if the variable that holds the "merge base"
> now (as discussed in those terms in the docs) is named "branch base",
> and we're seemingly unable (or at least I wasn't) to come up with a
> corresponding doc change that doesn't start sounding self-contradictory.
> 
> I.e. given that the whole point of 5/8 is to come up with a more
> descriptive name:
> 
> 	merge_base is not a very descriptive name, the variable always holds
> 	the merge-base of 'branch' and 'onto' which is commit at the base of
> 	the branch being rebased so rename it to branch_base.
> 
> Cheers.
> 
> 1. https://lore.kernel.org/git/c5b01472-7da9-6051-b127-1a8b8ddd2944@gmail.com/
> 
>>> 	git checkout origin/next
>>> 	touch f && git add f && git commit -m"file"
>>> 	git rebase --onto origin/master^{} HEAD~
>>> Here we transplant a commit on top of "next" to "master", without
>>> either
>>> of those *names* being involved, or their branches, just the
>>> corresponding OIDs/tips.
>>> That will go through e.g. can_fast_forward() which you're modifying
>>> here, and now populate a "branch_base" variable, instead of a
>>> "merge_base".
>>> I know that we conflate the meaning of "branch" somewhat, even in
>>> our
>>> own docs. E.g. we sometimes use "branch" and "named branch", but usually
>>> by "branch" we mean "named branch", and otherwise talk about a detached
>>> HEAD, <commit> or "tip".
>>> But in this case it's especially confusing in the post-image,
>>> because
>>> "git rebase --onto" explicitly uses an optional "<branch>" to
>>> distinguish the "named branch" case from the case where we're operating
>>> on detached a HEAD, or otherwise don't care about the "<branch>" (except
>>> as generic "restore us to where we were" behavior).
>>> So, if anything I'd think that we'd want something like this in
>>> various
>>> places in git-rebase.txt to make the distinction clearer:
>>> 	
>>> 	diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>>> 	index 9cb8931c7ac..e4700a6e777 100644
>>> 	--- a/Documentation/git-rebase.txt
>>> 	+++ b/Documentation/git-rebase.txt
>>> 	@@ -18,7 +18,7 @@ DESCRIPTION
>>> 	 -----------
>>> 	 If `<branch>` is specified, `git rebase` will perform an automatic
>>> 	 `git switch <branch>` before doing anything else.  Otherwise
>>> 	-it remains on the current branch.
>>> 	+it remains on the current tip or named branch.
>>> 	
>>> 	 If `<upstream>` is not specified, the upstream configured in
>>> 	 `branch.<name>.remote` and `branch.<name>.merge` options will be used (see
>>> But your post-image seems to be to make this sort of thing
>>> explicitly
>>> more confusing, and e.g. these parts:
>>> 	@@ -206,8 +206,8 @@ OPTIONS
>>> 	 --onto <newbase>::
>>> 	 	Starting point at which to create the new commits. If the
>>> 	 	`--onto` option is not specified, the starting point is
>>> 	-	`<upstream>`.  May be any valid commit, and not just an
>>> 	-	existing branch name.
>>> 	+	`<upstream>`.  May be any valid commit, and not just an <-- this
>>> 	+	existing branch name. <--- this
>>> 	 +
>>> 	 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
>>> To sum up why I find this confusing: Reading this from the docs
>>> onwards
>>> I'd think (as is the case) that "<branch>" is optional. Then when I read
>>> the code I'd think a "branch_base" is something that *only* had to do
>>> with the "<branch>" case.
>>> But that's not the case, it's just a generic "merge base" in the
>>> same
>>> sense that "git merge-base" accepts all of these
>>> 	$ git merge-base origin/master origin/next
>>> 	d420dda0576340909c3faff364cfbd1485f70376
>>> (These two are equivalent, just demo'ing that we don't need the peel
>>> syntax):
>>> 	$ git merge-base $(git rev-parse origin/master) $(git
>>> rev-parse origin/next)
>>> 	d420dda0576340909c3faff364cfbd1485f70376
>>> 	$ git merge-base origin/master^{} origin/next^{}
>>> 	d420dda0576340909c3faff364cfbd1485f70376
>>> What *would* make things much clearer is e.g. calling a variable
>>> "branch_merge_base" *if* there is a case where that's a merge base only
>>> for named branches, but I don't know (and didn't look carefully enough)
>>> if you've got such a case or cases here. It just seems like a generic
>>> "merge-base".
>>>
> 

[PATCH v3 6/8] rebase: factor out branch_base calculation

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Separate out calculating the merge base between 'onto' and 'HEAD' from
the check for whether we can fast-forward or not. This means we can skip
the fast-forward checks when the rebase is forced and avoid calculating
the merge-base between 'HEAD' and 'onto' when --keep-base is given.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index cbafcc41e75..40619a0fb2d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
-	merge_bases = get_merge_bases(onto, head);
-	if (!merge_bases || merge_bases->next) {
-		oidcpy(branch_base, null_oid());
-		goto done;
-	}
+	if (is_null_oid(branch_base))
+		goto done; /* fill_branch_base() found multiple merge bases */
 
-	oidcpy(branch_base, &merge_bases->item->object.oid);
 	if (!oideq(branch_base, &onto->object.oid))
 		goto done;
 
@@ -887,7 +883,6 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	if (!upstream)
 		goto done;
 
-	free_commit_list(merge_bases);
 	merge_bases = get_merge_bases(upstream, head);
 	if (!merge_bases || merge_bases->next)
 		goto done;
@@ -902,6 +897,20 @@ done:
 	return res && is_linear_history(onto, head);
 }
 
+static void fill_branch_base(struct rebase_options *options,
+			    struct object_id *branch_base)
+{
+	struct commit_list *merge_bases = NULL;
+
+	merge_bases = get_merge_bases(options->onto, options->orig_head);
+	if (!merge_bases || merge_bases->next)
+		oidcpy(branch_base, null_oid());
+	else
+		oidcpy(branch_base, &merge_bases->item->object.oid);
+
+	free_commit_list(merge_bases);
+}
+
 static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 {
 	struct rebase_options *opts = opt->value;
@@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (!options.onto)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
+		fill_branch_base(&options, &branch_base);
 	}
-
 	if (options.fork_point > 0)
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
@@ -1698,13 +1707,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * Check if we are already based on onto with linear history,
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
-	 *
-	 * Note that can_fast_forward() initializes branch_base, so we have to
-	 * call it before checking allow_preemptive_ff.
 	 */
-	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &branch_base) &&
-	    allow_preemptive_ff) {
+	if (allow_preemptive_ff &&
+	    can_fast_forward(options.onto, options.upstream, options.restrict_revision,
+			     options.orig_head, &branch_base)) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {
-- 
gitgitgadget

Re: [PATCH v3 6/8] rebase: factor out branch_base calculation

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 13 2022, Phillip Wood via GitGitGadget wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Separate out calculating the merge base between 'onto' and 'HEAD' from
> the check for whether we can fast-forward or not. This means we can skip
> the fast-forward checks when the rebase is forced and avoid calculating
> the merge-base between 'HEAD' and 'onto' when --keep-base is given.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  builtin/rebase.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index cbafcc41e75..40619a0fb2d 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>  	struct commit_list *merge_bases = NULL;
>  	int res = 0;
>  
> -	merge_bases = get_merge_bases(onto, head);
> -	if (!merge_bases || merge_bases->next) {
> -		oidcpy(branch_base, null_oid());
> -		goto done;
> -	}
> +	if (is_null_oid(branch_base))
> +		goto done; /* fill_branch_base() found multiple merge bases */
>  
> -	oidcpy(branch_base, &merge_bases->item->object.oid);
>  	if (!oideq(branch_base, &onto->object.oid))
>  		goto done;
>  
> @@ -887,7 +883,6 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
>  	if (!upstream)
>  		goto done;
>  
> -	free_commit_list(merge_bases);
>  	merge_bases = get_merge_bases(upstream, head);
>  	if (!merge_bases || merge_bases->next)
>  		goto done;
> @@ -902,6 +897,20 @@ done:
>  	return res && is_linear_history(onto, head);
>  }
>  
> +static void fill_branch_base(struct rebase_options *options,
> +			    struct object_id *branch_base)
> +{
> +	struct commit_list *merge_bases = NULL;
> +
> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
> +	if (!merge_bases || merge_bases->next)
> +		oidcpy(branch_base, null_oid());
> +	else
> +		oidcpy(branch_base, &merge_bases->item->object.oid);
> +
> +	free_commit_list(merge_bases);
> +}

I wondered if this could be a bit shorter/less wrap-y with shorter
variable names, anyway, I see it's code copied from above, so nevermind
in advance... :)
	
	static void fill_branch_base(struct rebase_options *o, struct object_id *dst)
	{
		struct commit_list *mb = get_merge_bases(o->onto, o->orig_head);
		const struct object_id *src = (!mb || mb->next) ? null_oid() :
			&mb->item->object.oid;
	
		oidcpy(dst, src);
		free_commit_list(mb);
	}

	
> @@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		if (!options.onto)
>  			die(_("Does not point to a valid commit '%s'"),
>  				options.onto_name);
> +		fill_branch_base(&options, &branch_base);
>  	}
> -
>  	if (options.fork_point > 0)
>  		options.restrict_revision =
>  			get_fork_point(options.upstream_name, options.orig_head);

I wouldn't mind the stray whitespace change, but here it seems
unintentional, in 7/8 your change on top is:
	
	@@ -1680,6 +1691,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
	 				options.onto_name);
	 		fill_branch_base(&options, &branch_base);
	 	}
	+	if (keep_base && options.reapply_cherry_picks)
	+		options.upstream = options.onto;
	+
	 	if (options.fork_point > 0)
	 		options.restrict_revision =
	 			get_fork_point(options.upstream_name, options.orig_head);

Presumably we want to have \n\n spacing for both of those, and to not
remove the spacing here in 6/8, only to add it back?

Re: [PATCH v3 6/8] rebase: factor out branch_base calculation

[Phillip Wood]

On 13/10/2022 20:21, Ævar Arnfjörð Bjarmason wrote:
>> +static void fill_branch_base(struct rebase_options *options,
>> +			    struct object_id *branch_base)
>> +{
>> +	struct commit_list *merge_bases = NULL;
>> +
>> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
>> +	if (!merge_bases || merge_bases->next)
>> +		oidcpy(branch_base, null_oid());
>> +	else
>> +		oidcpy(branch_base, &merge_bases->item->object.oid);
>> +
>> +	free_commit_list(merge_bases);
>> +}
> 
> I wondered if this could be a bit shorter/less wrap-y

Where's the wrapping?

> with shorter
> variable names, anyway, I see it's code copied from above, so nevermind
> in advance... :)

As it is copied it is easier to review leaving it as is I think.
  	
> 	static void fill_branch_base(struct rebase_options *o, struct object_id *dst)
> 	{
> 		struct commit_list *mb = get_merge_bases(o->onto, o->orig_head);
> 		const struct object_id *src = (!mb || mb->next) ? null_oid() :
> 			&mb->item->object.oid;
> 	
> 		oidcpy(dst, src);
> 		free_commit_list(mb);
> 	}
> 
> 	
>> @@ -1669,8 +1678,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		if (!options.onto)
>>   			die(_("Does not point to a valid commit '%s'"),
>>   				options.onto_name);
>> +		fill_branch_base(&options, &branch_base);
>>   	}
>> -
>>   	if (options.fork_point > 0)
>>   		options.restrict_revision =
>>   			get_fork_point(options.upstream_name, options.orig_head);
> 
> I wouldn't mind the stray whitespace change, but here it seems
> unintentional, in 7/8 your change on top is:

Thanks, well spotted, I'm sure I've fixed this at least once already, I 
must have reintroduced it when fixing a rebase conflict. I'll fix it again.

Best Wishes

Phillip
	
> 	@@ -1680,6 +1691,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> 	 				options.onto_name);
> 	 		fill_branch_base(&options, &branch_base);
> 	 	}
> 	+	if (keep_base && options.reapply_cherry_picks)
> 	+		options.upstream = options.onto;
> 	+
> 	 	if (options.fork_point > 0)
> 	 		options.restrict_revision =
> 	 			get_fork_point(options.upstream_name, options.orig_head);
> 
> Presumably we want to have \n\n spacing for both of those, and to not
> remove the spacing here in 6/8, only to add it back?

Re: [PATCH v3 6/8] rebase: factor out branch_base calculation

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 17 2022, Phillip Wood wrote:

> On 13/10/2022 20:21, Ævar Arnfjörð Bjarmason wrote:
>>> +static void fill_branch_base(struct rebase_options *options,
>>> +			    struct object_id *branch_base)
>>> +{
>>> +	struct commit_list *merge_bases = NULL;
>>> +
>>> +	merge_bases = get_merge_bases(options->onto, options->orig_head);
>>> +	if (!merge_bases || merge_bases->next)
>>> +		oidcpy(branch_base, null_oid());
>>> +	else
>>> +		oidcpy(branch_base, &merge_bases->item->object.oid);
>>> +
>>> +	free_commit_list(merge_bases);
>>> +}
>> I wondered if this could be a bit shorter/less wrap-y
>
> Where's the wrapping?

Sorry about being unclear, I meant (but completely failed to get across)
that you seemed to be pre-declaring the "merge_bases" to avoid wrapping
the "get_merge_bases()" line.

But reading it again maybe it was just copied as-is from the
pre-image. In any case as we're moving this to a new function maybe a
fix-up to make it:

	struct commit_list *merge_bases = get_merge_bases(options->onto,
							  options->orig_head);

would be marginally easier to read, as we never use that NULL-init
(which again, is also an issue in the pre-image).

Anyway, if you want to keep this all as-is that's fine with me.

>> with shorter
>> variable names, anyway, I see it's code copied from above, so nevermind
>> in advance... :)
>
> As it is copied it is easier to review leaving it as is I think.

*nod*

[PATCH v3 7/8] rebase --keep-base: imply --reapply-cherry-picks

[Phillip Wood via GitGitGadget]

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     | 26 ++++++++++++++++----------
 builtin/rebase.c                 | 16 +++++++++++++++-
 t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
 3 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 080658c8710..ee6cdd56949 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -218,12 +218,14 @@ 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
 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.
+rebasing on top of the upstream but to keep the base commit as-is. As
+the base commit is unchanged this option implies `--reapply-cherry-picks`
+to avoid losing commits.
 +
 Although both this option and `--fork-point` find the merge base between
 `<upstream>` and `<branch>`, this option uses the merge base as the _starting
@@ -278,7 +280,8 @@ See also INCOMPATIBLE OPTIONS below.
 Note that commits which start empty are kept (unless `--no-keep-empty`
 is specified), and commits which are clean cherry-picks (as determined
 by `git log --cherry-mark ...`) are detected and dropped as a
-preliminary step (unless `--reapply-cherry-picks` is passed).
+preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
+passed).
 +
 See also INCOMPATIBLE OPTIONS below.
 
@@ -311,13 +314,16 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-By default (or if `--no-reapply-cherry-picks` is given), these commits
-will be automatically dropped.  Because this necessitates reading all
-upstream commits, this can be expensive in repos with a large number
-of upstream commits that need to be read.  When using the 'merge'
-backend, warnings will be issued for each dropped commit (unless
-`--quiet` is given). Advice will also be issued unless
-`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
+
+In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
+given), these commits will be automatically dropped.  Because this
+necessitates reading all upstream commits, this can be expensive in
+repositories with a large number of upstream commits that need to be
+read. When using the 'merge' backend, warnings will be issued for each
+dropped commit (unless `--quiet` is given). Advice will also be issued
+unless `advice.skippedCherryPicks` is set to false (see
+linkgit:git-config[1]).
+
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 40619a0fb2d..ecc965c8586 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1180,6 +1180,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 */
@@ -1239,6 +1240,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 to avoid losing
+	 * commits 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");
@@ -1415,7 +1422,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,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				options.onto_name);
 		fill_branch_base(&options, &branch_base);
 	}
+	if (keep_base && options.reapply_cherry_picks)
+		options.upstream = options.onto;
+
 	if (options.fork_point > 0)
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 01eb9513d6c..ea501f2b42b 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -199,6 +199,27 @@ test_expect_success 'rebase --keep-base requires a single merge base' '
 	grep "need exactly one merge base with branch" err
 '
 
+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
-- 
gitgitgadget

[PATCH v3 8/8] rebase --keep-base: imply --no-fork-point

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Given the name of the option it is confusing if --keep-base actually
changes the base of the branch without --fork-point being explicitly
given on the command line.

The combination of --keep-base with an explicit --fork-point is still
supported even though --fork-point means we do not keep the same base
if the upstream branch has been rewound.  We do this in case anyone is
relying on this behavior which is tested in t3431[1]

[1] https://lore.kernel.org/git/20200715032014.GA10818@generichostname/

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-rebase.txt | 8 ++++----
 builtin/rebase.c             | 6 ++++++
 t/t3431-rebase-fork-point.sh | 2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ee6cdd56949..1e2d543ced9 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 --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
+	`git rebase --reapply-cherry-picks --no-fork-point --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
@@ -452,9 +452,9 @@ When `--fork-point` is active, 'fork_point' will be used instead of
 <branch>` command (see linkgit:git-merge-base[1]).  If 'fork_point'
 ends up being empty, the `<upstream>` will be used as a fallback.
 +
-If `<upstream>` is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`. See also
-`rebase.forkpoint` in linkgit:git-config[1].
+If `<upstream>` or `--keep-base` is given on the command line, then
+the default is `--no-fork-point`, otherwise the default is
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
 +
 If your branch was based on `<upstream>` but `<upstream>` was rewound and
 your branch contains commits which were dropped, this option can be used
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ecc965c8586..df8f7a8a5db 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1239,6 +1239,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
 		if (options.root)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
+		/*
+		 * --keep-base defaults to --no-fork-point to keep the
+		 * base the same.
+		 */
+		if (options.fork_point < 0)
+			options.fork_point = 0;
 	}
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 1d0b15380ed..70e81363569 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -50,7 +50,7 @@ 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 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
-- 
gitgitgadget

[PATCH v4 0/8] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

[Phillip Wood via GitGitGadget]

A while a go Philippe reported [1] that he was surprised 'git rebase
--keep-base' removed commits that had been cherry-picked upstream even
though to branch was not being rebased. I think it is also surprising if
'--keep-base' changes the base of the branch without '--fork-point' being
explicitly given on the command line. This series therefore changes the
default behavior of '--keep-base' to imply '--reapply-cherry-picks' and
'--no-fork-point' so that the base of the branch is unchanged and no commits
are removed.

Thanks to Junio and Ævar for their comments, the changes since V3 are:

 * Patch 3: added lookup_commit_object() that works like
   lookup_commit_reference() but without dereferencing tags.
 * Patch 4: changed lookup_commit_reference() to lookup_commit_object()
 * Patch 5: unchanged - Ævar has concerns about renaming merge_base to
   branch_base but I think this is an improvement overall.
 * Patches 6 & 8: fixed a whitespace issue

Thanks to Junio for his comments, the changes since V2 are:

 * Patch 3 new patch to make sure we're reading hex oids from state files
 * Patch 4 restored the call to read_refs() to avoid the dwim behavior of
   lookup_commit_reference_by_name()
 * Patch 6 added a comment to clarify what a null oid branch_base means

Thanks to everyone who commented for their reviews, the changes since V1
are:

 * Patch 1: new patch to tighten a couple of existing tests
 * Patch 2: reworded commit message in response to Junio's comments
 * Patch 3: fixed a typo in the commit message spotted by Elijah and tidied
   code formatting
 * Patch 4: new patch to rename a variable suggested by Junio
 * Patch 5: clarified commit message and removed some redundant code spotted
   by Junio
 * Patch 6: improved --reapply-cherry-picks documentation to mention
   --keep-base and vice-versa suggested by Philippe
 * Patch 7: expanded the commit message and documentation in response to
   Junio's comments

[1]
https://lore.kernel.org/git/0EA8C067-5805-40A7-857A-55C2633B8570@gmail.com/

Phillip Wood (8):
  t3416: tighten two tests
  t3416: set $EDITOR in subshell
  rebase: be stricter when reading state files containing oids
  rebase: store orig_head as a commit
  rebase: rename merge_base to branch_base
  rebase: factor out branch_base calculation
  rebase --keep-base: imply --reapply-cherry-picks
  rebase --keep-base: imply --no-fork-point

 Documentation/git-rebase.txt     |  32 ++++---
 builtin/rebase.c                 | 144 ++++++++++++++++++-------------
 commit.c                         |   8 ++
 commit.h                         |  13 +++
 t/t3416-rebase-onto-threedots.sh |  62 ++++++++++---
 t/t3431-rebase-fork-point.sh     |   2 +-
 6 files changed, 172 insertions(+), 89 deletions(-)


base-commit: afa70145a25e81faa685dc0b465e52b45d2444bd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1323%2Fphillipwood%2Fwip%2Frebase--keep-base-tweaks-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1323/phillipwood/wip/rebase--keep-base-tweaks-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1323

Range-diff vs v3:

 1:  12fb0ac6d5d = 1:  12fb0ac6d5d t3416: tighten two tests
 2:  d6f2f716c77 = 2:  d6f2f716c77 t3416: set $EDITOR in subshell
 3:  1fd58520253 ! 3:  1d5e0419c45 rebase: be stricter when reading state files containing oids
     @@ Commit message
      
          The state files for 'onto' and 'orig_head' should contain a full hex
          oid, change the reading functions from get_oid() to get_oid_hex() to
     -    reflect this.
     +    reflect this. They should also name commits and not tags so add and use
     +    a function that looks up a commit from an oid like
     +    lookup_commit_reference() but without dereferencing tags.
      
          Suggested-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     @@ builtin/rebase.c: static int read_basic_state(struct rebase_options *opts)
       		xstrdup(head_name.buf) : NULL;
       	strbuf_release(&head_name);
      -	if (get_oid(buf.buf, &oid))
     -+	if (get_oid_hex(buf.buf, &oid))
     - 		return error(_("could not get 'onto': '%s'"), buf.buf);
     - 	opts->onto = lookup_commit_or_die(&oid, buf.buf);
     +-		return error(_("could not get 'onto': '%s'"), buf.buf);
     +-	opts->onto = lookup_commit_or_die(&oid, buf.buf);
     ++	if (get_oid_hex(buf.buf, &oid) ||
     ++	    !(opts->onto = lookup_commit_object(the_repository, &oid)))
     ++		return error(_("invalid onto: '%s'"), buf.buf);
       
     + 	/*
     + 	 * We always write to orig-head, but interactive rebase used to write to
      @@ builtin/rebase.c: static int read_basic_state(struct rebase_options *opts)
       	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
       				  READ_ONELINER_WARN_MISSING))
     @@ builtin/rebase.c: static int read_basic_state(struct rebase_options *opts)
       		return error(_("invalid orig-head: '%s'"), buf.buf);
       
       	if (file_exists(state_dir_path("quiet", opts)))
     +
     + ## commit.c ##
     +@@ commit.c: struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
     + 	return c;
     + }
     + 
     ++struct commit *lookup_commit_object (struct repository *r,
     ++				     const struct object_id *oid)
     ++{
     ++	struct object *obj = parse_object(r, oid);
     ++	return obj ? object_as_type(obj, OBJ_COMMIT, 0) : NULL;
     ++
     ++}
     ++
     + struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
     + {
     + 	struct object *obj = lookup_object(r, oid);
     +
     + ## commit.h ##
     +@@ commit.h: enum decoration_type {
     + void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
     + const struct name_decoration *get_name_decoration(const struct object *obj);
     + 
     ++/*
     ++ * Look up commit named by "oid" respecting replacement objects.
     ++ * Returns NULL if "oid" is not a commit or does not exist.
     ++ */
     ++struct commit *lookup_commit_object(struct repository *r, const struct object_id *oid);
     ++
     ++/*
     ++ * Look up commit named by "oid" without replacement objects or
     ++ * checking for object existence. Returns the requested commit if it
     ++ * is found in the object cache, NULL if "oid" is in the object cache
     ++ * but is not a commit and a newly allocated unparsed commit object if
     ++ * "oid" is not in the object cache.
     ++ */
     + struct commit *lookup_commit(struct repository *r, const struct object_id *oid);
     + struct commit *lookup_commit_reference(struct repository *r,
     + 				       const struct object_id *oid);
 4:  dc056b13ed5 ! 4:  22f3d265b57 rebase: store orig_head as a commit
     @@ Commit message
          oid to a commit.
      
          To avoid changing the behavior of "git rebase <upstream> <branch>" we
     -    keep the existing call to read_ref() and use lookup_commit_reference()
     +    keep the existing call to read_ref() and use lookup_commit_object()
          on the oid returned by that rather than calling
          lookup_commit_reference_by_name() which applies the ref dwim rules to
     -    its argument. lookup_commit_reference() will dereference tag objects
     -    but we do not expect the branch being rebased to be pointing to a tag
     -    object.
     +    its argument.
      
          Helped-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     @@ builtin/rebase.c: static int read_basic_state(struct rebase_options *opts)
       		return -1;
      -	if (get_oid_hex(buf.buf, &opts->orig_head))
      +	if (get_oid_hex(buf.buf, &oid) ||
     -+	    !(opts->orig_head = lookup_commit_reference(the_repository, &oid)))
     ++	    !(opts->orig_head = lookup_commit_object(the_repository, &oid)))
       		return error(_("invalid orig-head: '%s'"), buf.buf);
       
       	if (file_exists(state_dir_path("quiet", opts)))
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       			die_if_checked_out(buf.buf, 1);
       			options.head_name = xstrdup(buf.buf);
      +			options.orig_head =
     -+				lookup_commit_reference(the_repository,
     -+							&branch_oid);
     ++				lookup_commit_object(the_repository,
     ++						     &branch_oid);
       		/* If not is it a valid ref (branch or commit)? */
       		} else {
      -			struct commit *commit =
 5:  00f70c90344 = 5:  79a8c0fe284 rebase: rename merge_base to branch_base
 6:  2efbfc94187 ! 6:  bd24409a266 rebase: factor out branch_base calculation
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       				options.onto_name);
      +		fill_branch_base(&options, &branch_base);
       	}
     --
     + 
       	if (options.fork_point > 0)
     - 		options.restrict_revision =
     - 			get_fork_point(options.upstream_name, options.orig_head);
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
       	 * Check if we are already based on onto with linear history,
       	 * in which case we could fast-forward without replacing the commits
 7:  bc39c76b217 ! 7:  367e44c6928 rebase --keep-base: imply --reapply-cherry-picks
     @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix
       
       	if (gpg_sign)
      @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)
     - 				options.onto_name);
       		fill_branch_base(&options, &branch_base);
       	}
     + 
      +	if (keep_base && options.reapply_cherry_picks)
      +		options.upstream = options.onto;
      +
 8:  4d0226e1dcc = 8:  656b9c9dab6 rebase --keep-base: imply --no-fork-point

-- 
gitgitgadget

[PATCH v4 1/8] t3416: tighten two tests

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add a check for the correct error message to the tests that check we
require a single merge base so we can be sure the rebase failed for
the correct reason. Also rename the tests to reflect what they are
testing.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 3e04802cb00..dc67d2308f3 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -97,13 +97,14 @@ test_expect_success 'rebase -i --onto main...' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --onto main...side' '
+test_expect_success 'rebase --onto main...side requires a single merge-base' '
 	git reset --hard &&
 	git checkout side &&
 	git reset --hard K &&
 
 	set_fake_editor &&
-	test_must_fail git rebase -i --onto main...side J
+	test_must_fail git rebase -i --onto main...side J 2>err &&
+	grep "need exactly one merge base" err
 '
 
 test_expect_success 'rebase --keep-base --onto incompatible' '
@@ -182,13 +183,14 @@ test_expect_success 'rebase -i --keep-base main topic from main' '
 	test_cmp expect actual
 '
 
-test_expect_success 'rebase -i --keep-base main from side' '
+test_expect_success 'rebase --keep-base requires a single merge base' '
 	git reset --hard &&
 	git checkout side &&
 	git reset --hard K &&
 
 	set_fake_editor &&
-	test_must_fail git rebase -i --keep-base main
+	test_must_fail git rebase -i --keep-base main 2>err &&
+	grep "need exactly one merge base with branch" err
 '
 
 test_done
-- 
gitgitgadget

[PATCH v4 2/8] t3416: set $EDITOR in subshell

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As $EDITOR is exported, setting it in one test affects all subsequent
tests. Avoid this by always setting it in a subshell. Also remove a
couple of unnecessary call to set_fake_editor where the editor does
not change the todo list.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3416-rebase-onto-threedots.sh | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index dc67d2308f3..01eb9513d6c 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -79,8 +79,10 @@ test_expect_success 'rebase -i --onto main...topic' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main...topic F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main...topic F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -90,8 +92,10 @@ test_expect_success 'rebase -i --onto main...' '
 	git reset --hard &&
 	git checkout topic &&
 	git reset --hard G &&
-	set_fake_editor &&
-	EXPECT_COUNT=1 git rebase -i --onto main... F &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=1 git rebase -i --onto main... F
+	) &&
 	git rev-parse HEAD^1 >actual &&
 	git rev-parse C^0 >expect &&
 	test_cmp expect actual
@@ -102,7 +106,6 @@ test_expect_success 'rebase --onto main...side requires a single merge-base' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --onto main...side J 2>err &&
 	grep "need exactly one merge base" err
 '
@@ -157,8 +160,10 @@ test_expect_success 'rebase -i --keep-base main from topic' '
 	git checkout topic &&
 	git reset --hard G &&
 
-	set_fake_editor &&
-	EXPECT_COUNT=2 git rebase -i --keep-base main &&
+	(
+		set_fake_editor &&
+		EXPECT_COUNT=2 git rebase -i --keep-base main
+	) &&
 	git rev-parse C >base.expect &&
 	git merge-base main HEAD >base.actual &&
 	test_cmp base.expect base.actual &&
@@ -172,8 +177,10 @@ 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 &&
+	(
+		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 &&
@@ -188,9 +195,13 @@ test_expect_success 'rebase --keep-base requires a single merge base' '
 	git checkout side &&
 	git reset --hard K &&
 
-	set_fake_editor &&
 	test_must_fail git rebase -i --keep-base main 2>err &&
 	grep "need exactly one merge base with branch" err
 '
 
+# This must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
 test_done
-- 
gitgitgadget

[PATCH v4 3/8] rebase: be stricter when reading state files containing oids

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

The state files for 'onto' and 'orig_head' should contain a full hex
oid, change the reading functions from get_oid() to get_oid_hex() to
reflect this. They should also name commits and not tags so add and use
a function that looks up a commit from an oid like
lookup_commit_reference() but without dereferencing tags.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c |  8 ++++----
 commit.c         |  8 ++++++++
 commit.h         | 13 +++++++++++++
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 56e4214b441..06903eb6d4d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -431,9 +431,9 @@ static int read_basic_state(struct rebase_options *opts)
 	opts->head_name = starts_with(head_name.buf, "refs/") ?
 		xstrdup(head_name.buf) : NULL;
 	strbuf_release(&head_name);
-	if (get_oid(buf.buf, &oid))
-		return error(_("could not get 'onto': '%s'"), buf.buf);
-	opts->onto = lookup_commit_or_die(&oid, buf.buf);
+	if (get_oid_hex(buf.buf, &oid) ||
+	    !(opts->onto = lookup_commit_object(the_repository, &oid)))
+		return error(_("invalid onto: '%s'"), buf.buf);
 
 	/*
 	 * We always write to orig-head, but interactive rebase used to write to
@@ -448,7 +448,7 @@ static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid(buf.buf, &opts->orig_head))
+	if (get_oid_hex(buf.buf, &opts->orig_head))
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
diff --git a/commit.c b/commit.c
index 0db461f9735..de30b098dd3 100644
--- a/commit.c
+++ b/commit.c
@@ -59,6 +59,14 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
 	return c;
 }
 
+struct commit *lookup_commit_object (struct repository *r,
+				     const struct object_id *oid)
+{
+	struct object *obj = parse_object(r, oid);
+	return obj ? object_as_type(obj, OBJ_COMMIT, 0) : NULL;
+
+}
+
 struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
 {
 	struct object *obj = lookup_object(r, oid);
diff --git a/commit.h b/commit.h
index 21e4d25ce78..fa39202fa6b 100644
--- a/commit.h
+++ b/commit.h
@@ -64,6 +64,19 @@ enum decoration_type {
 void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
 const struct name_decoration *get_name_decoration(const struct object *obj);
 
+/*
+ * Look up commit named by "oid" respecting replacement objects.
+ * Returns NULL if "oid" is not a commit or does not exist.
+ */
+struct commit *lookup_commit_object(struct repository *r, const struct object_id *oid);
+
+/*
+ * Look up commit named by "oid" without replacement objects or
+ * checking for object existence. Returns the requested commit if it
+ * is found in the object cache, NULL if "oid" is in the object cache
+ * but is not a commit and a newly allocated unparsed commit object if
+ * "oid" is not in the object cache.
+ */
 struct commit *lookup_commit(struct repository *r, const struct object_id *oid);
 struct commit *lookup_commit_reference(struct repository *r,
 				       const struct object_id *oid);
-- 
gitgitgadget

Re: [PATCH v4 3/8] rebase: be stricter when reading state files containing oids

[Junio C Hamano]

> diff --git a/commit.c b/commit.c
> index 0db461f9735..de30b098dd3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -59,6 +59,14 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
>  	return c;
>  }
>  
> +struct commit *lookup_commit_object (struct repository *r,
> +				     const struct object_id *oid)

Let's lose the SP before opening parenthesis (I'll do that while
queuing unless I forget ;-).

> +{
> +	struct object *obj = parse_object(r, oid);
> +	return obj ? object_as_type(obj, OBJ_COMMIT, 0) : NULL;
> +
> +}
> +
>  struct commit *lookup_commit(struct repository *r, const struct object_id *oid)
>  {
>  	struct object *obj = lookup_object(r, oid);
> diff --git a/commit.h b/commit.h
> index 21e4d25ce78..fa39202fa6b 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -64,6 +64,19 @@ enum decoration_type {
>  void add_name_decoration(enum decoration_type type, const char *name, struct object *obj);
>  const struct name_decoration *get_name_decoration(const struct object *obj);
>  
> +/*
> + * Look up commit named by "oid" respecting replacement objects.
> + * Returns NULL if "oid" is not a commit or does not exist.
> + */
> +struct commit *lookup_commit_object(struct repository *r, const struct object_id *oid);
> +
> +/*
> + * Look up commit named by "oid" without replacement objects or
> + * checking for object existence. Returns the requested commit if it
> + * is found in the object cache, NULL if "oid" is in the object cache
> + * but is not a commit and a newly allocated unparsed commit object if
> + * "oid" is not in the object cache.
> + */
>  struct commit *lookup_commit(struct repository *r, const struct object_id *oid);

I was going to ask documenting the differences of the two API
functions, which is done here.  Good.

>  struct commit *lookup_commit_reference(struct repository *r,
>  				       const struct object_id *oid);

Re: [PATCH v4 3/8] rebase: be stricter when reading state files containing oids

[Phillip Wood]

On 17/10/2022 19:51, Junio C Hamano wrote:
>> diff --git a/commit.c b/commit.c
>> index 0db461f9735..de30b098dd3 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -59,6 +59,14 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref
>>   	return c;
>>   }
>>   
>> +struct commit *lookup_commit_object (struct repository *r,
>> +				     const struct object_id *oid)
> 
> Let's lose the SP before opening parenthesis (I'll do that while
> queuing unless I forget ;-).

Sorry, thanks for fixing it up

Phillip

[PATCH v4 4/8] rebase: store orig_head as a commit

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Using a struct commit rather than a struct oid to hold orig_head means
that we error out straight away if the branch being rebased does not
point to a commit. It also simplifies the code that handles finding
the merge base and fork point as it no longer has to convert from an
oid to a commit.

To avoid changing the behavior of "git rebase <upstream> <branch>" we
keep the existing call to read_ref() and use lookup_commit_object()
on the oid returned by that rather than calling
lookup_commit_reference_by_name() which applies the ref dwim rules to
its argument.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 67 ++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 36 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 06903eb6d4d..211e9139f2e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -68,7 +68,7 @@ struct rebase_options {
 	const char *upstream_name;
 	const char *upstream_arg;
 	char *head_name;
-	struct object_id orig_head;
+	struct commit *orig_head;
 	struct commit *onto;
 	const char *onto_name;
 	const char *revisions;
@@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 	struct replay_opts replay = get_replay_opts(opts);
 	struct string_list commands = STRING_LIST_INIT_DUP;
 
-	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head,
+	if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid,
 				&revisions, &shortrevisions))
 		return -1;
 
 	if (init_basic_state(&replay,
 			     opts->head_name ? opts->head_name : "detached HEAD",
-			     opts->onto, &opts->orig_head)) {
+			     opts->onto, &opts->orig_head->object.oid)) {
 		free(revisions);
 		free(shortrevisions);
 
@@ -298,9 +298,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		split_exec_commands(opts->cmd, &commands);
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
-			&opts->orig_head, &commands, opts->autosquash,
-			opts->update_refs,
-			&todo_list);
+			&opts->orig_head->object.oid, &commands,
+			opts->autosquash, opts->update_refs, &todo_list);
 	}
 
 	string_list_clear(&commands, 0);
@@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts)
 	} else if (!read_oneliner(&buf, state_dir_path("head", opts),
 				  READ_ONELINER_WARN_MISSING))
 		return -1;
-	if (get_oid_hex(buf.buf, &opts->orig_head))
+	if (get_oid_hex(buf.buf, &oid) ||
+	    !(opts->orig_head = lookup_commit_object(the_repository, &oid)))
 		return error(_("invalid orig-head: '%s'"), buf.buf);
 
 	if (file_exists(state_dir_path("quiet", opts)))
@@ -517,7 +517,7 @@ static int rebase_write_basic_state(struct rebase_options *opts)
 	write_file(state_dir_path("onto", opts), "%s",
 		   opts->onto ? oid_to_hex(&opts->onto->object.oid) : "");
 	write_file(state_dir_path("orig-head", opts), "%s",
-		   oid_to_hex(&opts->orig_head));
+		   oid_to_hex(&opts->orig_head->object.oid));
 	if (!(opts->flags & REBASE_NO_QUIET))
 		write_file(state_dir_path("quiet", opts), "%s", "");
 	if (opts->flags & REBASE_VERBOSE)
@@ -646,7 +646,7 @@ static int run_am(struct rebase_options *opts)
 			       /* this is now equivalent to !opts->upstream */
 			       &opts->onto->object.oid :
 			       &opts->upstream->object.oid),
-		    oid_to_hex(&opts->orig_head));
+		    oid_to_hex(&opts->orig_head->object.oid));
 
 	rebased_patches = xstrdup(git_path("rebased-patches"));
 	format_patch.out = open(rebased_patches,
@@ -680,7 +680,7 @@ static int run_am(struct rebase_options *opts)
 		free(rebased_patches);
 		strvec_clear(&am.args);
 
-		ropts.oid = &opts->orig_head;
+		ropts.oid = &opts->orig_head->object.oid;
 		ropts.branch = opts->head_name;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		reset_head(the_repository, &ropts);
@@ -833,7 +833,7 @@ static int checkout_up_to_date(struct rebase_options *options)
 	strbuf_addf(&buf, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);
-	ropts.oid = &options->orig_head;
+	ropts.oid = &options->orig_head->object.oid;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	if (!ropts.branch)
@@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct object_id *head_oid, struct object_id *merge_base)
+			    struct commit *head, 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());
@@ -1312,13 +1308,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 		if (read_basic_state(&options))
 			exit(1);
-		ropts.oid = &options.orig_head;
+		ropts.oid = &options.orig_head->object.oid;
 		ropts.branch = options.head_name;
 		ropts.flags = RESET_HEAD_HARD;
 		ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
 		if (reset_head(the_repository, &ropts) < 0)
 			die(_("could not move back to %s"),
-			    oid_to_hex(&options.orig_head));
+			    oid_to_hex(&options.orig_head->object.oid));
 		remove_branch_state(the_repository, 0);
 		ret = finish_rebase(&options);
 		goto cleanup;
@@ -1604,25 +1600,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 */
 	if (argc == 1) {
 		/* Is it "rebase other branchname" or "rebase other commit"? */
+		struct object_id branch_oid;
 		branch_name = argv[0];
 		options.switch_to = argv[0];
 
 		/* Is it a local branch? */
 		strbuf_reset(&buf);
 		strbuf_addf(&buf, "refs/heads/%s", branch_name);
-		if (!read_ref(buf.buf, &options.orig_head)) {
+		if (!read_ref(buf.buf, &branch_oid)) {
 			die_if_checked_out(buf.buf, 1);
 			options.head_name = xstrdup(buf.buf);
+			options.orig_head =
+				lookup_commit_object(the_repository,
+						     &branch_oid);
 		/* If not is it a valid ref (branch or commit)? */
 		} else {
-			struct commit *commit =
+			options.orig_head =
 				lookup_commit_reference_by_name(branch_name);
-			if (!commit)
-				die(_("no such branch/commit '%s'"),
-				    branch_name);
-			oidcpy(&options.orig_head, &commit->object.oid);
 			options.head_name = NULL;
 		}
+		if (!options.orig_head)
+			die(_("no such branch/commit '%s'"), branch_name);
 	} else if (argc == 0) {
 		/* Do not need to switch branches, we are already on it. */
 		options.head_name =
@@ -1639,8 +1637,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			FREE_AND_NULL(options.head_name);
 			branch_name = "HEAD";
 		}
-		if (get_oid("HEAD", &options.orig_head))
-			die(_("Could not resolve HEAD to a revision"));
+		options.orig_head = lookup_commit_reference_by_name("HEAD");
+		if (!options.orig_head)
+			die(_("Could not resolve HEAD to a commit"));
 	} else
 		BUG("unexpected number of arguments left to parse");
 
@@ -1672,13 +1671,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				options.onto_name);
 	}
 
-	if (options.fork_point > 0) {
-		struct commit *head =
-			lookup_commit_reference(the_repository,
-						&options.orig_head);
+	if (options.fork_point > 0)
 		options.restrict_revision =
-			get_fork_point(options.upstream_name, head);
-	}
+			get_fork_point(options.upstream_name, options.orig_head);
 
 	if (repo_read_index(the_repository) < 0)
 		die(_("could not read index"));
@@ -1708,7 +1703,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    &options.orig_head, &merge_base) &&
+		    options.orig_head, &merge_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1785,7 +1780,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	strbuf_addf(&msg, "%s: checkout %s",
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);
 	ropts.oid = &options.onto->object.oid;
-	ropts.orig_head = &options.orig_head,
+	ropts.orig_head = &options.orig_head->object.oid,
 	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
 			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
 	ropts.head_msg = msg.buf;
@@ -1799,7 +1794,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head)) {
+	if (oideq(&merge_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
@@ -1820,7 +1815,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		    (options.restrict_revision ?
 		     oid_to_hex(&options.restrict_revision->object.oid) :
 		     oid_to_hex(&options.upstream->object.oid)),
-		    oid_to_hex(&options.orig_head));
+		    oid_to_hex(&options.orig_head->object.oid));
 
 	options.revisions = revisions.buf;
 
-- 
gitgitgadget

[PATCH v4 5/8] rebase: rename merge_base to branch_base

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

merge_base is not a very descriptive name, the variable always holds
the merge-base of 'branch' and 'onto' which is commit at the base of
the branch being rebased so rename it to branch_base.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 211e9139f2e..d70b9b248fa 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -866,22 +866,22 @@ static int is_linear_history(struct commit *from, struct commit *to)
 
 static int can_fast_forward(struct commit *onto, struct commit *upstream,
 			    struct commit *restrict_revision,
-			    struct commit *head, struct object_id *merge_base)
+			    struct commit *head, struct object_id *branch_base)
 {
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
 	merge_bases = get_merge_bases(onto, head);
 	if (!merge_bases || merge_bases->next) {
-		oidcpy(merge_base, null_oid());
+		oidcpy(branch_base, null_oid());
 		goto done;
 	}
 
-	oidcpy(merge_base, &merge_bases->item->object.oid);
-	if (!oideq(merge_base, &onto->object.oid))
+	oidcpy(branch_base, &merge_bases->item->object.oid);
+	if (!oideq(branch_base, &onto->object.oid))
 		goto done;
 
-	if (restrict_revision && !oideq(&restrict_revision->object.oid, merge_base))
+	if (restrict_revision && !oideq(&restrict_revision->object.oid, branch_base))
 		goto done;
 
 	if (!upstream)
@@ -1035,7 +1035,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf msg = STRBUF_INIT;
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
-	struct object_id merge_base;
+	struct object_id branch_base;
 	int ignore_whitespace = 0;
 	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
@@ -1653,7 +1653,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	} 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 (get_oid_mb(options.onto_name, &branch_base) < 0) {
 			if (keep_base)
 				die(_("'%s': need exactly one merge base with branch"),
 				    options.upstream_name);
@@ -1661,7 +1661,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 				die(_("'%s': need exactly one merge base"),
 				    options.onto_name);
 		}
-		options.onto = lookup_commit_or_die(&merge_base,
+		options.onto = lookup_commit_or_die(&branch_base,
 						    options.onto_name);
 	} else {
 		options.onto =
@@ -1699,11 +1699,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
 	 *
-	 * Note that can_fast_forward() initializes merge_base, so we have to
+	 * Note that can_fast_forward() initializes branch_base, so we have to
 	 * call it before checking allow_preemptive_ff.
 	 */
 	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &merge_base) &&
+		    options.orig_head, &branch_base) &&
 	    allow_preemptive_ff) {
 		int flag;
 
@@ -1745,12 +1745,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct diff_options opts;
 
 		if (options.flags & REBASE_VERBOSE) {
-			if (is_null_oid(&merge_base))
+			if (is_null_oid(&branch_base))
 				printf(_("Changes to %s:\n"),
 				       oid_to_hex(&options.onto->object.oid));
 			else
 				printf(_("Changes from %s to %s:\n"),
-				       oid_to_hex(&merge_base),
+				       oid_to_hex(&branch_base),
 				       oid_to_hex(&options.onto->object.oid));
 		}
 
@@ -1762,8 +1762,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
 		opts.detect_rename = DIFF_DETECT_RENAME;
 		diff_setup_done(&opts);
-		diff_tree_oid(is_null_oid(&merge_base) ?
-			      the_hash_algo->empty_tree : &merge_base,
+		diff_tree_oid(is_null_oid(&branch_base) ?
+			      the_hash_algo->empty_tree : &branch_base,
 			      &options.onto->object.oid, "", &opts);
 		diffcore_std(&opts);
 		diff_flush(&opts);
@@ -1794,7 +1794,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (oideq(&merge_base, &options.orig_head->object.oid)) {
+	if (oideq(&branch_base, &options.orig_head->object.oid)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",
-- 
gitgitgadget

[PATCH v4 6/8] rebase: factor out branch_base calculation

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Separate out calculating the merge base between 'onto' and 'HEAD' from
the check for whether we can fast-forward or not. This means we can skip
the fast-forward checks when the rebase is forced and avoid calculating
the merge-base between 'HEAD' and 'onto' when --keep-base is given.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/rebase.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index d70b9b248fa..90ae8fd8de7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 
-	merge_bases = get_merge_bases(onto, head);
-	if (!merge_bases || merge_bases->next) {
-		oidcpy(branch_base, null_oid());
-		goto done;
-	}
+	if (is_null_oid(branch_base))
+		goto done; /* fill_branch_base() found multiple merge bases */
 
-	oidcpy(branch_base, &merge_bases->item->object.oid);
 	if (!oideq(branch_base, &onto->object.oid))
 		goto done;
 
@@ -887,7 +883,6 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	if (!upstream)
 		goto done;
 
-	free_commit_list(merge_bases);
 	merge_bases = get_merge_bases(upstream, head);
 	if (!merge_bases || merge_bases->next)
 		goto done;
@@ -902,6 +897,20 @@ done:
 	return res && is_linear_history(onto, head);
 }
 
+static void fill_branch_base(struct rebase_options *options,
+			    struct object_id *branch_base)
+{
+	struct commit_list *merge_bases = NULL;
+
+	merge_bases = get_merge_bases(options->onto, options->orig_head);
+	if (!merge_bases || merge_bases->next)
+		oidcpy(branch_base, null_oid());
+	else
+		oidcpy(branch_base, &merge_bases->item->object.oid);
+
+	free_commit_list(merge_bases);
+}
+
 static int parse_opt_am(const struct option *opt, const char *arg, int unset)
 {
 	struct rebase_options *opts = opt->value;
@@ -1669,6 +1678,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		if (!options.onto)
 			die(_("Does not point to a valid commit '%s'"),
 				options.onto_name);
+		fill_branch_base(&options, &branch_base);
 	}
 
 	if (options.fork_point > 0)
@@ -1698,13 +1708,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * Check if we are already based on onto with linear history,
 	 * in which case we could fast-forward without replacing the commits
 	 * with new commits recreated by replaying their changes.
-	 *
-	 * Note that can_fast_forward() initializes branch_base, so we have to
-	 * call it before checking allow_preemptive_ff.
 	 */
-	if (can_fast_forward(options.onto, options.upstream, options.restrict_revision,
-		    options.orig_head, &branch_base) &&
-	    allow_preemptive_ff) {
+	if (allow_preemptive_ff &&
+	    can_fast_forward(options.onto, options.upstream, options.restrict_revision,
+			     options.orig_head, &branch_base)) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {
-- 
gitgitgadget

[PATCH v4 7/8] rebase --keep-base: imply --reapply-cherry-picks

[Phillip Wood via GitGitGadget]

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     | 26 ++++++++++++++++----------
 builtin/rebase.c                 | 16 +++++++++++++++-
 t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
 3 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 080658c8710..ee6cdd56949 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -218,12 +218,14 @@ 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
 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.
+rebasing on top of the upstream but to keep the base commit as-is. As
+the base commit is unchanged this option implies `--reapply-cherry-picks`
+to avoid losing commits.
 +
 Although both this option and `--fork-point` find the merge base between
 `<upstream>` and `<branch>`, this option uses the merge base as the _starting
@@ -278,7 +280,8 @@ See also INCOMPATIBLE OPTIONS below.
 Note that commits which start empty are kept (unless `--no-keep-empty`
 is specified), and commits which are clean cherry-picks (as determined
 by `git log --cherry-mark ...`) are detected and dropped as a
-preliminary step (unless `--reapply-cherry-picks` is passed).
+preliminary step (unless `--reapply-cherry-picks` or `--keep-base` is
+passed).
 +
 See also INCOMPATIBLE OPTIONS below.
 
@@ -311,13 +314,16 @@ See also INCOMPATIBLE OPTIONS below.
 	upstream changes, the behavior towards them is controlled by
 	the `--empty` flag.)
 +
-By default (or if `--no-reapply-cherry-picks` is given), these commits
-will be automatically dropped.  Because this necessitates reading all
-upstream commits, this can be expensive in repos with a large number
-of upstream commits that need to be read.  When using the 'merge'
-backend, warnings will be issued for each dropped commit (unless
-`--quiet` is given). Advice will also be issued unless
-`advice.skippedCherryPicks` is set to false (see linkgit:git-config[1]).
+
+In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is
+given), these commits will be automatically dropped.  Because this
+necessitates reading all upstream commits, this can be expensive in
+repositories with a large number of upstream commits that need to be
+read. When using the 'merge' backend, warnings will be issued for each
+dropped commit (unless `--quiet` is given). Advice will also be issued
+unless `advice.skippedCherryPicks` is set to false (see
+linkgit:git-config[1]).
+
 +
 `--reapply-cherry-picks` allows rebase to forgo reading all upstream
 commits, potentially improving performance.
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 90ae8fd8de7..d718b7fe888 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1180,6 +1180,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 */
@@ -1239,6 +1240,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 to avoid losing
+	 * commits 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");
@@ -1415,7 +1422,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)
@@ -1681,6 +1692,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		fill_branch_base(&options, &branch_base);
 	}
 
+	if (keep_base && options.reapply_cherry_picks)
+		options.upstream = options.onto;
+
 	if (options.fork_point > 0)
 		options.restrict_revision =
 			get_fork_point(options.upstream_name, options.orig_head);
diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
index 01eb9513d6c..ea501f2b42b 100755
--- a/t/t3416-rebase-onto-threedots.sh
+++ b/t/t3416-rebase-onto-threedots.sh
@@ -199,6 +199,27 @@ test_expect_success 'rebase --keep-base requires a single merge base' '
 	grep "need exactly one merge base with branch" err
 '
 
+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
-- 
gitgitgadget

[PATCH v4 8/8] rebase --keep-base: imply --no-fork-point

[Phillip Wood via GitGitGadget]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Given the name of the option it is confusing if --keep-base actually
changes the base of the branch without --fork-point being explicitly
given on the command line.

The combination of --keep-base with an explicit --fork-point is still
supported even though --fork-point means we do not keep the same base
if the upstream branch has been rewound.  We do this in case anyone is
relying on this behavior which is tested in t3431[1]

[1] https://lore.kernel.org/git/20200715032014.GA10818@generichostname/

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-rebase.txt | 8 ++++----
 builtin/rebase.c             | 6 ++++++
 t/t3431-rebase-fork-point.sh | 2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ee6cdd56949..1e2d543ced9 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 --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
+	`git rebase --reapply-cherry-picks --no-fork-point --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
@@ -452,9 +452,9 @@ When `--fork-point` is active, 'fork_point' will be used instead of
 <branch>` command (see linkgit:git-merge-base[1]).  If 'fork_point'
 ends up being empty, the `<upstream>` will be used as a fallback.
 +
-If `<upstream>` is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`. See also
-`rebase.forkpoint` in linkgit:git-config[1].
+If `<upstream>` or `--keep-base` is given on the command line, then
+the default is `--no-fork-point`, otherwise the default is
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
 +
 If your branch was based on `<upstream>` but `<upstream>` was rewound and
 your branch contains commits which were dropped, this option can be used
diff --git a/builtin/rebase.c b/builtin/rebase.c
index d718b7fe888..b9164a30bff 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1239,6 +1239,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
 		if (options.root)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
+		/*
+		 * --keep-base defaults to --no-fork-point to keep the
+		 * base the same.
+		 */
+		if (options.fork_point < 0)
+			options.fork_point = 0;
 	}
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 1d0b15380ed..70e81363569 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -50,7 +50,7 @@ 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 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
-- 
gitgitgadget