Re: [BUG] rebase no longer omits local commits
- From
John Keeping <john@keeping.me.uk>
- Date
- Jul 7, 2014, 21:14 UTC
- Message-ID
- <20140707211456.GA2322@serenity.lan>
- In-Reply-To
- <xmqqbnt1dpdk.fsf@gitster.dls.corp.google.com>
On Mon, Jul 07, 2014 at 10:56:23AM -0700, Junio C Hamano wrote:
Show 30 quoted lines
> John Keeping <john@keeping.me.uk> 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.