[PATCH 0/6] Default remote

[PATCH 0/6] Default remote

[marcnarc]

This series is a follow-up to an earlier patch and discussion[1] that
suggested adding a remote.default setting.

The first patch simply renames a couple of variables in remote.c so that
the second patch is easier to understand.

The second patch teaches remote.c to look for remote.default in the config:

 - When deciding which remote to use, the code first tries the currently
   checked-out branch's remote.  If there isn't one it tries remote.default,
   and if that also fails it falls back to "origin".

 - The patch also adds a couple of helper functions for other code to use:
   remote_get_default_name() and remote_count().  remote_get_default_name()
   returns the value of remote.default, or "origin" if it's not configured
   (this preserves existing behavior).  remote_count() is used in patch
   four by "git remote add".

The third patch teaches "git clone" to set remote.default.

The fourth patch teaches "git remote" about remote.default.  In addition to
modifying the existing "add" "rm" and "rename" commands, it also adds a new
"git remote default" command to get/set remote.default.  The advantage of this
over just using "git config" directly is twofold:

 - "git remote default foo" checks that foo is a configured remote.

 - "git remote default" (with no parameter) returns "origin" if 
   remote.default isn't configured.

Note that this patch changes the way push.default=matching works when the
currently checked-out branch has no remote.  Before "git push" would error out
with "No configured push destination" but now it succeeds with "Everything
up-to-date".  I personally think this is a good thing.

The fifth patch just adds a test that plain "git fetch" respects
remote.default when on remoteless branch.  I suppose it could be squashed into
patch four, but it's really more of a side-effect of that work and not
strictly required by it.  So I felt patch four would be more understandable
without it.

The sixth patch changes git-parse-remote.sh:get_default_remote() to use
"git remote default" instead of implementing its own default-finding logic.
Because "git remote default" returns "origin" when no remote.default is
configured, this change preserves the old behavior in existing repos.  The
test accompanying this patch essentially tests the same condition that
inspired the original discussion[1].  (git-submodule is pretty much the only
thing that uses get_default_remote().)

This series still needs documentation updates, which I'll do if/when we
agree on the code changes.

		M.

[1] http://article.gmane.org/gmane.comp.version-control.git/200145

Marc Branchaud (6):
      Rename remote.c's default_remote_name static variables.
      Teach remote.c about the remote.default configuration setting.
      Teach clone to set remote.default.
      Teach "git remote" about remote.default.
      Test that plain "git fetch" uses remote.default when on a detached HEAD.
      Teach get_default_remote to respect remote.default.

 builtin/clone.c            |  2 ++
 builtin/remote.c           | 29 +++++++++++++++++++++
 git-parse-remote.sh        |  5 +---
 remote.c                   | 35 ++++++++++++++++++++-----
 remote.h                   |  2 ++
 t/t5505-remote.sh          | 64 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t5510-fetch.sh           | 17 ++++++++++++
 t/t5512-ls-remote.sh       |  8 +++++-
 t/t5528-push-default.sh    |  4 +--
 t/t5601-clone.sh           | 10 ++++++++
 t/t5702-clone-options.sh   |  7 +++--
 t/t7400-submodule-basic.sh | 21 +++++++++++++++
 12 files changed, 188 insertions(+), 16 deletions(-)

[PATCH 1/6] Rename remote.c's default_remote_name static variables.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

This prepares the code to handle a true remote.default configuration value.

default_remote_name --> effective_remote_name
explicit_default_remote_name --> explicit_effective_remote_name

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 remote.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/remote.c b/remote.c
index 6833538..6f371e0 100644
--- a/remote.c
+++ b/remote.c
@@ -47,8 +47,8 @@ static int branches_alloc;
 static int branches_nr;
 
 static struct branch *current_branch;
-static const char *default_remote_name;
-static int explicit_default_remote_name;
+static const char *effective_remote_name;
+static int explicit_effective_remote_name;
 
 static struct rewrites rewrites;
 static struct rewrites rewrites_push;
@@ -360,8 +360,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 				return config_error_nonbool(key);
 			branch->remote_name = xstrdup(value);
 			if (branch == current_branch) {
-				default_remote_name = branch->remote_name;
-				explicit_default_remote_name = 1;
+				effective_remote_name = branch->remote_name;
+				explicit_effective_remote_name = 1;
 			}
 		} else if (!strcmp(subkey, ".merge")) {
 			if (!value)
@@ -481,9 +481,9 @@ static void read_config(void)
 	unsigned char sha1[20];
 	const char *head_ref;
 	int flag;
-	if (default_remote_name) /* did this already */
+	if (effective_remote_name) /* did this already */
 		return;
-	default_remote_name = xstrdup("origin");
+	effective_remote_name = xstrdup("origin");
 	current_branch = NULL;
 	head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
@@ -680,8 +680,8 @@ struct remote *remote_get(const char *name)
 	if (name)
 		name_given = 1;
 	else {
-		name = default_remote_name;
-		name_given = explicit_default_remote_name;
+		name = effective_remote_name;
+		name_given = explicit_effective_remote_name;
 	}
 
 	ret = make_remote(name, 0);
-- 
1.7.11.1

[PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

The code now has a default_remote_name and an effective_remote_name:
 - default_remote_name is set by remote.default in the config, or is "origin"
   if remote.default doesn't exist ("origin" was the fallback value before
   this change).
 - effective_remote_name is the name of the remote tracked by the current
   branch, or is default_remote_name if the current branch doesn't have a
   remote.

Also add two new helper functions for other code modules to use:
 - remote_get_default_name() returns default_remote_name.
 - remote_count() returns the number of remotes.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 remote.c | 27 ++++++++++++++++++++++++---
 remote.h |  2 ++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index 6f371e0..ccada0d 100644
--- a/remote.c
+++ b/remote.c
@@ -47,6 +47,7 @@ static int branches_alloc;
 static int branches_nr;
 
 static struct branch *current_branch;
+static const char *default_remote_name;
 static const char *effective_remote_name;
 static int explicit_effective_remote_name;
 
@@ -390,6 +391,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 	}
 	if (prefixcmp(key,  "remote."))
 		return 0;
+
 	name = key + 7;
 	if (*name == '/') {
 		warning("Config remote shorthand cannot begin with '/': %s",
@@ -397,8 +399,12 @@ static int handle_config(const char *key, const char *value, void *cb)
 		return 0;
 	}
 	subkey = strrchr(name, '.');
-	if (!subkey)
+	if (!subkey) {
+		/* Look for remote.default */
+		if (!strcmp(name, "default"))
+			default_remote_name = xstrdup(value);
 		return 0;
+	}
 	remote = make_remote(name, subkey - name);
 	remote->origin = REMOTE_CONFIG;
 	if (!strcmp(subkey, ".mirror"))
@@ -481,9 +487,8 @@ static void read_config(void)
 	unsigned char sha1[20];
 	const char *head_ref;
 	int flag;
-	if (effective_remote_name) /* did this already */
+	if (default_remote_name) /* did this already */
 		return;
-	effective_remote_name = xstrdup("origin");
 	current_branch = NULL;
 	head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
@@ -492,6 +497,10 @@ static void read_config(void)
 			make_branch(head_ref + strlen("refs/heads/"), 0);
 	}
 	git_config(handle_config, NULL);
