[PATCH 0/4] git remote improvements

[PATCH 0/4] git remote improvements

[Thomas Gummerer]

In builtin/remote.c there are a few cases which check whether a remote
exists or not.  These checks are however done inconsistently, and in a
few cases they don't check anything at all.  This patch series tries
to improve the situation.

I stumbled upon this when I mistyped the remote name in git remote rm,
and got a cryptic error message, and thought the other cases might be
worth fixing as well.

There is one such check left in get_remote_ref_states, but I'm not
quite sure what to do about that one.  That function gets called in
git remote show/set-head/prune, where it might make sense for the user
to provide a url, or a local repository, at least in the git remote
show case.

I think we have the following options there:

 (1) just remove the check, as nobody seems to have noticed until now,
     and the check doesn't actually do anything (get_remote() is never
     called with NULL, so it will never return NULL).  In this case
     git dies when a non-existent remote is encountered, e.g. git
     remote show nonexistent existent will die immediately, while git
     remote show existent nonexistent will show info about the
     existing remote, and die afterwards.
     
 (2) add an option not to die in the transport function, and let the
     caller of get_remote_ref_states handle the error i.e. show the
     error and keep going with the next remote.

 (3) anything I might be missing?

Thomas Gummerer (4):
  remote: use skip_prefix
  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          | 22 +++++-----------------
 remote.h          |  4 ++--
 t/t5505-remote.sh | 36 ++++++++++++++++++++++++++++++++++++
 5 files changed, 55 insertions(+), 35 deletions(-)

-- 
2.7.1.410.g6faf27b

[PATCH 1/4] remote: use skip_prefix

[Thomas Gummerer]

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 commit mentioned above, but can
benefit from the same treatment to avoid magic numbers.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 remote.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 02e698a..4b5b576 100644
--- a/remote.c
+++ b/remote.c
@@ -321,8 +321,7 @@ static int handle_config(const char *key, const char *value, void *cb)
 	const char *subkey;
 	struct remote *remote;
 	struct branch *branch;
-	if (starts_with(key, "branch.")) {
-		name = key + 7;
+	if (skip_prefix(key, "branch.", &name)) {
 		subkey = strrchr(name, '.');
 		if (!subkey)
 			return 0;
@@ -338,9 +337,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 		}
 		return 0;
 	}
-	if (starts_with(key, "url.")) {
+	if (skip_prefix(key, "url.", &name)) {
 		struct rewrite *rewrite;
-		name = key + 4;
 		subkey = strrchr(name, '.');
 		if (!subkey)
 			return 0;
@@ -357,9 +355,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 		}
 	}
 
-	if (!starts_with(key,  "remote."))
+	if (!skip_prefix(key, "remote.", &name))
 		return 0;
-	name = key + 7;
 
 	/* Handle remote.* variables */
 	if (!strcmp(name, "pushdefault"))
-- 
2.7.1.410.g6faf27b

Re: [PATCH 1/4] remote: use skip_prefix

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:

> 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 commit mentioned above, but can
> benefit from the same treatment to avoid magic numbers.

This is definitely an improvement, but I think we can actually go a step
further here, and use parse_config_key. Like:

