rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

From: Phillip Wood <[email protected]>

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.

Quoted 2 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

Quoted 3 lines

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

t/t3416-rebase-onto-threedots.sh test_expect_success 'rebase -i --onto main...side' ' · line 110

Quoted 7 lines

t/t3416-rebase-onto-threedots.sh +0 -1

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

t/t3416-rebase-onto-threedots.sh test_expect_success 'rebase -i --keep-base main from side' ' · line 198

Quoted 7 lines

t/t3416-rebase-onto-threedots.sh +0 -1

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

Hi Junio

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 4 lines

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.

Quoted 16 lines · Phillip Wood

Hi Junio

Quoted 12 lines · Junio C Hamano

Quoted 5 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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?

Quoted 2 lines

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.

From: Phillip Wood <[email protected]>

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.

From: Phillip Wood <[email protected]>

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.

From: Phillip Wood <[email protected]>

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 <[email protected]>

---
 builtin/rebase.c | 62 ++++++++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 34 deletions(-)

builtin/rebase.c +28 -35

index 56e4214b441..6cf9c95f4e1 100644
@@ -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

From: Phillip Wood <[email protected]>

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 <[email protected]>

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

builtin/rebase.c +0 -0

index 6cf9c95f4e1..86ea731ca3a 100644
@@ -871,13 +871,9 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream,
 	struct commit_list *merge_bases = NULL;
 	int res = 0;
 

builtin/rebase.c +14 -0

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

Quoted 7 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 4 lines

Signed-off-by Phillip Wood <[email protected]>

---
Note the unnecessary braces around "if (keep_base)" are added here reduce the churn on the next commit.

Quoted 4 lines

builtin/rebase.c +0 -0

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

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1685

Quoted 6 lines

builtin/rebase.c +5 -1

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

Hi Junio,

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 4 lines

Quoted 4 lines

Signed-off-by Phillip Wood <[email protected]>

---
Note the unnecessary braces around "if (keep_base)" are added here reduce the churn on the next commit.

This note...

Quoted 4 lines

Quoted 4 lines

builtin/rebase.c +0 -0

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

Quoted 8 lines

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1685

Quoted 6 lines

builtin/rebase.c +5 -1

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

Quoted 6 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 24 lines

Signed-off-by Phillip Wood <[email protected]>

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

builtin/rebase.c +1 -4

index 6cf9c95f4e1..86ea731ca3a 100644
@@ -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;
-	}
 

builtin/rebase.c static int can_fast_forward(struct commit *onto, struct commit *upstream, · line 878

Quoted 3 lines

builtin/rebase.c +0 -1

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

Quoted 20 lines

builtin/rebase.c +14 -0

@@ -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)
  {

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1682

Quoted 8 lines

builtin/rebase.c +2 -1

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

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1685

Quoted 3 lines

builtin/rebase.c +3 -0

+	} 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.

Quoted 5 lines

Quoted 5 lines

  -	merge_bases = get_merge_bases(onto, head);

builtin/rebase.c +1 -2