+	if (!default_remote_name)
+		default_remote_name = "origin";
+	if (!effective_remote_name)
+		effective_remote_name = default_remote_name;
 	alias_all_urls();
 }
 
@@ -671,6 +680,18 @@ static int valid_remote_nick(const char *name)
 	return !strchr(name, '/'); /* no slash */
 }
 
+const char *remote_get_default_name()
+{
+	read_config();
+	return default_remote_name;
+}
+
+int remote_count()
+{
+	read_config();
+	return remotes_nr;
+}
+
 struct remote *remote_get(const char *name)
 {
 	struct remote *ret;
diff --git a/remote.h b/remote.h
index 251d8fd..f9aac87 100644
--- a/remote.h
+++ b/remote.h
@@ -52,6 +52,8 @@ struct remote {
 
 struct remote *remote_get(const char *name);
 int remote_is_configured(const char *name);
+const char *remote_get_default_name();
+int remote_count();
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
-- 
1.7.11.1

[PATCH 3/6] Teach clone to set remote.default.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 builtin/clone.c          |  2 ++
 t/t5601-clone.sh         | 10 ++++++++++
 t/t5702-clone-options.sh |  7 +++++--
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index a4d8d25..b198456 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -770,6 +770,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);
 
+	git_config_set("remote.default", option_origin);
+
 	if (option_reference.nr)
 		setup_reference();
 
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 67869b4..046610d 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -57,6 +57,16 @@ test_expect_success 'clone checks out files' '
 
 '
 
+test_expect_success 'clone sets remote.default' '
+
+	rm -fr dst &&
+	git clone src dst &&
+	(
+		cd dst &&
+		test "$(git config --get remote.default)" = origin
+	)
+'
+
 test_expect_success 'clone respects GIT_WORK_TREE' '
 
 	GIT_WORK_TREE=worktree git clone src bare &&
diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh
index 02cb024..9e573dd 100755
--- a/t/t5702-clone-options.sh
+++ b/t/t5702-clone-options.sh
@@ -15,8 +15,11 @@ test_expect_success 'setup' '
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&
-	(cd clone-o && git rev-parse --verify refs/remotes/foo/master)
-
+	(
+		cd clone-o &&
+		git rev-parse --verify refs/remotes/foo/master &&
+		test "$(git config --get remote.default)" = foo
+	)
 '
 
 test_expect_success 'redirected clone' '
-- 
1.7.11.1

[PATCH 4/6] Teach "git remote" about remote.default.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

The "rename" and "rm" commands now handle the case where the remote being
changed is the default remote.

If the "add" command is used to add the repo's first remote, that remote
becomes the default remote.

Also added a "default" command to get or set the default remote:
 "git remote default" (with no parameter) displays the current default remote.
 "git remote default <remo>" checks if <remo> is a configured remote and if
 so changes remote.default to <remo>.

(The test in t5528 had to be changed because now "git push" when
push.default=matching succeeds with "Everything up-to-date".  It used to
fail with "No configured push destination".)

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 builtin/remote.c        | 29 ++++++++++++++++++++++
 t/t5505-remote.sh       | 64 +++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5512-ls-remote.sh    |  8 ++++++-
 t/t5528-push-default.sh |  4 ++--
 4 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 920262d..ac98765 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -12,6 +12,7 @@ static const char * const builtin_remote_usage[] = {
 	"git remote add [-t <branch>] [-m <master>] [-f] [--tags|--no-tags] [--mirror=<fetch|push>] <name> <url>",
 	"git remote rename <old> <new>",
 	"git remote rm <name>",
+	"git remote default [name]",
 	"git remote set-head <name> (-a | -d | <branch>)",
 	"git remote [-v | --verbose] show [-n] <name>",
 	"git remote prune [-n | --dry-run] <name>",
@@ -198,6 +199,9 @@ static int add(int argc, const char **argv)
 	if (!valid_fetch_refspec(buf2.buf))
 		die(_("'%s' is not a valid remote name"), name);
 
+	if (remote_count() == 1)
+		git_config_set("remote.default", name);
+
 	strbuf_addf(&buf, "remote.%s.url", name);
 	if (git_config_set(buf.buf, url))
 		return 1;
@@ -656,6 +660,10 @@ static int mv(int argc, const char **argv)
 		return error(_("Could not rename config section '%s' to '%s'"),
 				buf.buf, buf2.buf);
 
+	if (!strcmp(oldremote->name, remote_get_default_name())) {
+		git_config_set("remote.default", newremote->name);
+	}
+
 	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s.fetch", rename.new);
 	if (git_config_set_multivar(buf.buf, NULL, NULL, 1))
@@ -798,6 +806,10 @@ static int rm(int argc, const char **argv)
 	if (git_config_rename_section(buf.buf, NULL) < 1)
 		return error(_("Could not remove config section '%s'"), buf.buf);
 
+	if (!strcmp(remote->name, remote_get_default_name())) {
+		git_config_set("remote.default", NULL);
+	}
+
 	read_branches();
 	for (i = 0; i < branch_list.nr; i++) {
 		struct string_list_item *item = branch_list.items + i;
@@ -845,6 +857,21 @@ static int rm(int argc, const char **argv)
 	return result;
 }
 
+static int deflt(int argc, const char **argv)
+{
+	if (argc < 2)
+		printf_ln("%s", remote_get_default_name());
+	else {
+		const char *name = argv[1];
+		if (remote_is_configured(name)) {
+			git_config_set("remote.default", name);
+			printf_ln(_("Default remote set to '%s'."), name);
+		} else
+			return error(_("No remote named '%s'."), name);
+	}
+	return 0;
+}
+
 static void clear_push_info(void *util, const char *string)
 {
 	struct push_info *info = util;
@@ -1582,6 +1609,8 @@ int cmd_remote(int argc, const char **argv, const char *prefix)
 		result = mv(argc, argv);
 	else if (!strcmp(argv[0], "rm"))
 		result = rm(argc, argv);
+	else if (!strcmp(argv[0], "default"))
+		result = deflt(argc, argv);
 	else if (!strcmp(argv[0], "set-head"))
 		result = set_head(argc, argv);
 	else if (!strcmp(argv[0], "set-branches"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index e8af615..d9070cd 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -77,6 +77,15 @@ test_expect_success 'add another remote' '
 )
 '
 
+test_expect_success 'add first remote sets default' '
+(
+	test_create_repo default-first-remote &&
+	cd default-first-remote &&
+	git remote add first /path/to/first &&
+	test "$(git config --get remote.default)" = first
+)
+'
+
 test_expect_success 'remote forces tracking branches' '
 (
 	cd test &&
@@ -107,6 +116,16 @@ test_expect_success 'remove remote' '
 )
 '
 
+test_expect_success 'remove default remote' '
+(
+	git clone one no-default
+	cd no-default &&
+	test "$(git config --get remote.default)" = origin   # Sanity check
+	git remote rm origin &&
+	test_must_fail git config --get remote.default
+)
+'
+
 test_expect_success 'remove remote protects local branches' '
 (
 	cd test &&
@@ -662,6 +681,16 @@ test_expect_success 'rename a remote with name prefix of other remote' '
 
 '
 
+test_expect_success 'rename the default remote' '
+	git clone one default-rename &&
+	(cd default-rename &&
+		test "$(git config --get remote.default)" = origin   # Sanity?
+		git remote rename origin tyrion
+		test "$(git config --get remote.default)" = tyrion
+	)
+
+'
+
 cat > remotes_origin << EOF
 URL: $(pwd)/one
 Push: refs/heads/master:refs/heads/upstream
@@ -997,4 +1026,39 @@ test_expect_success 'remote set-url --delete baz' '
 	cmp expect actual
 '
 
+test_expect_success 'remote default' '
+(
+	git clone -o up one default
+	cd default &&
+	test "$(git remote default)" = up
+)
+'
+
+test_expect_success 'remote default (none)' '
+(
+	test_create_repo default-none &&
+	cd default-none &&
+	test "$(git remote default)" = origin
+)
+'
+
+test_expect_success 'remote default set' '
+(
+	test_create_repo default-set &&
+	cd default-set &&
+	git remote add A /foo-A &&
+	git remote add B /foo-A &&
+	git remote default B &&
+	test "$(git remote default)" = B
+)
+'
+
+test_expect_success 'remote default bad' '
+(
+	git clone one default-bad &&
+	cd default-bad &&
+	test_must_fail git remote default NonExistant
+)
+'
+
 test_done
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 6764d51..853b045 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -40,7 +40,13 @@ test_expect_success 'ls-remote self' '
 '
 
 test_expect_success 'dies when no remote specified and no default remotes found' '
-	test_must_fail git ls-remote
+	test_create_repo no-remotes &&
+	(
+		cd no-remotes &&
+		test_must_fail git ls-remote &&
+		test_config remote.default blop &&
+		test_must_fail git ls-remote
+	)
 '
 
 test_expect_success 'use "origin" when no remote specified' '
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 4736da8..6a08998 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -71,10 +71,10 @@ test_expect_success '"upstream" does not push when remotes do not match' '
 	test_must_fail git push parent2
 '
 
-test_expect_success 'push from/to new branch with upstream, matching and simple' '
+test_expect_success 'push from/to new remoteless branch with upstream, matching and simple' '
 	git checkout -b new-branch &&
 	test_push_failure simple &&
-	test_push_failure matching &&
+	test_push_success matching master &&
 	test_push_failure upstream
 '
 
-- 
1.7.11.1

[PATCH 5/6] Test that plain "git fetch" uses remote.default when on a detached HEAD.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 t/t5510-fetch.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index d7a19a1..8ecd996 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -69,6 +69,23 @@ test_expect_success "fetch test" '
 	test "z$mine" = "z$his"
 '
 
+test_expect_success "fetch test detached HEAD uses default remote" '
+	cd "$D" &&
+	git clone -o foo . default-remote &&
+	git checkout -b detached &&
+	test_commit update1 &&
+	(
+		cd default-remote &&
+		git checkout HEAD@{0} &&
+		git fetch &&
+		test -f .git/refs/remotes/foo/detached &&
+		mine=`git rev-parse refs/remotes/foo/detached` &&
+		his=`cd .. && git rev-parse refs/heads/detached` &&
+		test "z$mine" = "z$his"
+	) &&
+	git checkout master
+'
+
 test_expect_success "fetch test for-merge" '
 	cd "$D" &&
 	cd three &&
-- 
1.7.11.1

[PATCH 6/6] Teach get_default_remote to respect remote.default.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

Use "git remote default" instead of replicating its logic.

The unit test checks a relative-path submodule because the submodule code
is (almost) the only thing that uses get_default_remote.

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 git-parse-remote.sh        |  5 +----
 t/t7400-submodule-basic.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 484b2e6..49257f0 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -5,10 +5,7 @@
 GIT_DIR=$(git rev-parse -q --git-dir) || :;
 
 get_default_remote () {
-	curr_branch=$(git symbolic-ref -q HEAD)
-	curr_branch="${curr_branch#refs/heads/}"
-	origin=$(git config --get "branch.$curr_branch.remote")
-	echo ${origin:-origin}
+	echo $(git remote default)
 }
 
 get_remote_merge_branch () {
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 81827e6..2ac2ffc 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -507,6 +507,27 @@ test_expect_success 'relative path works with user@host:path' '
 	)
 '
 
+test_expect_success 'realtive path works when superproject on detached HEAD' '
+	(
+		test_create_repo detach &&
+		cd detach &&
+		test_create_repo sub &&
+		(
+			cd sub &&
+			test_commit foo
+		) &&
+		git add sub &&
+		git commit -m "added sub" &&
+		rm -rf sub &&
+		git checkout HEAD@{0} &&
+		git config -f .gitmodules submodule.sub.path sub &&
+		git config -f .gitmodules submodule.sub.url ../subrepo &&
+		git remote add awkward /path/to/awkward
+		git submodule init sub &&
+		test "$(git config submodule.sub.url)" = /path/to/subrepo
+	)
+'
+
 test_expect_success 'moving the superproject does not break submodules' '
 	(
 		cd addtest &&
-- 
1.7.11.1

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Junio C Hamano]

marcnarc@xiplink.com writes:

> From: Marc Branchaud <marcnarc@xiplink.com>
>
> The code now has a default_remote_name and an effective_remote_name:
>  - default_remote_name is set by remote.default in the config, or is "origin"
>    if remote.default doesn't exist ("origin" was the fallback value before
>    this change).


>  - effective_remote_name is the name of the remote tracked by the current
>    branch, or is default_remote_name if the current branch doesn't have a
>    remote.

The explanation of the latter belongs to the previous step, I think.
I am not sure if "effective" is the best name for the concept the
above explains, though.

> @@ -390,6 +391,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>  	}
>  	if (prefixcmp(key,  "remote."))
>  		return 0;
> +

Why?

>  	name = key + 7;
> @@ -671,6 +680,18 @@ static int valid_remote_nick(const char *name)
>  	return !strchr(name, '/'); /* no slash */
>  }
>  
> +const char *remote_get_default_name()

const char *remote_get_default_name(void)

> +{
> +	read_config();
> +	return default_remote_name;
> +}

Hrmph.  I am too lazy to read outside the context of your patch to
make sure, but isn't the root cause of the problem that when we try
to find which remote the current branch is configured to interact
with, we grab branch->remote_name (and this is done by calling
git_config() to open and read the configuration file once already)
and if it is empty we default to "origin"?  Wouldn't the callback
function that is used for that invocation of git_config() a much
better place to set "default_remote_name" variable, instead of
having us to read the entire configuration file one more time only
to get the value of this variable?

> +int remote_count()

int remote_count(void)

> +{
> +	read_config();
> +	return remotes_nr;
> +}

Likewise.  Especially it is unclear who benefits from the function
until a new caller is introduced.  I would prefer not to see the
addition of this function in this patch.

>  struct remote *remote_get(const char *name)
>  {
>  	struct remote *ret;
> diff --git a/remote.h b/remote.h
> index 251d8fd..f9aac87 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -52,6 +52,8 @@ struct remote {
>  
>  struct remote *remote_get(const char *name);
>  int remote_is_configured(const char *name);
> +const char *remote_get_default_name();
> +int remote_count();

const char *remote_get_default_name(void);
int remote_count(void);

Re: [PATCH 3/6] Teach clone to set remote.default.

[Junio C Hamano]

marcnarc@xiplink.com writes:

> From: Marc Branchaud <marcnarc@xiplink.com>
>
> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
> ---
>  builtin/clone.c          |  2 ++
>  t/t5601-clone.sh         | 10 ++++++++++
>  t/t5702-clone-options.sh |  7 +++++--
>  3 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index a4d8d25..b198456 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -770,6 +770,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	git_config_set(key.buf, repo);
>  	strbuf_reset(&key);
>  
> +	git_config_set("remote.default", option_origin);
> +

Is this something we would want to do unconditionally?  If so why?

Or is this what we want to do only when the "--origin name" option
is used?

Re: [PATCH 4/6] Teach "git remote" about remote.default.

[Phil Hord]

On Thu, Jul 5, 2012 at 6:11 PM,  <marcnarc@xiplink.com> wrote:
> From: Marc Branchaud <marcnarc@xiplink.com>
>
> The "rename" and "rm" commands now handle the case where the remote being
> changed is the default remote.

I think this is the right thing to do.  But I noticed a subtle
behavior change that we may wish to consider.

Today I might do this (contrived example):

git checkout somelocalbranch
git push # pushes to "origin" by default
git remote rename origin origin1
git add origin ssh://new-server/foo
git push  # pushes to "origin" by default

But after this change, the last command is different.  Now it pushes
to "origin1" because the rename set the remote.default to "origin1",
even though it was previously not set at all.  It did this because the
"oldname" is compared to the "remote_get_default_name()", which
returns "origin" by default.  So the old setting, which did not exist,
is now "renamed" to have an actual value, and the actual value is not
"origin".

One can easily contrive an alternative example showing that this is a
good thing.  As I said, I think it is the right thing to do.

But it is different, I think.

I doubt many script writers are counting on default settings to carry
the day, so they are probably more explicit about how they push.  But
I didn't see this mentioned in the patch.

Phil

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Marc Branchaud]

On 12-07-05 06:50 PM, Junio C Hamano wrote:
> marcnarc@xiplink.com writes:
> 
>> From: Marc Branchaud <marcnarc@xiplink.com>
>>
>> The code now has a default_remote_name and an effective_remote_name:
>>  - default_remote_name is set by remote.default in the config, or is "origin"
>>    if remote.default doesn't exist ("origin" was the fallback value before
>>    this change).
> 
> 
>>  - effective_remote_name is the name of the remote tracked by the current
>>    branch, or is default_remote_name if the current branch doesn't have a
>>    remote.
> 
> The explanation of the latter belongs to the previous step, I think.
> I am not sure if "effective" is the best name for the concept the
> above explains, though.

Well, the previous commit removes default_remote_name, so the explanation
wouldn't be valid verbatim.

How about keeping the above here, and I could add the following to the
previous commit's message:

	effective_remote_name is the remote name that is currently "in
	effect".  This is the currently checked-out branch's remote, or
	"origin" if the branch has no remote (or the working tree is a
	detached HEAD).

>> @@ -390,6 +391,7 @@ static int handle_config(const char *key, const char *value, void *cb)
>>  	}
>>  	if (prefixcmp(key,  "remote."))
>>  		return 0;
>> +
> 
> Why?

Oops, just a newline I neglected to clean up from some earlier hacking.  Sorry.

>>  	name = key + 7;
>> @@ -671,6 +680,18 @@ static int valid_remote_nick(const char *name)
>>  	return !strchr(name, '/'); /* no slash */
>>  }
>>  
>> +const char *remote_get_default_name()
> 
> const char *remote_get_default_name(void)

