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 <source> <target>` into believing that the <target> 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 <command>` 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.<target>.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 <<EOF -- 2.11.0.windows.3