threads / patch / 20127

patch, 2 partst5520-pull: Test for rebased upstream + fetch + pull --rebase

Subject: [PATCH 1/2] t5520-pull: Test for rebased upstream + fetch + pull --rebase

## tl;dr

19 messages between Jul 16, 2009 and Jul 19, 2009. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Santi Béjar· Jul 16, 2009, 00:09 UTC · lore
If your upstream has rebased you can do:
git pull --rebase
but only if you haven't fetch before.

Mark this case as test_expect_failure, in a later patch it will be changed to test_expect_success.

Signed-off-by: Santi Béjar <santi@agolina.net>
---
 t/t5520-pull.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
Show changes to t/t5520-pull.sh +14 −0
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c5a2e66..1aae494 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -117,6 +117,20 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
+test_expect_failure 'rebased upstream + fetch + pull --rebase' '
+
+	git update-ref refs/remotes/me/copy copy-orig &&
+	git reset --hard to-rebase-orig &&
+	git checkout --track -b to-rebase3 me/copy &&
+	git reset --hard to-rebase-orig &&
+	git fetch &&
+	test_must_fail git pull --rebase &&
+	git rebase --abort &&
+	test "conflicting modification" = "$(cat file)" &&
+	test file = $(cat file2)
+
+'
+
 test_expect_success 'pull --rebase dies early with dirty working directory' '
 
 	git checkout to-rebase &&
-- 
1.6.4.rc0.18.g60787.dirty
Santi Béjar· Jul 16, 2009, 00:09 UTC · re: Santi Béjar · lore

[PATCH 2/2] pull: support rebased upstream + fetch + pull --rebase

Use the fork commit of the current branch (where the tip of upstream branch used to be) as the upstream parameter of "git rebase". Compute it walking the reflog to find the first commit which is an ancestor of the current branch.

Signed-off-by: Santi Béjar <santi@agolina.net>
---
 git-pull.sh     |    9 +++++++--
 t/t5520-pull.sh |    5 ++---
 2 files changed, 9 insertions(+), 5 deletions(-)
Show changes to 2 files +9 −5

git-pull.sh, t/t5520-pull.sh

