[PATCHv4 0/4] git pull --rebase fixes and cleanup

[PATCHv4 0/4] git pull --rebase fixes and cleanup

[Santi Béjar]

Changes since v3:
  - Extend 1/4 commit message to explain why there are not behavior changes

Santi Béjar (4):
  parse-remote: function to get the tracking branch to be merge
  parse-remote: support default reflist in get_remote_merge_branch
  t5520-pull: --rebase with rebased upstream and two branches
  parse-remote: remove unused functions

 Documentation/git-parse-remote.txt |   20 ----
 git-parse-remote.sh                |  222 ++++-------------------------------
 git-pull.sh                        |    7 +-
 t/t5520-pull.sh                    |   29 +++++
 4 files changed, 57 insertions(+), 221 deletions(-)

[PATCHv4 1/4] parse-remote: function to get the tracking branch to be merge

[Santi Béjar]

The only user of get_remote_refs_for_fetch was "git pull --rebase" and
it only wanted the tracking branch to be merge. So, add a simple
function (get_remote_merge_branch) with this new meaning.

No behavior changes. The new function behaves like the old code in
"git pull --rebase". In particular, it only works with the default
refspec mapping and with remote branches, not tags.

Signed-off-by: Santi Béjar <santi@agolina.net>
---

Changes since v3:
  - Explain why the new function does not change the behavior.
  - Remove the for loop.

 git-parse-remote.sh |   29 +++++++++++++++++++++++++++++
 git-pull.sh         |    7 ++-----
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index a296719..a991564 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -229,6 +229,34 @@ get_remote_refs_for_fetch () {
 	esac
 }
 