OK.

>> +{
>> +	read_config();
>> +	return default_remote_name;
>> +}
> 
> Hrmph.  I am too lazy to read outside the context of your patch to
> make sure, but isn't the root cause of the problem that when we try
> to find which remote the current branch is configured to interact
> with, we grab branch->remote_name (and this is done by calling
> git_config() to open and read the configuration file once already)
> and if it is empty we default to "origin"?  Wouldn't the callback
> function that is used for that invocation of git_config() a much
> better place to set "default_remote_name" variable, instead of
> having us to read the entire configuration file one more time only
> to get the value of this variable?

The read_config() function already has logic to avoid re-parsing the entire
config over and over again.  There are many places in remote.c that call
read_config(), and I thought I was just following that pattern.

Also, making the code be

	if (!default_remote_name)
		read_config();
	return default_remote_name;

would just replicate the check that read_config() already does.

>> +int remote_count()
> 
> int remote_count(void)

OK.

>> +{
>> +	read_config();
>> +	return remotes_nr;
>> +}
> 
> Likewise.

Doing something like

	if (!remotes_nr)
		read_config():
	return remotes_nr;

would be wrong, since having 0 remotes is perfectly fine.  And making the
check here be
	if (!default_remote_name)
seems confusing, and again it duplicates the check read_config() already does.

