{"thread":{"id":"48971","subject":"[BUG] fetching sometimes doesn't update refs","startedAt":"2018-07-29T12:19:03Z","lastAt":"2018-08-02T16:40:30Z","messageCount":13,"participants":["Jeff King","Brandon Williams","Jonathan Tan","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"353821","messageId":"20180729121900.GA16770@sigill.intra.peff.net","threadId":"48971","inReplyTo":null,"subject":"[BUG] fetching sometimes doesn't update refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-29T12:19:00Z","receivedAt":"2018-07-29T12:19:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I've noticed for the past couple of weeks that some of my fetches don't\nseem to actually update refs, but a follow-up fetch will. I finally\nmanaged to catch it in the act and track it down. It bisects to your\n989b8c4452 (fetch-pack: put shallow info in output parameter,\n2018-06-27). \n\nA reproduction recipe is below. I can't imagine why this repo in\nparticular triggers it, but it was the one where I initially saw the\nproblem (and doing a tiny reproduction does not seem to work). I'm\nguessing it has something to do with the refs, since the main change in\nthe offending commit is that we recompute the refmap.\n\n-- >8 --\n# clone the repo as it is today\ngit clone https://github.com/cmcaine/tridactyl.git\ncd tridactyl\n\n# roll back the refs so that there is something to fetch\nfor i in refs/heads/master refs/remotes/origin/master; do\n\tgit update-ref $i $i^\ndone\n\n# and delete the now-unreferenced objects, pretending we are an earlier\n# clone that had not yet fetched\nrm -rf .git/logs\ngit repack -ad\n\n# now fetch; this will get the objects but fail to update refs\ngit fetch\n\n# and fetching again will actually update the refs\ngit fetch\n-- 8< --\n\n-Peff\n"},{"id":"353923","messageId":"20180730175341.GB154732@google.com","threadId":"48971","inReplyTo":"20180729121900.GA16770@sigill.intra.peff.net","subject":"Re: [BUG] fetching sometimes doesn't update refs","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-30T17:53:41Z","receivedAt":"2018-07-30T17:53:45Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/29, Jeff King wrote:\n> I've noticed for the past couple of weeks that some of my fetches don't\n> seem to actually update refs, but a follow-up fetch will. I finally\n> managed to catch it in the act and track it down. It bisects to your\n> 989b8c4452 (fetch-pack: put shallow info in output parameter,\n> 2018-06-27). \n> \n> A reproduction recipe is below. I can't imagine why this repo in\n> particular triggers it, but it was the one where I initially saw the\n> problem (and doing a tiny reproduction does not seem to work). I'm\n> guessing it has something to do with the refs, since the main change in\n> the offending commit is that we recompute the refmap.\n\nI've noticed this behavior sporadically as well, though I've never been\nable to reliably reproduce it, so thanks for creating a reproduction\nrecipe.  I suspected that it had to do with the ref-in-want series so\nthanks for tracking that down too.  We'll take a look.\n\n> \n> -- >8 --\n> # clone the repo as it is today\n> git clone https://github.com/cmcaine/tridactyl.git\n> cd tridactyl\n> \n> # roll back the refs so that there is something to fetch\n> for i in refs/heads/master refs/remotes/origin/master; do\n> \tgit update-ref $i $i^\n> done\n> \n> # and delete the now-unreferenced objects, pretending we are an earlier\n> # clone that had not yet fetched\n> rm -rf .git/logs\n> git repack -ad\n> \n> # now fetch; this will get the objects but fail to update refs\n> git fetch\n> \n> # and fetching again will actually update the refs\n> git fetch\n> -- 8< --\n> \n> -Peff\n\n-- \nBrandon Williams\n"},{"id":"353971","messageId":"20180730225601.107502-1-jonathantanmy@google.com","threadId":"48971","inReplyTo":"20180729121900.GA16770@sigill.intra.peff.net","subject":"[PATCH] transport: report refs only if transport does","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-30T22:56:01Z","receivedAt":"2018-07-30T22:56:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 989b8c4452 (\"fetch-pack: put shallow info in output parameter\",\n2018-06-28) allows transports to report the refs that they have fetched\nin a new out-parameter \"fetched_refs\". If they do so,\ntransport_fetch_refs() makes this information available to its caller.\n\nBecause transport_fetch_refs() filters the refs sent to the transport,\nit cannot just report the transport's result directly, but first needs\nto readd the excluded refs, pretending that they are fetched. However,\nthis results in a wrong result if the transport did not report the refs\nthat they have fetched in \"fetched_refs\" - the excluded refs would be\nadded and reported, presenting an incomplete picture to the caller.\n\nInstead, readd the excluded refs only if the transport reported fetched\nrefs.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThanks for the reproduction recipe, Peff. Here's a fix. It can be\nreproduced with something using a remote helper's fetch command (and not\nusing \"connect\" or \"stateless-connect\"), fetching at least one ref that\nrequires a ref update and at least one that does not (as you can see\nfrom the included test).\n---\n t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++\n transport.c                 | 32 ++++++++++++++++++++++++--------\n 2 files changed, 42 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 913089b144..989d034acc 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -369,6 +369,24 @@ test_expect_success 'custom http headers' '\n \t\tsubmodule update sub\n '\n \n+test_expect_success 'using fetch command in remote-curl updates refs' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/twobranch\" &&\n+\trm -rf \"$SERVER\" client &&\n+\n+\tgit init \"$SERVER\" &&\n+\ttest_commit -C \"$SERVER\" foo &&\n+\tgit -C \"$SERVER\" update-ref refs/heads/anotherbranch foo &&\n+\n+\tgit clone $HTTPD_URL/smart/twobranch client &&\n+\n+\ttest_commit -C \"$SERVER\" bar &&\n+\tgit -C client -c protocol.version=0 fetch &&\n+\n+\tgit -C \"$SERVER\" rev-parse master >expect &&\n+\tgit -C client rev-parse origin/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=1\" >cookies &&\ndiff --git a/transport.c b/transport.c\nindex fdd813f684..2a2415d79c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1230,17 +1230,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs,\n \tstruct ref **heads = NULL;\n \tstruct ref *nop_head = NULL, **nop_tail = &nop_head;\n \tstruct ref *rm;\n+\tstruct ref *fetched_by_transport = NULL;\n \n \tfor (rm = refs; rm; rm = rm->next) {\n \t\tnr_refs++;\n \t\tif (rm->peer_ref &&\n \t\t    !is_null_oid(&rm->old_oid) &&\n \t\t    !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {\n-\t\t\t/*\n-\t\t\t * These need to be reported as fetched, but we don't\n-\t\t\t * actually need to fetch them.\n-\t\t\t */\n \t\t\tif (fetched_refs) {\n+\t\t\t\t/*\n+\t\t\t\t * These may need to be reported as fetched,\n+\t\t\t\t * but we don't actually need to fetch them.\n+\t\t\t\t */\n \t\t\t\tstruct ref *nop_ref = copy_ref(rm);\n \t\t\t\t*nop_tail = nop_ref;\n \t\t\t\tnop_tail = &nop_ref->next;\n@@ -1264,10 +1265,25 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs,\n \t\t\theads[nr_heads++] = rm;\n \t}\n \n-\trc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);\n-\tif (fetched_refs && nop_head) {\n-\t\t*nop_tail = *fetched_refs;\n-\t\t*fetched_refs = nop_head;\n+\trc = transport->vtable->fetch(transport, nr_heads, heads,\n+\t\t\t\t      fetched_refs ? &fetched_by_transport : NULL);\n+\tif (fetched_refs) {\n+\t\tif (fetched_by_transport) {\n+\t\t\t/*\n+\t\t\t * The transport reported its fetched refs. Pretend\n+\t\t\t * that we also fetched the ones that we didn't need to\n+\t\t\t * fetch.\n+\t\t\t */\n+\t\t\t*nop_tail = fetched_by_transport;\n+\t\t\t*fetched_refs = nop_head;\n+\t\t} else if (!fetched_by_transport) {\n+\t\t\t/*\n+\t\t\t * The transport didn't report its fetched refs, so\n+\t\t\t * this function will not report them either. We have\n+\t\t\t * no use for nop_head.\n+\t\t\t */\n+\t\t\tfree_refs(nop_head);\n+\t\t}\n \t}\n \n \tfree(heads);\n-- \n2.18.0.345.g5c9ce644c3-goog\n\n"},{"id":"354106","messageId":"20180731192415.GC3372@sigill.intra.peff.net","threadId":"48971","inReplyTo":"20180730225601.107502-1-jonathantanmy@google.com","subject":"Re: [PATCH] transport: report refs only if transport does","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-07-31T19:24:15Z","receivedAt":"2018-07-31T19:24:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:\n\n> Commit 989b8c4452 (\"fetch-pack: put shallow info in output parameter\",\n> 2018-06-28) allows transports to report the refs that they have fetched\n> in a new out-parameter \"fetched_refs\". If they do so,\n> transport_fetch_refs() makes this information available to its caller.\n> \n> Because transport_fetch_refs() filters the refs sent to the transport,\n> it cannot just report the transport's result directly, but first needs\n> to readd the excluded refs, pretending that they are fetched. However,\n> this results in a wrong result if the transport did not report the refs\n> that they have fetched in \"fetched_refs\" - the excluded refs would be\n> added and reported, presenting an incomplete picture to the caller.\n\nThis part leaves me confused. If we are not fetching them, then why do\nwe need to pretend that they are fetched?\n\nI think I am showing my lack of understanding about the reason for this\nwhole \"return the fetched refs\" scheme from 989b8c4452, and probably\nreading the rest of that series would make it more clear. But from the\nperspective of somebody digging into history and finding just this\ncommit, it probably needs to lay out a little more of the reasoning.\n\n> Thanks for the reproduction recipe, Peff. Here's a fix. It can be\n> reproduced with something using a remote helper's fetch command (and not\n> using \"connect\" or \"stateless-connect\"), fetching at least one ref that\n> requires a ref update and at least one that does not (as you can see\n> from the included test).\n\nAh, that explains why I couldn't reproduce it with another repository; I\nwas using a direct git-upload-pack fetch, which wouldn't trigger the\nremote helper code.\n\n> diff --git a/transport.c b/transport.c\n> index fdd813f684..2a2415d79c 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1230,17 +1230,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs,\n>  \tstruct ref **heads = NULL;\n>  \tstruct ref *nop_head = NULL, **nop_tail = &nop_head;\n>  \tstruct ref *rm;\n> +\tstruct ref *fetched_by_transport = NULL;\n>  \n>  \tfor (rm = refs; rm; rm = rm->next) {\n>  \t\tnr_refs++;\n>  \t\tif (rm->peer_ref &&\n>  \t\t    !is_null_oid(&rm->old_oid) &&\n>  \t\t    !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {\n> -\t\t\t/*\n> -\t\t\t * These need to be reported as fetched, but we don't\n> -\t\t\t * actually need to fetch them.\n> -\t\t\t */\n>  \t\t\tif (fetched_refs) {\n> +\t\t\t\t/*\n> +\t\t\t\t * These may need to be reported as fetched,\n> +\t\t\t\t * but we don't actually need to fetch them.\n> +\t\t\t\t */\n\nSo it's really this comment that leaves me the most puzzled.\n\n> @@ -1264,10 +1265,25 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs,\n>  \t\t\theads[nr_heads++] = rm;\n>  \t}\n>  \n> -\trc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);\n> -\tif (fetched_refs && nop_head) {\n> -\t\t*nop_tail = *fetched_refs;\n> -\t\t*fetched_refs = nop_head;\n> +\trc = transport->vtable->fetch(transport, nr_heads, heads,\n> +\t\t\t\t      fetched_refs ? &fetched_by_transport : NULL);\n> +\tif (fetched_refs) {\n> +\t\tif (fetched_by_transport) {\n> +\t\t\t/*\n> +\t\t\t * The transport reported its fetched refs. Pretend\n> +\t\t\t * that we also fetched the ones that we didn't need to\n> +\t\t\t * fetch.\n> +\t\t\t */\n> +\t\t\t*nop_tail = fetched_by_transport;\n> +\t\t\t*fetched_refs = nop_head;\n> +\t\t} else if (!fetched_by_transport) {\n> +\t\t\t/*\n> +\t\t\t * The transport didn't report its fetched refs, so\n> +\t\t\t * this function will not report them either. We have\n> +\t\t\t * no use for nop_head.\n> +\t\t\t */\n> +\t\t\tfree_refs(nop_head);\n> +\t\t}\n\nThis part makes sense to me based on the description (and on the\nassumption that reporting those nop refs is useful in the first place ;)\n).\n\nSo I think your fix here is probably the right thing, but I'm just left\nconfused by the background a bit.\n\n-Peff\n"},{"id":"354128","messageId":"xmqqa7q79jcf.fsf@gitster-ct.c.googlers.com","threadId":"48971","inReplyTo":"20180731192415.GC3372@sigill.intra.peff.net","subject":"Re: [PATCH] transport: report refs only if transport does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-31T21:38:24Z","receivedAt":"2018-07-31T21:38:31Z","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> On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:\n>\n>> Commit 989b8c4452 (\"fetch-pack: put shallow info in output parameter\",\n>> 2018-06-28) allows transports to report the refs that they have fetched\n>> in a new out-parameter \"fetched_refs\". If they do so,\n>> transport_fetch_refs() makes this information available to its caller.\n>> \n>> Because transport_fetch_refs() filters the refs sent to the transport,\n>> it cannot just report the transport's result directly, but first needs\n>> to readd the excluded refs, pretending that they are fetched. However,\n>> this results in a wrong result if the transport did not report the refs\n>> that they have fetched in \"fetched_refs\" - the excluded refs would be\n>> added and reported, presenting an incomplete picture to the caller.\n>\n> This part leaves me confused. If we are not fetching them, then why do\n> we need to pretend that they are fetched?\n\nWhat leaves me even more confused is that the entire log message\ndoes not make it clear what the end-user observable problem the\npatch is trying to solve.\n\nIs this \"we sometimes follow and sometimes fail to follow refs while\nfetching\"?  Does it affect all protocol versions and transports, or\nonly just selected few (and if so which ones)?\n\nIn minds of those who reported an issue and wrote the fix, the issue\nmay be fresh, but let's write the commit log message for ourselves 6\nmonths down the road.\n\nThanks.\n"},{"id":"354133","messageId":"20180731232343.184463-1-jonathantanmy@google.com","threadId":"48971","inReplyTo":"20180731192415.GC3372@sigill.intra.peff.net","subject":"Re: [PATCH] transport: report refs only if transport does","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-31T23:23:43Z","receivedAt":"2018-07-31T23:23:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:\n> \n> > Commit 989b8c4452 (\"fetch-pack: put shallow info in output parameter\",\n> > 2018-06-28) allows transports to report the refs that they have fetched\n> > in a new out-parameter \"fetched_refs\". If they do so,\n> > transport_fetch_refs() makes this information available to its caller.\n> > \n> > Because transport_fetch_refs() filters the refs sent to the transport,\n> > it cannot just report the transport's result directly, but first needs\n> > to readd the excluded refs, pretending that they are fetched. However,\n> > this results in a wrong result if the transport did not report the refs\n> > that they have fetched in \"fetched_refs\" - the excluded refs would be\n> > added and reported, presenting an incomplete picture to the caller.\n> \n> This part leaves me confused. If we are not fetching them, then why do\n> we need to pretend that they are fetched?\n\nThe short answer is that we need:\n (1) the complete list of refs that was passed to\n     transport_fetch_refs(),\n (2) with shallow information (REF_STATUS_REJECT_SHALLOW set if\n     relevant), and\n (3) with updated OIDs if ref-in-want was used.\n\nThe fetched_refs out param already fulfils (2) and (3), and this patch\nmakes it fulfil (1). As for calling them fetched_refs, perhaps that is a\nmisnomer, but they do appear in FETCH_HEAD even though they are not\ntruly fetched.\n\nWhich raises the question...if completeness is so important, why not\nreuse the input list of refs and document that transport_fetch_refs()\ncan mutate the input list? You ask the same question below, so I'll put\nthe answer after quoting your paragraph.\n\n> I think I am showing my lack of understanding about the reason for this\n> whole \"return the fetched refs\" scheme from 989b8c4452, and probably\n> reading the rest of that series would make it more clear. But from the\n> perspective of somebody digging into history and finding just this\n> commit, it probably needs to lay out a little more of the reasoning.\n\nI think it's because 989b8c4452 is based on my earlier work [1] which\nalso had a fetched_refs out param. Its main reason is to enable the\ninvoker of transport_fetch_refs() to specify ref patterns (as you can\nsee in a later commit in the same patch set [2]) - and if we specify\npatterns, the invoker of transport_fetch_refs() needs the resulting refs\n(which are provided through fetched_refs).\n\nIn the version that made it to master, however, there was some debate\nabout whether ref patterns need to be allowed. In the end, ref patterns\nwere not allowed [3], but the fetched_refs out param was still left in.\n\nI think that reverting the API might work, but am on the fence about it.\nIt would reduce the number of questions about the code (and would\nprobably automatically fix the issue that I was fixing in the first\nplace), but if we were to revert the API and then decide that we do want\nref patterns in \"want-ref\" (or expand transport_fetch_refs in some\nsimilar way), we would need to revert our revert, causing code churn.\n\n[1] https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathantanmy@google.com/\n[2] https://public-inbox.org/git/eef2b77d88df0db08e4a1505b06e0af2d40143d5.1485381677.git.jonathantanmy@google.com/\n[3] https://public-inbox.org/git/20180620213235.10952-1-bmwill@google.com/\n"},{"id":"354134","messageId":"20180731232943.186226-1-jonathantanmy@google.com","threadId":"48971","inReplyTo":"xmqqa7q79jcf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] transport: report refs only if transport does","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-07-31T23:29:43Z","receivedAt":"2018-07-31T23:29:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> What leaves me even more confused is that the entire log message\n> does not make it clear what the end-user observable problem the\n> patch is trying to solve.\n> \n> Is this \"we sometimes follow and sometimes fail to follow refs while\n> fetching\"?  Does it affect all protocol versions and transports, or\n> only just selected few (and if so which ones)?\n\nNormally I would respond by creating a new patch with the answer in its\ncommit message, but I'm now not sure about whether it's better to revert\nback to the non-\"fetched_refs\" API entirely (as I explained in the reply\nto Peff I just sent [1]), so I'll answer your questions here for now:\n\n - Yes. We fail to follow when we fetch at least one ref that is\n   up-to-date and one ref that is not, and when we're using the \"fetch\"\n   command in a remote helper (for example, HTTP protocol v0).\n - I haven't checked exhaustively, but as far as I know, affects HTTP\n   protocol v0, and does not affect anything using connect or\n   stateless-connect (e.g. HTTP protocol v2, ssh).\n\nWhen I create a new patch, I'll also include these answers in its commit\nmessage.\n\n[1] https://public-inbox.org/git/20180731232343.184463-1-jonathantanmy@google.com/\n"},{"id":"354175","messageId":"20180801171806.GA122458@google.com","threadId":"48971","inReplyTo":"20180731232343.184463-1-jonathantanmy@google.com","subject":"Re: [PATCH] transport: report refs only if transport does","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-01T17:18:06Z","receivedAt":"2018-08-01T18:17:53Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 07/31, Jonathan Tan wrote:\n> > On Mon, Jul 30, 2018 at 03:56:01PM -0700, Jonathan Tan wrote:\n> > \n> > > Commit 989b8c4452 (\"fetch-pack: put shallow info in output parameter\",\n> > > 2018-06-28) allows transports to report the refs that they have fetched\n> > > in a new out-parameter \"fetched_refs\". If they do so,\n> > > transport_fetch_refs() makes this information available to its caller.\n> > > \n> > > Because transport_fetch_refs() filters the refs sent to the transport,\n> > > it cannot just report the transport's result directly, but first needs\n> > > to readd the excluded refs, pretending that they are fetched. However,\n> > > this results in a wrong result if the transport did not report the refs\n> > > that they have fetched in \"fetched_refs\" - the excluded refs would be\n> > > added and reported, presenting an incomplete picture to the caller.\n> > \n> > This part leaves me confused. If we are not fetching them, then why do\n> > we need to pretend that they are fetched?\n> \n> The short answer is that we need:\n>  (1) the complete list of refs that was passed to\n>      transport_fetch_refs(),\n>  (2) with shallow information (REF_STATUS_REJECT_SHALLOW set if\n>      relevant), and\n>  (3) with updated OIDs if ref-in-want was used.\n> \n> The fetched_refs out param already fulfils (2) and (3), and this patch\n> makes it fulfil (1). As for calling them fetched_refs, perhaps that is a\n> misnomer, but they do appear in FETCH_HEAD even though they are not\n> truly fetched.\n> \n> Which raises the question...if completeness is so important, why not\n> reuse the input list of refs and document that transport_fetch_refs()\n> can mutate the input list? You ask the same question below, so I'll put\n> the answer after quoting your paragraph.\n> \n> > I think I am showing my lack of understanding about the reason for this\n> > whole \"return the fetched refs\" scheme from 989b8c4452, and probably\n> > reading the rest of that series would make it more clear. But from the\n> > perspective of somebody digging into history and finding just this\n> > commit, it probably needs to lay out a little more of the reasoning.\n> \n> I think it's because 989b8c4452 is based on my earlier work [1] which\n> also had a fetched_refs out param. Its main reason is to enable the\n> invoker of transport_fetch_refs() to specify ref patterns (as you can\n> see in a later commit in the same patch set [2]) - and if we specify\n> patterns, the invoker of transport_fetch_refs() needs the resulting refs\n> (which are provided through fetched_refs).\n> \n> In the version that made it to master, however, there was some debate\n> about whether ref patterns need to be allowed. In the end, ref patterns\n> were not allowed [3], but the fetched_refs out param was still left in.\n> \n> I think that reverting the API might work, but am on the fence about it.\n> It would reduce the number of questions about the code (and would\n> probably automatically fix the issue that I was fixing in the first\n\nIf you believe the API is difficult to work with (which given this bug\nit is) then perhaps we go with your suggestion and revert the API back\nto only providing a list of input refs and having the fetch operation\nmutate that input list.\n\n> place), but if we were to revert the API and then decide that we do want\n> ref patterns in \"want-ref\" (or expand transport_fetch_refs in some\n> similar way), we would need to revert our revert, causing code churn.\n\nI haven't thought too much about what we would need to do in the event\nwe add patterns to ref-in-want, but couldn't we possible mutate the\ninput list again in this case and just simply add the resulting refs to\nthe input list?\n\n> \n> [1] https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathantanmy@google.com/\n> [2] https://public-inbox.org/git/eef2b77d88df0db08e4a1505b06e0af2d40143d5.1485381677.git.jonathantanmy@google.com/\n> [3] https://public-inbox.org/git/20180620213235.10952-1-bmwill@google.com/\n\n-- \nBrandon Williams\n"},{"id":"354187","messageId":"20180801201320.201133-1-jonathantanmy@google.com","threadId":"48971","inReplyTo":"20180729121900.GA16770@sigill.intra.peff.net","subject":"[PATCH] fetch-pack: unify ref in and out param","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-08-01T20:13:20Z","receivedAt":"2018-08-01T20:13:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When a user fetches:\n - at least one up-to-date ref and at least one non-up-to-date ref,\n - using HTTP with protocol v0 (or something else that uses the fetch\n   command of a remote helper)\nsome refs might not be updated after the fetch.\n\nThis bug was introduced in commit 989b8c4452 (\"fetch-pack: put shallow\ninfo in output parameter\", 2018-06-28) which allowed transports to\nreport the refs that they have fetched in a new out-parameter\n\"fetched_refs\". If they do so, transport_fetch_refs() makes this\ninformation available to its caller.\n\nUsers of \"fetched_refs\" rely on the following 3 properties:\n (1) it is the complete list of refs that was passed to\n     transport_fetch_refs(),\n (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if\n     relevant), and\n (3) it has updated OIDs if ref-in-want was used (introduced after\n     989b8c4452).\n\nIn an effort to satisfy (1), whenever transport_fetch_refs()\nfilters the refs sent to the transport, it re-adds the filtered refs to\nwhatever the transport supplies before returning it to the user.\nHowever, the implementation in 989b8c4452 unconditionally re-adds the\nfiltered refs without checking if the transport refrained from reporting\nanything in \"fetched_refs\" (which it is allowed to do), resulting in an\nincomplete list, no longer satisfying (1).\n\nAn earlier effort to resolve this [1] solved the issue by readding the\nfiltered refs only if the transport did not refrain from reporting in\n\"fetched_refs\", but after further discussion, it seems that the better\nsolution is to revert the API change that introduced \"fetched_refs\".\nThis API change was first suggested as part of a ref-in-want\nimplementation that allowed for ref patterns and, thus, there could be\ndrastic differences between the input refs and the refs actually fetched\n[2]; we eventually decided to only allow exact ref names, but this API\nchange remained even though its necessity was decreased.\n\nTherefore, revert this API change by reverting commit 989b8c4452, and\nmake receive_wanted_refs() update the OIDs in the sought array (like how\nupdate_shallow() updates shallow information in the sought array)\ninstead. A test is also included to show that the user-visible bug\ndiscussed at the beginning of this commit message no longer exists.\n\n[1] https://public-inbox.org/git/20180801171806.GA122458@google.com/\n[2] https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathantanmy@google.com/\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nI now think that it's better to revert the API change introducing\n\"fetched_refs\" (or as Peff describes it, \"this whole 'return the fetched\nrefs' scheme from 989b8c4452\"), so here is a patch doing so. I hope to\nhave covered all of Peff's and Junio's questions in the commit message.\n\nAs for Brandon's question:\n\n> I haven't thought too much about what we would need to do in the event\n> we add patterns to ref-in-want, but couldn't we possible mutate the\n> input list again in this case and just simply add the resulting refs to\n> the input list?\n\nIf we support ref patterns, we would need to support deletion of refs,\nnot just addition (because a ref might have existed in the initial ref\nadvertisement, but not when the packfile is delivered). But it should\nbe possible to add a flag stating \"don't use this\" to the ref, and\ndocument that transport_fetch_refs() can append additional refs to the\ntail of the input list. Upon hindsight, maybe this should have been the\noriginal API change instead of the \"fetched_refs\" mechanism.\n---\n builtin/clone.c             |  4 ++--\n builtin/fetch.c             | 28 ++++------------------------\n fetch-object.c              |  2 +-\n fetch-pack.c                | 30 +++++++++++++++---------------\n t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++\n transport-helper.c          |  6 ++----\n transport-internal.h        |  9 +--------\n transport.c                 | 34 ++++++----------------------------\n transport.h                 |  3 +--\n 9 files changed, 50 insertions(+), 84 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 5c439f139..76f7db47e 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1156,7 +1156,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \n \t\tif (!is_local && !complete_refs_before_fetch)\n-\t\t\ttransport_fetch_refs(transport, mapped_refs, NULL);\n+\t\t\ttransport_fetch_refs(transport, mapped_refs);\n \n \t\tremote_head = find_ref_by_name(refs, \"HEAD\");\n \t\tremote_head_points_at =\n@@ -1198,7 +1198,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tif (is_local)\n \t\tclone_local(path, git_dir);\n \telse if (refs && complete_refs_before_fetch)\n-\t\ttransport_fetch_refs(transport, mapped_refs, NULL);\n+\t\ttransport_fetch_refs(transport, mapped_refs);\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t   branch_top.buf, reflog_msg.buf, transport,\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac06f6a57..d136ace87 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -936,13 +936,11 @@ static int quickfetch(struct ref *ref_map)\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n-static int fetch_refs(struct transport *transport, struct ref *ref_map,\n-\t\t      struct ref **updated_remote_refs)\n+static int fetch_refs(struct transport *transport, struct ref *ref_map)\n {\n \tint ret = quickfetch(ref_map);\n \tif (ret)\n-\t\tret = transport_fetch_refs(transport, ref_map,\n-\t\t\t\t\t   updated_remote_refs);\n+\t\tret = transport_fetch_refs(transport, ref_map);\n \tif (!ret)\n \t\t/*\n \t\t * Keep the new pack's \".keep\" file around to allow the caller\n@@ -1107,7 +1105,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tif (!fetch_refs(transport, ref_map, NULL))\n+\tif (!fetch_refs(transport, ref_map))\n \t\tconsume_refs(transport, ref_map);\n \n \tif (gsecondary) {\n@@ -1123,7 +1121,6 @@ static int do_fetch(struct transport *transport,\n \tint autotags = (transport->remote->fetch_tags == 1);\n \tint retcode = 0;\n \tconst struct ref *remote_refs;\n-\tstruct ref *updated_remote_refs = NULL;\n \tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n \tif (tags == TAGS_DEFAULT) {\n@@ -1174,24 +1171,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t   transport->url);\n \t\t}\n \t}\n-\n-\tif (fetch_refs(transport, ref_map, &updated_remote_refs)) {\n-\t\tfree_refs(ref_map);\n-\t\tretcode = 1;\n-\t\tgoto cleanup;\n-\t}\n-\tif (updated_remote_refs) {\n-\t\t/*\n-\t\t * Regenerate ref_map using the updated remote refs.  This is\n-\t\t * to account for additional information which may be provided\n-\t\t * by the transport (e.g. shallow info).\n-\t\t */\n-\t\tfree_refs(ref_map);\n-\t\tref_map = get_ref_map(transport->remote, updated_remote_refs, rs,\n-\t\t\t\t      tags, &autotags);\n-\t\tfree_refs(updated_remote_refs);\n-\t}\n-\tif (consume_refs(transport, ref_map)) {\n+\tif (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {\n \t\tfree_refs(ref_map);\n \t\tretcode = 1;\n \t\tgoto cleanup;\ndiff --git a/fetch-object.c b/fetch-object.c\nindex 48fe63dd6..853624f81 100644\n--- a/fetch-object.c\n+++ b/fetch-object.c\n@@ -19,7 +19,7 @@ static void fetch_refs(const char *remote_name, struct ref *ref)\n \n \ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n \ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n-\ttransport_fetch_refs(transport, ref, NULL);\n+\ttransport_fetch_refs(transport, ref);\n \tfetch_if_missing = original_fetch_if_missing;\n }\n \ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 7ccb9c0d4..b80c13124 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1339,25 +1339,26 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \targs->deepen = 1;\n }\n \n-static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)\n+static void receive_wanted_refs(struct packet_reader *reader,\n+\t\t\t\tstruct ref **sought, int nr_sought)\n {\n \tprocess_section_header(reader, \"wanted-refs\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tstruct object_id oid;\n \t\tconst char *end;\n-\t\tstruct ref *r = NULL;\n+\t\tint i;\n \n \t\tif (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')\n \t\t\tdie(\"expected wanted-ref, got '%s'\", reader->line);\n \n-\t\tfor (r = refs; r; r = r->next) {\n-\t\t\tif (!strcmp(end, r->name)) {\n-\t\t\t\toidcpy(&r->old_oid, &oid);\n+\t\tfor (i = 0; i < nr_sought; i++) {\n+\t\t\tif (!strcmp(end, sought[i]->name)) {\n+\t\t\t\toidcpy(&sought[i]->old_oid, &oid);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\n \n-\t\tif (!r)\n+\t\tif (i == nr_sought)\n \t\t\tdie(\"unexpected wanted-ref: '%s'\", reader->line);\n \t}\n \n@@ -1440,7 +1441,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\treceive_shallow_info(args, &reader);\n \n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n-\t\t\t\treceive_wanted_refs(&reader, ref);\n+\t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n \n \t\t\t/* get the pack */\n \t\t\tprocess_section_header(&reader, \"packfile\", 0);\n@@ -1504,13 +1505,12 @@ static int remove_duplicates_in_refs(struct ref **ref, int nr)\n }\n \n static void update_shallow(struct fetch_pack_args *args,\n-\t\t\t   struct ref *refs,\n+\t\t\t   struct ref **sought, int nr_sought,\n \t\t\t   struct shallow_info *si)\n {\n \tstruct oid_array ref = OID_ARRAY_INIT;\n \tint *status;\n \tint i;\n-\tstruct ref *r;\n \n \tif (args->deepen && alternate_shallow_file) {\n \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n@@ -1552,8 +1552,8 @@ static void update_shallow(struct fetch_pack_args *args,\n \tremove_nonexistent_theirs_shallow(si);\n \tif (!si->nr_ours && !si->nr_theirs)\n \t\treturn;\n-\tfor (r = refs; r; r = r->next)\n-\t\toid_array_append(&ref, &r->old_oid);\n+\tfor (i = 0; i < nr_sought; i++)\n+\t\toid_array_append(&ref, &sought[i]->old_oid);\n \tsi->ref = &ref;\n \n \tif (args->update_shallow) {\n@@ -1587,12 +1587,12 @@ static void update_shallow(struct fetch_pack_args *args,\n \t * remote is also shallow, check what ref is safe to update\n \t * without updating .git/shallow\n \t */\n-\tstatus = xcalloc(ref.nr, sizeof(*status));\n+\tstatus = xcalloc(nr_sought, sizeof(*status));\n \tassign_shallow_commits_to_refs(si, NULL, status);\n \tif (si->nr_ours || si->nr_theirs) {\n-\t\tfor (r = refs, i = 0; r; r = r->next, i++)\n+\t\tfor (i = 0; i < nr_sought; i++)\n \t\t\tif (status[i])\n-\t\t\t\tr->status = REF_STATUS_REJECT_SHALLOW;\n+\t\t\t\tsought[i]->status = REF_STATUS_REJECT_SHALLOW;\n \t}\n \tfree(status);\n \toid_array_clear(&ref);\n@@ -1655,7 +1655,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\targs->connectivity_checked = 1;\n \t}\n \n-\tupdate_shallow(args, ref_cpy, &si);\n+\tupdate_shallow(args, sought, nr_sought, &si);\n cleanup:\n \tclear_shallow_info(&si);\n \treturn ref_cpy;\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex 913089b14..989d034ac 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -369,6 +369,24 @@ test_expect_success 'custom http headers' '\n \t\tsubmodule update sub\n '\n \n+test_expect_success 'using fetch command in remote-curl updates refs' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/twobranch\" &&\n+\trm -rf \"$SERVER\" client &&\n+\n+\tgit init \"$SERVER\" &&\n+\ttest_commit -C \"$SERVER\" foo &&\n+\tgit -C \"$SERVER\" update-ref refs/heads/anotherbranch foo &&\n+\n+\tgit clone $HTTPD_URL/smart/twobranch client &&\n+\n+\ttest_commit -C \"$SERVER\" bar &&\n+\tgit -C client -c protocol.version=0 fetch &&\n+\n+\tgit -C \"$SERVER\" rev-parse master >expect &&\n+\tgit -C client rev-parse origin/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '\n \trm -rf clone &&\n \techo \"Set-Cookie: Foo=1\" >cookies &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 8b5abca29..1f8ff7e94 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -651,16 +651,14 @@ static int connect_helper(struct transport *transport, const char *name,\n }\n \n static int fetch(struct transport *transport,\n-\t\t int nr_heads, struct ref **to_fetch,\n-\t\t struct ref **fetched_refs)\n+\t\t int nr_heads, struct ref **to_fetch)\n {\n \tstruct helper_data *data = transport->data;\n \tint i, count;\n \n \tif (process_connect(transport, 0)) {\n \t\tdo_take_over(transport);\n-\t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch,\n-\t\t\t\t\t\tfetched_refs);\n+\t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n \t}\n \n \tcount = 0;\ndiff --git a/transport-internal.h b/transport-internal.h\nindex eeb6c340e..1cde6258a 100644\n--- a/transport-internal.h\n+++ b/transport-internal.h\n@@ -36,18 +36,11 @@ struct transport_vtable {\n \t * Fetch the objects for the given refs. Note that this gets\n \t * an array, and should ignore the list structure.\n \t *\n-\t * The transport *may* provide, in fetched_refs, the list of refs that\n-\t * it fetched.  If the transport knows anything about the fetched refs\n-\t * that the caller does not know (for example, shallow status), it\n-\t * should provide that list of refs and include that information in the\n-\t * list.\n-\t *\n \t * If the transport did not get hashes for refs in\n \t * get_refs_list(), it should set the old_sha1 fields in the\n \t * provided refs now.\n \t **/\n-\tint (*fetch)(struct transport *transport, int refs_nr, struct ref **refs,\n-\t\t     struct ref **fetched_refs);\n+\tint (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);\n \n \t/**\n \t * Push the objects and refs. Send the necessary objects, and\ndiff --git a/transport.c b/transport.c\nindex fdd813f68..af6e692db 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -151,8 +151,7 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n }\n \n static int fetch_refs_from_bundle(struct transport *transport,\n-\t\t\t       int nr_heads, struct ref **to_fetch,\n-\t\t\t       struct ref **fetched_refs)\n+\t\t\t       int nr_heads, struct ref **to_fetch)\n {\n \tstruct bundle_transport_data *data = transport->data;\n \treturn unbundle(&data->header, data->fd,\n@@ -288,8 +287,7 @@ static struct ref *get_refs_via_connect(struct transport *transport, int for_pus\n }\n \n static int fetch_refs_via_pack(struct transport *transport,\n-\t\t\t       int nr_heads, struct ref **to_fetch,\n-\t\t\t       struct ref **fetched_refs)\n+\t\t\t       int nr_heads, struct ref **to_fetch)\n {\n \tint ret = 0;\n \tstruct git_transport_data *data = transport->data;\n@@ -357,12 +355,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \tif (report_unmatched_refs(to_fetch, nr_heads))\n \t\tret = -1;\n \n-\tif (fetched_refs)\n-\t\t*fetched_refs = refs;\n-\telse\n-\t\tfree_refs(refs);\n-\n \tfree_refs(refs_tmp);\n+\tfree_refs(refs);\n \tfree(dest);\n \treturn ret;\n }\n@@ -1222,31 +1216,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,\n \treturn transport->remote_refs;\n }\n \n-int transport_fetch_refs(struct transport *transport, struct ref *refs,\n-\t\t\t struct ref **fetched_refs)\n+int transport_fetch_refs(struct transport *transport, struct ref *refs)\n {\n \tint rc;\n \tint nr_heads = 0, nr_alloc = 0, nr_refs = 0;\n \tstruct ref **heads = NULL;\n-\tstruct ref *nop_head = NULL, **nop_tail = &nop_head;\n \tstruct ref *rm;\n \n \tfor (rm = refs; rm; rm = rm->next) {\n \t\tnr_refs++;\n \t\tif (rm->peer_ref &&\n \t\t    !is_null_oid(&rm->old_oid) &&\n-\t\t    !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) {\n-\t\t\t/*\n-\t\t\t * These need to be reported as fetched, but we don't\n-\t\t\t * actually need to fetch them.\n-\t\t\t */\n-\t\t\tif (fetched_refs) {\n-\t\t\t\tstruct ref *nop_ref = copy_ref(rm);\n-\t\t\t\t*nop_tail = nop_ref;\n-\t\t\t\tnop_tail = &nop_ref->next;\n-\t\t\t}\n+\t\t    !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid))\n \t\t\tcontinue;\n-\t\t}\n \t\tALLOC_GROW(heads, nr_heads + 1, nr_alloc);\n \t\theads[nr_heads++] = rm;\n \t}\n@@ -1264,11 +1246,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs,\n \t\t\theads[nr_heads++] = rm;\n \t}\n \n-\trc = transport->vtable->fetch(transport, nr_heads, heads, fetched_refs);\n-\tif (fetched_refs && nop_head) {\n-\t\t*nop_tail = *fetched_refs;\n-\t\t*fetched_refs = nop_head;\n-\t}\n+\trc = transport->vtable->fetch(transport, nr_heads, heads);\n \n \tfree(heads);\n \treturn rc;\ndiff --git a/transport.h b/transport.h\nindex 7a9a7fcaf..c057c44d3 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -229,8 +229,7 @@ int transport_push(struct transport *connection,\n const struct ref *transport_get_remote_refs(struct transport *transport,\n \t\t\t\t\t    const struct argv_array *ref_prefixes);\n \n-int transport_fetch_refs(struct transport *transport, struct ref *refs,\n-\t\t\t struct ref **fetched_refs);\n+int transport_fetch_refs(struct transport *transport, struct ref *refs);\n void transport_unlock_pack(struct transport *transport);\n int transport_disconnect(struct transport *transport);\n char *transport_anonymize_url(const char *url);\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354197","messageId":"20180801213826.GA66237@google.com","threadId":"48971","inReplyTo":"20180801201320.201133-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: unify ref in and out param","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-08-01T21:38:26Z","receivedAt":"2018-08-01T21:38:31Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 08/01, Jonathan Tan wrote:\n> When a user fetches:\n>  - at least one up-to-date ref and at least one non-up-to-date ref,\n>  - using HTTP with protocol v0 (or something else that uses the fetch\n>    command of a remote helper)\n> some refs might not be updated after the fetch.\n> \n> This bug was introduced in commit 989b8c4452 (\"fetch-pack: put shallow\n> info in output parameter\", 2018-06-28) which allowed transports to\n> report the refs that they have fetched in a new out-parameter\n> \"fetched_refs\". If they do so, transport_fetch_refs() makes this\n> information available to its caller.\n> \n> Users of \"fetched_refs\" rely on the following 3 properties:\n>  (1) it is the complete list of refs that was passed to\n>      transport_fetch_refs(),\n>  (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if\n>      relevant), and\n>  (3) it has updated OIDs if ref-in-want was used (introduced after\n>      989b8c4452).\n> \n> In an effort to satisfy (1), whenever transport_fetch_refs()\n> filters the refs sent to the transport, it re-adds the filtered refs to\n> whatever the transport supplies before returning it to the user.\n> However, the implementation in 989b8c4452 unconditionally re-adds the\n> filtered refs without checking if the transport refrained from reporting\n> anything in \"fetched_refs\" (which it is allowed to do), resulting in an\n> incomplete list, no longer satisfying (1).\n> \n> An earlier effort to resolve this [1] solved the issue by readding the\n> filtered refs only if the transport did not refrain from reporting in\n> \"fetched_refs\", but after further discussion, it seems that the better\n> solution is to revert the API change that introduced \"fetched_refs\".\n> This API change was first suggested as part of a ref-in-want\n> implementation that allowed for ref patterns and, thus, there could be\n> drastic differences between the input refs and the refs actually fetched\n> [2]; we eventually decided to only allow exact ref names, but this API\n> change remained even though its necessity was decreased.\n> \n> Therefore, revert this API change by reverting commit 989b8c4452, and\n> make receive_wanted_refs() update the OIDs in the sought array (like how\n> update_shallow() updates shallow information in the sought array)\n> instead. A test is also included to show that the user-visible bug\n> discussed at the beginning of this commit message no longer exists.\n> \n> [1] https://public-inbox.org/git/20180801171806.GA122458@google.com/\n> [2] https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathantanmy@google.com/\n> \n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> I now think that it's better to revert the API change introducing\n> \"fetched_refs\" (or as Peff describes it, \"this whole 'return the fetched\n> refs' scheme from 989b8c4452\"), so here is a patch doing so. I hope to\n> have covered all of Peff's and Junio's questions in the commit message.\n> \n> As for Brandon's question:\n> \n> > I haven't thought too much about what we would need to do in the event\n> > we add patterns to ref-in-want, but couldn't we possible mutate the\n> > input list again in this case and just simply add the resulting refs to\n> > the input list?\n> \n> If we support ref patterns, we would need to support deletion of refs,\n> not just addition (because a ref might have existed in the initial ref\n> advertisement, but not when the packfile is delivered). But it should\n> be possible to add a flag stating \"don't use this\" to the ref, and\n> document that transport_fetch_refs() can append additional refs to the\n> tail of the input list. Upon hindsight, maybe this should have been the\n> original API change instead of the \"fetched_refs\" mechanism.\n\nThanks for getting this out, it looks good to me.  If we end up adding\npatterns to ref-in-want then we can explore what changes would need to\nbe made then, I expect we may need to do a bit more work on the whole\nfetching stack to get what we'd want in that case (because we would want\nto avoid this issue again).\n\n-- \nBrandon Williams\n"},{"id":"354201","messageId":"xmqqk1p9681m.fsf@gitster-ct.c.googlers.com","threadId":"48971","inReplyTo":"20180801213826.GA66237@google.com","subject":"Re: [PATCH] fetch-pack: unify ref in and out param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-01T22:23:01Z","receivedAt":"2018-08-01T22:23:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams <bmwill@google.com> writes:\n\n> ..., I expect we may need to do a bit more work on the whole\n> fetching stack to get what we'd want in that case (because we would want\n> to avoid this issue again).\n\nAmen.  Thanks all.\n"},{"id":"354272","messageId":"20180802163007.GA15984@sigill.intra.peff.net","threadId":"48971","inReplyTo":"20180731232343.184463-1-jonathantanmy@google.com","subject":"Re: [PATCH] transport: report refs only if transport does","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T16:30:08Z","receivedAt":"2018-08-02T16:30:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 31, 2018 at 04:23:43PM -0700, Jonathan Tan wrote:\n\n> > > Because transport_fetch_refs() filters the refs sent to the transport,\n> > > it cannot just report the transport's result directly, but first needs\n> > > to readd the excluded refs, pretending that they are fetched. However,\n> > > this results in a wrong result if the transport did not report the refs\n> > > that they have fetched in \"fetched_refs\" - the excluded refs would be\n> > > added and reported, presenting an incomplete picture to the caller.\n> > \n> > This part leaves me confused. If we are not fetching them, then why do\n> > we need to pretend that they are fetched?\n> \n> The short answer is that we need:\n>  (1) the complete list of refs that was passed to\n>      transport_fetch_refs(),\n>  (2) with shallow information (REF_STATUS_REJECT_SHALLOW set if\n>      relevant), and\n>  (3) with updated OIDs if ref-in-want was used.\n> \n> The fetched_refs out param already fulfils (2) and (3), and this patch\n> makes it fulfil (1). As for calling them fetched_refs, perhaps that is a\n> misnomer, but they do appear in FETCH_HEAD even though they are not\n> truly fetched.\n\nThanks for this explanation. It does make more sense to me now, and I\nagree that a lot of my confusion was from calling it \"fetched_refs\" (and\nthe comment saying \"reported as fetched, but not actually fetched\").\n\n> Which raises the question...if completeness is so important, why not\n> reuse the input list of refs and document that transport_fetch_refs()\n> can mutate the input list? You ask the same question below, so I'll put\n> the answer after quoting your paragraph.\n> [...]\n\nThanks, the answer here was enlightening as well.\n\nI see you posted a patch to go back to mutating the list, and that seems\nreasonable to me. I'm fine with a separate \"out\" list, too. Its purpose\nand expectations just need to be reflected in the name (and possibly in\na comment).\n\n-Peff\n"},{"id":"354273","messageId":"20180802164026.GB15984@sigill.intra.peff.net","threadId":"48971","inReplyTo":"20180801201320.201133-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: unify ref in and out param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-02T16:40:26Z","receivedAt":"2018-08-02T16:40:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 01, 2018 at 01:13:20PM -0700, Jonathan Tan wrote:\n\n> When a user fetches:\n>  - at least one up-to-date ref and at least one non-up-to-date ref,\n>  - using HTTP with protocol v0 (or something else that uses the fetch\n>    command of a remote helper)\n> some refs might not be updated after the fetch.\n> \n> This bug was introduced in commit 989b8c4452 (\"fetch-pack: put shallow\n> info in output parameter\", 2018-06-28) which allowed transports to\n> report the refs that they have fetched in a new out-parameter\n> \"fetched_refs\". If they do so, transport_fetch_refs() makes this\n> information available to its caller.\n> \n> Users of \"fetched_refs\" rely on the following 3 properties:\n>  (1) it is the complete list of refs that was passed to\n>      transport_fetch_refs(),\n>  (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if\n>      relevant), and\n>  (3) it has updated OIDs if ref-in-want was used (introduced after\n>      989b8c4452).\n> [...]\n\nThanks, this is a very clear and well-organized commit message. It\nanswers my questions, and I agree with the general notion of \"we can\nfigure out the right API for ref patterns later\" approach.\n\n>  builtin/clone.c             |  4 ++--\n>  builtin/fetch.c             | 28 ++++------------------------\n>  fetch-object.c              |  2 +-\n>  fetch-pack.c                | 30 +++++++++++++++---------------\n>  t/t5551-http-fetch-smart.sh | 18 ++++++++++++++++++\n>  transport-helper.c          |  6 ++----\n>  transport-internal.h        |  9 +--------\n>  transport.c                 | 34 ++++++----------------------------\n>  transport.h                 |  3 +--\n\nThe patch itself looks sane to me, and obviously fixes the problem. I\ncannot offhand think of any reason that munging the existing list would\nbe a problem (though it has been a while since I have dealt with this\ncode, so take that with the appropriate grain of salt).\n\n-Peff\n"}]}