[PATCH 00/16] Cleanup {branches,remotes}-file cruft

[PATCH 00/16] Cleanup {branches,remotes}-file cruft

[Ramkumar Ramachandra]

Hi,

This is a cleanup operation to get rid of the historical
$GIT_DIR/{branches,remotes} cruft.  Mostly no-brainers that don't
deserve a second look.  The ordering of the series might be somewhat
weird, but that's because it's the order in which I wrote those
patches: rebasing results in stupid conflicts that's not worth
anyone's time.

Thanks.

Ramkumar Ramachandra (16):
  t/t5505-remote: modernize subshell-style of one test
  t/t5505-remote: test push-refspec in branches-file
  t/t5505-remote: use test_path_is_missing
  t/t5505-remote: remove dependency on $origin_url
  remote: remove dead code in read_branches_file()
  t/t5505-remote: test url-with-# in branches-file
  t/t5516-fetch-push: don't use branches-file
  t/t5516-fetch-push: use test_config()
  ls-remote doc: fix example invocation on git.git
  ls-remote doc: rewrite <repository> paragraph
  ls-remote doc: don't encourage use of branches-file
  t/t5505-remote: modernize subshell-style of one test
  t/t5505-remote: test multiple push/pull in remotes-file
  t/t5510-fetch: don't use remotes-file
  t/t5515-fetch-merge-logic: don't use {branches,remotes}-file
  remote: deprecate read_{branches,remotes}_file

 Documentation/git-ls-remote.txt | 13 ++++---
 remote.c                        | 34 ++++++-------------
 t/t5505-remote.sh               | 75 +++++++++++++++++++++++++++++------------
 t/t5510-fetch.sh                |  9 ++---
 t/t5515-fetch-merge-logic.sh    | 28 +++++++--------
 t/t5516-fetch-push.sh           | 62 +++++++++++++++++++---------------
 6 files changed, 121 insertions(+), 100 deletions(-)

-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 01/16] t/t5505-remote: modernize subshell-style of one test

[Ramkumar Ramachandra]

Since we plan to edit the test "migrate a remote from named file in
$GIT_DIR/remotes" in later patches, modernize the subshell-style by
putting the parenthesis on separate lines and indenting the body.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dd10ff0..4d5810f 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -691,13 +691,15 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
 	git clone one six &&
 	origin_url=$(pwd)/one &&
-	(cd six &&
-	 git remote rm origin &&
-	 echo "$origin_url" > .git/branches/origin &&
-	 git remote rename origin origin &&
-	 ! test -f .git/branches/origin &&
-	 test "$(git config remote.origin.url)" = "$origin_url" &&
-	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+	(
+		cd six &&
+		git remote rm origin &&
+		echo "$origin_url" > .git/branches/origin &&
+		git remote rename origin origin &&
+		! test -f .git/branches/origin &&
+		test "$(git config remote.origin.url)" = "$origin_url" &&
+		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
+	)
 '
 
 test_expect_success 'remote prune to cause a dangling symref' '
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 02/16] t/t5505-remote: test push-refspec in branches-file

[Ramkumar Ramachandra]

The test "migrate a remote from named file in $GIT_DIR/branches" reads
the branches-file, but only checks that the url and fetch-refspec are
set correctly.  Check that the push-refspec is also set correctly.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 4d5810f..38c62ec 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -698,7 +698,8 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
 		git remote rename origin origin &&
 		! test -f .git/branches/origin &&
 		test "$(git config remote.origin.url)" = "$origin_url" &&
-		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
+		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin" &&
+		test "$(git config remote.origin.push)" = "HEAD:refs/heads/master"
 	)
 '
 
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 03/16] t/t5505-remote: use test_path_is_missing

[Ramkumar Ramachandra]

Replace instances of ! test -f with test_path_is_missing.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 38c62ec..dcb6c2f 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -682,7 +682,7 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	 mkdir -p .git/remotes &&
 	 cat ../remotes_origin > .git/remotes/origin &&
 	 git remote rename origin origin &&
-	 ! test -f .git/remotes/origin &&
+	 test_path_is_missing .git/remotes/origin &&
 	 test "$(git config remote.origin.url)" = "$origin_url" &&
 	 test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
 	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
@@ -696,7 +696,7 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
 		git remote rm origin &&
 		echo "$origin_url" > .git/branches/origin &&
 		git remote rename origin origin &&
-		! test -f .git/branches/origin &&
+		test_path_is_missing .git/branches/origin &&
 		test "$(git config remote.origin.url)" = "$origin_url" &&
 		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin" &&
 		test "$(git config remote.origin.push)" = "HEAD:refs/heads/master"
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 04/16] t/t5505-remote: remove dependency on $origin_url

[Ramkumar Ramachandra]

In the tests "migrate a remote from named file in
$GIT_DIR/{remotes,branches}", we are only checking that a configuration
is migrated successfully; it has no correspondence with whether or not
those values do something sensible with other git
operations (fetch/push).  Therefore, there is no need to determine
$origin_url: just substitute it with the constant value "quux".

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index dcb6c2f..fd0a81e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -669,21 +669,20 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 '
 
 cat > remotes_origin << EOF
-URL: $(pwd)/one
+URL: quux
 Push: refs/heads/master:refs/heads/upstream
 Pull: refs/heads/master:refs/heads/origin
 EOF
 
 test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	git clone one five &&
-	origin_url=$(pwd)/one &&
 	(cd five &&
 	 git remote remove origin &&
 	 mkdir -p .git/remotes &&
 	 cat ../remotes_origin > .git/remotes/origin &&
 	 git remote rename origin origin &&
 	 test_path_is_missing .git/remotes/origin &&
-	 test "$(git config remote.origin.url)" = "$origin_url" &&
+	 test "$(git config remote.origin.url)" = "quux" &&
 	 test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
 	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
 '
@@ -694,10 +693,10 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
 	(
 		cd six &&
 		git remote rm origin &&
-		echo "$origin_url" > .git/branches/origin &&
+		echo quux > .git/branches/origin &&
 		git remote rename origin origin &&
 		test_path_is_missing .git/branches/origin &&
-		test "$(git config remote.origin.url)" = "$origin_url" &&
+		test "$(git config remote.origin.url)" = "quux" &&
 		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin" &&
 		test "$(git config remote.origin.push)" = "HEAD:refs/heads/master"
 	)
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 05/16] remote: remove dead code in read_branches_file()

[Ramkumar Ramachandra]

