[RFC/PATCH] rebase: use reflog to find common base with upstream

[RFC/PATCH] rebase: use reflog to find common base with upstream

[John Keeping]

Commit 15a147e (rebase: use @{upstream} if no upstream specified,
2011-02-09) says:

	Make it default to 'git rebase @{upstream}'. That is also what
	'git pull [--rebase]' defaults to, so it only makes sense that
	'git rebase' defaults to the same thing.

but that isn't actually the case.  Since commit d44e712 (pull: support
rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually
chosen the most recent reflog entry which is an ancestor of the current
branch if it can find one.

Change rebase so that it uses the same logic, but only when no upstream
argument is given on the command line.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
Last time this came up [1], there was some discussion about moving the
added block of code to affect upstreams given on the command line as
well as when the upstream is discovered from the config.  Having tried
that, it has some more fallout on the test suite than I like; this
pattern ends up dropping the tip commit of "side" because it's in the
reflog of "master":

	# start on "master"
	git branch side &&
	git reset --hard HEAD^ &&
	git checkout side &&
	git rebase master

I wonder if it would be better to add a --fork-point argument to
git-rebase and default it to true when no upstream is given on the
command line.

[1] http://article.gmane.org/gmane.comp.version-control.git/236252

 git-rebase.sh     | 6 ++++++
 t/t3400-rebase.sh | 6 ++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 226752f..17801ad 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -437,6 +437,12 @@ then
 			error_on_missing_default_upstream "rebase" "rebase" \
 				"against" "git rebase <branch>"
 		fi
+
+		fork_point=$(git merge-base --fork-point "$upstream_name" HEAD)
+		if test -n "$fork_point" && test "$fork_point" != "$upstream"
+		then
+			upstream=$fork_point
+		fi
 		;;
 	*)	upstream_name="$1"
 		shift
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ebf93b0..998503d 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -134,12 +134,14 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
-test_expect_success 'default to @{upstream} when upstream arg is missing' '
+test_expect_success 'default to common base in @{upstream}s reflog if no upstream arg' '
 	git checkout -b default topic &&
 	git config branch.default.remote . &&
 	git config branch.default.merge refs/heads/master &&
 	git rebase &&
-	test "$(git rev-parse default~1)" = "$(git rev-parse master)"
+	git rev-parse --verify master >expect &&
+	git rev-parse default~1 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -q is quiet' '
-- 
1.8.5.226.g0d60d77

Re: [RFC/PATCH] rebase: use reflog to find common base with upstream

[Martin von Zweigbergk]

On Sun, Dec 8, 2013 at 12:06 PM, John Keeping <john@keeping.me.uk> wrote:
> Commit 15a147e (rebase: use @{upstream} if no upstream specified,
> 2011-02-09) says:
>
>         Make it default to 'git rebase @{upstream}'. That is also what
>         'git pull [--rebase]' defaults to, so it only makes sense that
>         'git rebase' defaults to the same thing.
>
> but that isn't actually the case.  Since commit d44e712 (pull: support
> rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually
> chosen the most recent reflog entry which is an ancestor of the current
> branch if it can find one.

In my mind, 'git pull --rebase' does default to @{u}, it just started
interpreting it differently in d44e712, but maybe I'm just being
defensive :-).

In a similar way, I think your patch is about interpreting the
upstream argument differently, not about changing the default upstream
argument. This is why I think "git rebase" and "git rebase
origin/master" should be the same (when origin/master is the
configured upstream).

Re: [RFC/PATCH] rebase: use reflog to find common base with upstream

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> Last time this came up [1], there was some discussion about moving the
> added block of code to affect upstreams given on the command line as
> well as when the upstream is discovered from the config.  Having tried
> that, it has some more fallout on the test suite than I like; this
> pattern ends up dropping the tip commit of "side" because it's in the
> reflog of "master":
>
> 	# start on "master"
> 	git branch side &&
> 	git reset --hard HEAD^ &&
> 	git checkout side &&
> 	git rebase master