+get_remote_merge_branch () {
+	case "$#" in
+	0|1)
+	    die "internal error: get-remote-merge-branch." ;;
+	*)
+	    repo=$1
+	    shift
+	    ref=$1
+	    # FIXME: It should return the tracking branch
+	    #        Currently only works with the default mapping
+	    case "$ref" in
+	    +*)
+		ref=$(expr "z$ref" : 'z+\(.*\)')
+		;;
+	    esac
+	    expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
+	    remote=$(expr "z$ref" : 'z\([^:]*\):')
+	    case "$remote" in
+	    '' | HEAD ) remote=HEAD ;;
+	    heads/*) remote=${remote#heads/} ;;
+	    refs/heads/*) remote=${remote#refs/heads/} ;;
+	    refs/* | tags/* | remotes/* ) remote=
+	    esac
+
+	    [ -n "$remote" ] && echo "refs/remotes/$repo/$remote"
+	esac
+}
+
 resolve_alternates () {
 	# original URL (xxx.git)
 	top_=`expr "z$1" : 'z\([^:]*:/*[^/]*\)/'`
@@ -262,3 +290,4 @@ get_uploadpack () {
 		;;
 	esac
 }
+
diff --git a/git-pull.sh b/git-pull.sh
index 3526153..3cf2663 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -125,12 +125,9 @@ test true = "$rebase" && {
 	die "refusing to pull with rebase: your working tree is not up-to-date"
 
 	. git-parse-remote &&
-	origin="$1"
-	test -z "$origin" && origin=$(get_default_remote)
-	reflist="$(get_remote_refs_for_fetch "$@" 2>/dev/null |
-		sed "s|refs/heads/\(.*\):|\1|")" &&
+	reflist="$(get_remote_merge_branch "$@" 2>/dev/null)" &&
 	oldremoteref="$(git rev-parse -q --verify \
-		"refs/remotes/$origin/$reflist")"
+		"$reflist")"
 }
 orig_head=$(git rev-parse -q --verify HEAD)
 git fetch $verbosity --update-head-ok "$@" || exit 1
-- 
1.6.3.2.206.g417f7

[PATCHv4 2/4] parse-remote: support default reflist in get_remote_merge_branch

[Santi Béjar]

Expand get_remote_merge_branch to compute the tracking branch to merge
when called without arguments (or only the remote name). This allows
"git pull --rebase" without arguments (default upstream branch) to
work with a rebased upstream. With explicit arguments it already worked.

Also add a test to check for this case.

Signed-off-by: Santi Béjar <santi@agolina.net>
---

Changes since v1:
  - Hopefully provide a more useful and correct commit message
Changes since v2:
  - Add sentence about the "with explicit arguments" case
  - Move the test about two branch to a different patch
  - Restore the executable bit

 git-parse-remote.sh |    8 +++++++-
 t/t5520-pull.sh     |   14 ++++++++++++++
 2 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index a991564..75e1254 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -232,7 +232,13 @@ get_remote_refs_for_fetch () {
 get_remote_merge_branch () {
 	case "$#" in
 	0|1)
-	    die "internal error: get-remote-merge-branch." ;;
+	    origin="$1"
+	    default=$(get_default_remote)
+	    test -z "$origin" && origin=$default
+	    curr_branch=$(git symbolic-ref -q HEAD)
+	    [ "$origin" = "$default" ] &&
+	    echo $(git for-each-ref --format='%(upstream)' $curr_branch)
+	    ;;
 	*)
 	    repo=$1
 	    shift
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 725771f..c5a2e66 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -92,20 +92,34 @@ test_expect_success '--rebase with rebased upstream' '
 
 	git remote add -f me . &&
 	git checkout copy &&
+	git tag copy-orig &&
 	git reset --hard HEAD^ &&
 	echo conflicting modification > file &&
 	git commit -m conflict file &&
 	git checkout to-rebase &&
 	echo file > file2 &&
 	git commit -m to-rebase file2 &&
+	git tag to-rebase-orig &&
 	git pull --rebase me copy &&
 	test "conflicting modification" = "$(cat file)" &&
 	test file = $(cat file2)
 
 '
 
+test_expect_success '--rebase with rebased default upstream' '
+
+	git update-ref refs/remotes/me/copy copy-orig &&
+	git checkout --track -b to-rebase2 me/copy &&
+	git reset --hard to-rebase-orig &&
+	git pull --rebase &&
+	test "conflicting modification" = "$(cat file)" &&
+	test file = $(cat file2)
+
+'
+
 test_expect_success 'pull --rebase dies early with dirty working directory' '
 
+	git checkout to-rebase &&
 	git update-ref refs/remotes/me/copy copy^ &&
 	COPY=$(git rev-parse --verify me/copy) &&
 	git rebase --onto $COPY copy &&
-- 
1.6.3.2.206.g417f7

Re: [PATCHv4 2/4] parse-remote: support default reflist in get_remote_merge_branch

[Junio C Hamano]

Santi Béjar <santi@agolina.net> writes:

> Expand get_remote_merge_branch to compute the tracking branch to merge
> when called without arguments (or only the remote name).

I've queued this series (sans 3/4--see other message), but I am wondering
if this has the same "only works with the defeault layout" issue and if so
if it should be documented more clearly.

What plumbing support do you need to get the information in a more precise
way in the scripted Porcelain?  Is exposing branch_get() from remote.c
enough?  That is what is used by fill_tracking_info() in builtin-branch.c
and the call to format_tracking_info() remote.c made by report_tracking()
in builtin-checkout.c --- they are used to produce the "Your branch is
ahead of that branch you are tracking" messages.

Re: [PATCHv4 2/4] parse-remote: support default reflist in get_remote_merge_branch

[Santi Béjar]

2009/6/12 Junio C Hamano <gitster@pobox.com>:
> Santi Béjar <santi@agolina.net> writes:
>
>> Expand get_remote_merge_branch to compute the tracking branch to merge
>> when called without arguments (or only the remote name).
>
> I've queued this series (sans 3/4--see other message), but I am wondering
> if this has the same "only works with the defeault layout" issue and if so
> if it should be documented more clearly.

As it uses:

git for-each-ref --format='%(upstream)' $curr_branch

it works with any layout, AFAICS.

>
> What plumbing support do you need to get the information in a more precise
> way in the scripted Porcelain?  Is exposing branch_get() from remote.c
> enough?  That is what is used by fill_tracking_info() in builtin-branch.c
> and the call to format_tracking_info() remote.c made by report_tracking()
> in builtin-checkout.c --- they are used to produce the "Your branch is
> ahead of that branch you are tracking" messages.

For the explicit case I would need something similar, given the remote
name and the remote branch return the tracking branch, as is currently
done in branch_get() but with the remote.$branch.remote and
remote.$branch.merge as input.

Thanks,
Santi

[PATCHv4 3/4] t5520-pull: --rebase with rebased upstream and two branches

[Santi Béjar]

If you have two branches tracking an upstream that is rebased,
currently you have to do:

git checkout branch1
git pull --rebase remote branch
git checkout branch2
git pull --rebase remote branch

The second rebase works because the first "git pull --rebase" does not
store in the local tracking branch the new value, so the second rebase
detects that it is rebased.

I think one should be able to do the same without the explicit
arguments to "git pull --rebase", but without arguments it stores the
new state of the remote branch so the second "git pull --rebase" does
not work.

Mark this case as test_expect_failure to make people aware of this
behavior.

Signed-off-by: Santi Béjar <santi@agolina.net>
---
Changes since v3:
  - Move the "git rebase --abort" to the failing test to not polute the
    next test.

Hi,

I just wanted to make people aware of this behavior and then decide if
this is the correct behavior. Then we can document it, or try to find a better
solution.

Santi

 t/t5520-pull.sh |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index c5a2e66..a4ac6d2 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -117,6 +117,23 @@ test_expect_success '--rebase with rebased default upstream' '
 
 '
 
+test_expect_failure '--rebase with rebased upstream and two branches' '
+
+	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 pull --rebase &&
+	test "conflicting modification" = "$(cat file)" &&
+	test file = $(cat file2) &&
+	git checkout to-rebase2 &&
+	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.3.2.206.g417f7

Re: [PATCHv4 3/4] t5520-pull: --rebase with rebased upstream and two branches

[Junio C Hamano]

Santi Béjar <santi@agolina.net> writes:

> If you have two branches tracking an upstream that is rebased,
> currently you have to do:
>
> git checkout branch1
> git pull --rebase remote branch
> git checkout branch2
> git pull --rebase remote branch

I think the "ah, I see it rebased" computation is done at a wrong place
for the above to work.  You do not even have to do two pull--rebase; I
think you can simply do "git fetch; git pull --rebase" to break it.

My understanding of the current code we have since c85c7927 is that it was
done as a quick-and-dirty hack that is merely good enough for CVS style
history where everybody is linear.  Grabbing where it used to be last time
we looked at before the fetch, and assuming it is the correct original
fork point, will never work, if you updated the remote tracking ref
outside of the current invocation of "pull --rebase" (if everybody is
linear, you by definition have one single branch, so in that special case,
the hack may happen to work, if you do not fetch).

If you want to fix this, I think you need to use the knowledge of where
the tip of remote/$origin/$branch used to be.  That is kept in the reflog
of that remote tracking ref.  So perhaps

 (0) Lose "oldremoteref=..." before "git fetch" happens in git-pull.sh;

 (1) just before "exec git-rebase", check rtb@{1} is an ancestor of rtb,
     if so be happy;

 (2) otherwise, in the same place, for some (hopefully small) posint n,
     see if rtb@{n} is an ancestor of rtb; use that as the "oldremoteref"
     when invoking "git-rebase".

where rtb == remote tracking branch.

Re: [PATCHv4 3/4] t5520-pull: --rebase with rebased upstream and two branches

[Junio C Hamano]

I think marking breakages here is a good thing to do, but I do not think
this belongs to your rest of the series.  If a fix for this will upcoming
(I think I've outlined ideas in a separate message, IIRC), I think that
could go to 'maint', to which the remainder of your series will definitely
not belong to.

I'd queue this one separately from the other three patches (which will
fork from 'master'), which means this should not depend on [2/4], but
unfortunately it does.

[PATCHv4 4/4] parse-remote: remove unused functions

[Santi Béjar]

Signed-off-by: Santi Béjar <santi@agolina.net>
---
Changes from v1:
  - Only remove the functions documentation, not the whole file
  - Do not remove git-parse-remote from .gitignore

 Documentation/git-parse-remote.txt |   20 ----
 git-parse-remote.sh                |  204 ------------------------------------
 2 files changed, 0 insertions(+), 224 deletions(-)

diff --git a/Documentation/git-parse-remote.txt b/Documentation/git-parse-remote.txt
index cd43069..39d9daa 100644
--- a/Documentation/git-parse-remote.txt
+++ b/Documentation/git-parse-remote.txt
@@ -17,26 +17,6 @@ routines to parse files under $GIT_DIR/remotes/ and
 $GIT_DIR/branches/ and configuration variables that are related
 to fetching, pulling and pushing.
 
-The primary entry points are:
-
-get_remote_refs_for_fetch::
-	Given the list of user-supplied `<repo> <refspec>...`,
-	return the list of refs to fetch after canonicalizing
-	them into `$GIT_DIR` relative paths
-	(e.g. `refs/heads/foo`).  When `<refspec>...` is empty
-	the returned list of refs consists of the defaults
-	for the given `<repo>`, if specified in
-	`$GIT_DIR/remotes/`, `$GIT_DIR/branches/`, or `remote.*.fetch`
-	configuration.
-
-get_remote_refs_for_push::
-	Given the list of user-supplied `<repo> <refspec>...`,
-	return the list of refs to push in a form suitable to be
-	fed to the 'git-send-pack' command.  When `<refspec>...`
-	is empty the returned list of refs consists of the
-	defaults for the given `<repo>`, if specified in
-	`$GIT_DIR/remotes/`.
-
 Author
 ------
 Written by Junio C Hamano.
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 75e1254..5f47b18 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -60,175 +60,6 @@ get_default_remote () {
 	echo ${origin:-origin}
 }
 
-get_remote_default_refs_for_push () {
-	data_source=$(get_data_source "$1")
-	case "$data_source" in
-	'' | branches | self)
-		;; # no default push mapping, just send matching refs.
-	config)
-		git config --get-all "remote.$1.push" ;;
-	remotes)
-		sed -ne '/^Push: */{
-			s///p
-		}' "$GIT_DIR/remotes/$1" ;;
-	*)
-		die "internal error: get-remote-default-ref-for-push $1" ;;
-	esac
-}
-
-# Called from canon_refs_list_for_fetch -d "$remote", which
-# is called from get_remote_default_refs_for_fetch to grok
-# refspecs that are retrieved from the configuration, but not
-# from get_remote_refs_for_fetch when it deals with refspecs
-# supplied on the command line.  $ls_remote_result has the list
-# of refs available at remote.
-#
-# The first token returned is either "explicit" or "glob"; this
-# is to help prevent randomly "globbed" ref from being chosen as
-# a merge candidate
-expand_refs_wildcard () {
-	echo "$ls_remote_result" |
-	git fetch--tool expand-refs-wildcard "-" "$@"
-}
-
-# Subroutine to canonicalize remote:local notation.
-canon_refs_list_for_fetch () {
-	# If called from get_remote_default_refs_for_fetch
-	# leave the branches in branch.${curr_branch}.merge alone,
-	# or the first one otherwise; add prefix . to the rest
-	# to prevent the secondary branches to be merged by default.
-	merge_branches=
-	curr_branch=
-	if test "$1" = "-d"
-	then
-		shift ; remote="$1" ; shift
-		set $(expand_refs_wildcard "$remote" "$@")
-		is_explicit="$1"
-		shift
-		if test "$remote" = "$(get_default_remote)"
-		then
-			curr_branch=$(git symbolic-ref -q HEAD | \
-			    sed -e 's|^refs/heads/||')
-			merge_branches=$(git config \
-			    --get-all "branch.${curr_branch}.merge")
-		fi
-		if test -z "$merge_branches" && test $is_explicit != explicit
-		then
-			merge_branches=..this.will.never.match.any.ref..
-		fi
-	fi
-	for ref
-	do
-		force=
-		case "$ref" in
-		+*)
-			ref=$(expr "z$ref" : 'z+\(.*\)')
-			force=+
-			;;
-		esac
-		expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
-		remote=$(expr "z$ref" : 'z\([^:]*\):')
-		local=$(expr "z$ref" : 'z[^:]*:\(.*\)')
-		dot_prefix=.
-		if test -z "$merge_branches"
-		then
-			merge_branches=$remote
-			dot_prefix=
-		else
-			for merge_branch in $merge_branches
-			do
-			    [ "$remote" = "$merge_branch" ] &&
-			    dot_prefix= && break
-			done
-		fi
-		case "$remote" in
-		'' | HEAD ) remote=HEAD ;;
-		refs/*) ;;
-		heads/* | tags/* | remotes/* ) remote="refs/$remote" ;;
-		*) remote="refs/heads/$remote" ;;
-		esac
-		case "$local" in
-		'') local= ;;
-		refs/*) ;;
-		heads/* | tags/* | remotes/* ) local="refs/$local" ;;
-		*) local="refs/heads/$local" ;;
-		esac
-
-		if local_ref_name=$(expr "z$local" : 'zrefs/\(.*\)')
-		then
-		   git check-ref-format "$local_ref_name" ||
-		   die "* refusing to create funny ref '$local_ref_name' locally"
-		fi
-		echo "${dot_prefix}${force}${remote}:${local}"
-	done
-}
-
-# Returns list of src: (no store), or src:dst (store)
-get_remote_default_refs_for_fetch () {
-	data_source=$(get_data_source "$1")
-	case "$data_source" in
-	'')
-		echo "HEAD:" ;;
-	self)
-	        canon_refs_list_for_fetch -d "$1" \
-			$(git for-each-ref --format='%(refname):')
-		;;
-	config)
-		canon_refs_list_for_fetch -d "$1" \
-			$(git config --get-all "remote.$1.fetch") ;;
-	branches)
-		remote_branch=$(sed -ne '/#/s/.*#//p' "$GIT_DIR/branches/$1")
-		case "$remote_branch" in '') remote_branch=master ;; esac
-		echo "refs/heads/${remote_branch}:refs/heads/$1"
-		;;
-	remotes)
-		canon_refs_list_for_fetch -d "$1" $(sed -ne '/^Pull: */{
-						s///p
-					}' "$GIT_DIR/remotes/$1")
-		;;
-	*)
-		die "internal error: get-remote-default-ref-for-fetch $1" ;;
-	esac
-}
-
-get_remote_refs_for_push () {
-	case "$#" in
-	0) die "internal error: get-remote-refs-for-push." ;;
-	1) get_remote_default_refs_for_push "$@" ;;
-	*) shift; echo "$@" ;;
-	esac
-}
-
-get_remote_refs_for_fetch () {
-	case "$#" in
-	0)
-	    die "internal error: get-remote-refs-for-fetch." ;;
-	1)
-	    get_remote_default_refs_for_fetch "$@" ;;
-	*)
-	    shift
-	    tag_just_seen=
-	    for ref
-	    do
-		if test "$tag_just_seen"
-		then
-		    echo "refs/tags/${ref}:refs/tags/${ref}"
-		    tag_just_seen=
-		    continue
-		else
-		    case "$ref" in
-		    tag)
-			tag_just_seen=yes
-			continue
-			;;
-		    esac
-		fi
-		canon_refs_list_for_fetch "$ref"
-	    done
-	    ;;
-	esac
-}
-
 get_remote_merge_branch () {
 	case "$#" in
 	0|1)
@@ -262,38 +93,3 @@ get_remote_merge_branch () {
 	    [ -n "$remote" ] && echo "refs/remotes/$repo/$remote"
 	esac
 }
-
-resolve_alternates () {
-	# original URL (xxx.git)
-	top_=`expr "z$1" : 'z\([^:]*:/*[^/]*\)/'`
-	while read path
-	do
-		case "$path" in
-		\#* | '')
-			continue ;;
-		/*)
-			echo "$top_$path/" ;;
-		../*)
-			# relative -- ugly but seems to work.
-			echo "$1/objects/$path/" ;;
-		*)
-			# exit code may not be caught by the reader.
-			echo "bad alternate: $path"
-			exit 1 ;;
-		esac
-	done
-}
-
-get_uploadpack () {
-	data_source=$(get_data_source "$1")
-	case "$data_source" in
-	config)
-		uplp=$(git config --get "remote.$1.uploadpack")
-		echo ${uplp:-git-upload-pack}
-		;;
-	*)
-		echo "git-upload-pack"
-		;;
-	esac
-}
-
-- 
1.6.3.2.206.g417f7