> Especially it is unclear who benefits from the function
> until a new caller is introduced.  I would prefer not to see the
> addition of this function in this patch.

OK, I can move it to the "git remote" patch.  The same could be said of
remote_get_default_name() though.

>>  struct remote *remote_get(const char *name)
>>  {
>>  	struct remote *ret;
>> diff --git a/remote.h b/remote.h
>> index 251d8fd..f9aac87 100644
>> --- a/remote.h
>> +++ b/remote.h
>> @@ -52,6 +52,8 @@ struct remote {
>>  
>>  struct remote *remote_get(const char *name);
>>  int remote_is_configured(const char *name);
>> +const char *remote_get_default_name();
>> +int remote_count();
> 
> const char *remote_get_default_name(void);
> int remote_count(void);

Got it.

I'll make these changes in a few days, after folks have had a chance to
review.  If things settle down I'll re-roll with the documentation updates too.

		M.

Re: [PATCH 3/6] Teach clone to set remote.default.

[Marc Branchaud]

On 12-07-05 06:52 PM, Junio C Hamano wrote:
> marcnarc@xiplink.com writes:
> 
>> From: Marc Branchaud <marcnarc@xiplink.com>
>>
>> Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
>> ---
>>  builtin/clone.c          |  2 ++
>>  t/t5601-clone.sh         | 10 ++++++++++
>>  t/t5702-clone-options.sh |  7 +++++--
>>  3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index a4d8d25..b198456 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -770,6 +770,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>  	git_config_set(key.buf, repo);
>>  	strbuf_reset(&key);
>>  
>> +	git_config_set("remote.default", option_origin);
>> +
> 
> Is this something we would want to do unconditionally?  If so why?

I think so, yes.

> Or is this what we want to do only when the "--origin name" option
> is used?

If remote.default isn't set, then if someone does
		git remote rename origin foo
the default remote will still be "origin" (modulo the currently-checked-out
branch stuff).

		M.

Re: [PATCH 4/6] Teach "git remote" about remote.default.

[Marc Branchaud]

On 12-07-06 08:51 AM, Phil Hord wrote:
> On Thu, Jul 5, 2012 at 6:11 PM,  <marcnarc@xiplink.com> wrote:
>> From: Marc Branchaud <marcnarc@xiplink.com>
>>
>> The "rename" and "rm" commands now handle the case where the remote being
>> changed is the default remote.
> 
> I think this is the right thing to do.  But I noticed a subtle
> behavior change that we may wish to consider.
> 
> Today I might do this (contrived example):
> 
> git checkout somelocalbranch
> git push # pushes to "origin" by default
> git remote rename origin origin1
> git add origin ssh://new-server/foo
> git push  # pushes to "origin" by default
> 
> But after this change, the last command is different.  Now it pushes
> to "origin1" because the rename set the remote.default to "origin1",
> even though it was previously not set at all.  It did this because the
> "oldname" is compared to the "remote_get_default_name()", which
> returns "origin" by default.  So the old setting, which did not exist,
> is now "renamed" to have an actual value, and the actual value is not
> "origin".
> 
> One can easily contrive an alternative example showing that this is a
> good thing.  As I said, I think it is the right thing to do.
> 
> But it is different, I think.

Yes, I agree it is different.

> I doubt many script writers are counting on default settings to carry
> the day, so they are probably more explicit about how they push.  But
> I didn't see this mentioned in the patch.

I think this sort of thing is better suited to the documentation of
remote.default.  I'm planning to re-roll this series with documentation
updates, and I'll include your example.

		M.

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> On 12-07-05 06:50 PM, Junio C Hamano wrote:
>> 
>>>  - effective_remote_name is the name of the remote tracked by the current
>>>    branch, or is default_remote_name if the current branch doesn't have a
>>>    remote.
>> 
>> The explanation of the latter belongs to the previous step, I think.
>> I am not sure if "effective" is the best name for the concept the
>> above explains, though.
>
> Well, the previous commit removes default_remote_name, so the explanation
> wouldn't be valid verbatim.

The previous one introduces "effective" (which I still think is not
the best word for the semantics you are trying to give to the
variable) without explaining what the variable is for and justifying
why "effective" is the right word (or at least a better than
"default") for it.  Something like the "- effective_remote_name is the ..."
above is necessary in its commit log message.

> How about keeping the above here, and I could add the following to the
> previous commit's message:
>
> 	effective_remote_name is the remote name that is currently "in
> 	effect".  This is the currently checked-out branch's remote, or
> 	"origin" if the branch has no remote (or the working tree is a
> 	detached HEAD).

Yeah, along that line.

> The read_config() function already has logic to avoid re-parsing the entire
> config over and over again.  There are many places in remote.c that call
> read_config(), and I thought I was just following that pattern.

OK.

Re: [PATCH 3/6] Teach clone to set remote.default.

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> If remote.default isn't set, then if someone does
> 		git remote rename origin foo
> the default remote will still be "origin" (modulo the currently-checked-out
> branch stuff).

Why?  I thought the proposed semantics was "if remote.default is
unset, the default value of 'origin' is used where remote.default
would have been used _everywhere_".  If "remote rename" wants to
update the value of remote.default from 'origin' to 'foo' (which may
or may not be the right thing to do, for which a separate discussion
seems to exist already), and if it sees that the repository does not
have remote.default, shouldn't it still set it to 'foo', just like
the case where remote.default exists and set to 'origin'?

Your updated "remote rename" must work correctly in a repository
that was created long ago, where remote.default was not set to
anything (and default 'origin' was used) after all.

Or am I missing some subtle issues?

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Marc Branchaud]

On 12-07-06 03:31 PM, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> On 12-07-05 06:50 PM, Junio C Hamano wrote:
>>>
>>>>  - effective_remote_name is the name of the remote tracked by the current
>>>>    branch, or is default_remote_name if the current branch doesn't have a
>>>>    remote.
>>>
>>> The explanation of the latter belongs to the previous step, I think.
>>> I am not sure if "effective" is the best name for the concept the
>>> above explains, though.
>>
>> Well, the previous commit removes default_remote_name, so the explanation
>> wouldn't be valid verbatim.
> 
> The previous one introduces "effective" (which I still think is not
> the best word for the semantics you are trying to give to the
> variable)

I'm open to suggestions.

		M.

Re: [PATCH 3/6] Teach clone to set remote.default.

[Marc Branchaud]

On 12-07-06 03:39 PM, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> If remote.default isn't set, then if someone does
>> 		git remote rename origin foo
>> the default remote will still be "origin" (modulo the currently-checked-out
>> branch stuff).
> 
> Why?

Erm, actually, my statement is incorrect.  Doh!

> I thought the proposed semantics was "if remote.default is
> unset, the default value of 'origin' is used where remote.default
> would have been used _everywhere_".

Yes, true.

> If "remote rename" wants to
> update the value of remote.default from 'origin' to 'foo' (which may
> or may not be the right thing to do, for which a separate discussion
> seems to exist already),

Are you talking about the sub-thread Phil Hord & I spawned about patch #4?  I
think Phil & I are in agreement there that it is the right thing to do.  If
anyone disagrees please speak up!

> and if it sees that the repository does not
> have remote.default, shouldn't it still set it to 'foo', just like
> the case where remote.default exists and set to 'origin'?

The proposed code actually already does that.  I'll add a unit test for this
case.

So why change "git clone" to always set remote.default if the functionality
remains the same either way?

To me it makes a more consistent implementation.  Since "git remote add" sets
remote.default if it's adding the first remote to the repository, when clone
itself adds the first remote it should do the same.

Plus this approach makes "clone -o" also work without any special-casing, so
the code is cleaner, IMHO.

If this justification is adequate, I'll add it to the commit message.  It may
then make more sense to have this commit come after the "git remote" changes
in the series.

> Your updated "remote rename" must work correctly in a repository
> that was created long ago, where remote.default was not set to
> anything (and default 'origin' was used) after all.
> 
> Or am I missing some subtle issues?

I agree with that requirement, and believe the proposed code fulfils it.

		M.

Re: [PATCH 3/6] Teach clone to set remote.default.

[Marc Branchaud]

On 12-07-06 04:43 PM, Marc Branchaud wrote:
> 
> So why change "git clone" to always set remote.default if the functionality
> remains the same either way?
> 
> To me it makes a more consistent implementation.  Since "git remote add" sets
> remote.default if it's adding the first remote to the repository, when clone
> itself adds the first remote it should do the same.
> 
> Plus this approach makes "clone -o" also work without any special-casing, so
> the code is cleaner, IMHO.

Also, it means that

	git clone /some/repo

and

	git clone -o origin /some/repo

produce exactly the same result.

		M.

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

>> Is it even a _side effect_?  Isn't this one of the primary points of
>> the series?  I do not think this patch makes sense if we did not
>> want that change to happen.
>> 
>> Or am I missing something?
>
> No, you're not -- I agree that this change in behaviour makes sense.  I
> simply hadn't anticipated this effect when I first did the work.
>
> So should the commit message simply say "This changes the default behavior of
> 'git push' ..."?  Or are you suggesting the message needn't mention it at all?

It is one of the more important behaviour changes we are making on
purpose with this series; it deserves to be described in the log, I
would think.

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Marc Branchaud]

