[PATCH/RFC 0/3] Split fetch and merge logic

[PATCH/RFC 0/3] Split fetch and merge logic

[Santi Béjar]

This series implements the split of the fetch and merge logic.

The first patch adds tests for the merge logic. It is very extensive and
expensive (in time), so I just post this to show that (almost) nothing
changes (although some part of if could be included). And the third
patch shows what changes.

Santi Béjar (3):
      t/t5515-fetch-merge-logic.sh: Added tests for the merge login in git-fetch
      git-fetch: Split fetch and merge logic
      t/t5515: fixes for the separate fetch and merge logic

[PATCH/RFC 2/3] git-fetch: Split fetch and merge logic

[Santi Béjar]

git-fetch fetches the branches from the remote and saves this
information in .git/FETCH_FETCHED, and at the end it generates
the file .git/FETCH_HEAD.

There are two cases where the behaviour is changed:

1) branch.*.merge no longer must exactly match the remote part
   of the branch fetched. Both are expanded in full (as refs/heads/...)
   and matched afterwards.
2) When the remote is specified with $GIT_DIR/branches/... and there is
   a branch.*.merge, the remote branch name must match to get them merged.
   Before the branch in $GIT_DIR/branches/... was always merged.
   In the documentation the $GIT_DIR/branches/ is documented as a
   short-hand for a corresponding file in $GIT_DIR/remotes/, so I think
   this makes the new behaviour consistent.

Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 Documentation/config.txt |    2 +-
 git-fetch.sh             |   74 ++++++++++++++++++++++++++-------------------
 git-parse-remote.sh      |   60 +++++++++----------------------------
 t/t5510-fetch.sh         |   16 ++++++++++
 4 files changed, 75 insertions(+), 77 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9620126..e695de5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -238,7 +238,7 @@ branch.<name>.remote::
 
 branch.<name>.merge::
 	When in branch <name>, it tells `git fetch` the default refspec to
-	be marked for merging in FETCH_HEAD. The value has exactly to match
+	be marked for merging in FETCH_HEAD. The value has to match
 	a remote part of one of the refspecs which are fetched from the remote
 	given by "branch.<name>.remote".
 	The merge information is used by `git pull` (which at first calls
diff --git a/git-fetch.sh b/git-fetch.sh
index ca984e7..727538d 100755
--- a/git-fetch.sh
+++ b/git-fetch.sh
@@ -77,9 +77,9 @@ do
 	shift
 done
 
+origin=$(get_default_remote)
 case "$#" in
 0)
-	origin=$(get_default_remote)
 	test -n "$(get_remote_url ${origin})" ||
 		die "Where do you want to fetch from today?"
 	set x $origin ; shift ;;
@@ -101,6 +101,7 @@ if test "" = "$append"
 then
 	: >"$GIT_DIR/FETCH_HEAD"
 fi
+: >"$GIT_DIR/FETCH_FETCHED"
 
 # Global that is reused later
 ls_remote_result=$(git ls-remote $exec "$remote") ||
@@ -112,10 +113,6 @@ append_fetch_head () {
     remote_name_="$3"
     remote_nick_="$4"
     local_name_="$5"
-    case "$6" in
-    t) not_for_merge_='not-for-merge' ;;
-    '') not_for_merge_= ;;
-    esac
 
     # remote-nick is the URL given on the command line (or a shorthand)
     # remote-name is the $GIT_DIR relative refs/ path we computed
