{"thread":{"id":"27469","subject":"[PATCH] remote: allow \"-t\" with fetch mirrors","startedAt":"2011-05-26T15:11:00Z","lastAt":"2011-05-26T17:31:06Z","messageCount":5,"participants":["Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"168773","messageId":"20110526151100.GA2881@sigill.intra.peff.net","threadId":"27469","inReplyTo":null,"subject":"[PATCH] remote: allow \"-t\" with fetch mirrors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T15:11:00Z","receivedAt":"2011-05-26T15:11:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 13fc2c1 (remote: disallow some nonsensical option\ncombinations, 2011-03-30) made it impossible to use \"remote\nadd -t foo --mirror\". The argument was that specifying\nspecific branches is useless because:\n\n  1. Push mirrors do not want a refspec at all.\n\n  2. The point of fetch mirroring is to use a broad refspec\n     like \"refs/*\", but using \"-t\" overrides that.\n\nPoint (1) is valid; \"-t\" with push mirrors is useless. But\npoint (2) ignored another side effect of using --mirror: it\nfetches the refs directly into the refs/ namespace as they\nare found upstream, instead of placing them in a\nseparate-remote layout.\n\nSo 13fc2c1 was overly constrictive, and disallowed\nreasonable specific-branch mirroring, like:\n\n  git remote add -t heads/foo -t heads/bar --mirror=fetch\n\nwhich makes the local \"foo\" and \"bar\" branches direct\nmirrors of the remote, but does not fetch anything else.\n\nThis patch restores the original behavior, but only for\nfetch mirrors.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is on top of the jk/maint-remote-mirror-safer branch which made it\ninto 1.7.5 (the tip of which was 0990248).\n\nIf I were designing \"git remote\" from scratch, I would probably have\n\"-t\" specify branches in refs/heads, even in mirror mode, for\nconsistency with non-mirror mode. But making them relative to refs/ in\nmirror mode is how it has always worked, and I look at this patch not as\nadding a new feature but as unbreaking a feature that worked before.\nSo:\n\n  git remote add -t heads/foo -t heads/bar --mirror\n\nwill continue to work as it did before the original series (though it\nwill nag that you should be using --mirror=fetch).\n\n builtin/remote.c  |    4 ++--\n t/t5505-remote.sh |   22 ++++++++++++++++++++++\n 2 files changed, 24 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex eb1229d..c2eaa2d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -193,8 +193,8 @@ static int add(int argc, const char **argv)\n \n \tif (mirror && master)\n \t\tdie(\"specifying a master branch makes no sense with --mirror\");\n-\tif (mirror && track.nr)\n-\t\tdie(\"specifying branches to track makes no sense with --mirror\");\n+\tif (mirror && !(mirror & MIRROR_FETCH) && track.nr)\n+\t\tdie(\"specifying branches to track makes no sense with non-fetch mirrors\");\n \n \tname = argv[0];\n \turl = argv[1];\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 4e69c90..0d0222e 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -347,6 +347,21 @@ test_expect_success 'fetch mirrors do not act as mirrors during push' '\n \t)\n '\n \n+test_expect_success 'add fetch mirror with specific branches' '\n+\tgit init --bare mirror-fetch/track &&\n+\t(cd mirror-fetch/track &&\n+\t git remote add --mirror=fetch -t heads/new parent ../parent\n+\t)\n+'\n+\n+test_expect_success 'fetch mirror respects specific branches' '\n+\t(cd mirror-fetch/track &&\n+\t git fetch parent &&\n+\t git rev-parse --verify refs/heads/new &&\n+\t test_must_fail git rev-parse --verify refs/heads/renamed\n+\t)\n+'\n+\n test_expect_success 'add --mirror=push' '\n \tmkdir mirror-push &&\n \tgit init --bare mirror-push/public &&\n@@ -382,6 +397,13 @@ test_expect_success 'push mirrors do not act as mirrors during fetch' '\n \t)\n '\n \n+test_expect_success 'push mirrors do not allow you to specify refs' '\n+\tgit init mirror-push/track &&\n+\t(cd mirror-push/track &&\n+\t test_must_fail git remote add --mirror=push -t new public ../public\n+\t)\n+'\n+\n test_expect_success 'add alt && prune' '\n \t(mkdir alttst &&\n \t cd alttst &&\n-- \n1.7.4.5.13.gd3ff5\n"},{"id":"168776","messageId":"4DDE7116.4070900@viscovery.net","threadId":"27469","inReplyTo":"20110526151100.GA2881@sigill.intra.peff.net","subject":"Re: [PATCH] remote: allow \"-t\" with fetch mirrors","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-05-26T15:26:14Z","receivedAt":"2011-05-26T15:26:14Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 5/26/2011 17:11, schrieb Jeff King:\n> +\tif (mirror && !(mirror & MIRROR_FETCH) && track.nr)\n> +\t\tdie(\"specifying branches to track makes no sense with non-fetch mirrors\");\n\nDon't proliferate no double negation. How about:\n\n+\t\tdie(\"specifying branches to track makes sense only with fetch mirrors\");\n\n?\n\n-- Hannes\n"},{"id":"168777","messageId":"20110526152855.GA4120@sigill.intra.peff.net","threadId":"27469","inReplyTo":"4DDE7116.4070900@viscovery.net","subject":"Re: [PATCH] remote: allow \"-t\" with fetch mirrors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T15:28:56Z","receivedAt":"2011-05-26T15:28:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 05:26:14PM +0200, Johannes Sixt wrote:\n\n> Am 5/26/2011 17:11, schrieb Jeff King:\n> > +\tif (mirror && !(mirror & MIRROR_FETCH) && track.nr)\n> > +\t\tdie(\"specifying branches to track makes no sense with non-fetch mirrors\");\n> \n> Don't proliferate no double negation. How about:\n> \n> +\t\tdie(\"specifying branches to track makes sense only with fetch mirrors\");\n\nI can't fail to not disagree with your logic.\n\nJunio, can you squash it in?\n\n-Peff\n"},{"id":"168803","messageId":"7vk4ddxucf.fsf@alter.siamese.dyndns.org","threadId":"27469","inReplyTo":"20110526151100.GA2881@sigill.intra.peff.net","subject":"Re: [PATCH] remote: allow \"-t\" with fetch mirrors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-26T17:08:48Z","receivedAt":"2011-05-26T17:08:48Z","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> Commit 13fc2c1 (remote: disallow some nonsensical option\n> combinations, 2011-03-30) made it impossible to use \"remote\n> add -t foo --mirror\". The argument was that specifying\n> specific branches is useless because:\n>\n>   1. Push mirrors do not want a refspec at all.\n>\n>   2. The point of fetch mirroring is to use a broad refspec\n>      like \"refs/*\", but using \"-t\" overrides that.\n>\n> Point (1) is valid; \"-t\" with push mirrors is useless. But\n> point (2) ignored another side effect of using --mirror: it\n> fetches the refs directly into the refs/ namespace as they\n> are found upstream, instead of placing them in a\n> separate-remote layout.\n\nHmmm, I still fail to see the point of ignoring \"mirror\" aspect and\nconstricting that with -t.\n\n> So 13fc2c1 was overly constrictive, and disallowed\n> reasonable specific-branch mirroring, like:\n>\n>   git remote add -t heads/foo -t heads/bar --mirror=fetch\n\nI mildly disagree that it is \"reasonable\".  I would understand if it were\nsomething like this:\n\n  git remote add -t heads/foo:heads/foo -t heads/bar:heads/bar\n\nI am not saying selective copying is bad or useless.  It would be useful\nin some situations.  I am saying that is not a mirror, and reusing the\nsame --mirror option for a different meaning may introduce confusion.\n"},{"id":"168805","messageId":"20110526173106.GA2608@sigill.intra.peff.net","threadId":"27469","inReplyTo":"7vk4ddxucf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote: allow \"-t\" with fetch mirrors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-26T17:31:06Z","receivedAt":"2011-05-26T17:31:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2011 at 10:08:48AM -0700, Junio C Hamano wrote:\n\n> > So 13fc2c1 was overly constrictive, and disallowed\n> > reasonable specific-branch mirroring, like:\n> >\n> >   git remote add -t heads/foo -t heads/bar --mirror=fetch\n> \n> I mildly disagree that it is \"reasonable\".  I would understand if it were\n> something like this:\n> \n>   git remote add -t heads/foo:heads/foo -t heads/bar:heads/bar\n> \n> I am not saying selective copying is bad or useless.  It would be useful\n> in some situations.  I am saying that is not a mirror, and reusing the\n> same --mirror option for a different meaning may introduce confusion.\n\nI think of it as \"you are mirroring these specific refs to your\nrepository\" instead of \"you are fetching these specific refs into remote\ntracking branches\". But it is somewhat a matter of semantics.\n\nI agree that allowing generalized refspecs via \"-t\" is a nicer solution\nin the long run.  I am mostly trying to fix a regression in 13fc2c1\nhere; I guess the question is whether it is one worth fixing, or if\npeople who want to do this in the meantime should just use \"git config\".\n\n-Peff\n"}]}