threads / discuss / 19778

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

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

## tl;dr

9 messages between Jun 11, 2009 and Jun 12, 2009.

replies: 8people: 2as markdown or json

Santi Béjar· Jun 11, 2009, 22:39 UTC · lore
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(-)
Santi Béjar· Jun 11, 2009, 22:39 UTC · re: Santi Béjar · lore

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

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
Santi Béjar· Jun 11, 2009, 22:39 UTC · re: Santi Béjar · lore

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

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
Junio C Hamano· Jun 12, 2009, 03:09 UTC · re: Santi Béjar · lore

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

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.

Santi Béjar· Jun 12, 2009, 06:58 UTC · re: Junio C Hamano · lore

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

2009/6/12 Junio C Hamano <gitster@pobox.com>:
Show 8 quoted lines
> 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.
Show 7 quoted lines
>
> 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

Santi Béjar· Jun 11, 2009, 22:39 UTC · re: Santi Béjar · lore

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

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
Junio C Hamano· Jun 11, 2009, 23:19 UTC · re: Santi Béjar · lore

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

Santi Béjar <santi@agolina.net> writes:
Show 7 quoted lines
> 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.
Junio C Hamano· Jun 12, 2009, 02:57 UTC · re: Santi Béjar · lore

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

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.

Santi Béjar· Jun 11, 2009, 22:39 UTC · re: Santi Béjar · lore

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

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

← back to recent threads