@@ -146,9 +143,7 @@ append_fetch_head () {
     if git-cat-file commit "$head_" >/dev/null 2>&1
     then
 	headc_=$(git-rev-parse --verify "$head_^0") || exit
-	echo "$headc_	$not_for_merge_	$note_" >>"$GIT_DIR/FETCH_HEAD"
-    else
-	echo "$head_	not-for-merge	$note_" >>"$GIT_DIR/FETCH_HEAD"
+	echo "$headc_	$remote_name_:$local_name_	$note_" >>"$GIT_DIR/FETCH_FETCHED"
     fi
 
     update_local_ref "$local_name_" "$head_" "$note_"
@@ -256,7 +251,7 @@ then
 		  git-show-ref --exclude-existing=refs/tags/ |
 	          while read sha1 name
 		  do
-			echo ".${name}:${name}"
+			echo "${name}:${name}"
 		  done` || exit
 	if test "$#" -gt 1
 	then
@@ -279,13 +274,6 @@ fetch_main () {
 
       # These are relative path from $GIT_DIR, typically starting at refs/
       # but may be HEAD
-      if expr "z$ref" : 'z\.' >/dev/null
-      then
-	  not_for_merge=t
-	  ref=$(expr "z$ref" : 'z\.\(.*\)')
-      else
-	  not_for_merge=
-      fi
       if expr "z$ref" : 'z+' >/dev/null
       then
 	  single_force=t
@@ -366,7 +354,7 @@ fetch_main () {
       esac
 
       append_fetch_head "$head" "$remote" \
-	  "$remote_name" "$remote_nick" "$local_name" "$not_for_merge" || exit
+	  "$remote_name" "$remote_nick" "$local_name" || exit
 
   done
 
@@ -409,28 +397,16 @@ fetch_main () {
 	      case "$ref" in
 	      +$remote_name:*)
 		  single_force=t
-		  not_for_merge=
-		  found="$ref"
-		  break ;;
-	      .+$remote_name:*)
-		  single_force=t
-		  not_for_merge=t
-		  found="$ref"
-		  break ;;
-	      .$remote_name:*)
-		  not_for_merge=t
 		  found="$ref"
 		  break ;;
 	      $remote_name:*)
-		  not_for_merge=
 		  found="$ref"
 		  break ;;
 	      esac
 	  done
 	  local_name=$(expr "z$found" : 'z[^:]*:\(.*\)')
 	  append_fetch_head "$sha1" "$remote" \
-		  "$remote_name" "$remote_nick" "$local_name" \
-		  "$not_for_merge" || exit
+		  "$remote_name" "$remote_nick" "$local_name" || exit
         done
       )
     ) || exit ;;
@@ -454,7 +430,7 @@ case "$no_tags$tags" in
 		do
 			git-cat-file -t "$sha1" >/dev/null 2>&1 || continue
 			echo >&2 "Auto-following $name"
-			echo ".${name}:${name}"
+			echo "${name}:${name}"
 		done)
 	esac
 	case "$taglist" in
@@ -482,3 +458,39 @@ case "$orig_head" in
 	fi
 	;;
 esac
+
+# Generate $GIT_DIR/FETCH_HEAD
+case ",$#,$remote_nick," in
+,1,$origin,)
+	curr_branch=$(git-symbolic-ref -q HEAD | sed -e 's|^refs/heads/||')
+	merge_branches=$(git-repo-config \
+		--get-all "branch.${curr_branch}.merge")
+	[ -z "$merge_branches" ] && merge_first=yes;;
+,1,$remote,) merge_branches=HEAD;;
+,1,*)merge_first=yes ;;
+*)
+	shift
+	merge_branches=$(for ref; do
+		expr "z$ref" : 'z.*:' >/dev/null || ref="${ref}:"
+		echo "$(expr "z$ref" : 'z\([^:]*\):')"; done);;
+esac
+
+test "$merge_first" == "yes" &&
+test "$(get_remote_default_refs_for_fetch -t $remote_nick)" != "explicit" &&
+merge_branches= && merge_first=
+
+merge_branches=$(canon_refs_list_for_fetch $merge_branches | sed 's/:.*$//g')
+
+cat "$GIT_DIR"/FETCH_FETCHED | while IFS='	' read sha1 ref note ; do
+	remote_branch=$(expr "z$ref" : 'z\([^:]*\):')
+	for merge_branch in $merge_branches ; do
+		[ "$merge_branch" == "$remote_branch" ] &&
+			echo "$sha1		$note" && continue 2
+	done
+	if ! test "$merge_first" || test "$merge_first" == "done" ; then
+		echo "$sha1	not-for-merge	$note"
+	else
+		echo "$sha1		$note"
+		merge_first=done
+	fi
+done >> "$GIT_DIR/FETCH_HEAD"
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 5208ee6..212b3bc 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -70,8 +70,7 @@ get_remote_default_refs_for_push () {
 	esac
 }
 
-# Called from canon_refs_list_for_fetch -d "$remote", which
-# is called from get_remote_default_refs_for_fetch to grok
+# 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
@@ -130,30 +129,6 @@ 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=
@@ -166,18 +141,6 @@ canon_refs_list_for_fetch () {
 		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/* | refs/tags/* | refs/remotes/*) ;;
@@ -196,32 +159,39 @@ canon_refs_list_for_fetch () {
 		   git-check-ref-format "$local_ref_name" ||
 		   die "* refusing to create funny ref '$local_ref_name' locally"
 		fi
-		echo "${dot_prefix}${force}${remote}:${local}"
+		echo "${force}${remote}:${local}"
 	done
 }
 
 # Returns list of src: (no store), or src:dst (store)
 get_remote_default_refs_for_fetch () {
+	test "$1" == -t && type=yes && shift
 	data_source=$(get_data_source "$1")
 	case "$data_source" in
 	'')
-		echo "HEAD:" ;;
+		set explicit "HEAD:" ;;
 	config)
-		canon_refs_list_for_fetch -d "$1" \
-			$(git-config --get-all "remote.$1.fetch") ;;
+		set $(expand_refs_wildcard "$1" \
+			$(git-repo-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"
+		set explicit "refs/heads/${remote_branch}:refs/heads/$1"
 		;;
 	remotes)
-		canon_refs_list_for_fetch -d "$1" $(sed -ne '/^Pull: */{
+		set $(expand_refs_wildcard "$1" $(sed -ne '/^Pull: */{
 						s///p
-					}' "$GIT_DIR/remotes/$1")
+					}' "$GIT_DIR/remotes/$1"))
 		;;
 	*)
 		die "internal error: get-remote-default-ref-for-push $1" ;;
 	esac
+	if [ "$type" ] ; then
+		echo $1
+	else
+		shift
+		canon_refs_list_for_fetch "$@"
+	fi
 }
 
 get_remote_refs_for_push () {
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 50c6485..0a19a7d 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -50,6 +50,22 @@ test_expect_success "fetch test" '
 	test "z$mine" = "z$his"
 '
 
+test_expect_success "fetch test fetched" '
+	cd "$D" &&
+	cd three &&
+	git fetch &&
+	test -f .git/refs/heads/two &&
+	test -f .git/refs/heads/one &&
+	master_in_two=`cd ../two && git rev-parse master` &&
+	one_in_two=`cd ../two && git rev-parse one` &&
+	{
+		echo "$master_in_two	refs/heads/master:refs/heads/two"
+		echo "$one_in_two	refs/heads/one:refs/heads/one"
+	} >expected &&
+	cut -f -2 .git/FETCH_FETCHED >actual &&
+	diff expected actual'
+
+
 test_expect_success "fetch test for-merge" '
 	cd "$D" &&
 	cd three &&
-- 
1.5.0.35.gaaba

[PATCH/RFC 3/3] t/t5515: fixes for the separate fetch and merge logic

[Santi Béjar]

1) branch.*.merge no longer must exactly match the remote part
   of the branch fetched. Both are expanded in full (as refs/heads/...)
   and matched afterwards.
2) When the remote is specified with $GIT_DIR/branches/... and there is
   a branch.*.merge, the remote branch name must match to get them merged.
   Before the branch in $GIT_DIR/branches/... was always merged.
   In the documentation the $GIT_DIR/branches/ is documented as a
   short-hand for a corresponding file in $GIT_DIR/remotes/, so I think
   this makes the new behaviour consistent.


Signed-off-by: Santi Béjar <sbejar@gmail.com>
---
 t/t5515/fetch.br-branches-default-merge            |    2 +-
 ...etch.br-branches-default-merge_branches-default |    2 +-
 t/t5515/fetch.br-branches-default-octopus          |    2 +-
 ...ch.br-branches-default-octopus_branches-default |    2 +-
 t/t5515/fetch.br-branches-one-merge                |    2 +-
 t/t5515/fetch.br-branches-one-merge_branches-one   |    2 +-
 t/t5515/fetch.br-config-glob-octopus               |    2 +-
 t/t5515/fetch.br-config-glob-octopus_config-glob   |    2 +-
 t/t5515/fetch.br-remote-glob-octopus               |    2 +-
 t/t5515/fetch.br-remote-glob-octopus_remote-glob   |    2 +-
 10 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/t5515/fetch.br-branches-default-merge b/t/t5515/fetch.br-branches-default-merge
index ca85356..28b85e1 100644
--- a/t/t5515/fetch.br-branches-default-merge
+++ b/t/t5515/fetch.br-branches-default-merge
@@ -1,2 +1,2 @@
 # br-branches-default-merge
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
diff --git a/t/t5515/fetch.br-branches-default-merge_branches-default b/t/t5515/fetch.br-branches-default-merge_branches-default
index cb5acac..1d86ad1 100644
--- a/t/t5515/fetch.br-branches-default-merge_branches-default
+++ b/t/t5515/fetch.br-branches-default-merge_branches-default
@@ -1,2 +1,2 @@
 # br-branches-default-merge branches-default
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
diff --git a/t/t5515/fetch.br-branches-default-octopus b/t/t5515/fetch.br-branches-default-octopus
index 002b82e..22bed59 100644
--- a/t/t5515/fetch.br-branches-default-octopus
+++ b/t/t5515/fetch.br-branches-default-octopus
@@ -1,2 +1,2 @@
 # br-branches-default-octopus
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
diff --git a/t/t5515/fetch.br-branches-default-octopus_branches-default b/t/t5515/fetch.br-branches-default-octopus_branches-default
index 7443cf7..c584f16 100644
--- a/t/t5515/fetch.br-branches-default-octopus_branches-default
+++ b/t/t5515/fetch.br-branches-default-octopus_branches-default
@@ -1,2 +1,2 @@
 # br-branches-default-octopus branches-default
-754b754407bf032e9a2f9d5a9ad05ca79a6b228f	
+754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
diff --git a/t/t5515/fetch.br-branches-one-merge b/t/t5515/fetch.br-branches-one-merge
index e594bec..c7f5013 100644
--- a/t/t5515/fetch.br-branches-one-merge
+++ b/t/t5515/fetch.br-branches-one-merge
@@ -1,2 +1,2 @@
 # br-branches-one-merge
-8e32a6d901327a23ef831511badce7bf3bf46689	
+8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge
diff --git a/t/t5515/fetch.br-branches-one-merge_branches-one b/t/t5515/fetch.br-branches-one-merge_branches-one
index 168031c..1473f5c 100644
--- a/t/t5515/fetch.br-branches-one-merge_branches-one
+++ b/t/t5515/fetch.br-branches-one-merge_branches-one
@@ -1,2 +1,2 @@
 # br-branches-one-merge branches-one
-8e32a6d901327a23ef831511badce7bf3bf46689	
+8e32a6d901327a23ef831511badce7bf3bf46689	not-for-merge
diff --git a/t/t5515/fetch.br-config-glob-octopus b/t/t5515/fetch.br-config-glob-octopus
index da95823..0f324e0 100644
--- a/t/t5515/fetch.br-config-glob-octopus
+++ b/t/t5515/fetch.br-config-glob-octopus
@@ -2,4 +2,4 @@
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
 8e32a6d901327a23ef831511badce7bf3bf46689	
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge
-6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	
diff --git a/t/t5515/fetch.br-config-glob-octopus_config-glob b/t/t5515/fetch.br-config-glob-octopus_config-glob
index 0e7c769..8151e4c 100644
--- a/t/t5515/fetch.br-config-glob-octopus_config-glob
+++ b/t/t5515/fetch.br-config-glob-octopus_config-glob
@@ -2,4 +2,4 @@
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
 8e32a6d901327a23ef831511badce7bf3bf46689	
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge
-6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	
diff --git a/t/t5515/fetch.br-remote-glob-octopus b/t/t5515/fetch.br-remote-glob-octopus
index 92ef25c..4a14384 100644
--- a/t/t5515/fetch.br-remote-glob-octopus
+++ b/t/t5515/fetch.br-remote-glob-octopus
@@ -2,4 +2,4 @@
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
 8e32a6d901327a23ef831511badce7bf3bf46689	
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge
-6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	
diff --git a/t/t5515/fetch.br-remote-glob-octopus_remote-glob b/t/t5515/fetch.br-remote-glob-octopus_remote-glob
index 6ad3726..7f6f928 100644
--- a/t/t5515/fetch.br-remote-glob-octopus_remote-glob
+++ b/t/t5515/fetch.br-remote-glob-octopus_remote-glob
@@ -2,4 +2,4 @@
 754b754407bf032e9a2f9d5a9ad05ca79a6b228f	not-for-merge
 8e32a6d901327a23ef831511badce7bf3bf46689	
 0567da4d5edd2ff4bb292a465ba9e64dcad9536b	not-for-merge
-6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	not-for-merge
+6134ee8f857693b96ff1cc98d3e2fd62b199e5a8	
-- 
1.5.0.35.gaaba

Re: [PATCH/RFC 0/3] Split fetch and merge logic

[Junio C Hamano]

Santi Béjar <sbejar@gmail.com> writes:

> This series implements the split of the fetch and merge logic.

That's a description of what it does, but could you describe
what problem it solves, please?

The approach of presentation to introduce the test and showing
the behavior change in a later patch is very good, though.

Re: [PATCH/RFC 1/3] t/t5515-fetch-merge-logic.sh: Added tests for the merge login in git-fetch

[Santi Béjar]

[-- Attachment #1: Type: text/plain, Size: 3704 bytes --]

Hi *,

   I think it didn't get to the list due to the size limit. So I send it
   as a gziped attatchment, with the test inline.

Regards,

Santi

---
 t/t5515-fetch-merge-logic.sh                       |  124 ++++++++++++++++++++

diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
new file mode 100755
index 0000000..8d8a71f
--- /dev/null
+++ b/t/t5515-fetch-merge-logic.sh
@@ -0,0 +1,124 @@
+#!/bin/sh
+#
+# Copyright (c) 2007 Santi Béjar, based on t4013 by Junio C Hamano
+#
+#
+
+test_description='Merge logic in fetch'
+
+. ./test-lib.sh
+
+LF='
+'
+
+test_expect_success setup '
+	GIT_AUTHOR_DATE="2006-06-26 00:00:00 +0000" &&
+	GIT_COMMITTER_DATE="2006-06-26 00:00:00 +0000" &&
+	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
+
+	echo >file original &&
+	git add file &&
+	git commit -a -m One &&
+	git branch one &&
+
+	echo two >> file &&
+	git commit -a -m Two &&
+	git branch two &&
+
+	echo three >> file &&
+	git commit -a -m Three &&
+	git branch three &&
+
+	echo master >> file &&
+	git commit -a -m Master
+
+	git checkout three
+
+	git clone . cloned &&
+	cd cloned &&
+
+	git config remote.config-explicit.url ../.git/ &&
+	git config remote.config-explicit.fetch refs/heads/master:remotes/rem/master &&
+	git config --add remote.config-explicit.fetch refs/heads/one:remotes/rem/one &&
+	git config --add remote.config-explicit.fetch two:remotes/rem/two &&
+	git config --add remote.config-explicit.fetch refs/heads/three:remotes/rem/three &&
+
+	git config remote.config-glob.url ../.git/ &&
+	git config remote.config-glob.fetch refs/heads/*:refs/remotes/rem/* &&
+
+	mkdir -p .git/remotes &&
+	{
+		echo "URL: ../.git/"
+		echo "Pull: refs/heads/master:remotes/rem/master"
+		echo "Pull: refs/heads/one:remotes/rem/one"
+		echo "Pull: two:remotes/rem/two"
+		echo "Pull: refs/heads/three:remotes/rem/three"
+	} >.git/remotes/remote-explicit &&
+
+	{
+		echo "URL: ../.git/"
+		echo "Pull: refs/heads/*:refs/remotes/rem/*"
+	} >.git/remotes/remote-glob &&
+
+	mkdir -p .git/branches &&
+	echo "../.git" > .git/branches/branches-default &&
+	echo "../.git#one" > .git/branches/branches-one &&
+
+	for remote in config-explicit config-glob remote-explicit remote-glob \
+		branches-default branches-one ; do
+		git config branch.br-$remote.remote $remote &&
+		git config branch.br-$remote-merge.remote $remote &&
+		git config branch.br-$remote-merge.merge refs/heads/three &&
+		git config branch.br-$remote-octopus.remote $remote &&
+		git config branch.br-$remote-octopus.merge refs/heads/one &&
+		git config --add branch.br-$remote-octopus.merge two &&
+		git config --add branch.br-$remote-octopus.merge remotes/rem/three &&
+		branches="$branches br-$remote br-$remote-merge br-$remote-octopus"
+	done
+'
+
+for branch in br-unconfig $branches ; do
+    echo $branch
+    for remote in config-explicit config-glob remote-explicit remote-glob \
+	branches-default branches-one ../.git
+    do
+cat <<EOF
+$branch $remote
+$branch $remote one
+$branch $remote one two
+EOF
+    done
+done > tests
+
+while read cmd
+do
+	case "$cmd" in
+	'' | '#'*) continue ;;
+	esac
+	test=`echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g'`
+	cnt=`expr $test_count + 1`
+	pfx=`printf "%04d" $cnt`
+	expect="../../t5515/fetch.$test"
+	actual="$pfx-fetch.$test"
+
+	test_expect_success "$cmd" '
+		{
+			echo "# $cmd"
+			set x $cmd; shift
+			git symbolic-ref HEAD refs/heads/$1 ; shift
+			git fetch "$@" >/dev/null
+			cut -f -2 .git/FETCH_HEAD
+		} >"$actual" &&
+		if test -f "$expect"
+		then
+			diff -u "$expect" "$actual" &&
+			rm -f "$actual"
+		else
+			# this is to help developing new tests.
+			cp "$actual" "$expect"
+			false
+		fi
+	'
+done < tests
+
+test_done


[-- Attachment #2: 0001-t-t5515-fetch-merge-logic.sh-Added-tests-for-the-merge-login-in-git-fetch.patch.gz --]
[-- Type: application/x-gzip, Size: 13870 bytes --]

Re: [PATCH/RFC 0/3] Split fetch and merge logic

[Santi Béjar]

On 2/16/07, Junio C Hamano <junkio@cox.net> wrote:
> Santi Béjar <sbejar@gmail.com> writes:
>
> > This series implements the split of the fetch and merge logic.
>
> That's a description of what it does, but could you describe
> what problem it solves, please?

Sure.

In the past we've had problems when we have changed the merge logic
(as shows 4363dfbe3). This patch makes the two process completely
independent, and concentrate the merge logic in one place (leaving
git-parse-remote.sh independent of the merge logic).

>
> The approach of presentation to introduce the test and showing
> the behavior change in a later patch is very good, though.

Thanks.

Santi

Re: [PATCH/RFC 0/3] Split fetch and merge logic

[Junio C Hamano]

"Santi Béjar" <sbejar@gmail.com> writes:

> In the past we've had problems when we have changed the merge logic
> (as shows 4363dfbe3). This patch makes the two process completely
> independent, and concentrate the merge logic in one place (leaving
> git-parse-remote.sh independent of the merge logic).

But that is a solved problem, isn't it?  What else does it
solve?  The justification for moving around the logic could be
something like "these three patches do not do that themselves,
but it opens a door for further work such as ...", but without
something concrete in "..." part, your response makes the patch
look mostly needless code churn.

I was hoping to hear something like "now git-fetch has to do
much less than before, eventual C-rewrite of the command, which
can borrow some code already written for 'pu' branch, will
become much easier" ;-).

Re: [PATCH/RFC 0/3] Split fetch and merge logic

[Santi Béjar]

On 2/16/07, Junio C Hamano <junkio@cox.net> wrote:
> "Santi Béjar" <sbejar@gmail.com> writes:
>
> > In the past we've had problems when we have changed the merge logic
> > (as shows 4363dfbe3). This patch makes the two process completely
> > independent, and concentrate the merge logic in one place (leaving
> > git-parse-remote.sh independent of the merge logic).
>
> But that is a solved problem, isn't it?  What else does it
> solve?  The justification for moving around the logic could be
> something like "these three patches do not do that themselves,
> but it opens a door for further work such as ...", but without
> something concrete in "..." part, your response makes the patch
> look mostly needless code churn.
>
> I was hoping to hear something like "now git-fetch has to do
> much less than before, eventual C-rewrite of the command, which
> can borrow some code already written for 'pu' branch, will
> become much easier" ;-).

This sounds like a good justification, also :-)

Santi

Re: [PATCH/RFC 0/3] Split fetch and merge logic

[Junio C Hamano]

"Santi Béjar" <sbejar@gmail.com> writes:

> This sounds like a good justification, also :-)

