{"thread":{"id":"41419","subject":"[PATCH v2 0/4] git remote improvements","startedAt":"2016-02-15T22:39:40Z","lastAt":"2016-02-16T21:36:47Z","messageCount":17,"participants":["Thomas Gummerer","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"278234","messageId":"1455575984-24348-1-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":null,"subject":"[PATCH v2 0/4] git remote improvements","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T22:39:40Z","receivedAt":"2016-02-15T22:39:40Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Thanks Peff for the review on the previous round ($gmane/286214).\n\nThe series now uses parse_config_key() instead of skip_prefix, and I\nadded REMOTE_UNCONFIGURED to the enum used in the origin field in\nstruct remote.\n\nInterdiff below:\n\ndiff --git a/remote.c b/remote.c\nindex 3a4ca9b..d10ae00 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -318,90 +318,88 @@ static void read_branches_file(struct remote *remote)\n static int handle_config(const char *key, const char *value, void *cb)\n {\n \tconst char *name;\n+\tint namelen;\n \tconst char *subkey;\n \tstruct remote *remote;\n \tstruct branch *branch;\n-\tif (skip_prefix(key, \"branch.\", &name)) {\n-\t\tsubkey = strrchr(name, '.');\n-\t\tif (!subkey)\n+\tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tbranch = make_branch(name, subkey - name);\n-\t\tif (!strcmp(subkey, \".remote\")) {\n+\t\tbranch = make_branch(name, namelen);\n+\t\tif (!strcmp(subkey, \"remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n-\t\t} else if (!strcmp(subkey, \".pushremote\")) {\n+\t\t} else if (!strcmp(subkey, \"pushremote\")) {\n \t\t\treturn git_config_string(&branch->pushremote_name, key, value);\n-\t\t} else if (!strcmp(subkey, \".merge\")) {\n+\t\t} else if (!strcmp(subkey, \"merge\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_merge(branch, xstrdup(value));\n \t\t}\n \t\treturn 0;\n \t}\n-\tif (skip_prefix(key, \"url.\", &name)) {\n+\tif (parse_config_key(key, \"url\", &name, &namelen, &subkey) >= 0) {\n \t\tstruct rewrite *rewrite;\n-\t\tsubkey = strrchr(name, '.');\n-\t\tif (!subkey)\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tif (!strcmp(subkey, \".insteadof\")) {\n-\t\t\trewrite = make_rewrite(&rewrites, name, subkey - name);\n+\t\tif (!strcmp(subkey, \"insteadof\")) {\n+\t\t\trewrite = make_rewrite(&rewrites, name, namelen);\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n-\t\t} else if (!strcmp(subkey, \".pushinsteadof\")) {\n-\t\t\trewrite = make_rewrite(&rewrites_push, name, subkey - name);\n+\t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n+\t\t\trewrite = make_rewrite(&rewrites_push, name, namelen);\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t}\n \t}\n \n-\tif (!skip_prefix(key, \"remote.\", &name))\n+\tif (parse_config_key(key, \"remote\", &name, &namelen, &subkey) < 0)\n \t\treturn 0;\n \n \t/* Handle remote.* variables */\n-\tif (!strcmp(name, \"pushdefault\"))\n+\tif (!strcmp(subkey, \"pushdefault\"))\n \t\treturn git_config_string(&pushremote_name, key, value);\n \n \t/* Handle remote.<name>.* variables */\n-\tif (*name == '/') {\n+\tif (*(name ? name : subkey) == '/') {\n \t\twarning(\"Config remote shorthand cannot begin with '/': %s\",\n-\t\t\tname);\n+\t\t\tname ? name : subkey);\n \t\treturn 0;\n \t}\n-\tsubkey = strrchr(name, '.');\n-\tif (!subkey)\n+\tif (!name)\n \t\treturn 0;\n-\tremote = make_remote(name, subkey - name);\n+\tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n-\tif (!strcmp(subkey, \".mirror\"))\n+\tif (!strcmp(subkey, \"mirror\"))\n \t\tremote->mirror = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".skipdefaultupdate\"))\n+\telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n \t\tremote->skip_default_update = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".skipfetchall\"))\n+\telse if (!strcmp(subkey, \"skipfetchall\"))\n \t\tremote->skip_default_update = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".prune\"))\n+\telse if (!strcmp(subkey, \"prune\"))\n \t\tremote->prune = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".url\")) {\n+\telse if (!strcmp(subkey, \"url\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_url(remote, v);\n-\t} else if (!strcmp(subkey, \".pushurl\")) {\n+\t} else if (!strcmp(subkey, \"pushurl\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_pushurl(remote, v);\n-\t} else if (!strcmp(subkey, \".push\")) {\n+\t} else if (!strcmp(subkey, \"push\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_push_refspec(remote, v);\n-\t} else if (!strcmp(subkey, \".fetch\")) {\n+\t} else if (!strcmp(subkey, \"fetch\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_fetch_refspec(remote, v);\n-\t} else if (!strcmp(subkey, \".receivepack\")) {\n+\t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n@@ -409,7 +407,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tremote->receivepack = v;\n \t\telse\n \t\t\terror(\"more than one receivepack given, using the first\");\n-\t} else if (!strcmp(subkey, \".uploadpack\")) {\n+\t} else if (!strcmp(subkey, \"uploadpack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n@@ -417,18 +415,18 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tremote->uploadpack = v;\n \t\telse\n \t\t\terror(\"more than one uploadpack given, using the first\");\n-\t} else if (!strcmp(subkey, \".tagopt\")) {\n+\t} else if (!strcmp(subkey, \"tagopt\")) {\n \t\tif (!strcmp(value, \"--no-tags\"))\n \t\t\tremote->fetch_tags = -1;\n \t\telse if (!strcmp(value, \"--tags\"))\n \t\t\tremote->fetch_tags = 2;\n-\t} else if (!strcmp(subkey, \".proxy\")) {\n+\t} else if (!strcmp(subkey, \"proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n-\t} else if (!strcmp(subkey, \".proxyauthmethod\")) {\n+\t} else if (!strcmp(subkey, \"proxyauthmethod\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n \t\t\t\t\t key, value);\n-\t} else if (!strcmp(subkey, \".vcs\")) {\n+\t} else if (!strcmp(subkey, \"vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\n \treturn 0;\ndiff --git a/remote.h b/remote.h\nindex 7a5ee77..c21fd37 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,7 +5,8 @@\n #include \"hashmap.h\"\n \n enum {\n-\tREMOTE_CONFIG = 1,\n+\tREMOTE_UNCONFIGURED = 0,\n+\tREMOTE_CONFIG,\n \tREMOTE_REMOTES,\n \tREMOTE_BRANCHES\n };\n\nThomas Gummerer (4):\n  remote: use parse_config_key\n  remote: simplify remote_is_configured()\n  remote: actually check if remote exits\n  remote: use remote_is_configured() for add and rename\n\n builtin/fetch.c   |  5 ++--\n builtin/remote.c  | 23 +++++++--------\n remote.c          | 84 +++++++++++++++++++++++--------------------------------\n remote.h          |  3 +-\n t/t5505-remote.sh | 36 ++++++++++++++++++++++++\n 5 files changed, 85 insertions(+), 66 deletions(-)\n\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278239","messageId":"1455575984-24348-2-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455575984-24348-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v2 1/4] remote: use parse_config_key","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T22:39:41Z","receivedAt":"2016-02-15T22:39:41Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"95b567c7 (\"use skip_prefix to avoid repeating strings\") transformed\ncalls using starts_with() and then skipping the length of the prefix to\nskip_prefix() calls.  In remote.c there are a few calls like:\n\n  if (starts_with(foo, \"bar\"))\n      foo += 3\n\nThese calls weren't touched by the aformentioned commit, but can be\nreplaced by calls to parse_config_key(), to simplify the code and\nclarify the intentions.  Do that.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n remote.c | 71 ++++++++++++++++++++++++++++++----------------------------------\n 1 file changed, 33 insertions(+), 38 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 02e698a..bcd8eb6 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -318,93 +318,88 @@ static void read_branches_file(struct remote *remote)\n static int handle_config(const char *key, const char *value, void *cb)\n {\n \tconst char *name;\n+\tint namelen;\n \tconst char *subkey;\n \tstruct remote *remote;\n \tstruct branch *branch;\n-\tif (starts_with(key, \"branch.\")) {\n-\t\tname = key + 7;\n-\t\tsubkey = strrchr(name, '.');\n-\t\tif (!subkey)\n+\tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tbranch = make_branch(name, subkey - name);\n-\t\tif (!strcmp(subkey, \".remote\")) {\n+\t\tbranch = make_branch(name, namelen);\n+\t\tif (!strcmp(subkey, \"remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n-\t\t} else if (!strcmp(subkey, \".pushremote\")) {\n+\t\t} else if (!strcmp(subkey, \"pushremote\")) {\n \t\t\treturn git_config_string(&branch->pushremote_name, key, value);\n-\t\t} else if (!strcmp(subkey, \".merge\")) {\n+\t\t} else if (!strcmp(subkey, \"merge\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_merge(branch, xstrdup(value));\n \t\t}\n \t\treturn 0;\n \t}\n-\tif (starts_with(key, \"url.\")) {\n+\tif (parse_config_key(key, \"url\", &name, &namelen, &subkey) >= 0) {\n \t\tstruct rewrite *rewrite;\n-\t\tname = key + 4;\n-\t\tsubkey = strrchr(name, '.');\n-\t\tif (!subkey)\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tif (!strcmp(subkey, \".insteadof\")) {\n-\t\t\trewrite = make_rewrite(&rewrites, name, subkey - name);\n+\t\tif (!strcmp(subkey, \"insteadof\")) {\n+\t\t\trewrite = make_rewrite(&rewrites, name, namelen);\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n-\t\t} else if (!strcmp(subkey, \".pushinsteadof\")) {\n-\t\t\trewrite = make_rewrite(&rewrites_push, name, subkey - name);\n+\t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n+\t\t\trewrite = make_rewrite(&rewrites_push, name, namelen);\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t}\n \t}\n \n-\tif (!starts_with(key,  \"remote.\"))\n+\tif (parse_config_key(key, \"remote\", &name, &namelen, &subkey) < 0)\n \t\treturn 0;\n-\tname = key + 7;\n \n \t/* Handle remote.* variables */\n-\tif (!strcmp(name, \"pushdefault\"))\n+\tif (!strcmp(subkey, \"pushdefault\"))\n \t\treturn git_config_string(&pushremote_name, key, value);\n \n \t/* Handle remote.<name>.* variables */\n-\tif (*name == '/') {\n+\tif (*(name ? name : subkey) == '/') {\n \t\twarning(\"Config remote shorthand cannot begin with '/': %s\",\n-\t\t\tname);\n+\t\t\tname ? name : subkey);\n \t\treturn 0;\n \t}\n-\tsubkey = strrchr(name, '.');\n-\tif (!subkey)\n+\tif (!name)\n \t\treturn 0;\n-\tremote = make_remote(name, subkey - name);\n+\tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n-\tif (!strcmp(subkey, \".mirror\"))\n+\tif (!strcmp(subkey, \"mirror\"))\n \t\tremote->mirror = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".skipdefaultupdate\"))\n+\telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n \t\tremote->skip_default_update = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".skipfetchall\"))\n+\telse if (!strcmp(subkey, \"skipfetchall\"))\n \t\tremote->skip_default_update = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".prune\"))\n+\telse if (!strcmp(subkey, \"prune\"))\n \t\tremote->prune = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".url\")) {\n+\telse if (!strcmp(subkey, \"url\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_url(remote, v);\n-\t} else if (!strcmp(subkey, \".pushurl\")) {\n+\t} else if (!strcmp(subkey, \"pushurl\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_pushurl(remote, v);\n-\t} else if (!strcmp(subkey, \".push\")) {\n+\t} else if (!strcmp(subkey, \"push\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_push_refspec(remote, v);\n-\t} else if (!strcmp(subkey, \".fetch\")) {\n+\t} else if (!strcmp(subkey, \"fetch\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_fetch_refspec(remote, v);\n-\t} else if (!strcmp(subkey, \".receivepack\")) {\n+\t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n@@ -412,7 +407,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tremote->receivepack = v;\n \t\telse\n \t\t\terror(\"more than one receivepack given, using the first\");\n-\t} else if (!strcmp(subkey, \".uploadpack\")) {\n+\t} else if (!strcmp(subkey, \"uploadpack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n@@ -420,18 +415,18 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tremote->uploadpack = v;\n \t\telse\n \t\t\terror(\"more than one uploadpack given, using the first\");\n-\t} else if (!strcmp(subkey, \".tagopt\")) {\n+\t} else if (!strcmp(subkey, \"tagopt\")) {\n \t\tif (!strcmp(value, \"--no-tags\"))\n \t\t\tremote->fetch_tags = -1;\n \t\telse if (!strcmp(value, \"--tags\"))\n \t\t\tremote->fetch_tags = 2;\n-\t} else if (!strcmp(subkey, \".proxy\")) {\n+\t} else if (!strcmp(subkey, \"proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n-\t} else if (!strcmp(subkey, \".proxyauthmethod\")) {\n+\t} else if (!strcmp(subkey, \"proxyauthmethod\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n \t\t\t\t\t key, value);\n-\t} else if (!strcmp(subkey, \".vcs\")) {\n+\t} else if (!strcmp(subkey, \"vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\n \treturn 0;\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278235","messageId":"1455575984-24348-3-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455575984-24348-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v2 2/4] remote: simplify remote_is_configured()","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T22:39:42Z","receivedAt":"2016-02-15T22:39:42Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The remote_is_configured() function allows checking whether a remote\nexists or not.  The function however only works if remote_get() wasn't\ncalled before calling it.  In addition, it only checks the configuration\nfor remotes, but not remotes or branches files.\n\nMake use of the origin member of struct remote instead, which indicates\nwhere the remote comes from.  It will be set to some value if the remote\nis configured in any file in the repository, but is initialized to 0 if\nthe remote is only created in make_remote().\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/fetch.c  |  5 ++---\n builtin/remote.c | 12 ++++++------\n remote.c         | 13 ++-----------\n remote.h         |  3 ++-\n 4 files changed, 12 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8e74213..8121830 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1016,10 +1016,9 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n \n \tgit_config(get_remote_group, &g);\n \tif (list->nr == prev_nr) {\n-\t\tstruct remote *remote;\n-\t\tif (!remote_is_configured(name))\n+\t\tstruct remote *remote = remote_get(name);\n+\t\tif (!remote_is_configured(remote))\n \t\t\treturn 0;\n-\t\tremote = remote_get(name);\n \t\tstring_list_append(list, remote->name);\n \t}\n \treturn 1;\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 2b2ff9b..d966262 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1441,9 +1441,9 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \n \tstrbuf_addf(&key, \"remote.%s.fetch\", remotename);\n \n-\tif (!remote_is_configured(remotename))\n-\t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n+\tif (!remote_is_configured(remote))\n+\t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {\n \t\tstrbuf_release(&key);\n@@ -1498,9 +1498,9 @@ static int get_url(int argc, const char **argv)\n \n \tremotename = argv[0];\n \n-\tif (!remote_is_configured(remotename))\n-\t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n+\tif (!remote_is_configured(remote))\n+\t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \turl_nr = 0;\n \tif (push_mode) {\n@@ -1566,9 +1566,9 @@ static int set_url(int argc, const char **argv)\n \tif (delete_mode)\n \t\toldurl = newurl;\n \n-\tif (!remote_is_configured(remotename))\n-\t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n+\tif (!remote_is_configured(remote))\n+\t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (push_mode) {\n \t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\ndiff --git a/remote.c b/remote.c\nindex bcd8eb6..d10ae00 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -713,18 +713,9 @@ struct remote *pushremote_get(const char *name)\n \treturn remote_get_1(name, pushremote_for_branch);\n }\n \n-int remote_is_configured(const char *name)\n+int remote_is_configured(struct remote *remote)\n {\n-\tstruct remotes_hash_key lookup;\n-\tstruct hashmap_entry lookup_entry;\n-\tread_config();\n-\n-\tinit_remotes_hash();\n-\tlookup.str = name;\n-\tlookup.len = strlen(name);\n-\thashmap_entry_init(&lookup_entry, memhash(name, lookup.len));\n-\n-\treturn hashmap_get(&remotes_hash, &lookup_entry, &lookup) != NULL;\n+\treturn remote && remote->origin;\n }\n \n int for_each_remote(each_remote_fn fn, void *priv)\ndiff --git a/remote.h b/remote.h\nindex 4fd7a0f..c21fd37 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,6 +5,7 @@\n #include \"hashmap.h\"\n \n enum {\n+\tREMOTE_UNCONFIGURED = 0,\n \tREMOTE_CONFIG,\n \tREMOTE_REMOTES,\n \tREMOTE_BRANCHES\n@@ -59,7 +60,7 @@ struct remote {\n \n struct remote *remote_get(const char *name);\n struct remote *pushremote_get(const char *name);\n-int remote_is_configured(const char *name);\n+int remote_is_configured(struct remote *remote);\n \n typedef int each_remote_fn(struct remote *remote, void *priv);\n int for_each_remote(each_remote_fn fn, void *priv);\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278238","messageId":"1455575984-24348-4-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455575984-24348-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v2 3/4] remote: actually check if remote exits","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T22:39:43Z","receivedAt":"2016-02-15T22:39:43Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When converting the git remote command to a builtin in 211c89 (\"Make\ngit-remote a builtin\"), a few calls to check if a remote exists were\nconverted from:\n       if (!exists $remote->{$name}) {\n       \t  [...]\nto:\n       remote = remote_get(argv[1]);\n       if (!remote)\n          [...]\n\nThe new check is not quite correct, because remote_get() never returns\nNULL if a name is given.  This leaves us with the somewhat cryptic error\nmessage \"error: Could not remove config section 'remote.test'\", if we\nare trying to remove a remote that does not exist, or a similar error if\nwe try to rename a remote.\n\nUse the remote_is_configured() function to check whether the remote\nactually exists, and die with a more sensible error message (\"No such\nremote: $remotename\") instead if it doesn't.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/remote.c  |  4 ++--\n t/t5505-remote.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d966262..981c487 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -634,7 +634,7 @@ static int mv(int argc, const char **argv)\n \trename.remote_branches = &remote_branches;\n \n \toldremote = remote_get(rename.old);\n-\tif (!oldremote)\n+\tif (!remote_is_configured(oldremote))\n \t\tdie(_(\"No such remote: %s\"), rename.old);\n \n \tif (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)\n@@ -773,7 +773,7 @@ static int rm(int argc, const char **argv)\n \t\tusage_with_options(builtin_remote_rm_usage, options);\n \n \tremote = remote_get(argv[1]);\n-\tif (!remote)\n+\tif (!remote_is_configured(remote))\n \t\tdie(_(\"No such remote: %s\"), argv[1]);\n \n \tknown_remotes.to_delete = remote;\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 1a8e3b8..f1d073f 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -139,6 +139,24 @@ test_expect_success 'remove remote protects local branches' '\n \t)\n '\n \n+test_expect_success 'remove errors out early when deleting non-existent branch' '\n+\t(\n+\t\tcd test &&\n+\t\techo \"fatal: No such remote: foo\" >expect &&\n+\t\ttest_must_fail git remote rm foo 2>actual &&\n+\t\ttest_i18ncmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'rename errors out early when deleting non-existent branch' '\n+\t(\n+\t\tcd test &&\n+\t\techo \"fatal: No such remote: foo\" >expect &&\n+\t\ttest_must_fail git remote rename foo bar 2>actual &&\n+\t\ttest_i18ncmp expect actual\n+\t)\n+'\n+\n cat >test/expect <<EOF\n * remote origin\n   Fetch URL: $(pwd)/one\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278236","messageId":"1455575984-24348-5-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455575984-24348-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v2 4/4] remote: use remote_is_configured() for add and rename","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T22:39:44Z","receivedAt":"2016-02-15T22:39:44Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Both remote add and remote rename use a slightly different hand-rolled\ncheck if the remote exits.  The hand-rolled check may have some subtle\ncases in which it might fail to detect when a remote already exists.\nOne such case was fixed in fb86e32 (\"git remote: allow adding remotes\nagreeing with url.<...>.insteadOf\").  Another case is when a remote is\nconfigured as follows:\n\n  [remote \"foo\"]\n    vcs = bar\n\nIf we try to run `git remote add foo bar` with the above remote\nconfiguration, git segfaults.  This change fixes it.\n\nIn addition, git remote rename $existing foo with the configuration for\nfoo as above silently succeeds, even though foo already exists,\nmodifying its configuration.  With this patch it fails with \"remote foo\nalready exists\".\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/remote.c  |  7 ++-----\n t/t5505-remote.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 981c487..bd57f1b 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -186,10 +186,7 @@ static int add(int argc, const char **argv)\n \turl = argv[1];\n \n \tremote = remote_get(name);\n-\tif (remote && (remote->url_nr > 1 ||\n-\t\t\t(strcmp(name, remote->url[0]) &&\n-\t\t\t\tstrcmp(url, remote->url[0])) ||\n-\t\t\tremote->fetch_refspec_nr))\n+\tif (remote_is_configured(remote))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n \tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n@@ -641,7 +638,7 @@ static int mv(int argc, const char **argv)\n \t\treturn migrate_file(oldremote);\n \n \tnewremote = remote_get(rename.new);\n-\tif (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))\n+\tif (remote_is_configured(newremote))\n \t\tdie(_(\"remote %s already exists.\"), rename.new);\n \n \tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex f1d073f..142ae62 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n \t)\n '\n \n+test_expect_success 'add existing foreign_vcs remote' '\n+\tgit config --add remote.foo.vcs \"bar\" &&\n+\ttest_when_finished git remote rm foo &&\n+\techo \"fatal: remote foo already exists.\" >expect &&\n+\ttest_must_fail git remote add foo bar 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success 'add existing foreign_vcs remote' '\n+\tgit config --add remote.foo.vcs \"bar\" &&\n+\tgit config --add remote.bar.vcs \"bar\" &&\n+\ttest_when_finished git remote rm foo &&\n+\ttest_when_finished git remote rm bar &&\n+\techo \"fatal: remote bar already exists.\" >expect &&\n+\ttest_must_fail git remote rename foo bar 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >test/expect <<EOF\n * remote origin\n   Fetch URL: $(pwd)/one\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278240","messageId":"CAPig+cTH+S67_T=O58E_x--ZhawQEZKjCmK8G+unzm_8f2w8eA@mail.gmail.com","threadId":"41419","inReplyTo":"1455575984-24348-5-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-15T22:52:14Z","receivedAt":"2016-02-15T22:52:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 15, 2016 at 5:39 PM, Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> Both remote add and remote rename use a slightly different hand-rolled\n> check if the remote exits.  The hand-rolled check may have some subtle\n> cases in which it might fail to detect when a remote already exists.\n> One such case was fixed in fb86e32 (\"git remote: allow adding remotes\n> agreeing with url.<...>.insteadOf\").  Another case is when a remote is\n> configured as follows:\n> [...]\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n> +test_expect_success 'add existing foreign_vcs remote' '\n> +       git config --add remote.foo.vcs \"bar\" &&\n> +       git config --add remote.bar.vcs \"bar\" &&\n> +       test_when_finished git remote rm foo &&\n> +       test_when_finished git remote rm bar &&\n\nNit: If the second git-config fails, then none of the cleanup will\nhappen. You'd either want to re-order them like this:\n\n    git config --add remote.foo.vcs \"bar\" &&\n    test_when_finished git remote rm foo &&\n    git config --add remote.bar.vcs \"bar\" &&\n    test_when_finished git remote rm bar &&\n\nor this:\n\n    test_when_finished git remote rm foo &&\n    git config --add remote.foo.vcs \"bar\" &&\n    test_when_finished git remote rm bar &&\n    git config --add remote.bar.vcs \"bar\" &&\n\nor this:\n\n    test_when_finished git remote rm foo &&\n    test_when_finished git remote rm bar &&\n    git config --add remote.foo.vcs \"bar\" &&\n    git config --add remote.bar.vcs \"bar\" &&\n\nProbably not worth a re-roll, though.\n\n> +       echo \"fatal: remote bar already exists.\" >expect &&\n> +       test_must_fail git remote rename foo bar 2>actual &&\n> +       test_i18ncmp expect actual\n> +'\n"},{"id":"278242","messageId":"20160215230443.GB30631@sigill.intra.peff.net","threadId":"41419","inReplyTo":"1455575984-24348-2-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v2 1/4] remote: use parse_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-15T23:04:43Z","receivedAt":"2016-02-15T23:04:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:\n\n> -\tif (!starts_with(key,  \"remote.\"))\n> +\tif (parse_config_key(key, \"remote\", &name, &namelen, &subkey) < 0)\n>  \t\treturn 0;\n> -\tname = key + 7;\n>  \n>  \t/* Handle remote.* variables */\n> -\tif (!strcmp(name, \"pushdefault\"))\n> +\tif (!strcmp(subkey, \"pushdefault\"))\n>  \t\treturn git_config_string(&pushremote_name, key, value);\n\nI think this needs to become:\n\n  if (!name && !strcmp(subkey, \"pushdefault\"))\n\nso that we do not match \"remote.foo.pushdefault\", which is nonsense. The\noriginal avoided it by conflating \"name\" and \"subkey\" at various points,\nand not parsing out the subkey until later. Making that more explicit is\none of the things that I think is improved by your patch. :)\n\n>  \t/* Handle remote.<name>.* variables */\n> -\tif (*name == '/') {\n> +\tif (*(name ? name : subkey) == '/') {\n>  \t\twarning(\"Config remote shorthand cannot begin with '/': %s\",\n> -\t\t\tname);\n> +\t\t\tname ? name : subkey);\n>  \t\treturn 0;\n>  \t}\n> -\tsubkey = strrchr(name, '.');\n> -\tif (!subkey)\n> +\tif (!name)\n>  \t\treturn 0;\n\nI think you can bump the \"if (!name)\" check earlier. If it is empty, we\nknow that it does not start with \"/\". And then you can avoid the extra\nNULL-checks.\n\nThe rest of the patch looks good to me. I hadn't realized initially that\nall of the subkey compares would become \"foo\" and not \".foo\". That makes\nthe diff noisier, but IMHO the result is much better.\n\n-Peff\n"},{"id":"278243","messageId":"20160215230920.GC30631@sigill.intra.peff.net","threadId":"41419","inReplyTo":"CAPig+cTH+S67_T=O58E_x--ZhawQEZKjCmK8G+unzm_8f2w8eA@mail.gmail.com","subject":"Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-15T23:09:21Z","receivedAt":"2016-02-15T23:09:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 05:52:14PM -0500, Eric Sunshine wrote:\n\n> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> > @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n> > +test_expect_success 'add existing foreign_vcs remote' '\n> > +       git config --add remote.foo.vcs \"bar\" &&\n> > +       git config --add remote.bar.vcs \"bar\" &&\n> > +       test_when_finished git remote rm foo &&\n> > +       test_when_finished git remote rm bar &&\n> \n> Nit: If the second git-config fails, then none of the cleanup will\n> happen. You'd either want to re-order them like this:\n> \n>     git config --add remote.foo.vcs \"bar\" &&\n>     test_when_finished git remote rm foo &&\n>     git config --add remote.bar.vcs \"bar\" &&\n>     test_when_finished git remote rm bar &&\n\nGood catch. Do we actually care about \"--add\" here at all? We do not\nexpect these remotes to have any existing config, I think. So would:\n\n  test_config remote.foo.vcs bar &&\n  test_config remote.bar.vcs bar\n\ndo? I guess technically the failing \"git remote rename\" could introduce\nextra config that is not cleaned up by those invocations, and we need to\n\"git remote rm\" to get a clean slate, but I don't think that is the case\nnow (and it does not seem likely to become so in the future).\n\n-Peff\n"},{"id":"278250","messageId":"20160216001327.GA1831@hank","threadId":"41419","inReplyTo":"20160215230443.GB30631@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/4] remote: use parse_config_key","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T00:13:27Z","receivedAt":"2016-02-16T00:13:27Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/15, Jeff King wrote:\n> On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:\n>\n> > -\tif (!starts_with(key,  \"remote.\"))\n> > +\tif (parse_config_key(key, \"remote\", &name, &namelen, &subkey) < 0)\n> >  \t\treturn 0;\n> > -\tname = key + 7;\n> >\n> >  \t/* Handle remote.* variables */\n> > -\tif (!strcmp(name, \"pushdefault\"))\n> > +\tif (!strcmp(subkey, \"pushdefault\"))\n> >  \t\treturn git_config_string(&pushremote_name, key, value);\n>\n> I think this needs to become:\n>\n>   if (!name && !strcmp(subkey, \"pushdefault\"))\n>\n> so that we do not match \"remote.foo.pushdefault\", which is nonsense. The\n> original avoided it by conflating \"name\" and \"subkey\" at various points,\n> and not parsing out the subkey until later. Making that more explicit is\n> one of the things that I think is improved by your patch. :)\n\nGood catch.  I'll fix this in the re-roll.\n\n> >  \t/* Handle remote.<name>.* variables */\n> > -\tif (*name == '/') {\n> > +\tif (*(name ? name : subkey) == '/') {\n> >  \t\twarning(\"Config remote shorthand cannot begin with '/': %s\",\n> > -\t\t\tname);\n> > +\t\t\tname ? name : subkey);\n> >  \t\treturn 0;\n> >  \t}\n> > -\tsubkey = strrchr(name, '.');\n> > -\tif (!subkey)\n> > +\tif (!name)\n> >  \t\treturn 0;\n>\n> I think you can bump the \"if (!name)\" check earlier. If it is empty, we\n> know that it does not start with \"/\". And then you can avoid the extra\n> NULL-checks.\n\nThanks, will change that.\n\n> The rest of the patch looks good to me. I hadn't realized initially that\n> all of the subkey compares would become \"foo\" and not \".foo\". That makes\n> the diff noisier, but IMHO the result is much better.\n>\n> -Peff\n\n--\nThomas\n"},{"id":"278251","messageId":"20160216001609.GB1831@hank","threadId":"41419","inReplyTo":"20160215230920.GC30631@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T00:16:09Z","receivedAt":"2016-02-16T00:16:09Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/15, Jeff King wrote:\n> On Mon, Feb 15, 2016 at 05:52:14PM -0500, Eric Sunshine wrote:\n>\n> > > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> > > @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n> > > +test_expect_success 'add existing foreign_vcs remote' '\n> > > +       git config --add remote.foo.vcs \"bar\" &&\n> > > +       git config --add remote.bar.vcs \"bar\" &&\n> > > +       test_when_finished git remote rm foo &&\n> > > +       test_when_finished git remote rm bar &&\n> >\n> > Nit: If the second git-config fails, then none of the cleanup will\n> > happen. You'd either want to re-order them like this:\n> >\n> >     git config --add remote.foo.vcs \"bar\" &&\n> >     test_when_finished git remote rm foo &&\n> >     git config --add remote.bar.vcs \"bar\" &&\n> >     test_when_finished git remote rm bar &&\n>\n> Good catch. Do we actually care about \"--add\" here at all? We do not\n> expect these remotes to have any existing config, I think. So would:\n>\n>   test_config remote.foo.vcs bar &&\n>   test_config remote.bar.vcs bar\n>\n> do? I guess technically the failing \"git remote rename\" could introduce\n> extra config that is not cleaned up by those invocations, and we need to\n> \"git remote rm\" to get a clean slate, but I don't think that is the case\n> now (and it does not seem likely to become so in the future).\n\nGood point, I think the test_config is indeed enough.  Thanks, both,\nwill fix in the re-roll.\n\n> -Peff\n\n--\nThomas\n"},{"id":"278287","messageId":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455575984-24348-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v3 0/4] git remote improvements","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T09:47:48Z","receivedAt":"2016-02-16T09:47:48Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Previous rounds are at $gmane/286214 and $gmane/286278.  Thanks to\nPeff and Eric for the reviews on the previous round.\n\nThis version changes two checks in [1/4], and uses test_config in the\ntests instead of calling git config directly.\n\nInterdiff below:\n\ndiff --git a/remote.c b/remote.c\nindex d10ae00..f001681 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -358,17 +358,17 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\treturn 0;\n \n \t/* Handle remote.* variables */\n-\tif (!strcmp(subkey, \"pushdefault\"))\n+\tif (!name && !strcmp(subkey, \"pushdefault\"))\n \t\treturn git_config_string(&pushremote_name, key, value);\n \n+\tif (!name)\n+\t\treturn 0;\n \t/* Handle remote.<name>.* variables */\n-\tif (*(name ? name : subkey) == '/') {\n+\tif (*name == '/') {\n \t\twarning(\"Config remote shorthand cannot begin with '/': %s\",\n-\t\t\tname ? name : subkey);\n+\t\t\tname);\n \t\treturn 0;\n \t}\n-\tif (!name)\n-\t\treturn 0;\n \tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n \tif (!strcmp(subkey, \"mirror\"))\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 142ae62..94079a0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -158,18 +158,15 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n '\n \n test_expect_success 'add existing foreign_vcs remote' '\n-\tgit config --add remote.foo.vcs \"bar\" &&\n-\ttest_when_finished git remote rm foo &&\n+\ttest_config remote.foo.vcs bar &&\n \techo \"fatal: remote foo already exists.\" >expect &&\n \ttest_must_fail git remote add foo bar 2>actual &&\n \ttest_i18ncmp expect actual\n '\n \n test_expect_success 'add existing foreign_vcs remote' '\n-\tgit config --add remote.foo.vcs \"bar\" &&\n-\tgit config --add remote.bar.vcs \"bar\" &&\n-\ttest_when_finished git remote rm foo &&\n-\ttest_when_finished git remote rm bar &&\n+\ttest_config remote.foo.vcs bar &&\n+\ttest_config remote.bar.vcs bar &&\n \techo \"fatal: remote bar already exists.\" >expect &&\n \ttest_must_fail git remote rename foo bar 2>actual &&\n \ttest_i18ncmp expect actual\n\n\nThomas Gummerer (4):\n  remote: use parse_config_key\n  remote: simplify remote_is_configured()\n  remote: actually check if remote exits\n  remote: use remote_is_configured() for add and rename\n\n builtin/fetch.c   |  5 ++--\n builtin/remote.c  | 23 +++++++---------\n remote.c          | 82 +++++++++++++++++++++++--------------------------------\n remote.h          |  3 +-\n t/t5505-remote.sh | 33 ++++++++++++++++++++++\n 5 files changed, 81 insertions(+), 65 deletions(-)\n\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278288","messageId":"1455616072-20838-2-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v3 1/4] remote: use parse_config_key","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T09:47:49Z","receivedAt":"2016-02-16T09:47:49Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"95b567c7 (\"use skip_prefix to avoid repeating strings\") transformed\ncalls using starts_with() and then skipping the length of the prefix to\nskip_prefix() calls.  In remote.c there are a few calls like:\n\n  if (starts_with(foo, \"bar\"))\n      foo += 3\n\nThese calls weren't touched by the aformentioned commit, but can be\nreplaced by calls to parse_config_key(), to simplify the code and\nclarify the intentions.  Do that.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n remote.c | 69 ++++++++++++++++++++++++++++++----------------------------------\n 1 file changed, 32 insertions(+), 37 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 02e698a..c7d67c2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -318,93 +318,88 @@ static void read_branches_file(struct remote *remote)\n static int handle_config(const char *key, const char *value, void *cb)\n {\n \tconst char *name;\n+\tint namelen;\n \tconst char *subkey;\n \tstruct remote *remote;\n \tstruct branch *branch;\n-\tif (starts_with(key, \"branch.\")) {\n-\t\tname = key + 7;\n-\t\tsubkey = strrchr(name, '.');\n-\t\tif (!subkey)\n+\tif (parse_config_key(key, \"branch\", &name, &namelen, &subkey) >= 0) {\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tbranch = make_branch(name, subkey - name);\n-\t\tif (!strcmp(subkey, \".remote\")) {\n+\t\tbranch = make_branch(name, namelen);\n+\t\tif (!strcmp(subkey, \"remote\")) {\n \t\t\treturn git_config_string(&branch->remote_name, key, value);\n-\t\t} else if (!strcmp(subkey, \".pushremote\")) {\n+\t\t} else if (!strcmp(subkey, \"pushremote\")) {\n \t\t\treturn git_config_string(&branch->pushremote_name, key, value);\n-\t\t} else if (!strcmp(subkey, \".merge\")) {\n+\t\t} else if (!strcmp(subkey, \"merge\")) {\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_merge(branch, xstrdup(value));\n \t\t}\n \t\treturn 0;\n \t}\n-\tif (starts_with(key, \"url.\")) {\n+\tif (parse_config_key(key, \"url\", &name, &namelen, &subkey) >= 0) {\n \t\tstruct rewrite *rewrite;\n-\t\tname = key + 4;\n-\t\tsubkey = strrchr(name, '.');\n-\t\tif (!subkey)\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tif (!strcmp(subkey, \".insteadof\")) {\n-\t\t\trewrite = make_rewrite(&rewrites, name, subkey - name);\n+\t\tif (!strcmp(subkey, \"insteadof\")) {\n+\t\t\trewrite = make_rewrite(&rewrites, name, namelen);\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n-\t\t} else if (!strcmp(subkey, \".pushinsteadof\")) {\n-\t\t\trewrite = make_rewrite(&rewrites_push, name, subkey - name);\n+\t\t} else if (!strcmp(subkey, \"pushinsteadof\")) {\n+\t\t\trewrite = make_rewrite(&rewrites_push, name, namelen);\n \t\t\tif (!value)\n \t\t\t\treturn config_error_nonbool(key);\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t}\n \t}\n \n-\tif (!starts_with(key,  \"remote.\"))\n+\tif (parse_config_key(key, \"remote\", &name, &namelen, &subkey) < 0)\n \t\treturn 0;\n-\tname = key + 7;\n \n \t/* Handle remote.* variables */\n-\tif (!strcmp(name, \"pushdefault\"))\n+\tif (!name && !strcmp(subkey, \"pushdefault\"))\n \t\treturn git_config_string(&pushremote_name, key, value);\n \n+\tif (!name)\n+\t\treturn 0;\n \t/* Handle remote.<name>.* variables */\n \tif (*name == '/') {\n \t\twarning(\"Config remote shorthand cannot begin with '/': %s\",\n \t\t\tname);\n \t\treturn 0;\n \t}\n-\tsubkey = strrchr(name, '.');\n-\tif (!subkey)\n-\t\treturn 0;\n-\tremote = make_remote(name, subkey - name);\n+\tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n-\tif (!strcmp(subkey, \".mirror\"))\n+\tif (!strcmp(subkey, \"mirror\"))\n \t\tremote->mirror = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".skipdefaultupdate\"))\n+\telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n \t\tremote->skip_default_update = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".skipfetchall\"))\n+\telse if (!strcmp(subkey, \"skipfetchall\"))\n \t\tremote->skip_default_update = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".prune\"))\n+\telse if (!strcmp(subkey, \"prune\"))\n \t\tremote->prune = git_config_bool(key, value);\n-\telse if (!strcmp(subkey, \".url\")) {\n+\telse if (!strcmp(subkey, \"url\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_url(remote, v);\n-\t} else if (!strcmp(subkey, \".pushurl\")) {\n+\t} else if (!strcmp(subkey, \"pushurl\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_pushurl(remote, v);\n-\t} else if (!strcmp(subkey, \".push\")) {\n+\t} else if (!strcmp(subkey, \"push\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_push_refspec(remote, v);\n-\t} else if (!strcmp(subkey, \".fetch\")) {\n+\t} else if (!strcmp(subkey, \"fetch\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_fetch_refspec(remote, v);\n-\t} else if (!strcmp(subkey, \".receivepack\")) {\n+\t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n@@ -412,7 +407,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tremote->receivepack = v;\n \t\telse\n \t\t\terror(\"more than one receivepack given, using the first\");\n-\t} else if (!strcmp(subkey, \".uploadpack\")) {\n+\t} else if (!strcmp(subkey, \"uploadpack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n@@ -420,18 +415,18 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tremote->uploadpack = v;\n \t\telse\n \t\t\terror(\"more than one uploadpack given, using the first\");\n-\t} else if (!strcmp(subkey, \".tagopt\")) {\n+\t} else if (!strcmp(subkey, \"tagopt\")) {\n \t\tif (!strcmp(value, \"--no-tags\"))\n \t\t\tremote->fetch_tags = -1;\n \t\telse if (!strcmp(value, \"--tags\"))\n \t\t\tremote->fetch_tags = 2;\n-\t} else if (!strcmp(subkey, \".proxy\")) {\n+\t} else if (!strcmp(subkey, \"proxy\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy,\n \t\t\t\t\t key, value);\n-\t} else if (!strcmp(subkey, \".proxyauthmethod\")) {\n+\t} else if (!strcmp(subkey, \"proxyauthmethod\")) {\n \t\treturn git_config_string((const char **)&remote->http_proxy_authmethod,\n \t\t\t\t\t key, value);\n-\t} else if (!strcmp(subkey, \".vcs\")) {\n+\t} else if (!strcmp(subkey, \"vcs\")) {\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\n \treturn 0;\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278289","messageId":"1455616072-20838-3-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v3 2/4] remote: simplify remote_is_configured()","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T09:47:50Z","receivedAt":"2016-02-16T09:47:50Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"The remote_is_configured() function allows checking whether a remote\nexists or not.  The function however only works if remote_get() wasn't\ncalled before calling it.  In addition, it only checks the configuration\nfor remotes, but not remotes or branches files.\n\nMake use of the origin member of struct remote instead, which indicates\nwhere the remote comes from.  It will be set to some value if the remote\nis configured in any file in the repository, but is initialized to 0 if\nthe remote is only created in make_remote().\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/fetch.c  |  5 ++---\n builtin/remote.c | 12 ++++++------\n remote.c         | 13 ++-----------\n remote.h         |  3 ++-\n 4 files changed, 12 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8e74213..8121830 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1016,10 +1016,9 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n \n \tgit_config(get_remote_group, &g);\n \tif (list->nr == prev_nr) {\n-\t\tstruct remote *remote;\n-\t\tif (!remote_is_configured(name))\n+\t\tstruct remote *remote = remote_get(name);\n+\t\tif (!remote_is_configured(remote))\n \t\t\treturn 0;\n-\t\tremote = remote_get(name);\n \t\tstring_list_append(list, remote->name);\n \t}\n \treturn 1;\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 2b2ff9b..d966262 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1441,9 +1441,9 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \n \tstrbuf_addf(&key, \"remote.%s.fetch\", remotename);\n \n-\tif (!remote_is_configured(remotename))\n-\t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n+\tif (!remote_is_configured(remote))\n+\t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {\n \t\tstrbuf_release(&key);\n@@ -1498,9 +1498,9 @@ static int get_url(int argc, const char **argv)\n \n \tremotename = argv[0];\n \n-\tif (!remote_is_configured(remotename))\n-\t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n+\tif (!remote_is_configured(remote))\n+\t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \turl_nr = 0;\n \tif (push_mode) {\n@@ -1566,9 +1566,9 @@ static int set_url(int argc, const char **argv)\n \tif (delete_mode)\n \t\toldurl = newurl;\n \n-\tif (!remote_is_configured(remotename))\n-\t\tdie(_(\"No such remote '%s'\"), remotename);\n \tremote = remote_get(remotename);\n+\tif (!remote_is_configured(remote))\n+\t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (push_mode) {\n \t\tstrbuf_addf(&name_buf, \"remote.%s.pushurl\", remotename);\ndiff --git a/remote.c b/remote.c\nindex c7d67c2..f001681 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -713,18 +713,9 @@ struct remote *pushremote_get(const char *name)\n \treturn remote_get_1(name, pushremote_for_branch);\n }\n \n-int remote_is_configured(const char *name)\n+int remote_is_configured(struct remote *remote)\n {\n-\tstruct remotes_hash_key lookup;\n-\tstruct hashmap_entry lookup_entry;\n-\tread_config();\n-\n-\tinit_remotes_hash();\n-\tlookup.str = name;\n-\tlookup.len = strlen(name);\n-\thashmap_entry_init(&lookup_entry, memhash(name, lookup.len));\n-\n-\treturn hashmap_get(&remotes_hash, &lookup_entry, &lookup) != NULL;\n+\treturn remote && remote->origin;\n }\n \n int for_each_remote(each_remote_fn fn, void *priv)\ndiff --git a/remote.h b/remote.h\nindex 4fd7a0f..c21fd37 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,6 +5,7 @@\n #include \"hashmap.h\"\n \n enum {\n+\tREMOTE_UNCONFIGURED = 0,\n \tREMOTE_CONFIG,\n \tREMOTE_REMOTES,\n \tREMOTE_BRANCHES\n@@ -59,7 +60,7 @@ struct remote {\n \n struct remote *remote_get(const char *name);\n struct remote *pushremote_get(const char *name);\n-int remote_is_configured(const char *name);\n+int remote_is_configured(struct remote *remote);\n \n typedef int each_remote_fn(struct remote *remote, void *priv);\n int for_each_remote(each_remote_fn fn, void *priv);\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278290","messageId":"1455616072-20838-4-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v3 3/4] remote: actually check if remote exits","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T09:47:51Z","receivedAt":"2016-02-16T09:47:51Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"When converting the git remote command to a builtin in 211c89 (\"Make\ngit-remote a builtin\"), a few calls to check if a remote exists were\nconverted from:\n       if (!exists $remote->{$name}) {\n       \t  [...]\nto:\n       remote = remote_get(argv[1]);\n       if (!remote)\n          [...]\n\nThe new check is not quite correct, because remote_get() never returns\nNULL if a name is given.  This leaves us with the somewhat cryptic error\nmessage \"error: Could not remove config section 'remote.test'\", if we\nare trying to remove a remote that does not exist, or a similar error if\nwe try to rename a remote.\n\nUse the remote_is_configured() function to check whether the remote\nactually exists, and die with a more sensible error message (\"No such\nremote: $remotename\") instead if it doesn't.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/remote.c  |  4 ++--\n t/t5505-remote.sh | 18 ++++++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d966262..981c487 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -634,7 +634,7 @@ static int mv(int argc, const char **argv)\n \trename.remote_branches = &remote_branches;\n \n \toldremote = remote_get(rename.old);\n-\tif (!oldremote)\n+\tif (!remote_is_configured(oldremote))\n \t\tdie(_(\"No such remote: %s\"), rename.old);\n \n \tif (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)\n@@ -773,7 +773,7 @@ static int rm(int argc, const char **argv)\n \t\tusage_with_options(builtin_remote_rm_usage, options);\n \n \tremote = remote_get(argv[1]);\n-\tif (!remote)\n+\tif (!remote_is_configured(remote))\n \t\tdie(_(\"No such remote: %s\"), argv[1]);\n \n \tknown_remotes.to_delete = remote;\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 1a8e3b8..f1d073f 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -139,6 +139,24 @@ test_expect_success 'remove remote protects local branches' '\n \t)\n '\n \n+test_expect_success 'remove errors out early when deleting non-existent branch' '\n+\t(\n+\t\tcd test &&\n+\t\techo \"fatal: No such remote: foo\" >expect &&\n+\t\ttest_must_fail git remote rm foo 2>actual &&\n+\t\ttest_i18ncmp expect actual\n+\t)\n+'\n+\n+test_expect_success 'rename errors out early when deleting non-existent branch' '\n+\t(\n+\t\tcd test &&\n+\t\techo \"fatal: No such remote: foo\" >expect &&\n+\t\ttest_must_fail git remote rename foo bar 2>actual &&\n+\t\ttest_i18ncmp expect actual\n+\t)\n+'\n+\n cat >test/expect <<EOF\n * remote origin\n   Fetch URL: $(pwd)/one\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278291","messageId":"1455616072-20838-5-git-send-email-t.gummerer@gmail.com","threadId":"41419","inReplyTo":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH v3 4/4] remote: use remote_is_configured() for add and rename","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-16T09:47:52Z","receivedAt":"2016-02-16T09:47:52Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Both remote add and remote rename use a slightly different hand-rolled\ncheck if the remote exits.  The hand-rolled check may have some subtle\ncases in which it might fail to detect when a remote already exists.\nOne such case was fixed in fb86e32 (\"git remote: allow adding remotes\nagreeing with url.<...>.insteadOf\").  Another case is when a remote is\nconfigured as follows:\n\n  [remote \"foo\"]\n    vcs = bar\n\nIf we try to run `git remote add foo bar` with the above remote\nconfiguration, git segfaults.  This change fixes it.\n\nIn addition, git remote rename $existing foo with the configuration for\nfoo as above silently succeeds, even though foo already exists,\nmodifying its configuration.  With this patch it fails with \"remote foo\nalready exists\".\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n builtin/remote.c  |  7 ++-----\n t/t5505-remote.sh | 15 +++++++++++++++\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 981c487..bd57f1b 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -186,10 +186,7 @@ static int add(int argc, const char **argv)\n \turl = argv[1];\n \n \tremote = remote_get(name);\n-\tif (remote && (remote->url_nr > 1 ||\n-\t\t\t(strcmp(name, remote->url[0]) &&\n-\t\t\t\tstrcmp(url, remote->url[0])) ||\n-\t\t\tremote->fetch_refspec_nr))\n+\tif (remote_is_configured(remote))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n \tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n@@ -641,7 +638,7 @@ static int mv(int argc, const char **argv)\n \t\treturn migrate_file(oldremote);\n \n \tnewremote = remote_get(rename.new);\n-\tif (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))\n+\tif (remote_is_configured(newremote))\n \t\tdie(_(\"remote %s already exists.\"), rename.new);\n \n \tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex f1d073f..94079a0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -157,6 +157,21 @@ test_expect_success 'rename errors out early when deleting non-existent branch'\n \t)\n '\n \n+test_expect_success 'add existing foreign_vcs remote' '\n+\ttest_config remote.foo.vcs bar &&\n+\techo \"fatal: remote foo already exists.\" >expect &&\n+\ttest_must_fail git remote add foo bar 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n+test_expect_success 'add existing foreign_vcs remote' '\n+\ttest_config remote.foo.vcs bar &&\n+\ttest_config remote.bar.vcs bar &&\n+\techo \"fatal: remote bar already exists.\" >expect &&\n+\ttest_must_fail git remote rename foo bar 2>actual &&\n+\ttest_i18ncmp expect actual\n+'\n+\n cat >test/expect <<EOF\n * remote origin\n   Fetch URL: $(pwd)/one\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278361","messageId":"20160216155223.GA918@sigill.intra.peff.net","threadId":"41419","inReplyTo":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v3 0/4] git remote improvements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-16T15:52:23Z","receivedAt":"2016-02-16T15:52:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 16, 2016 at 10:47:48AM +0100, Thomas Gummerer wrote:\n\n> Previous rounds are at $gmane/286214 and $gmane/286278.  Thanks to\n> Peff and Eric for the reviews on the previous round.\n\nThis round looks good to me. Thanks for working on it!\n\nReviewed-by: Jeff King <peff@peff.net>\n\n-Peff\n"},{"id":"278392","messageId":"xmqqio1omj68.fsf@gitster.mtv.corp.google.com","threadId":"41419","inReplyTo":"1455616072-20838-1-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH v3 0/4] git remote improvements","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-16T21:36:47Z","receivedAt":"2016-02-16T21:36:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Previous rounds are at $gmane/286214 and $gmane/286278.  Thanks to\n> Peff and Eric for the reviews on the previous round.\n>\n> This version changes two checks in [1/4], and uses test_config in the\n> tests instead of calling git config directly.\n\nThanks; I agree that 1/4 is a noisy patch but the culprit of the\nnoisyness is that the old code, when doing tons of comparisons with\nfixed strings that begin with a dot, did not think it was crazy not\nto increment the \"subkey\" before doing so, and the resulting code is\nmuch easier to follow.\n\nThanks.\n"}]}