From 96601a26b4cdb2184b79b63aa670f695f5690183 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:38 +0000 Subject: [PATCH 1/8] t3416: tighten two tests 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 Signed-off-by: Junio C Hamano --- 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 3e04802cb0..dc67d2308f 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 From 05ec41855da2ba813e6cfb5fb1fb86aea623addc Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:39 +0000 Subject: [PATCH 2/8] 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. 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 Signed-off-by: Junio C Hamano --- 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 dc67d2308f..01eb9513d6 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 From b8dbfd030ccc8e5be6b22bc28a342119504e0c2b Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:40 +0000 Subject: [PATCH 3/8] rebase: be stricter when reading state files containing oids 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 Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- 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 70aa7c842f..4139562cdb 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -429,9 +429,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 @@ -446,7 +446,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 1fb1b2ea90..874b3014b5 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 21e4d25ce7..fa39202fa6 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); From f21becdd94d17ae792b951cf784898324bedfda5 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:41 +0000 Subject: [PATCH 4/8] 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 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 " 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 Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/rebase.c | 66 +++++++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 35 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 4139562cdb..1df5e2f2eb 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; @@ -260,13 +260,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); @@ -297,8 +297,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, - &todo_list); + &opts->orig_head->object.oid, &commands, + opts->autosquash, &todo_list); } string_list_clear(&commands, 0); @@ -446,7 +446,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))) @@ -515,7 +516,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) @@ -644,7 +645,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, @@ -678,7 +679,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); @@ -826,7 +827,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) @@ -859,15 +860,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()); @@ -1302,13 +1299,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; @@ -1594,25 +1591,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 = @@ -1629,8 +1628,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"); @@ -1662,13 +1662,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")); @@ -1698,7 +1694,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; @@ -1775,7 +1771,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; @@ -1789,7 +1785,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", @@ -1810,7 +1806,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; From a77060218dfae5e8209a0ee131f292b881ce533b Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:42 +0000 Subject: [PATCH 5/8] rebase: rename merge_base to branch_base 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 Signed-off-by: Junio C Hamano --- builtin/rebase.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 1df5e2f2eb..74024f49df 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -860,22 +860,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) @@ -1029,7 +1029,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; @@ -1644,7 +1644,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); @@ -1652,7 +1652,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 = @@ -1690,11 +1690,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; @@ -1736,12 +1736,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)); } @@ -1753,8 +1753,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); @@ -1785,7 +1785,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", From d42c9ffa0fa169ea51c971d1143b3f20d5a32d83 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:43 +0000 Subject: [PATCH 6/8] 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 between 'HEAD' and 'onto' when --keep-base is given. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- builtin/rebase.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index 74024f49df..9ee3ab6240 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -865,13 +865,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; @@ -881,7 +877,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; @@ -896,6 +891,20 @@ static int can_fast_forward(struct commit *onto, struct commit *upstream, 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; @@ -1660,6 +1669,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) @@ -1689,13 +1699,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)) { From ce5238a690821d1de230091dd6c9c13a99ed6752 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:44 +0000 Subject: [PATCH 7/8] rebase --keep-base: imply --reapply-cherry-picks 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 Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- 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 a872ab0fbd..092a355071 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 `` and ``. Running `git rebase --keep-base ` is equivalent to running - `git rebase --onto ... `. + `git rebase --reapply-cherry-picks --onto ... `. + 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 `` and ``, 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 9ee3ab6240..0287f3e817 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1171,6 +1171,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 */ @@ -1230,6 +1231,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"); @@ -1406,7 +1413,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) @@ -1672,6 +1683,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 01eb9513d6..ea501f2b42 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 From aa1df8146d70bb85c63b0999868fe29aebc1173e Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Mon, 17 Oct 2022 13:17:45 +0000 Subject: [PATCH 8/8] rebase --keep-base: imply --no-fork-point 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 Signed-off-by: Junio C Hamano --- 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 092a355071..65d500b28e 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 `` and ``. Running `git rebase --keep-base ` is equivalent to running - `git rebase --reapply-cherry-picks --onto ... `. + `git rebase --reapply-cherry-picks --no-fork-point --onto ... `. + 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 ` command (see linkgit:git-merge-base[1]). If 'fork_point' ends up being empty, the `` will be used as a fallback. + -If `` 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 `` 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 `` but `` 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 0287f3e817..ef520f66fb 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1230,6 +1230,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 1d0b15380e..70e8136356 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