rebase: learn --keep-base

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

[...]

Signed-off-by Denton Liu <[email protected]>

---

builtin/rebase.c +3 -1

@@ -1541,10 +1551,19 @@ 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);

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

A common scenario is if a user is working on a topic branch and they wish to make some changes to intermediate commits or autosquashing, 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.

Quoted 9 lines · Denton Liu

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

Since this is 0/3 I won't complain too loudly, but read the above again while pretending that you didn't know what your initial motivating example was. The last three lines does not explain anything useful to such a reader, and the reader needs to decipher the two sample commands to guess what you wanted to achieve.

Before "teaches rebase the --keep-base option", you must tell what you wanted to do with that new feature to attract readers---convince them your new contribution is worth their time to read over.

If I understand correctly, what "--onto $(git merge-base @{u} HEAD) @{u}" lets you express is:

no matter how much progress the upstream has made while I was away from the keyboard, I want to rebase the current topic on top of the same commit from the upstream, on which I based the current iteration of the topic.

I suspect that such a rebase will become no-op without "-i". Am I mistaken? I am not sure if "--keep-base" is useful without "-i".

But of course, it would be useful with "-i", i.e. when you want to further polish the topic. You need to give <upstream> to the command to let it know where their work stops and your work begins, i.e. letting the command figure out what commits to replay. But in such a workflow, you do not want <upstream> to affect on top of what commit the replayed history is built. And "keep base" would be a very direct way to achieve that.

Quoted 19 lines · Denton Liu

A common scenario is if a user is working on a topic branch and they wish to make some changes to intermediate commits or autosquashing, 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

Quoted 7 lines

	git rebase -i --keep-base master

and

	git rebase -x ./test.sh --keep-base master

respectively.

I never use the "feature" myself, but I recall that when "git rebase" is run on a branch appropriately prepared, you do not even have to say <upstream> (iow, you type "git rebase<RET>" and rebase on top of @{upstream}).

Can this new "--keep-base" feature mesh well with it? When the current branch has forked from origin/master, for example, it would be good if

	$ git rebase -i --same-base

becomes a usable short-hand for

	$ git rebase -i --same-base origin/master

aka

	$ git rebase -i --onto $(git merge-base HEAD origin/master) origin/master

Hi Junio,

Quoted 38 lines · Junio C Hamano

Quoted 19 lines · Denton Liu

A common scenario is if a user is working on a topic branch and they wish to make some changes to intermediate commits or autosquashing, 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

Quoted 7 lines

	git rebase -i --keep-base master

and

	git rebase -x ./test.sh --keep-base master

respectively.

I never use the "feature" myself, but I recall that when "git rebase" is run on a branch appropriately prepared, you do not even have to say <upstream> (iow, you type "git rebase<RET>" and rebase on top of @{upstream}).

Can this new "--keep-base" feature mesh well with it? When the current branch has forked from origin/master, for example, it would be good if

Quoted 5 lines

	$ git rebase -i --same-base

becomes a usable short-hand for

	$ git rebase -i --same-base origin/master

By "--same-base", I am assuming you mistyped and meant to write "--keep-base"? If that's the case, I can make it a shorthand.

Thanks,

Denton

Quoted 5 lines

aka

	$ git rebase -i --onto $(git merge-base HEAD origin/master) origin/master

A common scenario is if a user is working on a topic branch and they wish to make some changes to intermediate commits or autosquashing, 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.

Signed-off-by Denton Liu <[email protected]>

---
 builtin/rebase.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

builtin/rebase.c +22 -3

index 77deebc65c..fffee89064 100644
@@ -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,10 +1551,19 @@ 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)
+		    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,

Signature · 1 line

2.21.0.512.g57bf1b23e1

[...]

This allows us to rewrite the above as

	git rebase -i --keep-base master

and

	git rebase -x ./test.sh --keep-base master

respectively.

Hi Denton,

Quoted 11 lines · Denton Liu

[...]

This allows us to rewrite the above as

	git rebase -i --keep-base master

and

	git rebase -x ./test.sh --keep-base master

respectively.

Just a quick note: this breaks t5407 because that test uses git rebase --keep and expects that abbreviated option to be expanded to --keep-empty, which is now no longer the only possible expansion.

I just submitted a patch series to fix that, and other uses of abbreviated options in the test suite, in
https://public-inbox.org/git/[email protected]/T/#t

Ciao,
Johannes

P.S.: Did you run the test suite before submitting your patches?

Quoted 1 line · Johannes Schindelin

P.S.: Did you run the test suite before submitting your patches?

Usually I'm more diligent about running tests but I wrote this patchset in the back of a car when I was running low on batteries. I only ran the rebase-related tests but I guess that wasn't enough.

My mistake, though. I'll be sure to _always_ run tests in the future.

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

* https://stackoverflow.com/questions/53234798/can-i-rebase-on-a-branchs-fork-point-without-explicitly-specifying-the-parent
* https://stackoverflow.com/questions/41529128/how-do-you-rebase-only-changes-between-two-branches-into-another-branch
* https://stackoverflow.com/a/4207357

Quoted 9 lines · Denton Liu

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

Quoted 3 lines

* https://stackoverflow.com/questions/53234798/can-i-rebase-on-a-branchs-fork-point-without-explicitly-specifying-the-parent
* https://stackoverflow.com/questions/41529128/how-do-you-rebase-only-changes-between-two-branches-into-another-branch
* https://stackoverflow.com/a/4207357

Like with another series of yours I think this would be best squashed into one patch.

Maybe I've misunderstood this but isn't this like --fork-point except with just plain "git merge-base" instead of "git merge-base --fork-point", but then again 2/3 shows multiple base aren't supported, but merge-base supports that.

