{"thread":{"id":"28274","subject":"[PATCH 1/2] remote: write correct fetch spec when renaming remote 'remote'","startedAt":"2011-09-01T01:50:42Z","lastAt":"2011-09-02T15:55:21Z","messageCount":8,"participants":["Martin von Zweigbergk","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"174652","messageId":"1314841843-19868-1-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"28274","inReplyTo":null,"subject":"[PATCH 1/2] remote: write correct fetch spec when renaming remote 'remote'","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-09-01T01:50:42Z","receivedAt":"2011-09-01T01:50:42Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"When renaming a remote whose name is contained in a configured fetch\nrefspec for that remote, we currently replace the first occurrence of\nthe remote name in the refspec. This is correct in most cases, but\nbreaks if the remote name occurs in the fetch refspec before the\nexpected place. For example, we currently change\n\n[remote \"remote\"]\n\turl = git://git.kernel.org/pub/scm/git/git.git\n\tfetch = +refs/heads/*:refs/remotes/remote/*\n\ninto\n\n[remote \"origin\"]\n\turl = git://git.kernel.org/pub/scm/git/git.git\n\tfetch = +refs/heads/*:refs/origins/remote/*\n\nReduce the risk of changing incorrect sections of the refspec by\nrequiring the string to be matched to have leading and trailing\nslashes, i.e. match \"/<name>/\" instead of just \"<name>\".\n\nWe could have required even a leading \":refs/remotes/\", but that would\nmean that we would limit the types of refspecs we could help the user\nupdate.\n\nSigned-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>\n---\n builtin/remote.c  |   12 ++++++++----\n t/t5505-remote.sh |   20 ++++++++++++++++++++\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f2a9c26..c1763ed 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -621,7 +621,8 @@ static int mv(int argc, const char **argv)\n \t\tOPT_END()\n \t};\n \tstruct remote *oldremote, *newremote;\n-\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT;\n+\tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,\n+\t\told_remote_context = STRBUF_INIT;\n \tstruct string_list remote_branches = STRING_LIST_INIT_NODUP;\n \tstruct rename_info rename;\n \tint i;\n@@ -659,15 +660,18 @@ static int mv(int argc, const char **argv)\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new);\n \tif (git_config_set_multivar(buf.buf, NULL, NULL, 1))\n \t\treturn error(\"Could not remove config section '%s'\", buf.buf);\n+\tstrbuf_addf(&old_remote_context, \"/%s/\", rename.old);\n \tfor (i = 0; i < oldremote->fetch_refspec_nr; i++) {\n \t\tchar *ptr;\n \n \t\tstrbuf_reset(&buf2);\n \t\tstrbuf_addstr(&buf2, oldremote->fetch_refspec[i]);\n-\t\tptr = strstr(buf2.buf, rename.old);\n+\t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\tif (ptr)\n-\t\t\tstrbuf_splice(&buf2, ptr-buf2.buf, strlen(rename.old),\n-\t\t\t\t\trename.new, strlen(rename.new));\n+\t\t\tstrbuf_splice(&buf2,\n+\t\t\t\t      ptr-buf2.buf + 1,\n+\t\t\t\t      strlen(rename.old), rename.new,\n+\t\t\t\t      strlen(rename.new));\n \t\tif (git_config_set_multivar(buf.buf, buf2.buf, \"^$\", 0))\n \t\t\treturn error(\"Could not append '%s'\", buf.buf);\n \t}\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 0d0222e..7b6f443 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -631,6 +631,26 @@ test_expect_success 'rename a remote' '\n \n '\n \n+test_expect_success 'rename a remote with non-default fetch refspec' '\n+\n+\tgit clone one four.one &&\n+\t(cd four.one &&\n+\t git config remote.origin.fetch +refs/heads/*:refs/heads/origin/* &&\n+\t git remote rename origin upstream &&\n+\t test \"$(git config remote.upstream.fetch)\" = \"+refs/heads/*:refs/heads/upstream/*\")\n+\n+'\n+\n+test_expect_success 'rename a remote with name part of fetch spec' '\n+\n+\tgit clone one four.two &&\n+\t(cd four.two &&\n+\t git remote rename origin remote &&\n+\t git remote rename remote upstream &&\n+\t test \"$(git config remote.upstream.fetch)\" = \"+refs/heads/*:refs/remotes/upstream/*\")\n+\n+'\n+\n cat > remotes_origin << EOF\n URL: $(pwd)/one\n Push: refs/heads/master:refs/heads/upstream\n-- \n1.7.6.51.g07e0e\n"},{"id":"174653","messageId":"1314841843-19868-2-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"28274","inReplyTo":"1314841843-19868-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"[PATCH 2/2] remote: \"rename o foo\" should not rename ref \"origin/bar\"","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-09-01T01:50:43Z","receivedAt":"2011-09-01T01:50:43Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"When renaming a remote called 'o' using 'git remote rename o foo', git\nshould also rename any remote-tracking branches for the remote. This\ndoes happen, but any remote-tracking branches starting with\n'refs/remotes/o', such as 'refs/remotes/origin/bar', will also be\nrenamed (to 'refs/remotes/foorigin/bar' in this case).\n\nFix it by simply matching one more character, up to the slash\nfollowing the remote name.\n\nSigned-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>\n---\n builtin/remote.c  |    2 +-\n t/t5505-remote.sh |   10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c1763ed..127cff4 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -570,7 +570,7 @@ static int read_remote_branches(const char *refname,\n \tunsigned char orig_sha1[20];\n \tconst char *symref;\n \n-\tstrbuf_addf(&buf, \"refs/remotes/%s\", rename->old);\n+\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old);\n \tif (!prefixcmp(refname, buf.buf)) {\n \t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n \t\tsymref = resolve_ref(refname, orig_sha1, 1, &flag);\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 7b6f443..57b584c 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -651,6 +651,16 @@ test_expect_success 'rename a remote with name part of fetch spec' '\n \n '\n \n+test_expect_success 'rename a remote with name prefix of other remote' '\n+\n+\tgit clone one four.three &&\n+\t(cd four.three &&\n+\t git remote add o git://example.com/repo.git &&\n+\t git remote rename o upstream &&\n+\t test \"$(git rev-parse origin/master)\" = \"$(git rev-parse master)\")\n+\n+'\n+\n cat > remotes_origin << EOF\n URL: $(pwd)/one\n Push: refs/heads/master:refs/heads/upstream\n-- \n1.7.6.51.g07e0e\n"},{"id":"174655","messageId":"20110901024211.GC31838@sigill.intra.peff.net","threadId":"28274","inReplyTo":"1314841843-19868-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH 1/2] remote: write correct fetch spec when renaming remote 'remote'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-01T02:42:11Z","receivedAt":"2011-09-01T02:42:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2011 at 09:50:42PM -0400, Martin von Zweigbergk wrote:\n\n> When renaming a remote whose name is contained in a configured fetch\n> refspec for that remote, we currently replace the first occurrence of\n> the remote name in the refspec. This is correct in most cases, but\n> breaks if the remote name occurs in the fetch refspec before the\n> expected place. For example, we currently change\n> \n> [remote \"remote\"]\n> \turl = git://git.kernel.org/pub/scm/git/git.git\n> \tfetch = +refs/heads/*:refs/remotes/remote/*\n> \n> into\n> \n> [remote \"origin\"]\n> \turl = git://git.kernel.org/pub/scm/git/git.git\n> \tfetch = +refs/heads/*:refs/origins/remote/*\n\nOops.\n\n> Reduce the risk of changing incorrect sections of the refspec by\n> requiring the string to be matched to have leading and trailing\n> slashes, i.e. match \"/<name>/\" instead of just \"<name>\".\n\nDoesn't this just mean that:\n\n  git remote rename remotes foo\n\nwill break in the same way?\n\n> We could have required even a leading \":refs/remotes/\", but that would\n> mean that we would limit the types of refspecs we could help the user\n> update.\n\nActually, I think it's better to be more conservative, and rename only:\n\n  refs/remotes/$OLD/\n\ninto\n\n  refs/remotes/$NEW/\n\nIf we are tweaking the refspecs, it's because we assume that the refspec\nfollows a certain naming convention (i.e., the one we set up with \"git\nremote add\"). If somebody has tweaked this to be:\n\n  refs/heads/$OLD/*\n\nor even:\n\n  refs/heads/foo/$OLD/bar\n\nthen we are just guessing about what they want. And I suspect such a\nperson would not use \"git remote rename\", anyway, but would instead edit\nthe config themselves. I'd rather make it safe for people using the\ndefault config, and people who want to stray from that can deal with\nupdating the config themselves (since they would have to have done so to\nget into that state in the first place).\n\nMaybe we should even print a warning in that case.\n\n-Peff\n"},{"id":"174656","messageId":"20110901024617.GD31838@sigill.intra.peff.net","threadId":"28274","inReplyTo":"1314841843-19868-2-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH 2/2] remote: \"rename o foo\" should not rename ref \"origin/bar\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-01T02:46:17Z","receivedAt":"2011-09-01T02:46:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 31, 2011 at 09:50:43PM -0400, Martin von Zweigbergk wrote:\n\n> When renaming a remote called 'o' using 'git remote rename o foo', git\n> should also rename any remote-tracking branches for the remote. This\n> does happen, but any remote-tracking branches starting with\n> 'refs/remotes/o', such as 'refs/remotes/origin/bar', will also be\n> renamed (to 'refs/remotes/foorigin/bar' in this case).\n\nTo be totally correct, shouldn't this check each ref against the RHS of\nthe remote's old refspec, and rename it according to the remote's new\nrefspec?\n\nMaybe that is just being pedantic, though. This should work fine with\nthe default config[1].\n\n-Peff\n\n[1] Since this part of the renaming process obviously depends heavily on\n    refs/remotes/$OLD being the naming convention, shouldn't the\n    renaming of the refspecs do the same thing? I.e., it's another\n    reason that your patch 1/2 should only tweak refs/remotes/$OLD.\n    Otherwise you will get renamed refspecs, but the actual refs won't\n    be moved.\n"},{"id":"174657","messageId":"7vk49tx84v.fsf@alter.siamese.dyndns.org","threadId":"28274","inReplyTo":"20110901024211.GC31838@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] remote: write correct fetch spec when renaming remote 'remote'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-01T03:18:40Z","receivedAt":"2011-09-01T03:18:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Actually, I think it's better to be more conservative, and rename only:\n>\n>   refs/remotes/$OLD/\n>\n> into\n>\n>   refs/remotes/$NEW/\n\nThanks.\n\nI wholeheartedly agree with this. It would let me sleep better if we did\nthis rewriting only when remote.$name.fetch is set exactly to its default\nvalue, \"+refs/heads/*:refs/origin/$name/*\" (it is Ok if the leading '+' is\nmissing, though), and we didn't touch either config nor the existing\ntracking refs at all otherwise, and gave warning saying we didn't do\nanything to them.\n"},{"id":"174722","messageId":"alpine.DEB.2.00.1109011945560.7066@debian","threadId":"28274","inReplyTo":"20110901024211.GC31838@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] remote: write correct fetch spec when renaming remote 'remote'","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-09-02T00:02:53Z","receivedAt":"2011-09-02T00:02:53Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"\nOn Wed, 31 Aug 2011, Jeff King wrote:\n\n> On Wed, Aug 31, 2011 at 09:50:42PM -0400, Martin von Zweigbergk wrote:\n> \n> > Reduce the risk of changing incorrect sections of the refspec by\n> > requiring the string to be matched to have leading and trailing\n> > slashes, i.e. match \"/<name>/\" instead of just \"<name>\".\n> \n> Doesn't this just mean that:\n> \n>   git remote rename remotes foo\n> \n> will break in the same way?\n\nYes, \"r\", \"ead\" and such now work and only exactly \"refs\", \"heads\" and\n\"remotes\" don't work. Sorry, forgot to mention that in the commit\nmessage.\n\n> > We could have required even a leading \":refs/remotes/\", but that would\n> > mean that we would limit the types of refspecs we could help the user\n> > update.\n> \n> Actually, I think it's better to be more conservative [...]\n\nI think you are right. That's what I did (match all the way from\n\"refs/...\") at first, but then I thought that maybe the reason the\nmatching was relaxed was to allow non-default refspecs. However, as\nyou point out in the footnote to your reply to PATCH 2/2, that case is\nnot working consistently anyway, so it seems safe (w.r.t. backwards\ncompatibility too) to ignore it. Will switch back to matching\n\":refs/remotes/$OLD\".\n\n\nMartin\n"},{"id":"174723","messageId":"alpine.DEB.2.00.1109012022110.7267@debian","threadId":"28274","inReplyTo":"20110901024617.GD31838@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] remote: \"rename o foo\" should not rename ref \"origin/bar\"","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2011-09-02T00:35:01Z","receivedAt":"2011-09-02T00:35:01Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"\nOn Wed, 31 Aug 2011, Jeff King wrote:\n\n> On Wed, Aug 31, 2011 at 09:50:43PM -0400, Martin von Zweigbergk wrote:\n> \n> > When renaming a remote called 'o' using 'git remote rename o foo', git\n> > should also rename any remote-tracking branches for the remote. This\n> > does happen, but any remote-tracking branches starting with\n> > 'refs/remotes/o', such as 'refs/remotes/origin/bar', will also be\n> > renamed (to 'refs/remotes/foorigin/bar' in this case).\n> \n> To be totally correct, shouldn't this check each ref against the RHS of\n> the remote's old refspec, and rename it according to the remote's new\n> refspec?\n\nThat's what I thought too, but was planning on leaving it for a\nseparate patch. However, after changing patch 1 to only update the\nfetch refspecs from \"refs/remotes/$OLD\" to \"refs/remotes/$NEW\", there\nis no other place in the fetchspec where a remote name can occur and\n'git remote rename' still understands it. So since we're now being\nmore conservative about updating refspecs, I guess we need to be\nequally conservative about updating ref names.\n\n\nMartin\n"},{"id":"174751","messageId":"20110902155521.GA19690@sigill.intra.peff.net","threadId":"28274","inReplyTo":"alpine.DEB.2.00.1109012022110.7267@debian","subject":"Re: [PATCH 2/2] remote: \"rename o foo\" should not rename ref \"origin/bar\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-02T15:55:21Z","receivedAt":"2011-09-02T15:55:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 01, 2011 at 08:35:01PM -0400, Martin von Zweigbergk wrote:\n\n> > To be totally correct, shouldn't this check each ref against the RHS of\n> > the remote's old refspec, and rename it according to the remote's new\n> > refspec?\n> \n> That's what I thought too, but was planning on leaving it for a\n> separate patch. However, after changing patch 1 to only update the\n> fetch refspecs from \"refs/remotes/$OLD\" to \"refs/remotes/$NEW\", there\n> is no other place in the fetchspec where a remote name can occur and\n> 'git remote rename' still understands it. So since we're now being\n> more conservative about updating refspecs, I guess we need to be\n> equally conservative about updating ref names.\n\nYeah, I think with the change in patch 1, your patch 2 is very\nreasonable. It is admitting that \"git remote\" is really about tweaking\nthe default remote config, and punting on config that doesn't appear to\nfollow it. And doing it consistently. I don't think there's nothing\nwrong with that.\n\n-Peff\n"}]}