We shouldn't do anything funky using the reflog when an explicit
commit object name was given like in the last step above, I think.
Automation to help human end-users is good, but at some level there
must be a mechanism to reliably reproduce the same result given the
same precondition for those who implement such automation, and I do
not think it is a good idea to force scripts to say

	git rebase --do-not-look-at-reflog master

in order to do so.

> I wonder if it would be better to add a --fork-point argument to
> git-rebase and default it to true when no upstream is given on the
> command line.

I am not sure what you exactly mean by "when no upstream is given",
though.  Do you mean

	git rebase <no other arguments>

which we interpret as "rebase the current branch on @{u}", and it
should behave as if the command was run like so:

	git rebase --fork-point @{u}

If that is what you suggest, I certainly can buy that.  Those who
want to disable the automation can explicitly say

	git rebase @{u}

and rebase the current exactly on top of the named commit (e.g. the
current value of refs/remotes/origin/master or whatever remote-tracking
branch you forked from).

Re: [RFC/PATCH] rebase: use reflog to find common base with upstream

[John Keeping]

On Mon, Dec 09, 2013 at 12:11:50PM -0800, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
> 
> > Last time this came up [1], there was some discussion about moving the
> > added block of code to affect upstreams given on the command line as
> > well as when the upstream is discovered from the config.  Having tried
> > that, it has some more fallout on the test suite than I like; this
> > pattern ends up dropping the tip commit of "side" because it's in the
> > reflog of "master":
> >
> > 	# start on "master"
> > 	git branch side &&
> > 	git reset --hard HEAD^ &&
> > 	git checkout side &&
> > 	git rebase master
> 
> We shouldn't do anything funky using the reflog when an explicit
> commit object name was given like in the last step above, I think.
> Automation to help human end-users is good, but at some level there
> must be a mechanism to reliably reproduce the same result given the
> same precondition for those who implement such automation, and I do
> not think it is a good idea to force scripts to say
> 
> 	git rebase --do-not-look-at-reflog master
> 
> in order to do so.
> 
> > I wonder if it would be better to add a --fork-point argument to
> > git-rebase and default it to true when no upstream is given on the
> > command line.
> 
> I am not sure what you exactly mean by "when no upstream is given",
> though.  Do you mean
> 
> 	git rebase <no other arguments>
> 
> which we interpret as "rebase the current branch on @{u}", and it
> should behave as if the command was run like so:
> 
> 	git rebase --fork-point @{u}
> 
> If that is what you suggest, I certainly can buy that.  Those who
> want to disable the automation can explicitly say
> 
> 	git rebase @{u}
> 
> and rebase the current exactly on top of the named commit (e.g. the
> current value of refs/remotes/origin/master or whatever remote-tracking
> branch you forked from).

Yes, that's what I meant; the first non-option argument to "git rebase"
is called "upstream" in the manpage (and throughout the code).  So if
"<no other arguments>" means "<no non-option arguments>" then that's
exactly what I meant.

[PATCH v2] rebase: use reflog to find common base with upstream

[John Keeping]

Commit 15a147e (rebase: use @{upstream} if no upstream specified,
2011-02-09) says:

	Make it default to 'git rebase @{upstream}'. That is also what
	'git pull [--rebase]' defaults to, so it only makes sense that
	'git rebase' defaults to the same thing.

but that isn't actually the case.  Since commit d44e712 (pull: support
rebased upstream + fetch + pull --rebase, 2009-07-19), pull has actually
chosen the most recent reflog entry which is an ancestor of the current
branch if it can find one.

Add a '--fork-point' argument to git-rebase that can be used to trigger
this behaviour.  This option is turned on by default if no non-option
arguments are specified on the command line, otherwise we treat an
upstream specified on the command-line literally.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
On Mon, Dec 09, 2013 at 08:40:08PM +0000, John Keeping wrote:
> On Mon, Dec 09, 2013 at 12:11:50PM -0800, Junio C Hamano wrote:
> >          Do you mean
> > 
> > 	git rebase <no other arguments>
> > 
> > which we interpret as "rebase the current branch on @{u}", and it
> > should behave as if the command was run like so:
> > 
> > 	git rebase --fork-point @{u}
> > 
> > If that is what you suggest, I certainly can buy that.  Those who
> > want to disable the automation can explicitly say
> > 
> > 	git rebase @{u}
> > 
> > and rebase the current exactly on top of the named commit (e.g. the
> > current value of refs/remotes/origin/master or whatever remote-tracking
> > branch you forked from).
> 
> Yes, that's what I meant; the first non-option argument to "git rebase"
> is called "upstream" in the manpage (and throughout the code).  So if
> "<no other arguments>" means "<no non-option arguments>" then that's
> exactly what I meant.

