{"thread":{"id":"34233","subject":"[BUG?] remote prune origin interacts badly with clone --mirror and multiple remotes","startedAt":"2013-06-20T21:23:44Z","lastAt":"2013-06-26T23:42:36Z","messageCount":23,"participants":["Dennis Kaarsemaker","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"221523","messageId":"1371763424.17896.32.camel@localhost","threadId":"34233","inReplyTo":null,"subject":"[BUG?] remote prune origin interacts badly with clone --mirror and multiple remotes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-20T21:23:44Z","receivedAt":"2013-06-20T21:23:44Z","isPatch":false,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"[git version: next as of yesterday afternoon]\n\nIf I clone a repo with git clone --mirror, and add other remotes later,\n'git remote prune origin' deletes all branches and tags of the other\nremotes.\n\nEasily repeatable example:\n\n[core]\n\trepositoryformatversion = 0\n\tfilemode = true\n\tbare = true\n\tlogallrefupdates = false\n[remote \"origin\"]\n\turl = git://github.com/git/git.git\n\tfetch = +refs/*:refs/*\n\tmirror = true\n[remote \"peff\"]\n\turl = git://github.com/peff/git.git\n\tfetch = +refs/heads/*:refs/remotes/peff/*\n\n'git remote prune origin' will delete all peff's branches in this case.\nI'm guessing the wildcards refs/* and refs/remotes/peff/* interact badly\nin some place, and I'm trying to understand builtin/remote.c to see if I\ncan fix it, but haven't gotten very far yet.\n\ngit fetch --prune origin and git remote update --prune also show this\nbehaviour.\n\ngit remote prune peff does not delete non-peff branches in this\nscenario, further strengthening my belief that the refs/* and\nrefs/remotes/peff/* wildcards interact badly with prune.\n\nOr is this considered normal behaviour and is what I'm trying to do\nsimply unsupported? In that case a warning would be welcome when adding\nremotes to a --mirror'ed repository.\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"221530","messageId":"1371766304-4601-1-git-send-email-dennis@kaarsemaker.net","threadId":"34233","inReplyTo":"1371763424.17896.32.camel@localhost","subject":"[PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-20T22:11:44Z","receivedAt":"2013-06-20T22:11:44Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"When cloning a repo with --mirror, and adding more remotes later,\nget_stale_heads for origin would mark all refs from other repos as stale. In\nthis situation, with refs-src and refs->dst both equal to refs/*, we should\nignore refs/remotes/* when looking for stale refs to prevent this from\nhappening.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n remote.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex e71f66d..863c183 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1884,6 +1884,7 @@ struct stale_heads_info {\n \tstruct ref **stale_refs_tail;\n \tstruct refspec *refs;\n \tint ref_count;\n+\tint ignore_remotes;\n };\n \n static int get_stale_heads_cb(const char *refname,\n@@ -1903,7 +1904,8 @@ static int get_stale_heads_cb(const char *refname,\n \t * remote we consider it to be stale.\n \t */\n \tif (!((flags & REF_ISSYMREF) ||\n-\t      string_list_has_string(info->ref_names, query.src))) {\n+\t      string_list_has_string(info->ref_names, query.src) ||\n+\t      (info->ignore_remotes && !prefixcmp(refname, \"refs/remotes/\")))) {\n \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n \t\thashcpy(ref->new_sha1, sha1);\n \t}\n@@ -1917,6 +1919,8 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet\n \tstruct ref *ref, *stale_refs = NULL;\n \tstruct string_list ref_names = STRING_LIST_INIT_NODUP;\n \tstruct stale_heads_info info;\n+\tif(!strcmp(refs->src, \"refs/*\") && !strcmp(refs->dst, \"refs/*\"))\n+\t\tinfo.ignore_remotes = 1;\n \tinfo.ref_names = &ref_names;\n \tinfo.stale_refs_tail = &stale_refs;\n \tinfo.refs = refs;\n-- \n1.8.3.1-619-gbec0aa7\n"},{"id":"221534","messageId":"7vppvgpfib.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371766304-4601-1-git-send-email-dennis@kaarsemaker.net","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T22:46:20Z","receivedAt":"2013-06-20T22:46:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> When cloning a repo with --mirror, and adding more remotes later,\n> get_stale_heads for origin would mark all refs from other repos as stale. In\n> this situation, with refs-src and refs->dst both equal to refs/*, we should\n> ignore refs/remotes/* when looking for stale refs to prevent this from\n> happening.\n\nI do not think it is a right solution to single out refs/remotes/*.\n\nGoing back to your original example:\n\n    [remote \"origin\"]\n            url = git://github.com/git/git.git\n            fetch = +refs/*:refs/*\n            mirror = true\n    [remote \"peff\"]\n            url = git://github.com/peff/git.git\n            fetch = +refs/heads/*:refs/remotes/peff/*\n\nWouldn't you obtain \"refs/remotes/github/html\" from your \"origin\"\nvia \"git pull origin\"?  What happens to your local copy of that ref,\nwhen it goes away from the origin and then you try to \"fetch --prune\norigin\" the next time with this patch (and without this patch)?\n\nWhat should happen?\n\nWhat if you had this instead of the above version of remote.peff.*?\n\n    [remote \"peff\"]\n            url = git://github.com/peff/git.git\n            fetch = +refs/heads/*:refs/remotes/github/*\n\nI think this is an unsolvable problem, and I _think_ the root cause\nof the issue is the configuration above that allows the RHS of\ndifferent fetch refspecs to overlap.  refs/* is more generic and\ncovers refs/remotes/peff/* and refs/remotes/github/*.  You cannot\neven know, just by looking at \"origin\" and your local repository,\nif refs/remotes/github/html you have should go away or it might have\ncome from somewhere else.\n\nThe best we _could_ do, without contacting all the defined remotes,\nis probably to check each ref that we did not see from \"origin\" (for\nexample, you find \"refs/remotes/peff/frotz\" that your origin does\nnot have) and see if it could match RHS of fetch refspec of somebody\nelse (e.g. RHS of \"refs/heads/*:refs/remotes/peff/*\" matches that\nref).  Then we can conclude that refs/remotes/peff/frotz _might_\nhave come from Peff's repository and not from \"origin\", and then we\ncan optionally issue a warning and refrain from removing it.\n\nThis inevitably will have false positives and leave something that\ndid originally came from \"origin\", because peff may no longer have\n'frotz' branch in his repository.  I do not think we can do better\nthan that, because we are trying to see if we can improve things\nwithout having to contact all the remotes.\n\nBut if you go that route, the logic needs to go the same way when\nyou are pruning against 'peff', and anything that you do not see in\nhis repository right now but you have in refs/remotes/peff/ cannot\nbe pruned, because it might have come from your origin via more\ngeneric refs/*:refs/* mapping.  It follows that you could never\nprune anything under refs/remotes/peff/* hierarchy.\n\nYou could introduce a \"assume that more specific mapping never\noverlaps with a more generic mapping\" rule (i.e. refs/* from RHS of\nremote.origin.fetch is more generic than refs/remotes/peff/* from\nRHS of remote.peff.fetch, and assume everything that you see in your\nlocal refs/remotes/peff/* came from peff and not from origin, I\nthink, but at that point, is it worth the possible complexity to\ncode that rule in the prune codepath and brittleness of that\nassumption that your origin will never add a new ref under that\nhierarchy, e.g. refs/remotes/peff/xyzzy?\n\nSo, I dunno.\n\n> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n> ---\n>  remote.c | 6 +++++-\n>  1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index e71f66d..863c183 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1884,6 +1884,7 @@ struct stale_heads_info {\n>  \tstruct ref **stale_refs_tail;\n>  \tstruct refspec *refs;\n>  \tint ref_count;\n> +\tint ignore_remotes;\n>  };\n>  \n>  static int get_stale_heads_cb(const char *refname,\n> @@ -1903,7 +1904,8 @@ static int get_stale_heads_cb(const char *refname,\n>  \t * remote we consider it to be stale.\n>  \t */\n>  \tif (!((flags & REF_ISSYMREF) ||\n> -\t      string_list_has_string(info->ref_names, query.src))) {\n> +\t      string_list_has_string(info->ref_names, query.src) ||\n> +\t      (info->ignore_remotes && !prefixcmp(refname, \"refs/remotes/\")))) {\n>  \t\tstruct ref *ref = make_linked_ref(refname, &info->stale_refs_tail);\n>  \t\thashcpy(ref->new_sha1, sha1);\n>  \t}\n> @@ -1917,6 +1919,8 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet\n>  \tstruct ref *ref, *stale_refs = NULL;\n>  \tstruct string_list ref_names = STRING_LIST_INIT_NODUP;\n>  \tstruct stale_heads_info info;\n> +\tif(!strcmp(refs->src, \"refs/*\") && !strcmp(refs->dst, \"refs/*\"))\n> +\t\tinfo.ignore_remotes = 1;\n>  \tinfo.ref_names = &ref_names;\n>  \tinfo.stale_refs_tail = &stale_refs;\n>  \tinfo.refs = refs;\n"},{"id":"221536","messageId":"1371768806-5666-1-git-send-email-dennis@kaarsemaker.net","threadId":"34233","inReplyTo":"1371763424.17896.32.camel@localhost","subject":"[PATCH v2] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-20T22:53:26Z","receivedAt":"2013-06-20T22:53:26Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"When cloning a repo with --mirror, and adding more remotes later,\nget_stale_heads for origin would mark all refs from other repos as stale. In\nthis situation, with refs-src and refs->dst both equal to refs/*, we should\nignore refs/remotes/* and refs/tags/* when looking for stale refs to\nprevent this from happening.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n The previous attempt only ignored refs/remotes, but that's not good enough as\n that will still delete tags. So let's ignore refs/tags too. The downside is\n that tags removed at the origin don't get removed, but prune should only be\n pruning branches anyway if I read the documentation correctly.\n\n remote.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex e71f66d..efc5481 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1884,6 +1884,7 @@ struct stale_heads_info {\n \tstruct ref **stale_refs_tail;\n \tstruct refspec *refs;\n \tint ref_count;\n+\tint is_mirror;\n };\n \n static int get_stale_heads_cb(const char *refname,\n@@ -1896,6 +1897,13 @@ static int get_stale_heads_cb(const char *refname,\n \n \tif (query_refspecs(info->refs, info->ref_count, &query))\n \t\treturn 0; /* No matches */\n+\t/*\n+\t * If we're pruning a clone that was --mirror'ed, let's ignore refs/tags\n+\t * and refs/remotes\n+\t */\n+\tif (info->is_mirror && (!prefixcmp(refname, \"refs/tags/\") ||\n+\t    !prefixcmp(refname, \"refs/remotes/\")))\n+\t\treturn 0;\n \n \t/*\n \t * If we did find a suitable refspec and it's not a symref and\n@@ -1917,6 +1925,8 @@ struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fet\n \tstruct ref *ref, *stale_refs = NULL;\n \tstruct string_list ref_names = STRING_LIST_INIT_NODUP;\n \tstruct stale_heads_info info;\n+\tif(!strcmp(refs->src, \"refs/*\") && !strcmp(refs->dst, \"refs/*\"))\n+\t\tinfo.is_mirror = 1;\n \tinfo.ref_names = &ref_names;\n \tinfo.stale_refs_tail = &stale_refs;\n \tinfo.refs = refs;\n-- \n1.8.3.1-619-gbec0aa7\n"},{"id":"221537","messageId":"1371769636.17896.44.camel@localhost","threadId":"34233","inReplyTo":"7vppvgpfib.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-20T23:07:16Z","receivedAt":"2013-06-20T23:07:16Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"(Sorry, I sent v2 before seeing this mail)\n\nOn do, 2013-06-20 at 15:46 -0700, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > When cloning a repo with --mirror, and adding more remotes later,\n> > get_stale_heads for origin would mark all refs from other repos as stale. In\n> > this situation, with refs-src and refs->dst both equal to refs/*, we should\n> > ignore refs/remotes/* when looking for stale refs to prevent this from\n> > happening.\n> \n> I do not think it is a right solution to single out refs/remotes/*.\n> \n> Going back to your original example:\n> \n>     [remote \"origin\"]\n>             url = git://github.com/git/git.git\n>             fetch = +refs/*:refs/*\n>             mirror = true\n>     [remote \"peff\"]\n>             url = git://github.com/peff/git.git\n>             fetch = +refs/heads/*:refs/remotes/peff/*\n> \n> Wouldn't you obtain \"refs/remotes/github/html\" from your \"origin\"\n> via \"git pull origin\"?  What happens to your local copy of that ref,\n> when it goes away from the origin and then you try to \"fetch --prune\n> origin\" the next time with this patch (and without this patch)?\n\ngit pull origin gives me refs/html in this case. I did not try fetch\n--prune, but prune origin DTRT: if the html branch goes away at the\norigin, it goes away locally. Both with and without this patch.\n\nIt's refs/remotes/peff/somebranch that in this case *also* goes away\nwithout this patch, but is untouched with this patch\n\n> What should happen?\n\nExactly that.\n\n> What if you had this instead of the above version of remote.peff.*?\n> \n>     [remote \"peff\"]\n>             url = git://github.com/peff/git.git\n>             fetch = +refs/heads/*:refs/remotes/github/*\n\nThat doesn't change anything.\n\n> I think this is an unsolvable problem, and I _think_ the root cause\n> of the issue is the configuration above that allows the RHS of\n> different fetch refspecs to overlap.  refs/* is more generic and\n> covers refs/remotes/peff/* and refs/remotes/github/*.  You cannot\n> even know, just by looking at \"origin\" and your local repository,\n> if refs/remotes/github/html you have should go away or it might have\n> come from somewhere else.\n> \n> The best we _could_ do, without contacting all the defined remotes,\n> is probably to check each ref that we did not see from \"origin\" (for\n> example, you find \"refs/remotes/peff/frotz\" that your origin does\n> not have) and see if it could match RHS of fetch refspec of somebody\n> else (e.g. RHS of \"refs/heads/*:refs/remotes/peff/*\" matches that\n> ref).  Then we can conclude that refs/remotes/peff/frotz _might_\n> have come from Peff's repository and not from \"origin\", and then we\n> can optionally issue a warning and refrain from removing it.\n\nI like that idea, though I also like the simplicity of simply singling\nout \"remotes\" as that's where normal remotes usually sit. And don't\nforget about tags (see patch v2).\n\n> This inevitably will have false positives and leave something that\n> did originally came from \"origin\", because peff may no longer have\n> 'frotz' branch in his repository.  I do not think we can do better\n> than that, because we are trying to see if we can improve things\n> without having to contact all the remotes.\n\nBut then the ref would have to be called \"refs/remotes/peff/frotz\"\nupstream. Hmm, that is of course completely possible: cloning something\nthat's already a clone.\n\n> But if you go that route, the logic needs to go the same way when\n> you are pruning against 'peff', and anything that you do not see in\n> his repository right now but you have in refs/remotes/peff/ cannot\n> be pruned, because it might have come from your origin via more\n> generic refs/*:refs/* mapping.  It follows that you could never\n> prune anything under refs/remotes/peff/* hierarchy.\n> \n> You could introduce a \"assume that more specific mapping never\n> overlaps with a more generic mapping\" rule (i.e. refs/* from RHS of\n> remote.origin.fetch is more generic than refs/remotes/peff/* from\n> RHS of remote.peff.fetch, and assume everything that you see in your\n> local refs/remotes/peff/* came from peff and not from origin, I\n> think, but at that point, is it worth the possible complexity to\n> code that rule in the prune codepath and brittleness of that\n> assumption that your origin will never add a new ref under that\n> hierarchy, e.g. refs/remotes/peff/xyzzy?\n> \n> So, I dunno.\n\nYeah, I'm starting to think this is not such a good idea. How about plan\nB: issuing a warning when adding a remote with a refspec that also\nmatches another remote's refspec?\n\nOr plan C: add a per-remote pruneIgnore setting that in this case I\ncould set to refs/tags/* refs/remotes/* as I know it's correct? Could\neven be combined with plan B.\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"221538","messageId":"20130620230843.GA5780@sigill.intra.peff.net","threadId":"34233","inReplyTo":"7vppvgpfib.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-06-20T23:08:44Z","receivedAt":"2013-06-20T23:08:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 20, 2013 at 03:46:20PM -0700, Junio C Hamano wrote:\n\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > When cloning a repo with --mirror, and adding more remotes later,\n> > get_stale_heads for origin would mark all refs from other repos as stale. In\n> > this situation, with refs-src and refs->dst both equal to refs/*, we should\n> > ignore refs/remotes/* when looking for stale refs to prevent this from\n> > happening.\n> \n> I do not think it is a right solution to single out refs/remotes/*.\n\nYeah, I agree.\n\n> Going back to your original example:\n> \n>     [remote \"origin\"]\n>             url = git://github.com/git/git.git\n>             fetch = +refs/*:refs/*\n>             mirror = true\n>     [remote \"peff\"]\n>             url = git://github.com/peff/git.git\n>             fetch = +refs/heads/*:refs/remotes/peff/*\n\nThere is a fundamental namespace conflict here: one remote is claiming\nthe whole refs/* namespace, and another remote is claiming some subset.\n\"fetch --prune\" is only one type of problem you can have; you might also\noverwrite stuff from the \"peff\" remote with stuff from the \"origin\"\nremote (if it happens to have \"refs/remotes/peff/foo\" itself).\n\n> I think this is an unsolvable problem, and I _think_ the root cause\n> of the issue is the configuration above that allows the RHS of\n> different fetch refspecs to overlap.  refs/* is more generic and\n> covers refs/remotes/peff/* and refs/remotes/github/*.  You cannot\n> even know, just by looking at \"origin\" and your local repository,\n> if refs/remotes/github/html you have should go away or it might have\n> come from somewhere else.\n\nExactly.\n\n> The best we _could_ do, without contacting all the defined remotes,\n> is probably to check each ref that we did not see from \"origin\" (for\n> example, you find \"refs/remotes/peff/frotz\" that your origin does\n> not have) and see if it could match RHS of fetch refspec of somebody\n> else (e.g. RHS of \"refs/heads/*:refs/remotes/peff/*\" matches that\n> ref).  Then we can conclude that refs/remotes/peff/frotz _might_\n> have come from Peff's repository and not from \"origin\", and then we\n> can optionally issue a warning and refrain from removing it.\n\nI think this is just papering over the problem in one instance. What\nhappens when you _do_ have overlapping refs in the \"origin\" remote, and\nyou have a true conflict.\n\nI wonder why Dennis wants to \"refs/*:refs/*\" in the first place. It\nis not usually a useful thing to have in a non-bare repository, because\nfetches will overwrite local work on branches. If he just wanted the\nautomatic \"git push --mirror\" setting, that does not depend on the fetch\nrefspec.\n\nWe made this distinction in \"git remote --mirror={fetch,remote}\", but I\ndon't think \"git clone --mirror\" ever learned about it.\n\n-Peff\n"},{"id":"221539","messageId":"1371770988.17896.58.camel@localhost","threadId":"34233","inReplyTo":"20130620230843.GA5780@sigill.intra.peff.net","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-20T23:29:48Z","receivedAt":"2013-06-20T23:29:48Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On do, 2013-06-20 at 19:08 -0400, Jeff King wrote:\n\n> I wonder why Dennis wants to \"refs/*:refs/*\" in the first place. It\n> is not usually a useful thing to have in a non-bare repository,\n> because fetches will overwrite local work on branches. If he just\n> wanted the automatic \"git push --mirror\" setting, that does not depend\n> on the fetch refspec.\n\nI'm not doing that in non-bare repositories, neither do I use it for\npushing. It's for a continuous integration system, which never has any\nlocally created branches or commits, but does integrate things from\ndifferent remotes in some cases. The example with git.git is used\nroughly as follows:\n\n* git fetch all remotes (for most projects that will be 1 remote)\n* rebuild reflogs from github events (or fetch via http/ssh)\n* per push to next, check out to a separate $GIT_WORK_TREE and run make \n  test\n* for the last push, also build and publish daily tarball+deb+rpm\n\nThen, for further testing of local requirements:\n\n* cherry-pick your jk/blame_tree branch\n* test, build and install package\n\nGiven that this system works with clones of what should be considered\ncanonical copies of repositories, those remotes shouldn't have any\nremotes defined themselves, so at least being able to configure prune to\nignore refs/remotes/* and refs/tags/* would help me a lot.\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"221540","messageId":"7vhagspdfx.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371769636.17896.44.camel@localhost","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T23:30:58Z","receivedAt":"2013-06-20T23:30:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n>> Going back to your original example:\n>> \n>>     [remote \"origin\"]\n>>             url = git://github.com/git/git.git\n>>             fetch = +refs/*:refs/*\n>>             mirror = true\n>>     [remote \"peff\"]\n>>             url = git://github.com/peff/git.git\n>>             fetch = +refs/heads/*:refs/remotes/peff/*\n>> \n>> Wouldn't you obtain \"refs/remotes/github/html\" from your \"origin\"\n>> via \"git pull origin\"?  What happens to your local copy of that ref,\n>> when it goes away from the origin and then you try to \"fetch --prune\n>> origin\" the next time with this patch (and without this patch)?\n>\n> git pull origin gives me refs/html in this case.\n\nMaybe there is a miscommunication.\n\n\t$ git ls-remote git://github.com/git/git.git | grep remotes/\n\nshows that that repository, your origin, has refs/remotes/github/html\n\nAnd you have remote.origin.fetch = +refs/*:refs/* in your local\nrepository.  So that ref will come to your local repository under\nthe same name, i.e. refs/remotes/github/html, no?  Not as refs/html.\n\nSo this answer\n\n>> What if you had this instead of the above version of remote.peff.*?\n>> \n>>     [remote \"peff\"]\n>>             url = git://github.com/peff/git.git\n>>             fetch = +refs/heads/*:refs/remotes/github/*\n>\n> That doesn't change anything.\n\ncannot be correct.  You fetch from your origin and then its\nrefs/remotes/github/html *will* want to come to your local\nrepository and be stored there.  If Peff has html branch in that\nhypothetical configuration, you fetch from him and his html branch\n(i.e. refs/heads/html from his point of view) will want to replace\nyour local refs/remotes/github/html.  Or Peff may not have html\nbranch.  The point is that you cannot tell by only looking at your\nlocal ref namespace and what \"origin\" has.\n\n> Yeah, I'm starting to think this is not such a good idea. How about plan\n> B: issuing a warning when adding a remote with a refspec that also\n> matches another remote's refspec?\n\nSurely that will make things safer.\n\n> Or plan C: add a per-remote pruneIgnore setting that in this case I\n> could set to refs/tags/* refs/remotes/* as I know it's correct? Could\n> even be combined with plan B.\n\nAs I already said \"I dunno\", I am not sure if it is worth the effort\nto support overlapping RHSs of fetch refspecs, so between B and C, I\nwould vote for B.\n"},{"id":"221541","messageId":"7vd2rgpd7i.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371770988.17896.58.camel@localhost","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T23:36:01Z","receivedAt":"2013-06-20T23:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> I'm not doing that in non-bare repositories, neither do I use it for\n> pushing. It's for a continuous integration system, which never has any\n> locally created branches or commits, but does integrate things from\n> different remotes in some cases.\n\nPerhaps then\n\n\t[remote \"origin\"]\n        \turl = somewhere\n                fetch = +refs/heads/*:refs/heads/*\n                fetch = +refs/tags/*:refs/tags/*\n\t[remote \"another\"]\n               \turl = somewhere-else\n                fetch = +refs/heads/*:refs/remotes/another/*\n\t[remote \"third\"]\n               \turl = third-place\n                fetch = +refs/heads/*:refs/remotes/third/*\n\nwould be sufficient?  You may not even need tags, but that is an\nindependent issue.\n"},{"id":"221542","messageId":"1371771510.17896.61.camel@localhost","threadId":"34233","inReplyTo":"7vhagspdfx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-20T23:38:30Z","receivedAt":"2013-06-20T23:38:30Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On do, 2013-06-20 at 16:30 -0700, Junio C Hamano wrote:\n\n> Maybe there is a miscommunication.\n> \n> \t$ git ls-remote git://github.com/git/git.git | grep remotes/\n> \n> shows that that repository, your origin, has refs/remotes/github/html\n\nYes, I misunderstood you and see the problem now. Thanks for being\npatient with me :)\n\n> > Yeah, I'm starting to think this is not such a good idea. How about plan\n> > B: issuing a warning when adding a remote with a refspec that also\n> > matches another remote's refspec?\n> \n> Surely that will make things safer.\n> \n> > Or plan C: add a per-remote pruneIgnore setting that in this case I\n> > could set to refs/tags/* refs/remotes/* as I know it's correct? Could\n> > even be combined with plan B.\n> \n> As I already said \"I dunno\", I am not sure if it is worth the effort\n> to support overlapping RHSs of fetch refspecs, so between B and C, I\n> would vote for B.\n\nI'm halfway through cooking up a patch for B, as I agree that it will\nmake things safer.\n\nI'd really like to have C as well though, would you accept a patch that\nimplements it?\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"221543","messageId":"7v8v24pcsx.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371771510.17896.61.camel@localhost","subject":"Re: [PATCH] remote: make prune work for mixed mirror/non-mirror repos","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-20T23:44:46Z","receivedAt":"2013-06-20T23:44:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> I'd really like to have C as well though, would you accept a patch that\n> implements it?\n\nI already said I dunno, and asking me 5 minutes after that would not\nchange my answer X-<.  I tend to agree with Peff that it is papering\nover the underlying problem, which B solves.\n"},{"id":"221568","messageId":"1371809051-29988-1-git-send-email-dennis@kaarsemaker.net","threadId":"34233","inReplyTo":"1371763424.17896.32.camel@localhost","subject":"[PATCH 0/3] Handling overlapping refspecs slightly smarter","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-21T10:04:08Z","receivedAt":"2013-06-21T10:04:08Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"1/3 should be pretty sane, just adding a warning in documentation and 'git\nremote add' about overlapping refspecs.\n\n2/3 only makes sense if 3/3 is accepted, as it's a test for that change.\n\n3/3 I'm not 100% sure about, though the approach feels reasonably ok. It changes\nget_stale_heads to also detect overlapping refspecs and abort any prune action\nif it finds them. What I'm not sure about is whether this is the right place to\ndo it, or to do it in the callers of get_stale_heads and exit(1) in this\nsituation.\n\nBoth 1/3 and 3/3 ignore exactly matching refspecs, as that's a supported thing\nalready, another test in t5505 broke before I made both ignore exactly matching\nrefspecs.\n\n\nDennis Kaarsemaker (3):\n  remote: Add warnings about mixin --mirror and other remotes\n  remote: Add test for prune and mixed --mirror and normal remotes\n  remote: don't prune when detecting overlapping refspecs\n\n Documentation/git-remote.txt |  6 +++++-\n builtin/remote.c             | 17 +++++++++++++++++\n remote.c                     | 23 +++++++++++++++++++++++\n t/t5505-remote.sh            |  9 +++++++++\n 4 files changed, 54 insertions(+), 1 deletion(-)\n\n-- \n1.8.3.1-619-gbec0aa7\n"},{"id":"221571","messageId":"1371809051-29988-2-git-send-email-dennis@kaarsemaker.net","threadId":"34233","inReplyTo":"1371809051-29988-1-git-send-email-dennis@kaarsemaker.net","subject":"[PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-21T10:04:09Z","receivedAt":"2013-06-21T10:04:09Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"When cloning a repo with --mirror, and adding more remotes later,\nget_stale_heads for origin will mark all refs from other repos as stale.\nAdd a warning to the documentation, and warn the user when we detect\nsuch things during git remote add.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n Documentation/git-remote.txt |  6 +++++-\n builtin/remote.c             | 17 +++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\nindex 581bb4c..d7457df 100644\n--- a/Documentation/git-remote.txt\n+++ b/Documentation/git-remote.txt\n@@ -71,7 +71,11 @@ When a fetch mirror is created with `--mirror=fetch`, the refs will not\n be stored in the 'refs/remotes/' namespace, but rather everything in\n 'refs/' on the remote will be directly mirrored into 'refs/' in the\n local repository. This option only makes sense in bare repositories,\n-because a fetch would overwrite any local commits.\n+because a fetch would overwrite any local commits. If you want to add extra\n+remotes to a repository created with `--mirror=fetch`, you will need to change\n+the configuration of the mirrored remote to fetch only `refs/heads/*`,\n+otherwise `git remote prune <remote>` will remove all branches for the extra\n+remotes.\n +\n When a push mirror is created with `--mirror=push`, then `git push`\n will always behave as if `--mirror` was passed.\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5e54d36..a4f9cb8 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -112,6 +112,21 @@ enum {\n #define MIRROR_PUSH 2\n #define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH)\n \n+static int check_overlapping_refspec(struct remote *remote, void *priv)\n+{\n+\tchar *refspec = priv;\n+\tint i;\n+\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n+\t\tif(strcmp(refspec, remote->fetch[i].dst) &&\n+\t\t   (!fnmatch(refspec, remote->fetch[i].dst, 0) ||\n+\t\t    !fnmatch(remote->fetch[i].dst, refspec, 0))) {\n+\t\t\twarning(_(\"Overlapping refspecs detected: '%s' and '%s'\"),\n+\t\t\t        refspec, remote->fetch[i].dst);\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n static int add_branch(const char *key, const char *branchname,\n \t\tconst char *remotename, int mirror, struct strbuf *tmp)\n {\n@@ -123,6 +138,8 @@ static int add_branch(const char *key, const char *branchname,\n \telse\n \t\tstrbuf_addf(tmp, \"refs/heads/%s:refs/remotes/%s/%s\",\n \t\t\t\tbranchname, remotename, branchname);\n+\n+\tfor_each_remote(check_overlapping_refspec, strchr((const char*)tmp->buf, ':') + 1);\n \treturn git_config_set_multivar(key, tmp->buf, \"^$\", 0);\n }\n \n-- \n1.8.3.1-619-gbec0aa7\n"},{"id":"221569","messageId":"1371809051-29988-3-git-send-email-dennis@kaarsemaker.net","threadId":"34233","inReplyTo":"1371809051-29988-1-git-send-email-dennis@kaarsemaker.net","subject":"[PATCH 2/3] remote: Add test for prune and mixed --mirror and normal remotes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-21T10:04:10Z","receivedAt":"2013-06-21T10:04:10Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\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 dd10ff0..439e996 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -428,6 +428,15 @@ test_expect_success 'add alt && prune' '\n \t git rev-parse --verify refs/remotes/origin/side2)\n '\n \n+test_expect_failure 'prune and overlapping refspecs' '\n+\t(git clone --mirror one prunetst &&\n+\t cd prunetst &&\n+\t git remote add two ../two &&\n+     git fetch two &&\n+\t git remote prune origin &&\n+     git rev-parse --verify refs/remotes/two/another)\n+'\n+\n cat >test/expect <<\\EOF\n some-tag\n EOF\n-- \n1.8.3.1-619-gbec0aa7\n"},{"id":"221570","messageId":"1371809051-29988-4-git-send-email-dennis@kaarsemaker.net","threadId":"34233","inReplyTo":"1371809051-29988-1-git-send-email-dennis@kaarsemaker.net","subject":"[PATCH 3/3] remote: don't prune when detecting overlapping refspecs","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-21T10:04:11Z","receivedAt":"2013-06-21T10:04:11Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"When cloning a repo with --mirror, and adding more remotes later,\nget_stale_heads for origin would mark all refs from other repos as\nstale. There's no \"good\" way to solve, this so the best thing we can do\nis refusing to prune if we detect this and warning the user.\n\nSigned-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n---\n remote.c          | 23 +++++++++++++++++++++++\n t/t5505-remote.sh |  2 +-\n 2 files changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex e71f66d..39a9405 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1912,11 +1912,34 @@ static int get_stale_heads_cb(const char *refname,\n \treturn 0;\n }\n \n+static int check_overlapping_remotes(struct remote *first, void *priv) {\n+\tstruct remote *second = priv;\n+\tint i, j;\n+\tif(!second)\n+\t\treturn for_each_remote(check_overlapping_remotes, first);\n+\tif(first == second)\n+\t\treturn 0;\n+\tfor (i = 0; i < first->fetch_refspec_nr; i++) {\n+\t\tfor (j = 0; j < second->fetch_refspec_nr; j++) {\n+\t\t\tif(strcmp(first->fetch[i].dst, second->fetch[j].dst) &&\n+\t\t\t   (!fnmatch(first->fetch[i].dst, second->fetch[j].dst, 0) ||\n+\t\t\t    !fnmatch(second->fetch[j].dst, first->fetch[i].dst, 0))) {\n+\t\t\t\twarning(_(\"Overlapping refspecs detected: '%s' and '%s', not pruning.\"),\n+\t\t\t\t\tfirst->fetch[i].dst, second->fetch[j].dst);\n+\t\t\t\treturn 1;\n+\t\t\t}\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n struct ref *get_stale_heads(struct refspec *refs, int ref_count, struct ref *fetch_map)\n {\n \tstruct ref *ref, *stale_refs = NULL;\n \tstruct string_list ref_names = STRING_LIST_INIT_NODUP;\n \tstruct stale_heads_info info;\n+\tif(for_each_remote(check_overlapping_remotes, NULL))\n+\t\treturn NULL;\n \tinfo.ref_names = &ref_names;\n \tinfo.stale_refs_tail = &stale_refs;\n \tinfo.refs = refs;\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 439e996..d4ac6ce 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -428,7 +428,7 @@ test_expect_success 'add alt && prune' '\n \t git rev-parse --verify refs/remotes/origin/side2)\n '\n \n-test_expect_failure 'prune and overlapping refspecs' '\n+test_expect_success 'prune and overlapping refspecs' '\n \t(git clone --mirror one prunetst &&\n \t cd prunetst &&\n \t git remote add two ../two &&\n-- \n1.8.3.1-619-gbec0aa7\n"},{"id":"221625","messageId":"7v8v23mhjy.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371809051-29988-2-git-send-email-dennis@kaarsemaker.net","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T18:42:41Z","receivedAt":"2013-06-21T18:42:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> When cloning a repo with --mirror, and adding more remotes later,\n\nEven though --mirror is not the only way to trigger this, I think it\nis good to mention it because it is the most common way to do so.\n\n> get_stale_heads for origin will mark all refs from other repos as stale.\n\nAs Peff already said, that is only one symptom.  Another is \"git\nfetch\" from origin and then \"git fetch another\" can try to overwrite\nthe same ref in refs/remotes/* hierarchy.  We should say what we are\navoiding upfront, not just one of the consequences of the bad thing\nwe are avoiding.\n\n\t... more remotes later, we would end up with fetch refspecs\n\tfrom different remotes trying to overwrite the same ref for\n\ttheir remote tracking purposes.\n\n        This can cause confusion (e.g. where did\n\t\"refs/remotes/peff/master\" come from?  Is it a copy of the\n\tmaster branch of remote 'peff', or does our mirror origin\n\thave a remote tracking branch for a remote it calls 'peff',\n\twhich may not be related to our 'peff', and we just got a\n\tstraight copy from it?) and can cause \"remote prune\" to\n\tmisbehave.\n\nor something like that.\n\n> Add a warning to the documentation, and warn the user when we detect\n> such things during git remote add.\n>\n> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>\n> ---\n>  Documentation/git-remote.txt |  6 +++++-\n>  builtin/remote.c             | 17 +++++++++++++++++\n>  2 files changed, 22 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt\n> index 581bb4c..d7457df 100644\n> --- a/Documentation/git-remote.txt\n> +++ b/Documentation/git-remote.txt\n> @@ -71,7 +71,11 @@ When a fetch mirror is created with `--mirror=fetch`, the refs will not\n>  be stored in the 'refs/remotes/' namespace, but rather everything in\n>  'refs/' on the remote will be directly mirrored into 'refs/' in the\n>  local repository. This option only makes sense in bare repositories,\n> -because a fetch would overwrite any local commits.\n> +because a fetch would overwrite any local commits. If you want to add extra\n> +remotes to a repository created with `--mirror=fetch`, you will need to change\n> +the configuration of the mirrored remote to fetch only `refs/heads/*`,\n> +otherwise `git remote prune <remote>` will remove all branches for the extra\n> +remotes.\n>  +\n>  When a push mirror is created with `--mirror=push`, then `git push`\n>  will always behave as if `--mirror` was passed.\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 5e54d36..a4f9cb8 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -112,6 +112,21 @@ enum {\n>  #define MIRROR_PUSH 2\n>  #define MIRROR_BOTH (MIRROR_FETCH|MIRROR_PUSH)\n>  \n> +static int check_overlapping_refspec(struct remote *remote, void *priv)\n\nIt is customery to call that cb_data (i.e. data for the callback) in\nour codebase.\n\n> +{\n> +\tchar *refspec = priv;\n\nYou are passing only the RHS of the refspec, so this variable name\nis confusing to the reader.  Perhaps \"dst\", as you will be comparing\nwith \".dst\" which is RHS of the fetch refspec for the remotes?\n\n> +\tint i;\n> +\tfor (i = 0; i < remote->fetch_refspec_nr; i++) {\n> +\t\tif(strcmp(refspec, remote->fetch[i].dst) &&\n\nStyle (have SP between if and open paren).\n\n> +\t\t   (!fnmatch(refspec, remote->fetch[i].dst, 0) ||\n> +\t\t    !fnmatch(remote->fetch[i].dst, refspec, 0))) {\n\nDoes .dst always exist and is never a NULL?  My quick scan of\nremote.c::parse_refspec_internal() tells me otherwise.\n\nAlso what are you matching with what?  refs/* against refs/remotes/origin/*?\n\nWhat if remote->fetch[i] is not a wildcarded refspec, e.g.\n\n\t[remote \"origin\"]\n        \tfetch = +refs/heads/master:refs/heads/origin\n\t\tfetch = +refs/heads/next:refs/remotes/origin/next\n\nYou would want to check for equality in such a case against the RHS\nof the refspeec you have.\n\n> +\t\t\twarning(_(\"Overlapping refspecs detected: '%s' and '%s'\"),\n> +\t\t\t        refspec, remote->fetch[i].dst);\n\nWe know which remote is the offending one, so we can afford to be\nmore helpful to the user and say which existing remote's fetch\nrefspec overlaps with the one the user is attempting to add.\n\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n> +\n>  static int add_branch(const char *key, const char *branchname,\n>  \t\tconst char *remotename, int mirror, struct strbuf *tmp)\n>  {\n> @@ -123,6 +138,8 @@ static int add_branch(const char *key, const char *branchname,\n>  \telse\n>  \t\tstrbuf_addf(tmp, \"refs/heads/%s:refs/remotes/%s/%s\",\n>  \t\t\t\tbranchname, remotename, branchname);\n> +\n> +\tfor_each_remote(check_overlapping_refspec, strchr((const char*)tmp->buf, ':') + 1);\n\nDo you need this cast???\n\n>  \treturn git_config_set_multivar(key, tmp->buf, \"^$\", 0);\n>  }\n"},{"id":"221633","messageId":"7v4ncrmh22.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371809051-29988-4-git-send-email-dennis@kaarsemaker.net","subject":"Re: [PATCH 3/3] remote: don't prune when detecting overlapping refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T18:53:25Z","receivedAt":"2013-06-21T18:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> +static int check_overlapping_remotes(struct remote *first, void *priv) {\n> +\tstruct remote *second = priv;\n> +\tint i, j;\n> +\tif(!second)\n> +\t\treturn for_each_remote(check_overlapping_remotes, first);\n> +\tif(first == second)\n> +\t\treturn 0;\n> +\tfor (i = 0; i < first->fetch_refspec_nr; i++) {\n> +\t\tfor (j = 0; j < second->fetch_refspec_nr; j++) {\n> +\t\t\tif(strcmp(first->fetch[i].dst, second->fetch[j].dst) &&\n> +\t\t\t   (!fnmatch(first->fetch[i].dst, second->fetch[j].dst, 0) ||\n> +\t\t\t    !fnmatch(second->fetch[j].dst, first->fetch[i].dst, 0))) {\n> +\t\t\t\twarning(_(\"Overlapping refspecs detected: '%s' and '%s', not pruning.\"),\n> +\t\t\t\t\tfirst->fetch[i].dst, second->fetch[j].dst);\n> +\t\t\t\treturn 1;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n\nThis codepath essentially needs the same logic as 1/3, no?  Instead\nof open code the inner loop here, can't you call the \"check RHS of a\nsingle fetch refspec for overlap with refspecs from a remote\" helper\nyou introduced in 1/3?\n\nThe logic in the inner loop shares the same issue as the code in\n1/3; it needs to be extended to cover non-wildcard respecs and\nnon-storing refspecs.\n"},{"id":"221718","messageId":"1371994516.24315.8.camel@localhost","threadId":"34233","inReplyTo":"7v8v23mhjy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-23T13:35:16Z","receivedAt":"2013-06-23T13:35:16Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On vr, 2013-06-21 at 11:42 -0700, Junio C Hamano wrote:\n> > +                (!fnmatch(refspec, remote->fetch[i].dst, 0) ||\n> > +                 !fnmatch(remote->fetch[i].dst, refspec, 0))) {\n> \n> Does .dst always exist and is never a NULL?  My quick scan of\n> remote.c::parse_refspec_internal() tells me otherwise.\n> \n> Also what are you matching with what?  refs/* against\n> refs/remotes/origin/*?\n> \n> What if remote->fetch[i] is not a wildcarded refspec, e.g.\n> \n>         [remote \"origin\"]\n>                 fetch = +refs/heads/master:refs/heads/origin\n>                 fetch = +refs/heads/next:refs/remotes/origin/next\n> \n> You would want to check for equality in such a case against the RHS\n> of the refspeec you have.\n\nThanks for all the feedback, I've incorporated it all in a reroll that's\nin progress, except for the above.\n\nI've added a prefix check, so refs/foo and refs/foo/bar will be\nconsidered clashes, but not yet an equality check. Equality for\nwildcards is allowed and tested for, so do we really want to 'outlaw'\nequality of non-wildcard refspecs?\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"221742","messageId":"7vvc54ed53.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1371994516.24315.8.camel@localhost","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-23T21:22:00Z","receivedAt":"2013-06-23T21:22:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> Equality for\n> wildcards is allowed and tested for, so do we really want to 'outlaw'\n> equality of non-wildcard refspecs?\n\nI am not sure what you mean by \"equality for wildcards is allowed\".\nDo you mean this pair of remote definition is sane and not warned?\n\n\t[remote \"one\"]\n        \tfetch = refs/heads/*:refs/remotes/mixed/*\n\n\t[remote \"two\"]\n        \tfetch = refs/heads/*:refs/remotes/mixed/*\n\nFor non-wildcard ones, I think these pairs are both suspects for\npossible clashes and want to be warned.\n\n(1) literal-vs-literal\n\n\t[remote \"one\"]\n        \tfetch = refs/heads/master:refs/heads/origin\n\n\t[remote \"two\"]\n        \tfetch = refs/heads/master:refs/heads/origin\n\n(2) literal-vs-wildcard\n\n\t[remote \"one\"]\n        \tfetch = refs/heads/*:refs/remotes/origin/*\n\n\t[remote \"two\"]\n        \tfetch = refs/heads/master:refs/remotes/origin/master\n"},{"id":"221743","messageId":"1372023811.24315.15.camel@localhost","threadId":"34233","inReplyTo":"7vvc54ed53.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-23T21:43:31Z","receivedAt":"2013-06-23T21:43:31Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > Equality for\n> > wildcards is allowed and tested for, so do we really want to 'outlaw'\n> > equality of non-wildcard refspecs?\n> \n> I am not sure what you mean by \"equality for wildcards is allowed\".\n> Do you mean this pair of remote definition is sane and not warned?\n> \n> \t[remote \"one\"]\n>         \tfetch = refs/heads/*:refs/remotes/mixed/*\n> \n> \t[remote \"two\"]\n>         \tfetch = refs/heads/*:refs/remotes/mixed/*\n\nI personally don't consider them very sane and didn't originally support\nthat. But this behavior is tested for in t5505-remote.sh test 27, which\nstarted failing until I stopped warning for equal refspecs. This support\nfor \"alt remotes\" in prune was added by c175a7ad in 2008. The commit\nmessage for that commit give a plausible reason for using them.\n\n> For non-wildcard ones, I think these pairs are both suspects for\n> possible clashes and want to be warned.\n> \n> (1) literal-vs-literal\n> \n> \t[remote \"one\"]\n>         \tfetch = refs/heads/master:refs/heads/origin\n> \n> \t[remote \"two\"]\n>         \tfetch = refs/heads/master:refs/heads/origin\n\nI agree, but c175a7ad would disagree.\n\n> (2) literal-vs-wildcard\n> \n> \t[remote \"one\"]\n>         \tfetch = refs/heads/*:refs/remotes/origin/*\n> \n> \t[remote \"two\"]\n>         \tfetch = refs/heads/master:refs/remotes/origin/master\n> \n\nAgreed and was already covered in v1.\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"221745","messageId":"7vmwqge9u9.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1372023811.24315.15.camel@localhost","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-23T22:33:18Z","receivedAt":"2013-06-23T22:33:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote:\n>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n>> \n>> > Equality for\n>> > wildcards is allowed and tested for, so do we really want to 'outlaw'\n>> > equality of non-wildcard refspecs?\n>> \n>> I am not sure what you mean by \"equality for wildcards is allowed\".\n>> Do you mean this pair of remote definition is sane and not warned?\n>> \n>> \t[remote \"one\"]\n>>         \tfetch = refs/heads/*:refs/remotes/mixed/*\n>> \n>> \t[remote \"two\"]\n>>         \tfetch = refs/heads/*:refs/remotes/mixed/*\n>\n> I personally don't consider them very sane and didn't originally support\n> that. But this behavior is tested for in t5505-remote.sh test 27, which\n> started failing until I stopped warning for equal refspecs. This support\n> for \"alt remotes\" in prune was added by c175a7ad in 2008. The commit\n> message for that commit give a plausible reason for using them.\n\nI actually do not read it that way.  What it wanted to do primarily\nwas to avoid pruning \"refs/remotes/alt/*\" based on what it observed\nat the remote named \"alt\", when the refs fetched from that remote is\nset to update \"refs/remotes/origin/*\".\n\nThe example in the log message is a special case where two\nphysically different remotes are actually copies of a single logical\nrepository, so in that narrow use case, it may be OK, but it is an\nunusual thing to do and we should \"warn\" about it, I think.\n\nIn any case, I've been assuming in this discussion \"allow\" is to\nsilently accept, and overlaps are \"warned\" but not \"rejected\".  So\nif you meant by 'outlaw' to die and refuse to run, that is not what\nI meant.\n"},{"id":"222039","messageId":"1372281006.3602.2.camel@localhost","threadId":"34233","inReplyTo":"7vmwqge9u9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Dennis Kaarsemaker","fromEmail":"dennis@kaarsemaker.net","sentAt":"2013-06-26T21:10:06Z","receivedAt":"2013-06-26T21:10:06Z","isPatch":true,"sender":{"key":"dennis@kaarsemaker.net","avatar":"https://avatars.githubusercontent.com/u/200649?v=4"},"body":"On zo, 2013-06-23 at 15:33 -0700, Junio C Hamano wrote:\n> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> \n> > On zo, 2013-06-23 at 14:22 -0700, Junio C Hamano wrote:\n> >> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n> >> \n> >> > Equality for\n> >> > wildcards is allowed and tested for, so do we really want to 'outlaw'\n> >> > equality of non-wildcard refspecs?\n> >> \n> >> I am not sure what you mean by \"equality for wildcards is allowed\".\n> >> Do you mean this pair of remote definition is sane and not warned?\n> >> \n> >> \t[remote \"one\"]\n> >>         \tfetch = refs/heads/*:refs/remotes/mixed/*\n> >> \n> >> \t[remote \"two\"]\n> >>         \tfetch = refs/heads/*:refs/remotes/mixed/*\n> >\n> > I personally don't consider them very sane and didn't originally support\n> > that. But this behavior is tested for in t5505-remote.sh test 27, which\n> > started failing until I stopped warning for equal refspecs. This support\n> > for \"alt remotes\" in prune was added by c175a7ad in 2008. The commit\n> > message for that commit give a plausible reason for using them.\n> \n> I actually do not read it that way.  What it wanted to do primarily\n> was to avoid pruning \"refs/remotes/alt/*\" based on what it observed\n> at the remote named \"alt\", when the refs fetched from that remote is\n> set to update \"refs/remotes/origin/*\".\n>\n> The example in the log message is a special case where two\n> physically different remotes are actually copies of a single logical\n> repository, so in that narrow use case, it may be OK, but it is an\n> unusual thing to do and we should \"warn\" about it, I think.\n\nApart from the exactly matching refspecs, does git in any other way\ntreat this as a special case?\n\n> In any case, I've been assuming in this discussion \"allow\" is to\n> silently accept, and overlaps are \"warned\" but not \"rejected\".  So\n> if you meant by 'outlaw' to die and refuse to run, that is not what\n> I meant.\n\nWell, 1/3 is a warning on add, 3/3 is a warning and refusing to prune.\nShould 3/3 do something else instead? Perhaps prompt for confirmation or\nrequire some sort of --force?\n\n-- \nDennis Kaarsemaker\nwww.kaarsemaker.net\n"},{"id":"222050","messageId":"7vhagkv3pv.fsf@alter.siamese.dyndns.org","threadId":"34233","inReplyTo":"1372281006.3602.2.camel@localhost","subject":"Re: [PATCH 1/3] remote: Add warnings about mixin --mirror and other remotes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-26T23:42:36Z","receivedAt":"2013-06-26T23:42:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:\n\n> Apart from the exactly matching refspecs, does git in any other way\n> treat this as a special case?\n\nSorry, I do not quite understand, especially the \"exactly matching\"\npart, which as far as I know is not special (in other words, I am\nnot sure what kind of \"special casing\" you are discussing).\n\n>> In any case, I've been assuming in this discussion \"allow\" is to\n>> silently accept, and overlaps are \"warned\" but not \"rejected\".  So\n>> if you meant by 'outlaw' to die and refuse to run, that is not what\n>> I meant.\n>\n> Well, 1/3 is a warning on add, 3/3 is a warning and refusing to prune.\n\nBy \"refusing to prune\" do you mean \"error out with die()\"?\n\nI was trying to make sure your \"outlaw\" was not something else\n(e.g. \"die without not pruning anything that are not even\noverlapping\"), which is probably too strong.\n\nI think what your patch did was \"keep them around not pruned because\nwe do not know where they came from\" (I just checked $gmane/228589\nagain); that is in line with \"warned but not rejected\", so I think\nwe are OK.\n"}]}