Merge branch 'pw/rebase-keep-base-fixes'

"git rebase --keep-base" used to discard the commits that are
already cherry-picked to the upstream, even when "keep-base" meant
that the base, on top of which the history is being rebuilt, does
not yet include these cherry-picked commits.  The --keep-base
option now implies --reapply-cherry-picks and --no-fork-point
options.

* pw/rebase-keep-base-fixes:
  rebase --keep-base: imply --no-fork-point
  rebase --keep-base: imply --reapply-cherry-picks
  rebase: factor out branch_base calculation
  rebase: rename merge_base to branch_base
  rebase: store orig_head as a commit
  rebase: be stricter when reading state files containing oids
  t3416: set $EDITOR in subshell
  t3416: tighten two tests
This commit is contained in:
Taylor Blau 2022-10-30 21:04:42 -04:00
commit 003f815dd9
6 changed files with 173 additions and 90 deletions

View File

@ -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 --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
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.
@ -443,9 +449,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

View File

@ -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);
@ -431,9 +430,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 +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))
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,32 +866,23 @@ 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 *branch_base)
{
struct commit *head = lookup_commit(the_repository, head_oid);
struct commit_list *merge_bases = NULL;
int res = 0;
if (!head)
if (is_null_oid(branch_base))
goto done; /* fill_branch_base() found multiple merge bases */
if (!oideq(branch_base, &onto->object.oid))
goto done;
merge_bases = get_merge_bases(onto, head);
if (!merge_bases || merge_bases->next) {
oidcpy(merge_base, null_oid());
goto done;
}
oidcpy(merge_base, &merge_bases->item->object.oid);
if (!oideq(merge_base, &onto->object.oid))
goto done;
if (restrict_revision && !oideq(&restrict_revision->object.oid, merge_base))
if (restrict_revision && !oideq(&restrict_revision->object.oid, branch_base))
goto done;
if (!upstream)
goto done;
free_commit_list(merge_bases);
merge_bases = get_merge_bases(upstream, head);
if (!merge_bases || merge_bases->next)
goto done;
@ -906,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;
@ -1039,7 +1044,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;
@ -1175,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 */
@ -1233,7 +1239,19 @@ 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
* 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");
@ -1312,13 +1330,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;
@ -1410,7 +1428,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)
@ -1604,25 +1626,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 +1663,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");
@ -1654,7 +1679,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);
@ -1662,7 +1687,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 =
@ -1670,15 +1695,15 @@ 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) {
struct commit *head =
lookup_commit_reference(the_repository,
&options.orig_head);
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, head);
}
get_fork_point(options.upstream_name, options.orig_head);
if (repo_read_index(the_repository) < 0)
die(_("could not read index"));
@ -1703,13 +1728,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, &branch_base)) {
int flag;
if (!(options.flags & REBASE_FORCE)) {
@ -1750,12 +1772,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));
}
@ -1767,8 +1789,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 +1807,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 +1821,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(&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",
@ -1820,7 +1842,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;

View File

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

View File

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

View File

@ -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,20 +92,22 @@ 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
'
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' '
@ -156,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 &&
@ -171,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 &&
@ -182,13 +190,39 @@ 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_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
'
test_done

View File

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