diff --git a/git-pull.sh b/git-pull.sh
index 4b78a0c..f5bef53 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -126,8 +126,13 @@ test true = "$rebase" && {
 
 	. git-parse-remote &&
 	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
-	oldremoteref="$(git rev-parse -q --verify \
-		"$reflist")"
+	num=0 &&
+	while oldremoteref="$(git rev-parse -q --verify "$reflist@{$num}")"
+	do
+		test $oldremoteref = $(git merge-base $oldremoteref $curr_branch) &&
+		break
+		num=$((num+1))
+	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity --update-head-ok "$@" || exit 1
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 1aae494..37a7e33 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -117,15 +117,14 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
-test_expect_failure 'rebased upstream + fetch + pull --rebase' '
+test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git reset --hard to-rebase-orig &&
 	git checkout --track -b to-rebase3 me/copy &&
 	git reset --hard to-rebase-orig &&
 	git fetch &&
-	test_must_fail git pull --rebase &&
-	git rebase --abort &&
+	git pull --rebase &&
 	test "conflicting modification" = "$(cat file)" &&
 	test file = $(cat file2)
 
-- 
1.6.4.rc0.18.g60787.dirty
Junio C Hamano· Jul 16, 2009, 00:26 UTC · re: Santi Béjar · lore

Re: [PATCH 2/2] pull: support rebased upstream + fetch + pull --rebase

Santi Béjar <santi@agolina.net> writes:
Show 5 quoted lines
>  	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> -	oldremoteref="$(git rev-parse -q --verify \
> -		"$reflist")"
> +	num=0 &&
> +	while oldremoteref="$(git rev-parse -q --verify "$reflist@{$num}")"

Applying @{nth} reflog notation to something that identifies itself as a "list" made me go "Huh?". Why is this variable called refLIST? Shouldn't it be simply called something like "remoteref" or even "ref"?

> +	do
> +		test $oldremoteref = $(git merge-base $oldremoteref $curr_branch) &&
> +		break
> +		num=$((num+1))

I think we always write "num=$(( $num + 1 ))" for portability; notice the lack of $ in your version.

> +	done
Does this loop ever give up?  Should it?

What happens in the subsequent code outside of the patch context, when this loop does not find any suitable "old" value?

Other than that, looking good.
Thanks.
Santi Béjar· Jul 16, 2009, 06:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/16 Junio C Hamano <gitster@pobox.com>:
Show 11 quoted lines
> Santi Béjar <santi@agolina.net> writes:
>
>>       reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
>> -     oldremoteref="$(git rev-parse -q --verify \
>> -             "$reflist")"
>> +     num=0 &&
>> +     while oldremoteref="$(git rev-parse -q --verify "$reflist@{$num}")"
>
> Applying @{nth} reflog notation to something that identifies itself as a
> "list" made me go "Huh?".  Why is this variable called refLIST?  Shouldn't
> it be simply called something like "remoteref" or even "ref"?

It used to be a list, before my patch 97af7ff (parse-remote: function to get the tracking branch to be merge, 2009-06-12). I'll change it.

Show 8 quoted lines
>
>> +     do
>> +             test $oldremoteref = $(git merge-base $oldremoteref $curr_branch) &&
>> +             break
>> +             num=$((num+1))
>
> I think we always write "num=$(( $num + 1 ))" for portability; notice the
> lack of $ in your version.

Oops, you are right. I somehow missed, I even did "git grep "((" *.sh" to check it.

>
>> +     done
>
> Does this loop ever give up?  Should it?

When remote/$origin/$branch@{nth} does not exist. I don't think we need another way to give up (nth<10?) because normally nth is small, it does not harm the normal case and it can help when nth is large.

>
> What happens in the subsequent code outside of the patch context, when
> this loop does not find any suitable "old" value?

Then the $oldremoteref is empty and in the git-rebase command it is used as ${oldremoteref:-$merge_head} so it get replaced by $merge_head.

Thanks, Santi

Santi Béjar· Jul 16, 2009, 08:12 UTC · re: Santi Béjar · lore

[PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

Use the fork commit of the current branch (where the tip of upstream branch used to be) as the upstream parameter of "git rebase". Compute it walking the reflog to find the first commit which is an ancestor of the current branch.

Signed-off-by: Santi Béjar <santi@agolina.net>
Changed since v1:
  - rename reflist to remoteref to better reflect its use
  - (( $num + 1 ))
---
 git-pull.sh     |   11 ++++++++---
 t/t5520-pull.sh |    5 ++---
 2 files changed, 10 insertions(+), 6 deletions(-)
Show changes to 2 files +10 −6

git-pull.sh, t/t5520-pull.sh

diff --git a/git-pull.sh b/git-pull.sh
index 4b78a0c..31d3945 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -125,9 +125,14 @@ test true = "$rebase" && {
 	die "refusing to pull with rebase: your working tree is not up-to-date"
 
 	. git-parse-remote &&
-	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
-	oldremoteref="$(git rev-parse -q --verify \
-		"$reflist")"
+	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	num=0 &&
+	while oldremoteref="$(git rev-parse -q --verify "$remoteref@{$num}")"
+	do
+		test $oldremoteref = $(git merge-base $oldremoteref $curr_branch) &&
+		break
+		num=$(( $num + 1 ))
+	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity --update-head-ok "$@" || exit 1
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 1aae494..37a7e33 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -117,15 +117,14 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
-test_expect_failure 'rebased upstream + fetch + pull --rebase' '
+test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git reset --hard to-rebase-orig &&
 	git checkout --track -b to-rebase3 me/copy &&
 	git reset --hard to-rebase-orig &&
 	git fetch &&
-	test_must_fail git pull --rebase &&
-	git rebase --abort &&
+	git pull --rebase &&
 	test "conflicting modification" = "$(cat file)" &&
 	test file = $(cat file2)
 
-- 
1.6.4.rc0.19.g1b31.dirty
Santi Béjar· Jul 16, 2009, 08:12 UTC · re: Santi Béjar · lore

[PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

  and here the interdiff between v1 and v2.
Santi
Show changes to git-pull.sh +3 −3
diff --git a/git-pull.sh b/git-pull.sh
index f5bef53..31d3945 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -125,13 +125,13 @@ test true = "$rebase" && {
 	die "refusing to pull with rebase: your working tree is not up-to-date"
 
 	. git-parse-remote &&
-	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
 	num=0 &&
-	while oldremoteref="$(git rev-parse -q --verify "$reflist@{$num}")"
+	while oldremoteref="$(git rev-parse -q --verify "$remoteref@{$num}")"
 	do
 		test $oldremoteref = $(git merge-base $oldremoteref $curr_branch) &&
 		break
-		num=$((num+1))
+		num=$(( $num + 1 ))
 	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
Santi Béjar· Jul 16, 2009, 08:15 UTC · re: Santi Béjar · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/16 Santi Béjar <santi@agolina.net>:
Show 10 quoted lines
> Use the fork commit of the current branch (where
> the tip of upstream branch used to be) as the upstream parameter of
> "git rebase". Compute it walking the reflog to find the first commit
> which is an ancestor of the current branch.
>
> Signed-off-by: Santi Béjar <santi@agolina.net>
>
> Changed since v1:
>  - rename reflist to remoteref to better reflect its use
>  - (( $num + 1 ))

Arg! It should not be in the commit message! Sorry. Junio, can you amend it if applied? Thanks

Santi
Johannes Schindelin· Jul 16, 2009, 08:51 UTC · re: Santi Béjar · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

Hi,
On Thu, 16 Jul 2009, Santi Béjar wrote:
> Use the fork commit of the current branch (where
> the tip of upstream branch used to be) as the upstream parameter of
> "git rebase". Compute it walking the reflog to find the first commit
> which is an ancestor of the current branch.
I finally understand what this patch is about.  Thanks.
Show 15 quoted lines
> diff --git a/git-pull.sh b/git-pull.sh
> index 4b78a0c..31d3945 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -125,9 +125,14 @@ test true = "$rebase" && {
>  	die "refusing to pull with rebase: your working tree is not up-to-date"
>  
>  	. git-parse-remote &&
> -	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> -	oldremoteref="$(git rev-parse -q --verify \
> -		"$reflist")"
> +	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> +	num=0 &&
> +	while oldremoteref="$(git rev-parse -q --verify "$remoteref@{$num}")"
> +	do
How about
	oldremoteref="$(git rev-list --boundary HEAD --not \
			$(git rev-list -g $remoteref | sed 's/$/^@/') |
		sed -e '/^[^-]/d' -e q)"
Explanation: the "git rev-list -g $remoteref" lists the previous commits 
the remote ref pointed to, and the ^@ appended to them means all their 
parents.  Now, the outer rev-list says to take everything in HEAD but 
_not_ in those parents, showing the boundary commits.  The "sed" call 
lists the first such boundary commit (which must, by construction, be one 
of the commits shown by the first rev-list).

But maybe this is trying to be too clever, and we should not bother with it until git-pull is made a builtin?

Ciao, Dscho

Santi Béjar· Jul 16, 2009, 16:32 UTC · re: Johannes Schindelin · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/16 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 10 quoted lines
> Hi,
>
> On Thu, 16 Jul 2009, Santi Béjar wrote:
>
>> Use the fork commit of the current branch (where
>> the tip of upstream branch used to be) as the upstream parameter of
>> "git rebase". Compute it walking the reflog to find the first commit
>> which is an ancestor of the current branch.
>
> I finally understand what this patch is about.  Thanks.

Thanks, it was hard (at least for me) to provide a short and good commit message.

Show 29 quoted lines
>
>> diff --git a/git-pull.sh b/git-pull.sh
>> index 4b78a0c..31d3945 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -125,9 +125,14 @@ test true = "$rebase" && {
>>       die "refusing to pull with rebase: your working tree is not up-to-date"
>>
>>       . git-parse-remote &&
>> -     reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
>> -     oldremoteref="$(git rev-parse -q --verify \
>> -             "$reflist")"
>> +     remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
>> +     num=0 &&
>> +     while oldremoteref="$(git rev-parse -q --verify "$remoteref@{$num}")"
>> +     do
>
> How about
>
>        oldremoteref="$(git rev-list --boundary HEAD --not \
>                        $(git rev-list -g $remoteref | sed 's/$/^@/') |
>                sed -e '/^[^-]/d' -e q)"
>
> Explanation: the "git rev-list -g $remoteref" lists the previous commits
> the remote ref pointed to, and the ^@ appended to them means all their
> parents.  Now, the outer rev-list says to take everything in HEAD but
> _not_ in those parents, showing the boundary commits.  The "sed" call
> lists the first such boundary commit (which must, by construction, be one
> of the commits shown by the first rev-list).

It almost works, thanks. In fact this is how I represent it in my head, but I couldn't find a working command (hint, hint, the --boundaries trick). Based on yours here it is the one I am using right now:

	oldremoteref="$(git rev-list --boundary HEAD --not \
		$(git rev-list -g $remoteref 2>/dev/null) |
		sed -e '/^[^-]/d' -e 's/^-//;q' )"

i.e. without the ^@ as you want the commits in the reflog as boundary commits, and also remove the - in front of the commit.

Your version performs equally than mine for the normal case but much better if it has to walk many reflog entries. Also mine has the problem, at least currently, that it does not give up as "git rev-parse -q --verify $branch@{n}" does not return an error when n is too large:

  $ git rev-parse -q --verify origin/next@{18} ; echo $?
warning: Log for 'origin/next' only has 17 entries.
37eb784cfce07ba0048d64e352c5137454396d87
0
even with "-q --verify"!

So, I'll take yours and will send an updated patch (saying that is is based on a command by you). With your Signed-off-by?

Thanks, Santi

Johannes Schindelin· Jul 17, 2009, 10:13 UTC · re: Santi Béjar · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

Hi,
On Thu, 16 Jul 2009, Santi Béjar wrote:
Show 13 quoted lines
> 2009/7/16 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > On Thu, 16 Jul 2009, Santi Béjar wrote:
> >
> >> Use the fork commit of the current branch (where
> >> the tip of upstream branch used to be) as the upstream parameter of
> >> "git rebase". Compute it walking the reflog to find the first commit
> >> which is an ancestor of the current branch.
> >
> > I finally understand what this patch is about.  Thanks.
> 
> Thanks, it was hard (at least for me) to provide a short and good
> commit message.

It is the thing I found quite hard when I started contributing to Git, and it is still not exactly easy for me.

Show 40 quoted lines
> >> diff --git a/git-pull.sh b/git-pull.sh
> >> index 4b78a0c..31d3945 100755
> >> --- a/git-pull.sh
> >> +++ b/git-pull.sh
> >> @@ -125,9 +125,14 @@ test true = "$rebase" && {
> >>       die "refusing to pull with rebase: your working tree is not up-to-date"
> >>
> >>       . git-parse-remote &&
> >> -     reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> >> -     oldremoteref="$(git rev-parse -q --verify \
> >> -             "$reflist")"
> >> +     remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> >> +     num=0 &&
> >> +     while oldremoteref="$(git rev-parse -q --verify "$remoteref@{$num}")"
> >> +     do
> >
> > How about
> >
> >        oldremoteref="$(git rev-list --boundary HEAD --not \
> >                        $(git rev-list -g $remoteref | sed 's/$/^@/') |
> >                sed -e '/^[^-]/d' -e q)"
> >
> > Explanation: the "git rev-list -g $remoteref" lists the previous commits
> > the remote ref pointed to, and the ^@ appended to them means all their
> > parents.  Now, the outer rev-list says to take everything in HEAD but
> > _not_ in those parents, showing the boundary commits.  The "sed" call
> > lists the first such boundary commit (which must, by construction, be one
> > of the commits shown by the first rev-list).
> 
> It almost works, thanks. In fact this is how I represent it in my
> head, but I couldn't find a working command (hint, hint, the
> --boundaries trick). Based on yours here it is the one I am using
> right now:
> 
> 	oldremoteref="$(git rev-list --boundary HEAD --not \
> 		$(git rev-list -g $remoteref 2>/dev/null) |
> 		sed -e '/^[^-]/d' -e 's/^-//;q' )"
> 
> i.e. without the ^@ as you want the commits in the reflog as boundary
> commits, and also remove the - in front of the commit.

Thanks for fixing it. I should have mentioned that I did not test it (and usually stuff I do not test has blatant bugs in it, such as was the case here).

Show 14 quoted lines
> Your version performs equally than mine for the normal case but much 
> better if it has to walk many reflog entries. Also mine has the problem, 
> at least currently, that it does not give up as "git rev-parse -q 
> --verify $branch@{n}" does not return an error when n is too large:
> 
>   $ git rev-parse -q --verify origin/next@{18} ; echo $?
> warning: Log for 'origin/next' only has 17 entries.
> 37eb784cfce07ba0048d64e352c5137454396d87
> 0
> 
> even with "-q --verify"!
> 
> So, I'll take yours and will send an updated patch (saying that is is
> based on a command by you). With your Signed-off-by?
Maybe an ACK instead?

Thanks, Dscho

Junio C Hamano· Jul 16, 2009, 20:41 UTC · re: Johannes Schindelin · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 12 quoted lines
> How about
>
> 	oldremoteref="$(git rev-list --boundary HEAD --not \
> 			$(git rev-list -g $remoteref | sed 's/$/^@/') |
> 		sed -e '/^[^-]/d' -e q)"
>
> Explanation: the "git rev-list -g $remoteref" lists the previous commits 
> the remote ref pointed to, and the ^@ appended to them means all their 
> parents.  Now, the outer rev-list says to take everything in HEAD but 
> _not_ in those parents, showing the boundary commits.  The "sed" call 
> lists the first such boundary commit (which must, by construction, be one 
> of the commits shown by the first rev-list).
Hmm, I am not sure about that "(which must..." part.  When you have
          Y---X
         /
	B---o---o---o---H
wouldn't "rev-list --boundary H --not X^@" give B, not X nor Y?
Santi Béjar· Jul 16, 2009, 23:18 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/16 Junio C Hamano <gitster@pobox.com>:
Show 23 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> How about
>>
>>       oldremoteref="$(git rev-list --boundary HEAD --not \
>>                       $(git rev-list -g $remoteref | sed 's/$/^@/') |
>>               sed -e '/^[^-]/d' -e q)"
>>
>> Explanation: the "git rev-list -g $remoteref" lists the previous commits
>> the remote ref pointed to, and the ^@ appended to them means all their
>> parents.  Now, the outer rev-list says to take everything in HEAD but
>> _not_ in those parents, showing the boundary commits.  The "sed" call
>> lists the first such boundary commit (which must, by construction, be one
>> of the commits shown by the first rev-list).
>
> Hmm, I am not sure about that "(which must..." part.  When you have
>
>          Y---X
>         /
>        B---o---o---o---H
>
> wouldn't "rev-list --boundary H --not X^@" give B, not X nor Y?
>

$git rev-list --boundary H --not X and $git rev-list --boundary H --not X^@

return the same output in this case: o o o -B

In this case the correct command is without ^@, because you want the commits in the reflog as boundary commits.

In the simpler and usual case, without a rebased upstream:
    z---B---o---o---o---H
B=upstream@{0}

$git rev-list --boundary H --not B^@ o o o B -z

and:

$git rev-list --boundary H --not B o o o -B

Also in the rebased upstream case:
      Y---X
     /
    z---B---o---o---o---H

X=upstream@{0} B=upstream@{1}

$git rev-list --boundary H --not X^@ B^@ o o o B -z

and:

$git rev-list --boundary H --not X B o o o -B

HTH, Santi

Santi Béjar· Jul 17, 2009, 07:51 UTC · re: Santi Béjar · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/17 Santi Béjar <santi@agolina.net>:
Show 17 quoted lines
> 2009/7/16 Junio C Hamano <gitster@pobox.com>:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> How about
>>>
>>>       oldremoteref="$(git rev-list --boundary HEAD --not \
>>>                       $(git rev-list -g $remoteref | sed 's/$/^@/') |
>>>               sed -e '/^[^-]/d' -e q)"
>>>
>>> Explanation: the "git rev-list -g $remoteref" lists the previous commits
>>> the remote ref pointed to, and the ^@ appended to them means all their
>>> parents.  Now, the outer rev-list says to take everything in HEAD but
>>> _not_ in those parents, showing the boundary commits.  The "sed" call
>>> lists the first such boundary commit (which must, by construction, be one
>>> of the commits shown by the first rev-list).
>>
>> Hmm, I am not sure about that "(which must..." part.

