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

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

[Santi Béjar]

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

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

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

[Santi Béjar]

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

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

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

[Junio C Hamano]

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"?

> +	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.

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

[Santi Béjar]

2009/7/16 Junio C Hamano <gitster@pobox.com>:
> 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.

>
>> +     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

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

[Santi Béjar]

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

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

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

[Santi Béjar]

  and here the interdiff between v1 and v2.

Santi

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)

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

[Santi Béjar]

2009/7/16 Santi Béjar <santi@agolina.net>:
> 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

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1574 bytes --]

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.

> 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

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

[Santi Béjar]

2009/7/16 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> 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.

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3251 bytes --]

Hi,

On Thu, 16 Jul 2009, Santi Béjar wrote:

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

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

> 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

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

[Junio C Hamano]

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?

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

[Santi Béjar]

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

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

[Santi Béjar]

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

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

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

[Junio C Hamano]

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?

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

[Santi Béjar]

2009/7/17 Junio C Hamano <gitster@pobox.com>:
> 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

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

[Santi Béjar]

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

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

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

[Junio C Hamano]

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")"
>  }

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.

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

[Santi Béjar]

2009/7/18 Junio C Hamano <gitster@pobox.com>:
> 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.

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

>        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

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

[Santi Béjar]

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

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