Re: [PATCH v2] rebase: teach rebase --keep-base
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Mar 28, 2019, 23:13 UTC
- Message-ID
- <87tvfma8yt.fsf@evledraar.gmail.com>
- In-Reply-To
- <20190328221745.GA3941@dev-l>
On Thu, Mar 28 2019, Denton Liu wrote:
Show 50 quoted lines
> A common scenario is if a user is working on a topic branch and they > wish to make some changes to intermediate commits or autosquash, they > would run something such as > > git rebase -i --onto master... master > > in order to preserve the merge base. This prevents unnecessary commit > churning. > > Alternatively, a user wishing to test individual commits in a topic > branch without changing anything may run > > git rebase -x ./test.sh master... master > > Since rebasing onto the merge base of the branch and the upstream is > such a common case, introduce the --keep-base option as a shortcut. > > This allows us to rewrite the above as > > git rebase -i --keep-base master > > and > > git rebase -x ./test.sh --keep-base master > > respectively. > > Add tests to ensure --keep-base works correctly in the normal case and > fails when there are multiple merge bases, both in regular and > interactive mode. Also, test to make sure conflicting options cause > rebase to fail. While we're adding test cases, add a missing > set_fake_editor call to 'rebase -i --onto master...side'. > > While we're documenting the --keep-base option, change an instance of > "merge-base" to "merge base", which is the consistent spelling. > > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Helped-by: Junio C Hamano <gitster@pobox.com> > Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > > Ævar, I have a feeling that we're still miscommunicating and we don't > fully understand each other. I'm putting up v2 to hopefully clear things > up a little but I welcome more feedback. > > This patch now depends "[PATCH 1/8] tests (rebase): spell out the > `--keep-empty` option" which is the first patch of Johannes's "Do not > use abbreviated options in tests" patchset[1]. (Thanks for catching > that, Johannes!)
Yeah I'm still confused. Gotta go now, but just some early poking I did.
First, there's now docs saying "starting point" v.s. "fork point", but the tests are still the same, i.e. the ones I can just replace with either of `git merge-base [--fork-point] @{u} master`.
It would really help if the tests actually stressed the parts where this is different, including argument-less versions. I.e. just "git rebase --keep-base".
Speaking of that, even though you say this is different than "--fork-point" you may or may not have noticed that if "argc < 1" you *still* go through the whole fork_point codepath, which will set "options.restrict_revision" for you. This is part of what I mentioned upthread. I.e. with this on top of this patch all your tests still pass:
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 3347dd8975..e38a5044eb 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1515,7 +1515,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
NULL);
if (!options.upstream_name)
error_on_missing_default_upstream();
- if (fork_point < 0)
+ if (fork_point < 0 && !keep_base)
fork_point = 1;
} else {
options.upstream_name = argv[0];
@@ -1524,9 +1524,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
if (!strcmp(options.upstream_name, "-"))
options.upstream_name = "@{-1}";
}
- options.upstream = peel_committish(options.upstream_name);
- if (!options.upstream)
- die(_("invalid upstream '%s'"), options.upstream_name);
+ if (!keep_base) {
+ options.upstream = peel_committish(options.upstream_name);
+ if (!options.upstream)
+ /* I suppose we need to keep this die(...) somewhere still... */
+ die(_("invalid upstream '%s'"), options.upstream_name);
+ }
options.upstream_arg = options.upstream_name;
} else {
if (!options.onto_name) {
@@ -1564,6 +1566,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
}
options.onto = lookup_commit_or_die(&merge_base,
options.onto_name);
+ if (keep_base)
+ options.upstream = options.onto;
} else {
options.onto = peel_committish(options.onto_name);
if (!options.onto)But now because (and bear with me, I don't really get all this) we are not implicitly setting options.restrict_revision later in the "fork_point > 0" case *and* our "options.upstream" SHA-1 is the base you find with --keep-base instead of the @{u} SHA-1 the can_fast_forward(...) "if" kicks in, so now:
$ git status
On branch master
Your branch and 'origin/master' have diverged,
and have 3 and 37 different commits each, respectively. # Here your version would always re-rebase it
$ ~/g/git/git --exec-path=$PWD rebase --keep-base
Current branch master is up to date.But "-i" still works as intended:
$ ~/g/git/git --exec-path=$PWD rebase --keep-base -i
hint: Waiting for your editor to close the file... Waiting for Emacs...
Successfully rebased and updated refs/heads/master.
$ git status
On branch master
Your branch and 'origin/master' have diverged,
and have 3 and 37 different commits each, respectively.I don't know about the *other* use-cases you have in mind, but I can't see a reason for why *that* simple case shouldn't work like that. Why would we redundantly rebase every time in this case, just becase some mode of --onto does it? I think if anything we should teach it the same lazyness, or maybe that breaks stuff (what stuff?).
Peff discussed some of these variables & their interaction in https://public-inbox.org/git/20190224101029.GA13438@sigill.intra.peff.net/
I just re-read that, but this whole thing has paged out of my brain in the meantime, and on my quick reading before sending this E-Mail I'm still not sure what all the subtleties involved are.
But one thing's for sure, I think exhaustive testing of all the edge cases involved will make this a lot clearer. I.e.:
* Here upstream has diverged, we rebase (and in the "noop case?) * Here upstream has diverged, *and* rewound (and in the "noop case?) * etc. etc.
Show 219 quoted lines
> Changes since v1:
>
> * Squashed old set into one patch
> * Fixed indentation style and dangling else
> * Added more documentation after discussion with Ævar
>
> [1]: https://public-inbox.org/git/a1b4b74b9167e279dae4cd8c58fb28d8a714a66a.1553537656.git.gitgitgadget@gmail.com/
>
> Documentation/git-rebase.txt | 25 ++++++++++++--
> builtin/rebase.c | 32 ++++++++++++++----
> t/t3416-rebase-onto-threedots.sh | 57 ++++++++++++++++++++++++++++++++
> 3 files changed, 105 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 6363d674b7..27be1f48ff 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -8,8 +8,8 @@ git-rebase - Reapply commits on top of another base tip
> SYNOPSIS
> --------
> [verse]
> -'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]
> - [<upstream> [<branch>]]
> +'git rebase' [-i | --interactive] [<options>] [--exec <cmd>]
> + [--onto <newbase> | --keep-base] [<upstream> [<branch>]]
> 'git rebase' [-i | --interactive] [<options>] [--exec <cmd>] [--onto <newbase>]
> --root [<branch>]
> 'git rebase' --continue | --skip | --abort | --quit | --edit-todo | --show-current-patch
> @@ -217,6 +217,19 @@ As a special case, you may use "A\...B" as a shortcut for the
> merge base of A and B if there is exactly one merge base. You can
> leave out at most one of A and B, in which case it defaults to HEAD.
>
> +--keep-base::
> + Set the starting point at which to create the new commits to the
> + merge base of <upstream> <branch>. Running
> + 'git rebase --keep-base <upstream> <branch>' is equivalent to
> + running 'git rebase --onto <upstream>... <upstream>'.
> ++
> +Although both this option and --fork-point find the merge base between
> +<upstream> and <branch>, this option uses the merge base as the _starting
> +point_ on which new commits will be created, whereas --fork-point uses
> +the merge base to determine the _set of commits_ which will be rebased.
> ++
> +See also INCOMPATIBLE OPTIONS below.
> +
> <upstream>::
> Upstream branch to compare against. May be any valid commit,
> not just an existing branch name. Defaults to the configured
> @@ -364,6 +377,10 @@ ends up being empty, the <upstream> will be used as a fallback.
> +
> If either <upstream> or --root is given on the command line, then the
> default is `--no-fork-point`, otherwise the default is `--fork-point`.
> ++
> +If your branch was based on <upstream> but <upstream> was rewound and
> +your branch contains commits which were dropped, this option can be used
> +with `--keep-base` in order to drop those commits from your branch.
>
> --ignore-whitespace::
> --whitespace=<option>::
> @@ -539,6 +556,8 @@ In addition, the following pairs of options are incompatible:
> * --preserve-merges and --rebase-merges
> * --rebase-merges and --strategy
> * --rebase-merges and --strategy-option
> + * --keep-base and --onto
> + * --keep-base and --root
>
> BEHAVIORAL DIFFERENCES
> -----------------------
> @@ -863,7 +882,7 @@ NOTE: While an "easy case recovery" sometimes appears to be successful
> --interactive` will be **resurrected**!
>
> The idea is to manually tell 'git rebase' "where the old 'subsystem'
> -ended and your 'topic' began", that is, what the old merge-base
> +ended and your 'topic' began", that is, what the old merge base
> between them was. You will have to find a way to name the last commit
> of the old 'subsystem', for example:
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 77deebc65c..7c14a00460 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -27,8 +27,8 @@
> #include "branch.h"
>
> static char const * const builtin_rebase_usage[] = {
> - N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
> - "[<upstream>] [<branch>]"),
> + N_("git rebase [-i] [options] [--exec <cmd>] "
> + "[--onto <newbase> | --keep-base] [<upstream> [<branch>]]"),
> N_("git rebase [-i] [options] [--exec <cmd>] [--onto <newbase>] "
> "--root [<branch>]"),
> N_("git rebase --continue | --abort | --skip | --edit-todo"),
> @@ -1018,6 +1018,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> };
> const char *branch_name;
> int ret, flags, total_argc, in_progress = 0;
> + int keep_base = 0;
> int ok_to_skip_pre_rebase = 0;
> struct strbuf msg = STRBUF_INIT;
> struct strbuf revisions = STRBUF_INIT;
> @@ -1051,6 +1052,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> OPT_STRING(0, "onto", &options.onto_name,
> N_("revision"),
> N_("rebase onto given branch instead of upstream")),
> + OPT_BOOL(0, "keep-base", &keep_base,
> + N_("use the merge-base of upstream and branch as the current base")),
> OPT_BOOL(0, "no-verify", &ok_to_skip_pre_rebase,
> N_("allow pre-rebase hook to run")),
> OPT_NEGBIT('q', "quiet", &options.flags,
> @@ -1217,6 +1220,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> usage_with_options(builtin_rebase_usage,
> builtin_rebase_options);
>
> + if (keep_base) {
> + if (options.onto_name)
> + die(_("cannot combine '--keep-base' with '--onto'"));
> + if (options.root)
> + die(_("cannot combine '--keep-base' with '--root'"));
> + }
> +
> if (action != NO_ACTION && !in_progress)
> die(_("No rebase in progress?"));
> setenv(GIT_REFLOG_ACTION_ENVIRONMENT, "rebase", 0);
> @@ -1541,12 +1551,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> }
>
> /* Make sure the branch to rebase onto is valid. */
> - if (!options.onto_name)
> + if (keep_base) {
> + strbuf_reset(&buf);
> + strbuf_addstr(&buf, options.upstream_name);
> + strbuf_addstr(&buf, "...");
> + options.onto_name = xstrdup(buf.buf);
> + } else if (!options.onto_name)
> options.onto_name = options.upstream_name;
> if (strstr(options.onto_name, "...")) {
> - if (get_oid_mb(options.onto_name, &merge_base) < 0)
> - die(_("'%s': need exactly one merge base"),
> - options.onto_name);
> + if (get_oid_mb(options.onto_name, &merge_base) < 0) {
> + if (keep_base)
> + die(_("'%s': need exactly one merge base with branch"),
> + options.upstream_name);
> + else
> + die(_("'%s': need exactly one merge base"),
> + options.onto_name);
> + }
> options.onto = lookup_commit_or_die(&merge_base,
> options.onto_name);
> } else {
> diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
> index ddf2f64853..9c2548423b 100755
> --- a/t/t3416-rebase-onto-threedots.sh
> +++ b/t/t3416-rebase-onto-threedots.sh
> @@ -99,7 +99,64 @@ test_expect_success 'rebase -i --onto master...side' '
> git checkout side &&
> git reset --hard K &&
>
> + set_fake_editor &&
> test_must_fail git rebase -i --onto master...side J
> '
>
> +test_expect_success 'rebase --keep-base --onto incompatible' '
> + test_must_fail git rebase --keep-base --onto master...
> +'
> +
> +test_expect_success 'rebase --keep-base --root incompatible' '
> + test_must_fail git rebase --keep-base --root
> +'
> +
> +test_expect_success 'rebase --keep-base master from topic' '
> + git reset --hard &&
> + git checkout topic &&
> + git reset --hard G &&
> +
> + git rebase --keep-base master &&
> + git rev-parse C >base.expect &&
> + git merge-base master HEAD >base.actual &&
> + test_cmp base.expect base.actual &&
> +
> + git rev-parse HEAD~2 >actual &&
> + git rev-parse C^0 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'rebase --keep-base master from side' '
> + git reset --hard &&
> + git checkout side &&
> + git reset --hard K &&
> +
> + test_must_fail git rebase --keep-base master
> +'
> +
> +test_expect_success 'rebase -i --keep-base master from topic' '
> + git reset --hard &&
> + git checkout topic &&
> + git reset --hard G &&
> +
> + set_fake_editor &&
> + EXPECT_COUNT=2 git rebase -i --keep-base master &&
> + git rev-parse C >base.expect &&
> + git merge-base master HEAD >base.actual &&
> + test_cmp base.expect base.actual &&
> +
> + git rev-parse HEAD~2 >actual &&
> + git rev-parse C^0 >expect &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'rebase -i --keep-base master from side' '
> + git reset --hard &&
> + git checkout side &&
> + git reset --hard K &&
> +
> + set_fake_editor &&
> + test_must_fail git rebase -i --keep-base master
> +'
> +
> test_done