I'd find something like the "DISCUSSION ON FORK-POINT MODE" in git-merge-base helpful with examples of what we'd pick in the various scenarios, and also if whatever commit this picks was something you could have "git merge-base" spew out, so you could get what rebase would do here from other tooling (which maybe is possible, but I'm confused by the "no multiple bases"...).

Hi Ævar,

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Denton Liu

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

Quoted 6 lines

Quoted 3 lines

* https://stackoverflow.com/questions/53234798/can-i-rebase-on-a-branchs-fork-point-without-explicitly-specifying-the-parent
* https://stackoverflow.com/questions/41529128/how-do-you-rebase-only-changes-between-two-branches-into-another-branch
* https://stackoverflow.com/a/4207357

Like with another series of yours I think this would be best squashed into one patch.

Will do.

Quoted 6 lines

Maybe I've misunderstood this but isn't this like --fork-point except with just plain "git merge-base" instead of "git merge-base --fork-point", but then again 2/3 shows multiple base aren't supported, but merge-base supports that.

--fork-point gets used to determine the _set of_ commits which are to be rebased, whereas --keep-base (and --onto) determine the base where that set of commits will be spliced. As a result, these two options cover orthogonal use-cases.

reason that --onto already disallows multiple bases. If we have multiple bases, how do we determine which one is the "true base" to use? It makes more sense to error out and let the user manually specify it.

Quoted 6 lines

I'd find something like the "DISCUSSION ON FORK-POINT MODE" in git-merge-base helpful with examples of what we'd pick in the various scenarios, and also if whatever commit this picks was something you could have "git merge-base" spew out, so you could get what rebase would do here from other tooling (which maybe is possible, but I'm confused by the "no multiple bases"...).

If I'm understanding you correctly then yes, this could be done with other tooling. See the 0/3 for equivalent commands.

Perhaps I should update the rebase documentation to mention that --fork-point and --keep-base are orthogonal because it's unclear for you, it's probably unclear for other users as well.

Thanks,

Denton

Quoted 31 lines · Denton Liu

Hi Ævar,

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Denton Liu

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

Quoted 6 lines

Quoted 3 lines

* https://stackoverflow.com/questions/53234798/can-i-rebase-on-a-branchs-fork-point-without-explicitly-specifying-the-parent
* https://stackoverflow.com/questions/41529128/how-do-you-rebase-only-changes-between-two-branches-into-another-branch
* https://stackoverflow.com/a/4207357

Like with another series of yours I think this would be best squashed into one patch.

Will do.

Quoted 6 lines

Maybe I've misunderstood this but isn't this like --fork-point except with just plain "git merge-base" instead of "git merge-base --fork-point", but then again 2/3 shows multiple base aren't supported, but merge-base supports that.

Quoted 4 lines

--fork-point gets used to determine the _set of_ commits which are to be rebased, whereas --keep-base (and --onto) determine the base where that set of commits will be spliced. As a result, these two options cover orthogonal use-cases.

Right. After playing with this a bit more though --fork-point is mostly there, it it does find the same fork point, as evidenced all your tests (that aren't asserting incompatibility with other options) passing with this:

    diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
    index 9c2548423b..ab2d50e69a 100755
    --- a/t/t3416-rebase-onto-threedots.sh
    +++ b/t/t3416-rebase-onto-threedots.sh
    @@ -116,7 +116,7 @@ test_expect_success 'rebase --keep-base master from topic' '
            git checkout topic &&
            git reset --hard G &&

    -       git rebase --keep-base master &&
    +       git rebase $(git merge-base --fork-point master HEAD) &&
            git rev-parse C >base.expect &&
            git merge-base master HEAD >base.actual &&
            test_cmp base.expect base.actual &&
    @@ -140,7 +140,7 @@ test_expect_success 'rebase -i --keep-base master from topic' '
            git reset --hard G &&

            set_fake_editor &&
    -       EXPECT_COUNT=2 git rebase -i --keep-base master &&
    +       EXPECT_COUNT=2 git rebase -i $(git merge-base --fork-point master HEAD) &&
            git rev-parse C >base.expect &&
            git merge-base master HEAD >base.actual &&
            test_cmp base.expect base.actual &&

I've poked at some of this recently in
https://public-inbox.org/git/[email protected]/ as noted in the feedback there (I haven't gotten around to v2 yet) it's entirely possible that I haven't understood this at all :)

But it seems to me that this patch/implementation conflates two unrelated things.

Once is that we use --fork-point to mean that we're going to find the divergence point with "merge-base --fork-point". This gets you halfway to where you want to be, i.e. AFAICT the --keep-base and --fork-point will always find the same commit for "git rebase" and "git rebase --keep-base". See the "options.restrict_revision = get_fork_point(...)" part of the code.

The other, which you want to disable, is that --fork-point *also* says "OK, once we've found the divergence point, let's then rebase it on the latest upstream. Or in the example above the "master" part of "git merge-base --fork-point master HEAD".

Shouldn't --keep-base just be implemented in terms of skipping *that* part, i.e. we find the fork point using the upstream info, but then don't rebase *on* upstream.

The reason the distinction matters is because with your patch these two act differently:

    git rebase --keep-base
    git rebase $(git merge-base @{u} HEAD)

The latter will skip work ("Current branch master is up to date"), but --keep-base will always re-rebase things. There's some cases where --fork-point does that, which I was trying to address with my linked WIP patch above.

Whereas the thing you actually want to work is:

    git rebase -i --keep-base
    git rebase -i $(git merge-base @{u} HEAD)

I.e. to have both of those allow you to re-arrange/fixup whatever and still rebase on the same divergence point with @{u}, and won't run rebase when there's no work to do unless you give it --force-rebase.

Quoted 3 lines

reason that --onto already disallows multiple bases. If we have multiple bases, how do we determine which one is the "true base" to use? It makes more sense to error out and let the user manually specify it.

Ah, makes sense.

Quoted 17 lines

Quoted 6 lines

I'd find something like the "DISCUSSION ON FORK-POINT MODE" in git-merge-base helpful with examples of what we'd pick in the various scenarios, and also if whatever commit this picks was something you could have "git merge-base" spew out, so you could get what rebase would do here from other tooling (which maybe is possible, but I'm confused by the "no multiple bases"...).

If I'm understanding you correctly then yes, this could be done with other tooling. See the 0/3 for equivalent commands.

Perhaps I should update the rebase documentation to mention that --fork-point and --keep-base are orthogonal because it's unclear for you, it's probably unclear for other users as well.

Thanks,

Denton

Hi Ævar,

Quoted 75 lines · Ævar Arnfjörð Bjarmason

Quoted 31 lines · Denton Liu

Hi Ævar,

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Denton Liu

This series teaches rebase the --keep-base option.

'git rebase --keep-base <upstream>' is equivalent to 'git rebase --onto <upstream>... <upstream>' or
'git rebase --onto $(git merge-base <upstream> HEAD) <upstream>' .

This seems to be a common case that people (including myself!) run into; I was able to find these StackOverflow posts about this use case:

Quoted 6 lines

Quoted 3 lines

* https://stackoverflow.com/questions/53234798/can-i-rebase-on-a-branchs-fork-point-without-explicitly-specifying-the-parent
* https://stackoverflow.com/questions/41529128/how-do-you-rebase-only-changes-between-two-branches-into-another-branch
* https://stackoverflow.com/a/4207357

Like with another series of yours I think this would be best squashed into one patch.

Will do.

Quoted 6 lines

Maybe I've misunderstood this but isn't this like --fork-point except with just plain "git merge-base" instead of "git merge-base --fork-point", but then again 2/3 shows multiple base aren't supported, but merge-base supports that.

Quoted 4 lines

--fork-point gets used to determine the _set of_ commits which are to be rebased, whereas --keep-base (and --onto) determine the base where that set of commits will be spliced. As a result, these two options cover orthogonal use-cases.

Right. After playing with this a bit more though --fork-point is mostly there, it it does find the same fork point, as evidenced all your tests (that aren't asserting incompatibility with other options) passing with this:

    diff --git a/t/t3416-rebase-onto-threedots.sh b/t/t3416-rebase-onto-threedots.sh
    index 9c2548423b..ab2d50e69a 100755
    --- a/t/t3416-rebase-onto-threedots.sh
    +++ b/t/t3416-rebase-onto-threedots.sh
    @@ -116,7 +116,7 @@ test_expect_success 'rebase --keep-base master from topic' '
            git checkout topic &&
            git reset --hard G &&

    -       git rebase --keep-base master &&
    +       git rebase $(git merge-base --fork-point master HEAD) &&
            git rev-parse C >base.expect &&
            git merge-base master HEAD >base.actual &&
            test_cmp base.expect base.actual &&
    @@ -140,7 +140,7 @@ test_expect_success 'rebase -i --keep-base master from topic' '
            git reset --hard G &&

            set_fake_editor &&
    -       EXPECT_COUNT=2 git rebase -i --keep-base master &&
    +       EXPECT_COUNT=2 git rebase -i $(git merge-base --fork-point master HEAD) &&
            git rev-parse C >base.expect &&
            git merge-base master HEAD >base.actual &&
            test_cmp base.expect base.actual &&

I've poked at some of this recently in
https://public-inbox.org/git/[email protected]/ as noted in the feedback there (I haven't gotten around to v2 yet) it's entirely possible that I haven't understood this at all :)

But it seems to me that this patch/implementation conflates two unrelated things.

Quoted 6 lines

Once is that we use --fork-point to mean that we're going to find the divergence point with "merge-base --fork-point". This gets you halfway to where you want to be, i.e. AFAICT the --keep-base and --fork-point will always find the same commit for "git rebase" and "git rebase --keep-base". See the "options.restrict_revision = get_fork_point(...)" part of the code.

I don't think this is true. The code that --keep-base uses to find the merge base is get_oid_mb, see the relevant snippet

	if (strstr(options.onto_name, "...")) {
		if (get_oid_mb(options.onto_name, &merge_base) < 0)

whereas the --fork-point code uses get_fork_point, as you mentioned above. As a result, they don't necessarily refer to the same commit in the case where upstream is rewound.

Quoted 5 lines

The other, which you want to disable, is that --fork-point *also* says "OK, once we've found the divergence point, let's then rebase it on the latest upstream. Or in the example above the "master" part of "git merge-base --fork-point master HEAD".

Correct, I guess in essence this is what I'm doing.

Quoted 8 lines

Shouldn't --keep-base just be implemented in terms of skipping *that* part, i.e. we find the fork point using the upstream info, but then don't rebase *on* upstream.

The reason the distinction matters is because with your patch these two act differently:

Quoted 7 lines

    git rebase --keep-base
    git rebase $(git merge-base @{u} HEAD)

The latter will skip work ("Current branch master is up to date"), but --keep-base will always re-rebase things. There's some cases where --fork-point does that, which I was trying to address with my linked WIP patch above.

I believe this is desired behaviour. Suppose we have this (modified) graph from the git-merge-base docs, where B3 was formerly part of origin/master but it was then rewound:

           ---o---o---B2--o---o---o---B (origin/master)
                   \
                    B3
                     \
                      Derived (local master)

If we run "git rebase --keep-base", we'll get the following graph:

           ---o---o---B2--o---o---o---B (origin/master)
                   \
                    Derived (local master)

which I believe is the desired behaviour (we're abandoning B3 since upstream abandoned it).

I hope I'm understanding you correctly. Please let me know if I've misinterpreted anything you've said or if anything I've said is unclear.

Quoted 33 lines

Whereas the thing you actually want to work is:

    git rebase -i --keep-base
    git rebase -i $(git merge-base @{u} HEAD)

I.e. to have both of those allow you to re-arrange/fixup whatever and still rebase on the same divergence point with @{u}, and won't run rebase when there's no work to do unless you give it --force-rebase.

Quoted 3 lines

reason that --onto already disallows multiple bases. If we have multiple bases, how do we determine which one is the "true base" to use? It makes more sense to error out and let the user manually specify it.

Ah, makes sense.

Quoted 17 lines

Quoted 6 lines

I'd find something like the "DISCUSSION ON FORK-POINT MODE" in git-merge-base helpful with examples of what we'd pick in the various scenarios, and also if whatever commit this picks was something you could have "git merge-base" spew out, so you could get what rebase would do here from other tooling (which maybe is possible, but I'm confused by the "no multiple bases"...).

If I'm understanding you correctly then yes, this could be done with other tooling. See the 0/3 for equivalent commands.

Perhaps I should update the rebase documentation to mention that --fork-point and --keep-base are orthogonal because it's unclear for you, it's probably unclear for other users as well.

Thanks,

Denton

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

Helped-by Junio C Hamano <[email protected]>

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Denton Liu <[email protected]>

---

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

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

Documentation/git-rebase.txt +22 -3

index 6363d674b7..27be1f48ff 100644
@@ -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:
 

builtin/rebase.c +26 -6

index 77deebc65c..7c14a00460 100644
@@ -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 {

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

index ddf2f64853..9c2548423b 100755
@@ -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

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

Helped-by Junio C Hamano <[email protected]>

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Denton Liu <[email protected]>

---

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

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

Documentation/git-rebase.txt +22 -3

index 6363d674b7..27be1f48ff 100644
@@ -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:
 

builtin/rebase.c +26 -6

index 77deebc65c..7c14a00460 100644
@@ -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 {

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

index ddf2f64853..9c2548423b 100755
@@ -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
--

2.21.0.695.gaf8658f249

Hi Denton,

Quoted 8 lines · Denton Liu

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.

Maybe an example would clarify what you try to do here? Something like:

Example: When contributing a patch series to the Git mailing list, one often starts on top of the current master. However, while developing the patch series, master is also developed further, and it is sometimes not the best idea to keep rebasing on top of master, but to keep the base commit as-is. In such a case, the user can call

git rebase -i --onto master... master

as a shortcut to using the merge base between master and the current branch as base commit.

I wonder, however, whether it makes sense to introduce a shorter, sweeter way to do this:

git rebase -i master...

I.e. if we detect that the <upstream> argument is not a valid ref, that it ends with three dots, and that stripping those dots off makes it a valid ref, then we internally convert that to the same as --onto master... master.

What do you think?

Ciao,
Dscho

Quoted 265 lines

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

Helped-by Junio C Hamano <[email protected]>

Helped-by Ævar Arnfjörð Bjarmason <[email protected]>

Signed-off-by Denton Liu <[email protected]>

---

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

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

Documentation/git-rebase.txt +22 -3

index 6363d674b7..27be1f48ff 100644
@@ -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:
 

builtin/rebase.c +26 -6

index 77deebc65c..7c14a00460 100644
@@ -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 {

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

index ddf2f64853..9c2548423b 100755
@@ -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
--

2.21.0.695.gaf8658f249

Quoted 13 lines

Quoted 13 lines

I never use the "feature" myself, but I recall that when "git rebase" is run on a branch appropriately prepared, you do not even have to say <upstream> (iow, you type "git rebase<RET>" and rebase on top of @{upstream}).

Can this new "--keep-base" feature mesh well with it? When the current branch has forked from origin/master, for example, it would be good if

	$ git rebase -i --same-base

becomes a usable short-hand for

Quoted 4 lines

Quoted 1 line

	$ git rebase -i --same-base origin/master

By "--same-base", I am assuming you mistyped and meant to write "--keep-base"? If that's the case, I can make it a shorthand.

Sorry, I misunderstood your question. "--keep-base" already has the shorthand case handled by default.

Quoted 2 lines

I suspect that such a rebase will become no-op without "-i". Am I mistaken? I am not sure if "--keep-base" is useful without "-i".

It's useful in the case of "-x", although that is a grey area since "-x" uses interactive machinery internally. Aside from "-x", I can't really think of a situation where we would use "--keep-base" without "-i".

Hi Denton,

Quoted 4 lines · Denton Liu

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

Quoted 1 line

	git rebase -i --onto master... master

...
I wonder, however, whether it makes sense to introduce a shorter, sweeter way to do this:

git rebase -i master...

Before, when we had the following graph,

	A---B---C (master)
	    \
	     D (side)

running 'git rebase --onto master... master side' would result in D being always rebased, no matter what. However, the desired behavior is that rebase should notice that this is fast-forwardable and do that instead.

Add detection to can_fast_forward so that this case can be detected and a fast-forward will be performed.

---
 builtin/rebase.c               | 40 +++++++++++++++++++++++-----------
 t/t3400-rebase.sh              |  2 +-
 t/t3404-rebase-interactive.sh  |  2 +-
 t/t3432-rebase-fast-forward.sh |  2 +-
 4 files changed, 30 insertions(+), 16 deletions(-)

builtin/rebase.c +4 -4

index 77deebc65c..7aa6a090d4 100644
@@ -895,12 +895,12 @@ static int is_linear_history(struct commit *from, struct commit *to)
 	return 1;
 }
 
-static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
-			    struct object_id *merge_base)
+static int can_fast_forward(struct commit *onto, struct commit *upstream,
+			    struct object_id *head_oid, struct object_id *merge_base)
 {
 	struct commit *head = lookup_commit(the_repository, head_oid);
-	struct commit_list *merge_bases;
-	int res;
+	struct commit_list *merge_bases = NULL;
+	int res = 0;
 
 	if (!head)
 		return 0;

@@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
 	merge_bases = get_merge_bases(onto, head);

builtin/rebase.c +0 -0

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

t/t3400-rebase.sh +1 -1

index 460d0523be..604d624ff8 100755
@@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh and --show-current-patch' '
 		echo two >>init.t &&
 		git commit -a -m two &&
 		git tag two &&
-		test_must_fail git rebase --onto init HEAD^ &&
+		test_must_fail git rebase -f --onto init HEAD^ &&
 		GIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&
 		grep "show.*$(git rev-parse two)" stderr
 	)

t/t3404-rebase-interactive.sh +0 -0

index b60b11f9f2..f054186cc7 100755

Quoted 6 lines · Denton Liu

Before, when we had the following graph,

	A---B---C (master)
	    \
	     D (side)

Quoted 7 lines

running 'git rebase --onto master... master side' would result in D being always rebased, no matter what. However, the desired behavior is that rebase should notice that this is fast-forwardable and do that instead.

Add detection to can_fast_forward so that this case can be detected and a fast-forward will be performed.

OK. As long as the 'onto' commit is a strict ancestor of the side branch being rebased, the 'upstream' that is used only to determine which commits are on the side branch (essentially those that are not reachable from upstream but that are from the side branch) should not count in the equation to decide if we fast-forward or not. That makes sense.

Quoted 30 lines

---
 builtin/rebase.c               | 40 +++++++++++++++++++++++-----------
 t/t3400-rebase.sh              |  2 +-
 t/t3404-rebase-interactive.sh  |  2 +-
 t/t3432-rebase-fast-forward.sh |  2 +-
 4 files changed, 30 insertions(+), 16 deletions(-)

builtin/rebase.c +4 -4

index 77deebc65c..7aa6a090d4 100644
@@ -895,12 +895,12 @@ static int is_linear_history(struct commit *from, struct commit *to)
 	return 1;
 }
 
-static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
-			    struct object_id *merge_base)
+static int can_fast_forward(struct commit *onto, struct commit *upstream,
+			    struct object_id *head_oid, struct object_id *merge_base)
 {
 	struct commit *head = lookup_commit(the_repository, head_oid);
-	struct commit_list *merge_bases;
-	int res;
+	struct commit_list *merge_bases = NULL;
+	int res = 0;
 
 	if (!head)
 		return 0;

@@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
 	merge_bases = get_merge_bases(onto, head);

builtin/rebase.c static int can_fast_forward(struct commit *onto, struct object_id *head_oid, · line 916

Quoted 10 lines

builtin/rebase.c +3 -2

 	if (merge_bases && !merge_bases->next) {
 		oidcpy(merge_base, &merge_bases->item->object.oid);
-		res = oideq(merge_base, &onto->object.oid);
+		if (!oideq(merge_base, &onto->object.oid))
+			goto done;
 	} else {
 		oidcpy(merge_base, &null_oid);
-		res = 0;
+		goto done;
 	}

The above does not change any existing logic, but purely simplifies the code. In your picture in the log message

    A---B---C (master)
         \
          D (side)

where "rebase --onto master... master side" is run, "onto" in this function is B, and "head" is D. There is a single merge base B (i.e. merge_bases->next == NULL), so we jump to the label "done:" with res==0.

So why does the remainder of the function need to be changed?

builtin/rebase.c static int can_fast_forward(struct commit *onto, struct object_id *head_oid, · line 926

Quoted 10 lines

builtin/rebase.c +9 -0

+	if (!upstream)
+		goto done;
+
 	free_commit_list(merge_bases);
+	merge_bases = get_merge_bases(upstream, head);
+	if (merge_bases && !merge_bases->next) {
+		if (!oideq(&onto->object.oid, &merge_bases->item->object.oid))
+			goto done;
+	} else
+		goto done;

This computes the same between C and D (instead of B and D). Why is this needed?

Stepping back a bit, I understand that your argument we saw in the log message was that we only need to know if the commit we are transplanting the history on (i.e. "onto") already is an ancestor of the history being transplanted (i.e. "onto..head"), and it does not matter what upstream is. Am I mistaken? Why does this function now need to know what 'upstream' is?

Puzzled....

Quoted 67 lines · Junio C Hamano

Quoted 6 lines · Denton Liu

Before, when we had the following graph,

	A---B---C (master)
	    \
	     D (side)

Quoted 7 lines

running 'git rebase --onto master... master side' would result in D being always rebased, no matter what. However, the desired behavior is that rebase should notice that this is fast-forwardable and do that instead.

Add detection to can_fast_forward so that this case can be detected and a fast-forward will be performed.

OK. As long as the 'onto' commit is a strict ancestor of the side branch being rebased, the 'upstream' that is used only to determine which commits are on the side branch (essentially those that are not reachable from upstream but that are from the side branch) should not count in the equation to decide if we fast-forward or not. That makes sense.

Quoted 30 lines

---
 builtin/rebase.c               | 40 +++++++++++++++++++++++-----------
 t/t3400-rebase.sh              |  2 +-
 t/t3404-rebase-interactive.sh  |  2 +-
 t/t3432-rebase-fast-forward.sh |  2 +-
 4 files changed, 30 insertions(+), 16 deletions(-)

builtin/rebase.c +4 -4

index 77deebc65c..7aa6a090d4 100644
@@ -895,12 +895,12 @@ static int is_linear_history(struct commit *from, struct commit *to)
 	return 1;
 }
 
-static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
-			    struct object_id *merge_base)
+static int can_fast_forward(struct commit *onto, struct commit *upstream,
+			    struct object_id *head_oid, struct object_id *merge_base)
 {
 	struct commit *head = lookup_commit(the_repository, head_oid);
-	struct commit_list *merge_bases;
-	int res;
+	struct commit_list *merge_bases = NULL;
+	int res = 0;
 
 	if (!head)
 		return 0;

@@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
 	merge_bases = get_merge_bases(onto, head);

builtin/rebase.c static int can_fast_forward(struct commit *onto, struct object_id *head_oid, · line 916

Quoted 10 lines

builtin/rebase.c +3 -2

 	if (merge_bases && !merge_bases->next) {
 		oidcpy(merge_base, &merge_bases->item->object.oid);
-		res = oideq(merge_base, &onto->object.oid);
+		if (!oideq(merge_base, &onto->object.oid))
+			goto done;
 	} else {
 		oidcpy(merge_base, &null_oid);
-		res = 0;
+		goto done;
 	}

The above does not change any existing logic, but purely simplifies the code. In your picture in the log message

Quoted 8 lines

    A---B---C (master)
         \
          D (side)

where "rebase --onto master... master side" is run, "onto" in this function is B, and "head" is D. There is a single merge base B (i.e. merge_bases->next == NULL), so we jump to the label "done:" with res==0.

Quoted 16 lines

So why does the remainder of the function need to be changed?

builtin/rebase.c static int can_fast_forward(struct commit *onto, struct object_id *head_oid, · line 926

Quoted 10 lines

builtin/rebase.c +9 -0

+	if (!upstream)
+		goto done;
+
 	free_commit_list(merge_bases);
+	merge_bases = get_merge_bases(upstream, head);
+	if (merge_bases && !merge_bases->next) {
+		if (!oideq(&onto->object.oid, &merge_bases->item->object.oid))
+			goto done;
+	} else
+		goto done;

This computes the same between C and D (instead of B and D). Why is this needed?

Quoted 7 lines

Stepping back a bit, I understand that your argument we saw in the log message was that we only need to know if the commit we are transplanting the history on (i.e. "onto") already is an ancestor of the history being transplanted (i.e. "onto..head"), and it does not matter what upstream is. Am I mistaken? Why does this function now need to know what 'upstream' is?

Quoted 2 lines

Puzzled....

So I replaced the part for builtin/rebase.c from your patch with the above suggestion (attached) and the result seems to pass the three tests you touched. I am still puzzled.

Thanks.

 builtin/rebase.c               | 7 ++-----
 t/t3400-rebase.sh              | 2 +-
 t/t3404-rebase-interactive.sh  | 2 +-
 t/t3432-rebase-fast-forward.sh | 2 +-
 4 files changed, 5 insertions(+), 8 deletions(-)

builtin/rebase.c +2 -5

index 77deebc65c..fe61c2a899 100644
@@ -1682,13 +1682,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Check if we are already based on onto with linear history,
-	 * but this should be done only when upstream and onto are the same
-	 * and if this is not an interactive rebase.
+	 * but this should be done if this is not an interactive rebase.
 	 */
 	if (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&
-	    !is_interactive(&options) && !options.restrict_revision &&
-	    options.upstream &&
-	    !oidcmp(&options.upstream->object.oid, &options.onto->object.oid)) {
+	    !is_interactive(&options) && !options.restrict_revision) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {

t/t3400-rebase.sh +1 -1

index 460d0523be..604d624ff8 100755
@@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh and --show-current-patch' '
 		echo two >>init.t &&
 		git commit -a -m two &&
 		git tag two &&
-		test_must_fail git rebase --onto init HEAD^ &&
+		test_must_fail git rebase -f --onto init HEAD^ &&
 		GIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&
 		grep "show.*$(git rev-parse two)" stderr
 	)

t/t3404-rebase-interactive.sh +1 -1

index b60b11f9f2..f054186cc7 100755
@@ -1066,7 +1066,7 @@ test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-int
 	git reset --hard &&
 	git checkout conflict-branch &&
 	set_fake_editor &&
-	test_must_fail git rebase --onto HEAD~2 HEAD~ &&
+	test_must_fail git rebase -f --onto HEAD~2 HEAD~ &&
 	test_must_fail git rebase --edit-todo &&
 	git rebase --abort
 '

t/t3432-rebase-fast-forward.sh +1 -1

index 3e6362dd9c..414b4216d6 100755
@@ -54,6 +54,6 @@ test_expect_success 'add work to upstream' '
 changes='our and their changes'
 test_rebase_same_head success '--onto B B'
 test_rebase_same_head success '--onto B... B'
-test_rebase_same_head failure '--onto master... master'
+test_rebase_same_head success '--onto master... master'
 
 test_done

Signed-off-by Denton Liu <[email protected]>

---
 t/t3431-rebase-fork-point.sh | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 t/t3431-rebase-fork-point.sh

t/t3431-rebase-fork-point.sh new file +4 -0

new file mode 100755
index 0000000000..8e2483b73e
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Copyright (c) 2019 Denton Liu
+#

t/t3431-rebase-fork-point.sh new file +39 -0

+#
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	test_commit C &&
+	git branch -t side &&
+	git reset --hard HEAD^ &&
+	test_commit D &&
+	test_commit E &&
+	git checkout side &&
+	test_commit F &&
+	test_commit G
+'
+
+test_rebase() {
+	expected="$1" &&
+	shift &&
+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&
+		git checkout side &&
+		git reset --hard G &&
+		git rebase $@ &&
+		test_write_lines $expected >expect &&
+		git log --pretty=%s >actual &&
+		test_cmp expect actual
+	"
+}
+
+test_rebase 'G F E D B A' ''
+test_rebase 'G F D B A' '--onto D'
+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 E D B A' '--fork-point refs/heads/master'
+test_rebase 'G F D B A' '--fork-point --onto D refs/heads/master'
+test_rebase 'G F C E D B A' 'refs/heads/master'
+test_rebase 'G F C D B A' '--onto D refs/heads/master'
+
+test_done

Signature · 1 line

2.21.0.695.gaf8658f249

Signed-off-by Denton Liu <[email protected]>

---
 t/t3431-rebase-fork-point.sh | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 t/t3431-rebase-fork-point.sh

t/t3431-rebase-fork-point.sh new file +34 -0

new file mode 100755
index 0000000000..8e2483b73e
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Copyright (c) 2019 Denton Liu
+#
+
+test_description='git rebase --fork-point test'
+
+. ./test-lib.sh
+
+# A---B---D---E       (master)
+#     \
+#      C*---F---G (side)
+#
+# C was formerly part of master but is side out
+#
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	test_commit C &&
+	git branch -t side &&
+	git reset --hard HEAD^ &&
+	test_commit D &&
+	test_commit E &&
+	git checkout side &&
+	test_commit F &&
+	test_commit G
+'
+
+test_rebase() {
+	expected="$1" &&
+	shift &&
+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&

t/t3431-rebase-fork-point.sh new file +9 -0

+test_rebase 'G F D B A' '--onto D'
+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 E D B A' '--fork-point refs/heads/master'
+test_rebase 'G F D B A' '--fork-point --onto D refs/heads/master'
+test_rebase 'G F C E D B A' 'refs/heads/master'
+test_rebase 'G F C D B A' '--onto D refs/heads/master'
+
+test_done

Signature · 1 line

2.21.0.695.gaf8658f249

t/t3431-rebase-fork-point.sh new file +27 -0

new file mode 100755
index 0000000000..8e2483b73e
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Copyright (c) 2019 Denton Liu
+#
+
+test_description='git rebase --fork-point test'
+
+. ./test-lib.sh
+
+# A---B---D---E       (master)
+#     \
+#      C*---F---G (side)
+#
+# C was formerly part of master but is side out
+#
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	test_commit C &&
+	git branch -t side &&
+	git reset --hard HEAD^ &&
+	test_commit D &&
+	test_commit E &&
+	git checkout side &&
+	test_commit F &&
+	test_commit G
+'

t/t3431-rebase-fork-point.sh new file +16 -1

+		test_write_lines $expected >expect &&
+		git log --pretty=%s >actual &&
+		test_cmp expect actual
+	"
+}
+
+test_rebase 'G F E D B A' ''
+test_rebase 'G F D B A' '--onto D'
+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 E D B A' '--fork-point refs/heads/master'
+test_rebase 'G F D B A' '--fork-point --onto D refs/heads/master'
+test_rebase 'G F C E D B A' 'refs/heads/master'
+test_rebase 'G F C D B A' '--onto D refs/heads/master'
+
+test_done
--

2.21.0.695.gaf8658f249

Hi Denton,

Quoted 37 lines · Denton Liu

t/t3431-rebase-fork-point.sh new file +31 -0

new file mode 100755
index 0000000000..8e2483b73e
@@ -0,0 +1,53 @@
+#!/bin/sh
+#
+# Copyright (c) 2019 Denton Liu
+#
+
+test_description='git rebase --fork-point test'
+
+. ./test-lib.sh
+
+# A---B---D---E       (master)
+#     \
+#      C*---F---G (side)
+#
+# C was formerly part of master but is side out
+#
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	test_commit C &&
+	git branch -t side &&
+	git reset --hard HEAD^ &&
+	test_commit D &&
+	test_commit E &&
+	git checkout side &&
+	test_commit F &&
+	test_commit G
+'
+
+test_rebase() {
+	expected="$1" &&
+	shift &&

t/t3431-rebase-fork-point.sh line 37

Quoted 6 lines

t/t3431-rebase-fork-point.sh new file +6 -0

+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&
+		git checkout side &&
+		git reset --hard G &&
+		git rebase $@ &&

I think we need this patch, to make the macOS build happy:

-- snip --
Subject: fixup??? t3431: add rebase --fork-point tests

Try to fix the Mac build, which currently fails thusly:

++ git reset --hard G
HEAD is now at d8775ba G
++ git rebase $'\177'
fatal: invalid upstream '?'
error: last command exited with $?=128

Signed-off-by Johannes Schindelin <[email protected]>

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

index 4607e65de6..b41a0c0b68 100755
@@ -34,7 +34,7 @@ test_rebase() {
 		git reset --hard E &&
 		git checkout side &&
 		git reset --hard G &&
-		git rebase $@ &&
+		eval git rebase \"$@\" &&
 		test_write_lines $expected >expect &&
 		git log --pretty=%s >actual &&
 		test_cmp expect actual
-- snap --

Ciao,
Dscho

Quoted 20 lines

t/t3431-rebase-fork-point.sh new file +16 -1

+		test_write_lines $expected >expect &&
+		git log --pretty=%s >actual &&
+		test_cmp expect actual
+	"
+}
+
+test_rebase 'G F E D B A' ''
+test_rebase 'G F D B A' '--onto D'
+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 E D B A' '--fork-point refs/heads/master'
+test_rebase 'G F D B A' '--fork-point --onto D refs/heads/master'
+test_rebase 'G F C E D B A' 'refs/heads/master'
+test_rebase 'G F C D B A' '--onto D refs/heads/master'
+
+test_done
--

2.21.0.695.gaf8658f249

Quoted 12 lines · Denton Liu

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_rebase() {
+	expected="$1" &&
+	shift &&
+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&
+		git checkout side &&
+		git reset --hard G &&
+		git rebase $@ &&

I think we need this patch, to make the macOS build happy:

Thanks for digging into this, both.

Out of curiosity, t3432 is written similarly:

	test_rebase_same_head() {
		status="$1" &&
		shift &&
		test_expect_$status "git rebase $@ with $changes is no-op" "
			oldhead=\$(git rev-parse HEAD) &&
			test_when_finished 'git reset --hard \$oldhead' &&
			git rebase $@ &&
			newhead=\$(git rev-parse HEAD) &&
			test_cmp_rev \$oldhead \$newhead
		"
	}

Am 05.04.19 um 19:25 schrieb Denton Liu:

Quoted 13 lines · Johannes Schindelin

Quoted 12 lines · Denton Liu

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_rebase() {
+	expected="$1" &&
+	shift &&
+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&
+		git checkout side &&
+		git reset --hard G &&
+		git rebase $@ &&

I think we need this patch, to make the macOS build happy:

Quoted 4 lines

Thanks for digging into this, both.

Out of curiosity, t3432 is written similarly:

Quoted 11 lines

	test_rebase_same_head() {
		status="$1" &&
		shift &&
		test_expect_$status "git rebase $@ with $changes is no-op" "
			oldhead=\$(git rev-parse HEAD) &&
			test_when_finished 'git reset --hard \$oldhead' &&
			git rebase $@ &&
			newhead=\$(git rev-parse HEAD) &&
			test_cmp_rev \$oldhead \$newhead
		"
	}

Using $@ in these expansions is wrong. You do not want to forward an argument list, but you want to construct a command line. $* is correct here. Then you can remove the single-quotes at the invocation, like so:

	test_rebase_same_head success
	test_rebase_same_head success --onto B B

Function test_rebase() could be done in the same way, but the first argument, expected, still needs quotes at the call site, of course.

Quoted 17 lines · Johannes Sixt

Am 05.04.19 um 19:25 schrieb Denton Liu:

Quoted 11 lines · Johannes Schindelin

Quoted 10 lines · Denton Liu

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_rebase() {
+	expected="$1" &&
+	shift &&
+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&
+		git checkout side &&
+		git reset --hard G &&
+		git rebase $@ &&

Using $@ in these expansions is wrong. You do not want to forward an argument list, but you want to construct a command line. $* is correct here. Then you can remove the single-quotes at the invocation, like so:

Quoted 5 lines

	test_rebase_same_head success
	test_rebase_same_head success --onto B B

Function test_rebase() could be done in the same way, but the first argument, expected, still needs quotes at the call site, of course.

That's a good idea, let me run with it.

Hi,

Quoted 19 lines · Johannes Schindelin

Quoted 17 lines · Johannes Sixt

Am 05.04.19 um 19:25 schrieb Denton Liu:

Quoted 11 lines · Johannes Schindelin

Quoted 10 lines · Denton Liu

Review

Quoted 9 lines

(quoted diff) +9 -0

+test_rebase() {
+	expected="$1" &&
+	shift &&
+	test_expect_success "git rebase $@" "
+		git checkout master &&
+		git reset --hard E &&
+		git checkout side &&
+		git reset --hard G &&
+		git rebase $@ &&

Using $@ in these expansions is wrong. You do not want to forward an argument list, but you want to construct a command line. $* is correct here. Then you can remove the single-quotes at the invocation, like so:

Quoted 7 lines

Quoted 5 lines

	test_rebase_same_head success
	test_rebase_same_head success --onto B B

Function test_rebase() could be done in the same way, but the first argument, expected, still needs quotes at the call site, of course.

That's a good idea, let me run with it.

Indeed, this patch fixes it (see e.g.
https://dev.azure.com/Git-for-Windows/git/_build/results?buildId=34370):

-- snipsnap --
Subject: [PATCH] fixup??? t3431: add rebase --fork-point tests

Try to fix the Mac build, which currently fails thusly:

++ git reset --hard G
HEAD is now at d8775ba G
++ git rebase $'\177'
fatal: invalid upstream '?'
error: last command exited with $?=128

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3431-rebase-fork-point.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

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

index 4607e65de6..daa0c77467 100755
@@ -29,29 +29,29 @@ test_expect_success setup '
 test_rebase() {

t/t3431-rebase-fork-point.sh +12 -13

 		test_write_lines $expected >expect &&
 		git log --pretty=%s >actual &&
 		test_cmp expect actual
 	"
 }
 
-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 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'
-test_rebase 'G F E D B A' '--fork-point refs/heads/master'
-test_rebase 'G F D B A' '--fork-point --onto D refs/heads/master'
-test_rebase 'G F B A' '--fork-point --keep-base refs/heads/master'
-test_rebase 'G F C E D B A' 'refs/heads/master'
-test_rebase 'G F C D B A' '--onto D refs/heads/master'
-test_rebase 'G F C B A' '--keep-base refs/heads/master'
+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 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
+test_rebase 'G F E D B A' --fork-point refs/heads/master
+test_rebase 'G F D B A' --fork-point --onto D refs/heads/master
+test_rebase 'G F B A' --fork-point --keep-base refs/heads/master
+test_rebase 'G F C E D B A' refs/heads/master
+test_rebase 'G F C D B A' --onto D refs/heads/master
+test_rebase 'G F C B A' --keep-base refs/heads/master
 
 test_done
--

2.21.0.windows.1.152.g5895f170b6

Thanks again for your feedback, Ævar! I think we're both on the same page now. Hopefully I've addressed all of your high-level concerns with this patchset and we can move into a discussion on implementation detail.

Quoted 3 lines · Denton Liu

Thanks again for your feedback, Ævar! I think we're both on the same page now. Hopefully I've addressed all of your high-level concerns with this patchset and we can move into a discussion on implementation detail.

Late in replying to this, have been off-list. This also applies for your v4.

The current version you have still doesn't explain the "Why would we redundantly rebase every time in this case..." question I had in
https://public-inbox.org/git/[email protected]/

I *think* it's closer to "it was easier to implement this in terms of --onto, which happens to behave that way now" than "it must work this way for --keep-base", which is fair enough.

Although I see when I forward-port my POC patch from that E-Mail that one test fails now, which is good, that wasn't the case before, but it looks like that might be testing something else than just the lazy behavior.

It would be good to know in terms of commit message or (better) explicit tests so that if we teach these various rebase modes the same lazyness --fork-point uses in the future it's clear if that's OK or not.

t/t3431-rebase-fork-point.sh line 44

Quoted 8 lines

t/t3431-rebase-fork-point.sh new file +8 -0

+	test_expect_success "git rebase $@" "
+...
+		git rebase $@ &&
+...
+	"
+}
+
+test_rebase 'G F E D B A' ''

It appears that this last empty argument triggers some bug in Bash v4.2 and older (and on macOS such an old Bash is the default /bin/sh), as it turns that empty argument into something else, which in turn fails the test with:

<...>
++ git rebase $'\177'
fatal: invalid upstream '?'
error: last command exited with $?=128
not ok 2 - git rebase

https://travis-ci.org/git/git/jobs/516070862#L2276

Omitting that empty argument avoids this issue, and the test still checks what it was supposed to.

Quoted 12 lines

t/t3431-rebase-fork-point.sh new file +9 -0

+test_rebase 'G F D B A' '--onto D'
+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 E D B A' '--fork-point refs/heads/master'
+test_rebase 'G F D B A' '--fork-point --onto D refs/heads/master'
+test_rebase 'G F C E D B A' 'refs/heads/master'
+test_rebase 'G F C D B A' '--onto D refs/heads/master'
+
+test_done

Signature · 1 line

2.21.0.695.gaf8658f249

Quoted 10 lines · Ævar Arnfjörð Bjarmason

Quoted 3 lines · Denton Liu

Thanks again for your feedback, Ævar! I think we're both on the same page now. Hopefully I've addressed all of your high-level concerns with this patchset and we can move into a discussion on implementation detail.

Late in replying to this, have been off-list. This also applies for your v4.

Quoted 7 lines

The current version you have still doesn't explain the "Why would we redundantly rebase every time in this case..." question I had in
https://public-inbox.org/git/[email protected]/

I *think* it's closer to "it was easier to implement this in terms of --onto, which happens to behave that way now" than "it must work this way for --keep-base", which is fair enough.

Correct, the reason why --keep-base was not lazy initially was because "--onto did it that way". You are correct in that --keep-base should be lazily rebasing so I changed --onto's behaviour in 3/4 because it would also benefit from laziness. Thus, now --keep-base lazily rebases because --onto also does.

Quoted 5 lines

Although I see when I forward-port my POC patch from that E-Mail that one test fails now, which is good, that wasn't the case before, but it looks like that might be testing something else than just the lazy behavior.

The test fails because the patch disables fork_point if --keep-base is set. So, with the patch applied, C is rebased even though it is excluded when fork_point is set.

Quoted 1 line

Quoted 3 lines

It would be good to know in terms of commit message or (better) explicit tests so that if we teach these various rebase modes the same lazyness --fork-point uses in the future it's clear if that's OK or not.

Sorry, could you please clarify what you mean by the "lazyness --fork-point uses"? I don't understand what laziness is introduced by using --fork-point. Also, are the tests in t3432 not sufficient for testing fast-forwarding (aka lazy) behaviour?