{"thread":{"id":"20696","subject":"git fetch --depth=* broken?","startedAt":"2009-08-22T05:52:01Z","lastAt":"2009-08-28T19:07:21Z","messageCount":22,"participants":["Nicolas Pitre","Junio C Hamano","Johan Herland","Daniel Barkalow","Julian Phillips","Shawn O. Pearce","Sverre Rabbelier","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"121503","messageId":"alpine.LFD.2.00.0908220106470.6044@xanadu.home","threadId":"20696","inReplyTo":null,"subject":"git fetch --depth=* broken?","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-22T05:52:01Z","receivedAt":"2009-08-22T05:52:01Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"\ntry out:\n\n\tgit clone --depth=1 git://git.kernel.org/pub/scm/git/git\n\tcd git\n\tgit fetch --depth=2\n\nIt then silently fails, except for the return code of 1.\n\nWith -v this is the same result.  Only if I remove --depth= do I get:\n\n>From git://git.kernel.org/pub/scm/git/git\n = [up to date]      html       -> origin/html\n = [up to date]      maint      -> origin/maint\n = [up to date]      man        -> origin/man\n = [up to date]      master     -> origin/master\n = [up to date]      next       -> origin/next\n = [up to date]      pu         -> origin/pu\n = [up to date]      todo       -> origin/todo\n\nand a return code of 0.\n\nIt seems that commit c6bc400585 is partly responsible for that \nmisbehavior.  At least reverting it makes the status list appear again \neven with the presence of --depth=.\n\nBut still, actual result isn't any better.  Using --depth=2 or \n--depth=1000 doesn't change anything, unless there is _also_ a ref that \nwas updated on the remote end.  Looks like the code is happy to conclude \nthat there is nothing to do if local refs match remote refs and never go \nto talk further to the remote end (\"no \"shallow ...\" nor \"deepen ...\" \nare sent over the wire) despite the fact that --depth=1000 would \ncertainly have to trigger a pack transfer.\n\nI'm also surprised that such thing as simple deepening of a repo is not \nin the test suite.  We certainly document this operation in the \ngit-fetch man page though.\n\nThe code in builtin-fetch-pack.c still looks rather confusing to me, so \nhopefully you are more familiar with it and can provide a fix.\n\n\nNicolas\n"},{"id":"121607","messageId":"alpine.LFD.2.00.0908232320410.6044@xanadu.home","threadId":"20696","inReplyTo":"alpine.LFD.2.00.0908220106470.6044@xanadu.home","subject":"[PATCH] fix simple deepening of a repo","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-24T04:04:09Z","receivedAt":"2009-08-24T04:04:09Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"If all refs sent by the remote repo during a fetch are reachable \nlocally, then no further conversation is performed with the remote. This \ncheck is skipped when the --depth argument is provided to allow the \ndeepening of a shallow clone which corresponding remote repo has no \nchanged.\n\nHowever, some additional filtering was added in commit c29727d5 to \nremove those refs which are equal on both sides.  If the remote repo has \nnot changed, then the list of refs to give the remote process becomes \nempty and simply attempting to deepen a shallow repo always fails.\n\nLet's stop being smart in that case and simply send the whole list over\nwhen that condition is met.  The remote will do the right thing anyways.\n\nTest cases for this issue are also provided.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n\nOn Sat, 22 Aug 2009, Nicolas Pitre wrote:\n\n> try out:\n> \n> \tgit clone --depth=1 git://git.kernel.org/pub/scm/git/git\n> \tcd git\n> \tgit fetch --depth=2\n> \n> It then silently fails, except for the return code of 1.\n\nHere's the fix.  Problem wasn't in builtin-fetch-pack.c after all.\n\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex a8c2ca2..18376d6 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -139,6 +139,36 @@ test_expect_success 'fsck in shallow repo' '\n \t)\n '\n \n+test_expect_success 'simple fetch in shallow repo' '\n+\t(\n+\t\tcd shallow &&\n+\t\tgit fetch\n+\t)\n+'\n+\n+test_expect_success 'no changes expected' '\n+\t(\n+\t\tcd shallow &&\n+\t\tgit count-objects -v\n+\t) > count.shallow.2 &&\n+\tcmp count.shallow count.shallow.2\n+'\n+\n+test_expect_success 'fetch same depth in shallow repo' '\n+\t(\n+\t\tcd shallow &&\n+\t\tgit fetch --depth=2\n+\t)\n+'\n+\n+test_expect_success 'no changes expected' '\n+\t(\n+\t\tcd shallow &&\n+\t\tgit count-objects -v\n+\t) > count.shallow.3 &&\n+\tcmp count.shallow count.shallow.3\n+'\n+\n test_expect_success 'add two more' '\n \tadd B66 $B65 &&\n \tadd B67 $B66\n@@ -201,4 +231,21 @@ test_expect_success 'pull in shallow repo with missing merge base' '\n \t)\n '\n \n+test_expect_success 'additional simple shallow deepenings' '\n+\t(\n+\t\tcd shallow &&\n+\t\tgit fetch --depth=8 &&\n+\t\tgit fetch --depth=10 &&\n+\t\tgit fetch --depth=11\n+\t)\n+'\n+\n+test_expect_success 'clone shallow object count' '\n+\t(\n+\t\tcd shallow &&\n+\t\tgit count-objects -v\n+\t) > count.shallow &&\n+\tgrep \"^count: 52\" count.shallow\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex a8df319..ce91387 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -925,11 +925,12 @@ const struct ref *transport_get_remote_refs(struct transport *transport)\n int transport_fetch_refs(struct transport *transport, const struct ref *refs)\n {\n \tint rc;\n-\tint nr_heads = 0, nr_alloc = 0;\n+\tint nr_heads = 0, nr_alloc = 0, nr_refs = 0;\n \tconst struct ref **heads = NULL;\n \tconst struct ref *rm;\n \n \tfor (rm = refs; rm; rm = rm->next) {\n+\t\tnr_refs++;\n \t\tif (rm->peer_ref &&\n \t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n \t\t\tcontinue;\n@@ -937,6 +938,19 @@ int transport_fetch_refs(struct transport *transport, const struct ref *refs)\n \t\theads[nr_heads++] = rm;\n \t}\n \n+\tif (!nr_heads) {\n+\t\t/*\n+\t\t * When deepening of a shallow repository is requested,\n+\t\t * then local and remote refs are likely to still be equal.\n+\t\t * Just feed them all to the fetch method in that case.\n+\t\t * This condition shouldn't be met in a non-deepening fetch\n+\t\t * (see builtin-fetch.c:quickfetch()).\n+\t\t */\n+\t\theads = xmalloc(nr_refs * sizeof(*heads));\n+\t\tfor (rm = refs; rm; rm = rm->next)\n+\t\t\theads[nr_heads++] = rm;\n+\t}\n+\n \trc = transport->fetch(transport, nr_heads, heads);\n \tfree(heads);\n \treturn rc;\n"},{"id":"121610","messageId":"7vocq5q0j7.fsf@alter.siamese.dyndns.org","threadId":"20696","inReplyTo":"alpine.LFD.2.00.0908232320410.6044@xanadu.home","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-24T04:49:16Z","receivedAt":"2009-08-24T04:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> If all refs sent by the remote repo during a fetch are reachable \n> locally, then no further conversation is performed with the remote. This \n> check is skipped when the --depth argument is provided to allow the \n> deepening of a shallow clone which corresponding remote repo has no \n> changed.\n>\n> However, some additional filtering was added in commit c29727d5 to \n> remove those refs which are equal on both sides.  If the remote repo has \n> not changed, then the list of refs to give the remote process becomes \n> empty and simply attempting to deepen a shallow repo always fails.\n>\n> Let's stop being smart in that case and simply send the whole list over\n> when that condition is met.  The remote will do the right thing anyways.\n>\n> Test cases for this issue are also provided.\n>\n> Signed-off-by: Nicolas Pitre <nico@cam.org>\n> ---\n\nThanks.  The fix looks correct (as usual with patches from you).\n\nBut it makes me wonder if this logic to filter refs is buying us anything.\n\n>  \tfor (rm = refs; rm; rm = rm->next) {\n> +\t\tnr_refs++;\n>  \t\tif (rm->peer_ref &&\n>  \t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n>  \t\t\tcontinue;\n\t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n\t\theads[nr_heads++] = rm;\n\t}\n\nWhat is the point of not asking for the refs that we know are the same?\n\nIn other words, what breaks (not necessarily in the correctness sense, but\nalso in the performance sense) if we get rid of this filtering altogether?\n\nOver a native protocol, we will tell the other end what we have and the\nremote end will notice that we are asking for the same thing, so it won't\ninclude unnecessary objects in the pack anyway.  When calling out to a\nwalker, we will also notice that the ref matches what we have already and\nwill not fetch anything from the remote.  Because the commit at the tip of\nthe ref that is already up-to-date is marked as COMPLETE, we will not even\nlocally have to walk the object chain to make sure things are connected.\n\nI think the only thing that this possibly gains is if _everything_ is up\nto date.  Then we won't need to make a call to transport->fetch() and it\nwould save a new connection.\n\nBut that optimization is already done long ago by the caller's\nquickfetch() in the normal case.\n\nWhich makes me suspect that the real fix should be something a lot\nsimpler, like this (untested) patch.\n\ndiff --git a/transport.c b/transport.c\nindex f231b35..14dab81 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1053,18 +1053,16 @@ const struct ref *transport_get_remote_refs(struct transport *transport)\n int transport_fetch_refs(struct transport *transport, const struct ref *refs)\n {\n \tint rc;\n-\tint nr_heads = 0, nr_alloc = 0;\n+\tint nr_heads = 0;\n \tconst struct ref **heads = NULL;\n \tconst struct ref *rm;\n \n-\tfor (rm = refs; rm; rm = rm->next) {\n-\t\tif (rm->peer_ref &&\n-\t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n-\t\t\tcontinue;\n-\t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n+\tfor (rm = refs; rm; rm = rm->next)\n+\t\tnr_heads++;\n+\theads = xmalloc(nr_heads * sizeof(*heads));\n+\tnr_heads = 0;\n+\tfor (rm = refs; rm; rm = rm->next)\n \t\theads[nr_heads++] = rm;\n-\t}\n-\n \trc = transport->fetch(transport, nr_heads, heads);\n \tfree(heads);\n \treturn rc;\n"},{"id":"121632","messageId":"alpine.LFD.2.00.0908240946390.6044@xanadu.home","threadId":"20696","inReplyTo":"7vocq5q0j7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-24T13:55:13Z","receivedAt":"2009-08-24T13:55:13Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 23 Aug 2009, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > If all refs sent by the remote repo during a fetch are reachable \n> > locally, then no further conversation is performed with the remote. This \n> > check is skipped when the --depth argument is provided to allow the \n> > deepening of a shallow clone which corresponding remote repo has no \n> > changed.\n> >\n> > However, some additional filtering was added in commit c29727d5 to \n> > remove those refs which are equal on both sides.  If the remote repo has \n> > not changed, then the list of refs to give the remote process becomes \n> > empty and simply attempting to deepen a shallow repo always fails.\n> >\n> > Let's stop being smart in that case and simply send the whole list over\n> > when that condition is met.  The remote will do the right thing anyways.\n> >\n> > Test cases for this issue are also provided.\n> >\n> > Signed-off-by: Nicolas Pitre <nico@cam.org>\n> > ---\n> \n> Thanks.  The fix looks correct (as usual with patches from you).\n> \n> But it makes me wonder if this logic to filter refs is buying us anything.\n> \n> >  \tfor (rm = refs; rm; rm = rm->next) {\n> > +\t\tnr_refs++;\n> >  \t\tif (rm->peer_ref &&\n> >  \t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n> >  \t\t\tcontinue;\n> \t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n> \t\theads[nr_heads++] = rm;\n> \t}\n> \n> What is the point of not asking for the refs that we know are the same?\n\nI could see the advantage if the number of refs is really huge.  Wasn't \nsome converted repositories producing a lot of branches and/or tags \nsignificantly slowing down a fetch operation?  Granted that was long ago \nwhen that issue got \"fixed\" so the details are fuzzy to me.\n\n> In other words, what breaks (not necessarily in the correctness sense, but\n> also in the performance sense) if we get rid of this filtering altogether?\n\nIf you really want to get rid of that filtering, I'd still do it in a \nseparate patch.  That way if any issue appears because of that then \nbissection will point directly to that removal alone.\n\n\nNicolas\n"},{"id":"121635","messageId":"200908241620.52838.johan@herland.net","threadId":"20696","inReplyTo":"alpine.LFD.2.00.0908240946390.6044@xanadu.home","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2009-08-24T14:20:52Z","receivedAt":"2009-08-24T14:20:52Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Monday 24 August 2009, Nicolas Pitre wrote:\n> On Sun, 23 Aug 2009, Junio C Hamano wrote:\n> > Nicolas Pitre <nico@cam.org> writes:\n> > > If all refs sent by the remote repo during a fetch are reachable\n> > > locally, then no further conversation is performed with the\n> > > remote. This check is skipped when the --depth argument is\n> > > provided to allow the deepening of a shallow clone which\n> > > corresponding remote repo has no changed.\n> > >\n> > > However, some additional filtering was added in commit c29727d5\n> > > to remove those refs which are equal on both sides.  If the\n> > > remote repo has not changed, then the list of refs to give the\n> > > remote process becomes empty and simply attempting to deepen a\n> > > shallow repo always fails.\n> > >\n> > > Let's stop being smart in that case and simply send the whole\n> > > list over when that condition is met.  The remote will do the\n> > > right thing anyways.\n> > >\n> > > Test cases for this issue are also provided.\n> > >\n> > > Signed-off-by: Nicolas Pitre <nico@cam.org>\n> > > ---\n> >\n> > Thanks.  The fix looks correct (as usual with patches from you).\n> >\n> > But it makes me wonder if this logic to filter refs is buying us\n> > anything.\n> >\n> > >  \tfor (rm = refs; rm; rm = rm->next) {\n> > > +\t\tnr_refs++;\n> > >  \t\tif (rm->peer_ref &&\n> > >  \t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n> > >  \t\t\tcontinue;\n> >\n> > \t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n> > \t\theads[nr_heads++] = rm;\n> > \t}\n> >\n> > What is the point of not asking for the refs that we know are the\n> > same?\n>\n> I could see the advantage if the number of refs is really huge. \n> Wasn't some converted repositories producing a lot of branches and/or\n> tags significantly slowing down a fetch operation?  Granted that was\n> long ago when that issue got \"fixed\" so the details are fuzzy to me.\n\nI'm converting several CVS repos to Git with ~50 000 refs, so I'm happy \nwith any change that can speed things up for repos with many refs.\n\nRight now, my biggest gripe is that a 'git push --mirror' on such a repo \ncan easily take ~10 min. even though the actual pack generation and \ntransfer only takes a couple of seconds. It seems like it needs ~10 \nminutes to generate the list of changed/added/deleted refs...\nUnfortunately I haven't had time to look properly into it, yet...\n\n\n...Johan\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"121647","messageId":"alpine.LNX.2.00.0908240144530.28290@iabervon.org","threadId":"20696","inReplyTo":"7vocq5q0j7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-08-24T16:26:54Z","receivedAt":"2009-08-24T16:26:54Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sun, 23 Aug 2009, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > If all refs sent by the remote repo during a fetch are reachable \n> > locally, then no further conversation is performed with the remote. This \n> > check is skipped when the --depth argument is provided to allow the \n> > deepening of a shallow clone which corresponding remote repo has no \n> > changed.\n> >\n> > However, some additional filtering was added in commit c29727d5 to \n> > remove those refs which are equal on both sides.  If the remote repo has \n> > not changed, then the list of refs to give the remote process becomes \n> > empty and simply attempting to deepen a shallow repo always fails.\n> >\n> > Let's stop being smart in that case and simply send the whole list over\n> > when that condition is met.  The remote will do the right thing anyways.\n> >\n> > Test cases for this issue are also provided.\n> >\n> > Signed-off-by: Nicolas Pitre <nico@cam.org>\n> > ---\n> \n> Thanks.  The fix looks correct (as usual with patches from you).\n> \n> But it makes me wonder if this logic to filter refs is buying us anything.\n> \n> >  \tfor (rm = refs; rm; rm = rm->next) {\n> > +\t\tnr_refs++;\n> >  \t\tif (rm->peer_ref &&\n> >  \t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n> >  \t\t\tcontinue;\n> \t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n> \t\theads[nr_heads++] = rm;\n> \t}\n> \n> What is the point of not asking for the refs that we know are the same?\n\nThis code is part of the original C implementation of fetch; I suspect the \noptimization was somehow in the shell version and made sense there, \nperhaps because there wasn't a quickfetch in the shell version or that \nthere was some non-negligable per-ref cost in the code around there, since \nit was calling helper programs and such.\n\nAnyway, I think it makes sense to remove the filtering from \ntransport_fetch_refs(), like your patch does.\n\nIf it makes a difference for the actual protocol, fetch_refs_via_pack() \ncould filter them at that stage.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"121682","messageId":"7vmy5o3laj.fsf@alter.siamese.dyndns.org","threadId":"20696","inReplyTo":"alpine.LFD.2.00.0908240946390.6044@xanadu.home","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-24T22:21:40Z","receivedAt":"2009-08-24T22:21:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> If you really want to get rid of that filtering, I'd still do it in a \n> separate patch.  That way if any issue appears because of that then \n> bissection will point directly to that removal alone.\n\nFair enough.\n"},{"id":"121683","messageId":"alpine.LNX.2.00.0908242212260.26869@reaper.quantumfyre.co.uk","threadId":"20696","inReplyTo":"alpine.LNX.2.00.0908240144530.28290@iabervon.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Julian Phillips","fromEmail":"julian@quantumfyre.co.uk","sentAt":"2009-08-24T22:30:46Z","receivedAt":"2009-08-24T22:30:46Z","isPatch":true,"sender":{"key":"julian@quantumfyre.co.uk","avatar":"https://avatars.githubusercontent.com/u/948888?v=4"},"body":"On Mon, 24 Aug 2009, Daniel Barkalow wrote:\n\n> On Sun, 23 Aug 2009, Junio C Hamano wrote:\n>\n>> But it makes me wonder if this logic to filter refs is buying us anything.\n>>\n>>>  \tfor (rm = refs; rm; rm = rm->next) {\n>>> +\t\tnr_refs++;\n>>>  \t\tif (rm->peer_ref &&\n>>>  \t\t    !hashcmp(rm->peer_ref->old_sha1, rm->old_sha1))\n>>>  \t\t\tcontinue;\n>> \t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n>> \t\theads[nr_heads++] = rm;\n>> \t}\n>>\n>> What is the point of not asking for the refs that we know are the same?\n>\n> This code is part of the original C implementation of fetch; I suspect the\n> optimization was somehow in the shell version and made sense there,\n> perhaps because there wasn't a quickfetch in the shell version or that\n> there was some non-negligable per-ref cost in the code around there, since\n> it was calling helper programs and such.\n\nI don't remember copying it from the shell version but my memory is \nterrible, so I could easily be wrong.  The relevant commit message was:\n\n\"git-fetch2: remove ref_maps that are not interesting\n\nOnce we have the full list of ref_maps, remove any where the local\nand remote sha1s are the same - as we don't need to do anything for\nthem.\"\n\nSo that doesn't help.  I was very concerned about performance though \n(which was why I wanted fetch in C in the first place), so may have added \nit to speed up fetches that have only updated a few refs - and I assume \nthat quickfetch was something that came along later after you absorbed the \nwork into the transport series?\n\n> Anyway, I think it makes sense to remove the filtering from\n> transport_fetch_refs(), like your patch does.\n>\n> If it makes a difference for the actual protocol, fetch_refs_via_pack()\n> could filter them at that stage.\n\nI think it would certainly be worth investigating the performance aspects \n... no time tonight, but maybe tomorrow.\n\n-- \nJulian\n\n  ---\nSome circumstantial evidence is very strong, as when you find a trout in\nthe milk.\n \t\t-- Thoreau\n"},{"id":"121688","messageId":"alpine.LFD.2.00.0908242001250.6044@xanadu.home","threadId":"20696","inReplyTo":"alpine.LNX.2.00.0908242212260.26869@reaper.quantumfyre.co.uk","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2009-08-25T00:18:45Z","receivedAt":"2009-08-25T00:18:45Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 24 Aug 2009, Julian Phillips wrote:\n\n> On Mon, 24 Aug 2009, Daniel Barkalow wrote:\n> \n> > On Sun, 23 Aug 2009, Junio C Hamano wrote:\n> > \n> > > What is the point of not asking for the refs that we know are the same?\n> > \n> > This code is part of the original C implementation of fetch; I suspect the\n> > optimization was somehow in the shell version and made sense there,\n> > perhaps because there wasn't a quickfetch in the shell version or that\n> > there was some non-negligable per-ref cost in the code around there, since\n> > it was calling helper programs and such.\n> \n> I don't remember copying it from the shell version but my memory is terrible,\n> so I could easily be wrong.  The relevant commit message was:\n> \n> \"git-fetch2: remove ref_maps that are not interesting\n> \n> Once we have the full list of ref_maps, remove any where the local\n> and remote sha1s are the same - as we don't need to do anything for\n> them.\"\n> \n> So that doesn't help.  I was very concerned about performance though (which\n> was why I wanted fetch in C in the first place), so may have added it to speed\n> up fetches that have only updated a few refs - and I assume that quickfetch\n> was something that came along later after you absorbed the work into the\n> transport series?\n\nWell... Johan Herland says he has to deal with repositories containing \naround 50000 refs.  So in that case it is certainly a good idea not to \nsend the whole 50000 refs back if only one or two (or a hundred) need to \nbe updated.  And quickfetch() won't help in that case since its purpose \nis only to determine if there is anything at all to update.\n\n> > Anyway, I think it makes sense to remove the filtering from\n> > transport_fetch_refs(), like your patch does.\n> > \n> > If it makes a difference for the actual protocol, fetch_refs_via_pack()\n> > could filter them at that stage.\n> \n> I think it would certainly be worth investigating the performance aspects ...\n> no time tonight, but maybe tomorrow.\n\n50000 refs * 45 bytes each = 2.25 MB.  That's all wasted bandwidth if \nonly one ref needs updating.\n\n\nNicolas\n"},{"id":"121694","messageId":"20090825021223.GE1033@spearce.org","threadId":"20696","inReplyTo":"alpine.LFD.2.00.0908242001250.6044@xanadu.home","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-25T02:12:24Z","receivedAt":"2009-08-25T02:12:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> Well... Johan Herland says he has to deal with repositories containing \n> around 50000 refs.  So in that case it is certainly a good idea not to \n> send the whole 50000 refs back if only one or two (or a hundred) need to \n> be updated.  And quickfetch() won't help in that case since its purpose \n> is only to determine if there is anything at all to update.\n...\n> 50000 refs * 45 bytes each = 2.25 MB.  That's all wasted bandwidth if \n> only one ref needs updating.\n\nNot just Johan Herland.  Gerrit Code Review creates a new ref\nfor every patch proposed for review.  Imagine taking every email\nmessage on git ML that has \"[PATCH]\" in the subject, and creating\na new ref for that in a git.git clone.\n\nWe aren't quite at the 50k ref stage yet, but we're starting to\nconsider that some of our repositories have a ton of refs, and\nthat the initial advertisement for either fetch or push is horrid.\n\nSince the refs are immutable I could actually teach the JGit\ndaemon to hide them from JGit's receive-pack, thus cutting down the\nadvertisement on push, but the refs exist so you can literally say:\n\n  git fetch URL refs/changes/88/4488/2\n  git show FETCH_HEAD\n\nto inspect the \"v2\" version of whatever 4488 is, and if 4488 was\nthe last commit in a patch series, you'd also be able to do:\n\n  git log -p --reverse ..FETCH_HEAD\n\nto see the complete series.\n\nGiven how infrequent it is to grab a given change is though, I'm\nstarting to consider either a protocol extension that allows the\nclient to probe for a ref which wasn't in the initial advertisement,\nor take it on a command line flag, e.g.:\n\n  git fetch --uploadpack='git upload-pack --ref refs/changes/88/4488/2' URL refs/changes/88/4488/2\n\nPersonally I'd prefer extending the protocol, because making the\nend user supply information twice is stupid.\n\nI don't know enough about Johan's case though to know whether or\nnot he can get away with hiding the bulk of the refs in the initial\nadvertisement.  In the case of Gerrit Code Review, the bulk of the\nrefs is under refs/changes/, only a handful of things are under the\nrefs/heads/ and ref/tags/ namespace, and most fetches actually are\nfor only refs/heads/ and refs/tags/.  So hiding the refs/changes/\nnamespace would make large improvement in the advertisement cost.\n\n-- \nShawn.\n"},{"id":"121706","messageId":"fabb9a1e0908242200v22057a72ta4998633701c408b@mail.gmail.com","threadId":"20696","inReplyTo":"20090825021223.GE1033@spearce.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-08-25T05:00:39Z","receivedAt":"2009-08-25T05:00:39Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Mon, Aug 24, 2009 at 19:12, Shawn O. Pearce<spearce@spearce.org> wrote:\n>  git fetch --uploadpack='git upload-pack --ref refs/changes/88/4488/2' URL refs/changes/88/4488/2\n>\n> Personally I'd prefer extending the protocol, because making the\n> end user supply information twice is stupid.\n\n$ git fetch --unadvertised URL refs/changes/88/4488/2\n\nAnd we expand that to the above behind the screens?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"121710","messageId":"7vab1osc2m.fsf@alter.siamese.dyndns.org","threadId":"20696","inReplyTo":"20090825021223.GE1033@spearce.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-25T05:21:37Z","receivedAt":"2009-08-25T05:21:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> We aren't quite at the 50k ref stage yet, but we're starting to\n> consider that some of our repositories have a ton of refs, and\n> that the initial advertisement for either fetch or push is horrid.\n>\n> Since the refs are immutable I could actually teach the JGit\n> daemon to hide them from JGit's receive-pack, thus cutting down the\n> advertisement on push, but the refs exist so you can literally say:\n\nWhat do you mean \"refs are immutable\"?\n\nDo you mean \"In the particular application, Gerrit, the server knows that\ncertain refs will never move nor get deleted, once they are created\"?  If\nso, then I would understand, but otherwise what you are describing is not\ngit anymore ;-)\n\nAnd I think it is probably worth thinking things through to find a way to\ntake advantage of that knowledge.\n\nEven though refs under refs/changes/ hierarchy may have that property, the\nclient won't know what's available unless you advertise it in some way.\n\nYou could assume some offline measure outside the git protocol exists for\nclients to find out about them, and protocol extension could say \"I do not\nwant to hear about refs that match these globs during this exchange,\nbecause I have learnt about them offline\", and the server could skip\nadvertisement.\n\n>   git fetch --uploadpack='git upload-pack --ref refs/changes/88/4488/2' URL refs/changes/88/4488/2\n>\n> Personally I'd prefer extending the protocol, because making the\n> end user supply information twice is stupid.\n\nIn the upload-pack protocol, the server talks first, so it is rather hard\nto shoehorn a request from a client to ask \"I know about refs/changes/*\nhiearchy, so don't talk about them\".\n\nI however think it is entirely reasonable to have a server side\nconfiguration that tells upload-pack not to advertise refs/changes/*\nhierarchy but still remembers they are OUR_REF.  In send_ref() in\nupload-pack.c, you'd do something like (I know, I know, you'd be doing\nan equivalent of this in jgit):\n\n\tstatic const char *capabilities = \"multi_ack ...\";\n\tstruct object *o = parse_object(sha1);\n\tint skip_advertisement = exclude_ref_from_advertisement(refname);\n\n\tif (!o)\n\t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n\n\tif (!skip_advertisement) {\n\t\tif (capabilities)\n\t\t\tpacket_write(1, \"%s %s%c%s\\n\", sha1_to_hex(sha1), refname,\n\t\t\t\t0, capabilities);\n\t\telse\n\t\t\tpacket_write(1, \"%s %s\\n\", sha1_to_hex(sha1), refname);\n\t\tcapabilities = NULL;\n\t}\n\n\tif (!(o->flags & OUR_REF)) {\n\t\to->flags |= OUR_REF;\n\t\tnr_our_refs++;\n\t}\n\tif (o->type == OBJ_TAG) {\n\t\to = deref_tag(o, refname, 0);\n\t\tif (o && !skip_advertisement)\n\t\t\tpacket_write(1, \"%s %s^{}\\n\", sha1_to_hex(o->sha1), refname);\n\t}\n\treturn 0;\n\nDoing it this way, receive_needs() will allow refs/changes/88/4488/2 to be\nrequested, because that is what send_ref() saw and marked as OUR_REF.  It\nwas just not sent to the client.  And get_common_commits() will behave the\nsame with or without this abbreviated advertisement,\n\nOf course, the client side cannot grab everything with refs/*:refs/remotes/*\nwildcard refspecs from such a server, but I think that can be considered a\nfeature.\n"},{"id":"121714","messageId":"20090825061248.GG1033@spearce.org","threadId":"20696","inReplyTo":"7vab1osc2m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-25T06:12:48Z","receivedAt":"2009-08-25T06:12:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> > We aren't quite at the 50k ref stage yet, but we're starting to\n> > consider that some of our repositories have a ton of refs, and\n> > that the initial advertisement for either fetch or push is horrid.\n> >\n> > Since the refs are immutable I could actually teach the JGit\n> > daemon to hide them from JGit's receive-pack, thus cutting down the\n> > advertisement on push, but the refs exist so you can literally say:\n> \n> What do you mean \"refs are immutable\"?\n> \n> Do you mean \"In the particular application, Gerrit, the server knows that\n> certain refs will never move nor get deleted, once they are created\"?  If\n> so, then I would understand, but otherwise what you are describing is not\n> git anymore ;-)\n\nThe former.  :-)\n\nI mean that this particular server implementation will deny any\nupdate made to refs/changes/, as if one had the following as the\nupdate hook on that repository:\n\n  #!/bin/sh\n  case \"$1\" in\n  refs/changes/*) exit 1;;\n  *) exit 0;\n  esac\n\nThis of course is completely legal, and since the server knows the\nref cannot be moved, there is no need to advertise it to the client.\nBut this is a very specialized thing, its rare that the thing that\nformats the advertisement knows what the update hook will permit\nto be modified.\n \n> >   git fetch --uploadpack='git upload-pack --ref refs/changes/88/4488/2' URL refs/changes/88/4488/2\n> >\n> > Personally I'd prefer extending the protocol, because making the\n> > end user supply information twice is stupid.\n> \n> In the upload-pack protocol, the server talks first, so it is rather hard\n> to shoehorn a request from a client to ask \"I know about refs/changes/*\n> hiearchy, so don't talk about them\".\n\nActually, that assumption is still a problem.\n\nThe client knows the *name* of the ref, but not the SHA-1 the ref is\ncurrently valued at.  Thus when the client knows it wants a certain\nref by name, it needs to send a \"want \" line to the server that would\ngive it whatever that ref currently points at.  Unfortunately since we\nhave not obtained that value yet, we are stuck.\n\nHowever, we do have one name we want to know about, but the server may\nhave 50k other names in the same namespace we do not know about.\n\nI was thinking instead that we have a new protocol extension:\n\n  S: ... HEAD\\0side-band ... expand-refs\n  S: ... refs/heads/master\n  S: 0000\n\n  C: ... expand refs/changes/88/4488/2\n  C: 0000\n\n  S: ... refs/changes/88/4488/2\n  S: 0000\n\n  C: ... want XXXXXX\\0side-band-64k ...\n \n> Of course, the client side cannot grab everything with refs/*:refs/remotes/*\n> wildcard refspecs from such a server, but I think that can be considered a\n> feature.\n\nIf expand accepted globs like fetch does, then fetch can ask for\nexpand of refs/changes/* if it does not see any refs/changes/*\non advertisement.  Or just expand a particular ref, or handful of\nrefs, that the user has asked for on the fetch line.\n\nThe problem with this is servers which are sending this expand-refs\ntag have hidden certain namespaces from older clients.  Those names\ncan't be seen by older git clients, unless the user does an upgrade.\n\nThis might be OK for Gerrit Code Review's refs/changes/ namespace,\nbut it may not be good for others.\n\n\n-- \nShawn.\n"},{"id":"121715","messageId":"7vy6p8pfm1.fsf@alter.siamese.dyndns.org","threadId":"20696","inReplyTo":"20090825061248.GG1033@spearce.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-25T06:33:26Z","receivedAt":"2009-08-25T06:33:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n> The client knows the *name* of the ref, but not the SHA-1 the ref is\n> currently valued at.  Thus when the client knows it wants a certain\n> ref by name, it needs to send a \"want \" line to the server that would\n> give it whatever that ref currently points at.  Unfortunately since we\n> have not obtained that value yet, we are stuck.\n\nThat could be something you can fix in the out-of-band procedure Gerrit\nuses (you let the client learn both name and value offline, and then the\nclient uses that value on \"want\" line).\n\nHowever, even if we limit the discussion to Gerrit, you would need an\nupdated client that can be called with the out-of-band information\n(i.e. \"we know that changes/88/4488/2 points at X, so use X when\nrequesting\") when talking with such an updated server.\n\nSo I think that expand-refs is a much nicer general solution than just\n\"server side is configured to hide but still allow certain refs\", and\nclient updates cannot be avoided.\n\nAnd again, \n\n> The problem with this is servers which are sending this expand-refs\n> tag have hidden certain namespaces from older clients.  Those names\n> can't be seen by older git clients, unless the user does an upgrade.\n\nI do not think \"generally hidden, but if you need to know you are allowed\nto peek\" is much of a problem.  You do not do that for regular refs, only\nfor \"on-demand-as-needed\" type things.  If we are going to make extensive\nuse of notes on commits to give richer annotations, I suspect notes\nhierarchy could be hidden by default in a similar way.\n"},{"id":"121731","messageId":"20090825151424.GJ1033@spearce.org","threadId":"20696","inReplyTo":"7vy6p8pfm1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-25T15:14:24Z","receivedAt":"2009-08-25T15:14:24Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> > The client knows the *name* of the ref, but not the SHA-1 the ref is\n> > currently valued at.  Thus when the client knows it wants a certain\n> > ref by name, it needs to send a \"want \" line to the server that would\n> > give it whatever that ref currently points at.  Unfortunately since we\n> > have not obtained that value yet, we are stuck.\n> \n> That could be something you can fix in the out-of-band procedure Gerrit\n> uses (you let the client learn both name and value offline, and then the\n> client uses that value on \"want\" line).\n\nWell, we're trying to reduce out-of-band things with Gerrit.\n\nIts bad enough that Gerrit doesn't use git am and git send-email\nto slug around changes for discussion.  As it is we're an island\namong the git world, *despite* the fact that Gerrit speaks the git\nprotocol natively and you can push directly to it, avoiding the\nsend-email SMTP nonsense many folks run into.\n \n> However, even if we limit the discussion to Gerrit, you would need an\n> updated client that can be called with the out-of-band information\n> (i.e. \"we know that changes/88/4488/2 points at X, so use X when\n> requesting\") when talking with such an updated server.\n\nYes, exactly.  Existing clients wouldn't send an arbitrary want\nrequest, even if the server had a whitelist of objects it would\nallow to be requested.\n\nOne reason why Gerrit publishes pending changes with ref names is to\nmake it easier for any user to obtain the proposed change locally.\nIts hard to beat `git fetch URL blah`, that's even easier than\n\"save to mbox, git am mbox\".\n \n> So I think that expand-refs is a much nicer general solution than just\n> \"server side is configured to hide but still allow certain refs\", and\n> client updates cannot be avoided.\n\nYes, I agree.  Given 20/20 hindsight, its the way the protocol\nshould have been implemented:\n\n  C: 0014expand refs/heads/*\n  C: 0013expand refs/tags/*\n  C: 0000\n\n  S: ...refs/heads/master\n  S: ...refs/heads/next\n  S: ...refs/tags/v1.0\n  S: 0000\n\nThis would have permitted clients doing `git pull URL for-linus` to say:\n\n  C: 0011expand for-linus\n  C: 0000\n\n  S: ...refs/heads/for-linus\n  S: ...refs/remotes/k26/for-linus\n  S: 0000\n\nand thus significantly narrow the scope of what they are shown when\nthey connect for a given ref.\n\n> > The problem with this is servers which are sending this expand-refs\n> > tag have hidden certain namespaces from older clients.  Those names\n> > can't be seen by older git clients, unless the user does an upgrade.\n> \n> I do not think \"generally hidden, but if you need to know you are allowed\n> to peek\" is much of a problem.  You do not do that for regular refs, only\n> for \"on-demand-as-needed\" type things.  If we are going to make extensive\n> use of notes on commits to give richer annotations, I suspect notes\n> hierarchy could be hidden by default in a similar way.\n\nAfter sleeping on it, I'm OK with hiding some refs from older clients.\n\nSometimes things evolve, and you should just update your software\nto keep up with them.  If you really want the \"hidden refs\" that\nGerrit advertises, you should install a newer client.\n\nWe could consider supporting a legacy option through upload-pack,\nsuch as:\n\n  git fetch --upload-pack='git-upload-pack --expand refs/changes/' URL\n\nwhich tells the remote side to additionally expand those refs during\nthe initial advertisement.  Then users have an escape hatch if:\n\n* They know the remote is new enough to hide refs;\n* They suspect the remote is hiding refs;\n* They received an out-of-band notification telling this;\n* They have an older client which doesn't support expanding refs;\n* They cannot upgrade said client yet;\n\nI'm thinking about writing an RFC patch for this today for git.git.\nI think the expand refs feature neatly solves a number of problems\nfor me in Gerrit.  But I'm really hoping its not the only set of\nrepositories that would benefit from such a feature, because if so,\nits not worth the headache of the protocol change.\n\n-- \nShawn.\n"},{"id":"121764","messageId":"20090826021057.GL1033@spearce.org","threadId":"20696","inReplyTo":"20090825151424.GJ1033@spearce.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-26T02:10:57Z","receivedAt":"2009-08-26T02:10:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n> Junio C Hamano <gitster@pobox.com> wrote:\n> > So I think that expand-refs is a much nicer general solution than just\n> > \"server side is configured to hide but still allow certain refs\", and\n> > client updates cannot be avoided.\n> \n> Yes, I agree.\n...\n> I'm thinking about writing an RFC patch for this today for git.git.\n\nRFC patch below.  This is only the upload-pack side, and lacks\ntest, etc, so its posted *ONLY* for discussion.  I'll try to flesh\nit out further tomorrow into something that could be considered\nmore seriously.\n\n--8<--\n[RFC] upload-pack: expand capability advertises additional refs\n\nThe expand capability and associated command permits the client\nto ask for information about refs which were not in the initial\nadvertisement sent when the connection was first opened.\n\nIn the below exchange the server initially only advertises its\ncurrent HEAD, refs/heads and refs/tags namespaces.  However,\nthe client has been instructed to fetch anything which matches\nrefs/remotes/jc/*.\n\nSince no matching refs appeared in the initial advertisement,\nthe client requests the server to expand the desired pattern,\nand terminates its expand request list with a flush.\n\nUpon receiving a flush from the client, the server displays any\nlocal refs which match any of the expand patterns requested,\nand then closes this secondary advertisement list with a flush.\nIf no refs matched, the server immediately returns a flush.\n\nIf multiple expand patterns match the same ref, the ref is returned\nonly once in the secondary advertisement, avoid confusing the client\nwith duplicate results.\n\n  S: 008f... HEAD\\0...include-tag expand\n  S: 0043... refs/heads/build-next\n  S: 0040... refs/tags/v1.6.4.1\n  S: 0043... refs/tags/v1.6.4.1^{}\n  S: 0000\n\n  C: 001dexpand refs/remotes/jc/*\n  C: 0000\n\n  S: 0043... refs/remotes/jc/maint\n  S: 0044... refs/remotes/jc/master\n  S: 0000\n\n  C: 0031want ...\n  C: 0000\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\nSigned-off-by: Shawn O. Pearce <sop@google.com>\n---\n upload-pack.c |   97 +++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 91 insertions(+), 6 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4d8be83..e1ec608 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -10,6 +10,8 @@\n #include \"revision.h\"\n #include \"list-objects.h\"\n #include \"run-command.h\"\n+#include \"remote.h\"\n+#include \"string-list.h\"\n \n static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=nn] <dir>\";\n \n@@ -30,6 +32,18 @@ static int multi_ack, nr_our_refs;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n static int no_progress, daemon_mode;\n static int shallow_nr;\n+\n+struct adv_ref {\n+\tstruct adv_ref *next;\n+\tchar *name;\n+\tunsigned pattern:1;\n+};\n+static struct adv_ref *to_advertise;\n+static struct adv_ref **advertise_tail = &to_advertise;\n+\n+static struct ref *local_refs;\n+static struct ref **refs_tail = &local_refs;\n+\n static struct object_array have_obj;\n static struct object_array want_obj;\n static unsigned int timeout;\n@@ -470,6 +484,17 @@ static int get_common_commits(void)\n \t}\n }\n \n+static void push_advertise(const char *name)\n+{\n+\tstruct adv_ref *adv = xcalloc(1, sizeof(*adv));\n+\tadv->name = xstrdup(name);\n+\tadv->pattern = !!strchr(adv->name, '*');\n+\t*advertise_tail = adv;\n+\tadvertise_tail = &adv->next;\n+}\n+\n+static void send_refs(void);\n+\n static void receive_needs(void)\n {\n \tstruct object_array shallows = {0, 0, NULL};\n@@ -484,11 +509,22 @@ static void receive_needs(void)\n \t\tunsigned char sha1_buf[20];\n \t\tlen = packet_read_line(0, line, sizeof(line));\n \t\treset_timeout();\n-\t\tif (!len)\n+\t\tif (!len) {\n+\t\t\tif (to_advertise) {\n+\t\t\t\tsend_refs();\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tbreak;\n+\t\t}\n \t\tif (debug_fd)\n \t\t\twrite_in_full(debug_fd, line, len);\n \n+\t\tif (!prefixcmp(line, \"expand \")) {\n+\t\t\tif (line[len - 1] == '\\n')\n+\t\t\t\tline[len - 1] = 0;\n+\t\t\tpush_advertise(line + 7);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(line, \"shallow \")) {\n \t\t\tunsigned char sha1[20];\n \t\t\tstruct object *object;\n@@ -603,11 +639,14 @@ static void receive_needs(void)\n \tfree(shallows.objects);\n }\n \n-static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+static int send_ref(struct string_list_item *item, void *cb_data)\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow no-progress\"\n-\t\t\" include-tag\";\n+\t\t\" include-tag expand\";\n+\tconst struct ref *ref = item->util;\n+\tconst char *refname = ref->name;\n+\tconst unsigned char *sha1 = ref->new_sha1;\n \tstruct object *o = parse_object(sha1);\n \n \tif (!o)\n@@ -631,12 +670,58 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n+static void send_refs(void)\n+{\n+\tstruct ref *to_send = NULL, **tail = &to_send;\n+\tstruct ref *ref;\n+\tstruct adv_ref *adv, *next_adv;\n+\tstruct string_list sorted_names;\n+\n+\tfor (adv = to_advertise; adv; adv = next_adv) {\n+\t\tstruct refspec spec;\n+\n+\t\tmemset(&spec, 0, sizeof(spec));\n+\t\tspec.pattern = adv->pattern;\n+\t\tspec.src = adv->name;\n+\t\tspec.dst = adv->name;\n+\t\tnext_adv = adv->next;\n+\t\tget_fetch_map(local_refs, &spec, &tail, 1);\n+\n+\t\tfree(adv->name);\n+\t\tfree(adv);\n+\t}\n+\tto_advertise = NULL;\n+\tadvertise_tail = &to_advertise;\n+\n+\tmemset(&sorted_names, 0, sizeof(sorted_names));\n+\tfor (ref = to_send; ref; ref = ref->next)\n+\t\tstring_list_insert(ref->name, &sorted_names)->util = ref;\n+\tfor_each_string_list(send_ref, &sorted_names, NULL);\n+\tstring_list_clear(&sorted_names, 0);\n+\tfree_refs(to_send);\n+\tpacket_flush(1);\n+}\n+\n+static int scan_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+{\n+\tstruct ref *r = alloc_ref(refname);\n+\thashcpy(r->new_sha1, sha1);\n+\t*refs_tail = r;\n+\trefs_tail = &r->next;\n+\treturn 0;\n+}\n+\n static void upload_pack(void)\n {\n \treset_timeout();\n-\thead_ref(send_ref, NULL);\n-\tfor_each_ref(send_ref, NULL);\n-\tpacket_flush(1);\n+\thead_ref(scan_ref, NULL);\n+\tfor_each_ref(scan_ref, NULL);\n+\n+\tpush_advertise(\"HEAD\");\n+\tpush_advertise(\"refs/heads/*\");\n+\tpush_advertise(\"refs/tags/*\");\n+\tsend_refs();\n+\n \treceive_needs();\n \tif (want_obj.nr) {\n \t\tget_common_commits();\n-- \n1.6.4.1.331.gda1d56\n\n-- \nShawn.\n"},{"id":"121772","messageId":"4A94DF84.4050906@viscovery.net","threadId":"20696","inReplyTo":"20090826021057.GL1033@spearce.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-08-26T07:08:52Z","receivedAt":"2009-08-26T07:08:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Shawn O. Pearce schrieb:\n>  static void upload_pack(void)\n>  {\n>  \treset_timeout();\n> -\thead_ref(send_ref, NULL);\n> -\tfor_each_ref(send_ref, NULL);\n> -\tpacket_flush(1);\n> +\thead_ref(scan_ref, NULL);\n> +\tfor_each_ref(scan_ref, NULL);\n> +\n> +\tpush_advertise(\"HEAD\");\n> +\tpush_advertise(\"refs/heads/*\");\n> +\tpush_advertise(\"refs/tags/*\");\n> +\tsend_refs();\n> +\n\nHow will this mesh with 'git clone --mirror'? Is the client expected to\nask with 'expand refs/*'?\n\n-- Hannes\n"},{"id":"121773","messageId":"20090826082256.GO1033@spearce.org","threadId":"20696","inReplyTo":"4A94DF84.4050906@viscovery.net","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-26T08:22:56Z","receivedAt":"2009-08-26T08:22:56Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> wrote:\n> Shawn O. Pearce schrieb:\n> >  static void upload_pack(void)\n> >  {\n> >  \treset_timeout();\n> > -\thead_ref(send_ref, NULL);\n> > -\tfor_each_ref(send_ref, NULL);\n> > -\tpacket_flush(1);\n> > +\thead_ref(scan_ref, NULL);\n> > +\tfor_each_ref(scan_ref, NULL);\n> > +\n> > +\tpush_advertise(\"HEAD\");\n> > +\tpush_advertise(\"refs/heads/*\");\n> > +\tpush_advertise(\"refs/tags/*\");\n> > +\tsend_refs();\n> > +\n> \n> How will this mesh with 'git clone --mirror'?\n\nNot well.\n\n> Is the client expected to\n> ask with 'expand refs/*'?\n\nIf the client is new enough to understand the \"expand\" extension,\nyes, it would ask for \"expand refs/*\" and get everthing, allowing\nit to complete a full mirror.\n\nIf the client is older and doesn't know this extension, then it\nis hopeless.  The server isn't advertising refs/*, doesn't know\nthat the client can't ask for refs/*, and the client doesn't know\nit is missing refs when it makes the mirror.\n\nThis backwards incompatible breakage is something I have no real\nsolution for.  :-|\n\n-- \nShawn.\n"},{"id":"121780","messageId":"7vk50reykp.fsf@alter.siamese.dyndns.org","threadId":"20696","inReplyTo":"20090826082256.GO1033@spearce.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-26T09:03:50Z","receivedAt":"2009-08-26T09:03:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n>> How will this mesh with 'git clone --mirror'?\n>\n> Not well.\n>\n>> Is the client expected to\n>> ask with 'expand refs/*'?\n>\n> If the client is new enough to understand the \"expand\" extension,\n> yes, it would ask for \"expand refs/*\" and get everthing, allowing\n> it to complete a full mirror.\n>\n> If the client is older and doesn't know this extension, then it\n> is hopeless.  The server isn't advertising refs/*, doesn't know\n> that the client can't ask for refs/*, and the client doesn't know\n> it is missing refs when it makes the mirror.\n>\n> This backwards incompatible breakage is something I have no real\n> solution for.  :-|\n\nBut we at least can assume that the server operator is reasonable and\nwouldn't go overboard, (ab)using this \"abbreviated advertisement\" feature\nto hide heads and tags from the clients.  I would suspect that a server\noperated by such a sick person who hides these normal refs will be shunned\nby users so it won't either happen in practice, or even if it happens, it\nwon't be a problem---simply because nobody would want to interact with\nsuch a server.  \n\nThink about in what situation you would want to do a mirror clone.  The\nmost obvious is to have a back-up or secondary distribution point, and I\ndo not think of any other sane reason (other than \"because I can\", which\ndoes not count).  If the original repository has so many refs to benefit\nfrom the \"abbreviated advertisement\" feature (otherwise there is no point\nusing it in the first place), then its mirror repository would also want\nto use the feature when talking to its clients, acting as a back-up\ndistribution point.  That means the version of git used to prime, update\nand serve the mirror will know the expand extention.\n\nI am hoping that we can finish 1.6.5 by mid September (let's tentatively\nsay we will shoot for 16th).  I expect the expand extention to be in\n'next' by that time, cooking for 1.7.0.  How does that timetable sound?\n"},{"id":"121813","messageId":"20090826170314.GP1033@spearce.org","threadId":"20696","inReplyTo":"7vk50reykp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] fix simple deepening of a repo","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-26T17:03:14Z","receivedAt":"2009-08-26T17:03:14Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> \"Shawn O. Pearce\" <spearce@spearce.org> writes:\n> >> How will this mesh with 'git clone --mirror'?\n> >\n> > Not well.\n> \n> But we at least can assume that the server operator is reasonable and\n> wouldn't go overboard, (ab)using this \"abbreviated advertisement\" feature\n> to hide heads and tags from the clients.\n\nYes.  My patch is hardcoded to show only heads and tags, and nothing else.\n\nBut I think we want to make this configurable, and show everything\nby default, but if there is a configuration entry, show only what\nthe configuration entry patterns suggest to advertise.\n\nThus an admin could hide refs/heads/*, but maybe he wants to, and\nshow only refs/heads/master, refs/heads/maint, refs/heads/next by\ndefault.  This is actually a rather clear indication to a client\nthat although there may be individual cooking topics scattered\nthrough the expanded refs/heads/* space, any reasonable default\nclone wouldn't take them.\n\n> Think about in what situation you would want to do a mirror clone.\n...\n> That means the version of git used to prime, update\n> and serve the mirror will know the expand extention.\n\nGreat point Junio.  The backwards compatibility may be a non-issue\nthen, especially if this is configurable and we advertise refs/*\nby default like we do now, and any reasonable admin who does enable\nthe hiding still advertises the core namspaces that really matter\nto the majority of clients.\n\n> I am hoping that we can finish 1.6.5 by mid September (let's tentatively\n> say we will shoot for 16th).  I expect the expand extention to be in\n> 'next' by that time, cooking for 1.7.0.  How does that timetable sound?\n\nOh, if 1.6.5 is mid-September, this is certainly not 1.6.5 material.\nI'm not in any rush, this should go in when its ready, but 1.7\nmight be reasonable.\n\n-- \nShawn.\n"},{"id":"122022","messageId":"20090828173007.GX1033@spearce.org","threadId":"20696","inReplyTo":"20090826021057.GL1033@spearce.org","subject":"[RFC PATCH] upload-pack: expand capability advertises additional refs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-28T17:30:07Z","receivedAt":"2009-08-28T17:30:07Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The expand capability and associated command permits the client\nto ask for information about refs which were not in the initial\nadvertisement sent when the connection was first opened.\n\nIn the below exchange the server initially only advertises its\ncurrent HEAD, refs/heads and refs/tags namespaces.  However,\nthe client has been instructed to fetch anything which matches\nrefs/remotes/jc/*.\n\nSince no matching refs appeared in the initial advertisement,\nthe client requests the server to expand the desired pattern,\nand terminates its expand request list with a flush.\n\nUpon receiving a flush from the client, the server displays any\nlocal refs which match any of the expand patterns requested,\nand then closes this secondary advertisement list with a flush.\nIf no refs matched, the server immediately returns a flush.\n\nIf multiple expand patterns match the same ref, the ref is returned\nonly once in the secondary advertisement, avoid confusing the client\nwith duplicate results.\n\n  S: 008f... HEAD\\0...include-tag expand\n  S: 0043... refs/heads/build-next\n  S: 0040... refs/tags/v1.6.4.1\n  S: 0043... refs/tags/v1.6.4.1^{}\n  S: 0000\n\n  C: 001dexpand refs/remotes/jc/*\n  C: 0000\n\n  S: 0043... refs/remotes/jc/maint\n  S: 0044... refs/remotes/jc/master\n  S: 0000\n\n  C: 0031want ...\n  C: 0000\n\nRepository owners can control the set of refs which are sent as part\nof the initial advertisement by configuring upload.advertise in the\nrepository configuration file.  If not set this is assumed to be\n\"refs/*\", matching the prior behavior of advertising every local ref.\n\nFetch clients which are not using the anonymous git:// protocol\nand which do not support the expand protocol extension may still\nforce the server to expand its configured upload.advertise set by\npassing the --expand=<pattern> command line flag as part of the\n--upload-pack= command line given to clone or fetch.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\nSigned-off-by: Shawn O. Pearce <sop@google.com>\n---\n\n This is roughly my final server side version of this proposal.\n I still need to write the client code, but want to at least get\n this out there for further discussion.\n\n Documentation/config.txt          |   10 +++\n Documentation/git-upload-pack.txt |   10 +++-\n upload-pack.c                     |  131 +++++++++++++++++++++++++++++++++++--\n 3 files changed, 143 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 5256c7f..07907b6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1489,6 +1489,16 @@ transfer.unpackLimit::\n \tnot set, the value of this variable is used instead.\n \tThe default value is 100.\n \n+upload.advertise::\n+\tThe default set of refs to advertise when a fetch or\n+\tclone client connects to this repository.  Additional\n+\tlocal refs not in the default advertisement can still\n+\tbe guessed and requested by clients through additional\n+\tnetwork round trips.  Refs may be expressed as a complete\n+\tname (\"refs/heads/master\") or as a pattern expected by\n+\tremote.<name>.fetch (such as \"refs/heads/*\").  If not\n+\tspecified, all refs are advertised (\"refs/*\").\n+\n url.<base>.insteadOf::\n \tAny URL that starts with this value will be rewritten to\n \tstart, instead, with <base>. In cases where some site serves a\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex b8e49dc..4cd9cc0 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -8,7 +8,7 @@ git-upload-pack - Send objects packed back to git-fetch-pack\n \n SYNOPSIS\n --------\n-'git upload-pack' [--strict] [--timeout=<n>] <directory>\n+'git upload-pack' [--strict] [--timeout=<n>] [--expand=<pattern> ...] <directory>\n \n DESCRIPTION\n -----------\n@@ -30,6 +30,14 @@ OPTIONS\n --timeout=<n>::\n \tInterrupt transfer after <n> seconds of inactivity.\n \n+--expand=<pattern>::\n+\tExpand the requested pattern and advertise matching refs,\n+\teven if those refs were not matched by upload.advertise.\n+\tThis option may be repeated to request expansion of more\n+\tthan one pattern.  This option is intended only as an\n+\tescape hatch for older clients to fetch from a server\n+\twhich has hidden interesting refs.\n+\n <directory>::\n \tThe repository to sync from.\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4d8be83..890c1c5 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -10,6 +10,8 @@\n #include \"revision.h\"\n #include \"list-objects.h\"\n #include \"run-command.h\"\n+#include \"remote.h\"\n+#include \"string-list.h\"\n \n static const char upload_pack_usage[] = \"git upload-pack [--strict] [--timeout=nn] <dir>\";\n \n@@ -30,6 +32,19 @@ static int multi_ack, nr_our_refs;\n static int use_thin_pack, use_ofs_delta, use_include_tag;\n static int no_progress, daemon_mode;\n static int shallow_nr;\n+\n+struct adv_ref {\n+\tstruct adv_ref *next;\n+\tchar *name;\n+\tunsigned pattern:1;\n+};\n+static struct adv_ref *to_advertise;\n+static struct adv_ref **advertise_tail = &to_advertise;\n+static int configured_advertise;\n+\n+static struct ref *local_refs;\n+static struct ref **refs_tail = &local_refs;\n+\n static struct object_array have_obj;\n static struct object_array want_obj;\n static unsigned int timeout;\n@@ -470,6 +485,69 @@ static int get_common_commits(void)\n \t}\n }\n \n+static void push_advertise(const char *name)\n+{\n+\tstruct adv_ref *adv = xcalloc(1, sizeof(*adv));\n+\tadv->name = xstrdup(name);\n+\tadv->pattern = !!strchr(adv->name, '*');\n+\t*advertise_tail = adv;\n+\tadvertise_tail = &adv->next;\n+}\n+\n+static int upload_pack_config(const char *var, const char *value, void *cb)\n+{\n+\tif (strcmp(var, \"upload.advertise\") == 0) {\n+\t\tconfigured_advertise = 1;\n+\t\tpush_advertise(value);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n+static int send_ref(struct string_list_item *item, void *cb_data);\n+static void send_refs(void)\n+{\n+\tstruct ref *to_send = NULL, **tail = &to_send;\n+\tstruct ref *ref;\n+\tstruct adv_ref *adv, *next_adv;\n+\tstruct string_list sorted_names;\n+\n+\tfor (adv = to_advertise; adv; adv = next_adv) {\n+\t\tstruct refspec spec;\n+\n+\t\tmemset(&spec, 0, sizeof(spec));\n+\t\tspec.pattern = adv->pattern;\n+\t\tspec.src = adv->name;\n+\t\tspec.dst = adv->name;\n+\t\tnext_adv = adv->next;\n+\t\tget_fetch_map(local_refs, &spec, &tail, 1);\n+\n+\t\tfree(adv->name);\n+\t\tfree(adv);\n+\t}\n+\tto_advertise = NULL;\n+\tadvertise_tail = &to_advertise;\n+\n+\t/* We may have duplicate copies of the same ref above, if\n+\t * two advertise records matched the same local name.  To\n+\t * avoid sending the same ref twice to the client, we put\n+\t * them into a sorted list and then skip duplicates as we\n+\t * output them.\n+\t */\n+\tmemset(&sorted_names, 0, sizeof(sorted_names));\n+\tfor (ref = to_send; ref; ref = ref->next)\n+\t\tstring_list_append(ref->name, &sorted_names)->util = ref;\n+\tsort_string_list(&sorted_names);\n+\n+\tref = NULL;\n+\tfor_each_string_list(send_ref, &sorted_names, &ref);\n+\n+\tstring_list_clear(&sorted_names, 0);\n+\tfree_refs(to_send);\n+\tpacket_flush(1);\n+}\n+\n static void receive_needs(void)\n {\n \tstruct object_array shallows = {0, 0, NULL};\n@@ -484,11 +562,22 @@ static void receive_needs(void)\n \t\tunsigned char sha1_buf[20];\n \t\tlen = packet_read_line(0, line, sizeof(line));\n \t\treset_timeout();\n-\t\tif (!len)\n+\t\tif (!len) {\n+\t\t\tif (to_advertise) {\n+\t\t\t\tsend_refs();\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tbreak;\n+\t\t}\n \t\tif (debug_fd)\n \t\t\twrite_in_full(debug_fd, line, len);\n \n+\t\tif (!prefixcmp(line, \"expand \")) {\n+\t\t\tif (line[len - 1] == '\\n')\n+\t\t\t\tline[len - 1] = 0;\n+\t\t\tpush_advertise(line + 7);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(line, \"shallow \")) {\n \t\t\tunsigned char sha1[20];\n \t\t\tstruct object *object;\n@@ -603,13 +692,22 @@ static void receive_needs(void)\n \tfree(shallows.objects);\n }\n \n-static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+static int send_ref(struct string_list_item *item, void *cb_data)\n {\n \tstatic const char *capabilities = \"multi_ack thin-pack side-band\"\n \t\t\" side-band-64k ofs-delta shallow no-progress\"\n-\t\t\" include-tag\";\n-\tstruct object *o = parse_object(sha1);\n+\t\t\" include-tag expand\";\n+\tstruct ref **last_ref = cb_data;\n+\tstruct ref *ref = item->util;\n+\tconst char *refname = ref->name;\n+\tconst unsigned char *sha1 = ref->new_sha1;\n+\tstruct object *o;\n+\n+\tif (*last_ref && !strcmp(refname, (*last_ref)->name))\n+\t\treturn 0;\n+\t*last_ref = ref;\n \n+\to = parse_object(sha1);\n \tif (!o)\n \t\tdie(\"git upload-pack: cannot find object %s:\", sha1_to_hex(sha1));\n \n@@ -631,12 +729,26 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n+static int scan_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)\n+{\n+\tstruct ref *r = alloc_ref(refname);\n+\thashcpy(r->new_sha1, sha1);\n+\t*refs_tail = r;\n+\trefs_tail = &r->next;\n+\treturn 0;\n+}\n+\n static void upload_pack(void)\n {\n+\tgit_config(upload_pack_config, NULL);\n+\tif (!configured_advertise)\n+\t\tpush_advertise(\"refs/*\");\n+\n+\thead_ref(scan_ref, NULL);\n+\tfor_each_ref(scan_ref, NULL);\n+\n \treset_timeout();\n-\thead_ref(send_ref, NULL);\n-\tfor_each_ref(send_ref, NULL);\n-\tpacket_flush(1);\n+\tsend_refs();\n \treceive_needs();\n \tif (want_obj.nr) {\n \t\tget_common_commits();\n@@ -652,6 +764,7 @@ int main(int argc, char **argv)\n \n \tgit_extract_argv0_path(argv[0]);\n \tread_replace_refs = 0;\n+\tpush_advertise(\"HEAD\");\n \n \tfor (i = 1; i < argc; i++) {\n \t\tchar *arg = argv[i];\n@@ -667,6 +780,10 @@ int main(int argc, char **argv)\n \t\t\tdaemon_mode = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--expand=\")) {\n+\t\t\tpush_advertise(arg + 9);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\ti++;\n \t\t\tbreak;\n-- \n1.6.4.1.341.gf2a44\n"},{"id":"122038","messageId":"7vr5uvlpue.fsf@alter.siamese.dyndns.org","threadId":"20696","inReplyTo":"20090828173007.GX1033@spearce.org","subject":"Re: [RFC PATCH] upload-pack: expand capability advertises additional refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-28T19:07:21Z","receivedAt":"2009-08-28T19:07:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" <spearce@spearce.org> writes:\n\n>  static void upload_pack(void)\n>  {\n> +\tgit_config(upload_pack_config, NULL);\n> +\tif (!configured_advertise)\n> +\t\tpush_advertise(\"refs/*\");\n> +\n> +\thead_ref(scan_ref, NULL);\n> +\tfor_each_ref(scan_ref, NULL);\n> +\n>  \treset_timeout();\n> -\thead_ref(send_ref, NULL);\n> -\tfor_each_ref(send_ref, NULL);\n> -\tpacket_flush(1);\n> +\tsend_refs();\n>  \treceive_needs();\n>  \tif (want_obj.nr) {\n>  \t\tget_common_commits();\n> @@ -652,6 +764,7 @@ int main(int argc, char **argv)\n>  \n>  \tgit_extract_argv0_path(argv[0]);\n>  \tread_replace_refs = 0;\n> +\tpush_advertise(\"HEAD\");\n>  \n>  \tfor (i = 1; i < argc; i++) {\n>  \t\tchar *arg = argv[i];\n> @@ -667,6 +780,10 @@ int main(int argc, char **argv)\n>  \t\t\tdaemon_mode = 1;\n>  \t\t\tcontinue;\n>  \t\t}\n> +\t\tif (!prefixcmp(arg, \"--expand=\")) {\n> +\t\t\tpush_advertise(arg + 9);\n> +\t\t\tcontinue;\n> +\t\t}\n>  \t\tif (!strcmp(arg, \"--\")) {\n>  \t\t\ti++;\n>  \t\t\tbreak;\n\nThis arrangement of push_advertise() calls are rather curious.  I think\nyour design guidelines are:\n\n - We do want to advertise HEAD, so it can (and should) be unconditional;\n\n - We may want to restrict with configuration, or use \"refs/*\" as the\n   fallback default;\n\n - We may want to (extend|replace) advertised set configured in the\n   configuration file.\n\nI would naively expect the above to be implemented in this order:\n\n - In main, first thing is to do the git_config() bit; if to_advertise is\n   non-empty after git_config() returns, we have seen upload.advertise.\n   Otherwise we haven't.  Push \"refs/*\" in the latter case.\n\n - Then, parse the command line.  Do we see \"--expand=\"?  If so:\n\n   . When handling the first \"--expand=\", clear to_advertise list.  This\n     step is optional---necessary only if we want to make config variable\n     override-able ;-);\n\n   . Then, push the pattern in.\n\n - And finally push \"HEAD\" in unconditionally.\n\nWhich would mean a few things:\n\n - Your version always extends what is read from the configuration with\n   --expand=, but I think replacing would be the right thing to do.\n\n - configured_advertise global variable can go.\n\n - Instead, to implement \"--expand= replaces\", we need a local variable in\n   main() to remember if we already cleared to_advertise of the patterns\n   read from the configuration file, and use it in the part that parses\n   \"--expand=\".\n\nBut I may not be thinking clearly; I was up all night because I couldn't\nsleep under loud noises of fire-fighting helicopters...\n"}]}