{"thread":{"id":"44900","subject":"[PATCH 0/2] Fix remote_is_configured()","startedAt":"2017-01-17T21:19:29Z","lastAt":"2017-01-19T21:51:02Z","messageCount":21,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"309556","messageId":"cover.1484687919.git.johannes.schindelin@gmx.de","threadId":"44900","inReplyTo":null,"subject":"[PATCH 0/2] Fix remote_is_configured()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-17T21:18:58Z","receivedAt":"2017-01-17T21:19:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"A surprising behavior triggered the bug report in\nhttps://github.com/git-for-windows/git/issues/888: the mere existence of\nthe config setting \"remote.origin.prune\" (in this instance, configured\nvia ~/.gitconfig so that it applies to all repositories) fooled `git\nremote rename ` into believing that the remote\nis already there.\n\nThis patch pair demonstrates the problem, and then fixes it (along with\npotential similar problems, such as setting an HTTP proxy for remotes of\na given name via ~/.gitconfig).\n\n\nJohannes Schindelin (2):\n remote rename: demonstrate a bogus \"remote exists\" bug\n Be more careful when determining whether a remote was configured\n\n remote.c | 9 ++++++++-\n remote.h | 2 +-\n t/t5505-remote.sh | 9 +++++++++\n 3 files changed, 18 insertions(+), 2 deletions(-)\n\n\nbase-commit: d7dffce1cebde29a0c4b309a79e4345450bf352a\nPublished-As: https://github.com/dscho/git/releases/tag/rename-remote-v1\nFetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v1\n\n-- \n2.11.0.windows.3\n\n"},{"id":"309557","messageId":"5b981b6f4746c7073091d432871d2abdc74d55d3.1484687919.git.johannes.schindelin@gmx.de","threadId":"44900","inReplyTo":"cover.1484687919.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] remote rename: demonstrate a bogus \"remote exists\" bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-17T21:19:04Z","receivedAt":"2017-01-17T21:19:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Some users like to set `remote.origin.prune = true` in their ~/.gitconfig\nso that all of their repositories use that default.\n\nHowever, our code is ill-prepared for this, mistaking that single entry to\nmean that there is already a remote of the name \"origin\", even if there is\nnot.\n\nThis patch adds a test case demonstrating this issue.\n\nReported by Andrew Arnott.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5505-remote.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8198d8eb05..d7e41e9230 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -764,6 +764,15 @@ test_expect_success 'rename a remote with name prefix of other remote' '\n \t)\n '\n \n+test_expect_failure 'rename succeeds with existing remote..prune' '\n+\tgit clone one four.four &&\n+\t(\n+\t\tcd four.four &&\n+\t\tgit config remote.upstream.prune true &&\n+\t\tgit remote rename origin upstream\n+\t)\n+'\n+\n cat >remotes_origin <\n---\n remote.c | 9 ++++++++-\n remote.h | 2 +-\n t/t5505-remote.sh | 2 +-\n 3 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ad6c5424ed..298f2f93fa 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -255,6 +255,7 @@ static void read_remotes_file(struct remote *remote)\n \n \tif (!f)\n \t\treturn;\n+\tremote->configured = 1;\n \tremote->origin = REMOTE_REMOTES;\n \twhile (strbuf_getline(&buf, f) != EOF) {\n \t\tconst char *v;\n@@ -289,6 +290,7 @@ static void read_branches_file(struct remote *remote)\n \t\treturn;\n \t}\n \n+\tremote->configured = 1;\n \tremote->origin = REMOTE_BRANCHES;\n \n \t/*\n@@ -384,21 +386,25 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_url(remote, v);\n+\t\tremote->configured = 1;\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\tremote->configured = 1;\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\tremote->configured = 1;\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\tremote->configured = 1;\n \t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n@@ -427,6 +433,7 @@ static int handle_config(const char *key, const char *value, void *cb)\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\tremote->configured = 1;\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\n \treturn 0;\n@@ -716,7 +723,7 @@ struct remote *pushremote_get(const char *name)\n \n int remote_is_configured(struct remote *remote)\n {\n-\treturn remote && remote->origin;\n+\treturn remote && remote->configured;\n }\n \n int for_each_remote(each_remote_fn fn, void *priv)\ndiff --git a/remote.h b/remote.h\nindex 924881169d..7e6c8067bb 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -15,7 +15,7 @@ struct remote {\n \tstruct hashmap_entry ent; /* must be first */\n \n \tconst char *name;\n-\tint origin;\n+\tint origin, configured;\n \n \tconst char *foreign_vcs;\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex d7e41e9230..09c9823002 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -764,7 +764,7 @@ test_expect_success 'rename a remote with name prefix of other remote' '\n \t)\n '\n \n-test_expect_failure 'rename succeeds with existing remote..prune' '\n+test_expect_success 'rename succeeds with existing remote..prune' '\n \tgit clone one four.four &&\n \t(\n \t\tcd four.four &&\n-- \n2.11.0.windows.3\n"},{"id":"309564","messageId":"20170117214504.y7fnpkgnugbtdxpz@sigill.intra.peff.net","threadId":"44900","inReplyTo":"cover.1484687919.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 0/2] Fix remote_is_configured()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-17T21:45:04Z","receivedAt":"2017-01-17T21:46:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 17, 2017 at 10:18:58PM +0100, Johannes Schindelin wrote:\n\n> A surprising behavior triggered the bug report in\n> https://github.com/git-for-windows/git/issues/888: the mere existence of\n> the config setting \"remote.origin.prune\" (in this instance, configured\n> via ~/.gitconfig so that it applies to all repositories) fooled `git\n> remote rename ` into believing that the remote\n> is already there.\n> \n> This patch pair demonstrates the problem, and then fixes it (along with\n> potential similar problems, such as setting an HTTP proxy for remotes of\n> a given name via ~/.gitconfig).\n\nI thought it was intentional that any config \"created\" the remote, even\nwithout a url field. E.g., you can set:\n\n [remote \"https://example.com/foo/git\"]\n proxy = localhost:1234\n\nand then a bare \"git fetch https://example.com/foo/git\" will respect it.\n\nI admit that \"prune\" is probably useless without a fetch refspec,\nthough.\n\nNote that I don't disagree that the rule \"it's not a remote unless it\nhas a url\" would be a lot saner. But I have a feeling you may be\nbreaking some existing setups.\n\nI grepped around in the list but I couldn't find any relevant\ndiscussion. So maybe I just dreamed it.\n\n-Peff\n"},{"id":"309565","messageId":"20170117214723.p5rni6wwggei366j@sigill.intra.peff.net","threadId":"44900","inReplyTo":"41c347f22c80e96c54db34baa739b6e37e268b61.1484687919.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-17T21:47:23Z","receivedAt":"2017-01-17T21:47:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:\n\n> One of the really nice features of the ~/.gitconfig file is that users\n> can override defaults by their own preferred settings for all of their\n> repositories.\n> \n> One such default that some users like to override is whether the\n> \"origin\" remote gets auto-pruned or not. The user would simply call\n> \n> \tgit config --global remote.origin.prune true\n> \n> and from now on all \"origin\" remotes would be pruned automatically when\n> fetching into the local repository.\n> \n> There is just one catch: now Git thinks that the \"origin\" remote is\n> configured, as it does not discern between having a remote whose\n> fetch (and/or push) URL and refspec is set, and merely having\n> preemptively-configured, global flags for specific remotes.\n> \n> Let's fix this by telling Git that a remote is not configured unless any\n> fetch/push URL or refspect is configured explicitly.\n\nHmm. Old versions of GitHub for Windows used to set fetch refspecs in\nthe system gitconfig, for a similar purpose to what you want to do with\nremote.origin.prune.\n\nI notice here that setting a refspec _does_ define a remote. Is there a\nreason you drew the line there, and not at, say, whether it has a URL?\n\n-Peff\n"},{"id":"309571","messageId":"xmqqd1flcosv.fsf@gitster.mtv.corp.google.com","threadId":"44900","inReplyTo":"20170117214723.p5rni6wwggei366j@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-17T22:19:28Z","receivedAt":"2017-01-17T22:21:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Let's fix this by telling Git that a remote is not configured unless any\n>> fetch/push URL or refspec is configured explicitly.\n>\n> I notice here that setting a refspec _does_ define a remote. Is there a\n> reason you drew the line there, and not at, say, whether it has a URL?\n\n\"Not configured unless any URL or refspec is configured\" means that\nif URL is there, even if there is no refspec, then it is a remote\ndefinition, right? \n\nBut I think \"what does it mean to define a remote\" is a question\nthat you are asking, and that is not necessary to answer within the\nscope of this topic.\n\nI do agree that honoring .prune is nonsense unless refspecs are\ndefined, but the question \"does it make sense to allow prune?\" is\ndifferent from \"is it configured?\". Your \"you can set .proxy and\nother useful things, it is just .prune does not make sense\" is a\nquite appropriate statement to illustrate the difference.\n\nPerhaps instead of adding \"is it configured?\" flag that is too\nbroadly named and has too narrow meaning, would it make more sense\nto introduce \"int can_prune(struct remote *remote)\" that looks at\nthe remote refspecs?\n"},{"id":"309621","messageId":"alpine.DEB.2.20.1701181334040.3469@virtualbox","threadId":"44900","inReplyTo":"20170117214723.p5rni6wwggei366j@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-18T12:34:28Z","receivedAt":"2017-01-18T12:34:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 17 Jan 2017, Jeff King wrote:\n\n> On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:\n> \n> > One of the really nice features of the ~/.gitconfig file is that users\n> > can override defaults by their own preferred settings for all of their\n> > repositories.\n> > \n> > One such default that some users like to override is whether the\n> > \"origin\" remote gets auto-pruned or not. The user would simply call\n> > \n> > \tgit config --global remote.origin.prune true\n> > \n> > and from now on all \"origin\" remotes would be pruned automatically when\n> > fetching into the local repository.\n> > \n> > There is just one catch: now Git thinks that the \"origin\" remote is\n> > configured, as it does not discern between having a remote whose\n> > fetch (and/or push) URL and refspec is set, and merely having\n> > preemptively-configured, global flags for specific remotes.\n> > \n> > Let's fix this by telling Git that a remote is not configured unless any\n> > fetch/push URL or refspect is configured explicitly.\n> \n> Hmm. Old versions of GitHub for Windows used to set fetch refspecs in\n> the system gitconfig, for a similar purpose to what you want to do with\n> remote.origin.prune.\n> \n> I notice here that setting a refspec _does_ define a remote. Is there a\n> reason you drew the line there, and not at, say, whether it has a URL?\n\nI want to err on the side of caution. That's why.\n\nCiao,\nJohannes\n"},{"id":"309622","messageId":"alpine.DEB.2.20.1701181338041.3469@virtualbox","threadId":"44900","inReplyTo":"xmqqd1flcosv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-18T12:38:50Z","receivedAt":"2017-01-18T12:41:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 17 Jan 2017, Junio C Hamano wrote:\n\n> Perhaps instead of adding \"is it configured?\" flag that is too\n> broadly named and has too narrow meaning, would it make more sense\n> to introduce \"int can_prune(struct remote *remote)\" that looks at\n> the remote refspecs?\n\nThat (\"can we prune?\") is not the question we need to ask when determining\nwhether we can rename a remote to a new name.\n\nCiao,\nJohannes\n"},{"id":"309624","messageId":"20170118125422.gi3ppqiqhyykk7iy@sigill.intra.peff.net","threadId":"44900","inReplyTo":"alpine.DEB.2.20.1701181334040.3469@virtualbox","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-18T12:54:22Z","receivedAt":"2017-01-18T13:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:\n\n> > > Let's fix this by telling Git that a remote is not configured unless any\n> > > fetch/push URL or refspect is configured explicitly.\n> > \n> > Hmm. Old versions of GitHub for Windows used to set fetch refspecs in\n> > the system gitconfig, for a similar purpose to what you want to do with\n> > remote.origin.prune.\n> > \n> > I notice here that setting a refspec _does_ define a remote. Is there a\n> > reason you drew the line there, and not at, say, whether it has a URL?\n> \n> I want to err on the side of caution. That's why.\n\nI guess I just don't see why changing the behavior with respect to\n\"prune\" or \"proxy\" is any less conservative than changing the one for\n\"refspec\". Both can make some real-world cases work, and both can cause\nbreakage in some possible real-world cases. If we are going to change\nanything, it seems like we should at least aim for a simple and\nconsistent rule (since users have to know which keys can be put in\n~/.gitconfig and which cannot).\n\nI can think of one alternative approach that might be easier for users\nto understand, and that we already use elsewhere (e.g., with \"http.*\"\nconfig): have a set of \"default\" remote keys (e.g., just \"remote.key\")\nthat git falls back to when the remote.*.key isn't set. Then your use\ncase becomes something like:\n\n [remote]\n prune = true\n\nThat's not _exactly_ the same, as it applies to all remotes, not just\n\"origin\" (other remotes can override it, but would need to do so\nexplicitly). But I have a suspicion that may actually be what users\nwant.\n\n-Peff\n"},{"id":"309630","messageId":"alpine.DEB.2.20.1701181700020.3469@virtualbox","threadId":"44900","inReplyTo":"20170118125422.gi3ppqiqhyykk7iy@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-18T16:22:40Z","receivedAt":"2017-01-18T16:23:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 18 Jan 2017, Jeff King wrote:\n\n> On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:\n> \n> > > > Let's fix this by telling Git that a remote is not configured\n> > > > unless any fetch/push URL or refspect is configured explicitly.\n> > > \n> > > Hmm. Old versions of GitHub for Windows used to set fetch refspecs\n> > > in the system gitconfig, for a similar purpose to what you want to\n> > > do with remote.origin.prune.\n> > > \n> > > I notice here that setting a refspec _does_ define a remote. Is\n> > > there a reason you drew the line there, and not at, say, whether it\n> > > has a URL?\n> > \n> > I want to err on the side of caution. That's why.\n> \n> I guess I just don't see why changing the behavior with respect to\n> \"prune\" or \"proxy\" is any less conservative than changing the one for\n> \"refspec\".\n\nLet's take a step back and consider the problem I try to solve, okay?\n\nThe problem is that `git remote rename ` refuses to do its job\nif it detects a configured ``. And it detects a configured ``\neven in cases where there is not *really* any remote configured.\n\nThe example use case is to configure `remote.origin.prune = true` in\n~/.gitconfig, i.e. changing Git's default for all \"origin\" remotes in all\nof the user's repositories.\n\nNow, the *real* fix would be to detect whether the remote was \"configured\"\nin the current repository, or in ~/.gitconfig. But that may not even be\ndesirable, as we would want a more general fix for the question: can `git\nremote` rename a given remote to a new name, i.e. is that new name already\ntaken?\n\nAnd if you try to answer that last question, you will probably pick the\nsame set of keys for which you assume that remote.. really\nconfigures a remote or not.\n\nOriginally, I would even have put the \"vcs\" into that set, as I could see\na legitimate use case for users to configure \"remote.svn.vcs = vcs\" in\n~/.gitconfig. But the regression test suite specifically tests for that\ncase, and I trust that there was a good reason, even if Thomas did not\ndescribe that good reason in the commit message nor in any reply to this\npatch pair.\n\nSo how can things go wrong?\n\nLet's take your example that the user may have specified refspecs (or\nprune, or proxy) for a URL via \"remote..fetch\", and that `git rename`\nincorrectly allows that as a new remote name. You know what? Let's do a\nreal code review here, not just a patch glance-over. Let's test this and\n*know* whether it can be a problem:\n\n\tgit remote rename origin https://github.com/git/git\n\tfatal: 'https://github.com/git/git' is not a valid remote name\n\nA ha! It is *not* possible to hit that case because `git remote rename`\nalready complains much earlier about the new name not being a valid name.\n\nSo let's see what could go wrong with another example you mentioned, that\nthe proxy may not be used because we changed the logic of\nremote_is_configured(). But the only user of the remote proxy settings is\nin http_init() and reads:\n\n if (remote && remote->http_proxy)\n curl_http_proxy = xstrdup(remote->http_proxy);\n\n if (remote)\n var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);\n\nIt does not even test whether the remote is configured! So maybe the\ncaller does? Nope, the only caller of http_init() that passes a remote is\nremote-curl's cmd_main() function, and the relevant part reads:\n\n remote = remote_get(argv[1]);\n\n if (argc > 2) {\n end_url_with_slash(&url, argv[2]);\n } else {\n end_url_with_slash(&url, remote->url[0]);\n }\n\n http_init(remote, url.buf, 0);\n\nThis code also does not care whether the remote \"is configured\" or not.\n\nSo maybe there are any other downsides with callers of\nremote_is_configured()?\n\nThere is one caller in builtin/fetch.c's add_remote_or_group() which\nclearly is covered (we need a URL, unless the vcs is configured).\n\nAll other callers are in builtin/remote.c:\n\n- in add(), to test whether we can add a new remote,\n- in mv(), to test whether the remote to rename is configured,\n- in mv(), to test whether the new name is already taken,\n- in rm(), to test whether the remote exists,\n- in set_remote_branches(), to test whether the remote to be changed\n exists,\n- in get_url(), to test whether the remote exists, and\n- in set_url(), to test whether the remote exists.\n\nIt appears pretty obvious that in all of these cases, the suggested patch\nstill makes sense and does not introduce any nasty surprise.\n\n> I can think of one alternative approach that might be easier for users\n> to understand, and that we already use elsewhere (e.g., with \"http.*\"\n> config): have a set of \"default\" remote keys (e.g., just \"remote.key\")\n> that git falls back to when the remote.*.key isn't set. Then your use\n> case becomes something like:\n> \n> [remote]\n> prune = true\n> \n> That's not _exactly_ the same, as it applies to all remotes, not just\n> \"origin\" (other remotes can override it, but would need to do so\n> explicitly). But I have a suspicion that may actually be what users\n> want.\n\nSure. That'll be another patch series, though. And another contributor.\n\nCiao,\nJohannes\n"},{"id":"309747","messageId":"20170119182721.7y2zzrbaalfqjjn6@sigill.intra.peff.net","threadId":"44900","inReplyTo":"alpine.DEB.2.20.1701181700020.3469@virtualbox","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T18:27:21Z","receivedAt":"2017-01-19T18:27:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:\n\n> > > I want to err on the side of caution. That's why.\n> > \n> > I guess I just don't see why changing the behavior with respect to\n> > \"prune\" or \"proxy\" is any less conservative than changing the one for\n> > \"refspec\".\n> \n> Let's take a step back and consider the problem I try to solve, okay?\n\nOK. Though after reading your message over several times, I think I may\nhave confused things by raising two separate issues.\n\nSo let me re-state my issues here for clarity:\n\n 1. I'm concerned that a setting for remote..X would fail to apply\n to a bare \"git fetch \" if \"X\" is considered as \"not really\"\n configuring a remote.\n\n 2. I'm concerned that splitting the remote.*.X keys into two classes:\n \"really configures\" and \"does not really configure\" creates a\n confusing interface for users. Some keys are OK to set in your\n ~/.gitconfig and others are not.\n\nLet's talk about concern 1 first. Based on your analysis in this\nmessage, it looks like it _isn't_ a problem with your patch (because\nis_configured is never used for applying values, only for add/del/rename\ntypes of operations in remote.c).\n\nSo good. Thanks for addressing my concern. And that makes your\n\"conservative\" comment make more sense; the idea is that you are not\nbreaking anything, but just loosening selectively for some values of\n\"X\".\n\nI think there are still some weird corner cases even for \"prune\",\nthough. E.g., try:\n\n git init\n git remote add foo whatever\n git config remote.foo.prune true\n git config remote.other.prune false\n\nSo now we have:\n\n\t[remote \"foo\"]\n\t\turl = whatever\n\t\tfetch = +refs/heads/*:refs/remotes/foo/*\n\t\tprune = true\n\t[remote \"other\"]\n\t\tprune = false\n\nNow try \"git remote rename foo other\". With current versions of git,\nit's disallowed. With your patch, I get:\n\n\t[remote \"other\"]\n\t\turl = whatever\n\t\tprune = true\n\t[remote \"other\"]\n\t\tprune = false\n\t\tfetch = +refs/heads/*:refs/remotes/other/*\n\nThe old value of remote.other.prune is overriding the one we tried to\nmove into place.\n\nIn your motivating example, the old value is in ~/.gitconfig, so it\nautomatically takes lower precedence, and everything just works (I think\nit would also just work if \"other\" had been defined originally _before_\nfoo in the config file).\n\nI think you could fix it by having git-remote teach the \"not really\"\nconfig values (like \"prune\") to overwrite any existing value when\nrewriting the config. I think this just needs the multi_replace flag set\nwhen calling git_config_set_multivar().\n\nThat raises questions about what should happen when multi-value keys\nlike \"refspec\" would be set (if we were to add them to the \"not really\"\nset). Should they be overwritten, or merged? And in that sense, your\npatch lets you punt on those issues.\n\n\nI still think my second concern is valid. It's made worse by your patch\n(if only because everything was disallowed before, and now some things\nare and some things aren't). If this is a first step towards a final\nstate where the rules are simple, then starting conservatively makes\nsense. And until we get there, the answer \"yes, it should, but nobody\nhas worked out the semantics yet; care to make a patch?\" is an OK one.\nBut it sounds like you do not ever want to loosen the \"refspec\" case.\n\nI don't think that's ideal, but at the very least if that's the end\nstate, the list of \"OK to use in ~/.gitconfig\" keys should probably be\ndocumented.\n\nReading your message, though, I still wonder if we can do better...\n\n> The problem is that `git remote rename ` refuses to do its job\n> if it detects a configured ``. And it detects a configured ``\n> even in cases where there is not *really* any remote configured.\n\nI'd add to this that \"git remote add \" should work in a similar way\n(that was the one that I think people often ran into with\nremote.origin.fetch refspecs).\n\n> The example use case is to configure `remote.origin.prune = true` in\n> ~/.gitconfig, i.e. changing Git's default for all \"origin\" remotes in all\n> of the user's repositories.\n> \n> Now, the *real* fix would be to detect whether the remote was \"configured\"\n> in the current repository, or in ~/.gitconfig. But that may not even be\n> desirable, as we would want a more general fix for the question: can `git\n> remote` rename a given remote to a new name, i.e. is that new name already\n> taken?\n\nI think _this_ is a much better way of framing the problem. It is not\nabout which keys are set, but about _where_ they are set. IOW, a\nreasonable rule would be: if there is any remote.*.X in the repo config,\nthen git-remote should consider it a configured repo. And otherwise, no\nmatter what is in ~/.gitconfig or elsewhere, git-remote should proceed\nas if it doesn't exist (and repo-level config can take precedence over\nconfig defined elsewhere).\n\nI.e., something like this:\n\ndiff --git a/remote.c b/remote.c\nindex 298f2f93f..720d616be 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t}\n \tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n+\tif (current_config_scope() == CONFIG_SCOPE_REPO)\n+\t\tremote->configured = 1;\n \tif (!strcmp(subkey, \"mirror\"))\n \t\tremote->mirror = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n\nThat doesn't make your test pass, but I think that is only because your\ntest is not covering the interesting case (it puts the new config in the\nrepo config, not in ~/.gitconfig).\n\nWhat do you think?\n\n> Originally, I would even have put the \"vcs\" into that set, as I could see\n> a legitimate use case for users to configure \"remote.svn.vcs = vcs\" in\n> ~/.gitconfig. But the regression test suite specifically tests for that\n> case, and I trust that there was a good reason, even if Thomas did not\n> describe that good reason in the commit message nor in any reply to this\n> patch pair.\n\nThe config-scope thing above would allow \"remote.svn.vcs\" in\n~/.gitconfig. But I don't think the test script actually checks that; it\nchecks for the repo-level config. And we would continue to do the right\nthing there.\n\n-Peff\n"},{"id":"309768","messageId":"20170119202205.qtrbiexuisiy4vfa@sigill.intra.peff.net","threadId":"44900","inReplyTo":"xmqq4m0u24hs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T20:22:05Z","receivedAt":"2017-01-19T20:22:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 12:12:47PM -0800, Junio C Hamano wrote:\n\n> > The config-scope thing above would allow \"remote.svn.vcs\" in\n> > ~/.gitconfig. But I don't think the test script actually checks that; it\n> > checks for the repo-level config. And we would continue to do the right\n> > thing there.\n> \n> I am not \"you\" you are addressing to, but I think tying it to where\n> the variable came from makes quite sense. \n> \n> Because it makes it no longer possible to just inspect the\n> configured result to answer \"is the remote configured?\",\n> introduction of the configured field also needs to be preserved from\n> the original by Dscho, so does reading from historical non-config\n> sources like $GIT_DIR/remotes/*, which are by definition\n> per-repository thing.\n> \n> IOW, with this tweak (and not setting ->configured based on what\n> keys are set), I think Dscho's patch makes sense.\n\nYeah, worry if that wasn't clear: the hunk I posted was a just a\npartial. The actual thing I built and ran against the test suite was\nexactly as you described.\n\n-Peff\n"},{"id":"309769","messageId":"xmqq4m0u24hs.fsf@gitster.mtv.corp.google.com","threadId":"44900","inReplyTo":"20170119182721.7y2zzrbaalfqjjn6@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-19T20:12:47Z","receivedAt":"2017-01-19T20:27:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:\n>\n>> > > I want to err on the side of caution. That's why.\n>> > \n>> > I guess I just don't see why changing the behavior with respect to\n>> > \"prune\" or \"proxy\" is any less conservative than changing the one for\n>> > \"refspec\".\n>> \n> I think _this_ is a much better way of framing the problem. It is not\n> about which keys are set, but about _where_ they are set. IOW, a\n> reasonable rule would be: if there is any remote.*.X in the repo config,\n> then git-remote should consider it a configured repo. And otherwise, no\n> matter what is in ~/.gitconfig or elsewhere, git-remote should proceed\n> as if it doesn't exist (and repo-level config can take precedence over\n> config defined elsewhere).\n>\n> I.e., something like this:\n>\n> diff --git a/remote.c b/remote.c\n> index 298f2f93f..720d616be 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n> \t}\n> \tremote = make_remote(name, namelen);\n> \tremote->origin = REMOTE_CONFIG;\n> +\tif (current_config_scope() == CONFIG_SCOPE_REPO)\n> +\t\tremote->configured = 1;\n> \tif (!strcmp(subkey, \"mirror\"))\n> \t\tremote->mirror = git_config_bool(key, value);\n> \telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n>\n> That doesn't make your test pass, but I think that is only because your\n> test is not covering the interesting case (it puts the new config in the\n> repo config, not in ~/.gitconfig).\n>\n> What do you think?\n>\n>> Originally, I would even have put the \"vcs\" into that set, as I could see\n>> a legitimate use case for users to configure \"remote.svn.vcs = vcs\" in\n>> ~/.gitconfig. But the regression test suite specifically tests for that\n>> case, and I trust that there was a good reason, even if Thomas did not\n>> describe that good reason in the commit message nor in any reply to this\n>> patch pair.\n>\n> The config-scope thing above would allow \"remote.svn.vcs\" in\n> ~/.gitconfig. But I don't think the test script actually checks that; it\n> checks for the repo-level config. And we would continue to do the right\n> thing there.\n\nI am not \"you\" you are addressing to, but I think tying it to where\nthe variable came from makes quite sense. \n\nBecause it makes it no longer possible to just inspect the\nconfigured result to answer \"is the remote configured?\",\nintroduction of the configured field also needs to be preserved from\nthe original by Dscho, so does reading from historical non-config\nsources like $GIT_DIR/remotes/*, which are by definition\nper-repository thing.\n\nIOW, with this tweak (and not setting ->configured based on what\nkeys are set), I think Dscho's patch makes sense.\n\n"},{"id":"309782","messageId":"alpine.DEB.2.20.1701192217090.3469@virtualbox","threadId":"44900","inReplyTo":"20170119182721.7y2zzrbaalfqjjn6@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] Be more careful when determining whether a remote was configured","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-19T21:19:13Z","receivedAt":"2017-01-19T21:26:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 19 Jan 2017, Jeff King wrote:\n\n> diff --git a/remote.c b/remote.c\n> index 298f2f93f..720d616be 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n> \t}\n> \tremote = make_remote(name, namelen);\n> \tremote->origin = REMOTE_CONFIG;\n> +\tif (current_config_scope() == CONFIG_SCOPE_REPO)\n> +\t\tremote->configured = 1;\n> \tif (!strcmp(subkey, \"mirror\"))\n> \t\tremote->mirror = git_config_bool(key, value);\n> \telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n> \n> That doesn't make your test pass, but I think that is only because your\n> test is not covering the interesting case (it puts the new config in the\n> repo config, not in ~/.gitconfig).\n> \n> What do you think?\n\nHeh. After skimming the first three paragraphs of your mail, I had a\nsimilar idea and implemented it. It works great!\n\nv2 coming right up,\nJohannes\n"},{"id":"309784","messageId":"4e3c94cb58052af1c0021b7e4c96ab4029afb7bb.1484860744.git.johannes.schindelin@gmx.de","threadId":"44900","inReplyTo":"cover.1484860744.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/2] remote rename: demonstrate a bogus \"remote exists\" bug","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-19T21:19:58Z","receivedAt":"2017-01-19T21:27:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Some users like to set `remote.origin.prune = true` in their ~/.gitconfig\nso that all of their repositories use that default.\n\nHowever, our code is ill-prepared for this, mistaking that single entry to\nmean that there is already a remote of the name \"origin\", even if there is\nnot.\n\nThis patch adds a test case demonstrating this issue.\n\nReported by Andrew Arnott.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5505-remote.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8198d8eb05..2c03f44c85 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -764,6 +764,13 @@ test_expect_success 'rename a remote with name prefix of other remote' '\n \t)\n '\n \n+test_expect_failure 'rename succeeds with existing remote..prune' '\n+\tgit clone one four.four &&\n+\ttest_when_finished git config --global --unset remote.upstream.prune &&\n+\tgit config --global remote.upstream.prune true &&\n+\tgit -C four.four remote rename origin upstream\n+'\n+\n cat >remotes_origin < ` into believing that the remote\nis already there.\n\nThis patch pair demonstrates the problem, and then fixes it (along with\npotential similar problems, such as setting an HTTP proxy for remotes of\na given name via ~/.gitconfig).\n\nChanges since v1:\n\n - overhauled the strategy to fix the problem: instead of looking at the\n config key to determine whether it configures a remote or not, we now\n look at the config_source: if the remote setting was configured in\n .git/config, `git remote ` must not overwrite it. If it was\n configured elsewhere, `git remote` cannot overwrite any setting, as\n it only touches .git/config.\n\n\nJohannes Schindelin (2):\n remote rename: demonstrate a bogus \"remote exists\" bug\n Be more careful when determining whether a remote was configured\n\n builtin/fetch.c | 2 +-\n builtin/remote.c | 14 +++++++-------\n remote.c | 12 ++++++++++--\n remote.h | 4 ++--\n t/t5505-remote.sh | 7 +++++++\n 5 files changed, 27 insertions(+), 12 deletions(-)\n\n\nbase-commit: ffac48d093d4b518a0cc0e8bf1b7cb53e0c3d7a2\nPublished-As: https://github.com/dscho/git/releases/tag/rename-remote-v2\nFetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v2\n\nInterdiff vs v1:\n\n diff --git a/builtin/fetch.c b/builtin/fetch.c\n index f1570e3464..b5ad09d046 100644\n --- a/builtin/fetch.c\n +++ b/builtin/fetch.c\n @@ -1177,7 +1177,7 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n \tgit_config(get_remote_group, &g);\n \tif (list->nr == prev_nr) {\n \t\tstruct remote *remote = remote_get(name);\n -\t\tif (!remote_is_configured(remote))\n +\t\tif (!remote_is_configured(remote, 0))\n \t\t\treturn 0;\n \t\tstring_list_append(list, remote->name);\n \t}\n diff --git a/builtin/remote.c b/builtin/remote.c\n index e52cf3925b..5339ed6ad1 100644\n --- a/builtin/remote.c\n +++ b/builtin/remote.c\n @@ -186,7 +186,7 @@ static int add(int argc, const char **argv)\n \turl = argv[1];\n \n \tremote = remote_get(name);\n -\tif (remote_is_configured(remote))\n +\tif (remote_is_configured(remote, 1))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n \tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n @@ -618,14 +618,14 @@ static int mv(int argc, const char **argv)\n \trename.remote_branches = &remote_branches;\n \n \toldremote = remote_get(rename.old);\n -\tif (!remote_is_configured(oldremote))\n +\tif (!remote_is_configured(oldremote, 1))\n \t\tdie(_(\"No such remote: %s\"), rename.old);\n \n \tif (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)\n \t\treturn migrate_file(oldremote);\n \n \tnewremote = remote_get(rename.new);\n -\tif (remote_is_configured(newremote))\n +\tif (remote_is_configured(newremote, 1))\n \t\tdie(_(\"remote %s already exists.\"), rename.new);\n \n \tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n @@ -753,7 +753,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_is_configured(remote))\n +\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote: %s\"), argv[1]);\n \n \tknown_remotes.to_delete = remote;\n @@ -1415,7 +1415,7 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \tstrbuf_addf(&key, \"remote.%s.fetch\", remotename);\n \n \tremote = remote_get(remotename);\n -\tif (!remote_is_configured(remote))\n +\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {\n @@ -1469,7 +1469,7 @@ static int get_url(int argc, const char **argv)\n \tremotename = argv[0];\n \n \tremote = remote_get(remotename);\n -\tif (!remote_is_configured(remote))\n +\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \turl_nr = 0;\n @@ -1537,7 +1537,7 @@ static int set_url(int argc, const char **argv)\n \t\toldurl = newurl;\n \n \tremote = remote_get(remotename);\n -\tif (!remote_is_configured(remote))\n +\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (push_mode) {\n diff --git a/remote.c b/remote.c\n index 298f2f93fa..8524135de4 100644\n --- a/remote.c\n +++ b/remote.c\n @@ -255,7 +255,7 @@ static void read_remotes_file(struct remote *remote)\n \n \tif (!f)\n \t\treturn;\n -\tremote->configured = 1;\n +\tremote->configured_in_repo = 1;\n \tremote->origin = REMOTE_REMOTES;\n \twhile (strbuf_getline(&buf, f) != EOF) {\n \t\tconst char *v;\n @@ -290,7 +290,7 @@ static void read_branches_file(struct remote *remote)\n \t\treturn;\n \t}\n \n -\tremote->configured = 1;\n +\tremote->configured_in_repo = 1;\n \tremote->origin = REMOTE_BRANCHES;\n \n \t/*\n @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t}\n \tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n +\tif (current_config_scope() == CONFIG_SCOPE_REPO)\n +\t\tremote->configured_in_repo = 1;\n \tif (!strcmp(subkey, \"mirror\"))\n \t\tremote->mirror = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n @@ -386,25 +388,21 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\tif (git_config_string(&v, key, value))\n \t\t\treturn -1;\n \t\tadd_url(remote, v);\n -\t\tremote->configured = 1;\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\tremote->configured = 1;\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\tremote->configured = 1;\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\tremote->configured = 1;\n \t} else if (!strcmp(subkey, \"receivepack\")) {\n \t\tconst char *v;\n \t\tif (git_config_string(&v, key, value))\n @@ -433,7 +431,6 @@ static int handle_config(const char *key, const char *value, void *cb)\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\tremote->configured = 1;\n \t\treturn git_config_string(&remote->foreign_vcs, key, value);\n \t}\n \treturn 0;\n @@ -721,9 +718,13 @@ struct remote *pushremote_get(const char *name)\n \treturn remote_get_1(name, pushremote_for_branch);\n }\n \n -int remote_is_configured(struct remote *remote)\n +int remote_is_configured(struct remote *remote, int in_repo)\n {\n -\treturn remote && remote->configured;\n +\tif (!remote)\n +\t\treturn 0;\n +\tif (in_repo)\n +\t\treturn remote->configured_in_repo;\n +\treturn !!remote->origin;\n }\n \n int for_each_remote(each_remote_fn fn, void *priv)\n diff --git a/remote.h b/remote.h\n index 7e6c8067bb..a5bbbe0ef9 100644\n --- a/remote.h\n +++ b/remote.h\n @@ -15,7 +15,7 @@ struct remote {\n \tstruct hashmap_entry ent; /* must be first */\n \n \tconst char *name;\n -\tint origin, configured;\n +\tint origin, configured_in_repo;\n \n \tconst char *foreign_vcs;\n \n @@ -60,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(struct remote *remote);\n +int remote_is_configured(struct remote *remote, int in_repo);\n \n typedef int each_remote_fn(struct remote *remote, void *priv);\n int for_each_remote(each_remote_fn fn, void *priv);\n diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n index 09c9823002..ba46e86de0 100755\n --- a/t/t5505-remote.sh\n +++ b/t/t5505-remote.sh\n @@ -766,11 +766,9 @@ test_expect_success 'rename a remote with name prefix of other remote' '\n \n test_expect_success 'rename succeeds with existing remote..prune' '\n \tgit clone one four.four &&\n -\t(\n -\t\tcd four.four &&\n -\t\tgit config remote.upstream.prune true &&\n -\t\tgit remote rename origin upstream\n -\t)\n +\ttest_when_finished git config --global --unset remote.upstream.prune &&\n +\tgit config --global remote.upstream.prune true &&\n +\tgit -C four.four remote rename origin upstream\n '\n \n cat >remotes_origin < There is only one caller of remote_is_configured() (in `git fetch`) that\n> may want to take remotes into account even if they were configured\n> outside the repository config; all other callers essentially try to\n> prevent the Git command from overwriting settings in the repository\n> config.\n> \n> To accommodate that fact, the remote_is_configured() function now\n> requires a parameter that states whether the caller is interested in all\n> remotes, or only in those that were configured in the repository config.\n\nJust to make sure I understand the issue, the problem is that:\n\n git config --global remote.foo.url whatever\n git fetch --multiple foo bar\n\nwould not work without this part of the patch?\n\nI'm trying to figure out why \"fetch --multiple\" wouldn't just take a url\nin the first place. I guess it is because multiple fetch is useless\nwithout refspecs (since otherwise you're just writing to FETCH_HEAD,\nwhich gets immediately overwritten). I wonder if that test really should\nbe:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c85f3471d..9024cfffa 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1014,9 +1014,9 @@ static int add_remote_or_group(const char *name, struct string_list *list)\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\t\treturn 0;\n \t\tremote = remote_get(name);\n+\t\tif (!remote->fetch_refspec_nr)\n+\t\t\treturn 0;\n \t\tstring_list_append(list, remote->name);\n \t}\n \treturn 1;\n\nIt's outside the scope of your patches, so I think we are OK to just\nignore it. But if you want to pursue it, it avoids having to add the\nextra parameter to remote_is_configured().\n\n> Many thanks to Jeff King whose tireless review helped with settling for\n> nothing less than the current strategy.\n\nJust how I wanted to be immortalized in git's commit history. ;)\n\n> builtin/fetch.c | 2 +-\n> builtin/remote.c | 14 +++++++-------\n> remote.c | 12 ++++++++++--\n> remote.h | 4 ++--\n> t/t5505-remote.sh | 2 +-\n> 5 files changed, 21 insertions(+), 13 deletions(-)\n\nPatch itself looks OK to me.\n\n-Peff\n"},{"id":"309789","messageId":"1605031b76025f4bd0e485705c34a25557bb75a1.1484860744.git.johannes.schindelin@gmx.de","threadId":"44900","inReplyTo":"cover.1484860744.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/2] Be more careful when determining whether a remote was configured","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-19T21:20:02Z","receivedAt":"2017-01-19T21:35:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"One of the really nice features of the ~/.gitconfig file is that users\ncan override defaults by their own preferred settings for all of their\nrepositories.\n\nOne such default that some users like to override is whether the\n\"origin\" remote gets auto-pruned or not. The user would simply call\n\n\tgit config --global remote.origin.prune true\n\nand from now on all \"origin\" remotes would be pruned automatically when\nfetching into the local repository.\n\nThere is just one catch: now Git thinks that the \"origin\" remote is\nconfigured, even if the repository config has no [remote \"origin\"]\nsection at all, as it does not realize that the \"prune\" setting was\nconfigured globally and that there really is no \"origin\" remote\nconfigured in this repository.\n\nThat is a problem e.g. when renaming a remote to a new name, when Git\nmay be fooled into thinking that there is already a remote of that new\nname.\n\nLet's fix this by paying more attention to *where* the remote settings\ncame from: if they are configured in the local repository config, we\nmust not overwrite them. If they were configured elsewhere, we cannot\noverwrite them to begin with, as we only write the repository config.\n\nThere is only one caller of remote_is_configured() (in `git fetch`) that\nmay want to take remotes into account even if they were configured\noutside the repository config; all other callers essentially try to\nprevent the Git command from overwriting settings in the repository\nconfig.\n\nTo accommodate that fact, the remote_is_configured() function now\nrequires a parameter that states whether the caller is interested in all\nremotes, or only in those that were configured in the repository config.\n\nMany thanks to Jeff King whose tireless review helped with settling for\nnothing less than the current strategy.\n\nThis fixes https://github.com/git-for-windows/git/issues/888\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fetch.c | 2 +-\n builtin/remote.c | 14 +++++++-------\n remote.c | 12 ++++++++++--\n remote.h | 4 ++--\n t/t5505-remote.sh | 2 +-\n 5 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f1570e3464..b5ad09d046 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1177,7 +1177,7 @@ static int add_remote_or_group(const char *name, struct string_list *list)\n \tgit_config(get_remote_group, &g);\n \tif (list->nr == prev_nr) {\n \t\tstruct remote *remote = remote_get(name);\n-\t\tif (!remote_is_configured(remote))\n+\t\tif (!remote_is_configured(remote, 0))\n \t\t\treturn 0;\n \t\tstring_list_append(list, remote->name);\n \t}\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex e52cf3925b..5339ed6ad1 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -186,7 +186,7 @@ static int add(int argc, const char **argv)\n \turl = argv[1];\n \n \tremote = remote_get(name);\n-\tif (remote_is_configured(remote))\n+\tif (remote_is_configured(remote, 1))\n \t\tdie(_(\"remote %s already exists.\"), name);\n \n \tstrbuf_addf(&buf2, \"refs/heads/test:refs/remotes/%s/test\", name);\n@@ -618,14 +618,14 @@ static int mv(int argc, const char **argv)\n \trename.remote_branches = &remote_branches;\n \n \toldremote = remote_get(rename.old);\n-\tif (!remote_is_configured(oldremote))\n+\tif (!remote_is_configured(oldremote, 1))\n \t\tdie(_(\"No such remote: %s\"), rename.old);\n \n \tif (!strcmp(rename.old, rename.new) && oldremote->origin != REMOTE_CONFIG)\n \t\treturn migrate_file(oldremote);\n \n \tnewremote = remote_get(rename.new);\n-\tif (remote_is_configured(newremote))\n+\tif (remote_is_configured(newremote, 1))\n \t\tdie(_(\"remote %s already exists.\"), rename.new);\n \n \tstrbuf_addf(&buf, \"refs/heads/test:refs/remotes/%s/test\", rename.new);\n@@ -753,7 +753,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_is_configured(remote))\n+\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote: %s\"), argv[1]);\n \n \tknown_remotes.to_delete = remote;\n@@ -1415,7 +1415,7 @@ static int set_remote_branches(const char *remotename, const char **branches,\n \tstrbuf_addf(&key, \"remote.%s.fetch\", remotename);\n \n \tremote = remote_get(remotename);\n-\tif (!remote_is_configured(remote))\n+\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {\n@@ -1469,7 +1469,7 @@ static int get_url(int argc, const char **argv)\n \tremotename = argv[0];\n \n \tremote = remote_get(remotename);\n-\tif (!remote_is_configured(remote))\n+\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \turl_nr = 0;\n@@ -1537,7 +1537,7 @@ static int set_url(int argc, const char **argv)\n \t\toldurl = newurl;\n \n \tremote = remote_get(remotename);\n-\tif (!remote_is_configured(remote))\n+\tif (!remote_is_configured(remote, 1))\n \t\tdie(_(\"No such remote '%s'\"), remotename);\n \n \tif (push_mode) {\ndiff --git a/remote.c b/remote.c\nindex ad6c5424ed..8524135de4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -255,6 +255,7 @@ static void read_remotes_file(struct remote *remote)\n \n \tif (!f)\n \t\treturn;\n+\tremote->configured_in_repo = 1;\n \tremote->origin = REMOTE_REMOTES;\n \twhile (strbuf_getline(&buf, f) != EOF) {\n \t\tconst char *v;\n@@ -289,6 +290,7 @@ static void read_branches_file(struct remote *remote)\n \t\treturn;\n \t}\n \n+\tremote->configured_in_repo = 1;\n \tremote->origin = REMOTE_BRANCHES;\n \n \t/*\n@@ -371,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t}\n \tremote = make_remote(name, namelen);\n \tremote->origin = REMOTE_CONFIG;\n+\tif (current_config_scope() == CONFIG_SCOPE_REPO)\n+\t\tremote->configured_in_repo = 1;\n \tif (!strcmp(subkey, \"mirror\"))\n \t\tremote->mirror = git_config_bool(key, value);\n \telse if (!strcmp(subkey, \"skipdefaultupdate\"))\n@@ -714,9 +718,13 @@ struct remote *pushremote_get(const char *name)\n \treturn remote_get_1(name, pushremote_for_branch);\n }\n \n-int remote_is_configured(struct remote *remote)\n+int remote_is_configured(struct remote *remote, int in_repo)\n {\n-\treturn remote && remote->origin;\n+\tif (!remote)\n+\t\treturn 0;\n+\tif (in_repo)\n+\t\treturn remote->configured_in_repo;\n+\treturn !!remote->origin;\n }\n \n int for_each_remote(each_remote_fn fn, void *priv)\ndiff --git a/remote.h b/remote.h\nindex 924881169d..a5bbbe0ef9 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -15,7 +15,7 @@ struct remote {\n \tstruct hashmap_entry ent; /* must be first */\n \n \tconst char *name;\n-\tint origin;\n+\tint origin, configured_in_repo;\n \n \tconst char *foreign_vcs;\n \n@@ -60,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(struct remote *remote);\n+int remote_is_configured(struct remote *remote, int in_repo);\n \n typedef int each_remote_fn(struct remote *remote, void *priv);\n int for_each_remote(each_remote_fn fn, void *priv);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 2c03f44c85..ba46e86de0 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -764,7 +764,7 @@ test_expect_success 'rename a remote with name prefix of other remote' '\n \t)\n '\n \n-test_expect_failure 'rename succeeds with existing remote..prune' '\n+test_expect_success 'rename succeeds with existing remote..prune' '\n \tgit clone one four.four &&\n \ttest_when_finished git config --global --unset remote.upstream.prune &&\n \tgit config --global remote.upstream.prune true &&\n-- \n2.11.0.windows.3\n"},{"id":"309791","messageId":"alpine.DEB.2.20.1701192241430.3469@virtualbox","threadId":"44900","inReplyTo":"20170119213100.g72ml7r2khu7bvey@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-19T21:44:13Z","receivedAt":"2017-01-19T21:44:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 19 Jan 2017, Jeff King wrote:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index c85f3471d..9024cfffa 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1014,9 +1014,9 @@ static int add_remote_or_group(const char *name, struct string_list *list)\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\t\treturn 0;\n> \t\tremote = remote_get(name);\n> +\t\tif (!remote->fetch_refspec_nr)\n> +\t\t\treturn 0;\n\nPlease note that it is legitimate to fetch from foreign vcs, in which case\nthe fetch refspecs may not be required (or even make sense).\n\n> It's outside the scope of your patches, so I think we are OK to just\n> ignore it. But if you want to pursue it, it avoids having to add the\n> extra parameter to remote_is_configured().\n\nSure, it would avoid that. But that parameter makes semantic sense: some\ncallers may want to have all remotes, even those configured via the\ncommand-line, and others really are only interested in knowing whether the\ncurrent repository config already has settings for a remote of the given\nname.\n\n> > Many thanks to Jeff King whose tireless review helped with settling for\n> > nothing less than the current strategy.\n> \n> Just how I wanted to be immortalized in git's commit history. ;)\n\nYou are welcome ;-)\n\nCiao,\nJohannes\n"},{"id":"309792","messageId":"xmqqy3y6yb9i.fsf@gitster.mtv.corp.google.com","threadId":"44900","inReplyTo":"20170119213100.g72ml7r2khu7bvey@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-19T21:45:29Z","receivedAt":"2017-01-19T21:45:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm trying to figure out why \"fetch --multiple\" wouldn't just take a url\n> in the first place. I guess it is because multiple fetch is useless\n> without refspecs (since otherwise you're just writing to FETCH_HEAD,\n> which gets immediately overwritten).\n\nThis is probably a tangent, if FETCH_HEAD is overwritten, wouldn't\nthat be a bug in the implementation of --multiple? I somehow\nthought we had an option to tell second and subsequent \"fetch\" to\nappend to FETCH_HEAD instead of overwriting it.\n\n"},{"id":"309794","messageId":"20170119215016.j675tzhmhcku2y43@sigill.intra.peff.net","threadId":"44900","inReplyTo":"xmqqy3y6yb9i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] Be more careful when determining whether a remote was configured","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T21:50:17Z","receivedAt":"2017-01-19T21:51:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 01:45:29PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm trying to figure out why \"fetch --multiple\" wouldn't just take a url\n> > in the first place. I guess it is because multiple fetch is useless\n> > without refspecs (since otherwise you're just writing to FETCH_HEAD,\n> > which gets immediately overwritten).\n> \n> This is probably a tangent, if FETCH_HEAD is overwritten, wouldn't\n> that be a bug in the implementation of --multiple? I somehow\n> thought we had an option to tell second and subsequent \"fetch\" to\n> append to FETCH_HEAD instead of overwriting it.\n\nMaybe. I was just speculating on the reason.\n\nI just disabled that check and tried it with two local repos:\n\n git init\n git fetch --multiple /path/to/one /path/to/two\n cat .git/FETCH_HEAD\n\nand indeed it does seem to append.\n\nThe logic comes from 9c4a036b3 (Teach the --all option to 'git fetch',\n2009-11-09), but it does not seem to give any reasoning. Nor could I\nfind anything on the mailing list. \n\n-Peff\n"}]}