{"thread":{"id":"41416","subject":"[PATCH 0/4] git remote improvements","startedAt":"2016-02-15T17:42:26Z","lastAt":"2016-02-17T16:20:47Z","messageCount":18,"participants":["Thomas Gummerer","Jeff King","Eric Sunshine","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"278170","messageId":"1455558150-30267-1-git-send-email-t.gummerer@gmail.com","threadId":"41416","inReplyTo":null,"subject":"[PATCH 0/4] git remote improvements","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T17:42:26Z","receivedAt":"2016-02-15T17:42:26Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In builtin/remote.c there are a few cases which check whether a remote\nexists or not. These checks are however done inconsistently, and in a\nfew cases they don't check anything at all. This patch series tries\nto improve the situation.\n\nI stumbled upon this when I mistyped the remote name in git remote rm,\nand got a cryptic error message, and thought the other cases might be\nworth fixing as well.\n\nThere is one such check left in get_remote_ref_states, but I'm not\nquite sure what to do about that one. That function gets called in\ngit remote show/set-head/prune, where it might make sense for the user\nto provide a url, or a local repository, at least in the git remote\nshow case.\n\nI think we have the following options there:\n\n (1) just remove the check, as nobody seems to have noticed until now,\n and the check doesn't actually do anything (get_remote() is never\n called with NULL, so it will never return NULL). In this case\n git dies when a non-existent remote is encountered, e.g. git\n remote show nonexistent existent will die immediately, while git\n remote show existent nonexistent will show info about the\n existing remote, and die afterwards.\n \n (2) add an option not to die in the transport function, and let the\n caller of get_remote_ref_states handle the error i.e. show the\n error and keep going with the next remote.\n\n (3) anything I might be missing?\n\nThomas Gummerer (4):\n remote: use skip_prefix\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 | 22 +++++-----------------\n remote.h | 4 ++--\n t/t5505-remote.sh | 36 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 55 insertions(+), 35 deletions(-)\n\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278171","messageId":"1455558150-30267-2-git-send-email-t.gummerer@gmail.com","threadId":"41416","inReplyTo":"1455558150-30267-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 1/4] remote: use skip_prefix","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T17:42:27Z","receivedAt":"2016-02-15T17:42:27Z","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 commit mentioned above, but can\nbenefit from the same treatment to avoid magic numbers.\n\nSigned-off-by: Thomas Gummerer \n---\n remote.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 02e698a..4b5b576 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -321,8 +321,7 @@ static int handle_config(const char *key, const char *value, void *cb)\n \tconst char *subkey;\n \tstruct remote *remote;\n \tstruct branch *branch;\n-\tif (starts_with(key, \"branch.\")) {\n-\t\tname = key + 7;\n+\tif (skip_prefix(key, \"branch.\", &name)) {\n \t\tsubkey = strrchr(name, '.');\n \t\tif (!subkey)\n \t\t\treturn 0;\n@@ -338,9 +337,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t}\n \t\treturn 0;\n \t}\n-\tif (starts_with(key, \"url.\")) {\n+\tif (skip_prefix(key, \"url.\", &name)) {\n \t\tstruct rewrite *rewrite;\n-\t\tname = key + 4;\n \t\tsubkey = strrchr(name, '.');\n \t\tif (!subkey)\n \t\t\treturn 0;\n@@ -357,9 +355,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t}\n \t}\n \n-\tif (!starts_with(key, \"remote.\"))\n+\tif (!skip_prefix(key, \"remote.\", &name))\n \t\treturn 0;\n-\tname = key + 7;\n \n \t/* Handle remote.* variables */\n \tif (!strcmp(name, \"pushdefault\"))\n-- \n2.7.1.410.g6faf27b\n"},{"id":"278172","messageId":"1455558150-30267-3-git-send-email-t.gummerer@gmail.com","threadId":"41416","inReplyTo":"1455558150-30267-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 2/4] remote: simplify remote_is_configured()","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T17:42:28Z","receivedAt":"2016-02-15T17:42:28Z","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 \n---\n builtin/fetch.c | 5 ++---\n builtin/remote.c | 12 ++++++------\n remote.c | 13 ++-----------\n remote.h | 4 ++--\n 4 files changed, 12 insertions(+), 22 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 4b5b576..3a4ca9b 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -715,18 +715,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..7a5ee77 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -5,7 +5,7 @@\n #include \"hashmap.h\"\n \n enum {\n-\tREMOTE_CONFIG,\n+\tREMOTE_CONFIG = 1,\n \tREMOTE_REMOTES,\n \tREMOTE_BRANCHES\n };\n@@ -59,7 +59,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":"278173","messageId":"1455558150-30267-4-git-send-email-t.gummerer@gmail.com","threadId":"41416","inReplyTo":"1455558150-30267-1-git-send-email-t.gummerer@gmail.com","subject":"[PATCH 3/4] remote: actually check if remote exits","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T17:42:29Z","receivedAt":"2016-02-15T17:42:29Z","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 \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 <.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 \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 < 95b567c7 (\"use skip_prefix to avoid repeating strings\") transformed\n> calls using starts_with() and then skipping the length of the prefix to\n> skip_prefix() calls. In remote.c there are a few calls like:\n> \n> if (starts_with(foo, \"bar\"))\n> foo += 3\n> \n> These calls weren't touched by the commit mentioned above, but can\n> benefit from the same treatment to avoid magic numbers.\n\nThis is definitely an improvement, but I think we can actually go a step\nfurther here, and use parse_config_key. Like:\n\ndiff --git a/remote.c b/remote.c\nindex 21e4ec3..8d2c3ca 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -318,15 +318,14 @@ 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 (starts_with(key, \"branch\", &name, &namelen, &subkey)) {\n+\t\tif (!name)\n \t\t\treturn 0;\n-\t\tbranch = make_branch(name, subkey - name);\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\nand so on. The difference in lines of code isn't that great, but I think\nit makes the resulting code more obvious to read.\n\n-Peff\n"},{"id":"278180","messageId":"20160215182113.GF26443@sigill.intra.peff.net","threadId":"41416","inReplyTo":"1455558150-30267-3-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 2/4] remote: simplify remote_is_configured()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-15T18:21:13Z","receivedAt":"2016-02-15T18:21:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 06:42:28PM +0100, Thomas Gummerer wrote:\n\n> The remote_is_configured() function allows checking whether a remote\n> exists or not. The function however only works if remote_get() wasn't\n> called before calling it. In addition, it only checks the configuration\n> for remotes, but not remotes or branches files.\n> \n> Make use of the origin member of struct remote instead, which indicates\n> where the remote comes from. It will be set to some value if the remote\n> is configured in any file in the repository, but is initialized to 0 if\n> the remote is only created in make_remote().\n\nMakes sense. I wonder if we would want to give this an explicit slot in\nthe enum. I.e.:\n\n> diff --git a/remote.h b/remote.h\n> index 4fd7a0f..7a5ee77 100644\n> --- a/remote.h\n> +++ b/remote.h\n> @@ -5,7 +5,7 @@\n> #include \"hashmap.h\"\n> \n> enum {\n> -\tREMOTE_CONFIG,\n> +\tREMOTE_CONFIG = 1,\n> \tREMOTE_REMOTES,\n> \tREMOTE_BRANCHES\n> };\n\nAdd in \"REMOTE_UNCONFIGURED = 0\" here. It makes no difference to\ncorrectness, but is perhaps documents what is going on a bit better.\n\n-Peff\n"},{"id":"278181","messageId":"20160215182324.GG26443@sigill.intra.peff.net","threadId":"41416","inReplyTo":"1455558150-30267-4-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 3/4] remote: actually check if remote exits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-15T18:23:24Z","receivedAt":"2016-02-15T18:23:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 06:42:29PM +0100, Thomas Gummerer wrote:\n\n> When converting the git remote command to a builtin in 211c89 (\"Make\n> git-remote a builtin\"), a few calls to check if a remote exists were\n> converted from:\n> if (!exists $remote->{$name}) {\n> \t [...]\n> to:\n> remote = remote_get(argv[1]);\n> if (!remote)\n> [...]\n> \n> The new check is not quite correct, because remote_get() never returns\n> NULL if a name is given. This leaves us with the somewhat cryptic error\n> message \"error: Could not remove config section 'remote.test'\", if we\n> are trying to remove a remote that does not exist, or a similar error if\n> we try to rename a remote.\n> \n> Use the remote_is_configured() function to check whether the remote\n> actually exists, and die with a more sensible error message (\"No such\n> remote: $remotename\") instead if it doesn't.\n> \n> Signed-off-by: Thomas Gummerer \n> ---\n> builtin/remote.c | 4 ++--\n> t/t5505-remote.sh | 18 ++++++++++++++++++\n> 2 files changed, 20 insertions(+), 2 deletions(-)\n\nNice. Very cleanly explained and implemented.\n\n-Peff\n"},{"id":"278182","messageId":"20160215183334.GH26443@sigill.intra.peff.net","threadId":"41416","inReplyTo":"1455558150-30267-5-git-send-email-t.gummerer@gmail.com","subject":"Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-15T18:33:34Z","receivedAt":"2016-02-15T18:33:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:\n\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> [remote \"foo\"]\n> vcs = bar\n> \n> If we try to run `git remote add foo bar` with the above remote\n> configuration, git segfaults. This change fixes it.\n> \n> In addition, git remote rename $existing foo with the configuration for\n> foo as above silently succeeds, even though foo already exists,\n> modifying its configuration. With this patch it fails with \"remote foo\n> already exists\".\n\nChecking is_configured() certainly sounds like a better test, but...\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 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\nThis original is quite confusing. I thought at first that there was\nperhaps something going on with allowing repeated re-configuration of\nthe same remote, as long as some parameters matched. I.e., I am\nwondering if there is a case here that does _not_ segfault, that we\nwould be breaking.\n\nBut reading over fb86e32dcc, I think I have convinced myself that it was\nmerely an ad-hoc check for \"is_configured\", and using that function is a\nbetter replacement.\n\n-Peff\n"},{"id":"278183","messageId":"CAPig+cSkfEwa8TTJPg5B9W8FCqPXD9Uz6_EuFQeriwYHhP99gg@mail.gmail.com","threadId":"41416","inReplyTo":"20160215181837.GE26443@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] remote: use skip_prefix","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-02-15T18:35:03Z","receivedAt":"2016-02-15T18:35:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 15, 2016 at 1:18 PM, Jeff King wrote:\n> This is definitely an improvement, but I think we can actually go a step\n> further here, and use parse_config_key. Like:\n>\n> - if (starts_with(key, \"branch.\")) {\n> - name = key + 7;\n> - subkey = strrchr(name, '.');\n> - if (!subkey)\n> + if (starts_with(key, \"branch\", &name, &namelen, &subkey)) {\n\nI guess you meant: s/starts_with/parse_config_key/\n\n> + if (!name)\n> return 0;\n> - branch = make_branch(name, subkey - name);\n> + branch = make_branch(name, namelen);\n> if (!strcmp(subkey, \".remote\")) {\n> return git_config_string(&branch->remote_name, key, value);\n> } else if (!strcmp(subkey, \".pushremote\")) {\n"},{"id":"278184","messageId":"20160215183622.GA27244@sigill.intra.peff.net","threadId":"41416","inReplyTo":"CAPig+cSkfEwa8TTJPg5B9W8FCqPXD9Uz6_EuFQeriwYHhP99gg@mail.gmail.com","subject":"Re: [PATCH 1/4] remote: use skip_prefix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-15T18:36:22Z","receivedAt":"2016-02-15T18:36:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 15, 2016 at 01:35:03PM -0500, Eric Sunshine wrote:\n\n> On Mon, Feb 15, 2016 at 1:18 PM, Jeff King wrote:\n> > This is definitely an improvement, but I think we can actually go a step\n> > further here, and use parse_config_key. Like:\n> >\n> > - if (starts_with(key, \"branch.\")) {\n> > - name = key + 7;\n> > - subkey = strrchr(name, '.');\n> > - if (!subkey)\n> > + if (starts_with(key, \"branch\", &name, &namelen, &subkey)) {\n> \n> I guess you meant: s/starts_with/parse_config_key/\n\nWhoops, yeah. I'll belatedly add a \"not even compile-tested\" disclaimer.\n:)\n\n-Peff\n"},{"id":"278192","messageId":"20160215203730.GC13775@hank","threadId":"41416","inReplyTo":"20160215181837.GE26443@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] remote: use skip_prefix","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T20:37:30Z","receivedAt":"2016-02-15T20:37:30Z","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 06:42:27PM +0100, Thomas Gummerer wrote:\n>\n> > 95b567c7 (\"use skip_prefix to avoid repeating strings\") transformed\n> > calls using starts_with() and then skipping the length of the prefix to\n> > skip_prefix() calls. In remote.c there are a few calls like:\n> >\n> > if (starts_with(foo, \"bar\"))\n> > foo += 3\n> >\n> > These calls weren't touched by the commit mentioned above, but can\n> > benefit from the same treatment to avoid magic numbers.\n>\n> This is definitely an improvement, but I think we can actually go a step\n> further here, and use parse_config_key. Like:\n\nThanks, I had no idea about this function :) It makes the diff a lot\nnoisier, but I do think the end result is better.\n\n> diff --git a/remote.c b/remote.c\n> index 21e4ec3..8d2c3ca 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -318,15 +318,14 @@ 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 (starts_with(key, \"branch\", &name, &namelen, &subkey)) {\n> +\t\tif (!name)\n> \t\t\treturn 0;\n> -\t\tbranch = make_branch(name, subkey - name);\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>\n> and so on. The difference in lines of code isn't that great, but I think\n> it makes the resulting code more obvious to read.\n>\n> -Peff\n\n--\nThomas\n"},{"id":"278193","messageId":"20160215203846.GD13775@hank","threadId":"41416","inReplyTo":"20160215182113.GF26443@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] remote: simplify remote_is_configured()","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T20:38:46Z","receivedAt":"2016-02-15T20:38:46Z","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 06:42:28PM +0100, Thomas Gummerer wrote:\n>\n> > The remote_is_configured() function allows checking whether a remote\n> > exists or not. The function however only works if remote_get() wasn't\n> > called before calling it. In addition, it only checks the configuration\n> > for remotes, but not remotes or branches files.\n> >\n> > Make use of the origin member of struct remote instead, which indicates\n> > where the remote comes from. It will be set to some value if the remote\n> > is configured in any file in the repository, but is initialized to 0 if\n> > the remote is only created in make_remote().\n>\n> Makes sense. I wonder if we would want to give this an explicit slot in\n> the enum. I.e.:\n>\n> > diff --git a/remote.h b/remote.h\n> > index 4fd7a0f..7a5ee77 100644\n> > --- a/remote.h\n> > +++ b/remote.h\n> > @@ -5,7 +5,7 @@\n> > #include \"hashmap.h\"\n> >\n> > enum {\n> > -\tREMOTE_CONFIG,\n> > +\tREMOTE_CONFIG = 1,\n> > \tREMOTE_REMOTES,\n> > \tREMOTE_BRANCHES\n> > };\n>\n> Add in \"REMOTE_UNCONFIGURED = 0\" here. It makes no difference to\n> correctness, but is perhaps documents what is going on a bit better.\n\nAgreed, will change. Thanks.\n\n>\n> -Peff\n"},{"id":"278194","messageId":"20160215204351.GE13775@hank","threadId":"41416","inReplyTo":"20160215183334.GH26443@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-15T20:43:51Z","receivedAt":"2016-02-15T20:43:51Z","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 06:42:30PM +0100, Thomas Gummerer wrote:\n>\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> > [remote \"foo\"]\n> > vcs = bar\n> >\n> > If we try to run `git remote add foo bar` with the above remote\n> > configuration, git segfaults. This change fixes it.\n> >\n> > In addition, git remote rename $existing foo with the configuration for\n> > foo as above silently succeeds, even though foo already exists,\n> > modifying its configuration. With this patch it fails with \"remote foo\n> > already exists\".\n>\n> Checking is_configured() certainly sounds like a better test, but...\n>\n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index 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> This original is quite confusing. I thought at first that there was\n> perhaps something going on with allowing repeated re-configuration of\n> the same remote, as long as some parameters matched. I.e., I am\n> wondering if there is a case here that does _not_ segfault, that we\n> would be breaking.\n>\n> But reading over fb86e32dcc, I think I have convinced myself that it was\n> merely an ad-hoc check for \"is_configured\", and using that function is a\n> better replacement.\n\nIt took me a while too to convince myself there is nothing strange\ngoing on. But I could neither find anything in the history, nor could\nI think of any case that we could break.\n\nThanks for your review!\n\n> -Peff\n"},{"id":"278203","messageId":"xmqqwpq5psd3.fsf@gitster.mtv.corp.google.com","threadId":"41416","inReplyTo":"20160215203846.GD13775@hank","subject":"Re: [PATCH 2/4] remote: simplify remote_is_configured()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-15T21:37:44Z","receivedAt":"2016-02-15T21:37:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer writes:\n\n> Agreed, will change. Thanks.\n\nThanks for working on this, and thanks reviewers for helping.\n"},{"id":"278457","messageId":"alpine.DEB.2.20.1602171451030.6516@virtualbox","threadId":"41416","inReplyTo":"20160215183334.GH26443@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-17T13:54:05Z","receivedAt":"2016-02-17T13:54:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Thomas,\n\nOn Mon, 15 Feb 2016, Jeff King wrote:\n\n> On Mon, Feb 15, 2016 at 06:42:30PM +0100, Thomas Gummerer wrote:\n> \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> > [remote \"foo\"]\n> > vcs = bar\n> > \n> > If we try to run `git remote add foo bar` with the above remote\n> > configuration, git segfaults. This change fixes it.\n> > \n> > In addition, git remote rename $existing foo with the configuration for\n> > foo as above silently succeeds, even though foo already exists,\n> > modifying its configuration. With this patch it fails with \"remote foo\n> > already exists\".\n> \n> Checking is_configured() certainly sounds like a better test, but...\n> \n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index 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> This original is quite confusing. I thought at first that there was\n> perhaps something going on with allowing repeated re-configuration of\n> the same remote, as long as some parameters matched. I.e., I am\n> wondering if there is a case here that does _not_ segfault, that we\n> would be breaking.\n> \n> But reading over fb86e32dcc, I think I have convinced myself that it was\n> merely an ad-hoc check for \"is_configured\", and using that function is a\n> better replacement.\n\nYes, yes, yes. Y'all are absolutely correct. I shoulda added a test case\nright away, to make sure not only that what I fixed does not get broken in\nthe future, but also to document *what* was fixed, exactly.\n\nSo, belatedly, here goes a patch that verifies what that commit was\nsupposed to fix, and yes, it passes with Thomas' changes (Junio, would you\nplease apply this on top of tg/git-remote?):\n\n-- snipsnap --\nFrom: Johannes Schindelin \nDate: Wed, 17 Feb 2016 14:45:59 +0100\nSubject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x\n\nThis is the test missing from fb86e32 (git remote: allow adding\nremotes agreeing with url.<...>.insteadOf, 2014-12-23): we should\nallow adding a remote with the URL when it agrees with the\nurl.<...>.insteadOf setting.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5505-remote.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 94079a0..19e8e34 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -51,6 +51,11 @@ test_expect_success setup '\n \tgit clone one test\n '\n \n+test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '\n+\tgit config url.git@host.com:team/repo.git.insteadOf myremote &&\n+\tgit remote add myremote git@host.com:team/repo.git\n+'\n+\n test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '\n \t(\n \t\tcd test &&\n-- \n2.7.1.windows.2\n"},{"id":"278461","messageId":"20160217142455.GC1831@hank","threadId":"41416","inReplyTo":"alpine.DEB.2.20.1602171451030.6516@virtualbox","subject":"Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2016-02-17T14:24:55Z","receivedAt":"2016-02-17T14:24:55Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 02/17, Johannes Schindelin wrote:\n> Hi Peff & Thomas,\n>\n> On Mon, 15 Feb 2016, Jeff King wrote:\n> > This original is quite confusing. I thought at first that there was\n> > perhaps something going on with allowing repeated re-configuration of\n> > the same remote, as long as some parameters matched. I.e., I am\n> > wondering if there is a case here that does _not_ segfault, that we\n> > would be breaking.\n> >\n> > But reading over fb86e32dcc, I think I have convinced myself that it was\n> > merely an ad-hoc check for \"is_configured\", and using that function is a\n> > better replacement.\n>\n> Yes, yes, yes. Y'all are absolutely correct. I shoulda added a test case\n> right away, to make sure not only that what I fixed does not get broken in\n> the future, but also to document *what* was fixed, exactly.\n\nThanks for confirming and the test case so we don't break it again.\n\n> So, belatedly, here goes a patch that verifies what that commit was\n> supposed to fix, and yes, it passes with Thomas' changes (Junio, would you\n> please apply this on top of tg/git-remote?):\n>\n> -- snipsnap --\n> From: Johannes Schindelin \n> Date: Wed, 17 Feb 2016 14:45:59 +0100\n> Subject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x\n>\n> This is the test missing from fb86e32 (git remote: allow adding\n> remotes agreeing with url.<...>.insteadOf, 2014-12-23): we should\n> allow adding a remote with the URL when it agrees with the\n> url.<...>.insteadOf setting.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t5505-remote.sh | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index 94079a0..19e8e34 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -51,6 +51,11 @@ test_expect_success setup '\n> \tgit clone one test\n> '\n>\n> +test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '\n> +\tgit config url.git@host.com:team/repo.git.insteadOf myremote &&\n\nMinor nit: I think we should use test_config here.\n\n> +\tgit remote add myremote git@host.com:team/repo.git\n> +'\n> +\n> test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '\n> \t(\n> \t\tcd test &&\n> --\n> 2.7.1.windows.2\n--\nThomas\n"},{"id":"278464","messageId":"alpine.DEB.2.20.1602171720130.3152@virtualbox","threadId":"41416","inReplyTo":"20160217142455.GC1831@hank","subject":"Re: [PATCH 4/4] remote: use remote_is_configured() for add and rename","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-17T16:20:47Z","receivedAt":"2016-02-17T16:20:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Tomas,\n\nOn Wed, 17 Feb 2016, Thomas Gummerer wrote:\n\n> On 02/17, Johannes Schindelin wrote:\n> >\n> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> > index 94079a0..19e8e34 100755\n> > --- a/t/t5505-remote.sh\n> > +++ b/t/t5505-remote.sh\n> > @@ -51,6 +51,11 @@ test_expect_success setup '\n> > \tgit clone one test\n> > '\n> >\n> > +test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '\n> > +\tgit config url.git@host.com:team/repo.git.insteadOf myremote &&\n> \n> Minor nit: I think we should use test_config here.\n\nGood point.\n\n-- snipsnap --\nFrom: Johannes Schindelin \nDate: Wed, 17 Feb 2016 14:45:59 +0100\nSubject: [PATCH] t5505: 'remote add x y' should work when url.y.insteadOf = x\n\nThis is the test missing from fb86e32 (git remote: allow adding\nremotes agreeing with url.<...>.insteadOf, 2014-12-23): we should\nallow adding a remote with the URL when it agrees with the\nurl.<...>.insteadOf setting.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5505-remote.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 94079a0..949725e 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -51,6 +51,11 @@ test_expect_success setup '\n \tgit clone one test\n '\n \n+test_expect_success 'add remote whose URL agrees with url.<...>.insteadOf' '\n+\ttest_config url.git@host.com:team/repo.git.insteadOf myremote &&\n+\tgit remote add myremote git@host.com:team/repo.git\n+'\n+\n test_expect_success C_LOCALE_OUTPUT 'remote information for the origin' '\n \t(\n \t\tcd test &&\n-- \n2.7.1.windows.2\n"}]}