From: Johannes Schindelin Date: Thu, 19 Jan 2017 21:19:52 GMT Subject: [PATCH v2 0/2] Fix remote_is_configured() Message-ID: In-Reply-To: A surprising behavior triggered the bug report in https://github.com/git-for-windows/git/issues/888: the mere existence of the config setting "remote.origin.prune" (in this instance, configured via ~/.gitconfig so that it applies to all repositories) fooled `git remote rename ` into believing that the remote is already there. This patch pair demonstrates the problem, and then fixes it (along with potential similar problems, such as setting an HTTP proxy for remotes of a given name via ~/.gitconfig). Changes since v1: - overhauled the strategy to fix the problem: instead of looking at the config key to determine whether it configures a remote or not, we now look at the config_source: if the remote setting was configured in .git/config, `git remote ` must not overwrite it. If it was configured elsewhere, `git remote` cannot overwrite any setting, as it only touches .git/config. Johannes Schindelin (2): remote rename: demonstrate a bogus "remote exists" bug Be more careful when determining whether a remote was configured builtin/fetch.c | 2 +- builtin/remote.c | 14 +++++++------- remote.c | 12 ++++++++++-- remote.h | 4 ++-- t/t5505-remote.sh | 7 +++++++ 5 files changed, 27 insertions(+), 12 deletions(-) base-commit: ffac48d093d4b518a0cc0e8bf1b7cb53e0c3d7a2 Published-As: https://github.com/dscho/git/releases/tag/rename-remote-v2 Fetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v2 Interdiff vs v1: diff --git a/builtin/fetch.c b/builtin/fetch.c index f1570e3464..b5ad09d046 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1177,7 +1177,7 @@ static int add_remote_or_group(const char *name, struct string_list *list) git_config(get_remote_group, &g); if (list->nr == prev_nr) { struct remote *remote = remote_get(name); - if (!remote_is_configured(remote)) + if (!remote_is_configured(remote, 0)) return 0; string_list_append(list, remote->name); } diff --git a/builtin/remote.c b/builtin/remote.c index e52cf3925b..5339ed6ad1 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -186,7 +186,7 @@ static int add(int argc, const char **argv) url = argv[1]; remote = remote_get(name); - if (remote_is_configured(remote)) + if (remote_is_configured(remote, 1)) die(_("remote %s already exists."), name); strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name); @@ -618,14 +618,14 @@ static int mv(int argc, const char **argv) rename.remote_branches = &remote_branches; oldremote = remote_get(rename.old); - if (!remote_is_configured(oldremote)) + if (!remote_is_configured(oldremote, 1)) die(_("No such remote: %s"), rename.old); if (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG) return migrate_file(oldremote); newremote = remote_get(rename.new); - if (remote_is_configured(newremote)) + if (remote_is_configured(newremote, 1)) die(_("remote %s already exists."), rename.new); strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new); @@ -753,7 +753,7 @@ static int rm(int argc, const char **argv) usage_with_options(builtin_remote_rm_usage, options); remote = remote_get(argv[1]); - if (!remote_is_configured(remote)) + if (!remote_is_configured(remote, 1)) die(_("No such remote: %s"), argv[1]); known_remotes.to_delete = remote; @@ -1415,7 +1415,7 @@ static int set_remote_branches(const char *remotename, const char **branches, strbuf_addf(&key, "remote.%s.fetch", remotename); remote = remote_get(remotename); - if (!remote_is_configured(remote)) + if (!remote_is_configured(remote, 1)) die(_("No such remote '%s'"), remotename); if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) { @@ -1469,7 +1469,7 @@ static int get_url(int argc, const char **argv) remotename = argv[0]; remote = remote_get(remotename); - if (!remote_is_configured(remote)) + if (!remote_is_configured(remote, 1)) die(_("No such remote '%s'"), remotename); url_nr = 0; @@ -1537,7 +1537,7 @@ static int set_url(int argc, const char **argv) oldurl = newurl; remote = remote_get(remotename); - if (!remote_is_configured(remote)) + if (!remote_is_configured(remote, 1)) die(_("No such remote '%s'"), remotename); if (push_mode) { diff --git a/remote.c b/remote.c index 298f2f93fa..8524135de4 100644 --- a/remote.c +++ b/remote.c @@ -255,7 +255,7 @@ static void read_remotes_file(struct remote *remote) if (!f) return; - remote->configured = 1; + remote->configured_in_repo = 1; remote->origin = REMOTE_REMOTES; while (strbuf_getline(&buf, f) != EOF) { const char *v; @@ -290,7 +290,7 @@ static void read_branches_file(struct remote *remote) return; } - remote->configured = 1; + remote->configured_in_repo = 1; remote->origin = REMOTE_BRANCHES; /* @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb) } remote = make_remote(name, namelen); remote->origin = REMOTE_CONFIG; + if (current_config_scope() == CONFIG_SCOPE_REPO) + remote->configured_in_repo = 1; if (!strcmp(subkey, "mirror")) remote->mirror = git_config_bool(key, value); else if (!strcmp(subkey, "skipdefaultupdate")) @@ -386,25 +388,21 @@ static int handle_config(const char *key, const char *value, void *cb) if (git_config_string(&v, key, value)) return -1; add_url(remote, v); - remote->configured = 1; } else if (!strcmp(subkey, "pushurl")) { const char *v; if (git_config_string(&v, key, value)) return -1; add_pushurl(remote, v); - remote->configured = 1; } else if (!strcmp(subkey, "push")) { const char *v; if (git_config_string(&v, key, value)) return -1; add_push_refspec(remote, v); - remote->configured = 1; } else if (!strcmp(subkey, "fetch")) { const char *v; if (git_config_string(&v, key, value)) return -1; add_fetch_refspec(remote, v); - remote->configured = 1; } else if (!strcmp(subkey, "receivepack")) { const char *v; if (git_config_string(&v, key, value)) @@ -433,7 +431,6 @@ static int handle_config(const char *key, const char *value, void *cb) return git_config_string((const char **)&remote->http_proxy_authmethod, key, value); } else if (!strcmp(subkey, "vcs")) { - remote->configured = 1; return git_config_string(&remote->foreign_vcs, key, value); } return 0; @@ -721,9 +718,13 @@ struct remote *pushremote_get(const char *name) return remote_get_1(name, pushremote_for_branch); } -int remote_is_configured(struct remote *remote) +int remote_is_configured(struct remote *remote, int in_repo) { - return remote && remote->configured; + if (!remote) + return 0; + if (in_repo) + return remote->configured_in_repo; + return !!remote->origin; } int for_each_remote(each_remote_fn fn, void *priv) diff --git a/remote.h b/remote.h index 7e6c8067bb..a5bbbe0ef9 100644 --- a/remote.h +++ b/remote.h @@ -15,7 +15,7 @@ struct remote { struct hashmap_entry ent; /* must be first */ const char *name; - int origin, configured; + int origin, configured_in_repo; const char *foreign_vcs; @@ -60,7 +60,7 @@ struct remote { struct remote *remote_get(const char *name); struct remote *pushremote_get(const char *name); -int remote_is_configured(struct remote *remote); +int remote_is_configured(struct remote *remote, int in_repo); typedef int each_remote_fn(struct remote *remote, void *priv); int for_each_remote(each_remote_fn fn, void *priv); diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 09c9823002..ba46e86de0 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -766,11 +766,9 @@ test_expect_success 'rename a remote with name prefix of other remote' ' test_expect_success 'rename succeeds with existing remote..prune' ' git clone one four.four && - ( - cd four.four && - git config remote.upstream.prune true && - git remote rename origin upstream - ) + test_when_finished git config --global --unset remote.upstream.prune && + git config --global remote.upstream.prune true && + git -C four.four remote rename origin upstream ' cat >remotes_origin <