Unfortunatly you are right with the "(which must..." part. Even without the ^@. Normally gives the right answer, but it is not sure that the first commit boundary is the correct one. For example:

         o--C
        /
 A--x--y--B--o--z
     \      /
      o----o
A, B, C are upstream@{n}

It involves a merge with a branch forked before the fork commit for the current branch, and it will not work neither with git pull --rebase. We could say that it is not supported, but nevertheless it gives the wrong answer.

The right answer is B, but: $ git rev-list --boundary z --not C B A z o o o -x -B

in this case we could take the boundaries commits and filter the commits that are ancestor of some other boundary commit, and would get B (git show-branch --independent x B -> B).

Here it is a test case to see the above.

rm -rf test mkdir test cd test git init echo A > file git add . git commit -mA git tag A echo o > file git commit -a -mo git tag fork1 echo z > file git commit -a -mz git tag fork2 echo B > file git commit -a -mB git tag B git checkout -b topic fork1 echo oo > filetopic git add . git commit -moo echo ooo > filetopic git commit -a -mooo git checkout master git merge topic echo c > file git commit -a -mc git checkout -b upstream fork2 echo o > fileupstream git add . git commit -a -mo5 echo C > fileupstream git commit -a -mo git tag C git rev-list --boundary master --not C B A echo answer: $(git rev-parse B^{})