On 12-07-11 06:04 PM, Junio C Hamano wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>> On 12-07-11 02:21 PM, Junio C Hamano wrote:
>>> marcnarc@xiplink.com writes:
>>>
>>>> From: Marc Branchaud <marcnarc@xiplink.com>
>>>>
>>>> The code now has a default_remote_name and an effective_remote_name:
>>>>
>>>>  - default_remote_name is set by remote.default in the config, or is "origin"
>>>>    if remote.default doesn't exist ("origin" was the fallback value before
>>>>    this change).
>>>>
>>>>  - effective_remote_name is the name of the remote tracked by the current
>>>>    branch, or is default_remote_name if the current branch doesn't have a
>>>>    remote.
>>>>
>>>> This has a minor side effect on the default behavior of "git push"....
>>>
>>> Hrm, is this a _minor_ side effect?
>>
>> Well, I thought so...  :)
>>
>> It's minor because of what you say:
>>
>>> Isn't that a natural and direct consequence of "now you can set
>>> remote.default configuration to name the remote you want to use in
>>> place of traditional 'origin'" feature?  I think changing the
>>> behaviour of "git push" in such a way is the point (may not be the
>>> only but one of the important points) of this series.
>>
>> I agree.  Phil Hord pointed out the change in behaviour and felt the commit
>> message should explain it.
>>
>> Should I just remove "minor"?
> 
> Is it even a _side effect_?  Isn't this one of the primary points of
> the series?  I do not think this patch makes sense if we did not
> want that change to happen.
> 
> Or am I missing something?