Here's an updated patch that adds a --fork-point option to git-rebase,
with the behaviour described above.

 Documentation/git-rebase.txt | 10 ++++++++++
 git-rebase.sh                | 19 +++++++++++++++++++
 t/t3400-rebase.sh            |  6 ++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 94e07fd..2889be6 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -324,6 +324,16 @@ fresh commits so it can be remerged successfully without needing to "revert
 the reversion" (see the
 link:howto/revert-a-faulty-merge.html[revert-a-faulty-merge How-To] for details).
 
+--fork-point::
+--no-fork-point::
+	Use 'git merge-base --fork-point' to find a better common ancestor
+	between `upstream` and `branch` when calculating which commits have
+	have been introduced by `branch` (see linkgit:git-merge-base[1]).
++
+If no non-option arguments are given on the command line, then the default is
+`--fork-point @{u}` otherwise the `upstream` argument is interpreted literally
+unless the `--fork-point` option is specified.
+
 --ignore-whitespace::
 --whitespace=<option>::
 	These flag are passed to the 'git apply' program
diff --git a/git-rebase.sh b/git-rebase.sh
index 226752f..7185dc8 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -14,6 +14,7 @@ git-rebase --continue | --abort | --skip | --edit-todo
 v,verbose!         display a diffstat of what changed upstream
 q,quiet!           be quiet. implies --no-stat
 autostash!         automatically stash/stash pop before and after
+fork-point         use 'merge-base --fork-point' to refine upstream
 onto=!             rebase onto given branch instead of upstream
 p,preserve-merges! try to recreate merges instead of ignoring them
 s,strategy=!       use the given merge strategy
@@ -66,6 +67,7 @@ verbose=
 diffstat=
 test "$(git config --bool rebase.stat)" = true && diffstat=t
 autostash="$(git config --bool rebase.autostash || echo false)"
+fork_point=auto
 git_am_opt=
 rebase_root=
 force_rebase=
@@ -260,6 +262,12 @@ do
 	--no-autosquash)
 		autosquash=
 		;;
+	--fork-point)
+		fork_point=t
+		;;
+	--no-fork-point)
+		fork_point=
+		;;
 	-M|-m)
 		do_merge=t
 		;;
@@ -437,6 +445,8 @@ then
 			error_on_missing_default_upstream "rebase" "rebase" \
 				"against" "git rebase <branch>"
 		fi
+
+		test "$fork_point" = auto && fork_point=t
 		;;
 	*)	upstream_name="$1"
 		shift
@@ -522,6 +532,15 @@ case "$#" in
 	;;
 esac
 
+if test "$fork_point" = t
+then
+	new_upstream=$(git merge-base --fork-point "$upstream_name" "$switch_to")
+	if test -n "$new_upstream"
+	then
+		upstream=$new_upstream
+	fi
+fi
+
 if test "$autostash" = true && ! (require_clean_work_tree) 2>/dev/null
 then
 	stash_sha1=$(git stash create "autostash") ||
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ebf93b0..998503d 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -134,12 +134,14 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
-test_expect_success 'default to @{upstream} when upstream arg is missing' '
+test_expect_success 'default to common base in @{upstream}s reflog if no upstream arg' '
 	git checkout -b default topic &&
 	git config branch.default.remote . &&
 	git config branch.default.merge refs/heads/master &&
 	git rebase &&
-	test "$(git rev-parse default~1)" = "$(git rev-parse master)"
+	git rev-parse --verify master >expect &&
+	git rev-parse default~1 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'rebase -q is quiet' '
-- 
1.8.5.226.g0d60d77