HTH, Santi

Junio C Hamano· Jul 17, 2009, 08:25 UTC · re: Santi Béjar · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

Santi Béjar <santi@agolina.net> writes:
Show 45 quoted lines
> 2009/7/17 Santi Béjar <santi@agolina.net>:
>> 2009/7/16 Junio C Hamano <gitster@pobox.com>:
>>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>>
>>>> How about
>>>>
>>>>       oldremoteref="$(git rev-list --boundary HEAD --not \
>>>>                       $(git rev-list -g $remoteref | sed 's/$/^@/') |
>>>>               sed -e '/^[^-]/d' -e q)"
>>>>
>>>> Explanation: the "git rev-list -g $remoteref" lists the previous commits
>>>> the remote ref pointed to, and the ^@ appended to them means all their
>>>> parents.  Now, the outer rev-list says to take everything in HEAD but
>>>> _not_ in those parents, showing the boundary commits.  The "sed" call
>>>> lists the first such boundary commit (which must, by construction, be one
>>>> of the commits shown by the first rev-list).
>>>
>>> Hmm, I am not sure about that "(which must..." part.
>
> Unfortunatly you are right with the "(which must..." part. Even
> without the ^@. Normally gives the right answer, but it is not
> sure that the first commit boundary is the correct one. For
> example:
>
>          o--C
>         /
>  A--x--y--B--o--z
>      \      /
>       o----o
>
> A, B, C are upstream@{n}
>
> It involves a merge with a branch forked before the fork commit
> for the current branch, and it will not work neither with git
> pull --rebase. We could say that it is not supported, but
> nevertheless it gives the wrong answer.
>
> The right answer is B, but:
> $ git rev-list --boundary z --not C B A
> z
> o
> o
> o
> -x
> -B