The first line of the function checks that the remote-name contains a
slash ('/'), and sets the "slash" variable accordingly.  The only caller
of read_branches_file() is remote_get_1(); the calling codepath is
guarded by valid_remote_nick(), which checks that the remote does not
contain a slash.  Therefore, the "slash" variable can never be set:
remove the dead code that assumes otherwise.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/remote.c b/remote.c
index e71f66d..128b210 100644
--- a/remote.c
+++ b/remote.c
@@ -276,10 +276,9 @@ static void read_remotes_file(struct remote *remote)
 
 static void read_branches_file(struct remote *remote)
 {
-	const char *slash = strchr(remote->name, '/');
 	char *frag;
 	struct strbuf branch = STRBUF_INIT;
-	int n = slash ? slash - remote->name : 1000;
+	int n = 1000;
 	FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
 	char *s, *p;
 	int len;
@@ -299,36 +298,17 @@ static void read_branches_file(struct remote *remote)
 	while (isspace(p[-1]))
 		*--p = 0;
 	len = p - s;
-	if (slash)
-		len += strlen(slash);
 	p = xmalloc(len + 1);
 	strcpy(p, s);
-	if (slash)
-		strcat(p, slash);
 
-	/*
-	 * With "slash", e.g. "git fetch jgarzik/netdev-2.6" when
-	 * reading from $GIT_DIR/branches/jgarzik fetches "HEAD" from
-	 * the partial URL obtained from the branches file plus
-	 * "/netdev-2.6" and does not store it in any tracking ref.
-	 * #branch specifier in the file is ignored.
-	 *
-	 * Otherwise, the branches file would have URL and optionally
-	 * #branch specified.  The "master" (or specified) branch is
-	 * fetched and stored in the local branch of the same name.
-	 */
 	frag = strchr(p, '#');
 	if (frag) {
 		*(frag++) = '\0';
 		strbuf_addf(&branch, "refs/heads/%s", frag);
 	} else
 		strbuf_addstr(&branch, "refs/heads/master");
-	if (!slash) {
-		strbuf_addf(&branch, ":refs/heads/%s", remote->name);
-	} else {
-		strbuf_reset(&branch);
-		strbuf_addstr(&branch, "HEAD:");
-	}
+
+	strbuf_addf(&branch, ":refs/heads/%s", remote->name);
 	add_url_alias(remote, p);
 	add_fetch_refspec(remote, strbuf_detach(&branch, NULL));
 	/*
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 06/16] t/t5505-remote: test url-with-# in branches-file

[Ramkumar Ramachandra]

Add one more test similar to "migrate a remote from named file in
$GIT_DIR/branches" to check that a url with a # can be used to specify
the branch name (as opposed to the constant "master").

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index fd0a81e..93e11c8 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -702,27 +702,42 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
 	)
 '
 
-test_expect_success 'remote prune to cause a dangling symref' '
+test_expect_success 'migrate a remote from named file in $GIT_DIR/branches (2)' '
 	git clone one seven &&
+	origin_url=$(pwd)/one &&
+	(
+		cd seven &&
+		git remote rm origin &&
+		echo "quux#foom" > .git/branches/origin &&
+		git remote rename origin origin &&
+		test_path_is_missing .git/branches/origin &&
+		test "$(git config remote.origin.url)" = "quux" &&
+		test "$(git config remote.origin.fetch)" = "refs/heads/foom:refs/heads/origin"
+		test "$(git config remote.origin.push)" = "HEAD:refs/heads/foom"
+	)
+'
+
+test_expect_success 'remote prune to cause a dangling symref' '
+	git clone one eight &&
 	(
 		cd one &&
 		git checkout side2 &&
 		git branch -D master
 	) &&
 	(
-		cd seven &&
+		cd eight &&
 		git remote prune origin
 	) >err 2>&1 &&
 	test_i18ngrep "has become dangling" err &&
 
 	: And the dangling symref will not cause other annoying errors &&
 	(
-		cd seven &&
+		cd eight &&
 		git branch -a
 	) 2>err &&
 	! grep "points nowhere" err &&
 	(
-		cd seven &&
+		cd eight &&
 		test_must_fail git branch nomore origin
 	) 2>err &&
 	grep "dangling symref" err
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 07/16] t/t5516-fetch-push: don't use branches-file

[Ramkumar Ramachandra]

Four tests exercising fetch and push functionality unnecessarily depend
on $GIT_DIR/branches files.  Modern Git does not encourage the use of
those files, and the parser remote.c:read_branches_file() is only
provided for backward compatibility with older repositories.  We already
have tests in t/t5505-remote to verify that the parser works: so,
substitute the $GIT_DIR/branches configuration with an equivalent
gitconfig-style configuration, using the results of those tests.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 4691d51..6e9fa84 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -852,9 +852,11 @@ test_expect_success 'fetch with branches' '
 	mk_empty testrepo &&
 	git branch second $the_first_commit &&
 	git checkout second &&
-	echo ".." > testrepo/.git/branches/branch1 &&
 	(
 		cd testrepo &&
+		test_config remote.branch1.url ".."  &&
+		test_config remote.branch1.fetch "refs/heads/master:refs/heads/branch1"  &&
+		test_config remote.branch1.push "HEAD:refs/heads/master"  &&
 		git fetch branch1 &&
 		echo "$the_commit commit	refs/heads/branch1" >expect &&
 		git for-each-ref refs/heads >actual &&
@@ -865,9 +867,11 @@ test_expect_success 'fetch with branches' '
 
 test_expect_success 'fetch with branches containing #' '
 	mk_empty testrepo &&
-	echo "..#second" > testrepo/.git/branches/branch2 &&
 	(
 		cd testrepo &&
+		test_config remote.branch2.url ".."  &&
+		test_config remote.branch2.fetch "refs/heads/second:refs/heads/branch2"  &&
+		test_config remote.branch2.push "HEAD:refs/heads/second"  &&
 		git fetch branch2 &&
 		echo "$the_first_commit commit	refs/heads/branch2" >expect &&
 		git for-each-ref refs/heads >actual &&
@@ -879,7 +883,9 @@ test_expect_success 'fetch with branches containing #' '
 test_expect_success 'push with branches' '
 	mk_empty testrepo &&
 	git checkout second &&
-	echo "testrepo" > .git/branches/branch1 &&
+	test_config remote.branch1.url testrepo &&
+	test_config remote.branch1.fetch "refs/heads/master:refs/heads/branch1" &&
+	test_config remote.branch1.push "HEAD:refs/heads/master" &&
 	git push branch1 &&
 	(
 		cd testrepo &&
@@ -891,7 +897,9 @@ test_expect_success 'push with branches' '
 
 test_expect_success 'push with branches containing #' '
 	mk_empty testrepo &&
-	echo "testrepo#branch3" > .git/branches/branch2 &&
+	test_config remote.branch2.url testrepo &&
+	test_config remote.branch2.fetch "refs/heads/branch3:refs/heads/branch2" &&
+	test_config remote.branch2.push "HEAD:refs/heads/branch3" &&
 	git push branch2 &&
 	(
 		cd testrepo &&
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 08/16] t/t5516-fetch-push: use test_config()

[Ramkumar Ramachandra]

Replace the 'git config' calls in tests with test_config for greater
robustness.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5516-fetch-push.sh | 46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6e9fa84..afb25c4 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -142,8 +142,8 @@ test_expect_success 'fetch with wildcard' '
 	mk_empty testrepo &&
 	(
 		cd testrepo &&
-		git config remote.up.url .. &&
-		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
+		test_config remote.up.url .. &&
+		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
 		git fetch up &&
 
 		echo "$the_commit commit	refs/remotes/origin/master" >expect &&
@@ -157,9 +157,9 @@ test_expect_success 'fetch with insteadOf' '
 	(
 		TRASH=$(pwd)/ &&
 		cd testrepo &&
-		git config "url.$TRASH.insteadOf" trash/ &&
-		git config remote.up.url trash/. &&
-		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
+		test_config "url.$TRASH.insteadOf" trash/ &&
+		test_config remote.up.url trash/. &&
+		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
 		git fetch up &&
 
 		echo "$the_commit commit	refs/remotes/origin/master" >expect &&
@@ -173,9 +173,9 @@ test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '
 	(
 		TRASH=$(pwd)/ &&
 		cd testrepo &&
-		git config "url.trash/.pushInsteadOf" "$TRASH" &&
-		git config remote.up.url "$TRASH." &&
-		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
+		test_config "url.trash/.pushInsteadOf" "$TRASH" &&
+		test_config remote.up.url "$TRASH." &&
+		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
 		git fetch up &&
 
 		echo "$the_commit commit	refs/remotes/origin/master" >expect &&
@@ -780,7 +780,7 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
 
 test_expect_success 'allow deleting a ref using --delete' '
 	mk_test testrepo heads/master &&
-	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
+	(cd testrepo && test_config receive.denyDeleteCurrent warn) &&
 	git push testrepo --delete master &&
 	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
 '
@@ -809,7 +809,7 @@ test_expect_success 'warn on push to HEAD of non-bare repository' '
 	(
 		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch warn
+		test_config receive.denyCurrentBranch warn
 	) &&
 	git push testrepo master 2>stderr &&
 	grep "warning: updating the current branch" stderr
@@ -820,7 +820,7 @@ test_expect_success 'deny push to HEAD of non-bare repository' '
 	(
 		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch true
+		test_config receive.denyCurrentBranch true
 	) &&
 	test_must_fail git push testrepo master
 '
@@ -830,8 +830,8 @@ test_expect_success 'allow push to HEAD of bare repository (bare)' '
 	(
 		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch true &&
-		git config core.bare true
+		test_config receive.denyCurrentBranch true &&
+		test_config core.bare true
 	) &&
 	git push testrepo master 2>stderr &&
 	! grep "warning: updating the current branch" stderr
@@ -842,7 +842,7 @@ test_expect_success 'allow push to HEAD of non-bare repository (config)' '
 	(
 		cd testrepo &&
 		git checkout master &&
-		git config receive.denyCurrentBranch false
+		test_config receive.denyCurrentBranch false
 	) &&
 	git push testrepo master 2>stderr &&
 	! grep "warning: updating the current branch" stderr
@@ -918,7 +918,7 @@ test_expect_success 'push into aliased refs (consistent)' '
 		cd child1 &&
 		git branch foo &&
 		git symbolic-ref refs/heads/bar refs/heads/foo
-		git config receive.denyCurrentBranch false
+		test_config receive.denyCurrentBranch false
 	) &&
 	(
 		cd child2 &&
@@ -940,7 +940,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 		cd child1 &&
 		git branch foo &&
 		git symbolic-ref refs/heads/bar refs/heads/foo
-		git config receive.denyCurrentBranch false
+		test_config receive.denyCurrentBranch false
 	) &&
 	(
 		cd child2 &&
@@ -1006,7 +1006,7 @@ test_expect_success 'push --porcelain rejected' '
 	git push testrepo refs/heads/master:refs/remotes/origin/master &&
 	(cd testrepo &&
 		git reset --hard origin/master^
-		git config receive.denyCurrentBranch true) &&
+		test_config receive.denyCurrentBranch true) &&
 
 	echo >.git/foo  "To testrepo"  &&
 	echo >>.git/foo "!	refs/heads/master:refs/heads/master	[remote rejected] (branch is currently checked out)" &&
@@ -1020,7 +1020,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '
 	git push testrepo refs/heads/master:refs/remotes/origin/master &&
 	(cd testrepo &&
 		git reset --hard origin/master
-		git config receive.denyCurrentBranch true) &&
+		test_config receive.denyCurrentBranch true) &&
 
 	echo >.git/foo  "To testrepo"  &&
 	echo >>.git/foo "!	refs/heads/master^:refs/heads/master	[rejected] (non-fast-forward)" &&
@@ -1052,7 +1052,7 @@ do
 		mk_test testrepo heads/master hidden/one hidden/two hidden/three &&
 		(
 			cd testrepo &&
-			git config $configsection.hiderefs refs/hidden
+			test_config $configsection.hiderefs refs/hidden
 		) &&
 
 		# push to unhidden ref succeeds normally
@@ -1078,7 +1078,7 @@ test_expect_success 'fetch exact SHA1' '
 	git push testrepo master:refs/hidden/one &&
 	(
 		cd testrepo &&
-		git config transfer.hiderefs refs/hidden
+		test_config transfer.hiderefs refs/hidden
 	) &&
 	check_push_result testrepo $the_commit hidden/one &&
 
@@ -1098,7 +1098,7 @@ test_expect_success 'fetch exact SHA1' '
 		# the server side can allow it to succeed
 		(
 			cd ../testrepo &&
-			git config uploadpack.allowtipsha1inwant true
+			test_config uploadpack.allowtipsha1inwant true
 		) &&
 
 		git fetch -v ../testrepo $the_commit:refs/heads/copy &&
@@ -1126,8 +1126,8 @@ test_expect_success 'fetch follows tags by default' '
 	(
 		cd dst &&
 		git remote add origin ../src &&
-		git config branch.master.remote origin &&
-		git config branch.master.merge refs/heads/master &&
+		test_config branch.master.remote origin &&
+		test_config branch.master.merge refs/heads/master &&
 		git pull &&
 		git for-each-ref >../actual
 	) &&
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 09/16] ls-remote doc: fix example invocation on git.git

[Ramkumar Ramachandra]

Under the EXAMPLES section, there is one invocation on the git.git
repository that attempts to list the refs master, pu, and rc.  The ref
rc does not exist in today's repository, so remove it.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-ls-remote.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 774de5e..a24b8b6 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -67,10 +67,9 @@ EXAMPLES
 	7ceca275d047c90c0c7d5afb13ab97efdf51bd6e	refs/tags/v0.99.3
 	c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
 	0918385dbd9656cab0d1d81ba7453d49bbc16250	refs/tags/junio-gpg-pub
-	$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master pu rc
+	$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master pu
 	5fe978a5381f1fbad26a80e682ddd2a401966740	refs/heads/master
 	c781a84b5204fb294c9ccc79f8b3baceeb32c061	refs/heads/pu
-	b1d096f2926c4e37c9c0b6a7bf2119bedaa277cb	refs/heads/rc
 	$ echo http://www.kernel.org/pub/scm/git/git.git >.git/branches/public
 	$ git ls-remote --tags public v\*
 	d6602ec5194c87b0fc87103ca4d67251c76f233a	refs/tags/v0.99
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 10/16] ls-remote doc: rewrite <repository> paragraph

[Ramkumar Ramachandra]

Replace the <repository> paragraph containing specific references to
$GIT_DIR/branches and "." with a generic urls-or-remotes paragraph
referencing the relevant sections in the git-fetch(1) manpage.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-ls-remote.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index a24b8b6..0715892 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -48,9 +48,9 @@ OPTIONS
 	exit without talking to the remote.
 
 <repository>::
-	Location of the repository.  The shorthand defined in
-	$GIT_DIR/branches/ can be used. Use "." (dot) to list references in
-	the local repository.
+	The "remote" repository to query.  This parameter can be
+	either a URL or the name of a remote (see the GIT URLS and
+	REMOTES sections of linkgit:git-fetch[1]).
 
 <refs>...::
 	When unspecified, all references, after filtering done
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 11/16] ls-remote doc: don't encourage use of branches-file

[Ramkumar Ramachandra]

One outdated example encourages the use of $GIT_DIR/branches files.
Replace it with an equivalent example using a remote.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-ls-remote.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 0715892..340c3ff 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -70,8 +70,8 @@ EXAMPLES
 	$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master pu
 	5fe978a5381f1fbad26a80e682ddd2a401966740	refs/heads/master
 	c781a84b5204fb294c9ccc79f8b3baceeb32c061	refs/heads/pu
-	$ echo http://www.kernel.org/pub/scm/git/git.git >.git/branches/public
-	$ git ls-remote --tags public v\*
+	$ git remote add korg http://www.kernel.org/pub/scm/git/git.git
+	$ git ls-remote --tags korg v\*
 	d6602ec5194c87b0fc87103ca4d67251c76f233a	refs/tags/v0.99
 	f25a265a342aed6041ab0cc484224d9ca54b6f41	refs/tags/v0.99.1
 	c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 12/16] t/t5505-remote: modernize subshell-style of one test

[Ramkumar Ramachandra]

Since we plan to edit the test "migrate a remote from named file in
$GIT_DIR/remotes" in later patches, modernize the subshell-style by
putting the parenthesis on separate lines and indenting the body.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 93e11c8..06ed381 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -676,15 +676,17 @@ EOF
 
 test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 	git clone one five &&
-	(cd five &&
-	 git remote remove origin &&
-	 mkdir -p .git/remotes &&
-	 cat ../remotes_origin > .git/remotes/origin &&
-	 git remote rename origin origin &&
-	 test_path_is_missing .git/remotes/origin &&
-	 test "$(git config remote.origin.url)" = "quux" &&
-	 test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
-	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
+	(
+		cd five &&
+		git remote remove origin &&
+		mkdir -p .git/remotes &&
+		cat ../remotes_origin > .git/remotes/origin &&
+		git remote rename origin origin &&
+		test_path_is_missing .git/remotes/origin &&
+		test "$(git config remote.origin.url)" = "quux" &&
+		test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
+		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
+	)
 '
 
 test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 13/16] t/t5505-remote: test multiple push/pull in remotes-file

[Ramkumar Ramachandra]

Extend the test "migrate a remote from named file in $GIT_DIR/remotes"
to test that multiple "Push:" and "Pull:" lines in the remotes-file
works as expected.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5505-remote.sh | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 06ed381..5497a23 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -671,7 +671,9 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 cat > remotes_origin << EOF
 URL: quux
 Push: refs/heads/master:refs/heads/upstream
+Push: refs/heads/master:refs/heads/upstream2
 Pull: refs/heads/master:refs/heads/origin
+Pull: refs/heads/master:refs/heads/origin2
 EOF
 
 test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
@@ -684,8 +686,18 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
 		git remote rename origin origin &&
 		test_path_is_missing .git/remotes/origin &&
 		test "$(git config remote.origin.url)" = "quux" &&
-		test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
-		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
+		cat >push_expected <<-\EOF
+		refs/heads/master:refs/heads/upstream
+		refs/heads/master:refs/heads/upstream2
+		EOF
+		cat >fetch_expected <<-\EOF
+		refs/heads/master:refs/heads/origin
+		refs/heads/master:refs/heads/origin2
+		EOF
+		git config --get-all remote.origin.fetch >push_actual &&
+		git config --get-all remote.origin.fetch >fetch_actual &&
+		test_cmp push_expected push_actual &&
+		test_cmp fetch_expected fetch_actual &&
 	)
 '
 
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 14/16] t/t5510-fetch: don't use remotes-file

[Ramkumar Ramachandra]

Replace it with the equivalent gitconfig configuration, using the
results of a test in t/t5505-remote.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5510-fetch.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index fde6891..47aeac2 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -46,12 +46,9 @@ test_expect_success "clone and setup child repos" '
 		cd three &&
 		git config branch.master.remote two &&
 		git config branch.master.merge refs/heads/one &&
-		mkdir -p .git/remotes &&
-		{
-			echo "URL: ../two/.git/"
-			echo "Pull: refs/heads/master:refs/heads/two"
-			echo "Pull: refs/heads/one:refs/heads/one"
-		} >.git/remotes/two
+		git config remote.two.url "../two/.git/" &&
+		git config remote.two.fetch "refs/heads/master:refs/heads/two" &&
+		git config --add remote.two.fetch "refs/heads/one:refs/heads/one"
 	) &&
 	git clone . bundle &&
 	git clone . seven
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 15/16] t/t5515-fetch-merge-logic: don't use {branches,remotes}-file

[Ramkumar Ramachandra]

Replace it with the equivalent gitconfig configuration, using the
results of the corresponding tests in t/t5505-remote.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5515-fetch-merge-logic.sh | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
index dbb927d..cde44e0 100755
--- a/t/t5515-fetch-merge-logic.sh
+++ b/t/t5515-fetch-merge-logic.sh
@@ -55,27 +55,25 @@ test_expect_success setup '
 	git config remote.config-glob.fetch refs/heads/*:refs/remotes/rem/* &&
 	remotes="$remotes config-glob" &&
 
-	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 &&
+	git config remote.remote-explicit.url ../.git/ &&
+	git config remote.remote-explicit.fetch refs/heads/master:remotes/rem/master &&
+	git config --add remote.remote-explicit.fetch refs/heads/one:remotes/rem/one &&
+	git config --add remote.remote-explicit.fetch two:remotes/rem/two &&
+	git config --add remote.remote-explicit.fetch refs/heads/three:remotes/rem/three &&
 	remotes="$remotes remote-explicit" &&
 
-	{
-		echo "URL: ../.git/"
-		echo "Pull: refs/heads/*:refs/remotes/rem/*"
-	} >.git/remotes/remote-glob &&
+	git config remote.remote-glob.url ../.git/ &&
+	git config remote.remote-glob.fetch refs/heads/*:refs/remotes/rem/* &&
 	remotes="$remotes remote-glob" &&
 
-	mkdir -p .git/branches &&
-	echo "../.git" > .git/branches/branches-default &&
+	git config remote.branches-default.url ../.git/ &&
+	git config remote.branches-default.fetch refs/heads/master:refs/heads/branches-default &&
+	git config remote.branches-default.push HEAD:refs/heads/master &&
 	remotes="$remotes branches-default" &&
 
-	echo "../.git#one" > .git/branches/branches-one &&
+	git config remote.branches-one.url ../.git/ &&
+	git config remote.branches-one.fetch refs/heads/one:refs/heads/branches-one &&
+	git config remote.branches-one.push HEAD:refs/heads/one &&
 	remotes="$remotes branches-one" &&
 
 	for remote in $remotes ; do
-- 
1.8.3.1.499.g7ad3486.dirty

[PATCH 16/16] remote: deprecate read_{branches,remotes}_file

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 remote.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/remote.c b/remote.c
index 128b210..b487865 100644
--- a/remote.c
+++ b/remote.c
@@ -227,6 +227,10 @@ static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
 	rewrite->instead_of_nr++;
 }
 
+/*
+ * Deprecated; provided for backward compatibility with ancient
+ * repositories.  Modern Git does not encourage their use.
+ */
 static void read_remotes_file(struct remote *remote)
 {
 	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
@@ -274,6 +278,10 @@ static void read_remotes_file(struct remote *remote)
 	fclose(f);
 }
 
+/*
+ * Deprecated; provided for backward compatibility with ancient
+ * repositories.  Modern Git does not encourage their use.
+ */
 static void read_branches_file(struct remote *remote)
 {
 	char *frag;
-- 
1.8.3.1.499.g7ad3486.dirty

Re: [PATCH 00/16] Cleanup {branches,remotes}-file cruft

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> This is a cleanup operation to get rid of the historical
> $GIT_DIR/{branches,remotes} cruft.  Mostly no-brainers that don't
> deserve a second look.

Only reacting to "no-brainer".

The last time I hinted removal of .git/branches/, Andrew Morton
reminded me that there are those who use Git primarily to fetch from
many dozens of other people's branches, to maintain his own quilt
patch series on top of, and never push anything back.  To them,
being able to say

    $ echo the-repository-url#the-branch >.git/branches/the-subsys
    $ rm .git/branches/the-subsys

has been a much easier and simpler way than "git config".  The only
thing they do with them is essentially:

    $ git fetch the-subsys
    ... use FETCH_HEAD to integrate it to a larger whole
    ... against which his remaining quilt queue is rebuilt

I myself thought that replacing the established work process of
these people to the one that instead uses "git config" should be
simple enough even back then, and in the longer term, these old
mechanisms will become disused so that we can remove them, but
deciding _when_ is the good time is not a no-brainer at all.

Re: [PATCH 00/16] Cleanup {branches,remotes}-file cruft

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The last time I hinted removal of .git/branches/, Andrew Morton
> reminded me that there are those who use Git primarily to fetch from
> many dozens of other people's branches, to maintain his own quilt
> patch series on top of, and never push anything back.  To them,
> being able to say
>
>     $ echo the-repository-url#the-branch >.git/branches/the-subsys
>     $ rm .git/branches/the-subsys
>
> has been a much easier and simpler way than "git config".  The only
> thing they do with them is essentially:
>
>     $ git fetch the-subsys
>     ... use FETCH_HEAD to integrate it to a larger whole
>     ... against which his remaining quilt queue is rebuilt

Interesting.  A cheap alias for a URL + branch that can be
added/removed very easily.  I suppose it's worth keeping around.

> I myself thought that replacing the established work process of
> these people to the one that instead uses "git config" should be
> simple enough even back then, and in the longer term, these old
> mechanisms will become disused so that we can remove them, but
> deciding _when_ is the good time is not a no-brainer at all.

Oh, this series is not about removing the feature: it's about removing
dead code, testing the feature properly, and not mentioning it in
obscure places.  It is a prerequisite for removal, if we desire to do
that at some later point.  After your explanation, I think everything
but [16/16] is fine.  [16/16] adds a comment about deprecation; we
should tweak/drop it.

Re: [PATCH 00/16] Cleanup {branches,remotes}-file cruft

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I myself thought that replacing the established work process of
> these people to the one that instead uses "git config" should be
> simple enough even back then, and in the longer term, these old
> mechanisms will become disused so that we can remove them, but
> deciding _when_ is the good time is not a no-brainer at all.

I do not fundamentally have a strong objection against deprecation
of these older mechanisms, if the removal is aimed to coincide with
a major version bump, like Git 2.0.

It however needs to be very well advertised, with clear instruction
to help migrate people's workflow and scripts to the mechanism that
will survive the purge (i.e. "git config").

We do not want to repeat the "We have advertised that 'git-foo' will
stop working at v1.6.0 for 6 months since v1.5.4 release notes, but
people complained loudly only after it happend." fiasco again for
something "minor" like this.

I say "minor" only because I think the cost of keeping these old
mechanisms alive is very low (if it is a heavy burden on the
maintenance, please tell me and how).

So from my point of view, a proposal to remove them has an almost no
benefit vs potentially very high cost of having to break many people
who are silently happily using them; not a very good benefit/cost
ratio.

Re: [PATCH 00/16] Cleanup {branches,remotes}-file cruft

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> So from my point of view, a proposal to remove them has an almost no
> benefit vs potentially very high cost of having to break many people
> who are silently happily using them; not a very good benefit/cost
> ratio.

Yep, it should stay around because it's useful to some people and harms nobody.

> I say "minor" only because I think the cost of keeping these old
> mechanisms alive is very low (if it is a heavy burden on the
> maintenance, please tell me and how).

Yeah, the point of this series is to reduce that maintenance burden.
After it is applied, there will be exactly two functions (each with
one caller) that are tested fully by precisely three tests in
t/remote; and exactly one piece of documentation will refer to
branches-file/remotes-file, namely urls-remotes.txt.

Re: [PATCH 01/16] t/t5505-remote: modernize subshell-style of one test

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Since we plan to edit the test "migrate a remote from named file in
> $GIT_DIR/remotes" in later patches, modernize the subshell-style by
> putting the parenthesis on separate lines and indenting the body.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

Good, but a "style only" patch like this should consider taking
advantage of the occasion to clean up the entire file, as we do not
often get enough chance to do so without conflicting with in-flight
topics.  Is there something else that would conflict if this step
did so?

>  t/t5505-remote.sh | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index dd10ff0..4d5810f 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -691,13 +691,15 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
>  test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
>  	git clone one six &&
>  	origin_url=$(pwd)/one &&
> -	(cd six &&
> -	 git remote rm origin &&
> -	 echo "$origin_url" > .git/branches/origin &&
> -	 git remote rename origin origin &&
> -	 ! test -f .git/branches/origin &&
> -	 test "$(git config remote.origin.url)" = "$origin_url" &&
> -	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
> +	(
> +		cd six &&
> +		git remote rm origin &&
> +		echo "$origin_url" > .git/branches/origin &&

A SP between > and the filename is still there?

> +		git remote rename origin origin &&
> +		! test -f .git/branches/origin &&
> +		test "$(git config remote.origin.url)" = "$origin_url" &&
> +		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
> +	)
>  '
>  
>  test_expect_success 'remote prune to cause a dangling symref' '

Re: [PATCH 02/16] t/t5505-remote: test push-refspec in branches-file

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The test "migrate a remote from named file in $GIT_DIR/branches" reads
> the branches-file, but only checks that the url and fetch-refspec are
> set correctly.  Check that the push-refspec is also set correctly.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5505-remote.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 4d5810f..38c62ec 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -698,7 +698,8 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
>  		git remote rename origin origin &&
>  		! test -f .git/branches/origin &&
>  		test "$(git config remote.origin.url)" = "$origin_url" &&
> -		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
> +		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin" &&
> +		test "$(git config remote.origin.push)" = "HEAD:refs/heads/master"

OK.  That is an ancient "Cogito-compatible push" which might not be
a sensible setting, but there is no point changing it now, and there
is a value in making sure it will stay working the way it has.

Good.


>  	)
>  '

Re: [PATCH 04/16] t/t5505-remote: remove dependency on $origin_url

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> In the tests "migrate a remote from named file in
> $GIT_DIR/{remotes,branches}", we are only checking that a configuration
> is migrated successfully; it has no correspondence with whether or not
> those values do something sensible with other git
> operations (fetch/push).  Therefore, there is no need to determine
> $origin_url: just substitute it with the constant value "quux".

Is there a reason why "quux" is better than another randomly chosen
string "$(pwd)/one"?



>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5505-remote.sh | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index dcb6c2f..fd0a81e 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -669,21 +669,20 @@ test_expect_success 'rename a remote with name prefix of other remote' '
>  '
>  
>  cat > remotes_origin << EOF
> -URL: $(pwd)/one
> +URL: quux
>  Push: refs/heads/master:refs/heads/upstream
>  Pull: refs/heads/master:refs/heads/origin
>  EOF
>  
>  test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
>  	git clone one five &&
> -	origin_url=$(pwd)/one &&
>  	(cd five &&
>  	 git remote remove origin &&
>  	 mkdir -p .git/remotes &&
>  	 cat ../remotes_origin > .git/remotes/origin &&
>  	 git remote rename origin origin &&
>  	 test_path_is_missing .git/remotes/origin &&
> -	 test "$(git config remote.origin.url)" = "$origin_url" &&
> +	 test "$(git config remote.origin.url)" = "quux" &&
>  	 test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
>  	 test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin")
>  '
> @@ -694,10 +693,10 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
>  	(
>  		cd six &&
>  		git remote rm origin &&
> -		echo "$origin_url" > .git/branches/origin &&
> +		echo quux > .git/branches/origin &&
>  		git remote rename origin origin &&
>  		test_path_is_missing .git/branches/origin &&
> -		test "$(git config remote.origin.url)" = "$origin_url" &&
> +		test "$(git config remote.origin.url)" = "quux" &&
>  		test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin" &&
>  		test "$(git config remote.origin.push)" = "HEAD:refs/heads/master"
>  	)

Re: [PATCH 05/16] remote: remove dead code in read_branches_file()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The first line of the function checks that the remote-name contains a
> slash ('/'), and sets the "slash" variable accordingly.  The only caller
> of read_branches_file() is remote_get_1(); the calling codepath is
> guarded by valid_remote_nick(), which checks that the remote does not
> contain a slash.  Therefore, the "slash" variable can never be set:
> remove the dead code that assumes otherwise.

This is extremely interesting.

As far as I can tell, that valid-remote-nick was done in df93e33c
(Validate nicknames of remote branches to prohibit confusing ones,
2008-02-15), and back in that version, the codepath and the feature
that wants to see a slash and do magical things, which is described
by this comment you are removing:

> -	/*
> -	 * With "slash", e.g. "git fetch jgarzik/netdev-2.6" when
> -	 * reading from $GIT_DIR/branches/jgarzik fetches "HEAD" from
> -	 * the partial URL obtained from the branches file plus
> -	 * "/netdev-2.6" and does not store it in any tracking ref.
> -	 * #branch specifier in the file is ignored.

did exist (see "git show df93e33c:remote.c").  It expects to see
"jgarzik/netdev-2.6" in remote->name, grabs the leading "jgarzik"
part to form ".git/branches/jgarzik" to open and read from, and then
appends the remainder "/netdev-2.6" to the "partial URL" it read
to come up with the final URL.

So it appears that back then (and througout to today), nobody uses
that "partial URL" feature which is specific (and was a rather nice
invention/legacy by Cogito) to .git/branches file.

Reminds me of the strategy to deprecate functionality in X (cf. 
http://lwn.net/Articles/536520/) ;-)

Re: [PATCH 06/16] t/t5505-remote: test url-with-# in branches-file

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Add one more test similar to "migrate a remote from named file in
> $GIT_DIR/branches" to check that a url with a # can be used to specify
> the branch name (as opposed to the constant "master").
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5505-remote.sh | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index fd0a81e..93e11c8 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -702,27 +702,42 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
>  	)
>  '
>  
> -test_expect_success 'remote prune to cause a dangling symref' '
> +test_expect_success 'migrate a remote from named file in $GIT_DIR/branches (2)' '
>  	git clone one seven &&
> +	origin_url=$(pwd)/one &&

The variable assigned here does not seem to get used.  Is this needed?

> +	(
> +		cd seven &&
> +		git remote rm origin &&
> +		echo "quux#foom" > .git/branches/origin &&
> +		git remote rename origin origin &&
> +		test_path_is_missing .git/branches/origin &&
> +		test "$(git config remote.origin.url)" = "quux" &&
> +		test "$(git config remote.origin.fetch)" = "refs/heads/foom:refs/heads/origin"
> +		test "$(git config remote.origin.push)" = "HEAD:refs/heads/foom"
> +	)
> +'
> +
> +test_expect_success 'remote prune to cause a dangling symref' '
> +	git clone one eight &&
>  	(
>  		cd one &&
>  		git checkout side2 &&
>  		git branch -D master
>  	) &&
>  	(
> -		cd seven &&
> +		cd eight &&
>  		git remote prune origin
>  	) >err 2>&1 &&
>  	test_i18ngrep "has become dangling" err &&
>  
>  	: And the dangling symref will not cause other annoying errors &&
>  	(
> -		cd seven &&
> +		cd eight &&
>  		git branch -a
>  	) 2>err &&
>  	! grep "points nowhere" err &&
>  	(
> -		cd seven &&
> +		cd eight &&
>  		test_must_fail git branch nomore origin
>  	) 2>err &&
>  	grep "dangling symref" err

Re: [PATCH 07/16] t/t5516-fetch-push: don't use branches-file

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Four tests exercising fetch and push functionality unnecessarily depend
> on $GIT_DIR/branches files.  Modern Git does not encourage the use of
> those files, and the parser remote.c:read_branches_file() is only
> provided for backward compatibility with older repositories.  We already
> have tests in t/t5505-remote to verify that the parser works: so,
> substitute the $GIT_DIR/branches configuration with an equivalent
> gitconfig-style configuration, using the results of those tests.

As we are not deprecating, I think we agree we would prefer to keep
these working.  If we need tests using remotes/ or config, they
should be added as their own tests, not by removing branches/ one.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5516-fetch-push.sh | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 4691d51..6e9fa84 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -852,9 +852,11 @@ test_expect_success 'fetch with branches' '
>  	mk_empty testrepo &&
>  	git branch second $the_first_commit &&
>  	git checkout second &&
> -	echo ".." > testrepo/.git/branches/branch1 &&
>  	(
>  		cd testrepo &&
> +		test_config remote.branch1.url ".."  &&
> +		test_config remote.branch1.fetch "refs/heads/master:refs/heads/branch1"  &&
> +		test_config remote.branch1.push "HEAD:refs/heads/master"  &&
>  		git fetch branch1 &&
>  		echo "$the_commit commit	refs/heads/branch1" >expect &&
>  		git for-each-ref refs/heads >actual &&
> @@ -865,9 +867,11 @@ test_expect_success 'fetch with branches' '
>  
>  test_expect_success 'fetch with branches containing #' '
>  	mk_empty testrepo &&
> -	echo "..#second" > testrepo/.git/branches/branch2 &&
>  	(
>  		cd testrepo &&
> +		test_config remote.branch2.url ".."  &&
> +		test_config remote.branch2.fetch "refs/heads/second:refs/heads/branch2"  &&
> +		test_config remote.branch2.push "HEAD:refs/heads/second"  &&
>  		git fetch branch2 &&
>  		echo "$the_first_commit commit	refs/heads/branch2" >expect &&
>  		git for-each-ref refs/heads >actual &&
> @@ -879,7 +883,9 @@ test_expect_success 'fetch with branches containing #' '
>  test_expect_success 'push with branches' '
>  	mk_empty testrepo &&
>  	git checkout second &&
> -	echo "testrepo" > .git/branches/branch1 &&
> +	test_config remote.branch1.url testrepo &&
> +	test_config remote.branch1.fetch "refs/heads/master:refs/heads/branch1" &&
> +	test_config remote.branch1.push "HEAD:refs/heads/master" &&
>  	git push branch1 &&
>  	(
>  		cd testrepo &&
> @@ -891,7 +897,9 @@ test_expect_success 'push with branches' '
>  
>  test_expect_success 'push with branches containing #' '
>  	mk_empty testrepo &&
> -	echo "testrepo#branch3" > .git/branches/branch2 &&
> +	test_config remote.branch2.url testrepo &&
> +	test_config remote.branch2.fetch "refs/heads/branch3:refs/heads/branch2" &&
> +	test_config remote.branch2.push "HEAD:refs/heads/branch3" &&
>  	git push branch2 &&
>  	(
>  		cd testrepo &&

Re: [PATCH 08/16] t/t5516-fetch-push: use test_config()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Replace the 'git config' calls in tests with test_config for greater
> robustness.

That may be a good thing in principle, but I _think_

	mk_empty testrepo &&
        (
        	cd testrepo &&
                do whatever to its config &&
                run test
	)

sequence is used so that we do not even have to worry about what
leftover configuration values are in the testrepo/.git/config; so
does it really matter?

If this conversion had something more than "s/git config/test_config/"
replacement, that would indicate that you uncovered a bug in the
existing test and found a good fix, but that does not seem to be the
case for this particular patch.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t5516-fetch-push.sh | 46 +++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 6e9fa84..afb25c4 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -142,8 +142,8 @@ test_expect_success 'fetch with wildcard' '
>  	mk_empty testrepo &&
>  	(
>  		cd testrepo &&
> -		git config remote.up.url .. &&
> -		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
> +		test_config remote.up.url .. &&
> +		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
>  		git fetch up &&
>  
>  		echo "$the_commit commit	refs/remotes/origin/master" >expect &&
> @@ -157,9 +157,9 @@ test_expect_success 'fetch with insteadOf' '
>  	(
>  		TRASH=$(pwd)/ &&
>  		cd testrepo &&
> -		git config "url.$TRASH.insteadOf" trash/ &&
> -		git config remote.up.url trash/. &&
> -		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
> +		test_config "url.$TRASH.insteadOf" trash/ &&
> +		test_config remote.up.url trash/. &&
> +		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
>  		git fetch up &&
>  
>  		echo "$the_commit commit	refs/remotes/origin/master" >expect &&
> @@ -173,9 +173,9 @@ test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '
>  	(
>  		TRASH=$(pwd)/ &&
>  		cd testrepo &&
> -		git config "url.trash/.pushInsteadOf" "$TRASH" &&
> -		git config remote.up.url "$TRASH." &&
> -		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
> +		test_config "url.trash/.pushInsteadOf" "$TRASH" &&
> +		test_config remote.up.url "$TRASH." &&
> +		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
>  		git fetch up &&
>  
>  		echo "$the_commit commit	refs/remotes/origin/master" >expect &&
> @@ -780,7 +780,7 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
>  
>  test_expect_success 'allow deleting a ref using --delete' '
>  	mk_test testrepo heads/master &&
> -	(cd testrepo && git config receive.denyDeleteCurrent warn) &&
> +	(cd testrepo && test_config receive.denyDeleteCurrent warn) &&
>  	git push testrepo --delete master &&
>  	(cd testrepo && test_must_fail git rev-parse --verify refs/heads/master)
>  '
> @@ -809,7 +809,7 @@ test_expect_success 'warn on push to HEAD of non-bare repository' '
>  	(
>  		cd testrepo &&
>  		git checkout master &&
> -		git config receive.denyCurrentBranch warn
> +		test_config receive.denyCurrentBranch warn
>  	) &&
>  	git push testrepo master 2>stderr &&
>  	grep "warning: updating the current branch" stderr
> @@ -820,7 +820,7 @@ test_expect_success 'deny push to HEAD of non-bare repository' '
>  	(
>  		cd testrepo &&
>  		git checkout master &&
> -		git config receive.denyCurrentBranch true
> +		test_config receive.denyCurrentBranch true
>  	) &&
>  	test_must_fail git push testrepo master
>  '
> @@ -830,8 +830,8 @@ test_expect_success 'allow push to HEAD of bare repository (bare)' '
>  	(
>  		cd testrepo &&
>  		git checkout master &&
> -		git config receive.denyCurrentBranch true &&
> -		git config core.bare true
> +		test_config receive.denyCurrentBranch true &&
> +		test_config core.bare true
>  	) &&
>  	git push testrepo master 2>stderr &&
>  	! grep "warning: updating the current branch" stderr
> @@ -842,7 +842,7 @@ test_expect_success 'allow push to HEAD of non-bare repository (config)' '
>  	(
>  		cd testrepo &&
>  		git checkout master &&
> -		git config receive.denyCurrentBranch false
> +		test_config receive.denyCurrentBranch false
>  	) &&
>  	git push testrepo master 2>stderr &&
>  	! grep "warning: updating the current branch" stderr
> @@ -918,7 +918,7 @@ test_expect_success 'push into aliased refs (consistent)' '
>  		cd child1 &&
>  		git branch foo &&
>  		git symbolic-ref refs/heads/bar refs/heads/foo
> -		git config receive.denyCurrentBranch false
> +		test_config receive.denyCurrentBranch false
>  	) &&
>  	(
>  		cd child2 &&
> @@ -940,7 +940,7 @@ test_expect_success 'push into aliased refs (inconsistent)' '
>  		cd child1 &&
>  		git branch foo &&
>  		git symbolic-ref refs/heads/bar refs/heads/foo
> -		git config receive.denyCurrentBranch false
> +		test_config receive.denyCurrentBranch false
>  	) &&
>  	(
>  		cd child2 &&
> @@ -1006,7 +1006,7 @@ test_expect_success 'push --porcelain rejected' '
>  	git push testrepo refs/heads/master:refs/remotes/origin/master &&
>  	(cd testrepo &&
>  		git reset --hard origin/master^
> -		git config receive.denyCurrentBranch true) &&
> +		test_config receive.denyCurrentBranch true) &&
>  
>  	echo >.git/foo  "To testrepo"  &&
>  	echo >>.git/foo "!	refs/heads/master:refs/heads/master	[remote rejected] (branch is currently checked out)" &&
> @@ -1020,7 +1020,7 @@ test_expect_success 'push --porcelain --dry-run rejected' '
>  	git push testrepo refs/heads/master:refs/remotes/origin/master &&
>  	(cd testrepo &&
>  		git reset --hard origin/master
> -		git config receive.denyCurrentBranch true) &&
> +		test_config receive.denyCurrentBranch true) &&
>  
>  	echo >.git/foo  "To testrepo"  &&
>  	echo >>.git/foo "!	refs/heads/master^:refs/heads/master	[rejected] (non-fast-forward)" &&
> @@ -1052,7 +1052,7 @@ do
>  		mk_test testrepo heads/master hidden/one hidden/two hidden/three &&
>  		(
>  			cd testrepo &&
> -			git config $configsection.hiderefs refs/hidden
> +			test_config $configsection.hiderefs refs/hidden
>  		) &&
>  
>  		# push to unhidden ref succeeds normally
> @@ -1078,7 +1078,7 @@ test_expect_success 'fetch exact SHA1' '
>  	git push testrepo master:refs/hidden/one &&
>  	(
>  		cd testrepo &&
> -		git config transfer.hiderefs refs/hidden
> +		test_config transfer.hiderefs refs/hidden
>  	) &&
>  	check_push_result testrepo $the_commit hidden/one &&
>  
> @@ -1098,7 +1098,7 @@ test_expect_success 'fetch exact SHA1' '
>  		# the server side can allow it to succeed
>  		(
>  			cd ../testrepo &&
> -			git config uploadpack.allowtipsha1inwant true
> +			test_config uploadpack.allowtipsha1inwant true
>  		) &&
>  
>  		git fetch -v ../testrepo $the_commit:refs/heads/copy &&
> @@ -1126,8 +1126,8 @@ test_expect_success 'fetch follows tags by default' '
>  	(
>  		cd dst &&
>  		git remote add origin ../src &&
> -		git config branch.master.remote origin &&
> -		git config branch.master.merge refs/heads/master &&
> +		test_config branch.master.remote origin &&
> +		test_config branch.master.merge refs/heads/master &&
>  		git pull &&
>  		git for-each-ref >../actual
>  	) &&

Re: [PATCH 09/16] ls-remote doc: fix example invocation on git.git

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Under the EXAMPLES section, there is one invocation on the git.git
> repository that attempts to list the refs master, pu, and rc.  The ref
> rc does not exist in today's repository, so remove it.

Hmph, I am on the fence but slightly on the negative side.  It is
just an example to show how to use the command and how the command
may respond.  If you ask your repository about --tags today, you
will not get the v0.99 tags or a blob that has my public gpg key,
and the readers would understand it.

Removing the output line for rc without changing the command line
would be a good change, though.  We do not demonstrate that these
are patterns that filter, and do not explain that not having rc over
there is not an error.

> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-ls-remote.txt | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 774de5e..a24b8b6 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -67,10 +67,9 @@ EXAMPLES
>  	7ceca275d047c90c0c7d5afb13ab97efdf51bd6e	refs/tags/v0.99.3
>  	c5db5456ae3b0873fc659c19fafdde22313cc441	refs/tags/v0.99.2
>  	0918385dbd9656cab0d1d81ba7453d49bbc16250	refs/tags/junio-gpg-pub
> -	$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master pu rc
> +	$ git ls-remote http://www.kernel.org/pub/scm/git/git.git master pu
>  	5fe978a5381f1fbad26a80e682ddd2a401966740	refs/heads/master
>  	c781a84b5204fb294c9ccc79f8b3baceeb32c061	refs/heads/pu
> -	b1d096f2926c4e37c9c0b6a7bf2119bedaa277cb	refs/heads/rc
>  	$ echo http://www.kernel.org/pub/scm/git/git.git >.git/branches/public
>  	$ git ls-remote --tags public v\*
>  	d6602ec5194c87b0fc87103ca4d67251c76f233a	refs/tags/v0.99

Re: [PATCH 10/16] ls-remote doc: rewrite <repository> paragraph

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Replace the <repository> paragraph containing specific references to
> $GIT_DIR/branches and "." with a generic urls-or-remotes paragraph
> referencing the relevant sections in the git-fetch(1) manpage.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  Documentation/git-ls-remote.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index a24b8b6..0715892 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -48,9 +48,9 @@ OPTIONS
>  	exit without talking to the remote.
>  
>  <repository>::
> -	Location of the repository.  The shorthand defined in
> -	$GIT_DIR/branches/ can be used. Use "." (dot) to list references in
> -	the local repository.
> +	The "remote" repository to query.  This parameter can be
> +	either a URL or the name of a remote (see the GIT URLS and
> +	REMOTES sections of linkgit:git-fetch[1]).

Looks OK.  "." was written before "for-each-ref" and "branch --list"
have become the standard way to list your local refs and branches,
and I would agree that the description outlived its usefulness.

Re: [PATCH 13/16] t/t5505-remote: test multiple push/pull in remotes-file

[Eric Sunshine]

On Fri, Jun 21, 2013 at 7:12 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Extend the test "migrate a remote from named file in $GIT_DIR/remotes"
> to test that multiple "Push:" and "Pull:" lines in the remotes-file
> works as expected.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 06ed381..5497a23 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -684,8 +686,18 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/remotes' '
>                 git remote rename origin origin &&
>                 test_path_is_missing .git/remotes/origin &&
>                 test "$(git config remote.origin.url)" = "quux" &&
> -               test "$(git config remote.origin.push)" = "refs/heads/master:refs/heads/upstream" &&
> -               test "$(git config remote.origin.fetch)" = "refs/heads/master:refs/heads/origin"
> +               cat >push_expected <<-\EOF

Broken &&-chain.

> +               refs/heads/master:refs/heads/upstream
> +               refs/heads/master:refs/heads/upstream2
> +               EOF
> +               cat >fetch_expected <<-\EOF

Ditto.

> +               refs/heads/master:refs/heads/origin
> +               refs/heads/master:refs/heads/origin2
> +               EOF
> +               git config --get-all remote.origin.fetch >push_actual &&
> +               git config --get-all remote.origin.fetch >fetch_actual &&
> +               test_cmp push_expected push_actual &&
> +               test_cmp fetch_expected fetch_actual &&
>         )
>  '
>
> --

Re: [PATCH 01/16] t/t5505-remote: modernize subshell-style of one test

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Good, but a "style only" patch like this should consider taking
> advantage of the occasion to clean up the entire file, as we do not
> often get enough chance to do so without conflicting with in-flight
> topics.  Is there something else that would conflict if this step
> did so?

No, but the problem is that the file contains mixed-style which
prevents me from automating my work with regular-expressions and
macros easily.  I have to break it up into regions and operate
regexps/macros on specific regions, multiplying the number of errors I
can possibly commit.  Nevertheless, I'll do it: I don't want this to
be a maintenance burden for us.

Re: [PATCH 04/16] t/t5505-remote: remove dependency on $origin_url

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Is there a reason why "quux" is better than another randomly chosen
> string "$(pwd)/one"?

"$(pwd)/one" is not randomly chosen: that configuration will work with
push/pull, and is therefore misleading.  I put in a deliberately bogus
value because I wanted to make it clear that we're only testing
anything config-parsing.

Re: [PATCH 08/16] t/t5516-fetch-push: use test_config()

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> That may be a good thing in principle, but I _think_
> [...]
> sequence is used so that we do not even have to worry about what
> leftover configuration values are in the testrepo/.git/config; so
> does it really matter?

Yeah, you're right.  Dropped.

Re: [PATCH 06/16] t/t5505-remote: test url-with-# in branches-file

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
>> index fd0a81e..93e11c8 100755
>> --- a/t/t5505-remote.sh
>> +++ b/t/t5505-remote.sh
>> @@ -702,27 +702,42 @@ test_expect_success 'migrate a remote from named file in $GIT_DIR/branches' '
>>       )
>>  '
>>
>> -test_expect_success 'remote prune to cause a dangling symref' '
>> +test_expect_success 'migrate a remote from named file in $GIT_DIR/branches (2)' '
>>       git clone one seven &&
>> +     origin_url=$(pwd)/one &&
>
> The variable assigned here does not seem to get used.  Is this needed?

Silly error carried over from copy-pasting.  Fixed.

Re: [PATCH 05/16] remote: remove dead code in read_branches_file()

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> As far as I can tell, that valid-remote-nick was done in df93e33c
> (Validate nicknames of remote branches to prohibit confusing ones,
> 2008-02-15), and back in that version, the codepath and the feature
> that wants to see a slash and do magical things, which is described
> by this comment you are removing:

It's a bad hack, in my opinion.  When I say git fetch ../foomery, it
should catch the pattern at transport_get() and not even attempt to
look up a remote in the first place.  I will attempt to clean this up
soon.

> So it appears that back then (and througout to today), nobody uses
> that "partial URL" feature which is specific (and was a rather nice
> invention/legacy by Cogito) to .git/branches file.

It's quite a cute feature, even if a bit magical.

> Reminds me of the strategy to deprecate functionality in X (cf.
> http://lwn.net/Articles/536520/) ;-)

Leaving dead code around to confuse readers? :\

Re: [PATCH 13/16] t/t5505-remote: test multiple push/pull in remotes-file

[Ramkumar Ramachandra]

Eric Sunshine wrote:
> Broken &&-chain.

Good eyes, thanks.

Re: [PATCH 08/16] t/t5516-fetch-push: use test_config()

[Johannes Sixt]

Am 22.06.2013 00:32, schrieb Junio C Hamano:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
>> Replace the 'git config' calls in tests with test_config for greater
>> robustness.
> 
> That may be a good thing in principle, but I _think_
> 
> 	mk_empty testrepo &&
>         (
>         	cd testrepo &&
>                 do whatever to its config &&
>                 run test
> 	)
> 
> sequence is used so that we do not even have to worry about what
> leftover configuration values are in the testrepo/.git/config; so
> does it really matter?
> 
> If this conversion had something more than "s/git config/test_config/"
> replacement, that would indicate that you uncovered a bug in the
> existing test and found a good fix, but that does not seem to be the
> case for this particular patch.

And just let me add that the added benefit of test_config to remove the
configuration change after the test case finished would not work because
the test_when_finished registration that happens behind the scenes would
be forgotten when the sub-shell exits.

>> @@ -142,8 +142,8 @@ test_expect_success 'fetch with wildcard' '
>>  	mk_empty testrepo &&
>>  	(
>>  		cd testrepo &&
>> -		git config remote.up.url .. &&
>> -		git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
>> +		test_config remote.up.url .. &&
>> +		test_config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
>>  		git fetch up &&
...

-- Hannes

Re: [PATCH 04/16] t/t5505-remote: remove dependency on $origin_url

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Is there a reason why "quux" is better than another randomly chosen
>> string "$(pwd)/one"?
>
> "$(pwd)/one" is not randomly chosen: that configuration will work with
> push/pull, and is therefore misleading.

But isn't the "URL:" field in remotes file meant to be usable with
push and pull?  Why is it misleading to put a value that is more
plausible (I believe "'one' in the current directory" is another
repository and the intent is that the old .git/remotes/origin setup
lets you fetch from and push to that repository) there?

I do not think you are creating a quux repository to be pushed into
and fetched from with this change. Placing 'quux' in the "URL:" field
feels a lot more misleading to me.  What am I missing?

Re: [PATCH 04/16] t/t5505-remote: remove dependency on $origin_url

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I do not think you are creating a quux repository to be pushed into
> and fetched from with this change. Placing 'quux' in the "URL:" field
> feels a lot more misleading to me.  What am I missing?

Dropped.

I'll send in a re-roll based on what you put up on pu.

Re: [PATCH 05/16] remote: remove dead code in read_branches_file()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> Reminds me of the strategy to deprecate functionality in X (cf.
>> http://lwn.net/Articles/536520/) ;-)
>
> Leaving dead code around to confuse readers? :\

Did you actually read what was quoted?

We broke the use case to access jgarzik/netdev-2.6 only by having
jgarzik remote accidentally, and waited for quite a while (since
early 2008 until now) to see if anobody screamed.  Nobody did.  We
now know we can remove that feature.

That is exactly what Keith is describing, isn't it?

Re: [PATCH 05/16] remote: remove dead code in read_branches_file()

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>>> Reminds me of the strategy to deprecate functionality in X (cf.
>>> http://lwn.net/Articles/536520/) ;-)
>>
>> Leaving dead code around to confuse readers? :\
>
> We broke the use case to access jgarzik/netdev-2.6 only by having
> jgarzik remote accidentally, and waited for quite a while (since
> early 2008 until now) to see if anobody screamed.  Nobody did.  We
> now know we can remove that feature.

Did we _not_ leave dead code around to confuse readers (no comment or
log message indicating intent), or am I missing something?

> That is exactly what Keith is describing, isn't it?

Wasn't that a joke?

Re: [PATCH 05/16] remote: remove dead code in read_branches_file()

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>>> Reminds me of the strategy to deprecate functionality in X (cf.
>>>> http://lwn.net/Articles/536520/) ;-)
>>>
>>> Leaving dead code around to confuse readers? :\
>>
>> We broke the use case to access jgarzik/netdev-2.6 only by having
>> jgarzik remote accidentally, and waited for quite a while (since
>> early 2008 until now) to see if anobody screamed.  Nobody did.  We
>> now know we can remove that feature.
>
> Did we _not_ leave dead code around to confuse readers (no comment or
> log message indicating intent), or am I missing something?

The largest thing you are missing is that it does not have anything
to do with my "Reminds me of ..." comment.  The silent breakage by
df93e33c is exactly "broke silently and started waiting for bug
report, which never came."  It is not about those reading the code
(only to them, dead code matters) at all, but all about users who
use the system without reading code at all.