No, you're not -- I agree that this change in behaviour makes sense.  I
simply hadn't anticipated this effect when I first did the work.

So should the commit message simply say "This changes the default behavior of
'git push' ..."?  Or are you suggesting the message needn't mention it at all?

>>>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>>>> index cb97cc1..fc17d39 100644
>>>> --- a/Documentation/git-push.txt
>>>> +++ b/Documentation/git-push.txt
>>>> @@ -27,10 +27,16 @@ documentation for linkgit:git-receive-pack[1].
>>>>  OPTIONS[[OPTIONS]]
>>>>  ------------------
>>>>  <repository>::
>>>> -	The "remote" repository that is destination of a push
>>>> +	The "remote" repository that is the destination of the push
>>>>  	operation.  This parameter can be either a URL
>>>>  	(see the section <<URLS,GIT URLS>> below) or the name
>>>>  	of a remote (see the section <<REMOTES,REMOTES>> below).
>>>> +	If this parameter is omitted, git tries to use the remote
>>>> +	associated with the currently checked-out branch.  If there
>>>> +	is no remote associated with the current branch, git uses
>>>> +	the remote named by the "remote.default" configuration variable.
>>>> +	If "remote.default" is not set, git uses the name "origin" even
>>>> +	if there is no actual remote named "origin".
>>>
>>> This comment applies to other changes to the documentation in this
>>> patch I didn't quote, but I think the phrasing 'even if there is no
>>> acutual remote named "origin"' needs to be rethought, because "we
>>> use it even if there is no such remote determined with this logic"
>>> applies equally to branch.$name.remote, remote.default or built-in
>>> default value of remote.default which happens to be "origin".
>>
>> I'm sorry, but I'm having trouble understanding what you mean.  I don't see
>> how the "because ..." part of your sentence suggests what aspect needs
>> rephrasing.
> 
> You say the remote is taken from three places (branch.$name.remote,
> remote.default, or 'origin').
> 
> Imagine there is remote.origin.url at all.  If remote.default is set
> to 'origin', or branch.$name.remote for the current branch is set to
> 'origin', the repository you will try to use is 'origin'.  And you
> will fail the same way if you try to push there.  It does not matter
> if the hardcoded default gave you 'origin' or configuration variable
> gave it to you.  The name is used regardless, even if there is no
> actual remote under such name.
> 
> So "If 'remote.default' is not set, git uses the name "origin" even
> if there is no actual remote", while is not untrue, is misleading.
> Even if there is no actual remote 'origin', if 'remote.default' is
> set to 'origin', git will use that name, and will fail the same way.
> 
> Just saying "If 'remote.default' is not set, git uses 'origin'" or
> even "'remote.default', if unset, defaults to 'origin'" would be
> sufficient.

Thanks, I understand you now.

I feel it's rather surprising that git defaults to "origin".  I think the
docs should explain that git in fact ignores whatever remotes are actually
configured in the repository, that git only looks in specific places for a
remote name.  That's what I was trying to convey with the "even if" clause.

I'll try to rephrase.

		M.

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> On 12-07-11 02:21 PM, Junio C Hamano wrote:
>> marcnarc@xiplink.com writes:
>> 
>>> From: Marc Branchaud <marcnarc@xiplink.com>
>>>
>>> The code now has a default_remote_name and an effective_remote_name:
>>>
>>>  - default_remote_name is set by remote.default in the config, or is "origin"
>>>    if remote.default doesn't exist ("origin" was the fallback value before
>>>    this change).
>>>
>>>  - effective_remote_name is the name of the remote tracked by the current
>>>    branch, or is default_remote_name if the current branch doesn't have a
>>>    remote.
>>>
>>> This has a minor side effect on the default behavior of "git push"....
>> 
>> Hrm, is this a _minor_ side effect?
>
> Well, I thought so...  :)
>
> It's minor because of what you say:
>
>> Isn't that a natural and direct consequence of "now you can set
>> remote.default configuration to name the remote you want to use in
>> place of traditional 'origin'" feature?  I think changing the
>> behaviour of "git push" in such a way is the point (may not be the
>> only but one of the important points) of this series.
>
> I agree.  Phil Hord pointed out the change in behaviour and felt the commit
> message should explain it.
>
> Should I just remove "minor"?

Is it even a _side effect_?  Isn't this one of the primary points of
the series?  I do not think this patch makes sense if we did not
want that change to happen.

Or am I missing something?

>>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>>> index cb97cc1..fc17d39 100644
>>> --- a/Documentation/git-push.txt
>>> +++ b/Documentation/git-push.txt
>>> @@ -27,10 +27,16 @@ documentation for linkgit:git-receive-pack[1].
>>>  OPTIONS[[OPTIONS]]
>>>  ------------------
>>>  <repository>::
>>> -	The "remote" repository that is destination of a push
>>> +	The "remote" repository that is the destination of the push
>>>  	operation.  This parameter can be either a URL
>>>  	(see the section <<URLS,GIT URLS>> below) or the name
>>>  	of a remote (see the section <<REMOTES,REMOTES>> below).
>>> +	If this parameter is omitted, git tries to use the remote
>>> +	associated with the currently checked-out branch.  If there
>>> +	is no remote associated with the current branch, git uses
>>> +	the remote named by the "remote.default" configuration variable.
>>> +	If "remote.default" is not set, git uses the name "origin" even
>>> +	if there is no actual remote named "origin".
>> 
>> This comment applies to other changes to the documentation in this
>> patch I didn't quote, but I think the phrasing 'even if there is no
>> acutual remote named "origin"' needs to be rethought, because "we
>> use it even if there is no such remote determined with this logic"
>> applies equally to branch.$name.remote, remote.default or built-in
>> default value of remote.default which happens to be "origin".
>
> I'm sorry, but I'm having trouble understanding what you mean.  I don't see
> how the "because ..." part of your sentence suggests what aspect needs
> rephrasing.