Now a short question. Does your original loop give a correct answer in this case?

Santi Béjar· Jul 17, 2009, 13:24 UTC · re: Junio C Hamano · lore

Re: [PATCHv2 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/17 Junio C Hamano <gitster@pobox.com>:
Show 50 quoted lines
> Santi Béjar <santi@agolina.net> writes:
>
>> 2009/7/17 Santi Béjar <santi@agolina.net>:
>>> 2009/7/16 Junio C Hamano <gitster@pobox.com>:
>>>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>>>
>>>>> How about
>>>>>
>>>>>       oldremoteref="$(git rev-list --boundary HEAD --not \
>>>>>                       $(git rev-list -g $remoteref | sed 's/$/^@/') |
>>>>>               sed -e '/^[^-]/d' -e q)"
>>>>>
>>>>> Explanation: the "git rev-list -g $remoteref" lists the previous commits
>>>>> the remote ref pointed to, and the ^@ appended to them means all their
>>>>> parents.  Now, the outer rev-list says to take everything in HEAD but
>>>>> _not_ in those parents, showing the boundary commits.  The "sed" call
>>>>> lists the first such boundary commit (which must, by construction, be one
>>>>> of the commits shown by the first rev-list).
>>>>
>>>> Hmm, I am not sure about that "(which must..." part.
>>
>> Unfortunatly you are right with the "(which must..." part. Even
>> without the ^@. Normally gives the right answer, but it is not
>> sure that the first commit boundary is the correct one. For
>> example:
>>
>>          o--C
>>         /
>>  A--x--y--B--o--z
>>      \      /
>>       o----o
>>
>> A, B, C are upstream@{n}
>>
>> It involves a merge with a branch forked before the fork commit
>> for the current branch, and it will not work neither with git
>> pull --rebase. We could say that it is not supported, but
>> nevertheless it gives the wrong answer.
>>
>> The right answer is B, but:
>> $ git rev-list --boundary z --not C B A
>> z
>> o
>> o
>> o
>> -x
>> -B
>
> Now a short question.  Does your original loop give a correct answer in
> this case?

