Re: [BUG] rebase no longer omits local commits
On Mon, Jul 07, 2014 at 10:56:23AM -0700, Junio C Hamano wrote:
> John Keeping writes:
>
> > Perhaps we shuld do something like this (which passes the test suite):
> >
> > -- >8 --
> > diff --git a/git-rebase.sh b/git-rebase.sh
> > index 06c810b..0c6c5d3 100755
> > --- a/git-rebase.sh
> > +++ b/git-rebase.sh
> > @@ -544,7 +544,8 @@ if test "$fork_point" = t
> > then
> > new_upstream=$(git merge-base --fork-point "$upstream_name" \
> > "${switch_to:-HEAD}")
> > - if test -n "$new_upstream"
> > + if test -n "$new_upstream" &&
> > + ! git merge-base --is-ancestor "$new_upstream" "$upstream_name"
> > then
> > upstream=$new_upstream
> > fi
> > -- 8< --
> >
> > Since the intent of `--fork-point` is to find the best starting point
> > for the "$upstream...$orig_head" range, if the fork point is behind the
> > new location of the upstream then should we leave the upstream as it
> > was?
>
> Probably; but the check to avoid giving worse fork-point should be
> in the implementation of "merge-base --fork-point" itself, so that
> we do not have to do the above to both "rebase" and "pull --rebase",
> no?
I don't think so, since in that case we're not actually finding the fork
point as defined in the documentation, we're finding the upstream rebase
wants.
Having played with this a bit, I think we shouldn't be replacing the
upstream with the fork point but should instead add the fork point as an
additional negative ref:
$upstream...$orig_head ^$fork_point
Here's a script that creates a repository showing this:
-- >8 --
#!/bin/sh
git init rebase-test &&
cd rebase-test &&
echo one >file &&
git add file &&
git commit -m one &&
echo frist >file2 &&
git add file2 &&
git commit -m first &&
git branch --track dev &&
echo first >file2 &&
git commit -a --amend --no-edit &&
echo two >file &&
git commit -a -m two &&
echo three >file &&
git commit -a -m three &&
echo second >file2 &&
git commit -a -m second &&
git checkout dev &&
git cherry-pick -2 master &&
echo four >file &&
git commit -a -m four &&
printf '\nWithout fork point (old behaviour)\n' &&
git rev-list --oneline --cherry @{u}... &&
printf '\nFork point as upstream (current behaviour)\n' &&
git rev-list --oneline --cherry $(git merge-base --fork-point master HEAD)... &&
printf '\nWith fork point\n' &&
git rev-list --oneline --cherry @{u}... ^$(git merge-base --fork-point master
HEAD)
-- 8< --
In this case the rebase should be clean since the only applicable patch
changes "three" to "four" in "file", but the current rebase code fails
both with `--fork-point` and with `--no-fork-point`.
With `--fork-point` we try to apply "two" and "three" which have already
been cherry-picked across (as Ted originally reported) and with
`--no-fork-point`, we try to apply "first" which conflicts because we
have the version prior to it being fixed up on master.
I hacked up git-rebase to test this and the change to use the fork point
as in the last line of the script above does indeed make the rebase go
through cleanly, but I have not yet looked at how to cleanly patch in
that behaviour.
I haven't tested git-pull, but it looks like it has always (since 2009)
behaved in the way `git rebase --fork-point` does now, failing to detect
cherry-picked commits that are now in the upstream.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] rebase no longer omits local commits
John Keeping writes:
> Perhaps we shuld do something like this (which passes the test suite):
>
> -- >8 --
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 06c810b..0c6c5d3 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -544,7 +544,8 @@ if test "$fork_point" = t
> then
> new_upstream=$(git merge-base --fork-point "$upstream_name" \
> "${switch_to:-HEAD}")
> - if test -n "$new_upstream"
> + if test -n "$new_upstream" &&
> +! git merge-base --is-ancestor "$new_upstream" "$upstream_name"
> then
> upstream=$new_upstream
> fi
> -- 8< --
>
> Since the intent of `--fork-point` is to find the best starting point
> for the "$upstream...$orig_head" range, if the fork point is behind the
> new location of the upstream then should we leave the upstream as it
> was?
Probably; but the check to avoid giving worse fork-point should be
in the implementation of "merge-base --fork-point" itself, so that
we do not have to do the above to both "rebase" and "pull --rebase",
no?
> I haven't thought through this completely, but it seems like we should
> be doing a check like the above, at least when we're in
> "$fork_point=auto" mode.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] rebase no longer omits local commits
On Thu, Jul 03, 2014 at 08:09:17PM +0100, John Keeping wrote:
> On Thu, Jul 03, 2014 at 11:14:26AM -0400, Ted Felix wrote:
> > Starting with git 1.9.0, rebase no longer omits local commits that
> > appear in both the upstream and local branches.
>
> It is the problem that bb3f458 fixes. The change in behaviour is
> actually introduced by ad8261d (rebase: use reflog to find common base
> with upstream).
>
> In your example, I think this is working as designed. You can restore
> the previous behaviour either with `git rebase --no-fork-point` or with
> `git rebase @{u}`.
>
> The change is designed to help users recover from an upstream rebase, as
> described in the "DISCUSSION ON FORK-POINT MODE" section of
> git-merge-base(1).
Having thought about this a bit more, I think the case you've identified
is an unexpected side effect of that commit.
Perhaps we shuld do something like this (which passes the test suite):
-- >8 --
diff --git a/git-rebase.sh b/git-rebase.sh
index 06c810b..0c6c5d3 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -544,7 +544,8 @@ if test "$fork_point" = t
then
new_upstream=$(git merge-base --fork-point "$upstream_name" \
"${switch_to:-HEAD}")
- if test -n "$new_upstream"
+ if test -n "$new_upstream" &&
+ ! git merge-base --is-ancestor "$new_upstream" "$upstream_name"
then
upstream=$new_upstream
fi
-- 8< --
Since the intent of `--fork-point` is to find the best starting point
for the "$upstream...$orig_head" range, if the fork point is behind the
new location of the upstream then should we leave the upstream as it
was?
I haven't thought through this completely, but it seems like we should
be doing a check like the above, at least when we're in
"$fork_point=auto" mode.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] rebase no longer omits local commits
On Thu, Jul 03, 2014 at 11:14:26AM -0400, Ted Felix wrote:
> Starting with git 1.9.0, rebase no longer omits local commits that
> appear in both the upstream and local branches.
>
> I've bisected this down to commit bb3f458: "rebase: fix fork-point with
> zero arguments". The attached script reproduces the problem. Reverting
> the aforementioned commit fixes the problem.
>
> A failed run of this script will result in conflicts. A successful run
> against master with bb3f458 reverted ends as follows:
>
> From /tmp/rebase-issue/maint
> fe401cd..955af04 master -> origin/master
> fatal: Not a valid object name: ''
> First, rewinding head to replay your work on top of it...
> Applying: Third change
>
> (I'm not sure if that "fatal: Not a valid object name: ''" is of any
> concern. It started appearing for me at some point during the bisect.)
It is the problem that bb3f458 fixes. The change in behaviour is
actually introduced by ad8261d (rebase: use reflog to find common base
with upstream).
In your example, I think this is working as designed. You can restore
the previous behaviour either with `git rebase --no-fork-point` or with
`git rebase @{u}`.
The change is designed to help users recover from an upstream rebase, as
described in the "DISCUSSION ON FORK-POINT MODE" section of
git-merge-base(1) and makes `git rebase` match the behaviour of
`git pull --rebase` so that:
git fetch &&
git rebase
really is equivalent to:
git pull --rebase
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