You say the remote is taken from three places (branch.$name.remote,
remote.default, or 'origin').

Imagine there is remote.origin.url at all.  If remote.default is set
to 'origin', or branch.$name.remote for the current branch is set to
'origin', the repository you will try to use is 'origin'.  And you
will fail the same way if you try to push there.  It does not matter
if the hardcoded default gave you 'origin' or configuration variable
gave it to you.  The name is used regardless, even if there is no
actual remote under such name.

So "If 'remote.default' is not set, git uses the name "origin" even
if there is no actual remote", while is not untrue, is misleading.
Even if there is no actual remote 'origin', if 'remote.default' is
set to 'origin', git will use that name, and will fail the same way.

Just saying "If 'remote.default' is not set, git uses 'origin'" or
even "'remote.default', if unset, defaults to 'origin'" would be
sufficient.

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Marc Branchaud]

On 12-07-11 02:21 PM, Junio C Hamano wrote:
> marcnarc@xiplink.com writes:
> 
>> From: Marc Branchaud <marcnarc@xiplink.com>
>>
>> The code now has a default_remote_name and an effective_remote_name:
>>
>>  - default_remote_name is set by remote.default in the config, or is "origin"
>>    if remote.default doesn't exist ("origin" was the fallback value before
>>    this change).
>>
>>  - effective_remote_name is the name of the remote tracked by the current
>>    branch, or is default_remote_name if the current branch doesn't have a
>>    remote.
>>
>> This has a minor side effect on the default behavior of "git push".  Consider
>> the following sequence of commands:
>>
>>       git config remote.default foo                 # Set default remote
>>       git checkout somelocalbranch                  # Does not track a remote
>>       git remote add origin ssh://example.com/repo  # Add a new "origin"
>>       git push
>>
>> Prior to this change, the above "git push" would attempt to push to the new
>> "origin" repository at ssh://example.com/repo.  Now instead that "git push"
>> will attempt to push to the repository named "foo".
> 
> Hrm, is this a _minor_ side effect?

Well, I thought so...  :)

It's minor because of what you say:

> Isn't that a natural and direct consequence of "now you can set
> remote.default configuration to name the remote you want to use in
> place of traditional 'origin'" feature?  I think changing the
> behaviour of "git push" in such a way is the point (may not be the
> only but one of the important points) of this series.

I agree.  Phil Hord pointed out the change in behaviour and felt the commit
message should explain it.

Should I just remove "minor"?

>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>> index cb97cc1..fc17d39 100644
>> --- a/Documentation/git-push.txt
>> +++ b/Documentation/git-push.txt
>> @@ -27,10 +27,16 @@ documentation for linkgit:git-receive-pack[1].
>>  OPTIONS[[OPTIONS]]
>>  ------------------
>>  <repository>::
>> -	The "remote" repository that is destination of a push
>> +	The "remote" repository that is the destination of the push
>>  	operation.  This parameter can be either a URL
>>  	(see the section <<URLS,GIT URLS>> below) or the name
>>  	of a remote (see the section <<REMOTES,REMOTES>> below).
>> +	If this parameter is omitted, git tries to use the remote
>> +	associated with the currently checked-out branch.  If there
>> +	is no remote associated with the current branch, git uses
>> +	the remote named by the "remote.default" configuration variable.
>> +	If "remote.default" is not set, git uses the name "origin" even
>> +	if there is no actual remote named "origin".
> 
> This comment applies to other changes to the documentation in this
> patch I didn't quote, but I think the phrasing 'even if there is no
> acutual remote named "origin"' needs to be rethought, because "we
> use it even if there is no such remote determined with this logic"
> applies equally to branch.$name.remote, remote.default or built-in
> default value of remote.default which happens to be "origin".

I'm sorry, but I'm having trouble understanding what you mean.  I don't see
how the "because ..." part of your sentence suggests what aspect needs
rephrasing.

(BTW, I'm under the impression that when git sets branch.$name.remote it
doesn't ever have to fall back to the default remote name.  That is, commands
that set the value always have the user explicitly, though perhaps
indirectly, specifying the remote.  Did I miss something?)

>> diff --git a/remote.c b/remote.c
>> index 6f371e0..2ebdbbd 100644
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -47,6 +47,7 @@ static int branches_alloc;
>>  static int branches_nr;
>>  
>>  static struct branch *current_branch;
>> +static const char *default_remote_name;
>>  static const char *effective_remote_name;
> 
> Coming from UNIX background, "effective" gives me an impression that
> it is contrasted with "real" (i.e. there is "real remote" for the
> branch, but during this particular invocation it is overridden and
> "effective remote" is used instead).  Perhaps it is just me.

I did have something like that in mind when I chose "effective":  There's a
default remote name that's normally used, but in the certain circumstances
it's overridden.  Perhaps "effective_default_remote_name" would be better,
though that's getting to be a mouthful (plus it would mean having an
"explicit_effective_default_remote_name", which IMHO is a bit much).

> Something along the line of remote-name-to-use, use-remote, might
> sound more natural.

current_remote_name?

		M.

Re: [PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[Junio C Hamano]

marcnarc@xiplink.com writes:

> From: Marc Branchaud <marcnarc@xiplink.com>
>
> The code now has a default_remote_name and an effective_remote_name:
>
>  - default_remote_name is set by remote.default in the config, or is "origin"
>    if remote.default doesn't exist ("origin" was the fallback value before
>    this change).
>
>  - effective_remote_name is the name of the remote tracked by the current
>    branch, or is default_remote_name if the current branch doesn't have a
>    remote.
>
> This has a minor side effect on the default behavior of "git push".  Consider
> the following sequence of commands:
>
>       git config remote.default foo                 # Set default remote
>       git checkout somelocalbranch                  # Does not track a remote
>       git remote add origin ssh://example.com/repo  # Add a new "origin"
>       git push
>
> Prior to this change, the above "git push" would attempt to push to the new
> "origin" repository at ssh://example.com/repo.  Now instead that "git push"
> will attempt to push to the repository named "foo".

Hrm, is this a _minor_ side effect?

Isn't that a natural and direct consequence of "now you can set
remote.default configuration to name the remote you want to use in
place of traditional 'origin'" feature?  I think changing the
behaviour of "git push" in such a way is the point (may not be the
only but one of the important points) of this series.

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index cb97cc1..fc17d39 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -27,10 +27,16 @@ documentation for linkgit:git-receive-pack[1].
>  OPTIONS[[OPTIONS]]
>  ------------------
>  <repository>::
> -	The "remote" repository that is destination of a push
> +	The "remote" repository that is the destination of the push
>  	operation.  This parameter can be either a URL
>  	(see the section <<URLS,GIT URLS>> below) or the name
>  	of a remote (see the section <<REMOTES,REMOTES>> below).
> +	If this parameter is omitted, git tries to use the remote
> +	associated with the currently checked-out branch.  If there
> +	is no remote associated with the current branch, git uses
> +	the remote named by the "remote.default" configuration variable.
> +	If "remote.default" is not set, git uses the name "origin" even
> +	if there is no actual remote named "origin".