Yes, it returns B. But there are other cases where there is not a single right answer if you allow merges. For the moment the more sensible thing to do is to not allow merges in the local commits. I hope nobody relies on "git pull --rebase" with local merges.

Just an example:
        E
       /
      D----a topic2
     /      \
 A--B--C--b--c--d--topic1
A, B, C, D, E are upstream@{n} (n = 4,3,2,1,0)

if you are on branch "topic1", and run "git pull --rebase" you would want to rebase only b, c and d (maybe "a" but you should not), but for sure not D. And my algorithm returns C, but a more "correct" answer would be C and D.

Another possibility could be to check that there is only one boundary commit (only one fork point for all the local commits).

Wait! Let's return to the original problem. The original problem is that you cannot do a "git pull --rebase" with a rebased upstream if you have already done "git fetch" before. And the solution would be: Try to behaved as if the "git fetch" was not run.

And this is exactly what my patch does.
All the other "problems" happens already.

Now I only have to solve the "git rev-parse -q --verify upstream@{large_n}" problem or workaround it.

Sometimes thinks aloud, Santi

Santi Béjar· Jul 18, 2009, 13:46 UTC · re: Santi Béjar · lore

[PATCHv3 2/2] pull: support rebased upstream + fetch + pull --rebase

The problem is that you cannot do a "git pull --rebase" with a rebased upstream if you have already run "git fetch" before. And the solution: Try to behaved as if the "git fetch" was not run.

Or in other words, use the fork commit of the current branch (where the tip of upstream branch used to be) as the upstream parameter of "git rebase".

Compute it walking the reflog to find the first commit which is an ancestor of the current branch. Maybe there are smarter ways to compute it, but this is a straight forward implementation of the above "Try to behaved as if the "git fetch" was not run".

Signed-off-by: Santi Béjar <santi@agolina.net>
---
Changes since v2:
  - Hopefully enhance the commit log
  - Use a 'for' loop for the reflog entries
  - provide a default value in case there is no reflog