What I said was an example.  I do not think your code churning
actually would make it easier.

Re: [PATCH/RFC 0/3] Split fetch and merge logic

[Santi Béjar]

On 2/16/07, Junio C Hamano <junkio@cox.net> wrote:
> "Santi Béjar" <sbejar@gmail.com> writes:
>
> > In the past we've had problems when we have changed the merge logic
> > (as shows 4363dfbe3). This patch makes the two process completely
> > independent, and concentrate the merge logic in one place (leaving
> > git-parse-remote.sh independent of the merge logic).
>
> But that is a solved problem, isn't it?  What else does it
> solve?  The justification for moving around the logic could be
> something like "these three patches do not do that themselves,
> but it opens a door for further work such as ...", but without
> something concrete in "..." part, your response makes the patch
> look mostly needless code churn.
>
> I was hoping to hear something like "now git-fetch has to do
> much less than before, eventual C-rewrite of the command, which
> can borrow some code already written for 'pu' branch, will
> become much easier" ;-).

On 2/16/07, Junio C Hamano <junkio@cox.net> wrote:
> "Santi Béjar" <sbejar@gmail.com> writes:
>
> > This sounds like a good justification, also :-)
>
> What I said was an example.  I do not think your code churning
> actually would make it easier.
>
>

What I said was an example of why having the two logic intermixed can
be problematic. Anyway, basically:

