{"thread":{"id":"53920","subject":"[RFC PATCH] Modify fetch-pack to no longer die on error?","startedAt":"2020-07-24T22:38:51Z","lastAt":"2020-08-18T23:57:30Z","messageCount":57,"participants":["Jonathan Tan","Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402029","messageId":"20200724223844.2723397-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":null,"subject":"[RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-24T22:38:44Z","receivedAt":"2020-07-24T22:38:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"We've had a few instances where a lazy fetch in a partial clone fails,\nleading to a fatal error, when the calling code could have easily\nrecovered - in other words, the severity of the bug should have just a\nwasted fetch instead of stopping the whole command. Part of the issue\n(and possibly the whole issue - I haven't looked at this beyond\nfetch-pack yet) is that fetch-pack dies whenever it encounters an\nerror, so I took a look at fixing that.\n\n(Note that fetch-pack is sometimes run through a remote helper, meaning\nthat we could leave the die() invocations in and just make sure that we\nhandle failure in the separate process correctly. But when the promisor\nremote is HTTP protocol v2 or SSH protocol v0/v2, this is not true -\nfetch_pack() is run in-process.)\n\nI think the best way for easy authorship and review is to convert each\npossibly-dying function into a function that either returns a\npossibly-null error string or returns success/failure and writes the\nerror string into an \"out\" parameter. In this way, the change is rather\nmechanical and should be easy to review. In the patch below I chose the\nformer approach, and I modified 2 functions (one that returns no value\nand one that returns a value) to demonstrate what it would look like.\n\nWe could also take this further and have a \"struct error\" for type\nsafety and macros - e.g. THROW() to return a \"struct error\", TRY() to\nexecute what's inside the parentheses and return the error if there is\none, and OR_DIE() to execute what's inside the parentheses and die if\nthere is an error.\n\nAny opinions before I continue working on this?\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 78 +++++++++++++++++++++++++++++++++++-----------------\n 1 file changed, 53 insertions(+), 25 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 80fb3bd899..20a7e05ea8 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -24,6 +24,8 @@\n #include \"fsck.h\"\n #include \"shallow.h\"\n \n+typedef char * error_string;\n+\n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n static int unpack_limit = 100;\n@@ -136,8 +138,8 @@ enum ack_type {\n \tACK_ready\n };\n \n-static void consume_shallow_list(struct fetch_pack_args *args,\n-\t\t\t\t struct packet_reader *reader)\n+static error_string consume_shallow_list(struct fetch_pack_args *args,\n+\t\t\t\t\t struct packet_reader *reader)\n {\n \tif (args->stateless_rpc && args->deepen) {\n \t\t/* If we sent a depth we will get back \"duplicate\"\n@@ -149,41 +151,54 @@ static void consume_shallow_list(struct fetch_pack_args *args,\n \t\t\t\tcontinue;\n \t\t\tif (starts_with(reader->line, \"unshallow \"))\n \t\t\t\tcontinue;\n-\t\t\tdie(_(\"git fetch-pack: expected shallow list\"));\n+\t\t\treturn xstrdup(_(\"git fetch-pack: expected shallow list\"));\n \t\t}\n \t\tif (reader->status != PACKET_READ_FLUSH)\n-\t\t\tdie(_(\"git fetch-pack: expected a flush packet after shallow list\"));\n+\t\t\treturn xstrdup(_(\"git fetch-pack: expected a flush packet after shallow list\"));\n \t}\n+\treturn NULL;\n }\n \n-static enum ack_type get_ack(struct packet_reader *reader,\n-\t\t\t struct object_id *result_oid)\n+static error_string get_ack(struct packet_reader *reader,\n+\t\t\t enum ack_type *result_ack,\n+\t\t\t struct object_id *result_oid)\n {\n \tint len;\n \tconst char *arg;\n \n \tif (packet_reader_read(reader) != PACKET_READ_NORMAL)\n-\t\tdie(_(\"git fetch-pack: expected ACK/NAK, got a flush packet\"));\n+\t\treturn xstrdup(_(\"git fetch-pack: expected ACK/NAK, got a flush packet\"));\n \tlen = reader->pktlen;\n \n-\tif (!strcmp(reader->line, \"NAK\"))\n-\t\treturn NAK;\n+\tif (!strcmp(reader->line, \"NAK\")) {\n+\t\t*result_ack = NAK;\n+\t\treturn NULL;\n+\t}\n \tif (skip_prefix(reader->line, \"ACK \", &arg)) {\n \t\tconst char *p;\n \t\tif (!parse_oid_hex(arg, result_oid, &p)) {\n \t\t\tlen -= p - reader->line;\n-\t\t\tif (len < 1)\n-\t\t\t\treturn ACK;\n-\t\t\tif (strstr(p, \"continue\"))\n-\t\t\t\treturn ACK_continue;\n-\t\t\tif (strstr(p, \"common\"))\n-\t\t\t\treturn ACK_common;\n-\t\t\tif (strstr(p, \"ready\"))\n-\t\t\t\treturn ACK_ready;\n-\t\t\treturn ACK;\n+\t\t\tif (len < 1) {\n+\t\t\t\t*result_ack = ACK;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\tif (strstr(p, \"continue\")) {\n+\t\t\t\t*result_ack = ACK_continue;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\tif (strstr(p, \"common\")) {\n+\t\t\t\t*result_ack = ACK_common;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\tif (strstr(p, \"ready\")) {\n+\t\t\t\t*result_ack = ACK_ready;\n+\t\t\t\treturn NULL;\n+\t\t\t}\n+\t\t\t*result_ack = ACK;\n+\t\t\treturn NULL;\n \t\t}\n \t}\n-\tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), reader->line);\n+\treturn xstrfmt(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), reader->line);\n }\n \n static void send_request(struct fetch_pack_args *args,\n@@ -394,7 +409,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\tprint_verbose(args, \"have %s\", oid_to_hex(oid));\n \t\tin_vain++;\n \t\tif (flush_at <= ++count) {\n-\t\t\tint ack;\n+\t\t\tenum ack_type ack;\n+\t\t\terror_string err;\n \n \t\t\tpacket_buf_flush(&req_buf);\n \t\t\tsend_request(args, fd[1], &req_buf);\n@@ -409,9 +425,11 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\tif (!args->stateless_rpc && count == INITIAL_FLUSH)\n \t\t\t\tcontinue;\n \n-\t\t\tconsume_shallow_list(args, &reader);\n+\t\t\tif ((err = consume_shallow_list(args, &reader)))\n+\t\t\t\tdie(\"%s\", err);\n \t\t\tdo {\n-\t\t\t\tack = get_ack(&reader, result_oid);\n+\t\t\t\tif ((err = get_ack(&reader, &ack, result_oid)))\n+\t\t\t\t\tdie(\"%s\", err);\n \t\t\t\tif (ack)\n \t\t\t\t\tprint_verbose(args, _(\"got %s %d %s\"), \"ack\",\n \t\t\t\t\t\t ack, oid_to_hex(result_oid));\n@@ -457,6 +475,9 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\t\t\t\tgot_ready = 1;\n \t\t\t\t\tbreak;\n \t\t\t\t\t}\n+\t\t\t\tcase NAK:\n+\t\t\t\t\t/* nothing */\n+\t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t} while (ack);\n \t\t\tflushes--;\n@@ -481,10 +502,17 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t}\n \tstrbuf_release(&req_buf);\n \n-\tif (!got_ready || !no_done)\n-\t\tconsume_shallow_list(args, &reader);\n+\tif (!got_ready || !no_done) {\n+\t\terror_string err;\n+\t\tif ((err = consume_shallow_list(args, &reader)))\n+\t\t\tdie(\"%s\", err);\n+\t}\n \twhile (flushes || multi_ack) {\n-\t\tint ack = get_ack(&reader, result_oid);\n+\t\terror_string err;\n+\t\tenum ack_type ack;\n+\n+\t\tif ((err = get_ack(&reader, &ack, result_oid)))\n+\t\t\tdie(\"%s\", err);\n \t\tif (ack) {\n \t\t\tprint_verbose(args, _(\"got %s (%d) %s\"), \"ack\",\n \t\t\t\t ack, oid_to_hex(result_oid));\n-- \n2.28.0.rc0.142.g3c755180ce-goog\n\n"},{"id":"402031","messageId":"xmqqblk4jyvc.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"20200724223844.2723397-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-24T23:07:35Z","receivedAt":"2020-07-24T23:07:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> We've had a few instances where a lazy fetch in a partial clone fails,\n> leading to a fatal error, when the calling code could have easily\n> recovered - in other words, the severity of the bug should have just a\n> wasted fetch instead of stopping the whole command. Part of the issue\n> (and possibly the whole issue - I haven't looked at this beyond\n> fetch-pack yet) is that fetch-pack dies whenever it encounters an\n> error, so I took a look at fixing that.\n>\n> (Note that fetch-pack is sometimes run through a remote helper, meaning\n> that we could leave the die() invocations in and just make sure that we\n> handle failure in the separate process correctly. But when the promisor\n> remote is HTTP protocol v2 or SSH protocol v0/v2, this is not true -\n> fetch_pack() is run in-process.)\n\nThat is easily fixable ;-) After all, we are talking about network\nlatency once we \"lazily fetch\" during a main operation, so why not\ntake advantage of the isolation that the operating system gives us?\n\n> I think the best way for easy authorship and review is to convert each\n> possibly-dying function into a function that either returns a\n> possibly-null error string or returns success/failure and writes the\n> error string into an \"out\" parameter.\n\nUgly.\n"},{"id":"402032","messageId":"xmqq7dusjyos.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"20200724223844.2723397-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-24T23:11:31Z","receivedAt":"2020-07-24T23:11:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> I think the best way for easy authorship and review is to convert each\n> possibly-dying function into a function that either returns a\n> possibly-null error string or returns success/failure and writes the\n> error string into an \"out\" parameter.\n\nUgly.\n\nRather, if we were to pass an extra parameter through the entire\ncallchain, I'd rather see that parameter to be what to do when we\nsee an error. Depending on that, the callee can die(), return\nerror(), or silently return -1, or do something else.\n\nI really do no think it is a good idea to pass text string back to\nthe caller, which invites sentence lego and untranslatable mess.\n"},{"id":"402047","messageId":"20200725214118.GA4508@coredump.intra.peff.net","threadId":"53920","inReplyTo":"xmqq7dusjyos.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-25T21:41:18Z","receivedAt":"2020-07-25T21:41:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 24, 2020 at 04:11:31PM -0700, Junio C Hamano wrote:\n\n> Jonathan Tan writes:\n> \n> > I think the best way for easy authorship and review is to convert each\n> > possibly-dying function into a function that either returns a\n> > possibly-null error string or returns success/failure and writes the\n> > error string into an \"out\" parameter.\n> \n> Ugly.\n> \n> Rather, if we were to pass an extra parameter through the entire\n> callchain, I'd rather see that parameter to be what to do when we\n> see an error. Depending on that, the callee can die(), return\n> error(), or silently return -1, or do something else.\n\nAgreed. I had a proposal a while ago to pass a \"struct error_context\"\nwhich was basically a function pointer and any accompanying data. That\nlets the caller instruct us to die(), call error(), stuff the error in a\nstrbuf owned by the caller, and so on.\n\nI think it was this one:\n\n https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n\nI still think it's a good idea, but the hard part is lib-ifying all of\nthe functions that call die() to instead return an error up the stack\n(and handle cleanup, etc). Which is why I never really pushed it\nfurther. But if we're going to be lib-ifying such calls anyway, I think\nit's nice to do this flexible thing (from their perspective it's no more\nwork to trigger the callback than it is to call error() and return).\n\nThat said, I do think for this particular case, your \"just run it in a\nsub-process\" suggestion may be simpler and more robust.\n\n-Peff\n"},{"id":"402048","messageId":"xmqqft9fi4hm.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"20200725214118.GA4508@coredump.intra.peff.net","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-25T23:01:25Z","receivedAt":"2020-07-25T23:01:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think it was this one:\n>\n> https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n>\n> I still think it's a good idea, but the hard part is lib-ifying all of\n> the functions that call die() to instead return an error up the stack\n> (and handle cleanup, etc). Which is why I never really pushed it\n> further. But if we're going to be lib-ifying such calls anyway, I think\n> it's nice to do this flexible thing (from their perspective it's no more\n> work to trigger the callback than it is to call error() and return).\n\nYeah, I almost 80%+ agree.\n\nThe remainder of 20% is that I am not so convinced that (fmt_string\n+ va_list) that isn't pretty much usable for anything but generating\nhuman readable error messages is enough. It is certainly a good\nenough replacement for (and much better than) the approach to\nprepare an error string in a \"struct strbuf err\" that was passeed by\nthe caller, but I am not sure if that is a good bar to aim to pass\nto begin with ;-).\n\n> That said, I do think for this particular case, your \"just run it in a\n> sub-process\" suggestion may be simpler and more robust.\n\nFor this particular case, with the performance and all, I agree that\nthe stupid and robust approach would be the best.\n\n"},{"id":"402146","messageId":"20200727171143.GA789310@coredump.intra.peff.net","threadId":"53920","inReplyTo":"xmqqft9fi4hm.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-27T17:11:43Z","receivedAt":"2020-07-27T17:11:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 25, 2020 at 04:01:25PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think it was this one:\n> >\n> > https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n> >\n> > I still think it's a good idea, but the hard part is lib-ifying all of\n> > the functions that call die() to instead return an error up the stack\n> > (and handle cleanup, etc). Which is why I never really pushed it\n> > further. But if we're going to be lib-ifying such calls anyway, I think\n> > it's nice to do this flexible thing (from their perspective it's no more\n> > work to trigger the callback than it is to call error() and return).\n> \n> Yeah, I almost 80%+ agree.\n> \n> The remainder of 20% is that I am not so convinced that (fmt_string\n> + va_list) that isn't pretty much usable for anything but generating\n> human readable error messages is enough. It is certainly a good\n> enough replacement for (and much better than) the approach to\n> prepare an error string in a \"struct strbuf err\" that was passeed by\n> the caller, but I am not sure if that is a good bar to aim to pass\n> to begin with ;-).\n\nYeah, I certainly agree that just passing around strings does not\nprovide as much information as some callers would like. But I think\nintroducing a system of machine-readable error tokens would be\ncumbersome for a lot of cases, to the point that it may tip this from\n\"seems like an easy enough change\" to something that nobody wants to\ntake on.\n\nOne in-between step that would be pretty easy is to record errno when\navailable (i.e., if error_errno() becomes error_ctx_errno(), then we\njust stuff that errno value into the error_context struct). That would\nlet us reliably propagate errno up the stack, or even give intermediate\ncallers the opportunity to munge it (e.g., if a ref lookup failed with\nEISDIR while opening a file, it might convert that to ENOENT as it\npropagates up the stack).\n\n-Peff\n"},{"id":"402227","messageId":"20200728192350.352978-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqft9fi4hm.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-28T19:23:50Z","receivedAt":"2020-07-28T19:23:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jeff King writes:\n> \n> > I think it was this one:\n> >\n> > https://lore.kernel.org/git/20160927191955.mympqgylrxhkp24n@sigill.intra.peff.net/\n> >\n> > I still think it's a good idea, but the hard part is lib-ifying all of\n> > the functions that call die() to instead return an error up the stack\n> > (and handle cleanup, etc). Which is why I never really pushed it\n> > further. But if we're going to be lib-ifying such calls anyway, I think\n> > it's nice to do this flexible thing (from their perspective it's no more\n> > work to trigger the callback than it is to call error() and return).\n> \n> Yeah, I almost 80%+ agree.\n\nThanks Peff for the pointer. That looks like a good idea, and more\ncomprehensive than my suggested approach.\n\n> The remainder of 20% is that I am not so convinced that (fmt_string\n> + va_list) that isn't pretty much usable for anything but generating\n> human readable error messages is enough. It is certainly a good\n> enough replacement for (and much better than) the approach to\n> prepare an error string in a \"struct strbuf err\" that was passeed by\n> the caller, but I am not sure if that is a good bar to aim to pass\n> to begin with ;-).\n\nI think that functions that inform their callers about different errors\nalready do so through return values, but in any case I think that Peff's\nidea can be extended once we need it (e.g. by adding error reporting\nfunctions that can fallback to the string function if the special\nfunction is not present).\n\n> > That said, I do think for this particular case, your \"just run it in a\n> > sub-process\" suggestion may be simpler and more robust.\n> \n> For this particular case, with the performance and all, I agree that\n> the stupid and robust approach would be the best.\n\nI'm concerned that we will be painting ourselves into a corner here - I\nhave been appreciating the richer interface that a C call provides us,\ncompared to sub-processes where we have to communicate through a single\ninput stream and a single output stream. For example, \"wanted-refs\" and\npackfile URIs (a.k.a. CDN offloading) were made much simpler because it\nwas quite easy to communicate back the updated hashes for refs (for\n\"wanted-refs\") and more than one lockfile (for packfile URIs). If we\nwere doing it with \"fetch\" in remote helpers (like in HTTP protocol v0),\nwe would have to augment the textual protocol to communicate back the\nupdated hashes and the list of lockfiles - doable, but more cumbersome.\n(That is also why I only implemented those for protocol v2, because\nprotocol v2 had \"stateless-connect\" for HTTP.)\n\nBeing limited to sub-processes also stopped me from implementing an\nimprovement to how fetch-pack calls index-pack - when I added debugging\ninformation (list of refs and hashes) when writing the .promisor file,\nwhat I wanted to do was to pass a callback (function pointer + data\npointer) to index-pack to tell it what to write to .promisor, but I\ncouldn't do that. I also couldn't write the list to the stdin of\nindex-pack, because the stdin was already used to communicate the\npackfile. I could write the list to a temporary file and pass that name\nto index-pack, but by that stage I thought it would be simpler to just\nhave fetch-pack write the .promisor file itself, even though I think\nthat this should be an index-pack concern, not a fetch-pack one.\n\n(Also, I think that debugging within a process is easier than debugging\nacross processes, but that might not be a concern that other people\nshare.)\n\nIn this particular case, when doing the lazy fetch, we pass a special\nflag that skips some steps - among other things, negotiation. Admittedly\nwe could add this flag to \"fetch\" (or whatever command we're going to\ninvoke to implement this sub-process part), or add the equivalent\ngeneral-purpose flags/configs (e.g. fetch.negotiationAlgorithm=null)\n(but I haven't looked thoroughly at what flags would be needed). These\nseem surmountable and I can't think of anything unsurmountable, but I'm\nstill worried that we're painting ourselves into a corner.\n"},{"id":"402228","messageId":"20200728200847.GA1019822@coredump.intra.peff.net","threadId":"53920","inReplyTo":"20200728192350.352978-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-28T20:08:47Z","receivedAt":"2020-07-28T20:08:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 28, 2020 at 12:23:50PM -0700, Jonathan Tan wrote:\n\n> > For this particular case, with the performance and all, I agree that\n> > the stupid and robust approach would be the best.\n> \n> I'm concerned that we will be painting ourselves into a corner here - I\n> have been appreciating the richer interface that a C call provides us,\n> compared to sub-processes where we have to communicate through a single\n> input stream and a single output stream. For example, \"wanted-refs\" and\n> [...]\n\nYeah, that's a compelling reason. I'd have thought for this use case you\ncould just say \"hey, make sure these objects exist\", which doesn't\nrequire a lot of communication. But often when I think things like that\nand start coding, it turns out to be much more complicated. So I am\nperfectly willing to believe you that it doesn't apply here. And even\nif it did, you're right that we may run into other spots that do need to\npass back more information, but benefit from more lib-ified code that\ndoesn't die().\n\nJust to play devil's advocate for a moment...\n\n> (Also, I think that debugging within a process is easier than debugging\n> across processes, but that might not be a concern that other people\n> share.)\n\nThis is definitely true sometimes, but I think is sometimes the\nopposite. When we push things out to a sub-process, then the interface\nbetween the two processes has to be well-defined (e.g., writing results\nto a file with a particular format). And that can make debugging easier,\nbecause you can pick up from that intermediate state (munging it in the\nmiddle, or even generating it from scratch for testing).\n\nLikewise, that can result in a more flexible and robust system from the\nperspective of users. If we had invented \"git log\" first, we probably\nwouldn't have \"git rev-list | git diff-tree --stdin\" at all. But having\nthat as two separate tools is sometimes useful for people doing things\n_besides_ log, since it gives different entry points to the code.\n\nThat said, I think I could buy the argument that \"fetch\" works pretty\nwell as a basic building block for users. It's pretty rare to actually\nuse fetch-pack as a distinct operation. This is all a monolith vs module\ntradeoff question, and the tradeoff around modularity for fetch isn't\nthat compelling.\n\n-Peff\n"},{"id":"402349","messageId":"20200729185347.653706-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"20200728200847.GA1019822@coredump.intra.peff.net","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-29T18:53:47Z","receivedAt":"2020-07-29T18:53:54Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Tue, Jul 28, 2020 at 12:23:50PM -0700, Jonathan Tan wrote:\n> \n> > > For this particular case, with the performance and all, I agree that\n> > > the stupid and robust approach would be the best.\n> > \n> > I'm concerned that we will be painting ourselves into a corner here - I\n> > have been appreciating the richer interface that a C call provides us,\n> > compared to sub-processes where we have to communicate through a single\n> > input stream and a single output stream. For example, \"wanted-refs\" and\n> > [...]\n> \n> Yeah, that's a compelling reason. I'd have thought for this use case you\n> could just say \"hey, make sure these objects exist\", which doesn't\n> require a lot of communication. But often when I think things like that\n> and start coding, it turns out to be much more complicated. So I am\n> perfectly willing to believe you that it doesn't apply here. And even\n> if it did, you're right that we may run into other spots that do need to\n> pass back more information, but benefit from more lib-ified code that\n> doesn't die().\n\nThanks. Just to be clear, I can't think of any hard stops to\nimplementing lazy fetch in a sub-process right now, but (as you said)\nwhile programming I could discover something, or we could need something\nin the future.\n\n> Just to play devil's advocate for a moment...\n> \n> > (Also, I think that debugging within a process is easier than debugging\n> > across processes, but that might not be a concern that other people\n> > share.)\n> \n> This is definitely true sometimes, but I think is sometimes the\n> opposite. When we push things out to a sub-process, then the interface\n> between the two processes has to be well-defined (e.g., writing results\n> to a file with a particular format). And that can make debugging easier,\n> because you can pick up from that intermediate state (munging it in the\n> middle, or even generating it from scratch for testing).\n\nWell, unless there is some sort of interactivity like in remote helpers\n:-)\n\n> Likewise, that can result in a more flexible and robust system from the\n> perspective of users. If we had invented \"git log\" first, we probably\n> wouldn't have \"git rev-list | git diff-tree --stdin\" at all. But having\n> that as two separate tools is sometimes useful for people doing things\n> _besides_ log, since it gives different entry points to the code.\n\nThat's true. And the lazy fetch might be one of them - after looking at\nthe code, I think that I can get to where I want just by implementing a\nnull negotiator, which will be useful for end users. (In particular,\nsimulating a lazy fetch might be useful sometimes, and re-fetching a\npackfile could be a crude way of repairing object corruption.)\n\n> That said, I think I could buy the argument that \"fetch\" works pretty\n> well as a basic building block for users. It's pretty rare to actually\n> use fetch-pack as a distinct operation. This is all a monolith vs module\n> tradeoff question, and the tradeoff around modularity for fetch isn't\n> that compelling.\n\nIf we are going the sub-process route, I was planning to use \"fetch\" as\nthe sub-process, actually, not \"fetch-pack\" - among other things,\n\"fetch\" allows us to specify a fetch negotiator. So it seems like you\nare saying that if we had to use \"fetch-pack\", you have no problem with\nlibifying it and calling it in the same process, but if we can use\n\"fetch\", we should use it as a sub-process?\n\nIn any case, I'll look into using \"fetch\" as a sub-process and see if it\nworks.\n"},{"id":"402350","messageId":"xmqqr1su9mbf.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"20200728200847.GA1019822@coredump.intra.peff.net","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-29T19:02:28Z","receivedAt":"2020-07-29T19:02:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Just to play devil's advocate for a moment...\n>\n>> (Also, I think that debugging within a process is easier than debugging\n>> across processes, but that might not be a concern that other people\n>> share.)\n>\n> This is definitely true sometimes, but I think is sometimes the\n> opposite. When we push things out to a sub-process, then the interface\n> between the two processes has to be well-defined (e.g., writing results\n> to a file with a particular format).\n\nI agree that it forces us to be more disciplined to separate them\ninto two processes and clearly define the way they communicate with\neach other. When done poorly, it tends to make a bigger mess.\n\n> That said, I think I could buy the argument that \"fetch\" works pretty\n> well as a basic building block for users. It's pretty rare to actually\n> use fetch-pack as a distinct operation. This is all a monolith vs module\n> tradeoff question, and the tradeoff around modularity for fetch isn't\n> that compelling.\n\nYup. \n\nThe \"more naive and stupid way to run them as subprocesses, just\nlike we do for cURL based transport\" was primarily because I thought\nit would be easier to get right (as the transport API and interface\nhas long been established) than hunting down all the callers of\ndie() and touch all the functions involved in the callchain. I\nwon't complain or block if a harder-but-more-correct approach is\ntaken ;-)\n"},{"id":"402352","messageId":"20200729192915.GA2351724@coredump.intra.peff.net","threadId":"53920","inReplyTo":"20200729185347.653706-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-29T19:29:15Z","receivedAt":"2020-07-29T19:29:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 29, 2020 at 11:53:47AM -0700, Jonathan Tan wrote:\n\n> > This is definitely true sometimes, but I think is sometimes the\n> > opposite. When we push things out to a sub-process, then the interface\n> > between the two processes has to be well-defined (e.g., writing results\n> > to a file with a particular format). And that can make debugging easier,\n> > because you can pick up from that intermediate state (munging it in the\n> > middle, or even generating it from scratch for testing).\n> \n> Well, unless there is some sort of interactivity like in remote helpers\n> :-)\n\nYes, debugging remote helpers is its own special hell. :)\n\n> > That said, I think I could buy the argument that \"fetch\" works pretty\n> > well as a basic building block for users. It's pretty rare to actually\n> > use fetch-pack as a distinct operation. This is all a monolith vs module\n> > tradeoff question, and the tradeoff around modularity for fetch isn't\n> > that compelling.\n> \n> If we are going the sub-process route, I was planning to use \"fetch\" as\n> the sub-process, actually, not \"fetch-pack\" - among other things,\n> \"fetch\" allows us to specify a fetch negotiator. So it seems like you\n> are saying that if we had to use \"fetch-pack\", you have no problem with\n> libifying it and calling it in the same process, but if we can use\n> \"fetch\", we should use it as a sub-process?\n\nNo, I just meant that in general fetching is a monolithic operation from\nthe users perspective, and we don't often need to break it down further.\nSo if you have to build more options into \"fetch\" (that _could_ have\nbeen broken out into separate sub-commands) I don't think anybody will\nbe sad.\n\nI guess that is kind of orthogonal to your original problem, though,\nwhich is \"should fetch be triggered in-process by other processes\". So\none could still make the argument that whatever features are needed by\nthat calling code (e.g., \"use a different negotiation scheme\") might\nalso be needed by other (script) callers of git-fetch, so there may be\nsome benefit to users in making the cross-process interface more rich\n(of course in the case of negotiation schemes, it is not \"make it more\nrich now\" but \"earlier efforts to make it rich are now paying off\").\n\n-Peff\n"},{"id":"402398","messageId":"20200729225518.700585-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqr1su9mbf.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH] Modify fetch-pack to no longer die on error?","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-29T22:55:18Z","receivedAt":"2020-07-29T22:55:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> The \"more naive and stupid way to run them as subprocesses, just\n> like we do for cURL based transport\" was primarily because I thought\n> it would be easier to get right (as the transport API and interface\n> has long been established) than hunting down all the callers of\n> die() and touch all the functions involved in the callchain. I\n> won't complain or block if a harder-but-more-correct approach is\n> taken ;-)\n\nWell, I was prepared to do the harder approach (as you can see from my\nfirst email) but it will be harder not just for the author but also for\nthe reviewer and future readers of the code; now I think that I should\ntry the subprocess way first, especially since there is another part of\nGit that will do that too (the maintenance prefetch [1]). I'll try that\nand write back about how it goes.\n\n[1] https://lore.kernel.org/git/3165b8916d2d80bf72dac6596a42c871ccd4cbe6.1595527000.git.gitgitgadget@gmail.com/\n"},{"id":"402882","messageId":"cover.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"20200724223844.2723397-1-jonathantanmy@google.com","subject":"[RFC PATCH 0/7] Lazy fetch with subprocess","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:43Z","receivedAt":"2020-08-05T01:21:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here's some code that performs the lazy fetch in a subprocess. It was\nmore involved than I thought it would be, but here it is.\n\nMy main concern is the user-facing interface for the null fetch\nnegotiator. It not only does not negotiate at all but affects the\neverything_local() check, so I wonder if it should be exposed\ndifferently. More information is in the commit message for the 2nd\npatch.\n\nI know that the following still needs to be done, but I thought I'd send\nout the patches for early feedback first since the main design and code\nis already done:\n\n - Commit messages\n - User-facing documentation\n - A way to prevent a promisor-remote fetch from invoking another\n promisor-remote fetch (use a file as a lock?)\n - Remove no_dependents code (fetch-pack, transport)\n\nJonathan Tan (7):\n fetch-pack: allow NULL negotiator->add_tip\n fetch-pack: allow NULL negotiator->known_common\n negotiator/null: add null fetch negotiator\n fetch: --stdin\n fetch: submodule config\n fetch: only populate existing_refs if needed\n promisor-remote: use subprocess to fetch\n\n Documentation/config/fetch.txt | 5 +++-\n Makefile | 1 +\n builtin/fetch.c | 42 +++++++++++++++++++++++------\n fetch-negotiator.c | 5 ++++\n fetch-pack.c | 23 +++++++++++-----\n negotiator/null.c | 34 +++++++++++++++++++++++\n negotiator/null.h | 8 ++++++\n promisor-remote.c | 46 +++++++++++++++-----------------\n repo-settings.c | 2 ++\n repository.h | 1 +\n submodule-config.c | 5 ++--\n t/t0410-partial-clone.sh | 2 +-\n t/t4067-diff-partial-clone.sh | 8 +++---\n t/t5554-null-fetch-negotiator.sh | 22 +++++++++++++++\n t/t5601-clone.sh | 2 +-\n 15 files changed, 157 insertions(+), 49 deletions(-)\n create mode 100644 negotiator/null.c\n create mode 100644 negotiator/null.h\n create mode 100755 t/t5554-null-fetch-negotiator.sh\n\n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402883","messageId":"044a782863dd690ed4e430b3793538f701a850c6.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 1/7] fetch-pack: allow NULL negotiator->add_tip","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:44Z","receivedAt":"2020-08-05T01:21:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a subsequent patch, a null fetch negotiator will be introduced. This\nnegotiator, among other things, will not need any tip information and\nwill have a NULL add_tip.\n\nTeach fetch-pack to allow this, and to take advantage of this by\navoiding a ref and alternate traversal when it can. To do this, this\npatch combines all invocations of functions that invoke add_tip into one\nfunction, and that function first checks whether add_tip is NULL.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 20 +++++++++++++-------\n 1 file changed, 13 insertions(+), 7 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 80fb3bd899..6c786f5970 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -240,6 +240,16 @@ static void mark_tips(struct fetch_negotiator *negotiator,\n \treturn;\n }\n \n+static void add_tips_and_alternates(struct fetch_negotiator *negotiator,\n+\t\t\t\t const struct oid_array *negotiation_tips)\n+{\n+\tif (!negotiator->add_tip)\n+\t\treturn;\n+\n+\tmark_tips(negotiator, negotiation_tips);\n+\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n+}\n+\n static int find_common(struct fetch_negotiator *negotiator,\n \t\t struct fetch_pack_args *args,\n \t\t int fd[2], struct object_id *result_oid,\n@@ -262,10 +272,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n-\tif (!args->no_dependents) {\n-\t\tmark_tips(negotiator, args->negotiation_tips);\n-\t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n-\t}\n+\tif (!args->no_dependents)\n+\t\tadd_tips_and_alternates(negotiator, args->negotiation_tips);\n \n \tfetching = 0;\n \tfor ( ; refs ; refs = refs->next) {\n@@ -1575,9 +1583,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\telse\n \t\t\t\t\tstate = FETCH_SEND_REQUEST;\n \n-\t\t\t\tmark_tips(negotiator, args->negotiation_tips);\n-\t\t\t\tfor_each_cached_alternate(negotiator,\n-\t\t\t\t\t\t\t insert_one_alternate_object);\n+\t\t\t\tadd_tips_and_alternates(negotiator, args->negotiation_tips);\n \t\t\t} else {\n \t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402884","messageId":"78d47204044dbe650cc782001de793a7cdf85692.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 3/7] negotiator/null: add null fetch negotiator","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:46Z","receivedAt":"2020-08-05T01:21:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a null fetch negotiator. This is introduced to allow partial clones\nto perform lazy fetches (which do not need negotiation at all) by\nspawning a subprocess (to be done in a subsequent patch), but can be\nuseful for end users, e.g. as a blunt fix for object corruption.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config/fetch.txt | 5 ++++-\n Makefile | 1 +\n fetch-negotiator.c | 5 +++++\n negotiator/null.c | 34 ++++++++++++++++++++++++++++++++\n negotiator/null.h | 8 ++++++++\n repo-settings.c | 2 ++\n repository.h | 1 +\n t/t5554-null-fetch-negotiator.sh | 22 +++++++++++++++++++++\n 8 files changed, 77 insertions(+), 1 deletion(-)\n create mode 100644 negotiator/null.c\n create mode 100644 negotiator/null.h\n create mode 100755 t/t5554-null-fetch-negotiator.sh\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex 0aaa05e8c0..09aff404be 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -60,7 +60,10 @@ fetch.negotiationAlgorithm::\n \tsent when negotiating the contents of the packfile to be sent by the\n \tserver. Set to \"skipping\" to use an algorithm that skips commits in an\n \teffort to converge faster, but may result in a larger-than-necessary\n-\tpackfile; The default is \"default\" which instructs Git to use the default algorithm\n+\tpackfile; or set to \"null\" to not send any information at all, which\n+\twill almost certainly result in a larger-than-necessary packfile, but\n+\twill skip the negotiation step.\n+\tThe default is \"default\" which instructs Git to use the default algorithm\n \tthat never skips commits (unless the server has acknowledged it or one\n \tof its descendants). If `feature.experimental` is enabled, then this\n \tsetting defaults to \"skipping\".\ndiff --git a/Makefile b/Makefile\nindex 372139f1f2..297ea5e517 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -917,6 +917,7 @@ LIB_OBJS += mergesort.o\n LIB_OBJS += midx.o\n LIB_OBJS += name-hash.o\n LIB_OBJS += negotiator/default.o\n+LIB_OBJS += negotiator/null.o\n LIB_OBJS += negotiator/skipping.o\n LIB_OBJS += notes-cache.o\n LIB_OBJS += notes-merge.o\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 0a1357dc9d..e2ecbe4367 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -2,6 +2,7 @@\n #include \"fetch-negotiator.h\"\n #include \"negotiator/default.h\"\n #include \"negotiator/skipping.h\"\n+#include \"negotiator/null.h\"\n #include \"repository.h\"\n \n void fetch_negotiator_init(struct repository *r,\n@@ -13,6 +14,10 @@ void fetch_negotiator_init(struct repository *r,\n \t\tskipping_negotiator_init(negotiator);\n \t\treturn;\n \n+\tcase FETCH_NEGOTIATION_NULL:\n+\t\tnull_negotiator_init(negotiator);\n+\t\treturn;\n+\n \tcase FETCH_NEGOTIATION_DEFAULT:\n \tdefault:\n \t\tdefault_negotiator_init(negotiator);\ndiff --git a/negotiator/null.c b/negotiator/null.c\nnew file mode 100644\nindex 0000000000..db63ce66c2\n--- /dev/null\n+++ b/negotiator/null.c\n@@ -0,0 +1,34 @@\n+#include \"cache.h\"\n+#include \"null.h\"\n+#include \"../commit.h\"\n+#include \"../fetch-negotiator.h\"\n+\n+static const struct object_id *next(struct fetch_negotiator *n)\n+{\n+\treturn NULL;\n+}\n+\n+static int ack(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/*\n+\t * This negotiator does not emit any commits, so there is no commit to\n+\t * be acknowledged. If there is any ack, there is a bug.\n+\t */\n+\tBUG(\"ack with null negotiator, which does not emit any commits\");\n+\treturn 0;\n+}\n+\n+static void release(struct fetch_negotiator *n)\n+{\n+\t/* nothing to release */\n+}\n+\n+void null_negotiator_init(struct fetch_negotiator *negotiator)\n+{\n+\tnegotiator->known_common = NULL;\n+\tnegotiator->add_tip = NULL;\n+\tnegotiator->next = next;\n+\tnegotiator->ack = ack;\n+\tnegotiator->release = release;\n+\tnegotiator->data = NULL;\n+}\ndiff --git a/negotiator/null.h b/negotiator/null.h\nnew file mode 100644\nindex 0000000000..96713f280e\n--- /dev/null\n+++ b/negotiator/null.h\n@@ -0,0 +1,8 @@\n+#ifndef NEGOTIATOR_NULL_H\n+#define NEGOTIATOR_NULL_H\n+\n+struct fetch_negotiator;\n+\n+void null_negotiator_init(struct fetch_negotiator *negotiator);\n+\n+#endif\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0918408b34..a8c7e1edd7 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -39,6 +39,8 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\telse if (!strcasecmp(strval, \"null\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NULL;\n \t\telse\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\ndiff --git a/repository.h b/repository.h\nindex 3c1f7d54bd..f72911a185 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -23,6 +23,7 @@ enum fetch_negotiation_setting {\n \tFETCH_NEGOTIATION_NONE = 0,\n \tFETCH_NEGOTIATION_DEFAULT = 1,\n \tFETCH_NEGOTIATION_SKIPPING = 2,\n+\tFETCH_NEGOTIATION_NULL = 3,\n };\n \n struct repo_settings {\ndiff --git a/t/t5554-null-fetch-negotiator.sh b/t/t5554-null-fetch-negotiator.sh\nnew file mode 100755\nindex 0000000000..09a8f0d608\n--- /dev/null\n+++ b/t/t5554-null-fetch-negotiator.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description='test null fetch negotiator'\n+. ./test-lib.sh\n+\n+test_expect_success 'null negotiator does not emit any \"have\"' '\n+\trm -f trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_commit -C server to_fetch &&\n+\n+\ttest_create_repo client &&\n+\ttest_commit -C client we_have &&\n+\n+\ttest_config -C client fetch.negotiationalgorithm null &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\n+\t! grep \"fetch> have\" trace &&\n+\tgrep \"fetch> done\" trace\n+'\n+\n+test_done\n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402885","messageId":"401227c2220b6b45d80e21b52e29b6821ca139f9.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:45Z","receivedAt":"2020-08-05T01:21:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a subsequent patch, a null fetch negotiator will be introduced. This\nnegotiator, among other things, will not need any information about\ncommon objects and will have a NULL known_common. Teach fetch-pack to\nallow this.\n\n[NEEDSWORK]\nOptimizing out the ref iteration also affects the execution\nof everything_local(), which relies on COMPLETE being set. (Having said\nthat, the typical use case - lazy fetching - would be fine with\neverything_local() always returning that not everything is local.)\n\nThis optimization is needed so that in the future, fetch_pack() can be\nused to lazily fetch in a partial clone (without the no_dependents\nflag). This means that fetch_pack() needs a way to execute without\nrelying on any targets of refs being present, and thus it cannot use the\nref iterator (because it checks and lazy-fetches any missing targets).\n(Git currently does not have this problem because we use the\nno_dependents flag, but lazy-fetching will in a subsequent patch be\nchanged to use the user-facing fetch command, which does not use this\nflag.)\n[/NEEDSWORK]\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 6c786f5970..5f5474dbed 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -677,6 +677,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n \n+\tif (!negotiator->known_common)\n+\t\treturn;\n+\n \tsave_commit_buffer = 0;\n \n \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402886","messageId":"9ebcc5808a21e1f87370b6c18ea28168e63d4e3a.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 4/7] fetch: --stdin","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:47Z","receivedAt":"2020-08-05T01:21:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/fetch.c | 19 +++++++++++++++++--\n 1 file changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3ccf69753f..a5498646bf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -80,6 +80,7 @@ static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n+static int stdin_refspecs = 0;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -209,6 +210,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"check for forced-updates on all updated branches\")),\n \tOPT_BOOL(0, \"write-commit-graph\", &fetch_write_commit_graph,\n \t\t N_(\"write the commit-graph after fetching\")),\n+\tOPT_BOOL(0, \"stdin\", &stdin_refspecs,\n+\t\t N_(\"accept refspecs from stdin\")),\n \tOPT_END()\n };\n \n@@ -1684,7 +1687,8 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \treturn;\n }\n \n-static int fetch_one(struct remote *remote, int argc, const char **argv, int prune_tags_ok)\n+static int fetch_one(struct remote *remote, int argc, const char **argv,\n+\t\t int prune_tags_ok, int use_stdin_refspecs)\n {\n \tstruct refspec rs = REFSPEC_INIT_FETCH;\n \tint i;\n@@ -1741,6 +1745,13 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n \t\t}\n \t}\n \n+\tif (use_stdin_refspecs) {\n+\t\tstruct strbuf line = STRBUF_INIT;\n+\t\twhile (strbuf_getline_lf(&line, stdin) != EOF)\n+\t\t\trefspec_append(&rs, line.buf);\n+\t\tstrbuf_release(&line);\n+\t}\n+\n \tif (server_options.nr)\n \t\tgtransport->server_options = &server_options;\n \n@@ -1841,7 +1852,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n-\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok);\n+\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);\n \t} else {\n \t\tint max_children = max_jobs;\n \n@@ -1849,6 +1860,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \n+\t\tif (stdin_refspecs)\n+\t\t\tdie(_(\"--stdin can only be used when fetching \"\n+\t\t\t \"from one remote\"));\n+\n \t\tif (max_children < 0)\n \t\t\tmax_children = fetch_parallel_config;\n \n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402887","messageId":"c4cb657ae0452397f97c2b01dbc2e6c9b0cc5ee7.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 5/7] fetch: submodule config","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:48Z","receivedAt":"2020-08-05T01:21:24Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/fetch.c | 10 ++++++++--\n submodule-config.c | 5 +++--\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a5498646bf..29db219c68 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1786,12 +1786,18 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tfree(anon);\n \t}\n \n-\tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n-\t\t\t\t &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n+\t\tint *sfjc = submodule_fetch_jobs_config == -1\n+\t\t\t ? &submodule_fetch_jobs_config : NULL;\n+\t\tint *rs = recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n+\t\t\t ? &recurse_submodules : NULL;\n+\n+\t\tfetch_config_from_gitmodules(sfjc, rs);\n+\t}\n \n \tif (deepen_relative) {\n \t\tif (deepen_relative < 0)\ndiff --git a/submodule-config.c b/submodule-config.c\nindex e175dfbc38..8d65273ed2 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -776,10 +776,11 @@ struct fetch_config {\n static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n {\n \tstruct fetch_config *config = cb;\n-\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n+\tif (!strcmp(var, \"submodule.fetchjobs\") && config->max_children) {\n \t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n \t\treturn 0;\n-\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n+\t} else if (!strcmp(var, \"fetch.recursesubmodules\") &&\n+\t\t config->recurse_submodules) {\n \t\t*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n \t\treturn 0;\n \t}\n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402888","messageId":"38bbad333e3eda1322943e29efabad9d9c6a996a.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 7/7] promisor-remote: use subprocess to fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:50Z","receivedAt":"2020-08-05T01:21:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"---\n promisor-remote.c | 46 ++++++++++++++++-------------------\n t/t0410-partial-clone.sh | 2 +-\n t/t4067-diff-partial-clone.sh | 8 +++---\n t/t5601-clone.sh | 2 +-\n 4 files changed, 27 insertions(+), 31 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex baaea12fd6..6e647610e9 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -3,6 +3,7 @@\n #include \"promisor-remote.h\"\n #include \"config.h\"\n #include \"transport.h\"\n+#include \"argv-array.h\"\n \n static char *repository_format_partial_clone;\n static const char *core_partial_clone_filter_default;\n@@ -12,39 +13,34 @@ void set_repository_format_partial_clone(char *partial_clone)\n \trepository_format_partial_clone = xstrdup_or_null(partial_clone);\n }\n \n-static int fetch_refs(const char *remote_name, struct ref *ref)\n-{\n-\tstruct remote *remote;\n-\tstruct transport *transport;\n-\tint res;\n-\n-\tremote = remote_get(remote_name);\n-\tif (!remote->url[0])\n-\t\tdie(_(\"Remote with no URL\"));\n-\ttransport = transport_get(remote, remote->url[0]);\n-\n-\ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n-\ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n-\tres = transport_fetch_refs(transport, ref);\n-\n-\treturn res;\n-}\n-\n static int fetch_objects(const char *remote_name,\n \t\t\t const struct object_id *oids,\n \t\t\t int oid_nr)\n {\n-\tstruct ref *ref = NULL;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tint i;\n+\tFILE *child_in;\n+\n+\tchild.git_cmd = 1;\n+\tchild.in = -1;\n+\targv_array_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=null\",\n+\t\t\t \"fetch\", remote_name, \"--no-tags\",\n+\t\t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n+\t\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n+\tif (start_command(&child))\n+\t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n+\tchild_in = xfdopen(child.in, \"w\");\n \n \tfor (i = 0; i < oid_nr; i++) {\n-\t\tstruct ref *new_ref = alloc_ref(oid_to_hex(&oids[i]));\n-\t\toidcpy(&new_ref->old_oid, &oids[i]);\n-\t\tnew_ref->exact_oid = 1;\n-\t\tnew_ref->next = ref;\n-\t\tref = new_ref;\n+\t\tif (fputs(oid_to_hex(&oids[i]), child_in) < 0)\n+\t\t\tdie_errno(_(\"promisor-remote: could not write to fetch subprocess\"));\n+\t\tif (fputc('\\n', child_in) < 0)\n+\t\t\tdie_errno(_(\"promisor-remote: could not write to fetch subprocess\"));\n \t}\n-\treturn fetch_refs(remote_name, ref);\n+\n+\tif (fclose(child_in) < 0)\n+\t\tdie_errno(_(\"promisor-remote: could not close stdin to fetch subprocess\"));\n+\treturn finish_command(&child) ? -1 : 0;\n }\n \n static struct promisor_remote *promisors;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be..3e454f934e 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -203,7 +203,7 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \trm -rf repo/.git/objects/* &&\n \trm -f trace &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C repo cat-file -p \"$HASH\" &&\n-\tgrep \"git< fetch=.*ref-in-want\" trace\n+\tgrep \"fetch< fetch=.*ref-in-want\" trace\n '\n \n test_expect_success 'fetching of missing objects from another promisor remote' '\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex ef8e0e9cb0..804f2a82e8 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -20,7 +20,7 @@ test_expect_success 'git show batches blobs' '\n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -44,7 +44,7 @@ test_expect_success 'diff batches blobs' '\n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -127,7 +127,7 @@ test_expect_success 'diff with rename detection batches blobs' '\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD >out &&\n \tgrep \":100644 100644.*R[0-9][0-9][0-9].*b.*c\" out &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -175,7 +175,7 @@ test_expect_success 'diff --break-rewrites fetches only if necessary, and batche\n \t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n \t# end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb7..f82b0dbb5a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -702,7 +702,7 @@ test_expect_success 'batch missing blob request during checkout' '\n \t# Ensure that there is only one negotiation by checking that there is\n \t# only \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client checkout HEAD^ &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402889","messageId":"d4dd33e465e9cf808e3cfb9a2679174a1dac8f91.1596590295.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1596590295.git.jonathantanmy@google.com","subject":"[RFC PATCH 6/7] fetch: only populate existing_refs if needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T01:20:49Z","receivedAt":"2020-08-05T01:21:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Signed-off-by: Jonathan Tan \n---\n builtin/fetch.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 29db219c68..6460ce3f4e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -449,6 +449,7 @@ static struct ref *get_ref_map(struct remote *remote,\n \tstruct ref *orefs = NULL, **oref_tail = &orefs;\n \n \tstruct hashmap existing_refs;\n+\tint existing_refs_populated = 0;\n \n \tif (rs->nr) {\n \t\tstruct refspec *fetch_refspec;\n@@ -542,15 +543,18 @@ static struct ref *get_ref_map(struct remote *remote,\n \n \tref_map = ref_remove_duplicates(ref_map);\n \n-\trefname_hash_init(&existing_refs);\n-\tfor_each_ref(add_one_refname, &existing_refs);\n-\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->peer_ref) {\n \t\t\tconst char *refname = rm->peer_ref->name;\n \t\t\tstruct refname_hash_entry *peer_item;\n \t\t\tunsigned int hash = strhash(refname);\n \n+\t\t\tif (!existing_refs_populated) {\n+\t\t\t\trefname_hash_init(&existing_refs);\n+\t\t\t\tfor_each_ref(add_one_refname, &existing_refs);\n+\t\t\t\texisting_refs_populated = 1;\n+\t\t\t}\n+\n \t\t\tpeer_item = hashmap_get_entry_from_hash(&existing_refs,\n \t\t\t\t\t\thash, refname,\n \t\t\t\t\t\tstruct refname_hash_entry, ent);\n@@ -560,7 +564,8 @@ static struct ref *get_ref_map(struct remote *remote,\n \t\t\t}\n \t\t}\n \t}\n-\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n+\tif (existing_refs_populated)\n+\t\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n \n \treturn ref_map;\n }\n-- \n2.28.0.163.g6104cc2f0b6-goog\n\n"},{"id":"402947","messageId":"xmqqo8noooni.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"044a782863dd690ed4e430b3793538f701a850c6.1596590295.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 1/7] fetch-pack: allow NULL negotiator->add_tip","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T19:53:21Z","receivedAt":"2020-08-05T19:53:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> In a subsequent patch, a null fetch negotiator will be introduced. This\n> negotiator, among other things, will not need any tip information and\n> will have a NULL add_tip.\n\nAfter reading the original code that the patch touches, I know what\n\"a NULL add_tip\" is, but it would have been nicer to readers if you\nsaid \"callback\" or \"method\" or \"function\".\n\nWith or without the \"if add_tip method is set to NULL, return\nwithout doing anything\" optimization, it does make sense to refactor\ntwo existing places that do identical things. \n\nIf all the current negotiators never have NULL in the .add_tip\nfield, however, I'd suspect that it would make it easier to\nunderstand if this step is explained as refactoring duplicated code\ninto a single helper _without_ the early-return for optimization,\nand then the step that introduces the negotiator that does want to\nlack add_tip method to add the early return. After all, if the null\nnegotiator added a pointer to a no-op function to its .add_tip field,\nthings would still work correctly without the early-return in the\nnew helper function, no?\n\n> Teach fetch-pack to allow this, and to take advantage of this by\n> avoiding a ref and alternate traversal when it can. To do this, this\n> patch combines all invocations of functions that invoke add_tip into one\n> function, and that function first checks whether add_tip is NULL.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 20 +++++++++++++-------\n> 1 file changed, 13 insertions(+), 7 deletions(-)\n\n\n\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 80fb3bd899..6c786f5970 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -240,6 +240,16 @@ static void mark_tips(struct fetch_negotiator *negotiator,\n> \treturn;\n> }\n> \n> +static void add_tips_and_alternates(struct fetch_negotiator *negotiator,\n> +\t\t\t\t const struct oid_array *negotiation_tips)\n> +{\n> +\tif (!negotiator->add_tip)\n> +\t\treturn;\n> +\n> +\tmark_tips(negotiator, negotiation_tips);\n> +\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n> +}\n> +\n> static int find_common(struct fetch_negotiator *negotiator,\n> \t\t struct fetch_pack_args *args,\n> \t\t int fd[2], struct object_id *result_oid,\n> @@ -262,10 +272,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n> \t\t\t PACKET_READ_CHOMP_NEWLINE |\n> \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n> \n> -\tif (!args->no_dependents) {\n> -\t\tmark_tips(negotiator, args->negotiation_tips);\n> -\t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n> -\t}\n> +\tif (!args->no_dependents)\n> +\t\tadd_tips_and_alternates(negotiator, args->negotiation_tips);\n> \n> \tfetching = 0;\n> \tfor ( ; refs ; refs = refs->next) {\n> @@ -1575,9 +1583,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n> \t\t\t\telse\n> \t\t\t\t\tstate = FETCH_SEND_REQUEST;\n> \n> -\t\t\t\tmark_tips(negotiator, args->negotiation_tips);\n> -\t\t\t\tfor_each_cached_alternate(negotiator,\n> -\t\t\t\t\t\t\t insert_one_alternate_object);\n> +\t\t\t\tadd_tips_and_alternates(negotiator, args->negotiation_tips);\n> \t\t\t} else {\n> \t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n> \t\t\t\tstate = FETCH_SEND_REQUEST;\n"},{"id":"402953","messageId":"xmqqk0yconyw.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"401227c2220b6b45d80e21b52e29b6821ca139f9.1596590295.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T20:08:07Z","receivedAt":"2020-08-05T20:08:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> In a subsequent patch, a null fetch negotiator will be introduced. This\n> negotiator, among other things, will not need any information about\n> common objects and will have a NULL known_common. Teach fetch-pack to\n> allow this.\n\nHmph, both the default and the skipping negotiator seem to put NULL\nin known_common and add_tip when its next() method is called. Also\nthey clear known_common to NULL after add_tip is called even once.\n\nSo, how have we survived so far without this patch to \"allow this\n(i.e. known_common method to be NULL)\"? Is there something that\nmakes sure a negotiator never gets called from this function after\nits .next or .add_tip method is called?\n\nPuzzled. Or is this merely an optimization? If so, it's not like\nthe change \"allows this\", but it starts to take advantage of it in\nsome way.\n\n\t... goes and looks at mark_complete_and_common_ref()\n\nThe function seems to have an unconditional call to ->known_common(),\nso anybody passing a negotiator whose known_common is NULL would\nalready be segfaulting, so this does not appear to be an optimization\nbut necessary to keep the code from crashing. I cannot quite tell\nif it is avoiding unnecessary work, or sweeping crashes under the\nrug, though. \n\nIs the untold assumption that mark_complete_and_common_ref() will\nnever be called after either mark_tips() or find_common() have been\ncalled?\n\nThanks.\n\n> [NEEDSWORK]\n> Optimizing out the ref iteration also affects the execution\n> of everything_local(), which relies on COMPLETE being set. (Having said\n> that, the typical use case - lazy fetching - would be fine with\n> everything_local() always returning that not everything is local.)\n>\n> This optimization is needed so that in the future, fetch_pack() can be\n> used to lazily fetch in a partial clone (without the no_dependents\n> flag). This means that fetch_pack() needs a way to execute without\n> relying on any targets of refs being present, and thus it cannot use the\n> ref iterator (because it checks and lazy-fetches any missing targets).\n> (Git currently does not have this problem because we use the\n> no_dependents flag, but lazy-fetching will in a subsequent patch be\n> changed to use the user-facing fetch command, which does not use this\n> flag.)\n> [/NEEDSWORK]\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 6c786f5970..5f5474dbed 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -677,6 +677,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \tint old_save_commit_buffer = save_commit_buffer;\n> \ttimestamp_t cutoff = 0;\n> \n> +\tif (!negotiator->known_common)\n> +\t\treturn;\n> +\n> \tsave_commit_buffer = 0;\n> \n> \ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n"},{"id":"402960","messageId":"xmqqft90omsw.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"9ebcc5808a21e1f87370b6c18ea28168e63d4e3a.1596590295.git.jonathantanmy@google.com","subject":"Re: [RFC PATCH 4/7] fetch: --stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T20:33:19Z","receivedAt":"2020-08-05T20:33:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/fetch.c | 19 +++++++++++++++++--\n> 1 file changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 3ccf69753f..a5498646bf 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -80,6 +80,7 @@ static struct list_objects_filter_options filter_options;\n> static struct string_list server_options = STRING_LIST_INIT_DUP;\n> static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n> static int fetch_write_commit_graph = -1;\n> +static int stdin_refspecs = 0;\n\nDon't initialize statics to 0 (leave that to BSS).\n\n> @@ -209,6 +210,8 @@ static struct option builtin_fetch_options[] = {\n> \t\t N_(\"check for forced-updates on all updated branches\")),\n> \tOPT_BOOL(0, \"write-commit-graph\", &fetch_write_commit_graph,\n> \t\t N_(\"write the commit-graph after fetching\")),\n> +\tOPT_BOOL(0, \"stdin\", &stdin_refspecs,\n> +\t\t N_(\"accept refspecs from stdin\")),\n> \tOPT_END()\n> };\n> \n> @@ -1684,7 +1687,8 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n> \treturn;\n> }\n> \n> -static int fetch_one(struct remote *remote, int argc, const char **argv, int prune_tags_ok)\n> +static int fetch_one(struct remote *remote, int argc, const char **argv,\n> +\t\t int prune_tags_ok, int use_stdin_refspecs)\n> {\n> \tstruct refspec rs = REFSPEC_INIT_FETCH;\n> \tint i;\n> @@ -1741,6 +1745,13 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n> \t\t}\n> \t}\n> \n> +\tif (use_stdin_refspecs) {\n> +\t\tstruct strbuf line = STRBUF_INIT;\n> +\t\twhile (strbuf_getline_lf(&line, stdin) != EOF)\n> +\t\t\trefspec_append(&rs, line.buf);\n> +\t\tstrbuf_release(&line);\n> +\t}\n\nThis will use refspecs both from the command line and the standard\ninput by appending? IOW, these refspecs that came from the standard\ninput are treated otherwise identically to those that came from the\ncommand line?\n\nI do not particularly care whether it is \"append to command line\" or\n\"replace command line\", as I do not think it makes much difference\nin usability. Just wanted to be sure you coded the behaviour you\nwanted.\n\n> @@ -1849,6 +1860,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\t\tdie(_(\"--filter can only be used with the remote \"\n> \t\t\t \"configured in extensions.partialclone\"));\n> \n> +\t\tif (stdin_refspecs)\n> +\t\t\tdie(_(\"--stdin can only be used when fetching \"\n> +\t\t\t \"from one remote\"));\n\nIs that only because you happened to have implemented the reading in\nfetch_one() that is designed to be called once per remote? \n\nYou could read them here to a refspec for everybody, and then pass a\npointer to that refspec as the extra parameter to fetch_one(), and\nfetch_one() can use that by duplicating and appending to its \"rs\",\nif we wanted to, no? I do not know how important to support such a\nuse case, though. It just feels a bit of shame if this restriction\nis purely imposed by the implementation, when lifting the refstiction\ndoes not seem too involved.\n\nThanks.\n\n\n> \t\tif (max_children < 0)\n> \t\t\tmax_children = fetch_parallel_config;\n"},{"id":"402992","messageId":"xmqqpn84n3pi.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"xmqqk0yconyw.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-05T22:11:05Z","receivedAt":"2020-08-05T22:11:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Tan writes:\n>\n>> In a subsequent patch, a null fetch negotiator will be introduced. This\n>> negotiator, among other things, will not need any information about\n>> common objects and will have a NULL known_common. Teach fetch-pack to\n>> allow this.\n>\n> Hmph, both the default and the skipping negotiator seem to put NULL\n> in known_common and add_tip when its next() method is called. Also\n> they clear known_common to NULL after add_tip is called even once.\n>\n> So, how have we survived so far without this patch to \"allow this\n> (i.e. known_common method to be NULL)\"? Is there something that\n> makes sure a negotiator never gets called from this function after\n> its .next or .add_tip method is called?\n>\n> Puzzled. Or is this merely an optimization? If so, it's not like\n> the change \"allows this\", but it starts to take advantage of it in\n> some way.\n>\n> \t... goes and looks at mark_complete_and_common_ref()\n>\n> The function seems to have an unconditional call to ->known_common(),\n> so anybody passing a negotiator whose known_common is NULL would\n> already be segfaulting, so this does not appear to be an optimization\n> but necessary to keep the code from crashing. I cannot quite tell\n> if it is avoiding unnecessary work, or sweeping crashes under the\n> rug, though. \n>\n> Is the untold assumption that mark_complete_and_common_ref() will\n> never be called after either mark_tips() or find_common() have been\n> called?\n\nShot in the dark. Perhaps clearing of .add_tip and .known_common in\nthe .next method was done to catch a wrong calling sequence where\nmark_complete_and_common_ref() gets called after mark_tips() and/or\nfind_common() have by forcing the code to segfault? If so, this\npatch removes the safety and we may want to add an equivalent safety\nlogic. Perhaps by adding a state field in the negotiator instance\nto record that mark_tips() and/or find_common() have been used and\ncall a BUG() if mark_complete_and_common_ref() gets called after that,\nif enforcing such an invariant was the original reason why these\nfields were cleared.\n"},{"id":"403151","messageId":"20200807205308.2732007-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqo8noooni.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 1/7] fetch-pack: allow NULL negotiator->add_tip","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-07T20:53:08Z","receivedAt":"2020-08-07T20:53:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > In a subsequent patch, a null fetch negotiator will be introduced. This\n> > negotiator, among other things, will not need any tip information and\n> > will have a NULL add_tip.\n> \n> After reading the original code that the patch touches, I know what\n> \"a NULL add_tip\" is, but it would have been nicer to readers if you\n> said \"callback\" or \"method\" or \"function\".\n\nOK.\n\n> With or without the \"if add_tip method is set to NULL, return\n> without doing anything\" optimization, it does make sense to refactor\n> two existing places that do identical things. \n> \n> If all the current negotiators never have NULL in the .add_tip\n> field, however, I'd suspect that it would make it easier to\n> understand if this step is explained as refactoring duplicated code\n> into a single helper _without_ the early-return for optimization,\n> and then the step that introduces the negotiator that does want to\n> lack add_tip method to add the early return.\n\nThis makes sense.\n\n> After all, if the null\n> negotiator added a pointer to a no-op function to its .add_tip field,\n> things would still work correctly without the early-return in the\n> new helper function, no?\n\nRight now no, because in a partial clone, I also want to\navoid any iterations over refs since whenever refs are iterated, their\ntargets are checked for existence (see the call to\nref_resolves_to_object() in packed_ref_iterator_advance() in\npacked-refs.c). However after looking at the code again, I see that\nfor_each_rawref() exists, which might do what I want (coupled with our\nown way of deref-fing tags in fetch-pack). I'll take another look and if\nthis works, I'll just avoid the whole null method thing.\n"},{"id":"403152","messageId":"20200807205940.2734236-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqpn84n3pi.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-07T20:59:40Z","receivedAt":"2020-08-07T20:59:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > Hmph, both the default and the skipping negotiator seem to put NULL\n> > in known_common and add_tip when its next() method is called. Also\n> > they clear known_common to NULL after add_tip is called even once.\n> >\n> > So, how have we survived so far without this patch to \"allow this\n> > (i.e. known_common method to be NULL)\"? Is there something that\n> > makes sure a negotiator never gets called from this function after\n> > its .next or .add_tip method is called?\n\n[snip]\n\n> > Is the untold assumption that mark_complete_and_common_ref() will\n> > never be called after either mark_tips() or find_common() have been\n> > called?\n> \n> Shot in the dark. Perhaps clearing of .add_tip and .known_common in\n> the .next method was done to catch a wrong calling sequence where\n> mark_complete_and_common_ref() gets called after mark_tips() and/or\n> find_common() have by forcing the code to segfault?\n\nAh...yes, if I remember correctly, that was my original intention when I\nset them to NULL.\n\n> If so, this\n> patch removes the safety and we may want to add an equivalent safety\n> logic. Perhaps by adding a state field in the negotiator instance\n> to record that mark_tips() and/or find_common() have been used and\n> call a BUG() if mark_complete_and_common_ref() gets called after that,\n> if enforcing such an invariant was the original reason why these\n> fields were cleared.\n\nSounds good. As I said in my reply to your query on patch 1, we might\nnot need to set NULL anymore, but if we do, I'll do this.\n"},{"id":"403155","messageId":"20200807211028.2736644-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqft90omsw.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 4/7] fetch: --stdin","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-07T21:10:28Z","receivedAt":"2020-08-07T21:10:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > @@ -1741,6 +1745,13 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n> > \t\t}\n> > \t}\n> > \n> > +\tif (use_stdin_refspecs) {\n> > +\t\tstruct strbuf line = STRBUF_INIT;\n> > +\t\twhile (strbuf_getline_lf(&line, stdin) != EOF)\n> > +\t\t\trefspec_append(&rs, line.buf);\n> > +\t\tstrbuf_release(&line);\n> > +\t}\n> \n> This will use refspecs both from the command line and the standard\n> input by appending? IOW, these refspecs that came from the standard\n> input are treated otherwise identically to those that came from the\n> command line?\n> \n> I do not particularly care whether it is \"append to command line\" or\n> \"replace command line\", as I do not think it makes much difference\n> in usability. Just wanted to be sure you coded the behaviour you\n> wanted.\n\nYes, except that I didn't plan to support the \"tag foo\" format. (My aim\nwith this is just to allow \"git fetch\" to take large numbers of\nrefspecs, because when we lazy fetch, the number of objects we fetch\nmight be large.)\n\n> > @@ -1849,6 +1860,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> > \t\t\tdie(_(\"--filter can only be used with the remote \"\n> > \t\t\t \"configured in extensions.partialclone\"));\n> > \n> > +\t\tif (stdin_refspecs)\n> > +\t\t\tdie(_(\"--stdin can only be used when fetching \"\n> > +\t\t\t \"from one remote\"));\n> \n> Is that only because you happened to have implemented the reading in\n> fetch_one() that is designed to be called once per remote? \n> \n> You could read them here to a refspec for everybody, and then pass a\n> pointer to that refspec as the extra parameter to fetch_one(), and\n> fetch_one() can use that by duplicating and appending to its \"rs\",\n> if we wanted to, no? I do not know how important to support such a\n> use case, though. It just feels a bit of shame if this restriction\n> is purely imposed by the implementation, when lifting the refstiction\n> does not seem too involved.\n\nYes, and I only implemented the reading in fetch_one() because\nfetch_multiple() does not read additional refspecs from the command-line\n(it does not take \"argv\"). Looking at the code, this seems to be on\npurpose - there is the error message \"fetch --all does not make sense\nwith refspecs\", and when --multiple is set, all args are assumed to be\nremotes.\n"},{"id":"403156","messageId":"20200807211043.2738308-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqft90omsw.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH 4/7] fetch: --stdin","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-07T21:10:43Z","receivedAt":"2020-08-07T21:10:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > @@ -1741,6 +1745,13 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n> > \t\t}\n> > \t}\n> > \n> > +\tif (use_stdin_refspecs) {\n> > +\t\tstruct strbuf line = STRBUF_INIT;\n> > +\t\twhile (strbuf_getline_lf(&line, stdin) != EOF)\n> > +\t\t\trefspec_append(&rs, line.buf);\n> > +\t\tstrbuf_release(&line);\n> > +\t}\n> \n> This will use refspecs both from the command line and the standard\n> input by appending? IOW, these refspecs that came from the standard\n> input are treated otherwise identically to those that came from the\n> command line?\n> \n> I do not particularly care whether it is \"append to command line\" or\n> \"replace command line\", as I do not think it makes much difference\n> in usability. Just wanted to be sure you coded the behaviour you\n> wanted.\n\nYes, except that I didn't plan to support the \"tag foo\" format. (My aim\nwith this is just to allow \"git fetch\" to take large numbers of\nrefspecs, because when we lazy fetch, the number of objects we fetch\nmight be large.)\n\n> > @@ -1849,6 +1860,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> > \t\t\tdie(_(\"--filter can only be used with the remote \"\n> > \t\t\t \"configured in extensions.partialclone\"));\n> > \n> > +\t\tif (stdin_refspecs)\n> > +\t\t\tdie(_(\"--stdin can only be used when fetching \"\n> > +\t\t\t \"from one remote\"));\n> \n> Is that only because you happened to have implemented the reading in\n> fetch_one() that is designed to be called once per remote? \n> \n> You could read them here to a refspec for everybody, and then pass a\n> pointer to that refspec as the extra parameter to fetch_one(), and\n> fetch_one() can use that by duplicating and appending to its \"rs\",\n> if we wanted to, no? I do not know how important to support such a\n> use case, though. It just feels a bit of shame if this restriction\n> is purely imposed by the implementation, when lifting the refstiction\n> does not seem too involved.\n\nYes, and I only implemented the reading in fetch_one() because\nfetch_multiple() does not read additional refspecs from the command-line\n(it does not take \"argv\"). Looking at the code, this seems to be on\npurpose - there is the error message \"fetch --all does not make sense\nwith refspecs\", and when --multiple is set, all args are assumed to be\nremotes.\n"},{"id":"403162","messageId":"xmqqbljmf791.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"20200807211028.2736644-1-jonathantanmy@google.com","subject":"Re: [RFC PATCH 4/7] fetch: --stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-07T21:58:34Z","receivedAt":"2020-08-07T21:58:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Yes, and I only implemented the reading in fetch_one() because\n> fetch_multiple() does not read additional refspecs from the command-line\n> (it does not take \"argv\"). Looking at the code, this seems to be on\n> purpose - there is the error message \"fetch --all does not make sense\n> with refspecs\", and when --multiple is set, all args are assumed to be\n> remotes.\n\nOK, that does make sense. so the multiple one is just a short-hand\nfor \n\n\tfor remote in ...\n\tdo\n\t\tgit fetch $remote\n\tdone\n\n"},{"id":"403418","messageId":"cover.1597184948.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"20200724223844.2723397-1-jonathantanmy@google.com","subject":"[PATCH v2 0/7] Lazy fetch with subprocess","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:15Z","receivedAt":"2020-08-11T22:52:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"These patches are based on jc/no-update-fetch-head.\n\nAfter the email exchange with Junio, I went back and took a look to see\nif I could accomplish what I needed without using NULL methods in the\nfetch negotiators. And I could do it - I just had to be more careful\nwhen iterating over refs and dereferencing them. You can see it in patch\n5.\n\nIn version 1, I wrote that these needed to be done:\n\n - Commit messages\n - User-facing documentation\n - A way to prevent a promisor-remote fetch from invoking another\n promisor-remote fetch (use a file as a lock?)\n - Remove no_dependents code (fetch-pack, transport)\n\n1, 2, and 4 are already done. As for 3, we currently don't have this\nsafeguard before this patch set, so I did not include one in this patch\nset either.\n\nJonathan Tan (7):\n negotiator/null: add null fetch negotiator\n fetch: allow refspecs specified through stdin\n fetch: avoid reading submodule config until needed\n fetch: only populate existing_refs if needed\n fetch-pack: do not lazy-fetch during ref iteration\n promisor-remote: lazy-fetch objects in subprocess\n fetch-pack: remove no_dependents code\n\n Documentation/config/fetch.txt | 5 +-\n Documentation/git-fetch.txt | 4 +\n Documentation/technical/partial-clone.txt | 13 +-\n Makefile | 1 +\n builtin/fetch-pack.c | 4 -\n builtin/fetch.c | 42 ++++-\n fetch-negotiator.c | 5 +\n fetch-pack.c | 189 +++++++++-------------\n fetch-pack.h | 14 --\n negotiator/null.c | 44 +++++\n negotiator/null.h | 8 +\n promisor-remote.c | 46 +++---\n remote-curl.c | 6 -\n repo-settings.c | 2 +\n repository.h | 1 +\n submodule-config.c | 5 +-\n t/t0410-partial-clone.sh | 2 +-\n t/t4067-diff-partial-clone.sh | 8 +-\n t/t5554-null-fetch-negotiator.sh | 22 +++\n t/t5601-clone.sh | 2 +-\n t/t5616-partial-clone.sh | 20 +++\n transport.c | 4 -\n transport.h | 7 -\n 23 files changed, 252 insertions(+), 202 deletions(-)\n create mode 100644 negotiator/null.c\n create mode 100644 negotiator/null.h\n create mode 100755 t/t5554-null-fetch-negotiator.sh\n\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403419","messageId":"35bdd372ae3691f54775dd616576e8ed6d68f1d3.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 1/7] negotiator/null: add null fetch negotiator","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:16Z","receivedAt":"2020-08-11T22:52:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a null fetch negotiator. This is introduced to allow partial clones\nto skip the unneeded negotiation step when fetching missing objects\nusing a \"git fetch\" subprocess. (The implementation of spawning a \"git\nfetch\" subprocess will be done in a subsequent patch.) But this can also\nbe useful for end users, e.g. as a blunt fix for object corruption.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config/fetch.txt | 5 +++-\n Makefile | 1 +\n fetch-negotiator.c | 5 ++++\n negotiator/null.c | 44 ++++++++++++++++++++++++++++++++\n negotiator/null.h | 8 ++++++\n repo-settings.c | 2 ++\n repository.h | 1 +\n t/t5554-null-fetch-negotiator.sh | 22 ++++++++++++++++\n 8 files changed, 87 insertions(+), 1 deletion(-)\n create mode 100644 negotiator/null.c\n create mode 100644 negotiator/null.h\n create mode 100755 t/t5554-null-fetch-negotiator.sh\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex 0aaa05e8c0..09aff404be 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -60,7 +60,10 @@ fetch.negotiationAlgorithm::\n \tsent when negotiating the contents of the packfile to be sent by the\n \tserver. Set to \"skipping\" to use an algorithm that skips commits in an\n \teffort to converge faster, but may result in a larger-than-necessary\n-\tpackfile; The default is \"default\" which instructs Git to use the default algorithm\n+\tpackfile; or set to \"null\" to not send any information at all, which\n+\twill almost certainly result in a larger-than-necessary packfile, but\n+\twill skip the negotiation step.\n+\tThe default is \"default\" which instructs Git to use the default algorithm\n \tthat never skips commits (unless the server has acknowledged it or one\n \tof its descendants). If `feature.experimental` is enabled, then this\n \tsetting defaults to \"skipping\".\ndiff --git a/Makefile b/Makefile\nindex 372139f1f2..297ea5e517 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -917,6 +917,7 @@ LIB_OBJS += mergesort.o\n LIB_OBJS += midx.o\n LIB_OBJS += name-hash.o\n LIB_OBJS += negotiator/default.o\n+LIB_OBJS += negotiator/null.o\n LIB_OBJS += negotiator/skipping.o\n LIB_OBJS += notes-cache.o\n LIB_OBJS += notes-merge.o\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 0a1357dc9d..e2ecbe4367 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -2,6 +2,7 @@\n #include \"fetch-negotiator.h\"\n #include \"negotiator/default.h\"\n #include \"negotiator/skipping.h\"\n+#include \"negotiator/null.h\"\n #include \"repository.h\"\n \n void fetch_negotiator_init(struct repository *r,\n@@ -13,6 +14,10 @@ void fetch_negotiator_init(struct repository *r,\n \t\tskipping_negotiator_init(negotiator);\n \t\treturn;\n \n+\tcase FETCH_NEGOTIATION_NULL:\n+\t\tnull_negotiator_init(negotiator);\n+\t\treturn;\n+\n \tcase FETCH_NEGOTIATION_DEFAULT:\n \tdefault:\n \t\tdefault_negotiator_init(negotiator);\ndiff --git a/negotiator/null.c b/negotiator/null.c\nnew file mode 100644\nindex 0000000000..1bd834f121\n--- /dev/null\n+++ b/negotiator/null.c\n@@ -0,0 +1,44 @@\n+#include \"cache.h\"\n+#include \"null.h\"\n+#include \"../commit.h\"\n+#include \"../fetch-negotiator.h\"\n+\n+static void known_common(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/* do nothing */\n+}\n+\n+static void add_tip(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/* do nothing */\n+}\n+\n+static const struct object_id *next(struct fetch_negotiator *n)\n+{\n+\treturn NULL;\n+}\n+\n+static int ack(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/*\n+\t * This negotiator does not emit any commits, so there is no commit to\n+\t * be acknowledged. If there is any ack, there is a bug.\n+\t */\n+\tBUG(\"ack with null negotiator, which does not emit any commits\");\n+\treturn 0;\n+}\n+\n+static void release(struct fetch_negotiator *n)\n+{\n+\t/* nothing to release */\n+}\n+\n+void null_negotiator_init(struct fetch_negotiator *negotiator)\n+{\n+\tnegotiator->known_common = known_common;\n+\tnegotiator->add_tip = add_tip;\n+\tnegotiator->next = next;\n+\tnegotiator->ack = ack;\n+\tnegotiator->release = release;\n+\tnegotiator->data = NULL;\n+}\ndiff --git a/negotiator/null.h b/negotiator/null.h\nnew file mode 100644\nindex 0000000000..96713f280e\n--- /dev/null\n+++ b/negotiator/null.h\n@@ -0,0 +1,8 @@\n+#ifndef NEGOTIATOR_NULL_H\n+#define NEGOTIATOR_NULL_H\n+\n+struct fetch_negotiator;\n+\n+void null_negotiator_init(struct fetch_negotiator *negotiator);\n+\n+#endif\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0918408b34..a8c7e1edd7 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -39,6 +39,8 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\telse if (!strcasecmp(strval, \"null\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NULL;\n \t\telse\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\ndiff --git a/repository.h b/repository.h\nindex 3c1f7d54bd..f72911a185 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -23,6 +23,7 @@ enum fetch_negotiation_setting {\n \tFETCH_NEGOTIATION_NONE = 0,\n \tFETCH_NEGOTIATION_DEFAULT = 1,\n \tFETCH_NEGOTIATION_SKIPPING = 2,\n+\tFETCH_NEGOTIATION_NULL = 3,\n };\n \n struct repo_settings {\ndiff --git a/t/t5554-null-fetch-negotiator.sh b/t/t5554-null-fetch-negotiator.sh\nnew file mode 100755\nindex 0000000000..09a8f0d608\n--- /dev/null\n+++ b/t/t5554-null-fetch-negotiator.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description='test null fetch negotiator'\n+. ./test-lib.sh\n+\n+test_expect_success 'null negotiator does not emit any \"have\"' '\n+\trm -f trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_commit -C server to_fetch &&\n+\n+\ttest_create_repo client &&\n+\ttest_commit -C client we_have &&\n+\n+\ttest_config -C client fetch.negotiationalgorithm null &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\n+\t! grep \"fetch> have\" trace &&\n+\tgrep \"fetch> done\" trace\n+'\n+\n+test_done\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403420","messageId":"00ad7dd875d8a6754c0ccf6f2789cec27aaa1d1d.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 2/7] fetch: allow refspecs specified through stdin","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:17Z","receivedAt":"2020-08-11T22:52:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a subsequent patch, partial clones will be taught to fetch missing\nobjects using a \"git fetch\" subprocess. Because the number of objects\nfetched may be too numerous to fit on the command line, teach \"fetch\" to\naccept refspecs passed through stdin.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-fetch.txt | 4 ++++\n builtin/fetch.c | 19 +++++++++++++++++--\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt\nindex 45b6d8e633..9067c2079e 100644\n--- a/Documentation/git-fetch.txt\n+++ b/Documentation/git-fetch.txt\n@@ -48,6 +48,10 @@ include::fetch-options.txt[]\n \n include::pull-fetch-param.txt[]\n \n+--stdin::\n+\tRead refspecs, one per line, from stdin in addition to those provided\n+\tas arguments. The \"tag \" format is not supported.\n+\n include::urls-remotes.txt[]\n \n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3ccf69753f..a5498646bf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -80,6 +80,7 @@ static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n+static int stdin_refspecs = 0;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -209,6 +210,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"check for forced-updates on all updated branches\")),\n \tOPT_BOOL(0, \"write-commit-graph\", &fetch_write_commit_graph,\n \t\t N_(\"write the commit-graph after fetching\")),\n+\tOPT_BOOL(0, \"stdin\", &stdin_refspecs,\n+\t\t N_(\"accept refspecs from stdin\")),\n \tOPT_END()\n };\n \n@@ -1684,7 +1687,8 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \treturn;\n }\n \n-static int fetch_one(struct remote *remote, int argc, const char **argv, int prune_tags_ok)\n+static int fetch_one(struct remote *remote, int argc, const char **argv,\n+\t\t int prune_tags_ok, int use_stdin_refspecs)\n {\n \tstruct refspec rs = REFSPEC_INIT_FETCH;\n \tint i;\n@@ -1741,6 +1745,13 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n \t\t}\n \t}\n \n+\tif (use_stdin_refspecs) {\n+\t\tstruct strbuf line = STRBUF_INIT;\n+\t\twhile (strbuf_getline_lf(&line, stdin) != EOF)\n+\t\t\trefspec_append(&rs, line.buf);\n+\t\tstrbuf_release(&line);\n+\t}\n+\n \tif (server_options.nr)\n \t\tgtransport->server_options = &server_options;\n \n@@ -1841,7 +1852,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n-\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok);\n+\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);\n \t} else {\n \t\tint max_children = max_jobs;\n \n@@ -1849,6 +1860,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \n+\t\tif (stdin_refspecs)\n+\t\t\tdie(_(\"--stdin can only be used when fetching \"\n+\t\t\t \"from one remote\"));\n+\n \t\tif (max_children < 0)\n \t\t\tmax_children = fetch_parallel_config;\n \n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403421","messageId":"8b4a522a13089ea144e922280f1c71a4ec3afa1f.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 3/7] fetch: avoid reading submodule config until needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:18Z","receivedAt":"2020-08-11T22:52:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach \"git fetch\" to avoid reading the submodule config until necessary.\nThis allows users to avoid the lazy-fetching of this potentially missing\nconfig file by specifying the --recurse-submodules=no command line\noption.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 10 ++++++++--\n submodule-config.c | 5 +++--\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a5498646bf..29db219c68 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1786,12 +1786,18 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tfree(anon);\n \t}\n \n-\tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n-\t\t\t\t &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n+\t\tint *sfjc = submodule_fetch_jobs_config == -1\n+\t\t\t ? &submodule_fetch_jobs_config : NULL;\n+\t\tint *rs = recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n+\t\t\t ? &recurse_submodules : NULL;\n+\n+\t\tfetch_config_from_gitmodules(sfjc, rs);\n+\t}\n \n \tif (deepen_relative) {\n \t\tif (deepen_relative < 0)\ndiff --git a/submodule-config.c b/submodule-config.c\nindex e175dfbc38..8d65273ed2 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -776,10 +776,11 @@ struct fetch_config {\n static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n {\n \tstruct fetch_config *config = cb;\n-\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n+\tif (!strcmp(var, \"submodule.fetchjobs\") && config->max_children) {\n \t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n \t\treturn 0;\n-\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n+\t} else if (!strcmp(var, \"fetch.recursesubmodules\") &&\n+\t\t config->recurse_submodules) {\n \t\t*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n \t\treturn 0;\n \t}\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403422","messageId":"77bc83e7f2c0a9c95e2ff31aa7a11295bbdf054c.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 4/7] fetch: only populate existing_refs if needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:19Z","receivedAt":"2020-08-11T22:52:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching tags, Git only writes tags that do not already exist in\nthe client repository. This necessitates an iteration over all the refs,\nbut fetch performs this iteration even if no tags are fetched.\n\nThis issue is more severe in a partial clone because the iteration over\nrefs also checks that the targets of those refs are present,\nnecessitating a lazy fetch if the target is missing.\n\nTherefore, iterate over the refs only when necessary. The user can avoid\nthis iteration by refraining from fetching tags, for example, by passing\n--no-tags as an argument. A subsequent patch will also teach Git to use\n\"git fetch\" to lazy-fetch missing objects in a partial clone, thus also\nmaking use of this change.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 29db219c68..6460ce3f4e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -449,6 +449,7 @@ static struct ref *get_ref_map(struct remote *remote,\n \tstruct ref *orefs = NULL, **oref_tail = &orefs;\n \n \tstruct hashmap existing_refs;\n+\tint existing_refs_populated = 0;\n \n \tif (rs->nr) {\n \t\tstruct refspec *fetch_refspec;\n@@ -542,15 +543,18 @@ static struct ref *get_ref_map(struct remote *remote,\n \n \tref_map = ref_remove_duplicates(ref_map);\n \n-\trefname_hash_init(&existing_refs);\n-\tfor_each_ref(add_one_refname, &existing_refs);\n-\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->peer_ref) {\n \t\t\tconst char *refname = rm->peer_ref->name;\n \t\t\tstruct refname_hash_entry *peer_item;\n \t\t\tunsigned int hash = strhash(refname);\n \n+\t\t\tif (!existing_refs_populated) {\n+\t\t\t\trefname_hash_init(&existing_refs);\n+\t\t\t\tfor_each_ref(add_one_refname, &existing_refs);\n+\t\t\t\texisting_refs_populated = 1;\n+\t\t\t}\n+\n \t\t\tpeer_item = hashmap_get_entry_from_hash(&existing_refs,\n \t\t\t\t\t\thash, refname,\n \t\t\t\t\t\tstruct refname_hash_entry, ent);\n@@ -560,7 +564,8 @@ static struct ref *get_ref_map(struct remote *remote,\n \t\t\t}\n \t\t}\n \t}\n-\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n+\tif (existing_refs_populated)\n+\t\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n \n \treturn ref_map;\n }\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403423","messageId":"d42e98ff0760932cffb021eb680b76da74e4461f.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 5/7] fetch-pack: do not lazy-fetch during ref iteration","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:20Z","receivedAt":"2020-08-11T22:52:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach \"fetch-pack\" not to lazy fetch whenever iterating over refs. This\nis done by using the raw form of ref iteration and by dereferencing tags\nourselves.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 79 ++++++++++++++++++++++------------------\n t/t5616-partial-clone.sh | 20 ++++++++++\n 2 files changed, 64 insertions(+), 35 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 80fb3bd899..707bbc31fd 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -108,24 +108,48 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n+\t\t\t\t\t int mark_tags_complete)\n+{\n+\tenum object_type type;\n+\tstruct object_info info = { .typep = &type };\n+\n+\twhile (1) {\n+\t\tif (oid_object_info_extended(the_repository, oid, &info,\n+\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))\n+\t\t\treturn NULL;\n+\t\tif (type == OBJ_TAG) {\n+\t\t\tstruct tag *tag = (struct tag *)\n+\t\t\t\tparse_object(the_repository, oid);\n+\n+\t\t\tif (!tag->tagged)\n+\t\t\t\treturn NULL;\n+\t\t\tif (mark_tags_complete)\n+\t\t\t\ttag->object.flags |= COMPLETE;\n+\t\t\toid = &tag->tagged->oid;\n+\t\t} else {\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tif (type == OBJ_COMMIT)\n+\t\treturn (struct commit *) parse_object(the_repository, oid);\n+\treturn NULL;\n+}\n+\n static int rev_list_insert_ref(struct fetch_negotiator *negotiator,\n-\t\t\t const char *refname,\n \t\t\t const struct object_id *oid)\n {\n-\tstruct object *o = deref_tag(the_repository,\n-\t\t\t\t parse_object(the_repository, oid),\n-\t\t\t\t refname, 0);\n-\n-\tif (o && o->type == OBJ_COMMIT)\n-\t\tnegotiator->add_tip(negotiator, (struct commit *)o);\n+\tstruct commit *c = deref_without_lazy_fetch(oid, 0);\n \n+\tif (c)\n+\t\tnegotiator->add_tip(negotiator, c);\n \treturn 0;\n }\n \n static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,\n \t\t\t\t int flag, void *cb_data)\n {\n-\treturn rev_list_insert_ref(cb_data, refname, oid);\n+\treturn rev_list_insert_ref(cb_data, oid);\n }\n \n enum ack_type {\n@@ -201,7 +225,7 @@ static void send_request(struct fetch_pack_args *args,\n static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n \t\t\t\t\tstruct object *obj)\n {\n-\trev_list_insert_ref(negotiator, NULL, &obj->oid);\n+\trev_list_insert_ref(negotiator, &obj->oid);\n }\n \n #define INITIAL_FLUSH 16\n@@ -230,13 +254,12 @@ static void mark_tips(struct fetch_negotiator *negotiator,\n \tint i;\n \n \tif (!negotiation_tips) {\n-\t\tfor_each_ref(rev_list_insert_ref_oid, negotiator);\n+\t\tfor_each_rawref(rev_list_insert_ref_oid, negotiator);\n \t\treturn;\n \t}\n \n \tfor (i = 0; i < negotiation_tips->nr; i++)\n-\t\trev_list_insert_ref(negotiator, NULL,\n-\t\t\t\t &negotiation_tips->oid[i]);\n+\t\trev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);\n \treturn;\n }\n \n@@ -503,21 +526,11 @@ static struct commit_list *complete;\n \n static int mark_complete(const struct object_id *oid)\n {\n-\tstruct object *o = parse_object(the_repository, oid);\n-\n-\twhile (o && o->type == OBJ_TAG) {\n-\t\tstruct tag *t = (struct tag *) o;\n-\t\tif (!t->tagged)\n-\t\t\tbreak; /* broken repository */\n-\t\to->flags |= COMPLETE;\n-\t\to = parse_object(the_repository, &t->tagged->oid);\n-\t}\n-\tif (o && o->type == OBJ_COMMIT) {\n-\t\tstruct commit *commit = (struct commit *)o;\n-\t\tif (!(commit->object.flags & COMPLETE)) {\n-\t\t\tcommit->object.flags |= COMPLETE;\n-\t\t\tcommit_list_insert(commit, &complete);\n-\t\t}\n+\tstruct commit *commit = deref_without_lazy_fetch(oid, 1);\n+\n+\tif (commit && !(commit->object.flags & COMPLETE)) {\n+\t\tcommit->object.flags |= COMPLETE;\n+\t\tcommit_list_insert(commit, &complete);\n \t}\n \treturn 0;\n }\n@@ -702,7 +715,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t */\n \ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n-\t\tfor_each_ref(mark_complete_oid, NULL);\n+\t\tfor_each_rawref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n \t\tcommit_list_sort_by_date(&complete);\n \t\tif (cutoff)\n@@ -716,16 +729,12 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t */\n \ttrace2_region_enter(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n-\t\tstruct object *o = deref_tag(the_repository,\n-\t\t\t\t\t lookup_object(the_repository,\n-\t\t\t\t\t &ref->old_oid),\n-\t\t\t\t\t NULL, 0);\n+\t\tstruct commit *c = deref_without_lazy_fetch(&ref->old_oid, 0);\n \n-\t\tif (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))\n+\t\tif (!c || !(c->object.flags & COMPLETE))\n \t\t\tcontinue;\n \n-\t\tnegotiator->known_common(negotiator,\n-\t\t\t\t\t (struct commit *)o);\n+\t\tnegotiator->known_common(negotiator, c);\n \t}\n \ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8a27452a51..e53492d595 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -384,6 +384,26 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n+test_expect_success 'fetch does not lazy-fetch missing targets of its refs' '\n+\trm -rf server client trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\ttest_commit -C server foo &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\t# Make all refs point to nothing by deleting all objects.\n+\trm client/.git/objects/pack/* &&\n+\n+\ttest_commit -C server bar &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \\\n+\t\t--no-tags --recurse-submodules=no \\\n+\t\torigin refs/tags/bar &&\n+\tFOO_HASH=$(git -C server rev-parse foo) &&\n+\t! grep \"want $FOO_HASH\" trace\n+'\n+\n # The following two tests must be in this order. It is important that\n # the srv.bare repository did not have tags during clone, but has tags\n # in the fetch.\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403424","messageId":"55d2e5a4cccee0ae719f4210c2cbeeb6a691cf2f.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 6/7] promisor-remote: lazy-fetch objects in subprocess","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:21Z","receivedAt":"2020-08-11T22:52:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git to lazy-fetch missing objects in a subprocess instead of doing\nit in-process. This allows any fatal errors that occur during the fetch\nto be isolated and converted into an error return value, instead of\ncausing the current command being run to terminate.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/partial-clone.txt | 13 ++-----\n promisor-remote.c | 46 +++++++++++------------\n t/t0410-partial-clone.sh | 2 +-\n t/t4067-diff-partial-clone.sh | 8 ++--\n t/t5601-clone.sh | 2 +-\n 5 files changed, 30 insertions(+), 41 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex b9e17e7a28..0780d30cac 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -171,20 +171,13 @@ additional flag.\n Fetching Missing Objects\n ------------------------\n \n-- Fetching of objects is done using the existing transport mechanism using\n- transport_fetch_refs(), setting a new transport option\n- TRANS_OPT_NO_DEPENDENTS to indicate that only the objects themselves are\n- desired, not any object that they refer to.\n-+\n-Because some transports invoke fetch_pack() in the same process, fetch_pack()\n-has been updated to not use any object flags when the corresponding argument\n-(no_dependents) is set.\n+- Fetching of objects is done by invoking a \"git fetch\" subprocess.\n \n - The local repository sends a request with the hashes of all requested\n- objects as \"want\" lines, and does not perform any packfile negotiation.\n+ objects, and does not perform any packfile negotiation.\n It then receives a packfile.\n \n-- Because we are reusing the existing fetch-pack mechanism, fetching\n+- Because we are reusing the existing fetch mechanism, fetching\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex baaea12fd6..6e647610e9 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -3,6 +3,7 @@\n #include \"promisor-remote.h\"\n #include \"config.h\"\n #include \"transport.h\"\n+#include \"argv-array.h\"\n \n static char *repository_format_partial_clone;\n static const char *core_partial_clone_filter_default;\n@@ -12,39 +13,34 @@ void set_repository_format_partial_clone(char *partial_clone)\n \trepository_format_partial_clone = xstrdup_or_null(partial_clone);\n }\n \n-static int fetch_refs(const char *remote_name, struct ref *ref)\n-{\n-\tstruct remote *remote;\n-\tstruct transport *transport;\n-\tint res;\n-\n-\tremote = remote_get(remote_name);\n-\tif (!remote->url[0])\n-\t\tdie(_(\"Remote with no URL\"));\n-\ttransport = transport_get(remote, remote->url[0]);\n-\n-\ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n-\ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n-\tres = transport_fetch_refs(transport, ref);\n-\n-\treturn res;\n-}\n-\n static int fetch_objects(const char *remote_name,\n \t\t\t const struct object_id *oids,\n \t\t\t int oid_nr)\n {\n-\tstruct ref *ref = NULL;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tint i;\n+\tFILE *child_in;\n+\n+\tchild.git_cmd = 1;\n+\tchild.in = -1;\n+\targv_array_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=null\",\n+\t\t\t \"fetch\", remote_name, \"--no-tags\",\n+\t\t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n+\t\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n+\tif (start_command(&child))\n+\t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n+\tchild_in = xfdopen(child.in, \"w\");\n \n \tfor (i = 0; i < oid_nr; i++) {\n-\t\tstruct ref *new_ref = alloc_ref(oid_to_hex(&oids[i]));\n-\t\toidcpy(&new_ref->old_oid, &oids[i]);\n-\t\tnew_ref->exact_oid = 1;\n-\t\tnew_ref->next = ref;\n-\t\tref = new_ref;\n+\t\tif (fputs(oid_to_hex(&oids[i]), child_in) < 0)\n+\t\t\tdie_errno(_(\"promisor-remote: could not write to fetch subprocess\"));\n+\t\tif (fputc('\\n', child_in) < 0)\n+\t\t\tdie_errno(_(\"promisor-remote: could not write to fetch subprocess\"));\n \t}\n-\treturn fetch_refs(remote_name, ref);\n+\n+\tif (fclose(child_in) < 0)\n+\t\tdie_errno(_(\"promisor-remote: could not close stdin to fetch subprocess\"));\n+\treturn finish_command(&child) ? -1 : 0;\n }\n \n static struct promisor_remote *promisors;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be..3e454f934e 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -203,7 +203,7 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \trm -rf repo/.git/objects/* &&\n \trm -f trace &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C repo cat-file -p \"$HASH\" &&\n-\tgrep \"git< fetch=.*ref-in-want\" trace\n+\tgrep \"fetch< fetch=.*ref-in-want\" trace\n '\n \n test_expect_success 'fetching of missing objects from another promisor remote' '\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex ef8e0e9cb0..804f2a82e8 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -20,7 +20,7 @@ test_expect_success 'git show batches blobs' '\n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -44,7 +44,7 @@ test_expect_success 'diff batches blobs' '\n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -127,7 +127,7 @@ test_expect_success 'diff with rename detection batches blobs' '\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD >out &&\n \tgrep \":100644 100644.*R[0-9][0-9][0-9].*b.*c\" out &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -175,7 +175,7 @@ test_expect_success 'diff --break-rewrites fetches only if necessary, and batche\n \t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n \t# end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb7..f82b0dbb5a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -702,7 +702,7 @@ test_expect_success 'batch missing blob request during checkout' '\n \t# Ensure that there is only one negotiation by checking that there is\n \t# only \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client checkout HEAD^ &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403425","messageId":"e8f16d69089a5011c355d5939c56fa53b7a1eb2d.1597184949.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"[PATCH v2 7/7] fetch-pack: remove no_dependents code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-11T22:52:22Z","receivedAt":"2020-08-11T22:52:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Now that Git has switched to using a subprocess to lazy-fetch missing\nobjects, remove the no_dependents code as it is no longer used.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch-pack.c | 4 --\n fetch-pack.c | 110 ++++++++++++-------------------------------\n fetch-pack.h | 14 ------\n remote-curl.c | 6 ---\n transport.c | 4 --\n transport.h | 7 ---\n 6 files changed, 30 insertions(+), 115 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bbb5c96167..58b7c1fbdc 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,10 +153,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(\"--no-dependents\", arg)) {\n-\t\t\targs.no_dependents = 1;\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 707bbc31fd..3212957dae 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -285,10 +285,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n-\tif (!args->no_dependents) {\n-\t\tmark_tips(negotiator, args->negotiation_tips);\n-\t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n-\t}\n+\tmark_tips(negotiator, args->negotiation_tips);\n+\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n \n \tfetching = 0;\n \tfor ( ; refs ; refs = refs->next) {\n@@ -305,12 +303,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t * We use lookup_object here because we are only\n \t\t * interested in the case we *know* the object is\n \t\t * reachable and we have already scanned it.\n-\t\t *\n-\t\t * Do this only if args->no_dependents is false (if it is true,\n-\t\t * we cannot trust the object flags).\n \t\t */\n-\t\tif (!args->no_dependents &&\n-\t\t ((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n \t\t\t\t(o->flags & COMPLETE)) {\n \t\t\tcontinue;\n \t\t}\n@@ -410,8 +404,6 @@ static int find_common(struct fetch_negotiator *negotiator,\n \ttrace2_region_enter(\"fetch-pack\", \"negotiation_v0_v1\", the_repository);\n \tflushes = 0;\n \tretval = -1;\n-\tif (args->no_dependents)\n-\t\tgoto done;\n \twhile ((oid = negotiator->next(negotiator))) {\n \t\tpacket_buf_write(&req_buf, \"have %s\\n\", oid_to_hex(oid));\n \t\tprint_verbose(args, \"have %s\", oid_to_hex(oid));\n@@ -666,9 +658,7 @@ struct loose_object_iter {\n \n /*\n * Mark recent commits available locally and reachable from a local ref as\n- * COMPLETE. If args->no_dependents is false, also mark COMPLETE remote refs as\n- * COMMON_REF (otherwise, we are not planning to participate in negotiation, and\n- * thus do not need COMMON_REF marks).\n+ * COMPLETE.\n *\n * The cutoff time for recency is determined by this heuristic: it is the\n * earliest commit time of the objects in refs that are commits and that we know\n@@ -969,12 +959,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \n-\tif (args->no_dependents) {\n-\t\tnegotiator = NULL;\n-\t} else {\n-\t\tnegotiator = &negotiator_alloc;\n-\t\tfetch_negotiator_init(r, negotiator);\n-\t}\n+\tnegotiator = &negotiator_alloc;\n+\tfetch_negotiator_init(r, negotiator);\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n@@ -1062,15 +1048,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (!server_supports_hash(the_hash_algo->name, NULL))\n \t\tdie(_(\"Server does not support this repository's object format\"));\n \n-\tif (!args->no_dependents) {\n-\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n-\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\tif (everything_local(args, &ref)) {\n-\t\t\tpacket_flush(fd[1]);\n-\t\t\tgoto all_done;\n-\t\t}\n-\t} else {\n-\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\tmark_complete_and_common_ref(negotiator, args, &ref);\n+\tfilter_refs(args, &ref, sought, nr_sought);\n+\tif (everything_local(args, &ref)) {\n+\t\tpacket_flush(fd[1]);\n+\t\tgoto all_done;\n \t}\n \tif (find_common(negotiator, args, fd, &oid, ref) < 0)\n \t\tif (!args->keep_pack)\n@@ -1119,7 +1101,7 @@ static void add_shallow_requests(struct strbuf *req_buf,\n \t\tpacket_buf_write(req_buf, \"deepen-relative\\n\");\n }\n \n-static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)\n+static void add_wants(const struct ref *wants, struct strbuf *req_buf)\n {\n \tint use_ref_in_want = server_supports_feature(\"fetch\", \"ref-in-want\", 0);\n \n@@ -1136,12 +1118,8 @@ static void add_wants(int no_dependents, const struct ref *wants, struct strbuf\n \t\t * We use lookup_object here because we are only\n \t\t * interested in the case we *know* the object is\n \t\t * reachable and we have already scanned it.\n-\t\t *\n-\t\t * Do this only if args->no_dependents is false (if it is true,\n-\t\t * we cannot trust the object flags).\n \t\t */\n-\t\tif (!no_dependents &&\n-\t\t ((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n \t\t (o->flags & COMPLETE)) {\n \t\t\tcontinue;\n \t\t}\n@@ -1275,19 +1253,14 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t}\n \n \t/* add wants */\n-\tadd_wants(args->no_dependents, wants, &req_buf);\n+\tadd_wants(wants, &req_buf);\n \n-\tif (args->no_dependents) {\n-\t\tpacket_buf_write(&req_buf, \"done\");\n-\t\tret = 1;\n-\t} else {\n-\t\t/* Add all of the common commits we've found in previous rounds */\n-\t\tadd_common(&req_buf, common);\n+\t/* Add all of the common commits we've found in previous rounds */\n+\tadd_common(&req_buf, common);\n \n-\t\t/* Add initial haves */\n-\t\tret = add_haves(negotiator, seen_ack, &req_buf,\n-\t\t\t\thaves_to_send, in_vain);\n-\t}\n+\t/* Add initial haves */\n+\tret = add_haves(negotiator, seen_ack, &req_buf,\n+\t\t\thaves_to_send, in_vain);\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n@@ -1547,12 +1520,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n \tint i;\n \n-\tif (args->no_dependents) {\n-\t\tnegotiator = NULL;\n-\t} else {\n-\t\tnegotiator = &negotiator_alloc;\n-\t\tfetch_negotiator_init(r, negotiator);\n-\t}\n+\tnegotiator = &negotiator_alloc;\n+\tfetch_negotiator_init(r, negotiator);\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -1576,21 +1545,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\targs->deepen = 1;\n \n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n-\t\t\tif (!args->no_dependents) {\n-\t\t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n-\t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\t\tif (everything_local(args, &ref))\n-\t\t\t\t\tstate = FETCH_DONE;\n-\t\t\t\telse\n-\t\t\t\t\tstate = FETCH_SEND_REQUEST;\n-\n-\t\t\t\tmark_tips(negotiator, args->negotiation_tips);\n-\t\t\t\tfor_each_cached_alternate(negotiator,\n-\t\t\t\t\t\t\t insert_one_alternate_object);\n-\t\t\t} else {\n-\t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n+\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\t\t\tif (everything_local(args, &ref))\n+\t\t\t\tstate = FETCH_DONE;\n+\t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-\t\t\t}\n+\n+\t\t\tmark_tips(negotiator, args->negotiation_tips);\n+\t\t\tfor_each_cached_alternate(negotiator,\n+\t\t\t\t\t\t insert_one_alternate_object);\n \t\t\tbreak;\n \t\tcase FETCH_SEND_REQUEST:\n \t\t\tif (!negotiation_started) {\n@@ -1911,20 +1875,6 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n-\tif (args->no_dependents && !args->filter_options.choice) {\n-\t\t/*\n-\t\t * The protocol does not support requesting that only the\n-\t\t * wanted objects be sent, so approximate this by setting a\n-\t\t * \"blob:none\" filter if no filter is already set. This works\n-\t\t * for all object types: note that wanted blobs will still be\n-\t\t * sent because they are directly specified as a \"want\".\n-\t\t *\n-\t\t * NEEDSWORK: Add an option in the protocol to request that\n-\t\t * only the wanted objects be sent, and implement it.\n-\t\t */\n-\t\tparse_list_objects_filter(&args->filter_options, \"blob:none\");\n-\t}\n-\n \tif (version != protocol_v2 && !ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 85d1e39fe7..bbe2938059 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -42,20 +42,6 @@ struct fetch_pack_args {\n \tunsigned deepen:1;\n \tunsigned from_promisor:1;\n \n-\t/*\n-\t * Attempt to fetch only the wanted objects, and not any objects\n-\t * referred to by them. Due to protocol limitations, extraneous\n-\t * objects may still be included. (When fetching non-blob\n-\t * objects, only blobs are excluded; when fetching a blob, the\n-\t * blob itself will still be sent. The client does not need to\n-\t * know whether a wanted object is a blob or not.)\n-\t *\n-\t * If 1, fetch_pack() will also not modify any object flags.\n-\t * This allows fetch_pack() to safely be called by any function,\n-\t * regardless of which object flags it uses (if any).\n-\t */\n-\tunsigned no_dependents:1;\n-\n \t/*\n \t * Because fetch_pack() overwrites the shallow file upon a\n \t * successful deepening non-clone fetch, if this struct\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 5cbc6e5002..a0c81a64bc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -40,7 +40,6 @@ struct options {\n \t\tpush_cert : 2,\n \t\tdeepen_relative : 1,\n \t\tfrom_promisor : 1,\n-\t\tno_dependents : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1;\n \tconst struct git_hash_algo *hash_algo;\n@@ -186,9 +185,6 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n-\t} else if (!strcmp(name, \"no-dependents\")) {\n-\t\toptions.no_dependents = 1;\n-\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n@@ -1171,8 +1167,6 @@ static int fetch_git(struct discovery *heads,\n \t\targv_array_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\targv_array_push(&args, \"--from-promisor\");\n-\tif (options.no_dependents)\n-\t\targv_array_push(&args, \"--no-dependents\");\n \tif (options.filter)\n \t\targv_array_pushf(&args, \"--filter=%s\", options.filter);\n \targv_array_push(&args, url.buf);\ndiff --git a/transport.c b/transport.c\nindex b41386eccb..32e1f21f0c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -232,9 +232,6 @@ static int set_git_option(struct git_transport_options *opts,\n \t} else if (!strcmp(name, TRANS_OPT_FROM_PROMISOR)) {\n \t\topts->from_promisor = !!value;\n \t\treturn 0;\n-\t} else if (!strcmp(name, TRANS_OPT_NO_DEPENDENTS)) {\n-\t\topts->no_dependents = !!value;\n-\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_LIST_OBJECTS_FILTER)) {\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n@@ -359,7 +356,6 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.cloning = transport->cloning;\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n-\targs.no_dependents = data->options.no_dependents;\n \targs.filter_options = data->options.filter_options;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\ndiff --git a/transport.h b/transport.h\nindex b3c30133ea..7aa1f33145 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,7 +16,6 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned deepen_relative : 1;\n \tunsigned from_promisor : 1;\n-\tunsigned no_dependents : 1;\n \n \t/*\n \t * If this transport supports connect or stateless-connect,\n@@ -201,12 +200,6 @@ void transport_check_allowed(const char *type);\n /* Indicate that these objects are being fetched by a promisor */\n #define TRANS_OPT_FROM_PROMISOR \"from-promisor\"\n \n-/*\n- * Indicate that only the objects wanted need to be fetched, not their\n- * dependents\n- */\n-#define TRANS_OPT_NO_DEPENDENTS \"no-dependents\"\n-\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403445","messageId":"9d969367-72ba-828b-ca75-29eb532cecee@gmail.com","threadId":"53920","inReplyTo":"cover.1597184948.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/7] Lazy fetch with subprocess","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-12T12:51:54Z","receivedAt":"2020-08-12T12:51:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/11/2020 6:52 PM, Jonathan Tan wrote:\n> - A way to prevent a promisor-remote fetch from invoking another\n> promisor-remote fetch (use a file as a lock?)\n\nIn the VFS for Git world, the C# layer frequently needs to ask\nGit for some information, but doesn't want to trigger a download\nof a missing object. We use a config option when running our\ncommand that disables the GVFS protocol for that command.\n\nIt's possible that a config option _or_ environment variable would\nsuffice instead of adding another file-based lock.\n\nThanks,\n-Stolee\n"},{"id":"403446","messageId":"2f6b3e7e-8f83-bfde-ab63-1eed08ff32b5@gmail.com","threadId":"53920","inReplyTo":"35bdd372ae3691f54775dd616576e8ed6d68f1d3.1597184949.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/7] negotiator/null: add null fetch negotiator","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-08-12T12:55:14Z","receivedAt":"2020-08-12T12:55:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/11/2020 6:52 PM, Jonathan Tan wrote:\n> Add a null fetch negotiator. \n\nI understand the value of this negotiator. I'm concerned about using\n\"null\" as the name, since it has a clear relationship to zero-valued\npointers and that's not what is happening. (My gut feeling starting the\npatch was that some function pointers would be NULL or something.)\n\nInstead, might I recommend \"noop\" or \"no_op\" in place of \"null\" here?\n\nThanks,\n-Stolee\n\n"},{"id":"403470","messageId":"xmqqsgcr7r1p.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"2f6b3e7e-8f83-bfde-ab63-1eed08ff32b5@gmail.com","subject":"Re: [PATCH v2 1/7] negotiator/null: add null fetch negotiator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T16:44:02Z","receivedAt":"2020-08-12T16:44:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 8/11/2020 6:52 PM, Jonathan Tan wrote:\n>> Add a null fetch negotiator. \n>\n> I understand the value of this negotiator. I'm concerned about using\n> \"null\" as the name, since it has a clear relationship to zero-valued\n> pointers and that's not what is happening. (My gut feeling starting the\n> patch was that some function pointers would be NULL or something.)\n>\n> Instead, might I recommend \"noop\" or \"no_op\" in place of \"null\" here?\n\nPersonally I am OK with null [*], but noop is also fine.\n\n\tSide note. I actually would find it good to establish the\n\tpattern that something that does not use NULL pointer as its\n\timplementation detail can be called null if \"null-ness\" of\n\tits behaviour is its defining characteristics.\n\nThanks.\n\n"},{"id":"403477","messageId":"20200812172943.1253226-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqsgcr7r1p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/7] negotiator/null: add null fetch negotiator","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-12T17:29:43Z","receivedAt":"2020-08-12T17:29:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Derrick Stolee writes:\n> \n> > On 8/11/2020 6:52 PM, Jonathan Tan wrote:\n> >> Add a null fetch negotiator. \n> >\n> > I understand the value of this negotiator. I'm concerned about using\n> > \"null\" as the name, since it has a clear relationship to zero-valued\n> > pointers and that's not what is happening. (My gut feeling starting the\n> > patch was that some function pointers would be NULL or something.)\n> >\n> > Instead, might I recommend \"noop\" or \"no_op\" in place of \"null\" here?\n> \n> Personally I am OK with null [*], but noop is also fine.\n> \n> \tSide note. I actually would find it good to establish the\n> \tpattern that something that does not use NULL pointer as its\n> \timplementation detail can be called null if \"null-ness\" of\n> \tits behaviour is its defining characteristics.\n> \n> Thanks.\n\nOK, in a future version I'll go with \"noop\".\n"},{"id":"403478","messageId":"xmqqo8nf7opg.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"8b4a522a13089ea144e922280f1c71a4ec3afa1f.1597184949.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 3/7] fetch: avoid reading submodule config until needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T17:34:35Z","receivedAt":"2020-08-12T17:34:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Teach \"git fetch\" to avoid reading the submodule config until necessary.\n> This allows users to avoid the lazy-fetching of this potentially missing\n> config file by specifying the --recurse-submodules=no command line\n> option.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/fetch.c | 10 ++++++++--\n> submodule-config.c | 5 +++--\n> 2 files changed, 11 insertions(+), 4 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index a5498646bf..29db219c68 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1786,12 +1786,18 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n> \t\tfree(anon);\n> \t}\n> \n> -\tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n> -\t\t\t\t &recurse_submodules);\n> \tgit_config(git_fetch_config, NULL);\n> \n> \targc = parse_options(argc, argv, prefix,\n> \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n> +\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n> +\t\tint *sfjc = submodule_fetch_jobs_config == -1\n> +\t\t\t ? &submodule_fetch_jobs_config : NULL;\n> +\t\tint *rs = recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n> +\t\t\t ? &recurse_submodules : NULL;\n> +\n> +\t\tfetch_config_from_gitmodules(sfjc, rs);\n> +\t}\n\nHmph. Instead of calling fetch_config_from... upfront, first\nreading the fetch configuration and also command line options to see\nif we are told to recurse, and only enter the new \"if()\" statement\ndoes make sense---we'll avoid calling fetch_config_from... when we\nare told not to recurse into submodules.\n\nBut it is not quite clear why the two parameters to the function can\nnow be conditional, and what benefit we gain by doing so, in the\nbody of the new \"if()\" statement. Don't you need to explain to your\nfuture peer developers what is going on here in the log message?\n\n> \tif (deepen_relative) {\n> \t\tif (deepen_relative < 0)\n> diff --git a/submodule-config.c b/submodule-config.c\n> index e175dfbc38..8d65273ed2 100644\n> --- a/submodule-config.c\n> +++ b/submodule-config.c\n> @@ -776,10 +776,11 @@ struct fetch_config {\n> static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n> {\n> \tstruct fetch_config *config = cb;\n> -\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n> +\tif (!strcmp(var, \"submodule.fetchjobs\") && config->max_children) {\n> \t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n> \t\treturn 0;\n\nBecause the new \"if()\" statement in cmd_fetch can pass NULL to\neither of the two parameters to fetch_config_from_gitmodules(), it\nnow becomes possible for this function to get an instance of \"struct\nfetch_config\" with .max_children field set to NULL.\n\nThe above justifies why assignment to *(config->max_children) must\nbe skipped when .max_children is NULL, but it does not justify the\nnew code, where .max_children==NULL does not stop the if/else if/\ncascade.\n\n\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n\t\tif (config->max_children)\n\t\t\t*(config->max_children) = parse...\n\t\treturn 0;\n\t}\n\nWhy sfjc or rs is allowed to be NULL in certain cases, and why doing\nso is a good idea, should be explained nevertheless, though.\n\n> -\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n> +\t} else if (!strcmp(var, \"fetch.recursesubmodules\") &&\n> +\t\t config->recurse_submodules) {\n> \t\t*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n> \t\treturn 0;\n\nLikewise.\n\nThanks.\n\n"},{"id":"403480","messageId":"xmqqk0y37n8p.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"77bc83e7f2c0a9c95e2ff31aa7a11295bbdf054c.1597184949.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 4/7] fetch: only populate existing_refs if needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T18:06:14Z","receivedAt":"2020-08-12T18:06:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When fetching tags, Git only writes tags that do not already exist in\n> the client repository. This necessitates an iteration over all the refs,\n> but fetch performs this iteration even if no tags are fetched.\n\nHmph, the change itself mechanically makes sense (i.e. if the\nref_map has no entry with peer_ref defined, existing_refs hashmap is\nnever looked up, so there is no reason to populate it), but the\nexplanation of log message, especially the part that places strees\non tags, does not. Wouldn't a plain vanilla \"git fetch\" with no\nargument that uses the remote.origin.fetch populated with standard\nrefspecs to maintain remote-tracking branches use this hash?\n\nI think the readers need to be somehow made aware of the fact that\nthe author of this commit message is concentrated primarily on fetch\nused as a mechanism to grab specific objects, often used by lazy\nfetch, without touching any remote-tracking refs. Because tag\nfollowing is on by default (which is a convenient default for the\nplain vanilla fetches that updates remote-tracking branches),\nhowever, such a \"these specific objects only\" fetch still tries to\ntrigger the auto following of tags, and necessitates \"--no-tags\" to\ntake advantage of the optimization proposed by this patch.\n\nSo nothing written in the proposed log message is incorrect per-se,\nbut it is not very friendly to readers without clarivoyance.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 29db219c68..6460ce3f4e 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -449,6 +449,7 @@ static struct ref *get_ref_map(struct remote *remote,\n> \tstruct ref *orefs = NULL, **oref_tail = &orefs;\n> \n> \tstruct hashmap existing_refs;\n> +\tint existing_refs_populated = 0;\n> \n> \tif (rs->nr) {\n> \t\tstruct refspec *fetch_refspec;\n> @@ -542,15 +543,18 @@ static struct ref *get_ref_map(struct remote *remote,\n> \n> \tref_map = ref_remove_duplicates(ref_map);\n> \n> -\trefname_hash_init(&existing_refs);\n> -\tfor_each_ref(add_one_refname, &existing_refs);\n> -\n> \tfor (rm = ref_map; rm; rm = rm->next) {\n> \t\tif (rm->peer_ref) {\n> \t\t\tconst char *refname = rm->peer_ref->name;\n> \t\t\tstruct refname_hash_entry *peer_item;\n> \t\t\tunsigned int hash = strhash(refname);\n> \n> +\t\t\tif (!existing_refs_populated) {\n> +\t\t\t\trefname_hash_init(&existing_refs);\n> +\t\t\t\tfor_each_ref(add_one_refname, &existing_refs);\n> +\t\t\t\texisting_refs_populated = 1;\n> +\t\t\t}\n> +\n> \t\t\tpeer_item = hashmap_get_entry_from_hash(&existing_refs,\n> \t\t\t\t\t\thash, refname,\n> \t\t\t\t\t\tstruct refname_hash_entry, ent);\n> @@ -560,7 +564,8 @@ static struct ref *get_ref_map(struct remote *remote,\n> \t\t\t}\n> \t\t}\n> \t}\n> -\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n> +\tif (existing_refs_populated)\n> +\t\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n> \n> \treturn ref_map;\n> }\n"},{"id":"403482","messageId":"xmqqft8r7mcd.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"d42e98ff0760932cffb021eb680b76da74e4461f.1597184949.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 5/7] fetch-pack: do not lazy-fetch during ref iteration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T18:25:38Z","receivedAt":"2020-08-12T18:25:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Teach \"fetch-pack\" not to lazy fetch whenever iterating over\n> refs. \n\nDon't readers need the reason why is it a good thing to do explained\nhere?\n\n> This\n> is done by using the raw form of ref iteration and by dereferencing tags\n> ourselves.\n\nHmph. The way this patch implements deref_without_lazy_fetch()\nmakes it hard to reuse in other contexts, as it mixes what is done\nby mark_complete(), which is very much \"git fetch\" specific.\n\nWould \"git fetch\" be the only user potentially benefit from being\nable to dereference a tag that already exists locally and ignore\nmissing ones? I wonder if teaching deref_tag() a new flag would be\na better alternative to keep the separation of concerns at different\nlayers.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 79 ++++++++++++++++++++++------------------\n> t/t5616-partial-clone.sh | 20 ++++++++++\n> 2 files changed, 64 insertions(+), 35 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 80fb3bd899..707bbc31fd 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -108,24 +108,48 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n> \t\tcb(negotiator, cache.items[i]);\n> }\n> \n> +static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n> +\t\t\t\t\t int mark_tags_complete)\n> +{\n> +\tenum object_type type;\n> +\tstruct object_info info = { .typep = &type };\n> +\n> +\twhile (1) {\n> +\t\tif (oid_object_info_extended(the_repository, oid, &info,\n> +\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))\n> +\t\t\treturn NULL;\n> +\t\tif (type == OBJ_TAG) {\n> +\t\t\tstruct tag *tag = (struct tag *)\n> +\t\t\t\tparse_object(the_repository, oid);\n> +\n> +\t\t\tif (!tag->tagged)\n> +\t\t\t\treturn NULL;\n> +\t\t\tif (mark_tags_complete)\n> +\t\t\t\ttag->object.flags |= COMPLETE;\n> +\t\t\toid = &tag->tagged->oid;\n> +\t\t} else {\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tif (type == OBJ_COMMIT)\n> +\t\treturn (struct commit *) parse_object(the_repository, oid);\n> +\treturn NULL;\n> +}\n> +\n> static int rev_list_insert_ref(struct fetch_negotiator *negotiator,\n> -\t\t\t const char *refname,\n> \t\t\t const struct object_id *oid)\n> {\n> -\tstruct object *o = deref_tag(the_repository,\n> -\t\t\t\t parse_object(the_repository, oid),\n> -\t\t\t\t refname, 0);\n> -\n> -\tif (o && o->type == OBJ_COMMIT)\n> -\t\tnegotiator->add_tip(negotiator, (struct commit *)o);\n> +\tstruct commit *c = deref_without_lazy_fetch(oid, 0);\n> \n> +\tif (c)\n> +\t\tnegotiator->add_tip(negotiator, c);\n> \treturn 0;\n> }\n> \n> static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,\n> \t\t\t\t int flag, void *cb_data)\n> {\n> -\treturn rev_list_insert_ref(cb_data, refname, oid);\n> +\treturn rev_list_insert_ref(cb_data, oid);\n> }\n> \n> enum ack_type {\n> @@ -201,7 +225,7 @@ static void send_request(struct fetch_pack_args *args,\n> static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n> \t\t\t\t\tstruct object *obj)\n> {\n> -\trev_list_insert_ref(negotiator, NULL, &obj->oid);\n> +\trev_list_insert_ref(negotiator, &obj->oid);\n> }\n> \n> #define INITIAL_FLUSH 16\n> @@ -230,13 +254,12 @@ static void mark_tips(struct fetch_negotiator *negotiator,\n> \tint i;\n> \n> \tif (!negotiation_tips) {\n> -\t\tfor_each_ref(rev_list_insert_ref_oid, negotiator);\n> +\t\tfor_each_rawref(rev_list_insert_ref_oid, negotiator);\n> \t\treturn;\n> \t}\n> \n> \tfor (i = 0; i < negotiation_tips->nr; i++)\n> -\t\trev_list_insert_ref(negotiator, NULL,\n> -\t\t\t\t &negotiation_tips->oid[i]);\n> +\t\trev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);\n> \treturn;\n> }\n> \n> @@ -503,21 +526,11 @@ static struct commit_list *complete;\n> \n> static int mark_complete(const struct object_id *oid)\n> {\n> -\tstruct object *o = parse_object(the_repository, oid);\n> -\n> -\twhile (o && o->type == OBJ_TAG) {\n> -\t\tstruct tag *t = (struct tag *) o;\n> -\t\tif (!t->tagged)\n> -\t\t\tbreak; /* broken repository */\n> -\t\to->flags |= COMPLETE;\n> -\t\to = parse_object(the_repository, &t->tagged->oid);\n> -\t}\n> -\tif (o && o->type == OBJ_COMMIT) {\n> -\t\tstruct commit *commit = (struct commit *)o;\n> -\t\tif (!(commit->object.flags & COMPLETE)) {\n> -\t\t\tcommit->object.flags |= COMPLETE;\n> -\t\t\tcommit_list_insert(commit, &complete);\n> -\t\t}\n> +\tstruct commit *commit = deref_without_lazy_fetch(oid, 1);\n> +\n> +\tif (commit && !(commit->object.flags & COMPLETE)) {\n> +\t\tcommit->object.flags |= COMPLETE;\n> +\t\tcommit_list_insert(commit, &complete);\n> \t}\n> \treturn 0;\n> }\n> @@ -702,7 +715,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \t */\n> \ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n> \tif (!args->deepen) {\n> -\t\tfor_each_ref(mark_complete_oid, NULL);\n> +\t\tfor_each_rawref(mark_complete_oid, NULL);\n> \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n> \t\tcommit_list_sort_by_date(&complete);\n> \t\tif (cutoff)\n> @@ -716,16 +729,12 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n> \t */\n> \ttrace2_region_enter(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n> \tfor (ref = *refs; ref; ref = ref->next) {\n> -\t\tstruct object *o = deref_tag(the_repository,\n> -\t\t\t\t\t lookup_object(the_repository,\n> -\t\t\t\t\t &ref->old_oid),\n> -\t\t\t\t\t NULL, 0);\n> +\t\tstruct commit *c = deref_without_lazy_fetch(&ref->old_oid, 0);\n> \n> -\t\tif (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))\n> +\t\tif (!c || !(c->object.flags & COMPLETE))\n> \t\t\tcontinue;\n> \n> -\t\tnegotiator->known_common(negotiator,\n> -\t\t\t\t\t (struct commit *)o);\n> +\t\tnegotiator->known_common(negotiator, c);\n> \t}\n> \ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n> \n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 8a27452a51..e53492d595 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -384,6 +384,26 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n> \tgrep \"want $(cat hash)\" trace\n> '\n> \n> +test_expect_success 'fetch does not lazy-fetch missing targets of its refs' '\n> +\trm -rf server client trace &&\n> +\n> +\ttest_create_repo server &&\n> +\ttest_config -C server uploadpack.allowfilter 1 &&\n> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\ttest_commit -C server foo &&\n> +\n> +\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n> +\t# Make all refs point to nothing by deleting all objects.\n> +\trm client/.git/objects/pack/* &&\n> +\n> +\ttest_commit -C server bar &&\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \\\n> +\t\t--no-tags --recurse-submodules=no \\\n> +\t\torigin refs/tags/bar &&\n> +\tFOO_HASH=$(git -C server rev-parse foo) &&\n> +\t! grep \"want $FOO_HASH\" trace\n> +'\n> +\n> # The following two tests must be in this order. It is important that\n> # the srv.bare repository did not have tags during clone, but has tags\n> # in the fetch.\n"},{"id":"403483","messageId":"xmqqbljf7m88.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"55d2e5a4cccee0ae719f4210c2cbeeb6a691cf2f.1597184949.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 6/7] promisor-remote: lazy-fetch objects in subprocess","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-12T18:28:07Z","receivedAt":"2020-08-12T18:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Teach Git to lazy-fetch missing objects in a subprocess instead of doing\n> it in-process. This allows any fatal errors that occur during the fetch\n> to be isolated and converted into an error return value, instead of\n> causing the current command being run to terminate.\n>\n> ...\n> static int fetch_objects(const char *remote_name,\n> \t\t\t const struct object_id *oids,\n> \t\t\t int oid_nr)\n> {\n> -\tstruct ref *ref = NULL;\n> +\tstruct child_process child = CHILD_PROCESS_INIT;\n> \tint i;\n> +\tFILE *child_in;\n> +\n> +\tchild.git_cmd = 1;\n> +\tchild.in = -1;\n> +\targv_array_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=null\",\n> +\t\t\t \"fetch\", remote_name, \"--no-tags\",\n> +\t\t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n> +\t\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n\nFinally we get to use the new negotiator introduced earlier. Nice.\n\n> +\tif (start_command(&child))\n> +\t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n> +\tchild_in = xfdopen(child.in, \"w\");\n"},{"id":"403913","messageId":"cover.1597722941.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"20200724223844.2723397-1-jonathantanmy@google.com","subject":"[PATCH v3 0/7] Lazy fetch with subprocess","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:30Z","receivedAt":"2020-08-18T04:02:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"These patches are based on jc/no-update-fetch-head, once again.\n\nI've addressed most reviewer comments by expanding commit messages and\nfixing the cascading \"if\" issue in patch 3.\n\n> Would \"git fetch\" be the only user potentially benefit from being\n> able to dereference a tag that already exists locally and ignore\n> missing ones? I wonder if teaching deref_tag() a new flag would be\n> a better alternative to keep the separation of concerns at different\n> layers.\n\nRight now, I think so - \"fetch\" is a special case of having to avoid\ninfinite loops when lazy fetching, and of being able to tolerate missing\nref targets. I don't think most other commands are like that.\n\nJonathan Tan (7):\n negotiator/noop: add noop fetch negotiator\n fetch: allow refspecs specified through stdin\n fetch: avoid reading submodule config until needed\n fetch: only populate existing_refs if needed\n fetch-pack: do not lazy-fetch during ref iteration\n promisor-remote: lazy-fetch objects in subprocess\n fetch-pack: remove no_dependents code\n\n Documentation/config/fetch.txt | 5 +-\n Documentation/git-fetch.txt | 4 +\n Documentation/technical/partial-clone.txt | 13 +-\n Makefile | 1 +\n builtin/fetch-pack.c | 4 -\n builtin/fetch.c | 42 ++++-\n fetch-negotiator.c | 5 +\n fetch-pack.c | 189 +++++++++-------------\n fetch-pack.h | 14 --\n negotiator/noop.c | 44 +++++\n negotiator/noop.h | 8 +\n promisor-remote.c | 46 +++---\n remote-curl.c | 6 -\n repo-settings.c | 2 +\n repository.h | 1 +\n submodule-config.c | 8 +-\n t/t0410-partial-clone.sh | 2 +-\n t/t4067-diff-partial-clone.sh | 8 +-\n t/t5554-noop-fetch-negotiator.sh | 22 +++\n t/t5601-clone.sh | 2 +-\n t/t5616-partial-clone.sh | 20 +++\n transport.c | 4 -\n transport.h | 7 -\n 23 files changed, 255 insertions(+), 202 deletions(-)\n create mode 100644 negotiator/noop.c\n create mode 100644 negotiator/noop.h\n create mode 100755 t/t5554-noop-fetch-negotiator.sh\n\nRange-diff against v2:\n1: 35bdd372ae ! 1: 5b3b49d123 negotiator/null: add null fetch negotiator\n @@ Metadata\n Author: Jonathan Tan \n \n ## Commit message ##\n - negotiator/null: add null fetch negotiator\n + negotiator/noop: add noop fetch negotiator\n \n - Add a null fetch negotiator. This is introduced to allow partial clones\n + Add a noop fetch negotiator. This is introduced to allow partial clones\n to skip the unneeded negotiation step when fetching missing objects\n using a \"git fetch\" subprocess. (The implementation of spawning a \"git\n fetch\" subprocess will be done in a subsequent patch.) But this can also\n @@ Documentation/config/fetch.txt: fetch.negotiationAlgorithm::\n \tserver. Set to \"skipping\" to use an algorithm that skips commits in an\n \teffort to converge faster, but may result in a larger-than-necessary\n -\tpackfile; The default is \"default\" which instructs Git to use the default algorithm\n -+\tpackfile; or set to \"null\" to not send any information at all, which\n ++\tpackfile; or set to \"noop\" to not send any information at all, which\n +\twill almost certainly result in a larger-than-necessary packfile, but\n +\twill skip the negotiation step.\n +\tThe default is \"default\" which instructs Git to use the default algorithm\n @@ Makefile: LIB_OBJS += mergesort.o\n LIB_OBJS += midx.o\n LIB_OBJS += name-hash.o\n LIB_OBJS += negotiator/default.o\n -+LIB_OBJS += negotiator/null.o\n ++LIB_OBJS += negotiator/noop.o\n LIB_OBJS += negotiator/skipping.o\n LIB_OBJS += notes-cache.o\n LIB_OBJS += notes-merge.o\n @@ fetch-negotiator.c\n #include \"fetch-negotiator.h\"\n #include \"negotiator/default.h\"\n #include \"negotiator/skipping.h\"\n -+#include \"negotiator/null.h\"\n ++#include \"negotiator/noop.h\"\n #include \"repository.h\"\n \n void fetch_negotiator_init(struct repository *r,\n @@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,\n \t\tskipping_negotiator_init(negotiator);\n \t\treturn;\n \n -+\tcase FETCH_NEGOTIATION_NULL:\n -+\t\tnull_negotiator_init(negotiator);\n ++\tcase FETCH_NEGOTIATION_NOOP:\n ++\t\tnoop_negotiator_init(negotiator);\n +\t\treturn;\n +\n \tcase FETCH_NEGOTIATION_DEFAULT:\n \tdefault:\n \t\tdefault_negotiator_init(negotiator);\n \n - ## negotiator/null.c (new) ##\n + ## negotiator/noop.c (new) ##\n @@\n +#include \"cache.h\"\n -+#include \"null.h\"\n ++#include \"noop.h\"\n +#include \"../commit.h\"\n +#include \"../fetch-negotiator.h\"\n +\n @@ negotiator/null.c (new)\n +\t * This negotiator does not emit any commits, so there is no commit to\n +\t * be acknowledged. If there is any ack, there is a bug.\n +\t */\n -+\tBUG(\"ack with null negotiator, which does not emit any commits\");\n ++\tBUG(\"ack with noop negotiator, which does not emit any commits\");\n +\treturn 0;\n +}\n +\n @@ negotiator/null.c (new)\n +\t/* nothing to release */\n +}\n +\n -+void null_negotiator_init(struct fetch_negotiator *negotiator)\n ++void noop_negotiator_init(struct fetch_negotiator *negotiator)\n +{\n +\tnegotiator->known_common = known_common;\n +\tnegotiator->add_tip = add_tip;\n @@ negotiator/null.c (new)\n +\tnegotiator->data = NULL;\n +}\n \n - ## negotiator/null.h (new) ##\n + ## negotiator/noop.h (new) ##\n @@\n -+#ifndef NEGOTIATOR_NULL_H\n -+#define NEGOTIATOR_NULL_H\n ++#ifndef NEGOTIATOR_NOOP_H\n ++#define NEGOTIATOR_NOOP_H\n +\n +struct fetch_negotiator;\n +\n -+void null_negotiator_init(struct fetch_negotiator *negotiator);\n ++void noop_negotiator_init(struct fetch_negotiator *negotiator);\n +\n +#endif\n \n @@ repo-settings.c: void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n -+\t\telse if (!strcasecmp(strval, \"null\"))\n -+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NULL;\n ++\t\telse if (!strcasecmp(strval, \"noop\"))\n ++\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n \t\telse\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\n @@ repository.h: enum fetch_negotiation_setting {\n \tFETCH_NEGOTIATION_NONE = 0,\n \tFETCH_NEGOTIATION_DEFAULT = 1,\n \tFETCH_NEGOTIATION_SKIPPING = 2,\n -+\tFETCH_NEGOTIATION_NULL = 3,\n ++\tFETCH_NEGOTIATION_NOOP = 3,\n };\n \n struct repo_settings {\n \n - ## t/t5554-null-fetch-negotiator.sh (new) ##\n + ## t/t5554-noop-fetch-negotiator.sh (new) ##\n @@\n +#!/bin/sh\n +\n -+test_description='test null fetch negotiator'\n ++test_description='test noop fetch negotiator'\n +. ./test-lib.sh\n +\n -+test_expect_success 'null negotiator does not emit any \"have\"' '\n ++test_expect_success 'noop negotiator does not emit any \"have\"' '\n +\trm -f trace &&\n +\n +\ttest_create_repo server &&\n @@ t/t5554-null-fetch-negotiator.sh (new)\n +\ttest_create_repo client &&\n +\ttest_commit -C client we_have &&\n +\n -+\ttest_config -C client fetch.negotiationalgorithm null &&\n ++\ttest_config -C client fetch.negotiationalgorithm noop &&\n +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n +\n +\t! grep \"fetch> have\" trace &&\n2: 00ad7dd875 = 2: 9f277f1631 fetch: allow refspecs specified through stdin\n3: 8b4a522a13 ! 3: fda9f834f6 fetch: avoid reading submodule config until needed\n @@ Metadata\n ## Commit message ##\n fetch: avoid reading submodule config until needed\n \n - Teach \"git fetch\" to avoid reading the submodule config until necessary.\n - This allows users to avoid the lazy-fetching of this potentially missing\n - config file by specifying the --recurse-submodules=no command line\n - option.\n + In \"fetch\", there are two parameters submodule_fetch_jobs_config and\n + recurse_submodules that can be set in a variety of ways: through\n + .gitmodules, through .git/config, and through the command line.\n + Currently \"fetch\" handles this by first reading .gitmodules, then\n + reading .git/config (allowing it to overwrite existing values), then\n + reading the command line (allowing it to overwrite existing values).\n +\n + Notice that we can avoid reading .gitmodules if .git/config and/or the\n + command line already provides us with what we need. In addition, if\n + recurse_submodules is found to be \"no\", we do not need the value of\n + submodule_fetch_jobs_config.\n +\n + Avoiding reading .gitmodules is especially important when we use \"git\n + fetch\" to perform lazy fetches in a partial clone because the\n + .gitmodules file itself might need to be lazy fetched (and otherwise\n + causing an infinite loop).\n +\n + In light of all this, avoid reading .gitmodules until necessary. When\n + reading it, we may only need one of the two parameters it provides, so\n + teach fetch_config_from_gitmodules() to support NULL arguments. With\n + this patch, users (including Git itself when invoking \"git fetch\" to\n + lazy-fetch) will be able to guarantee avoiding reading .gitmodules by\n + passing --recurse-submodules=no.\n \n Signed-off-by: Jonathan Tan \n \n @@ builtin/fetch.c: int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tif (deepen_relative < 0)\n \n ## submodule-config.c ##\n -@@ submodule-config.c: struct fetch_config {\n - static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n +@@ submodule-config.c: static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n {\n \tstruct fetch_config *config = cb;\n --\tif (!strcmp(var, \"submodule.fetchjobs\")) {\n -+\tif (!strcmp(var, \"submodule.fetchjobs\") && config->max_children) {\n - \t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n + \tif (!strcmp(var, \"submodule.fetchjobs\")) {\n +-\t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n ++\t\tif (config->max_children)\n ++\t\t\t*(config->max_children) =\n ++\t\t\t\tparse_submodule_fetchjobs(var, value);\n \t\treturn 0;\n --\t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n -+\t} else if (!strcmp(var, \"fetch.recursesubmodules\") &&\n -+\t\t config->recurse_submodules) {\n - \t\t*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n + \t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n +-\t\t*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n ++\t\tif (config->recurse_submodules)\n ++\t\t\t*(config->recurse_submodules) =\n ++\t\t\t\tparse_fetch_recurse_submodules_arg(var, value);\n \t\treturn 0;\n \t}\n + \n4: 77bc83e7f2 ! 4: a5554cd27f fetch: only populate existing_refs if needed\n @@ Metadata\n ## Commit message ##\n fetch: only populate existing_refs if needed\n \n - When fetching tags, Git only writes tags that do not already exist in\n - the client repository. This necessitates an iteration over all the refs,\n - but fetch performs this iteration even if no tags are fetched.\n + In \"fetch\", get_ref_map() iterates over all refs to populate\n + \"existing_refs\" in order to populate peer_ref->old_oid in the returned\n + refmap, even if the refmap has no peer_ref set - which is the case when\n + only literal hashes (i.e. no refs by name) are fetched.\n \n - This issue is more severe in a partial clone because the iteration over\n - refs also checks that the targets of those refs are present,\n - necessitating a lazy fetch if the target is missing.\n + Iterating over refs causes the targets of those refs to be checked for\n + existence. Avoiding this is especially important when we use \"git fetch\"\n + to perform lazy fetches in a partial clone because a target of such a\n + ref may need to be itself lazy-fetched (and otherwise causing an\n + infinite loop).\n \n - Therefore, iterate over the refs only when necessary. The user can avoid\n - this iteration by refraining from fetching tags, for example, by passing\n - --no-tags as an argument. A subsequent patch will also teach Git to use\n - \"git fetch\" to lazy-fetch missing objects in a partial clone, thus also\n - making use of this change.\n + Therefore, avoid populating \"existing_refs\" until necessary. With this\n + patch, because Git lazy-fetches objects by literal hashes (to be done in\n + a subsequent commit), it will then be able to guarantee avoiding reading\n + targets of refs.\n \n Signed-off-by: Jonathan Tan \n \n5: d42e98ff07 ! 5: f56c24dd1b fetch-pack: do not lazy-fetch during ref iteration\n @@ Metadata\n ## Commit message ##\n fetch-pack: do not lazy-fetch during ref iteration\n \n - Teach \"fetch-pack\" not to lazy fetch whenever iterating over refs. This\n - is done by using the raw form of ref iteration and by dereferencing tags\n - ourselves.\n + In order to determine negotiation tips, \"fetch-pack\" iterates over all\n + refs and dereferences all annotated tags found. This causes the\n + existence of targets of refs and annotated tags to be checked. Avoiding\n + this is especially important when we use \"git fetch\" (which invokes\n + \"fetch-pack\") to perform lazy fetches in a partial clone because a\n + target of such a ref or annotated tag may need to be itself lazy-fetched\n + (and otherwise causing an infinite loop).\n +\n + Therefore, teach \"fetch-pack\" not to lazy fetch whenever iterating over\n + refs. This is done by using the raw form of ref iteration and by\n + dereferencing tags ourselves.\n \n Signed-off-by: Jonathan Tan \n \n6: 55d2e5a4cc = 6: 4e72ca6258 promisor-remote: lazy-fetch objects in subprocess\n7: e8f16d6908 = 7: 3ff9d034e9 fetch-pack: remove no_dependents code\n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403912","messageId":"5b3b49d1236756dc65479f7e6c062479f2e19b3c.1597722941.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 1/7] negotiator/noop: add noop fetch negotiator","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:31Z","receivedAt":"2020-08-18T04:02:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Add a noop fetch negotiator. This is introduced to allow partial clones\nto skip the unneeded negotiation step when fetching missing objects\nusing a \"git fetch\" subprocess. (The implementation of spawning a \"git\nfetch\" subprocess will be done in a subsequent patch.) But this can also\nbe useful for end users, e.g. as a blunt fix for object corruption.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/config/fetch.txt | 5 +++-\n Makefile | 1 +\n fetch-negotiator.c | 5 ++++\n negotiator/noop.c | 44 ++++++++++++++++++++++++++++++++\n negotiator/noop.h | 8 ++++++\n repo-settings.c | 2 ++\n repository.h | 1 +\n t/t5554-noop-fetch-negotiator.sh | 22 ++++++++++++++++\n 8 files changed, 87 insertions(+), 1 deletion(-)\n create mode 100644 negotiator/noop.c\n create mode 100644 negotiator/noop.h\n create mode 100755 t/t5554-noop-fetch-negotiator.sh\n\ndiff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt\nindex 0aaa05e8c0..19383ddf7b 100644\n--- a/Documentation/config/fetch.txt\n+++ b/Documentation/config/fetch.txt\n@@ -60,7 +60,10 @@ fetch.negotiationAlgorithm::\n \tsent when negotiating the contents of the packfile to be sent by the\n \tserver. Set to \"skipping\" to use an algorithm that skips commits in an\n \teffort to converge faster, but may result in a larger-than-necessary\n-\tpackfile; The default is \"default\" which instructs Git to use the default algorithm\n+\tpackfile; or set to \"noop\" to not send any information at all, which\n+\twill almost certainly result in a larger-than-necessary packfile, but\n+\twill skip the negotiation step.\n+\tThe default is \"default\" which instructs Git to use the default algorithm\n \tthat never skips commits (unless the server has acknowledged it or one\n \tof its descendants). If `feature.experimental` is enabled, then this\n \tsetting defaults to \"skipping\".\ndiff --git a/Makefile b/Makefile\nindex 372139f1f2..94a8fb54d1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -917,6 +917,7 @@ LIB_OBJS += mergesort.o\n LIB_OBJS += midx.o\n LIB_OBJS += name-hash.o\n LIB_OBJS += negotiator/default.o\n+LIB_OBJS += negotiator/noop.o\n LIB_OBJS += negotiator/skipping.o\n LIB_OBJS += notes-cache.o\n LIB_OBJS += notes-merge.o\ndiff --git a/fetch-negotiator.c b/fetch-negotiator.c\nindex 0a1357dc9d..57ed5784e1 100644\n--- a/fetch-negotiator.c\n+++ b/fetch-negotiator.c\n@@ -2,6 +2,7 @@\n #include \"fetch-negotiator.h\"\n #include \"negotiator/default.h\"\n #include \"negotiator/skipping.h\"\n+#include \"negotiator/noop.h\"\n #include \"repository.h\"\n \n void fetch_negotiator_init(struct repository *r,\n@@ -13,6 +14,10 @@ void fetch_negotiator_init(struct repository *r,\n \t\tskipping_negotiator_init(negotiator);\n \t\treturn;\n \n+\tcase FETCH_NEGOTIATION_NOOP:\n+\t\tnoop_negotiator_init(negotiator);\n+\t\treturn;\n+\n \tcase FETCH_NEGOTIATION_DEFAULT:\n \tdefault:\n \t\tdefault_negotiator_init(negotiator);\ndiff --git a/negotiator/noop.c b/negotiator/noop.c\nnew file mode 100644\nindex 0000000000..60569b8350\n--- /dev/null\n+++ b/negotiator/noop.c\n@@ -0,0 +1,44 @@\n+#include \"cache.h\"\n+#include \"noop.h\"\n+#include \"../commit.h\"\n+#include \"../fetch-negotiator.h\"\n+\n+static void known_common(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/* do nothing */\n+}\n+\n+static void add_tip(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/* do nothing */\n+}\n+\n+static const struct object_id *next(struct fetch_negotiator *n)\n+{\n+\treturn NULL;\n+}\n+\n+static int ack(struct fetch_negotiator *n, struct commit *c)\n+{\n+\t/*\n+\t * This negotiator does not emit any commits, so there is no commit to\n+\t * be acknowledged. If there is any ack, there is a bug.\n+\t */\n+\tBUG(\"ack with noop negotiator, which does not emit any commits\");\n+\treturn 0;\n+}\n+\n+static void release(struct fetch_negotiator *n)\n+{\n+\t/* nothing to release */\n+}\n+\n+void noop_negotiator_init(struct fetch_negotiator *negotiator)\n+{\n+\tnegotiator->known_common = known_common;\n+\tnegotiator->add_tip = add_tip;\n+\tnegotiator->next = next;\n+\tnegotiator->ack = ack;\n+\tnegotiator->release = release;\n+\tnegotiator->data = NULL;\n+}\ndiff --git a/negotiator/noop.h b/negotiator/noop.h\nnew file mode 100644\nindex 0000000000..2b4ec5d07a\n--- /dev/null\n+++ b/negotiator/noop.h\n@@ -0,0 +1,8 @@\n+#ifndef NEGOTIATOR_NOOP_H\n+#define NEGOTIATOR_NOOP_H\n+\n+struct fetch_negotiator;\n+\n+void noop_negotiator_init(struct fetch_negotiator *negotiator);\n+\n+#endif\ndiff --git a/repo-settings.c b/repo-settings.c\nindex 0918408b34..aa61a35338 100644\n--- a/repo-settings.c\n+++ b/repo-settings.c\n@@ -39,6 +39,8 @@ void prepare_repo_settings(struct repository *r)\n \tif (!repo_config_get_string(r, \"fetch.negotiationalgorithm\", &strval)) {\n \t\tif (!strcasecmp(strval, \"skipping\"))\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;\n+\t\telse if (!strcasecmp(strval, \"noop\"))\n+\t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;\n \t\telse\n \t\t\tr->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;\n \t}\ndiff --git a/repository.h b/repository.h\nindex 3c1f7d54bd..628c834367 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -23,6 +23,7 @@ enum fetch_negotiation_setting {\n \tFETCH_NEGOTIATION_NONE = 0,\n \tFETCH_NEGOTIATION_DEFAULT = 1,\n \tFETCH_NEGOTIATION_SKIPPING = 2,\n+\tFETCH_NEGOTIATION_NOOP = 3,\n };\n \n struct repo_settings {\ndiff --git a/t/t5554-noop-fetch-negotiator.sh b/t/t5554-noop-fetch-negotiator.sh\nnew file mode 100755\nindex 0000000000..2ac7b5859e\n--- /dev/null\n+++ b/t/t5554-noop-fetch-negotiator.sh\n@@ -0,0 +1,22 @@\n+#!/bin/sh\n+\n+test_description='test noop fetch negotiator'\n+. ./test-lib.sh\n+\n+test_expect_success 'noop negotiator does not emit any \"have\"' '\n+\trm -f trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_commit -C server to_fetch &&\n+\n+\ttest_create_repo client &&\n+\ttest_commit -C client we_have &&\n+\n+\ttest_config -C client fetch.negotiationalgorithm noop &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server\" &&\n+\n+\t! grep \"fetch> have\" trace &&\n+\tgrep \"fetch> done\" trace\n+'\n+\n+test_done\n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403914","messageId":"9f277f1631a37785dcd72a552dd86738d639ea59.1597722942.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 2/7] fetch: allow refspecs specified through stdin","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:32Z","receivedAt":"2020-08-18T04:02:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a subsequent patch, partial clones will be taught to fetch missing\nobjects using a \"git fetch\" subprocess. Because the number of objects\nfetched may be too numerous to fit on the command line, teach \"fetch\" to\naccept refspecs passed through stdin.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-fetch.txt | 4 ++++\n builtin/fetch.c | 19 +++++++++++++++++--\n 2 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt\nindex 45b6d8e633..9067c2079e 100644\n--- a/Documentation/git-fetch.txt\n+++ b/Documentation/git-fetch.txt\n@@ -48,6 +48,10 @@ include::fetch-options.txt[]\n \n include::pull-fetch-param.txt[]\n \n+--stdin::\n+\tRead refspecs, one per line, from stdin in addition to those provided\n+\tas arguments. The \"tag \" format is not supported.\n+\n include::urls-remotes.txt[]\n \n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 3ccf69753f..a5498646bf 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -80,6 +80,7 @@ static struct list_objects_filter_options filter_options;\n static struct string_list server_options = STRING_LIST_INIT_DUP;\n static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;\n static int fetch_write_commit_graph = -1;\n+static int stdin_refspecs = 0;\n \n static int git_fetch_config(const char *k, const char *v, void *cb)\n {\n@@ -209,6 +210,8 @@ static struct option builtin_fetch_options[] = {\n \t\t N_(\"check for forced-updates on all updated branches\")),\n \tOPT_BOOL(0, \"write-commit-graph\", &fetch_write_commit_graph,\n \t\t N_(\"write the commit-graph after fetching\")),\n+\tOPT_BOOL(0, \"stdin\", &stdin_refspecs,\n+\t\t N_(\"accept refspecs from stdin\")),\n \tOPT_END()\n };\n \n@@ -1684,7 +1687,8 @@ static inline void fetch_one_setup_partial(struct remote *remote)\n \treturn;\n }\n \n-static int fetch_one(struct remote *remote, int argc, const char **argv, int prune_tags_ok)\n+static int fetch_one(struct remote *remote, int argc, const char **argv,\n+\t\t int prune_tags_ok, int use_stdin_refspecs)\n {\n \tstruct refspec rs = REFSPEC_INIT_FETCH;\n \tint i;\n@@ -1741,6 +1745,13 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int pru\n \t\t}\n \t}\n \n+\tif (use_stdin_refspecs) {\n+\t\tstruct strbuf line = STRBUF_INIT;\n+\t\twhile (strbuf_getline_lf(&line, stdin) != EOF)\n+\t\t\trefspec_append(&rs, line.buf);\n+\t\tstrbuf_release(&line);\n+\t}\n+\n \tif (server_options.nr)\n \t\tgtransport->server_options = &server_options;\n \n@@ -1841,7 +1852,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \tif (remote) {\n \t\tif (filter_options.choice || has_promisor_remote())\n \t\t\tfetch_one_setup_partial(remote);\n-\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok);\n+\t\tresult = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs);\n \t} else {\n \t\tint max_children = max_jobs;\n \n@@ -1849,6 +1860,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"--filter can only be used with the remote \"\n \t\t\t \"configured in extensions.partialclone\"));\n \n+\t\tif (stdin_refspecs)\n+\t\t\tdie(_(\"--stdin can only be used when fetching \"\n+\t\t\t \"from one remote\"));\n+\n \t\tif (max_children < 0)\n \t\t\tmax_children = fetch_parallel_config;\n \n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403915","messageId":"a5554cd27f8aac5018e5699837e3b9e9badcd3a4.1597722942.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 4/7] fetch: only populate existing_refs if needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:34Z","receivedAt":"2020-08-18T04:02:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In \"fetch\", get_ref_map() iterates over all refs to populate\n\"existing_refs\" in order to populate peer_ref->old_oid in the returned\nrefmap, even if the refmap has no peer_ref set - which is the case when\nonly literal hashes (i.e. no refs by name) are fetched.\n\nIterating over refs causes the targets of those refs to be checked for\nexistence. Avoiding this is especially important when we use \"git fetch\"\nto perform lazy fetches in a partial clone because a target of such a\nref may need to be itself lazy-fetched (and otherwise causing an\ninfinite loop).\n\nTherefore, avoid populating \"existing_refs\" until necessary. With this\npatch, because Git lazy-fetches objects by literal hashes (to be done in\na subsequent commit), it will then be able to guarantee avoiding reading\ntargets of refs.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 29db219c68..6460ce3f4e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -449,6 +449,7 @@ static struct ref *get_ref_map(struct remote *remote,\n \tstruct ref *orefs = NULL, **oref_tail = &orefs;\n \n \tstruct hashmap existing_refs;\n+\tint existing_refs_populated = 0;\n \n \tif (rs->nr) {\n \t\tstruct refspec *fetch_refspec;\n@@ -542,15 +543,18 @@ static struct ref *get_ref_map(struct remote *remote,\n \n \tref_map = ref_remove_duplicates(ref_map);\n \n-\trefname_hash_init(&existing_refs);\n-\tfor_each_ref(add_one_refname, &existing_refs);\n-\n \tfor (rm = ref_map; rm; rm = rm->next) {\n \t\tif (rm->peer_ref) {\n \t\t\tconst char *refname = rm->peer_ref->name;\n \t\t\tstruct refname_hash_entry *peer_item;\n \t\t\tunsigned int hash = strhash(refname);\n \n+\t\t\tif (!existing_refs_populated) {\n+\t\t\t\trefname_hash_init(&existing_refs);\n+\t\t\t\tfor_each_ref(add_one_refname, &existing_refs);\n+\t\t\t\texisting_refs_populated = 1;\n+\t\t\t}\n+\n \t\t\tpeer_item = hashmap_get_entry_from_hash(&existing_refs,\n \t\t\t\t\t\thash, refname,\n \t\t\t\t\t\tstruct refname_hash_entry, ent);\n@@ -560,7 +564,8 @@ static struct ref *get_ref_map(struct remote *remote,\n \t\t\t}\n \t\t}\n \t}\n-\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n+\tif (existing_refs_populated)\n+\t\thashmap_free_entries(&existing_refs, struct refname_hash_entry, ent);\n \n \treturn ref_map;\n }\n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403916","messageId":"f56c24dd1b05081b9c66e87746a67e12394ef012.1597722942.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 5/7] fetch-pack: do not lazy-fetch during ref iteration","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:35Z","receivedAt":"2020-08-18T04:02:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In order to determine negotiation tips, \"fetch-pack\" iterates over all\nrefs and dereferences all annotated tags found. This causes the\nexistence of targets of refs and annotated tags to be checked. Avoiding\nthis is especially important when we use \"git fetch\" (which invokes\n\"fetch-pack\") to perform lazy fetches in a partial clone because a\ntarget of such a ref or annotated tag may need to be itself lazy-fetched\n(and otherwise causing an infinite loop).\n\nTherefore, teach \"fetch-pack\" not to lazy fetch whenever iterating over\nrefs. This is done by using the raw form of ref iteration and by\ndereferencing tags ourselves.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 79 ++++++++++++++++++++++------------------\n t/t5616-partial-clone.sh | 20 ++++++++++\n 2 files changed, 64 insertions(+), 35 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 80fb3bd899..707bbc31fd 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -108,24 +108,48 @@ static void for_each_cached_alternate(struct fetch_negotiator *negotiator,\n \t\tcb(negotiator, cache.items[i]);\n }\n \n+static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n+\t\t\t\t\t int mark_tags_complete)\n+{\n+\tenum object_type type;\n+\tstruct object_info info = { .typep = &type };\n+\n+\twhile (1) {\n+\t\tif (oid_object_info_extended(the_repository, oid, &info,\n+\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK))\n+\t\t\treturn NULL;\n+\t\tif (type == OBJ_TAG) {\n+\t\t\tstruct tag *tag = (struct tag *)\n+\t\t\t\tparse_object(the_repository, oid);\n+\n+\t\t\tif (!tag->tagged)\n+\t\t\t\treturn NULL;\n+\t\t\tif (mark_tags_complete)\n+\t\t\t\ttag->object.flags |= COMPLETE;\n+\t\t\toid = &tag->tagged->oid;\n+\t\t} else {\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tif (type == OBJ_COMMIT)\n+\t\treturn (struct commit *) parse_object(the_repository, oid);\n+\treturn NULL;\n+}\n+\n static int rev_list_insert_ref(struct fetch_negotiator *negotiator,\n-\t\t\t const char *refname,\n \t\t\t const struct object_id *oid)\n {\n-\tstruct object *o = deref_tag(the_repository,\n-\t\t\t\t parse_object(the_repository, oid),\n-\t\t\t\t refname, 0);\n-\n-\tif (o && o->type == OBJ_COMMIT)\n-\t\tnegotiator->add_tip(negotiator, (struct commit *)o);\n+\tstruct commit *c = deref_without_lazy_fetch(oid, 0);\n \n+\tif (c)\n+\t\tnegotiator->add_tip(negotiator, c);\n \treturn 0;\n }\n \n static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,\n \t\t\t\t int flag, void *cb_data)\n {\n-\treturn rev_list_insert_ref(cb_data, refname, oid);\n+\treturn rev_list_insert_ref(cb_data, oid);\n }\n \n enum ack_type {\n@@ -201,7 +225,7 @@ static void send_request(struct fetch_pack_args *args,\n static void insert_one_alternate_object(struct fetch_negotiator *negotiator,\n \t\t\t\t\tstruct object *obj)\n {\n-\trev_list_insert_ref(negotiator, NULL, &obj->oid);\n+\trev_list_insert_ref(negotiator, &obj->oid);\n }\n \n #define INITIAL_FLUSH 16\n@@ -230,13 +254,12 @@ static void mark_tips(struct fetch_negotiator *negotiator,\n \tint i;\n \n \tif (!negotiation_tips) {\n-\t\tfor_each_ref(rev_list_insert_ref_oid, negotiator);\n+\t\tfor_each_rawref(rev_list_insert_ref_oid, negotiator);\n \t\treturn;\n \t}\n \n \tfor (i = 0; i < negotiation_tips->nr; i++)\n-\t\trev_list_insert_ref(negotiator, NULL,\n-\t\t\t\t &negotiation_tips->oid[i]);\n+\t\trev_list_insert_ref(negotiator, &negotiation_tips->oid[i]);\n \treturn;\n }\n \n@@ -503,21 +526,11 @@ static struct commit_list *complete;\n \n static int mark_complete(const struct object_id *oid)\n {\n-\tstruct object *o = parse_object(the_repository, oid);\n-\n-\twhile (o && o->type == OBJ_TAG) {\n-\t\tstruct tag *t = (struct tag *) o;\n-\t\tif (!t->tagged)\n-\t\t\tbreak; /* broken repository */\n-\t\to->flags |= COMPLETE;\n-\t\to = parse_object(the_repository, &t->tagged->oid);\n-\t}\n-\tif (o && o->type == OBJ_COMMIT) {\n-\t\tstruct commit *commit = (struct commit *)o;\n-\t\tif (!(commit->object.flags & COMPLETE)) {\n-\t\t\tcommit->object.flags |= COMPLETE;\n-\t\t\tcommit_list_insert(commit, &complete);\n-\t\t}\n+\tstruct commit *commit = deref_without_lazy_fetch(oid, 1);\n+\n+\tif (commit && !(commit->object.flags & COMPLETE)) {\n+\t\tcommit->object.flags |= COMPLETE;\n+\t\tcommit_list_insert(commit, &complete);\n \t}\n \treturn 0;\n }\n@@ -702,7 +715,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t */\n \ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n-\t\tfor_each_ref(mark_complete_oid, NULL);\n+\t\tfor_each_rawref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n \t\tcommit_list_sort_by_date(&complete);\n \t\tif (cutoff)\n@@ -716,16 +729,12 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t */\n \ttrace2_region_enter(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n-\t\tstruct object *o = deref_tag(the_repository,\n-\t\t\t\t\t lookup_object(the_repository,\n-\t\t\t\t\t &ref->old_oid),\n-\t\t\t\t\t NULL, 0);\n+\t\tstruct commit *c = deref_without_lazy_fetch(&ref->old_oid, 0);\n \n-\t\tif (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))\n+\t\tif (!c || !(c->object.flags & COMPLETE))\n \t\t\tcontinue;\n \n-\t\tnegotiator->known_common(negotiator,\n-\t\t\t\t\t (struct commit *)o);\n+\t\tnegotiator->known_common(negotiator, c);\n \t}\n \ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8a27452a51..e53492d595 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -384,6 +384,26 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n+test_expect_success 'fetch does not lazy-fetch missing targets of its refs' '\n+\trm -rf server client trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\ttest_commit -C server foo &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\t# Make all refs point to nothing by deleting all objects.\n+\trm client/.git/objects/pack/* &&\n+\n+\ttest_commit -C server bar &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \\\n+\t\t--no-tags --recurse-submodules=no \\\n+\t\torigin refs/tags/bar &&\n+\tFOO_HASH=$(git -C server rev-parse foo) &&\n+\t! grep \"want $FOO_HASH\" trace\n+'\n+\n # The following two tests must be in this order. It is important that\n # the srv.bare repository did not have tags during clone, but has tags\n # in the fetch.\n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403918","messageId":"fda9f834f691adb3bf18c3b01deb058e8fe36c0c.1597722942.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 3/7] fetch: avoid reading submodule config until needed","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:33Z","receivedAt":"2020-08-18T04:02:09Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In \"fetch\", there are two parameters submodule_fetch_jobs_config and\nrecurse_submodules that can be set in a variety of ways: through\n.gitmodules, through .git/config, and through the command line.\nCurrently \"fetch\" handles this by first reading .gitmodules, then\nreading .git/config (allowing it to overwrite existing values), then\nreading the command line (allowing it to overwrite existing values).\n\nNotice that we can avoid reading .gitmodules if .git/config and/or the\ncommand line already provides us with what we need. In addition, if\nrecurse_submodules is found to be \"no\", we do not need the value of\nsubmodule_fetch_jobs_config.\n\nAvoiding reading .gitmodules is especially important when we use \"git\nfetch\" to perform lazy fetches in a partial clone because the\n.gitmodules file itself might need to be lazy fetched (and otherwise\ncausing an infinite loop).\n\nIn light of all this, avoid reading .gitmodules until necessary. When\nreading it, we may only need one of the two parameters it provides, so\nteach fetch_config_from_gitmodules() to support NULL arguments. With\nthis patch, users (including Git itself when invoking \"git fetch\" to\nlazy-fetch) will be able to guarantee avoiding reading .gitmodules by\npassing --recurse-submodules=no.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 10 ++++++++--\n submodule-config.c | 8 ++++++--\n 2 files changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a5498646bf..29db219c68 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1786,12 +1786,18 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)\n \t\tfree(anon);\n \t}\n \n-\tfetch_config_from_gitmodules(&submodule_fetch_jobs_config,\n-\t\t\t\t &recurse_submodules);\n \tgit_config(git_fetch_config, NULL);\n \n \targc = parse_options(argc, argv, prefix,\n \t\t\t builtin_fetch_options, builtin_fetch_usage, 0);\n+\tif (recurse_submodules != RECURSE_SUBMODULES_OFF) {\n+\t\tint *sfjc = submodule_fetch_jobs_config == -1\n+\t\t\t ? &submodule_fetch_jobs_config : NULL;\n+\t\tint *rs = recurse_submodules == RECURSE_SUBMODULES_DEFAULT\n+\t\t\t ? &recurse_submodules : NULL;\n+\n+\t\tfetch_config_from_gitmodules(sfjc, rs);\n+\t}\n \n \tif (deepen_relative) {\n \t\tif (deepen_relative < 0)\ndiff --git a/submodule-config.c b/submodule-config.c\nindex e175dfbc38..c569e22aa3 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -777,10 +777,14 @@ static int gitmodules_fetch_config(const char *var, const char *value, void *cb)\n {\n \tstruct fetch_config *config = cb;\n \tif (!strcmp(var, \"submodule.fetchjobs\")) {\n-\t\t*(config->max_children) = parse_submodule_fetchjobs(var, value);\n+\t\tif (config->max_children)\n+\t\t\t*(config->max_children) =\n+\t\t\t\tparse_submodule_fetchjobs(var, value);\n \t\treturn 0;\n \t} else if (!strcmp(var, \"fetch.recursesubmodules\")) {\n-\t\t*(config->recurse_submodules) = parse_fetch_recurse_submodules_arg(var, value);\n+\t\tif (config->recurse_submodules)\n+\t\t\t*(config->recurse_submodules) =\n+\t\t\t\tparse_fetch_recurse_submodules_arg(var, value);\n \t\treturn 0;\n \t}\n \n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403917","messageId":"4e72ca62583a2938509511818ce6bef8af24c271.1597722942.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 6/7] promisor-remote: lazy-fetch objects in subprocess","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:36Z","receivedAt":"2020-08-18T04:02:10Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach Git to lazy-fetch missing objects in a subprocess instead of doing\nit in-process. This allows any fatal errors that occur during the fetch\nto be isolated and converted into an error return value, instead of\ncausing the current command being run to terminate.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/technical/partial-clone.txt | 13 ++-----\n promisor-remote.c | 46 +++++++++++------------\n t/t0410-partial-clone.sh | 2 +-\n t/t4067-diff-partial-clone.sh | 8 ++--\n t/t5601-clone.sh | 2 +-\n 5 files changed, 30 insertions(+), 41 deletions(-)\n\ndiff --git a/Documentation/technical/partial-clone.txt b/Documentation/technical/partial-clone.txt\nindex b9e17e7a28..0780d30cac 100644\n--- a/Documentation/technical/partial-clone.txt\n+++ b/Documentation/technical/partial-clone.txt\n@@ -171,20 +171,13 @@ additional flag.\n Fetching Missing Objects\n ------------------------\n \n-- Fetching of objects is done using the existing transport mechanism using\n- transport_fetch_refs(), setting a new transport option\n- TRANS_OPT_NO_DEPENDENTS to indicate that only the objects themselves are\n- desired, not any object that they refer to.\n-+\n-Because some transports invoke fetch_pack() in the same process, fetch_pack()\n-has been updated to not use any object flags when the corresponding argument\n-(no_dependents) is set.\n+- Fetching of objects is done by invoking a \"git fetch\" subprocess.\n \n - The local repository sends a request with the hashes of all requested\n- objects as \"want\" lines, and does not perform any packfile negotiation.\n+ objects, and does not perform any packfile negotiation.\n It then receives a packfile.\n \n-- Because we are reusing the existing fetch-pack mechanism, fetching\n+- Because we are reusing the existing fetch mechanism, fetching\n currently fetches all objects referred to by the requested objects, even\n though they are not necessary.\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex baaea12fd6..6e647610e9 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -3,6 +3,7 @@\n #include \"promisor-remote.h\"\n #include \"config.h\"\n #include \"transport.h\"\n+#include \"argv-array.h\"\n \n static char *repository_format_partial_clone;\n static const char *core_partial_clone_filter_default;\n@@ -12,39 +13,34 @@ void set_repository_format_partial_clone(char *partial_clone)\n \trepository_format_partial_clone = xstrdup_or_null(partial_clone);\n }\n \n-static int fetch_refs(const char *remote_name, struct ref *ref)\n-{\n-\tstruct remote *remote;\n-\tstruct transport *transport;\n-\tint res;\n-\n-\tremote = remote_get(remote_name);\n-\tif (!remote->url[0])\n-\t\tdie(_(\"Remote with no URL\"));\n-\ttransport = transport_get(remote, remote->url[0]);\n-\n-\ttransport_set_option(transport, TRANS_OPT_FROM_PROMISOR, \"1\");\n-\ttransport_set_option(transport, TRANS_OPT_NO_DEPENDENTS, \"1\");\n-\tres = transport_fetch_refs(transport, ref);\n-\n-\treturn res;\n-}\n-\n static int fetch_objects(const char *remote_name,\n \t\t\t const struct object_id *oids,\n \t\t\t int oid_nr)\n {\n-\tstruct ref *ref = NULL;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tint i;\n+\tFILE *child_in;\n+\n+\tchild.git_cmd = 1;\n+\tchild.in = -1;\n+\targv_array_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=null\",\n+\t\t\t \"fetch\", remote_name, \"--no-tags\",\n+\t\t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n+\t\t\t \"--filter=blob:none\", \"--stdin\", NULL);\n+\tif (start_command(&child))\n+\t\tdie(_(\"promisor-remote: unable to fork off fetch subprocess\"));\n+\tchild_in = xfdopen(child.in, \"w\");\n \n \tfor (i = 0; i < oid_nr; i++) {\n-\t\tstruct ref *new_ref = alloc_ref(oid_to_hex(&oids[i]));\n-\t\toidcpy(&new_ref->old_oid, &oids[i]);\n-\t\tnew_ref->exact_oid = 1;\n-\t\tnew_ref->next = ref;\n-\t\tref = new_ref;\n+\t\tif (fputs(oid_to_hex(&oids[i]), child_in) < 0)\n+\t\t\tdie_errno(_(\"promisor-remote: could not write to fetch subprocess\"));\n+\t\tif (fputc('\\n', child_in) < 0)\n+\t\t\tdie_errno(_(\"promisor-remote: could not write to fetch subprocess\"));\n \t}\n-\treturn fetch_refs(remote_name, ref);\n+\n+\tif (fclose(child_in) < 0)\n+\t\tdie_errno(_(\"promisor-remote: could not close stdin to fetch subprocess\"));\n+\treturn finish_command(&child) ? -1 : 0;\n }\n \n static struct promisor_remote *promisors;\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 463dc3a8be..3e454f934e 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -203,7 +203,7 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \trm -rf repo/.git/objects/* &&\n \trm -f trace &&\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C repo cat-file -p \"$HASH\" &&\n-\tgrep \"git< fetch=.*ref-in-want\" trace\n+\tgrep \"fetch< fetch=.*ref-in-want\" trace\n '\n \n test_expect_success 'fetching of missing objects from another promisor remote' '\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex ef8e0e9cb0..804f2a82e8 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -20,7 +20,7 @@ test_expect_success 'git show batches blobs' '\n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -44,7 +44,7 @@ test_expect_success 'diff batches blobs' '\n \t# Ensure that there is exactly 1 negotiation by checking that there is\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -127,7 +127,7 @@ test_expect_success 'diff with rename detection batches blobs' '\n \t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD >out &&\n \tgrep \":100644 100644.*R[0-9][0-9][0-9].*b.*c\" out &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n@@ -175,7 +175,7 @@ test_expect_success 'diff --break-rewrites fetches only if necessary, and batche\n \t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n \t# end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb7..f82b0dbb5a 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -702,7 +702,7 @@ test_expect_success 'batch missing blob request during checkout' '\n \t# Ensure that there is only one negotiation by checking that there is\n \t# only \"done\" line sent. (\"done\" marks the end of negotiation.)\n \tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client checkout HEAD^ &&\n-\tgrep \"git> done\" trace >done_lines &&\n+\tgrep \"fetch> done\" trace >done_lines &&\n \ttest_line_count = 1 done_lines\n '\n \n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403919","messageId":"3ff9d034e91f394457e1c1813437603e70741aa6.1597722942.git.jonathantanmy@google.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"[PATCH v3 7/7] fetch-pack: remove no_dependents code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T04:01:37Z","receivedAt":"2020-08-18T04:02:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Now that Git has switched to using a subprocess to lazy-fetch missing\nobjects, remove the no_dependents code as it is no longer used.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch-pack.c | 4 --\n fetch-pack.c | 110 ++++++++++++-------------------------------\n fetch-pack.h | 14 ------\n remote-curl.c | 6 ---\n transport.c | 4 --\n transport.h | 7 ---\n 6 files changed, 30 insertions(+), 115 deletions(-)\n\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bbb5c96167..58b7c1fbdc 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -153,10 +153,6 @@ int cmd_fetch_pack(int argc, const char **argv, const char *prefix)\n \t\t\targs.from_promisor = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(\"--no-dependents\", arg)) {\n-\t\t\targs.no_dependents = 1;\n-\t\t\tcontinue;\n-\t\t}\n \t\tif (skip_prefix(arg, (\"--\" CL_ARG__FILTER \"=\"), &arg)) {\n \t\t\tparse_list_objects_filter(&args.filter_options, arg);\n \t\t\tcontinue;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 707bbc31fd..3212957dae 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -285,10 +285,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n-\tif (!args->no_dependents) {\n-\t\tmark_tips(negotiator, args->negotiation_tips);\n-\t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n-\t}\n+\tmark_tips(negotiator, args->negotiation_tips);\n+\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n \n \tfetching = 0;\n \tfor ( ; refs ; refs = refs->next) {\n@@ -305,12 +303,8 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t * We use lookup_object here because we are only\n \t\t * interested in the case we *know* the object is\n \t\t * reachable and we have already scanned it.\n-\t\t *\n-\t\t * Do this only if args->no_dependents is false (if it is true,\n-\t\t * we cannot trust the object flags).\n \t\t */\n-\t\tif (!args->no_dependents &&\n-\t\t ((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n \t\t\t\t(o->flags & COMPLETE)) {\n \t\t\tcontinue;\n \t\t}\n@@ -410,8 +404,6 @@ static int find_common(struct fetch_negotiator *negotiator,\n \ttrace2_region_enter(\"fetch-pack\", \"negotiation_v0_v1\", the_repository);\n \tflushes = 0;\n \tretval = -1;\n-\tif (args->no_dependents)\n-\t\tgoto done;\n \twhile ((oid = negotiator->next(negotiator))) {\n \t\tpacket_buf_write(&req_buf, \"have %s\\n\", oid_to_hex(oid));\n \t\tprint_verbose(args, \"have %s\", oid_to_hex(oid));\n@@ -666,9 +658,7 @@ struct loose_object_iter {\n \n /*\n * Mark recent commits available locally and reachable from a local ref as\n- * COMPLETE. If args->no_dependents is false, also mark COMPLETE remote refs as\n- * COMMON_REF (otherwise, we are not planning to participate in negotiation, and\n- * thus do not need COMMON_REF marks).\n+ * COMPLETE.\n *\n * The cutoff time for recency is determined by this heuristic: it is the\n * earliest commit time of the objects in refs that are commits and that we know\n@@ -969,12 +959,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tstruct fetch_negotiator negotiator_alloc;\n \tstruct fetch_negotiator *negotiator;\n \n-\tif (args->no_dependents) {\n-\t\tnegotiator = NULL;\n-\t} else {\n-\t\tnegotiator = &negotiator_alloc;\n-\t\tfetch_negotiator_init(r, negotiator);\n-\t}\n+\tnegotiator = &negotiator_alloc;\n+\tfetch_negotiator_init(r, negotiator);\n \n \tsort_ref_list(&ref, ref_compare_name);\n \tQSORT(sought, nr_sought, cmp_ref_by_name);\n@@ -1062,15 +1048,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (!server_supports_hash(the_hash_algo->name, NULL))\n \t\tdie(_(\"Server does not support this repository's object format\"));\n \n-\tif (!args->no_dependents) {\n-\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n-\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\tif (everything_local(args, &ref)) {\n-\t\t\tpacket_flush(fd[1]);\n-\t\t\tgoto all_done;\n-\t\t}\n-\t} else {\n-\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\tmark_complete_and_common_ref(negotiator, args, &ref);\n+\tfilter_refs(args, &ref, sought, nr_sought);\n+\tif (everything_local(args, &ref)) {\n+\t\tpacket_flush(fd[1]);\n+\t\tgoto all_done;\n \t}\n \tif (find_common(negotiator, args, fd, &oid, ref) < 0)\n \t\tif (!args->keep_pack)\n@@ -1119,7 +1101,7 @@ static void add_shallow_requests(struct strbuf *req_buf,\n \t\tpacket_buf_write(req_buf, \"deepen-relative\\n\");\n }\n \n-static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)\n+static void add_wants(const struct ref *wants, struct strbuf *req_buf)\n {\n \tint use_ref_in_want = server_supports_feature(\"fetch\", \"ref-in-want\", 0);\n \n@@ -1136,12 +1118,8 @@ static void add_wants(int no_dependents, const struct ref *wants, struct strbuf\n \t\t * We use lookup_object here because we are only\n \t\t * interested in the case we *know* the object is\n \t\t * reachable and we have already scanned it.\n-\t\t *\n-\t\t * Do this only if args->no_dependents is false (if it is true,\n-\t\t * we cannot trust the object flags).\n \t\t */\n-\t\tif (!no_dependents &&\n-\t\t ((o = lookup_object(the_repository, remote)) != NULL) &&\n+\t\tif (((o = lookup_object(the_repository, remote)) != NULL) &&\n \t\t (o->flags & COMPLETE)) {\n \t\t\tcontinue;\n \t\t}\n@@ -1275,19 +1253,14 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t}\n \n \t/* add wants */\n-\tadd_wants(args->no_dependents, wants, &req_buf);\n+\tadd_wants(wants, &req_buf);\n \n-\tif (args->no_dependents) {\n-\t\tpacket_buf_write(&req_buf, \"done\");\n-\t\tret = 1;\n-\t} else {\n-\t\t/* Add all of the common commits we've found in previous rounds */\n-\t\tadd_common(&req_buf, common);\n+\t/* Add all of the common commits we've found in previous rounds */\n+\tadd_common(&req_buf, common);\n \n-\t\t/* Add initial haves */\n-\t\tret = add_haves(negotiator, seen_ack, &req_buf,\n-\t\t\t\thaves_to_send, in_vain);\n-\t}\n+\t/* Add initial haves */\n+\tret = add_haves(negotiator, seen_ack, &req_buf,\n+\t\t\thaves_to_send, in_vain);\n \n \t/* Send request */\n \tpacket_buf_flush(&req_buf);\n@@ -1547,12 +1520,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \tstruct string_list packfile_uris = STRING_LIST_INIT_DUP;\n \tint i;\n \n-\tif (args->no_dependents) {\n-\t\tnegotiator = NULL;\n-\t} else {\n-\t\tnegotiator = &negotiator_alloc;\n-\t\tfetch_negotiator_init(r, negotiator);\n-\t}\n+\tnegotiator = &negotiator_alloc;\n+\tfetch_negotiator_init(r, negotiator);\n \n \tpacket_reader_init(&reader, fd[0], NULL, 0,\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n@@ -1576,21 +1545,16 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\targs->deepen = 1;\n \n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n-\t\t\tif (!args->no_dependents) {\n-\t\t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n-\t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\t\tif (everything_local(args, &ref))\n-\t\t\t\t\tstate = FETCH_DONE;\n-\t\t\t\telse\n-\t\t\t\t\tstate = FETCH_SEND_REQUEST;\n-\n-\t\t\t\tmark_tips(negotiator, args->negotiation_tips);\n-\t\t\t\tfor_each_cached_alternate(negotiator,\n-\t\t\t\t\t\t\t insert_one_alternate_object);\n-\t\t\t} else {\n-\t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\t\t\tmark_complete_and_common_ref(negotiator, args, &ref);\n+\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\t\t\tif (everything_local(args, &ref))\n+\t\t\t\tstate = FETCH_DONE;\n+\t\t\telse\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-\t\t\t}\n+\n+\t\t\tmark_tips(negotiator, args->negotiation_tips);\n+\t\t\tfor_each_cached_alternate(negotiator,\n+\t\t\t\t\t\t insert_one_alternate_object);\n \t\t\tbreak;\n \t\tcase FETCH_SEND_REQUEST:\n \t\t\tif (!negotiation_started) {\n@@ -1911,20 +1875,6 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n-\tif (args->no_dependents && !args->filter_options.choice) {\n-\t\t/*\n-\t\t * The protocol does not support requesting that only the\n-\t\t * wanted objects be sent, so approximate this by setting a\n-\t\t * \"blob:none\" filter if no filter is already set. This works\n-\t\t * for all object types: note that wanted blobs will still be\n-\t\t * sent because they are directly specified as a \"want\".\n-\t\t *\n-\t\t * NEEDSWORK: Add an option in the protocol to request that\n-\t\t * only the wanted objects be sent, and implement it.\n-\t\t */\n-\t\tparse_list_objects_filter(&args->filter_options, \"blob:none\");\n-\t}\n-\n \tif (version != protocol_v2 && !ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 85d1e39fe7..bbe2938059 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -42,20 +42,6 @@ struct fetch_pack_args {\n \tunsigned deepen:1;\n \tunsigned from_promisor:1;\n \n-\t/*\n-\t * Attempt to fetch only the wanted objects, and not any objects\n-\t * referred to by them. Due to protocol limitations, extraneous\n-\t * objects may still be included. (When fetching non-blob\n-\t * objects, only blobs are excluded; when fetching a blob, the\n-\t * blob itself will still be sent. The client does not need to\n-\t * know whether a wanted object is a blob or not.)\n-\t *\n-\t * If 1, fetch_pack() will also not modify any object flags.\n-\t * This allows fetch_pack() to safely be called by any function,\n-\t * regardless of which object flags it uses (if any).\n-\t */\n-\tunsigned no_dependents:1;\n-\n \t/*\n \t * Because fetch_pack() overwrites the shallow file upon a\n \t * successful deepening non-clone fetch, if this struct\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 5cbc6e5002..a0c81a64bc 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -40,7 +40,6 @@ struct options {\n \t\tpush_cert : 2,\n \t\tdeepen_relative : 1,\n \t\tfrom_promisor : 1,\n-\t\tno_dependents : 1,\n \t\tatomic : 1,\n \t\tobject_format : 1;\n \tconst struct git_hash_algo *hash_algo;\n@@ -186,9 +185,6 @@ static int set_option(const char *name, const char *value)\n \t} else if (!strcmp(name, \"from-promisor\")) {\n \t\toptions.from_promisor = 1;\n \t\treturn 0;\n-\t} else if (!strcmp(name, \"no-dependents\")) {\n-\t\toptions.no_dependents = 1;\n-\t\treturn 0;\n \t} else if (!strcmp(name, \"filter\")) {\n \t\toptions.filter = xstrdup(value);\n \t\treturn 0;\n@@ -1171,8 +1167,6 @@ static int fetch_git(struct discovery *heads,\n \t\targv_array_push(&args, \"--deepen-relative\");\n \tif (options.from_promisor)\n \t\targv_array_push(&args, \"--from-promisor\");\n-\tif (options.no_dependents)\n-\t\targv_array_push(&args, \"--no-dependents\");\n \tif (options.filter)\n \t\targv_array_pushf(&args, \"--filter=%s\", options.filter);\n \targv_array_push(&args, url.buf);\ndiff --git a/transport.c b/transport.c\nindex b41386eccb..32e1f21f0c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -232,9 +232,6 @@ static int set_git_option(struct git_transport_options *opts,\n \t} else if (!strcmp(name, TRANS_OPT_FROM_PROMISOR)) {\n \t\topts->from_promisor = !!value;\n \t\treturn 0;\n-\t} else if (!strcmp(name, TRANS_OPT_NO_DEPENDENTS)) {\n-\t\topts->no_dependents = !!value;\n-\t\treturn 0;\n \t} else if (!strcmp(name, TRANS_OPT_LIST_OBJECTS_FILTER)) {\n \t\tlist_objects_filter_die_if_populated(&opts->filter_options);\n \t\tparse_list_objects_filter(&opts->filter_options, value);\n@@ -359,7 +356,6 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.cloning = transport->cloning;\n \targs.update_shallow = data->options.update_shallow;\n \targs.from_promisor = data->options.from_promisor;\n-\targs.no_dependents = data->options.no_dependents;\n \targs.filter_options = data->options.filter_options;\n \targs.stateless_rpc = transport->stateless_rpc;\n \targs.server_options = transport->server_options;\ndiff --git a/transport.h b/transport.h\nindex b3c30133ea..7aa1f33145 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -16,7 +16,6 @@ struct git_transport_options {\n \tunsigned update_shallow : 1;\n \tunsigned deepen_relative : 1;\n \tunsigned from_promisor : 1;\n-\tunsigned no_dependents : 1;\n \n \t/*\n \t * If this transport supports connect or stateless-connect,\n@@ -201,12 +200,6 @@ void transport_check_allowed(const char *type);\n /* Indicate that these objects are being fetched by a promisor */\n #define TRANS_OPT_FROM_PROMISOR \"from-promisor\"\n \n-/*\n- * Indicate that only the objects wanted need to be fetched, not their\n- * dependents\n- */\n-#define TRANS_OPT_NO_DEPENDENTS \"no-dependents\"\n-\n /* Filter objects for partial clone and fetch */\n #define TRANS_OPT_LIST_OBJECTS_FILTER \"filter\"\n \n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403967","messageId":"xmqqimdfybh5.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"cover.1597722941.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 0/7] Lazy fetch with subprocess","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-18T19:56:22Z","receivedAt":"2020-08-18T19:56:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> These patches are based on jc/no-update-fetch-head, once again.\n\nFWIW, that topic no longer exists (it has been swallowed by\nDerrick's maintenance topics). As that topic is not near 'next'\nyet, we might want to kick the patch out of it into a separate\njc/no-update-fetch-head topic again, base this series and also\nmaintenance topics on top of it.\n\n> 2: 00ad7dd875 = 2: 9f277f1631 fetch: allow refspecs specified through stdin\n> 3: 8b4a522a13 ! 3: fda9f834f6 fetch: avoid reading submodule config until needed\n> @@ Metadata\n> ## Commit message ##\n> fetch: avoid reading submodule config until needed\n> \n> - Teach \"git fetch\" to avoid reading the submodule config until necessary.\n> - This allows users to avoid the lazy-fetching of this potentially missing\n> - config file by specifying the --recurse-submodules=no command line\n> - option.\n> + In \"fetch\", there are two parameters submodule_fetch_jobs_config and\n> + recurse_submodules that can be set in a variety of ways: through\n> + .gitmodules, through .git/config, and through the command line.\n> + Currently \"fetch\" handles this by first reading .gitmodules, then\n> + reading .git/config (allowing it to overwrite existing values), then\n> + reading the command line (allowing it to overwrite existing values).\n> +\n> + Notice that we can avoid reading .gitmodules if .git/config and/or the\n> + command line already provides us with what we need. In addition, if\n> + recurse_submodules is found to be \"no\", we do not need the value of\n> + submodule_fetch_jobs_config.\n> +\n> + Avoiding reading .gitmodules is especially important when we use \"git\n> + fetch\" to perform lazy fetches in a partial clone because the\n> + .gitmodules file itself might need to be lazy fetched (and otherwise\n> + causing an infinite loop).\n> +\n> + In light of all this, avoid reading .gitmodules until necessary. When\n> + reading it, we may only need one of the two parameters it provides, so\n> + teach fetch_config_from_gitmodules() to support NULL arguments. With\n> + this patch, users (including Git itself when invoking \"git fetch\" to\n> + lazy-fetch) will be able to guarantee avoiding reading .gitmodules by\n> + passing --recurse-submodules=no.\n\nQuite sensible.\n\n> 4: 77bc83e7f2 ! 4: a5554cd27f fetch: only populate existing_refs if needed\n> @@ Metadata\n> ## Commit message ##\n> fetch: only populate existing_refs if needed\n> \n> - When fetching tags, Git only writes tags that do not already exist in\n> - the client repository. This necessitates an iteration over all the refs,\n> - but fetch performs this iteration even if no tags are fetched.\n> + In \"fetch\", get_ref_map() iterates over all refs to populate\n> + \"existing_refs\" in order to populate peer_ref->old_oid in the returned\n> + refmap, even if the refmap has no peer_ref set - which is the case when\n> + only literal hashes (i.e. no refs by name) are fetched.\n\nMuch better---the previous round gave us a wrong impression that\nthe change is about the behaviour when fetching tags, but the\nupdated explanation makes it clear that the primary use case is to\navoid tag-following while directly fetching objects by names, not\nvia refs.\n"},{"id":"403978","messageId":"xmqqk0xvwpob.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"xmqqimdfybh5.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 0/7] Lazy fetch with subprocess","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-18T22:32:36Z","receivedAt":"2020-08-18T22:32:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Tan writes:\n>\n>> These patches are based on jc/no-update-fetch-head, once again.\n>\n> FWIW, that topic no longer exists (it has been swallowed by\n> Derrick's maintenance topics). As that topic is not near 'next'\n> yet, we might want to kick the patch out of it into a separate\n> jc/no-update-fetch-head topic again, base this series and also\n> maintenance topics on top of it.\n\nSo, I've recreated jc/no-update-fetch-head on top of 'master' and\nthen rebased these patches on top (which required some adjustment\ndue to argv-array -> strvec API rename).\n\nt5300 stops passing at \"[6/7] promisor-remote: lazy-fetch\", which\nI haven't looked into details, but the topic is queued near the tip\nof 'seen' for today's integration cycle.\n\nThanks.\n\n\n"},{"id":"403984","messageId":"20200818233630.2829739-1-jonathantanmy@google.com","threadId":"53920","inReplyTo":"xmqqk0xvwpob.fsf@gitster.c.googlers.com","subject":"[PATCH] fixup! promisor-remote: lazy-fetch objects in subprocess","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-18T23:36:30Z","receivedAt":"2020-08-18T23:36:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"---\nAh, sorry about that - here's a fixup. I forgot to switch the name of\nthe negotiator in the patch (and also because of the change to a \"fetch\"\nsubprocess, the trace is reported as \"fetch>\", not \"git>\").\n\n promisor-remote.c | 2 +-\n t/t5300-pack-object.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 56d6d4d821..6530e26f98 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -23,7 +23,7 @@ static int fetch_objects(const char *remote_name,\n \n \tchild.git_cmd = 1;\n \tchild.in = -1;\n-\tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=null\",\n+\tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n \t\t \"fetch\", remote_name, \"--no-tags\",\n \t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n \t\t \"--filter=blob:none\", \"--stdin\", NULL);\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex 3d6a93343a..392201cabd 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -528,7 +528,7 @@ test_expect_success 'prefetch objects' '\n \tTWO=$(git -C server rev-parse three_branch^) &&\n \tgit -C client fetch --filter=blob:none origin \"$TWO\" &&\n \tGIT_TRACE_PACKET=$(pwd)/trace git -C client push origin \"$TWO\":refs/heads/two_branch &&\n-\tgrep \"git> done\" trace >donelines &&\n+\tgrep \"fetch> done\" trace >donelines &&\n \ttest_line_count = 1 donelines\n '\n \n-- \n2.28.0.220.ged08abb693-goog\n\n"},{"id":"403987","messageId":"xmqq7dtvwlr1.fsf@gitster.c.googlers.com","threadId":"53920","inReplyTo":"20200818233630.2829739-1-jonathantanmy@google.com","subject":"Re: [PATCH] fixup! promisor-remote: lazy-fetch objects in subprocess","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-18T23:57:22Z","receivedAt":"2020-08-18T23:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> ---\n> Ah, sorry about that - here's a fixup. I forgot to switch the name of\n> the negotiator in the patch (and also because of the change to a \"fetch\"\n> subprocess, the trace is reported as \"fetch>\", not \"git>\").\n\nExcellent. Thanks for a quick update.\n\n>\n> promisor-remote.c | 2 +-\n> t/t5300-pack-object.sh | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 56d6d4d821..6530e26f98 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -23,7 +23,7 @@ static int fetch_objects(const char *remote_name,\n> \n> \tchild.git_cmd = 1;\n> \tchild.in = -1;\n> -\tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=null\",\n> +\tstrvec_pushl(&child.args, \"-c\", \"fetch.negotiationAlgorithm=noop\",\n> \t\t \"fetch\", remote_name, \"--no-tags\",\n> \t\t \"--no-write-fetch-head\", \"--recurse-submodules=no\",\n> \t\t \"--filter=blob:none\", \"--stdin\", NULL);\n> diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\n> index 3d6a93343a..392201cabd 100755\n> --- a/t/t5300-pack-object.sh\n> +++ b/t/t5300-pack-object.sh\n> @@ -528,7 +528,7 @@ test_expect_success 'prefetch objects' '\n> \tTWO=$(git -C server rev-parse three_branch^) &&\n> \tgit -C client fetch --filter=blob:none origin \"$TWO\" &&\n> \tGIT_TRACE_PACKET=$(pwd)/trace git -C client push origin \"$TWO\":refs/heads/two_branch &&\n> -\tgrep \"git> done\" trace >donelines &&\n> +\tgrep \"fetch> done\" trace >donelines &&\n> \ttest_line_count = 1 donelines\n> '\n"}]}