Changed since v1:
  - rename reflist to remoteref to better reflect its use
  - (( $num + 1 ))
 git-pull.sh     |   11 +++++++++--
 t/t5520-pull.sh |    5 ++---
 2 files changed, 11 insertions(+), 5 deletions(-)
Show changes to 2 files +11 −5

git-pull.sh, t/t5520-pull.sh

diff --git a/git-pull.sh b/git-pull.sh
index 4b78a0c..c8f1674 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -125,9 +125,16 @@ test true = "$rebase" && {
 	die "refusing to pull with rebase: your working tree is not up-to-date"
 
 	. git-parse-remote &&
-	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	oldremoteref= &&
+	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
+	do
+		test $reflog = $(git merge-base $reflog $curr_branch) &&
+		oldremoteref=$reflog && break
+	done
+	[ -z "$oldremoteref" ] &&
 	oldremoteref="$(git rev-parse -q --verify \
-		"$reflist")"
+		"$remoteref")"
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity --update-head-ok "$@" || exit 1
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 1aae494..37a7e33 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -117,15 +117,14 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
-test_expect_failure 'rebased upstream + fetch + pull --rebase' '
+test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git reset --hard to-rebase-orig &&
 	git checkout --track -b to-rebase3 me/copy &&
 	git reset --hard to-rebase-orig &&
 	git fetch &&
-	test_must_fail git pull --rebase &&
-	git rebase --abort &&
+	git pull --rebase &&
 	test "conflicting modification" = "$(cat file)" &&
 	test file = $(cat file2)
 
-- 
1.6.3.2.408.g8ecf
Junio C Hamano· Jul 18, 2009, 17:55 UTC · re: Santi Béjar · lore

Re: [PATCHv3 2/2] pull: support rebased upstream + fetch + pull --rebase

Santi Béjar <santi@agolina.net> writes:
Show 25 quoted lines
> Changes since v2:
>   - Hopefully enhance the commit log
>   - Use a 'for' loop for the reflog entries
>   - provide a default value in case there is no reflog
> diff --git a/git-pull.sh b/git-pull.sh
> index 4b78a0c..c8f1674 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -125,9 +125,16 @@ test true = "$rebase" && {
>  	die "refusing to pull with rebase: your working tree is not up-to-date"
>  
>  	. git-parse-remote &&
> -	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> +	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
> +	oldremoteref= &&
> +	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
> +	do
> +		test $reflog = $(git merge-base $reflog $curr_branch) &&
> +		oldremoteref=$reflog && break
> +	done
> +	[ -z "$oldremoteref" ] &&
>  	oldremoteref="$(git rev-parse -q --verify \
> -		"$reflist")"
> +		"$remoteref")"
>  }
Looks nicer.
I notice that you are breaking && chain with this patch.

If get_remote_merge_branch fails, oldremoteref is not initialized to empty string, the for loop is skipped and then the last step (by the way, please write that as 'test -z "$oldremoteref"') may not kick in, using whatever random value the variable originally had in the environment.

It probably makes more sense to do it in a slightly different order:
        . git-parse-remote &&
        oldremoteref="$(get_remote_merge...)" &&
	remoteref=$oldremoteref &&
        for old in $(git rev-list -g "$remoteref" 2>/dev/null)
        do
        	if test "$old" = "$(git merge-base "$old" "$current_branch")
		then
			oldremoteref="$old"
			break
                fi
	done
	# and you do not need 'if test -z "$oldremoteref"' anymore...

But other than that, I agree that this is the most straightforward algorithm to express what you wanted to do. I guess another possibility is to instead look in the reflog of the _current_ branch to check how the previous rebase was done, iow, find out onto which commit the recent part of the current branch was rebased to, and rebase onto the current remote tip using that as the base.

Santi Béjar· Jul 19, 2009, 07:27 UTC · re: Junio C Hamano · lore

Re: [PATCHv3 2/2] pull: support rebased upstream + fetch + pull --rebase

2009/7/18 Junio C Hamano <gitster@pobox.com>:
Show 32 quoted lines
> Santi Béjar <santi@agolina.net> writes:
>
>> Changes since v2:
>>   - Hopefully enhance the commit log
>>   - Use a 'for' loop for the reflog entries
>>   - provide a default value in case there is no reflog
>> diff --git a/git-pull.sh b/git-pull.sh
>> index 4b78a0c..c8f1674 100755
>> --- a/git-pull.sh
>> +++ b/git-pull.sh
>> @@ -125,9 +125,16 @@ test true = "$rebase" && {
>>       die "refusing to pull with rebase: your working tree is not up-to-date"
>>
>>       . git-parse-remote &&
>> -     reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
>> +     remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
>> +     oldremoteref= &&
>> +     for reflog in $(git rev-list -g $remoteref 2>/dev/null)
>> +     do
>> +             test $reflog = $(git merge-base $reflog $curr_branch) &&
>> +             oldremoteref=$reflog && break
>> +     done
>> +     [ -z "$oldremoteref" ] &&
>>       oldremoteref="$(git rev-parse -q --verify \
>> -             "$reflist")"
>> +             "$remoteref")"
>>  }
>
> If get_remote_merge_branch fails, oldremoteref is not initialized to empty
> string, the for loop is skipped and then the last step (by the way, please
> write that as 'test -z "$oldremoteref"') may not kick in, using whatever
> random value the variable originally had in the environment.
Not a justification but it also happens with the original code.
Show 5 quoted lines
>
> It probably makes more sense to do it in a slightly different order:
>
>        . git-parse-remote &&
>        oldremoteref="$(get_remote_merge...)" &&