* It is a cleanup
* It makes the code cleaner
* It makes the code easier to maintain
* It makes the future changes easier
* It makes git-parse-remote and almost all git-fetch independent of
the merge logic
* The merge logic is in a few lines
* Do one thing and do it well

Regards,

Santi

Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic

[Junio C Hamano]

Santi Béjar <sbejar@gmail.com> writes:

> git-fetch fetches the branches from the remote and saves this
> information in .git/FETCH_FETCHED, and at the end it generates
> the file .git/FETCH_HEAD.
>
> There are two cases where the behaviour is changed:
>
> 1) branch.*.merge no longer must exactly match the remote part
>    of the branch fetched. Both are expanded in full (as refs/heads/...)
>    and matched afterwards.

How hard would it be to fix this?  I see this as a regression.
If you are setting configuration, wouldn't you rather see the
behaviour consistent even when remote adds new refs?

> 2) When the remote is specified with $GIT_DIR/branches/... and there is
>    a branch.*.merge, the remote branch name must match to get them merged.
>    Before the branch in $GIT_DIR/branches/... was always merged.

I do not think the current $GIT_DIR/branches/ support with
respect to choosing which remote branch to choose was done with
any deep thinking, other than to stay backward compatible, so I
would not put too much trust in what is in the documentation.
At the same time, I personally can be pursuaded to go either
way, exactly because I do not think the current behaviour has
strict reasoning behind it.