This comment applies to other changes to the documentation in this
patch I didn't quote, but I think the phrasing 'even if there is no
acutual remote named "origin"' needs to be rethought, because "we
use it even if there is no such remote determined with this logic"
applies equally to branch.$name.remote, remote.default or built-in
default value of remote.default which happens to be "origin".

> diff --git a/remote.c b/remote.c
> index 6f371e0..2ebdbbd 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -47,6 +47,7 @@ static int branches_alloc;
>  static int branches_nr;
>  
>  static struct branch *current_branch;
> +static const char *default_remote_name;
>  static const char *effective_remote_name;

Coming from UNIX background, "effective" gives me an impression that
it is contrasted with "real" (i.e. there is "real remote" for the
branch, but during this particular invocation it is overridden and
"effective remote" is used instead).  Perhaps it is just me.

Something along the line of remote-name-to-use, use-remote, might
sound more natural.

[PATCH 2/6] Teach remote.c about the remote.default configuration setting.

[marcnarc]

From: Marc Branchaud <marcnarc@xiplink.com>

The code now has a default_remote_name and an effective_remote_name:

 - default_remote_name is set by remote.default in the config, or is "origin"
   if remote.default doesn't exist ("origin" was the fallback value before
   this change).

 - effective_remote_name is the name of the remote tracked by the current
   branch, or is default_remote_name if the current branch doesn't have a
   remote.

This has a minor side effect on the default behavior of "git push".  Consider
the following sequence of commands:

      git config remote.default foo                 # Set default remote
      git checkout somelocalbranch                  # Does not track a remote
      git remote add origin ssh://example.com/repo  # Add a new "origin"
      git push

Prior to this change, the above "git push" would attempt to push to the new
"origin" repository at ssh://example.com/repo.  Now instead that "git push"
will attempt to push to the repository named "foo".

Signed-off-by: Marc Branchaud <marcnarc@xiplink.com>
---
 Documentation/config.txt           |  8 ++++++++
 Documentation/git-pull.txt         |  6 +++++-
 Documentation/git-push.txt         |  8 +++++++-
 Documentation/pull-fetch-param.txt |  6 ++++++
 remote.c                           | 14 +++++++++++---
 5 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 915cb5a..7869e1b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1856,6 +1856,14 @@ remote.<name>.vcs::
 	Setting this to a value <vcs> will cause git to interact with
 	the remote with the git-remote-<vcs> helper.
 
+remote.default::
+	This value is the <name> of a remote.  When Git needs to automatically
+	choose a remote to use, it first tries the 'branch.<branchname>.remote'
+	value of the currently checked-out branch.  If the currently checked-out
+	branch has no remote, Git uses the remote named by 'remote.default', or
+	the remote named "origin" if no value is set (even if there is no
+	actual remote named "origin").
+
 remotes.<group>::
 	The list of remotes which are fetched by "git remote update
 	<group>".  See linkgit:git-remote[1].
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index defb544..2610253 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -33,7 +33,11 @@ but usually it is the name of a branch in the remote repository.
 
 Default values for <repository> and <branch> are read from the
 "remote" and "merge" configuration for the current branch
-as set by linkgit:git-branch[1] `--track`.
+as set by linkgit:git-branch[1] `--track`.  If the current branch
+has no remote configured, the default for <repository> is read
+from the "remote.default" configuration variable.  If that variable
+is not set, the default <repository> is "origin" even if there is no
+actual remote named "origin".
 
 Assume the following history exists and the current branch is
 "`master`":
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index cb97cc1..fc17d39 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -27,10 +27,16 @@ documentation for linkgit:git-receive-pack[1].
 OPTIONS[[OPTIONS]]
 ------------------
 <repository>::
-	The "remote" repository that is destination of a push
+	The "remote" repository that is the destination of the push
 	operation.  This parameter can be either a URL
 	(see the section <<URLS,GIT URLS>> below) or the name
 	of a remote (see the section <<REMOTES,REMOTES>> below).
+	If this parameter is omitted, git tries to use the remote
+	associated with the currently checked-out branch.  If there
+	is no remote associated with the current branch, git uses
+	the remote named by the "remote.default" configuration variable.
+	If "remote.default" is not set, git uses the name "origin" even
+	if there is no actual remote named "origin".
 
 <refspec>...::
 	The format of a <refspec> parameter is an optional plus
diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt
index 94a9d32..696f1fb 100644
--- a/Documentation/pull-fetch-param.txt
+++ b/Documentation/pull-fetch-param.txt
@@ -3,6 +3,12 @@
 	or pull operation.  This parameter can be either a URL
 	(see the section <<URLS,GIT URLS>> below) or the name
 	of a remote (see the section <<REMOTES,REMOTES>> below).
+	If this parameter is omitted, git tries to use the remote
+	associated with the currently checked-out branch.  If there
+	is no remote associated with the current branch, git uses
+	the remote named by the "remote.default" configuration variable.
+	If "remote.default" is not set, git uses the name "origin" even
+	if there is no actual remote named "origin".
 
 ifndef::git-pull[]
 <group>::
diff --git a/remote.c b/remote.c
index 6f371e0..2ebdbbd 100644
--- a/remote.c
+++ b/remote.c
@@ -47,6 +47,7 @@ static int branches_alloc;
 static int branches_nr;
 
 static struct branch *current_branch;
+static const char *default_remote_name;
 static const char *effective_remote_name;
 static int explicit_effective_remote_name;
 
@@ -397,8 +398,12 @@ static int handle_config(const char *key, const char *value, void *cb)
 		return 0;
 	}
 	subkey = strrchr(name, '.');
-	if (!subkey)
+	if (!subkey) {
+		/* Look for remote.default */
+		if (!strcmp(name, "default"))
+			default_remote_name = xstrdup(value);
 		return 0;
+	}
 	remote = make_remote(name, subkey - name);
 	remote->origin = REMOTE_CONFIG;
 	if (!strcmp(subkey, ".mirror"))
@@ -481,9 +486,8 @@ static void read_config(void)
 	unsigned char sha1[20];
 	const char *head_ref;
 	int flag;
-	if (effective_remote_name) /* did this already */
+	if (default_remote_name) /* did this already */
 		return;
-	effective_remote_name = xstrdup("origin");
 	current_branch = NULL;
 	head_ref = resolve_ref_unsafe("HEAD", sha1, 0, &flag);
 	if (head_ref && (flag & REF_ISSYMREF) &&
@@ -492,6 +496,10 @@ static void read_config(void)
 			make_branch(head_ref + strlen("refs/heads/"), 0);
 	}
 	git_config(handle_config, NULL);
+	if (!default_remote_name)
+		default_remote_name = "origin";
+	if (!effective_remote_name)
+		effective_remote_name = default_remote_name;
 	alias_all_urls();
 }
 
-- 
1.7.11.1