oldremotref must be a sha1, because it changes during the fetch. But I'll use more o less this order.

Show 17 quoted lines
>        remoteref=$oldremoteref &&
>        for old in $(git rev-list -g "$remoteref" 2>/dev/null)
>        do
>                if test "$old" = "$(git merge-base "$old" "$current_branch")
>                then
>                        oldremoteref="$old"
>                        break
>                fi
>        done
>        # and you do not need 'if test -z "$oldremoteref"' anymore...
>
> But other than that, I agree that this is the most straightforward
> algorithm to express what you wanted to do.  I guess another possibility
> is to instead look in the reflog of the _current_ branch to check how the
> previous rebase was done, iow, find out onto which commit the recent part
> of the current branch was rebased to, and rebase onto the current remote
> tip using that as the base.

It supposes that it was rebased already, and you have to interpret the reflog history. And there are situations where you don't get the same answer, i.e you create a new local branch using another local branch as the starting point but with the same upstream.

Santi
Santi Béjar· Jul 19, 2009, 07:45 UTC · re: Santi Béjar · lore

[PATCHv4 2/2] pull: support rebased upstream + fetch + pull --rebase

The problem is that you cannot do a "git pull --rebase" with a rebased upstream if you have already run "git fetch" before. And the solution: Try to behaved as if the "git fetch" was not run.

Or in other words, use the fork commit of the current branch (where the tip of upstream branch used to be) as the upstream parameter of "git rebase".

Compute it walking the reflog to find the first commit which is an ancestor of the current branch. Maybe there are smarter ways to compute it, but this is a straight forward implementation of the above "Try to behaved as if the "git fetch" was not run".

Signed-off-by: Santi Béjar <santi@agolina.net>
---
Changes since v3:
  - Provide the default at the begining
  - some quotes
  - replace && chain with an if
Changes since v2:
  - Hopefully enhance the commit log
  - Use a 'for' loop for the reflog entries
  - provide a default value in case there is no reflog
Changed since v1:
  - rename reflist to remoteref to better reflect its use
  - (( $num + 1 ))
 git-pull.sh     |   13 +++++++++++--
 t/t5520-pull.sh |    5 ++---
 2 files changed, 13 insertions(+), 5 deletions(-)
Show changes to 2 files +13 −5

git-pull.sh, t/t5520-pull.sh

diff --git a/git-pull.sh b/git-pull.sh
index 4b78a0c..a7795b0 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -124,10 +124,19 @@ test true = "$rebase" && {
 	git diff-index --ignore-submodules --cached --quiet HEAD -- ||
 	die "refusing to pull with rebase: your working tree is not up-to-date"
 
+	oldremoteref= &&
 	. git-parse-remote &&
-	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
+	remoteref="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
 	oldremoteref="$(git rev-parse -q --verify \
-		"$reflist")"
+		"$remoteref")" &&
+	for reflog in $(git rev-list -g $remoteref 2>/dev/null)
+	do
+		if test "$reflog" = "$(git merge-base $reflog $curr_branch)"
+		then
+			oldremoteref="$reflog"
+			break
+		fi
+	done
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity --update-head-ok "$@" || exit 1
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 1aae494..37a7e33 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -117,15 +117,14 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
-test_expect_failure 'rebased upstream + fetch + pull --rebase' '
+test_expect_success 'rebased upstream + fetch + pull --rebase' '
 
 	git update-ref refs/remotes/me/copy copy-orig &&
 	git reset --hard to-rebase-orig &&
 	git checkout --track -b to-rebase3 me/copy &&
 	git reset --hard to-rebase-orig &&
 	git fetch &&
-	test_must_fail git pull --rebase &&
-	git rebase --abort &&
+	git pull --rebase &&
 	test "conflicting modification" = "$(cat file)" &&
 	test file = $(cat file2)
 
-- 
1.6.4.rc1.2.gb13e8

← back to recent threads