However, I wonder how this change would affect existing setups
people may have.

Merging this at this moment would be a pain even if there were
no downsides, as there are a few topics that want to touch
parse-remote and fetch (two already in 'pu', and git-bundle
series also wants to hook into git-fetch); these topics would
need to get adjusted if this clean-up goes in first.

Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic

[Santi Béjar]

On 2/19/07, Junio C Hamano <junkio@cox.net> wrote:
> Santi Béjar <sbejar@gmail.com> writes:
>
> > git-fetch fetches the branches from the remote and saves this
> > information in .git/FETCH_FETCHED, and at the end it generates
> > the file .git/FETCH_HEAD.
> >
> > There are two cases where the behaviour is changed:
> >
> > 1) branch.*.merge no longer must exactly match the remote part
> >    of the branch fetched. Both are expanded in full (as refs/heads/...)
> >    and matched afterwards.
>
> How hard would it be to fix this?

At least it is not a 2 lines "fix".

>  I see this as a regression.
> If you are setting configuration, wouldn't you rather see the
> behaviour consistent even when remote adds new refs?

I don't see it a regression. All current setups continue to work
properly and, sorry but I don't see how adding a new ref changes this.

>
> > 2) When the remote is specified with $GIT_DIR/branches/... and there is
> >    a branch.*.merge, the remote branch name must match to get them merged.
> >    Before the branch in $GIT_DIR/branches/... was always merged.
>
> I do not think the current $GIT_DIR/branches/ support with
> respect to choosing which remote branch to choose was done with
> any deep thinking, other than to stay backward compatible, so I
> would not put too much trust in what is in the documentation.
> At the same time, I personally can be pursuaded to go either
> way, exactly because I do not think the current behaviour has
> strict reasoning behind it.

