threads / patch / 41419

v2, 4 partsgit remote improvements

Subject: [PATCH v2 0/4] git remote improvements

## tl;dr

17 messages between Feb 15, 2016 and Feb 16, 2016. Diffs are folded; open one to read it.

replies: 16people: 4as markdown or json

Thomas Gummerer· Feb 15, 2016, 22:39 UTC · lore
Thanks Peff for the review on the previous round ($gmane/286214).

The series now uses parse_config_key() instead of skip_prefix, and I added REMOTE_UNCONFIGURED to the enum used in the origin field in struct remote.

Interdiff below:
Show changes to 2 files +35 −36

remote.c, remote.h

diff --git a/remote.c b/remote.c
index 3a4ca9b..d10ae00 100644
--- a/remote.c
+++ b/remote.c
@@ -318,90 +318,88 @@ static void read_branches_file(struct remote *remote)
 static int handle_config(const char *key, const char *value, void *cb)
 {
 	const char *name;
+	int namelen;
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
-	if (skip_prefix(key, "branch.", &name)) {
-		subkey = strrchr(name, '.');
-		if (!subkey)
+	if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
+		if (!name)
 			return 0;
-		branch = make_branch(name, subkey - name);
-		if (!strcmp(subkey, ".remote")) {
+		branch = make_branch(name, namelen);
+		if (!strcmp(subkey, "remote")) {
 			return git_config_string(&branch->remote_name, key, value);
-		} else if (!strcmp(subkey, ".pushremote")) {
+		} else if (!strcmp(subkey, "pushremote")) {
 			return git_config_string(&branch->pushremote_name, key, value);
-		} else if (!strcmp(subkey, ".merge")) {
+		} else if (!strcmp(subkey, "merge")) {
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
 		}
 		return 0;
 	}
-	if (skip_prefix(key, "url.", &name)) {
+	if (parse_config_key(key, "url", &name, &namelen, &subkey) >= 0) {
 		struct rewrite *rewrite;
-		subkey = strrchr(name, '.');
-		if (!subkey)
+		if (!name)
 			return 0;
-		if (!strcmp(subkey, ".insteadof")) {
-			rewrite = make_rewrite(&rewrites, name, subkey - name);
+		if (!strcmp(subkey, "insteadof")) {
+			rewrite = make_rewrite(&rewrites, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
 			add_instead_of(rewrite, xstrdup(value));
-		} else if (!strcmp(subkey, ".pushinsteadof")) {
-			rewrite = make_rewrite(&rewrites_push, name, subkey - name);
+		} else if (!strcmp(subkey, "pushinsteadof")) {
+			rewrite = make_rewrite(&rewrites_push, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
 
-	if (!skip_prefix(key, "remote.", &name))
+	if (parse_config_key(key, "remote", &name, &namelen, &subkey) < 0)
 		return 0;
 
 	/* Handle remote.* variables */
-	if (!strcmp(name, "pushdefault"))
+	if (!strcmp(subkey, "pushdefault"))
 		return git_config_string(&pushremote_name, key, value);
 
 	/* Handle remote.<name>.* variables */
-	if (*name == '/') {
+	if (*(name ? name : subkey) == '/') {
 		warning("Config remote shorthand cannot begin with '/': %s",
-			name);
+			name ? name : subkey);
 		return 0;
 	}
-	subkey = strrchr(name, '.');
-	if (!subkey)
+	if (!name)
 		return 0;
-	remote = make_remote(name, subkey - name);
+	remote = make_remote(name, namelen);
 	remote->origin = REMOTE_CONFIG;
-	if (!strcmp(subkey, ".mirror"))
+	if (!strcmp(subkey, "mirror"))
 		remote->mirror = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".skipdefaultupdate"))
+	else if (!strcmp(subkey, "skipdefaultupdate"))
 		remote->skip_default_update = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".skipfetchall"))
+	else if (!strcmp(subkey, "skipfetchall"))
 		remote->skip_default_update = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".prune"))
+	else if (!strcmp(subkey, "prune"))
 		remote->prune = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".url")) {
+	else if (!strcmp(subkey, "url")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_url(remote, v);
-	} else if (!strcmp(subkey, ".pushurl")) {
+	} else if (!strcmp(subkey, "pushurl")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_pushurl(remote, v);
-	} else if (!strcmp(subkey, ".push")) {
+	} else if (!strcmp(subkey, "push")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_push_refspec(remote, v);
-	} else if (!strcmp(subkey, ".fetch")) {
+	} else if (!strcmp(subkey, "fetch")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_fetch_refspec(remote, v);
-	} else if (!strcmp(subkey, ".receivepack")) {
+	} else if (!strcmp(subkey, "receivepack")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
@@ -409,7 +407,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->receivepack = v;
 		else
 			error("more than one receivepack given, using the first");
-	} else if (!strcmp(subkey, ".uploadpack")) {
+	} else if (!strcmp(subkey, "uploadpack")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
@@ -417,18 +415,18 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->uploadpack = v;
 		else
 			error("more than one uploadpack given, using the first");
-	} else if (!strcmp(subkey, ".tagopt")) {
+	} else if (!strcmp(subkey, "tagopt")) {
 		if (!strcmp(value, "--no-tags"))
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
-	} else if (!strcmp(subkey, ".proxy")) {
+	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-	} else if (!strcmp(subkey, ".proxyauthmethod")) {
+	} else if (!strcmp(subkey, "proxyauthmethod")) {
 		return git_config_string((const char **)&remote->http_proxy_authmethod,
 					 key, value);
-	} else if (!strcmp(subkey, ".vcs")) {
+	} else if (!strcmp(subkey, "vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
 	return 0;
diff --git a/remote.h b/remote.h
index 7a5ee77..c21fd37 100644
--- a/remote.h
+++ b/remote.h
@@ -5,7 +5,8 @@
 #include "hashmap.h"
 
 enum {
-	REMOTE_CONFIG = 1,
+	REMOTE_UNCONFIGURED = 0,
+	REMOTE_CONFIG,
 	REMOTE_REMOTES,
 	REMOTE_BRANCHES
 };

Thomas Gummerer (4):
  remote: use parse_config_key
  remote: simplify remote_is_configured()
  remote: actually check if remote exits
  remote: use remote_is_configured() for add and rename

 builtin/fetch.c   |  5 ++--
 builtin/remote.c  | 23 +++++++--------
 remote.c          | 84 +++++++++++++++++++++++--------------------------------
 remote.h          |  3 +-
 t/t5505-remote.sh | 36 ++++++++++++++++++++++++
 5 files changed, 85 insertions(+), 66 deletions(-)
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 15, 2016, 22:39 UTC · re: Thomas Gummerer · lore

[PATCH v2 1/4] remote: use parse_config_key

95b567c7 ("use skip_prefix to avoid repeating strings") transformed calls using starts_with() and then skipping the length of the prefix to skip_prefix() calls. In remote.c there are a few calls like:

  if (starts_with(foo, "bar"))
      foo += 3

These calls weren't touched by the aformentioned commit, but can be replaced by calls to parse_config_key(), to simplify the code and clarify the intentions. Do that.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 remote.c | 71 ++++++++++++++++++++++++++++++----------------------------------
 1 file changed, 33 insertions(+), 38 deletions(-)
Show changes to remote.c +33 −38
diff --git a/remote.c b/remote.c
index 02e698a..bcd8eb6 100644
--- a/remote.c
+++ b/remote.c
@@ -318,93 +318,88 @@ static void read_branches_file(struct remote *remote)
 static int handle_config(const char *key, const char *value, void *cb)
 {
 	const char *name;
+	int namelen;
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
-	if (starts_with(key, "branch.")) {
-		name = key + 7;
-		subkey = strrchr(name, '.');
-		if (!subkey)
+	if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
+		if (!name)
 			return 0;
-		branch = make_branch(name, subkey - name);
-		if (!strcmp(subkey, ".remote")) {
+		branch = make_branch(name, namelen);
+		if (!strcmp(subkey, "remote")) {
 			return git_config_string(&branch->remote_name, key, value);
-		} else if (!strcmp(subkey, ".pushremote")) {
+		} else if (!strcmp(subkey, "pushremote")) {
 			return git_config_string(&branch->pushremote_name, key, value);
-		} else if (!strcmp(subkey, ".merge")) {
+		} else if (!strcmp(subkey, "merge")) {
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
 		}
 		return 0;
 	}
-	if (starts_with(key, "url.")) {
+	if (parse_config_key(key, "url", &name, &namelen, &subkey) >= 0) {
 		struct rewrite *rewrite;
-		name = key + 4;
-		subkey = strrchr(name, '.');
-		if (!subkey)
+		if (!name)
 			return 0;
-		if (!strcmp(subkey, ".insteadof")) {
-			rewrite = make_rewrite(&rewrites, name, subkey - name);
+		if (!strcmp(subkey, "insteadof")) {
+			rewrite = make_rewrite(&rewrites, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
 			add_instead_of(rewrite, xstrdup(value));
-		} else if (!strcmp(subkey, ".pushinsteadof")) {
-			rewrite = make_rewrite(&rewrites_push, name, subkey - name);
+		} else if (!strcmp(subkey, "pushinsteadof")) {
+			rewrite = make_rewrite(&rewrites_push, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
 
-	if (!starts_with(key,  "remote."))
+	if (parse_config_key(key, "remote", &name, &namelen, &subkey) < 0)
 		return 0;
-	name = key + 7;
 
 	/* Handle remote.* variables */
-	if (!strcmp(name, "pushdefault"))
+	if (!strcmp(subkey, "pushdefault"))
 		return git_config_string(&pushremote_name, key, value);
 
 	/* Handle remote.<name>.* variables */
-	if (*name == '/') {
+	if (*(name ? name : subkey) == '/') {
 		warning("Config remote shorthand cannot begin with '/': %s",
-			name);
+			name ? name : subkey);
 		return 0;
 	}
-	subkey = strrchr(name, '.');
-	if (!subkey)
+	if (!name)
 		return 0;
-	remote = make_remote(name, subkey - name);
+	remote = make_remote(name, namelen);
 	remote->origin = REMOTE_CONFIG;
-	if (!strcmp(subkey, ".mirror"))
+	if (!strcmp(subkey, "mirror"))
 		remote->mirror = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".skipdefaultupdate"))
+	else if (!strcmp(subkey, "skipdefaultupdate"))
 		remote->skip_default_update = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".skipfetchall"))
+	else if (!strcmp(subkey, "skipfetchall"))
 		remote->skip_default_update = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".prune"))
+	else if (!strcmp(subkey, "prune"))
 		remote->prune = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".url")) {
+	else if (!strcmp(subkey, "url")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_url(remote, v);
-	} else if (!strcmp(subkey, ".pushurl")) {
+	} else if (!strcmp(subkey, "pushurl")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_pushurl(remote, v);
-	} else if (!strcmp(subkey, ".push")) {
+	} else if (!strcmp(subkey, "push")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_push_refspec(remote, v);
-	} else if (!strcmp(subkey, ".fetch")) {
+	} else if (!strcmp(subkey, "fetch")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_fetch_refspec(remote, v);
-	} else if (!strcmp(subkey, ".receivepack")) {
+	} else if (!strcmp(subkey, "receivepack")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
@@ -412,7 +407,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->receivepack = v;
 		else
 			error("more than one receivepack given, using the first");
-	} else if (!strcmp(subkey, ".uploadpack")) {
+	} else if (!strcmp(subkey, "uploadpack")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
@@ -420,18 +415,18 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->uploadpack = v;
 		else
 			error("more than one uploadpack given, using the first");
-	} else if (!strcmp(subkey, ".tagopt")) {
+	} else if (!strcmp(subkey, "tagopt")) {
 		if (!strcmp(value, "--no-tags"))
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
-	} else if (!strcmp(subkey, ".proxy")) {
+	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-	} else if (!strcmp(subkey, ".proxyauthmethod")) {
+	} else if (!strcmp(subkey, "proxyauthmethod")) {
 		return git_config_string((const char **)&remote->http_proxy_authmethod,
 					 key, value);
-	} else if (!strcmp(subkey, ".vcs")) {
+	} else if (!strcmp(subkey, "vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
 	return 0;
-- 
2.7.1.410.g6faf27b
Jeff King· Feb 15, 2016, 23:04 UTC · re: Thomas Gummerer · lore

Re: [PATCH v2 1/4] remote: use parse_config_key

On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:
Show 9 quoted lines
> -	if (!starts_with(key,  "remote."))
> +	if (parse_config_key(key, "remote", &name, &namelen, &subkey) < 0)
>  		return 0;
> -	name = key + 7;
>  
>  	/* Handle remote.* variables */
> -	if (!strcmp(name, "pushdefault"))
> +	if (!strcmp(subkey, "pushdefault"))
>  		return git_config_string(&pushremote_name, key, value);
I think this needs to become:
  if (!name && !strcmp(subkey, "pushdefault"))

so that we do not match "remote.foo.pushdefault", which is nonsense. The original avoided it by conflating "name" and "subkey" at various points, and not parsing out the subkey until later. Making that more explicit is one of the things that I think is improved by your patch. :)

Show 12 quoted lines
>  	/* Handle remote.<name>.* variables */
> -	if (*name == '/') {
> +	if (*(name ? name : subkey) == '/') {
>  		warning("Config remote shorthand cannot begin with '/': %s",
> -			name);
> +			name ? name : subkey);
>  		return 0;
>  	}
> -	subkey = strrchr(name, '.');
> -	if (!subkey)
> +	if (!name)
>  		return 0;

I think you can bump the "if (!name)" check earlier. If it is empty, we know that it does not start with "/". And then you can avoid the extra NULL-checks.

The rest of the patch looks good to me. I hadn't realized initially that all of the subkey compares would become "foo" and not ".foo". That makes the diff noisier, but IMHO the result is much better.

-Peff
Thomas Gummerer· Feb 16, 2016, 00:13 UTC · re: Jeff King · lore

Re: [PATCH v2 1/4] remote: use parse_config_key

On 02/15, Jeff King wrote:
Show 20 quoted lines
> On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:
>
> > -	if (!starts_with(key,  "remote."))
> > +	if (parse_config_key(key, "remote", &name, &namelen, &subkey) < 0)
> >  		return 0;
> > -	name = key + 7;
> >
> >  	/* Handle remote.* variables */
> > -	if (!strcmp(name, "pushdefault"))
> > +	if (!strcmp(subkey, "pushdefault"))
> >  		return git_config_string(&pushremote_name, key, value);
>
> I think this needs to become:
>
>   if (!name && !strcmp(subkey, "pushdefault"))
>
> so that we do not match "remote.foo.pushdefault", which is nonsense. The
> original avoided it by conflating "name" and "subkey" at various points,
> and not parsing out the subkey until later. Making that more explicit is
> one of the things that I think is improved by your patch. :)
Good catch.  I'll fix this in the re-roll.
Show 16 quoted lines
> >  	/* Handle remote.<name>.* variables */
> > -	if (*name == '/') {
> > +	if (*(name ? name : subkey) == '/') {
> >  		warning("Config remote shorthand cannot begin with '/': %s",
> > -			name);
> > +			name ? name : subkey);
> >  		return 0;
> >  	}
> > -	subkey = strrchr(name, '.');
> > -	if (!subkey)
> > +	if (!name)
> >  		return 0;
>
> I think you can bump the "if (!name)" check earlier. If it is empty, we
> know that it does not start with "/". And then you can avoid the extra
> NULL-checks.
Thanks, will change that.
Show 5 quoted lines
> The rest of the patch looks good to me. I hadn't realized initially that
> all of the subkey compares would become "foo" and not ".foo". That makes
> the diff noisier, but IMHO the result is much better.
>
> -Peff

-- Thomas

Thomas Gummerer· Feb 15, 2016, 22:39 UTC · re: Thomas Gummerer · lore

[PATCH v2 2/4] remote: simplify remote_is_configured()

The remote_is_configured() function allows checking whether a remote exists or not. The function however only works if remote_get() wasn't called before calling it. In addition, it only checks the configuration for remotes, but not remotes or branches files.

Make use of the origin member of struct remote instead, which indicates where the remote comes from. It will be set to some value if the remote is configured in any file in the repository, but is initialized to 0 if the remote is only created in make_remote().

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/fetch.c  |  5 ++---
 builtin/remote.c | 12 ++++++------
 remote.c         | 13 ++-----------
 remote.h         |  3 ++-
 4 files changed, 12 insertions(+), 21 deletions(-)
Show changes to 4 files +12 −21

builtin/fetch.c, builtin/remote.c, remote.c, remote.h

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..8121830 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1016,10 +1016,9 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 
 	git_config(get_remote_group, &g);
 	if (list->nr == prev_nr) {
-		struct remote *remote;
-		if (!remote_is_configured(name))
+		struct remote *remote = remote_get(name);
+		if (!remote_is_configured(remote))
 			return 0;
-		remote = remote_get(name);
 		string_list_append(list, remote->name);
 	}
 	return 1;
diff --git a/builtin/remote.c b/builtin/remote.c
index 2b2ff9b..d966262 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1441,9 +1441,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
 
 	strbuf_addf(&key, "remote.%s.fetch", remotename);
 
-	if (!remote_is_configured(remotename))
-		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
+	if (!remote_is_configured(remote))
+		die(_("No such remote '%s'"), remotename);
 
 	if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
 		strbuf_release(&key);
@@ -1498,9 +1498,9 @@ static int get_url(int argc, const char **argv)
 
 	remotename = argv[0];
 
-	if (!remote_is_configured(remotename))
-		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
+	if (!remote_is_configured(remote))
+		die(_("No such remote '%s'"), remotename);
 
 	url_nr = 0;
 	if (push_mode) {
@@ -1566,9 +1566,9 @@ static int set_url(int argc, const char **argv)
 	if (delete_mode)
 		oldurl = newurl;
 
-	if (!remote_is_configured(remotename))
-		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
+	if (!remote_is_configured(remote))
+		die(_("No such remote '%s'"), remotename);
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
diff --git a/remote.c b/remote.c
index bcd8eb6..d10ae00 100644
--- a/remote.c
+++ b/remote.c
@@ -713,18 +713,9 @@ struct remote *pushremote_get(const char *name)
 	return remote_get_1(name, pushremote_for_branch);
 }
 
-int remote_is_configured(const char *name)
+int remote_is_configured(struct remote *remote)
 {
-	struct remotes_hash_key lookup;
-	struct hashmap_entry lookup_entry;
-	read_config();
-
-	init_remotes_hash();
-	lookup.str = name;
-	lookup.len = strlen(name);
-	hashmap_entry_init(&lookup_entry, memhash(name, lookup.len));
-
-	return hashmap_get(&remotes_hash, &lookup_entry, &lookup) != NULL;
+	return remote && remote->origin;
 }
 
 int for_each_remote(each_remote_fn fn, void *priv)
diff --git a/remote.h b/remote.h
index 4fd7a0f..c21fd37 100644
--- a/remote.h
+++ b/remote.h
@@ -5,6 +5,7 @@
 #include "hashmap.h"
 
 enum {
+	REMOTE_UNCONFIGURED = 0,
 	REMOTE_CONFIG,
 	REMOTE_REMOTES,
 	REMOTE_BRANCHES
@@ -59,7 +60,7 @@ struct remote {
 
 struct remote *remote_get(const char *name);
 struct remote *pushremote_get(const char *name);
-int remote_is_configured(const char *name);
+int remote_is_configured(struct remote *remote);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 15, 2016, 22:39 UTC · re: Thomas Gummerer · lore

[PATCH v2 3/4] remote: actually check if remote exits

When converting the git remote command to a builtin in 211c89 ("Make
git-remote a builtin"), a few calls to check if a remote exists were
converted from:
       if (!exists $remote->{$name}) {
       	  [...]
to:
       remote = remote_get(argv[1]);
       if (!remote)
          [...]

The new check is not quite correct, because remote_get() never returns NULL if a name is given. This leaves us with the somewhat cryptic error message "error: Could not remove config section 'remote.test'", if we are trying to remove a remote that does not exist, or a similar error if we try to rename a remote.

Use the remote_is_configured() function to check whether the remote actually exists, and die with a more sensible error message ("No such remote: $remotename") instead if it doesn't.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/remote.c  |  4 ++--
 t/t5505-remote.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)
Show changes to 2 files +20 −2

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index d966262..981c487 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -634,7 +634,7 @@ static int mv(int argc, const char **argv)
 	rename.remote_branches = &remote_branches;
 
 	oldremote = remote_get(rename.old);
-	if (!oldremote)
+	if (!remote_is_configured(oldremote))
 		die(_("No such remote: %s"), rename.old);
 
 	if (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)
@@ -773,7 +773,7 @@ static int rm(int argc, const char **argv)
 		usage_with_options(builtin_remote_rm_usage, options);
 
 	remote = remote_get(argv[1]);
-	if (!remote)
+	if (!remote_is_configured(remote))
 		die(_("No such remote: %s"), argv[1]);
 
 	known_remotes.to_delete = remote;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1a8e3b8..f1d073f 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -139,6 +139,24 @@ test_expect_success 'remove remote protects local branches' '
 	)
 '
 
+test_expect_success 'remove errors out early when deleting non-existent branch' '
+	(
+		cd test &&
+		echo "fatal: No such remote: foo" >expect &&
+		test_must_fail git remote rm foo 2>actual &&
+		test_i18ncmp expect actual
+	)
+'
+
+test_expect_success 'rename errors out early when deleting non-existent branch' '
+	(
+		cd test &&
+		echo "fatal: No such remote: foo" >expect &&
+		test_must_fail git remote rename foo bar 2>actual &&
+		test_i18ncmp expect actual
+	)
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 15, 2016, 22:39 UTC · re: Thomas Gummerer · lore

[PATCH v2 4/4] remote: use remote_is_configured() for add and rename

Both remote add and remote rename use a slightly different hand-rolled check if the remote exits. The hand-rolled check may have some subtle cases in which it might fail to detect when a remote already exists. One such case was fixed in fb86e32 ("git remote: allow adding remotes agreeing with url.<...>.insteadOf"). Another case is when a remote is configured as follows:

  [remote "foo"]
    vcs = bar

If we try to run `git remote add foo bar` with the above remote configuration, git segfaults. This change fixes it.

In addition, git remote rename $existing foo with the configuration for foo as above silently succeeds, even though foo already exists, modifying its configuration. With this patch it fails with "remote foo already exists".

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/remote.c  |  7 ++-----
 t/t5505-remote.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)
Show changes to 2 files +20 −5

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index 981c487..bd57f1b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
 	url = argv[1];
 
 	remote = remote_get(name);
-	if (remote && (remote->url_nr > 1 ||
-			(strcmp(name, remote->url[0]) &&
-				strcmp(url, remote->url[0])) ||
-			remote->fetch_refspec_nr))
+	if (remote_is_configured(remote))
 		die(_("remote %s already exists."), name);
 
 	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
@@ -641,7 +638,7 @@ static int mv(int argc, const char **argv)
 		return migrate_file(oldremote);
 
 	newremote = remote_get(rename.new);
-	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+	if (remote_is_configured(newremote))
 		die(_("remote %s already exists."), rename.new);
 
 	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index f1d073f..142ae62 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
 	)
 '
 
+test_expect_success 'add existing foreign_vcs remote' '
+	git config --add remote.foo.vcs "bar" &&
+	test_when_finished git remote rm foo &&
+	echo "fatal: remote foo already exists." >expect &&
+	test_must_fail git remote add foo bar 2>actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'add existing foreign_vcs remote' '
+	git config --add remote.foo.vcs "bar" &&
+	git config --add remote.bar.vcs "bar" &&
+	test_when_finished git remote rm foo &&
+	test_when_finished git remote rm bar &&
+	echo "fatal: remote bar already exists." >expect &&
+	test_must_fail git remote rename foo bar 2>actual &&
+	test_i18ncmp expect actual
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
-- 
2.7.1.410.g6faf27b
Eric Sunshine· Feb 15, 2016, 22:52 UTC · re: Thomas Gummerer · lore

Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename

On Mon, Feb 15, 2016 at 5:39 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:
Show 16 quoted lines
> Both remote add and remote rename use a slightly different hand-rolled
> check if the remote exits.  The hand-rolled check may have some subtle
> cases in which it might fail to detect when a remote already exists.
> One such case was fixed in fb86e32 ("git remote: allow adding remotes
> agreeing with url.<...>.insteadOf").  Another case is when a remote is
> configured as follows:
> [...]
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
> +test_expect_success 'add existing foreign_vcs remote' '
> +       git config --add remote.foo.vcs "bar" &&
> +       git config --add remote.bar.vcs "bar" &&
> +       test_when_finished git remote rm foo &&
> +       test_when_finished git remote rm bar &&
Nit: If the second git-config fails, then none of the cleanup will
happen. You'd either want to re-order them like this:
    git config --add remote.foo.vcs "bar" &&
    test_when_finished git remote rm foo &&
    git config --add remote.bar.vcs "bar" &&
    test_when_finished git remote rm bar &&
or this:
    test_when_finished git remote rm foo &&
    git config --add remote.foo.vcs "bar" &&
    test_when_finished git remote rm bar &&
    git config --add remote.bar.vcs "bar" &&
or this:
    test_when_finished git remote rm foo &&
    test_when_finished git remote rm bar &&
    git config --add remote.foo.vcs "bar" &&
    git config --add remote.bar.vcs "bar" &&
Probably not worth a re-roll, though.
> +       echo "fatal: remote bar already exists." >expect &&
> +       test_must_fail git remote rename foo bar 2>actual &&
> +       test_i18ncmp expect actual
> +'
Jeff King· Feb 15, 2016, 23:09 UTC · re: Eric Sunshine · lore

Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename

On Mon, Feb 15, 2016 at 05:52:14PM -0500, Eric Sunshine wrote:
Show 15 quoted lines
> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
> > +test_expect_success 'add existing foreign_vcs remote' '
> > +       git config --add remote.foo.vcs "bar" &&
> > +       git config --add remote.bar.vcs "bar" &&
> > +       test_when_finished git remote rm foo &&
> > +       test_when_finished git remote rm bar &&
> 
> Nit: If the second git-config fails, then none of the cleanup will
> happen. You'd either want to re-order them like this:
> 
>     git config --add remote.foo.vcs "bar" &&
>     test_when_finished git remote rm foo &&
>     git config --add remote.bar.vcs "bar" &&
>     test_when_finished git remote rm bar &&

Good catch. Do we actually care about "--add" here at all? We do not expect these remotes to have any existing config, I think. So would:

  test_config remote.foo.vcs bar &&
  test_config remote.bar.vcs bar

do? I guess technically the failing "git remote rename" could introduce extra config that is not cleaned up by those invocations, and we need to "git remote rm" to get a clean slate, but I don't think that is the case now (and it does not seem likely to become so in the future).

-Peff
Thomas Gummerer· Feb 16, 2016, 00:16 UTC · re: Jeff King · lore

Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename

On 02/15, Jeff King wrote:
Show 28 quoted lines
> On Mon, Feb 15, 2016 at 05:52:14PM -0500, Eric Sunshine wrote:
>
> > > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > > @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
> > > +test_expect_success 'add existing foreign_vcs remote' '
> > > +       git config --add remote.foo.vcs "bar" &&
> > > +       git config --add remote.bar.vcs "bar" &&
> > > +       test_when_finished git remote rm foo &&
> > > +       test_when_finished git remote rm bar &&
> >
> > Nit: If the second git-config fails, then none of the cleanup will
> > happen. You'd either want to re-order them like this:
> >
> >     git config --add remote.foo.vcs "bar" &&
> >     test_when_finished git remote rm foo &&
> >     git config --add remote.bar.vcs "bar" &&
> >     test_when_finished git remote rm bar &&
>
> Good catch. Do we actually care about "--add" here at all? We do not
> expect these remotes to have any existing config, I think. So would:
>
>   test_config remote.foo.vcs bar &&
>   test_config remote.bar.vcs bar
>
> do? I guess technically the failing "git remote rename" could introduce
> extra config that is not cleaned up by those invocations, and we need to
> "git remote rm" to get a clean slate, but I don't think that is the case
> now (and it does not seem likely to become so in the future).

Good point, I think the test_config is indeed enough. Thanks, both, will fix in the re-roll.

> -Peff

-- Thomas

Thomas Gummerer· Feb 16, 2016, 09:47 UTC · re: Thomas Gummerer · lore

[PATCH v3 0/4] git remote improvements

Previous rounds are at $gmane/286214 and $gmane/286278. Thanks to Peff and Eric for the reviews on the previous round.

This version changes two checks in [1/4], and uses test_config in the tests instead of calling git config directly.

Interdiff below:
Show changes to 2 files +8 −11

remote.c, t/t5505-remote.sh

diff --git a/remote.c b/remote.c
index d10ae00..f001681 100644
--- a/remote.c
+++ b/remote.c
@@ -358,17 +358,17 @@ static int handle_config(const char *key, const char *value, void *cb)
 		return 0;
 
 	/* Handle remote.* variables */
-	if (!strcmp(subkey, "pushdefault"))
+	if (!name && !strcmp(subkey, "pushdefault"))
 		return git_config_string(&pushremote_name, key, value);
 
+	if (!name)
+		return 0;
 	/* Handle remote.<name>.* variables */
-	if (*(name ? name : subkey) == '/') {
+	if (*name == '/') {
 		warning("Config remote shorthand cannot begin with '/': %s",
-			name ? name : subkey);
+			name);
 		return 0;
 	}
-	if (!name)
-		return 0;
 	remote = make_remote(name, namelen);
 	remote->origin = REMOTE_CONFIG;
 	if (!strcmp(subkey, "mirror"))
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 142ae62..94079a0 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -158,18 +158,15 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
 '
 
 test_expect_success 'add existing foreign_vcs remote' '
-	git config --add remote.foo.vcs "bar" &&
-	test_when_finished git remote rm foo &&
+	test_config remote.foo.vcs bar &&
 	echo "fatal: remote foo already exists." >expect &&
 	test_must_fail git remote add foo bar 2>actual &&
 	test_i18ncmp expect actual
 '
 
 test_expect_success 'add existing foreign_vcs remote' '
-	git config --add remote.foo.vcs "bar" &&
-	git config --add remote.bar.vcs "bar" &&
-	test_when_finished git remote rm foo &&
-	test_when_finished git remote rm bar &&
+	test_config remote.foo.vcs bar &&
+	test_config remote.bar.vcs bar &&
 	echo "fatal: remote bar already exists." >expect &&
 	test_must_fail git remote rename foo bar 2>actual &&
 	test_i18ncmp expect actual


Thomas Gummerer (4):
  remote: use parse_config_key
  remote: simplify remote_is_configured()
  remote: actually check if remote exits
  remote: use remote_is_configured() for add and rename

 builtin/fetch.c   |  5 ++--
 builtin/remote.c  | 23 +++++++---------
 remote.c          | 82 +++++++++++++++++++++++--------------------------------
 remote.h          |  3 +-
 t/t5505-remote.sh | 33 ++++++++++++++++++++++
 5 files changed, 81 insertions(+), 65 deletions(-)
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 16, 2016, 09:47 UTC · re: Thomas Gummerer · lore

[PATCH v3 1/4] remote: use parse_config_key

95b567c7 ("use skip_prefix to avoid repeating strings") transformed calls using starts_with() and then skipping the length of the prefix to skip_prefix() calls. In remote.c there are a few calls like:

  if (starts_with(foo, "bar"))
      foo += 3

These calls weren't touched by the aformentioned commit, but can be replaced by calls to parse_config_key(), to simplify the code and clarify the intentions. Do that.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 remote.c | 69 ++++++++++++++++++++++++++++++----------------------------------
 1 file changed, 32 insertions(+), 37 deletions(-)
Show changes to remote.c +32 −37
diff --git a/remote.c b/remote.c
index 02e698a..c7d67c2 100644
--- a/remote.c
+++ b/remote.c
@@ -318,93 +318,88 @@ static void read_branches_file(struct remote *remote)
 static int handle_config(const char *key, const char *value, void *cb)
 {
 	const char *name;
+	int namelen;
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
-	if (starts_with(key, "branch.")) {
-		name = key + 7;
-		subkey = strrchr(name, '.');
-		if (!subkey)
+	if (parse_config_key(key, "branch", &name, &namelen, &subkey) >= 0) {
+		if (!name)
 			return 0;
-		branch = make_branch(name, subkey - name);
-		if (!strcmp(subkey, ".remote")) {
+		branch = make_branch(name, namelen);
+		if (!strcmp(subkey, "remote")) {
 			return git_config_string(&branch->remote_name, key, value);
-		} else if (!strcmp(subkey, ".pushremote")) {
+		} else if (!strcmp(subkey, "pushremote")) {
 			return git_config_string(&branch->pushremote_name, key, value);
-		} else if (!strcmp(subkey, ".merge")) {
+		} else if (!strcmp(subkey, "merge")) {
 			if (!value)
 				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
 		}
 		return 0;
 	}
-	if (starts_with(key, "url.")) {
+	if (parse_config_key(key, "url", &name, &namelen, &subkey) >= 0) {
 		struct rewrite *rewrite;
-		name = key + 4;
-		subkey = strrchr(name, '.');
-		if (!subkey)
+		if (!name)
 			return 0;
-		if (!strcmp(subkey, ".insteadof")) {
-			rewrite = make_rewrite(&rewrites, name, subkey - name);
+		if (!strcmp(subkey, "insteadof")) {
+			rewrite = make_rewrite(&rewrites, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
 			add_instead_of(rewrite, xstrdup(value));
-		} else if (!strcmp(subkey, ".pushinsteadof")) {
-			rewrite = make_rewrite(&rewrites_push, name, subkey - name);
+		} else if (!strcmp(subkey, "pushinsteadof")) {
+			rewrite = make_rewrite(&rewrites_push, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
 
-	if (!starts_with(key,  "remote."))
+	if (parse_config_key(key, "remote", &name, &namelen, &subkey) < 0)
 		return 0;
-	name = key + 7;
 
 	/* Handle remote.* variables */
-	if (!strcmp(name, "pushdefault"))
+	if (!name && !strcmp(subkey, "pushdefault"))
 		return git_config_string(&pushremote_name, key, value);
 
+	if (!name)
+		return 0;
 	/* Handle remote.<name>.* variables */
 	if (*name == '/') {
 		warning("Config remote shorthand cannot begin with '/': %s",
 			name);
 		return 0;
 	}
-	subkey = strrchr(name, '.');
-	if (!subkey)
-		return 0;
-	remote = make_remote(name, subkey - name);
+	remote = make_remote(name, namelen);
 	remote->origin = REMOTE_CONFIG;
-	if (!strcmp(subkey, ".mirror"))
+	if (!strcmp(subkey, "mirror"))
 		remote->mirror = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".skipdefaultupdate"))
+	else if (!strcmp(subkey, "skipdefaultupdate"))
 		remote->skip_default_update = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".skipfetchall"))
+	else if (!strcmp(subkey, "skipfetchall"))
 		remote->skip_default_update = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".prune"))
+	else if (!strcmp(subkey, "prune"))
 		remote->prune = git_config_bool(key, value);
-	else if (!strcmp(subkey, ".url")) {
+	else if (!strcmp(subkey, "url")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_url(remote, v);
-	} else if (!strcmp(subkey, ".pushurl")) {
+	} else if (!strcmp(subkey, "pushurl")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_pushurl(remote, v);
-	} else if (!strcmp(subkey, ".push")) {
+	} else if (!strcmp(subkey, "push")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_push_refspec(remote, v);
-	} else if (!strcmp(subkey, ".fetch")) {
+	} else if (!strcmp(subkey, "fetch")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
 		add_fetch_refspec(remote, v);
-	} else if (!strcmp(subkey, ".receivepack")) {
+	} else if (!strcmp(subkey, "receivepack")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
@@ -412,7 +407,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->receivepack = v;
 		else
 			error("more than one receivepack given, using the first");
-	} else if (!strcmp(subkey, ".uploadpack")) {
+	} else if (!strcmp(subkey, "uploadpack")) {
 		const char *v;
 		if (git_config_string(&v, key, value))
 			return -1;
@@ -420,18 +415,18 @@ static int handle_config(const char *key, const char *value, void *cb)
 			remote->uploadpack = v;
 		else
 			error("more than one uploadpack given, using the first");
-	} else if (!strcmp(subkey, ".tagopt")) {
+	} else if (!strcmp(subkey, "tagopt")) {
 		if (!strcmp(value, "--no-tags"))
 			remote->fetch_tags = -1;
 		else if (!strcmp(value, "--tags"))
 			remote->fetch_tags = 2;
-	} else if (!strcmp(subkey, ".proxy")) {
+	} else if (!strcmp(subkey, "proxy")) {
 		return git_config_string((const char **)&remote->http_proxy,
 					 key, value);
-	} else if (!strcmp(subkey, ".proxyauthmethod")) {
+	} else if (!strcmp(subkey, "proxyauthmethod")) {
 		return git_config_string((const char **)&remote->http_proxy_authmethod,
 					 key, value);
-	} else if (!strcmp(subkey, ".vcs")) {
+	} else if (!strcmp(subkey, "vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
 	}
 	return 0;
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 16, 2016, 09:47 UTC · re: Thomas Gummerer · lore

[PATCH v3 2/4] remote: simplify remote_is_configured()

The remote_is_configured() function allows checking whether a remote exists or not. The function however only works if remote_get() wasn't called before calling it. In addition, it only checks the configuration for remotes, but not remotes or branches files.

Make use of the origin member of struct remote instead, which indicates where the remote comes from. It will be set to some value if the remote is configured in any file in the repository, but is initialized to 0 if the remote is only created in make_remote().

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/fetch.c  |  5 ++---
 builtin/remote.c | 12 ++++++------
 remote.c         | 13 ++-----------
 remote.h         |  3 ++-
 4 files changed, 12 insertions(+), 21 deletions(-)
Show changes to 4 files +12 −21

builtin/fetch.c, builtin/remote.c, remote.c, remote.h

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..8121830 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1016,10 +1016,9 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 
 	git_config(get_remote_group, &g);
 	if (list->nr == prev_nr) {
-		struct remote *remote;
-		if (!remote_is_configured(name))
+		struct remote *remote = remote_get(name);
+		if (!remote_is_configured(remote))
 			return 0;
-		remote = remote_get(name);
 		string_list_append(list, remote->name);
 	}
 	return 1;
diff --git a/builtin/remote.c b/builtin/remote.c
index 2b2ff9b..d966262 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1441,9 +1441,9 @@ static int set_remote_branches(const char *remotename, const char **branches,
 
 	strbuf_addf(&key, "remote.%s.fetch", remotename);
 
-	if (!remote_is_configured(remotename))
-		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
+	if (!remote_is_configured(remote))
+		die(_("No such remote '%s'"), remotename);
 
 	if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
 		strbuf_release(&key);
@@ -1498,9 +1498,9 @@ static int get_url(int argc, const char **argv)
 
 	remotename = argv[0];
 
-	if (!remote_is_configured(remotename))
-		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
+	if (!remote_is_configured(remote))
+		die(_("No such remote '%s'"), remotename);
 
 	url_nr = 0;
 	if (push_mode) {
@@ -1566,9 +1566,9 @@ static int set_url(int argc, const char **argv)
 	if (delete_mode)
 		oldurl = newurl;
 
-	if (!remote_is_configured(remotename))
-		die(_("No such remote '%s'"), remotename);
 	remote = remote_get(remotename);
+	if (!remote_is_configured(remote))
+		die(_("No such remote '%s'"), remotename);
 
 	if (push_mode) {
 		strbuf_addf(&name_buf, "remote.%s.pushurl", remotename);
diff --git a/remote.c b/remote.c
index c7d67c2..f001681 100644
--- a/remote.c
+++ b/remote.c
@@ -713,18 +713,9 @@ struct remote *pushremote_get(const char *name)
 	return remote_get_1(name, pushremote_for_branch);
 }
 
-int remote_is_configured(const char *name)
+int remote_is_configured(struct remote *remote)
 {
-	struct remotes_hash_key lookup;
-	struct hashmap_entry lookup_entry;
-	read_config();
-
-	init_remotes_hash();
-	lookup.str = name;
-	lookup.len = strlen(name);
-	hashmap_entry_init(&lookup_entry, memhash(name, lookup.len));
-
-	return hashmap_get(&remotes_hash, &lookup_entry, &lookup) != NULL;
+	return remote && remote->origin;
 }
 
 int for_each_remote(each_remote_fn fn, void *priv)
diff --git a/remote.h b/remote.h
index 4fd7a0f..c21fd37 100644
--- a/remote.h
+++ b/remote.h
@@ -5,6 +5,7 @@
 #include "hashmap.h"
 
 enum {
+	REMOTE_UNCONFIGURED = 0,
 	REMOTE_CONFIG,
 	REMOTE_REMOTES,
 	REMOTE_BRANCHES
@@ -59,7 +60,7 @@ struct remote {
 
 struct remote *remote_get(const char *name);
 struct remote *pushremote_get(const char *name);
-int remote_is_configured(const char *name);
+int remote_is_configured(struct remote *remote);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 16, 2016, 09:47 UTC · re: Thomas Gummerer · lore

[PATCH v3 3/4] remote: actually check if remote exits

When converting the git remote command to a builtin in 211c89 ("Make
git-remote a builtin"), a few calls to check if a remote exists were
converted from:
       if (!exists $remote->{$name}) {
       	  [...]
to:
       remote = remote_get(argv[1]);
       if (!remote)
          [...]

The new check is not quite correct, because remote_get() never returns NULL if a name is given. This leaves us with the somewhat cryptic error message "error: Could not remove config section 'remote.test'", if we are trying to remove a remote that does not exist, or a similar error if we try to rename a remote.

Use the remote_is_configured() function to check whether the remote actually exists, and die with a more sensible error message ("No such remote: $remotename") instead if it doesn't.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/remote.c  |  4 ++--
 t/t5505-remote.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)
Show changes to 2 files +20 −2

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index d966262..981c487 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -634,7 +634,7 @@ static int mv(int argc, const char **argv)
 	rename.remote_branches = &remote_branches;
 
 	oldremote = remote_get(rename.old);
-	if (!oldremote)
+	if (!remote_is_configured(oldremote))
 		die(_("No such remote: %s"), rename.old);
 
 	if (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)
@@ -773,7 +773,7 @@ static int rm(int argc, const char **argv)
 		usage_with_options(builtin_remote_rm_usage, options);
 
 	remote = remote_get(argv[1]);
-	if (!remote)
+	if (!remote_is_configured(remote))
 		die(_("No such remote: %s"), argv[1]);
 
 	known_remotes.to_delete = remote;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1a8e3b8..f1d073f 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -139,6 +139,24 @@ test_expect_success 'remove remote protects local branches' '
 	)
 '
 
+test_expect_success 'remove errors out early when deleting non-existent branch' '
+	(
+		cd test &&
+		echo "fatal: No such remote: foo" >expect &&
+		test_must_fail git remote rm foo 2>actual &&
+		test_i18ncmp expect actual
+	)
+'
+
+test_expect_success 'rename errors out early when deleting non-existent branch' '
+	(
+		cd test &&
+		echo "fatal: No such remote: foo" >expect &&
+		test_must_fail git remote rename foo bar 2>actual &&
+		test_i18ncmp expect actual
+	)
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
-- 
2.7.1.410.g6faf27b
Thomas Gummerer· Feb 16, 2016, 09:47 UTC · re: Thomas Gummerer · lore

[PATCH v3 4/4] remote: use remote_is_configured() for add and rename

Both remote add and remote rename use a slightly different hand-rolled check if the remote exits. The hand-rolled check may have some subtle cases in which it might fail to detect when a remote already exists. One such case was fixed in fb86e32 ("git remote: allow adding remotes agreeing with url.<...>.insteadOf"). Another case is when a remote is configured as follows:

  [remote "foo"]
    vcs = bar

If we try to run `git remote add foo bar` with the above remote configuration, git segfaults. This change fixes it.

In addition, git remote rename $existing foo with the configuration for foo as above silently succeeds, even though foo already exists, modifying its configuration. With this patch it fails with "remote foo already exists".

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 builtin/remote.c  |  7 ++-----
 t/t5505-remote.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 5 deletions(-)
Show changes to 2 files +17 −5

builtin/remote.c, t/t5505-remote.sh

diff --git a/builtin/remote.c b/builtin/remote.c
index 981c487..bd57f1b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
 	url = argv[1];
 
 	remote = remote_get(name);
-	if (remote && (remote->url_nr > 1 ||
-			(strcmp(name, remote->url[0]) &&
-				strcmp(url, remote->url[0])) ||
-			remote->fetch_refspec_nr))
+	if (remote_is_configured(remote))
 		die(_("remote %s already exists."), name);
 
 	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
@@ -641,7 +638,7 @@ static int mv(int argc, const char **argv)
 		return migrate_file(oldremote);
 
 	newremote = remote_get(rename.new);
-	if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+	if (remote_is_configured(newremote))
 		die(_("remote %s already exists."), rename.new);
 
 	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index f1d073f..94079a0 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -157,6 +157,21 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
 	)
 '
 
+test_expect_success 'add existing foreign_vcs remote' '
+	test_config remote.foo.vcs bar &&
+	echo "fatal: remote foo already exists." >expect &&
+	test_must_fail git remote add foo bar 2>actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'add existing foreign_vcs remote' '
+	test_config remote.foo.vcs bar &&
+	test_config remote.bar.vcs bar &&
+	echo "fatal: remote bar already exists." >expect &&
+	test_must_fail git remote rename foo bar 2>actual &&
+	test_i18ncmp expect actual
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one
-- 
2.7.1.410.g6faf27b
Jeff King· Feb 16, 2016, 15:52 UTC · re: Thomas Gummerer · lore

Re: [PATCH v3 0/4] git remote improvements

On Tue, Feb 16, 2016 at 10:47:48AM +0100, Thomas Gummerer wrote:
> Previous rounds are at $gmane/286214 and $gmane/286278.  Thanks to
> Peff and Eric for the reviews on the previous round.
This round looks good to me. Thanks for working on it!
Reviewed-by: Jeff King <peff@peff.net>
-Peff
Junio C Hamano· Feb 16, 2016, 21:36 UTC · re: Thomas Gummerer · lore

Re: [PATCH v3 0/4] git remote improvements

Thomas Gummerer <t.gummerer@gmail.com> writes:
Show 5 quoted lines
> Previous rounds are at $gmane/286214 and $gmane/286278.  Thanks to
> Peff and Eric for the reviews on the previous round.
>
> This version changes two checks in [1/4], and uses test_config in the
> tests instead of calling git config directly.

Thanks; I agree that 1/4 is a noisy patch but the culprit of the noisyness is that the old code, when doing tons of comparisons with fixed strings that begin with a dot, did not think it was crazy not to increment the "subkey" before doing so, and the resulting code is much easier to follow.

Thanks.

← back to recent threads