-	if (!merge_bases || merge_bases->next) {
-		oidcpy(merge_base, null_oid());
+	if (is_null_oid(merge_base))
  		goto done;

Quoted 8 lines

builtin/rebase.c static int can_fast_forward(struct commit *onto, struct commit *upstream, · line 878

Quoted 4 lines

builtin/rebase.c +0 -1

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

From: Phillip Wood <[email protected]>

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 <[email protected]>

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

builtin/rebase.c +20 -6

index 6cf9c95f4e1..86ea731ca3a 100644
@@ -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.

builtin/rebase.c +0 -1

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

gitgitgadget

Quoted 9 lines · Phillip Wood via GitGitGadget

builtin/rebase.c +4 -1

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

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1688

Quoted 4 lines

builtin/rebase.c +1 -0

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

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
 Documentation/git-rebase.txt     |  2 +-
 builtin/rebase.c                 | 15 ++++++++++++++-
 t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

Documentation/git-rebase.txt +1 -1

index 080658c8710..dc0c6c54e27 100644
@@ -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

builtin/rebase.c +1 -0

index 86ea731ca3a..b6b3e00e3b1 100644
@@ -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 */

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1249

Quoted 10 lines · Phillip Wood via GitGitGadget

builtin/rebase.c +6 -0

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

Quoted 4 lines

builtin/rebase.c +0 -0

@@ -1416,7 +1423,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
  	if (options.empty != EMPTY_UNSPECIFIED)
  		imply_merge(&options, "--empty");
 

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1431

Quoted 7 lines

builtin/rebase.c +5 -1

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

From: Phillip Wood <[email protected]>

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 <[email protected]>

Signed-off-by Phillip Wood <[email protected]>

---
 Documentation/git-rebase.txt     |  2 +-
 builtin/rebase.c                 | 15 ++++++++++++++-
 t/t3416-rebase-onto-threedots.sh | 21 +++++++++++++++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

Documentation/git-rebase.txt +0 -0

index 080658c8710..dc0c6c54e27 100644

Quoted 8 lines

Documentation/git-rebase.txt +0 -0

index 080658c8710..dc0c6c54e27 100644
@@ -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

Documentation/git-rebase.txt leave out at most one of A and B, in which case it defaults to HEAD. · line 224

Quoted 5 lines

Documentation/git-rebase.txt +1 -1

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

From: Phillip Wood <[email protected]>

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.

builtin/rebase.c +2 -0

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

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1248

Quoted 10 lines · Phillip Wood via GitGitGadget

builtin/rebase.c +6 -0

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

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.

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/[email protected]/

From: Phillip Wood <[email protected]>

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 <[email protected]>

---

 t/t3416-rebase-onto-threedots.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

t/t3416-rebase-onto-threedots.sh +6 -4

index 3e04802cb00..dc67d2308f3 100755
@@ -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

From: Phillip Wood <[email protected]>

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.

builtin/rebase.c +0 -0

@@ -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,

builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 450

Quoted 10 lines · Phillip Wood via GitGitGadget

builtin/rebase.c +1 -1

index 56e4214b441..a3cf1ef5923 100644
@@ -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.

Quoted 4 lines

builtin/rebase.c +0 -0

@@ -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,

builtin/rebase.c static int is_linear_history(struct commit *from, struct commit *to) · line 877

Quoted 10 lines

builtin/rebase.c +1 -5

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

Quoted 4 lines

builtin/rebase.c +0 -0

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

builtin/rebase.c int cmd_rebase(int argc, const char **argv, const char *prefix) · line 1613

Quoted 6 lines

builtin/rebase.c +2 -1

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

From: Phillip Wood <[email protected]>

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.

t/t3431-rebase-fork-point.sh +0 -0

 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

Signature · 1 line

gitgitgadget

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.

Quoted 6 lines

t/t3431-rebase-fork-point.sh +0 -0

index 1d0b15380ed..70e81363569 100755
@@ -50,7 +50,7 @@ test_rebase () {
 

t/t3431-rebase-fork-point.sh test_rebase () { · line 53

Quoted 4 lines

t/t3431-rebase-fork-point.sh +1 -1

  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.

From: Phillip Wood <[email protected]>

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.

builtin/rebase.c +2 -2

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

From: Phillip Wood <[email protected]>

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.

Quoted 5 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

...

builtin/rebase.c static int read_basic_state(struct rebase_options *opts) · line 434

Quoted 6 lines

builtin/rebase.c +1 -1

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

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.

Quoted 5 lines · Phillip Wood via GitGitGadget

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

Quoted 3 lines

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.

From: Phillip Wood <[email protected]>

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.

Quoted 5 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 1 line

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

Quoted 7 lines · Ævar Arnfjörð Bjarmason

Quoted 5 lines · Phillip Wood via GitGitGadget

From: Phillip Wood <[email protected]>

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.

Quoted 7 lines

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.

Quoted 1 line

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