I prefer my way, but I don't mind much either and it can be "fixed".

>
> However, I wonder how this change would affect existing setups
> people may have.
>
> Merging this at this moment would be a pain even if there were
> no downsides, as there are a few topics that want to touch
> parse-remote and fetch (two already in 'pu', and git-bundle
> series also wants to hook into git-fetch); these topics would
> need to get adjusted if this clean-up goes in first.

A problematic decision :)

Santi

Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic

[Junio C Hamano]

"Santi Béjar" <sbejar@gmail.com> writes:

>> > There are two cases where the behaviour is changed:
>> >
>> > 1) branch.*.merge no longer must exactly match the remote part
>> >    of the branch fetched. Both are expanded in full (as refs/heads/...)
>> >    and matched afterwards.
> ...
>>  I see this as a regression.
>> If you are setting configuration, wouldn't you rather see the
>> behaviour consistent even when remote adds new refs?

Maybe I misread your description, but I took it to mean that you
are allowing:

	branch.master.merge = a

to mean what we traditionally spelled

	branch.master.merge = refs/heads/a

and guessed (I haven't looked for where it happens in the code)
the way you do that conversion is by tail-matching the ref; if
the other end creates "refs/heads/b/a", suddenly remote branch
b/a starts matching that pattern wouldn't it?

Earlier we fixed the ambiguous use of branch.*.merge in
756373da; I think the same reasoning should apply here.

Configuration is something you set once because you want to
forget about it afterwards (iow, not having to type every time),
and I think making sure it names things unambiguously outweighs
one-time convenience of being able to write the configuration in
a looser fashion.  

If somebody does "git checkout -B origin/next" which does:

	git checkout -b next origin/next &&
        git repo-config branch.next.merge $merge

I would expect that the enhanced "checkout" script would not
have any trouble arranging $merge to fully spell out
refs/heads/next.

>> Merging this at this moment would be a pain even if there were
>> no downsides, as there are a few topics that want to touch
>> parse-remote and fetch (two already in 'pu', and git-bundle
>> series also wants to hook into git-fetch); these topics would
>> need to get adjusted if this clean-up goes in first.
>
> A problematic decision :)