diff --git a/remote.c b/remote.c
index 21e4ec3..8d2c3ca 100644
--- a/remote.c
+++ b/remote.c
@@ -318,15 +318,14 @@ 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 (starts_with(key, "branch", &name, &namelen, &subkey)) {
+		if (!name)
 			return 0;
-		branch = make_branch(name, subkey - name);
+		branch = make_branch(name, namelen);
 		if (!strcmp(subkey, ".remote")) {
 			return git_config_string(&branch->remote_name, key, value);
 		} else if (!strcmp(subkey, ".pushremote")) {

and so on. The difference in lines of code isn't that great, but I think
it makes the resulting code more obvious to read.

-Peff

Re: [PATCH 1/4] remote: use skip_prefix

[Eric Sunshine]

On Mon, Feb 15, 2016 at 1:18 PM, Jeff King <peff@peff.net> wrote:
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:
>
> -       if (starts_with(key, "branch.")) {
> -               name = key + 7;
> -               subkey = strrchr(name, '.');
> -               if (!subkey)
> +       if (starts_with(key, "branch", &name, &namelen, &subkey)) {

I guess you meant: s/starts_with/parse_config_key/

> +               if (!name)
>                         return 0;
> -               branch = make_branch(name, subkey - name);
> +               branch = make_branch(name, namelen);
>                 if (!strcmp(subkey, ".remote")) {
>                         return git_config_string(&branch->remote_name, key, value);
>                 } else if (!strcmp(subkey, ".pushremote")) {

Re: [PATCH 1/4] remote: use skip_prefix

[Jeff King]

On Mon, Feb 15, 2016 at 01:35:03PM -0500, Eric Sunshine wrote:

> On Mon, Feb 15, 2016 at 1:18 PM, Jeff King <peff@peff.net> wrote:
> > This is definitely an improvement, but I think we can actually go a step
> > further here, and use parse_config_key. Like:
> >
> > -       if (starts_with(key, "branch.")) {
> > -               name = key + 7;
> > -               subkey = strrchr(name, '.');
> > -               if (!subkey)
> > +       if (starts_with(key, "branch", &name, &namelen, &subkey)) {
> 
> I guess you meant: s/starts_with/parse_config_key/

Whoops, yeah. I'll belatedly add a "not even compile-tested" disclaimer.
:)

-Peff

Re: [PATCH 1/4] remote: use skip_prefix

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:
>
> > 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 commit mentioned above, but can
> > benefit from the same treatment to avoid magic numbers.
>
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:

Thanks, I had no idea about this function :) It makes the diff a lot
noisier, but I do think the end result is better.

> diff --git a/remote.c b/remote.c
> index 21e4ec3..8d2c3ca 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -318,15 +318,14 @@ 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 (starts_with(key, "branch", &name, &namelen, &subkey)) {
> +		if (!name)
>  			return 0;
> -		branch = make_branch(name, subkey - name);
> +		branch = make_branch(name, namelen);
>  		if (!strcmp(subkey, ".remote")) {
>  			return git_config_string(&branch->remote_name, key, value);
>  		} else if (!strcmp(subkey, ".pushremote")) {
>
> and so on. The difference in lines of code isn't that great, but I think
> it makes the resulting code more obvious to read.
>
> -Peff

--
Thomas

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

[Thomas Gummerer]

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         |  4 ++--
 4 files changed, 12 insertions(+), 22 deletions(-)

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 4b5b576..3a4ca9b 100644
--- a/remote.c
+++ b/remote.c
@@ -715,18 +715,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..7a5ee77 100644
--- a/remote.h
+++ b/remote.h
@@ -5,7 +5,7 @@
 #include "hashmap.h"
 
 enum {
-	REMOTE_CONFIG,
+	REMOTE_CONFIG = 1,
 	REMOTE_REMOTES,
 	REMOTE_BRANCHES
 };
@@ -59,7 +59,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

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

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:28PM +0100, Thomas Gummerer wrote:

> 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().

Makes sense. I wonder if we would want to give this an explicit slot in
the enum. I.e.:

> diff --git a/remote.h b/remote.h
> index 4fd7a0f..7a5ee77 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -5,7 +5,7 @@
>  #include "hashmap.h"
>  
>  enum {
> -	REMOTE_CONFIG,
> +	REMOTE_CONFIG = 1,
>  	REMOTE_REMOTES,
>  	REMOTE_BRANCHES
>  };

Add in "REMOTE_UNCONFIGURED = 0" here. It makes no difference to
correctness, but is perhaps documents what is going on a bit better.

-Peff

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

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 06:42:28PM +0100, Thomas Gummerer wrote:
>
> > 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().
>
> Makes sense. I wonder if we would want to give this an explicit slot in
> the enum. I.e.:
>
> > diff --git a/remote.h b/remote.h
> > index 4fd7a0f..7a5ee77 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -5,7 +5,7 @@
> >  #include "hashmap.h"
> >
> >  enum {
> > -	REMOTE_CONFIG,
> > +	REMOTE_CONFIG = 1,
> >  	REMOTE_REMOTES,
> >  	REMOTE_BRANCHES
> >  };
>
> Add in "REMOTE_UNCONFIGURED = 0" here. It makes no difference to
> correctness, but is perhaps documents what is going on a bit better.

Agreed, will change.  Thanks.

>
> -Peff

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

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> Agreed, will change.  Thanks.

Thanks for working on this, and thanks reviewers for helping.

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

[Thomas Gummerer]

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(-)

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

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

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:29PM +0100, Thomas Gummerer wrote:

> 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(-)

Nice. Very cleanly explained and implemented.

-Peff

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

[Thomas Gummerer]

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(-)

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

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

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:

> 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".

Checking is_configured() certainly sounds like a better test, but...

> 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);

This original is quite confusing. I thought at first that there was
perhaps something going on with allowing repeated re-configuration of
the same remote, as long as some parameters matched. I.e., I am
wondering if there is a case here that does _not_ segfault, that we
would be breaking.

But reading over fb86e32dcc, I think I have convinced myself that it was
merely an ad-hoc check for "is_configured", and using that function is a
better replacement.

-Peff

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

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:
>
> > 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".
>
> Checking is_configured() certainly sounds like a better test, but...
>
> > 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);
>
> This original is quite confusing. I thought at first that there was
> perhaps something going on with allowing repeated re-configuration of
> the same remote, as long as some parameters matched. I.e., I am
> wondering if there is a case here that does _not_ segfault, that we
> would be breaking.
>
> But reading over fb86e32dcc, I think I have convinced myself that it was
> merely an ad-hoc check for "is_configured", and using that function is a
> better replacement.

It took me a while too to convince myself there is nothing strange
going on.  But I could neither find anything in the history, nor could
I think of any case that we could break.

Thanks for your review!

> -Peff

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

[Johannes Schindelin]

Hi Peff & Thomas,

On Mon, 15 Feb 2016, Jeff King wrote:

> On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:
> 
> > 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".
> 
> Checking is_configured() certainly sounds like a better test, but...
> 
> > 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);
> 
> This original is quite confusing. I thought at first that there was
> perhaps something going on with allowing repeated re-configuration of
> the same remote, as long as some parameters matched. I.e., I am
> wondering if there is a case here that does _not_ segfault, that we
> would be breaking.
> 
> But reading over fb86e32dcc, I think I have convinced myself that it was
> merely an ad-hoc check for "is_configured", and using that function is a
> better replacement.

Yes, yes, yes. Y'all are absolutely correct. I shoulda added a test case
right away, to make sure not only that what I fixed does not get broken in
the future, but also to document *what* was fixed, exactly.

So, belatedly, here goes a patch that verifies what that commit was
supposed to fix, and yes, it passes with Thomas' changes (Junio, would you
please apply this on top of tg/git-remote?):

-- snipsnap --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 17 Feb 2016 14:45:59 +0100
Subject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x

This is the test missing from fb86e32 (git remote: allow adding
remotes agreeing with url.<...>.insteadOf, 2014-12-23): we should
allow adding a remote with the URL when it agrees with the
url.<...>.insteadOf setting.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5505-remote.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 94079a0..19e8e34 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -51,6 +51,11 @@ test_expect_success setup '
 	git clone one test
 '
 
+test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
+	git config url.git@host.com:team/repo.git.insteadOf myremote &&
+	git remote add myremote git@host.com:team/repo.git
+'
+
 test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '
 	(
 		cd test &&
-- 
2.7.1.windows.2

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

[Thomas Gummerer]

On 02/17, Johannes Schindelin wrote:
> Hi Peff & Thomas,
>
> On Mon, 15 Feb 2016, Jeff King wrote:
> > This original is quite confusing. I thought at first that there was
> > perhaps something going on with allowing repeated re-configuration of
> > the same remote, as long as some parameters matched. I.e., I am
> > wondering if there is a case here that does _not_ segfault, that we
> > would be breaking.
> >
> > But reading over fb86e32dcc, I think I have convinced myself that it was
> > merely an ad-hoc check for "is_configured", and using that function is a
> > better replacement.
>
> Yes, yes, yes. Y'all are absolutely correct. I shoulda added a test case
> right away, to make sure not only that what I fixed does not get broken in
> the future, but also to document *what* was fixed, exactly.

Thanks for confirming and the test case so we don't break it again.

> So, belatedly, here goes a patch that verifies what that commit was
> supposed to fix, and yes, it passes with Thomas' changes (Junio, would you
> please apply this on top of tg/git-remote?):
>
> -- snipsnap --
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> Date: Wed, 17 Feb 2016 14:45:59 +0100
> Subject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x
>
> This is the test missing from fb86e32 (git remote: allow adding
> remotes agreeing with url.<...>.insteadOf, 2014-12-23): we should
> allow adding a remote with the URL when it agrees with the
> url.<...>.insteadOf setting.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t5505-remote.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 94079a0..19e8e34 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -51,6 +51,11 @@ test_expect_success setup '
>  	git clone one test
>  '
>
> +test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
> +	git config url.git@host.com:team/repo.git.insteadOf myremote &&

Minor nit: I think we should use test_config here.

> +	git remote add myremote git@host.com:team/repo.git
> +'
> +
>  test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '
>  	(
>  		cd test &&
> --
> 2.7.1.windows.2
--
Thomas

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

[Johannes Schindelin]

Hi Tomas,

On Wed, 17 Feb 2016, Thomas Gummerer wrote:

> On 02/17, Johannes Schindelin wrote:
> >
> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > index 94079a0..19e8e34 100755
> > --- a/t/t5505-remote.sh
> > +++ b/t/t5505-remote.sh
> > @@ -51,6 +51,11 @@ test_expect_success setup '
> >  	git clone one test
> >  '
> >
> > +test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
> > +	git config url.git@host.com:team/repo.git.insteadOf myremote &&
> 
> Minor nit: I think we should use test_config here.

Good point.

-- snipsnap --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 17 Feb 2016 14:45:59 +0100
Subject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x

This is the test missing from fb86e32 (git remote: allow adding
remotes agreeing with url.<...>.insteadOf, 2014-12-23): we should
allow adding a remote with the URL when it agrees with the
url.<...>.insteadOf setting.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5505-remote.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 94079a0..949725e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -51,6 +51,11 @@ test_expect_success setup '
 	git clone one test
 '
 
+test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '
+	test_config url.git@host.com:team/repo.git.insteadOf myremote &&
+	git remote add myremote git@host.com:team/repo.git
+'
+
 test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '
 	(
 		cd test &&
-- 
2.7.1.windows.2