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

Santi Béjar [off-list ref] 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.

2009/7/16 Junio C Hamano [off-list ref]:

Santi Béjar [off-list ref] writes:

quoted

      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.

quoted

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

quoted

+     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

  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)

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

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

2009/7/16 Santi Béjar [off-list ref]:

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

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

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.

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

2009/7/16 Johannes Schindelin [off-list ref]:

Hi,

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

quoted

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.

quoted

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

Hi,

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

2009/7/16 Johannes Schindelin [off-list ref]:

quoted

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

quoted

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.

quoted

quoted

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