Not at all.

Re: [PATCH/RFC 2/3] git-fetch: Split fetch and merge logic

[Santi Béjar]

On 2/20/07, Junio C Hamano <junkio@cox.net> wrote:
> "Santi Béjar" <sbejar@gmail.com> writes:
>
> >> > There are two cases where the behaviour is changed:
> >> >
> >> > 1) branch.*.merge no longer must exactly match the remote part
> >> >    of the branch fetched. Both are expanded in full (as refs/heads/...)
> >> >    and matched afterwards.
> > ...
> >>  I see this as a regression.
> >> If you are setting configuration, wouldn't you rather see the
> >> behaviour consistent even when remote adds new refs?
>
> Maybe I misread your description, but I took it to mean that you
> are allowing:
>
>         branch.master.merge = a
>
> to mean what we traditionally spelled
>
>         branch.master.merge = refs/heads/a
>
> and guessed (I haven't looked for where it happens in the code)
> the way you do that conversion is by tail-matching the ref; if
> the other end creates "refs/heads/b/a", suddenly remote branch
> b/a starts matching that pattern wouldn't it?

No. branch.master.merge = a is equivalent to refs/heads/a and only
matches with the remote branch refs/heads/a. It continues to exactly
match the two branches, but with the full patch (refs/...). So now it
is possible to have:

[remote "origin"]
url = ...
fetch = refs/heads/*:refs/heads/origin/*

[branch "master"]
remote = origin
merge = master

or the other way:

[remote "origin"]
url = ...
fetch = master:refs/heads/origin

[branch "master"]
remote = origin
merge = refs/heads/master

>
> Earlier we fixed the ambiguous use of branch.*.merge in
> 756373da; I think the same reasoning should apply here.
>
> Configuration is something you set once because you want to
> forget about it afterwards (iow, not having to type every time),
> and I think making sure it names things unambiguously outweighs
> one-time convenience of being able to write the configuration in
> a looser fashion.

It is unambiguous.

But if it is problematic I'll try to keep the current behaviour.

Santi