{"thread":{"id":"50820","subject":"[PATCH 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully","startedAt":"2019-03-25T20:43:32Z","lastAt":"2019-03-26T19:31:39Z","messageCount":18,"participants":["Jonathan Tan","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"372468","messageId":"cover.1553546216.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":null,"subject":"[PATCH 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-25T20:43:21Z","receivedAt":"2019-03-25T20:43:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is built on master+jt/test-protocol-version, but should apply\ncleanly to master.\n\nAfter [1] and this patch set, there is only one more small thing to fix\nor mark GIT_TEST_PROTOCOL_VERSION=0 at master+jt/test-protocol-version.\n\nThis is very similar to [2], which was neither reviewed nor merged,\nperhaps because I had another patch [3] that I declared to supersede [2]\nand that was merged as bd0b42aed3 (\"fetch-pack: do not take shallow lock\nunnecessarily\", 2019-01-10). But it turns out that [3] was not enough to\nmake all tests pass with GIT_TEST_PROTOCOL_VERSION=2, hence this patch\nset.\n\n[1] https://public-inbox.org/git/20190322190139.151189-1-jonathantanmy@google.com/\n[2] https://public-inbox.org/git/20181220195349.92214-1-jonathantanmy@google.com/\n[3] https://public-inbox.org/git/20190110193645.34080-1-jonathantanmy@google.com/\n\nJonathan Tan (2):\n  fetch-pack: call prepare_shallow_info only if v0\n  fetch-pack: respect --no-update-shallow in v2\n\n commit.h     |  4 ++++\n fetch-pack.c | 54 +++++++++++++++++++++++++++++++++++++++++-----------\n 2 files changed, 47 insertions(+), 11 deletions(-)\n\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372469","messageId":"d2eb101709cc9b300e38151b14c97f96a2b14188.1553546216.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553546216.git.jonathantanmy@google.com","subject":"[PATCH 1/2] fetch-pack: call prepare_shallow_info only if v0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-25T20:43:22Z","receivedAt":"2019-03-25T20:43:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In fetch_pack(), be clearer that there is no shallow information before\nthe fetch when v2 is used - memset the struct shallow_info to 0 instead\nof calling prepare_shallow_info().\n\nThis patch is in preparation for a future patch in which a v2 fetch\nmight call prepare_shallow_info() after shallow info has been retrieved\nduring the fetch, so I needed to ensure that prepare_shallow_info() is\nnot called before the fetch.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n commit.h     |  4 ++++\n fetch-pack.c | 10 +++++++---\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 42728c2906..a3f2b2eddb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -257,6 +257,10 @@ extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n extern const char *setup_temporary_shallow(const struct oid_array *extra);\n extern void advertise_shallow_grafts(int);\n \n+/*\n+ * Initialize with prepare_shallow_info() or zero-initialize (equivalent to\n+ * prepare_shallow_info with a NULL oid_array).\n+ */\n struct shallow_info {\n \tstruct oid_array *shallow;\n \tint *ours, nr_ours;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex e69993b2eb..a0eb268dfc 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1648,13 +1648,17 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\n \t}\n-\tprepare_shallow_info(&si, shallow);\n-\tif (version == protocol_v2)\n+\tif (version == protocol_v2) {\n+\t\tif (shallow && shallow->nr)\n+\t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n+\t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t   pack_lockfile);\n-\telse\n+\t} else {\n+\t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t&si, pack_lockfile);\n+\t}\n \treprepare_packed_git(the_repository);\n \n \tif (!args->cloning && args->deepen) {\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372470","messageId":"c4d2f409e246cce02ebfdb8c7110e3700d066ec8.1553546216.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553546216.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-25T20:43:23Z","receivedAt":"2019-03-25T20:43:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In protocol v0, when sending \"shallow\" lines, the server distinguishes\nbetween lines caused by the remote repo being shallow and lines caused\nby client-specified depth settings. Unless \"--update-shallow\" is\nspecified, there is a difference in behavior: refs that reach the former\n\"shallow\" lines, but not the latter, are rejected. But in v2, the server\ndoes not, and the client treats all \"shallow\" lines like lines caused by\nclient-specified depth settings.\n\nFull restoration of v0 functionality is not possible without protocol\nchange, but we can implement a heuristic: if we specify any depth\nsetting, treat all \"shallow\" lines like lines caused by client-specified\ndepth settings (that is, unaffected by \"--no-update-shallow\"), but\notherwise, treat them like lines caused by the remote repo being shallow\n(that is, affected by \"--no-update-shallow\"). This restores most of v0\nbehavior, except in the case where a client fetches from a shallow\nrepository with depth settings.\n\nThis patch causes a test that previously failed with\nGIT_TEST_PROTOCOL_VERSION=2 to pass.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 36 insertions(+), 8 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a0eb268dfc..672c79c91a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1253,9 +1253,13 @@ static int process_acks(struct fetch_negotiator *negotiator,\n }\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n-\t\t\t\t struct packet_reader *reader)\n+\t\t\t\t struct packet_reader *reader,\n+\t\t\t\t struct shallow_info *si)\n {\n-\tint line_received = 0;\n+\tstruct oid_array *shallows;\n+\tint unshallow_received = 0;\n+\n+\tshallows = xcalloc(1, sizeof(*shallows));\n \n \tprocess_section_header(reader, \"shallow-info\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n@@ -1265,8 +1269,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\tif (skip_prefix(reader->line, \"shallow \", &arg)) {\n \t\t\tif (get_oid_hex(arg, &oid))\n \t\t\t\tdie(_(\"invalid shallow line: %s\"), reader->line);\n-\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\tline_received = 1;\n+\t\t\toid_array_append(shallows, &oid);\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(reader->line, \"unshallow \", &arg)) {\n@@ -1279,7 +1282,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\t\t\tdie(_(\"error in object: %s\"), reader->line);\n \t\t\tif (unregister_shallow(&oid))\n \t\t\t\tdie(_(\"no shallow found: %s\"), reader->line);\n-\t\t\tline_received = 1;\n+\t\t\tunshallow_received = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tdie(_(\"expected shallow/unshallow, got %s\"), reader->line);\n@@ -1289,11 +1292,35 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t    reader->status != PACKET_READ_DELIM)\n \t\tdie(_(\"error processing shallow info: %d\"), reader->status);\n \n-\tif (line_received) {\n+\tif (args->deepen || unshallow_received) {\n+\t\t/*\n+\t\t * Treat these as shallow lines caused by our depth settings.\n+\t\t * In v0, these lines cannot cause refs to be rejected; do the\n+\t\t * same.\n+\t\t */\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < shallows->nr; i++)\n+\t\t\tregister_shallow(the_repository, &shallows->oid[i]);\n+\t\toid_array_clear(shallows);\n+\t\tfree(shallows);\n \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n \t\t\t\t\tNULL);\n \t\targs->deepen = 1;\n+\t} else if (shallows->nr) {\n+\t\t/*\n+\t\t * Treat these as shallow lines caused by the remote being\n+\t\t * shallow. In v0, remote refs that reach these objects are\n+\t\t * rejected (unless --update-shallow is set); do the same.\n+\t\t */\n+\t\tprepare_shallow_info(si, shallows);\n+\t\tif (si->nr_ours || si->nr_theirs)\n+\t\t\talternate_shallow_file =\n+\t\t\t\tsetup_temporary_shallow(si->shallow);\n+\t\telse\n+\t\t\talternate_shallow_file = NULL;\n \t} else {\n+\t\tfree(shallows);\n \t\talternate_shallow_file = NULL;\n \t}\n }\n@@ -1337,6 +1364,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t    int fd[2],\n \t\t\t\t    const struct ref *orig_ref,\n \t\t\t\t    struct ref **sought, int nr_sought,\n+\t\t\t\t    struct shallow_info *si,\n \t\t\t\t    char **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1411,7 +1439,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_GET_PACK:\n \t\t\t/* Check for shallow-info section */\n \t\t\tif (process_section_header(&reader, \"shallow-info\", 1))\n-\t\t\t\treceive_shallow_info(args, &reader);\n+\t\t\t\treceive_shallow_info(args, &reader, si);\n \n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n@@ -1653,7 +1681,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n \t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n-\t\t\t\t\t   pack_lockfile);\n+\t\t\t\t\t   &si, pack_lockfile);\n \t} else {\n \t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372492","messageId":"20190326050011.GA1933@sigill.intra.peff.net","threadId":"50820","inReplyTo":"d2eb101709cc9b300e38151b14c97f96a2b14188.1553546216.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] fetch-pack: call prepare_shallow_info only if v0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T05:00:12Z","receivedAt":"2019-03-26T05:00:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2019 at 01:43:22PM -0700, Jonathan Tan wrote:\n\n> In fetch_pack(), be clearer that there is no shallow information before\n> the fetch when v2 is used - memset the struct shallow_info to 0 instead\n> of calling prepare_shallow_info().\n> \n> This patch is in preparation for a future patch in which a v2 fetch\n> might call prepare_shallow_info() after shallow info has been retrieved\n> during the fetch, so I needed to ensure that prepare_shallow_info() is\n> not called before the fetch.\n\nMakes sense.\n\nI wondered here:\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index e69993b2eb..a0eb268dfc 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1648,13 +1648,17 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n>  \t\tpacket_flush(fd[1]);\n>  \t\tdie(_(\"no matching remote head\"));\n>  \t}\n> -\tprepare_shallow_info(&si, shallow);\n> -\tif (version == protocol_v2)\n> +\tif (version == protocol_v2) {\n> +\t\tif (shallow && shallow->nr)\n> +\t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n> +\t\tmemset(&si, 0, sizeof(si));\n>  \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n>  \t\t\t\t\t   pack_lockfile);\n\n...who actually might set \"shallow\". Specifically, I was curious whether\na v2 server could erroneously send us shallow lines, which would trigger\nour BUG(), when in fact we should be complaining about the server.\n\nBut the answer is no, they have no opportunity. AFAICT, this would only\nbe set by a call to get_remote_heads(), which we do only for v0/v1\nprotocols. So there's no way to trigger the BUG. Good.\n\n-Peff\n"},{"id":"372493","messageId":"20190326052011.GB1933@sigill.intra.peff.net","threadId":"50820","inReplyTo":"c4d2f409e246cce02ebfdb8c7110e3700d066ec8.1553546216.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T05:20:11Z","receivedAt":"2019-03-26T05:20:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:\n\n> In protocol v0, when sending \"shallow\" lines, the server distinguishes\n> between lines caused by the remote repo being shallow and lines caused\n> by client-specified depth settings. Unless \"--update-shallow\" is\n> specified, there is a difference in behavior: refs that reach the former\n> \"shallow\" lines, but not the latter, are rejected. But in v2, the server\n> does not, and the client treats all \"shallow\" lines like lines caused by\n> client-specified depth settings.\n> \n> Full restoration of v0 functionality is not possible without protocol\n> change,\n\nThat's rather unfortunate. Is this because the v2 ls-refs phase is\nseparate, and that's when a v0 server would tell us about its shallows?\nIt looks like in v2 it comes in a separate \"shallow-info\" section.\n\nWhat would the protocol change look like?  Would we just need a\ncapability to instruct the server to mark the two different types of\nshallow distinctly? Or do we actually need to convey the information\nseparately (e.g., in the ls-refs phase)?\n\nNone of that matters for your patch here, but I'm just wondering what\nthe path forward is.\n\n> but we can implement a heuristic: if we specify any depth\n> setting, treat all \"shallow\" lines like lines caused by client-specified\n> depth settings (that is, unaffected by \"--no-update-shallow\"), but\n> otherwise, treat them like lines caused by the remote repo being shallow\n> (that is, affected by \"--no-update-shallow\"). This restores most of v0\n> behavior, except in the case where a client fetches from a shallow\n> repository with depth settings.\n\nThat seems like the best we can do without the protocol change. And even\nif we adjust the protocol, we need some fallback behavior for existing\nv2 servers, so this is worth doing.\n\n>  fetch-pack.c | 44 ++++++++++++++++++++++++++++++++++++--------\n>  1 file changed, 36 insertions(+), 8 deletions(-)\n\nThe patch looks reasonable to me, though I am far from an expert on the\nshallow bits of the protocol. One thing I did notice:\n\n>  static void receive_shallow_info(struct fetch_pack_args *args,\n> -\t\t\t\t struct packet_reader *reader)\n> +\t\t\t\t struct packet_reader *reader,\n> +\t\t\t\t struct shallow_info *si)\n>  {\n> -\tint line_received = 0;\n> +\tstruct oid_array *shallows;\n> +\tint unshallow_received = 0;\n> +\n> +\tshallows = xcalloc(1, sizeof(*shallows));\n\nThis has to be heap-allocated, since we pass off ownership to \"si\"\n(sometimes). But in the v0 case, it comes from the transport's\n&data->shallows of a local variable in cmd_fetch_pack(), and we never\nfree it. So I think this oid_array ends up getting leaked.\n\nPerhaps it's worth passing down the shallows array we get from the\ncaller of fetch_pack(). Something like the patch below (I think it is\nnever NULL, which means in your patch 1 you can simplify the conditional\nfor the BUG).\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 672c79c91a..af2ee47a83 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1254,13 +1254,11 @@ static int process_acks(struct fetch_negotiator *negotiator,\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n \t\t\t\t struct packet_reader *reader,\n+\t\t\t\t struct oid_array *shallows,\n \t\t\t\t struct shallow_info *si)\n {\n-\tstruct oid_array *shallows;\n \tint unshallow_received = 0;\n \n-\tshallows = xcalloc(1, sizeof(*shallows));\n-\n \tprocess_section_header(reader, \"shallow-info\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tconst char *arg;\n@@ -1303,7 +1301,6 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\tfor (i = 0; i < shallows->nr; i++)\n \t\t\tregister_shallow(the_repository, &shallows->oid[i]);\n \t\toid_array_clear(shallows);\n-\t\tfree(shallows);\n \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n \t\t\t\t\tNULL);\n \t\targs->deepen = 1;\n@@ -1320,7 +1317,6 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\telse\n \t\t\talternate_shallow_file = NULL;\n \t} else {\n-\t\tfree(shallows);\n \t\talternate_shallow_file = NULL;\n \t}\n }\n@@ -1365,6 +1361,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t    const struct ref *orig_ref,\n \t\t\t\t    struct ref **sought, int nr_sought,\n \t\t\t\t    struct shallow_info *si,\n+\t\t\t\t    struct oid_array *shallows,\n \t\t\t\t    char **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1439,7 +1436,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_GET_PACK:\n \t\t\t/* Check for shallow-info section */\n \t\t\tif (process_section_header(&reader, \"shallow-info\", 1))\n-\t\t\t\treceive_shallow_info(args, &reader, si);\n+\t\t\t\treceive_shallow_info(args, &reader, shallows, si);\n \n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n@@ -1681,7 +1678,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n \t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n-\t\t\t\t\t   &si, pack_lockfile);\n+\t\t\t\t\t   &si, shallow, pack_lockfile);\n \t} else {\n \t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n"},{"id":"372509","messageId":"CACsJy8BHBR2OAHjr-LO4HEmwrmga++5RS1K76qKnh36Hsw9Chw@mail.gmail.com","threadId":"50820","inReplyTo":"20190326052011.GB1933@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-26T10:14:11Z","receivedAt":"2019-03-26T10:14:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 26, 2019 at 12:20 PM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:\n>\n> > In protocol v0, when sending \"shallow\" lines, the server distinguishes\n> > between lines caused by the remote repo being shallow and lines caused\n> > by client-specified depth settings. Unless \"--update-shallow\" is\n> > specified, there is a difference in behavior: refs that reach the former\n> > \"shallow\" lines, but not the latter, are rejected. But in v2, the server\n> > does not, and the client treats all \"shallow\" lines like lines caused by\n> > client-specified depth settings.\n> >\n> > Full restoration of v0 functionality is not possible without protocol\n> > change,\n>\n> That's rather unfortunate. Is this because the v2 ls-refs phase is\n> separate, and that's when a v0 server would tell us about its shallows?\n> It looks like in v2 it comes in a separate \"shallow-info\" section.\n>\n> What would the protocol change look like?  Would we just need a\n> capability to instruct the server to mark the two different types of\n> shallow distinctly? Or do we actually need to convey the information\n> separately (e.g., in the ls-refs phase)?\n>\n> None of that matters for your patch here, but I'm just wondering what\n> the path forward is.\n\nIf it helps (because I'm still catching up with v2 to actually help\nreview), this case is for cloning from a shallow repo. The commit that\noutlines how .git/shallow is updated is 58babfffde (shallow.c: the 8\nsteps to select new commits for .git/shallow, 2013-12-05).\n\nSince the first shallow info is about the shape of the remote repo\n(where refs are the tips), ls-refs sounds like the right place to\ninclude the information. In other words, ls-refs currently tells the\ntip/top of the repo, what's missing is the piece about \"the bottom\"\n(shallow cut points).\n\n> > but we can implement a heuristic: if we specify any depth\n> > setting, treat all \"shallow\" lines like lines caused by client-specified\n> > depth settings (that is, unaffected by \"--no-update-shallow\"), but\n> > otherwise, treat them like lines caused by the remote repo being shallow\n> > (that is, affected by \"--no-update-shallow\"). This restores most of v0\n> > behavior, except in the case where a client fetches from a shallow\n> > repository with depth settings.\n>\n> That seems like the best we can do without the protocol change. And even\n> if we adjust the protocol, we need some fallback behavior for existing\n> v2 servers, so this is worth doing.\n\nAre people actually doing this (i.e. cloning from or  pushing to a\nshallow repo)? I added this with the intention that a big shallow repo\n(e.g. one year long history) is served as the common source to reduce\nserver loads and everything, while the full/big repo is available but\nrarely needed. I never saw anyone complain about it (so, likely not\nusing it).\n\nThe description of --update-shallow probably should mention this\nfallback behavior? --update-shallow was not default because I feared\nthe local repo could be cut short by unsolicited shallow requests from\nthe server side, and it looks like --update-shallow is default (by\nmistake) in v2? Maybe I worried for nothing. I dunno.\n-- \nDuy\n"},{"id":"372523","messageId":"20190326151825.GA26167@sigill.intra.peff.net","threadId":"50820","inReplyTo":"CACsJy8BHBR2OAHjr-LO4HEmwrmga++5RS1K76qKnh36Hsw9Chw@mail.gmail.com","subject":"Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T15:18:25Z","receivedAt":"2019-03-26T15:18:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2019 at 05:14:11PM +0700, Duy Nguyen wrote:\n\n> > That seems like the best we can do without the protocol change. And even\n> > if we adjust the protocol, we need some fallback behavior for existing\n> > v2 servers, so this is worth doing.\n> \n> Are people actually doing this (i.e. cloning from or  pushing to a\n> shallow repo)? I added this with the intention that a big shallow repo\n> (e.g. one year long history) is served as the common source to reduce\n> server loads and everything, while the full/big repo is available but\n> rarely needed. I never saw anyone complain about it (so, likely not\n> using it).\n\nI don't think I've ever seen anybody serve fetches out of a shallow\nclone in practice (I don't think we ever seriously considered them at\nGitHub, but given their general incompatibility with reachability\nbitmaps, I suspect it would cause more performance problems than it\nsolves).\n\nI've always imagined people do it for one-offs. E.g., they have a\nshallow clone, and fetch out of that to a temporary copy. That may be\nless useful these days with the advent of separate worktrees.\n\n-Peff\n"},{"id":"372525","messageId":"CACsJy8A5F5Cj02ViO31oFHzOPrdUr+=Gp1q1BEXz7kHDkZTz3A@mail.gmail.com","threadId":"50820","inReplyTo":"20190326151825.GA26167@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-26T15:39:39Z","receivedAt":"2019-03-26T15:40:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 26, 2019 at 10:18 PM Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Mar 26, 2019 at 05:14:11PM +0700, Duy Nguyen wrote:\n>\n> > > That seems like the best we can do without the protocol change. And even\n> > > if we adjust the protocol, we need some fallback behavior for existing\n> > > v2 servers, so this is worth doing.\n> >\n> > Are people actually doing this (i.e. cloning from or  pushing to a\n> > shallow repo)? I added this with the intention that a big shallow repo\n> > (e.g. one year long history) is served as the common source to reduce\n> > server loads and everything, while the full/big repo is available but\n> > rarely needed. I never saw anyone complain about it (so, likely not\n> > using it).\n>\n> I don't think I've ever seen anybody serve fetches out of a shallow\n> clone in practice (I don't think we ever seriously considered them at\n> GitHub, but given their general incompatibility with reachability\n> bitmaps, I suspect it would cause more performance problems than it\n> solves).\n\nYeah. I still think it scales better long term because you can control\nthis history depth in the common case instead of trying to make git\nwork well with 30+ years worth of commits. I might try to make\nreachability bitmaps and commit-graph work with shallow clones one\nday, but that's pretty much in the \"hey this is fun, are you bored?\"\ncategory.\n\n> I've always imagined people do it for one-offs. E.g., they have a\n> shallow clone, and fetch out of that to a temporary copy. That may be\n> less useful these days with the advent of separate worktrees.\n-- \nDuy\n"},{"id":"372532","messageId":"20190326173706.175638-1-jonathantanmy@google.com","threadId":"50820","inReplyTo":"20190326052011.GB1933@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T17:37:06Z","receivedAt":"2019-03-26T17:37:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:\n> \n> > In protocol v0, when sending \"shallow\" lines, the server distinguishes\n> > between lines caused by the remote repo being shallow and lines caused\n> > by client-specified depth settings. Unless \"--update-shallow\" is\n> > specified, there is a difference in behavior: refs that reach the former\n> > \"shallow\" lines, but not the latter, are rejected. But in v2, the server\n> > does not, and the client treats all \"shallow\" lines like lines caused by\n> > client-specified depth settings.\n> > \n> > Full restoration of v0 functionality is not possible without protocol\n> > change,\n> \n> That's rather unfortunate. Is this because the v2 ls-refs phase is\n> separate, and that's when a v0 server would tell us about its shallows?\n> It looks like in v2 it comes in a separate \"shallow-info\" section.\n\nThat's right. In v2, it comes in \"shallow-info\", which happens right\nbefore the server sends the packfile.\n\n> What would the protocol change look like?  Would we just need a\n> capability to instruct the server to mark the two different types of\n> shallow distinctly? Or do we actually need to convey the information\n> separately (e.g., in the ls-refs phase)?\n> \n> None of that matters for your patch here, but I'm just wondering what\n> the path forward is.\n\nConveying it in the ls-refs would work.\n\n> > but we can implement a heuristic: if we specify any depth\n> > setting, treat all \"shallow\" lines like lines caused by client-specified\n> > depth settings (that is, unaffected by \"--no-update-shallow\"), but\n> > otherwise, treat them like lines caused by the remote repo being shallow\n> > (that is, affected by \"--no-update-shallow\"). This restores most of v0\n> > behavior, except in the case where a client fetches from a shallow\n> > repository with depth settings.\n> \n> That seems like the best we can do without the protocol change. And even\n> if we adjust the protocol, we need some fallback behavior for existing\n> v2 servers, so this is worth doing.\n\nThanks.\n\n> The patch looks reasonable to me, though I am far from an expert on the\n> shallow bits of the protocol. One thing I did notice:\n> \n> >  static void receive_shallow_info(struct fetch_pack_args *args,\n> > -\t\t\t\t struct packet_reader *reader)\n> > +\t\t\t\t struct packet_reader *reader,\n> > +\t\t\t\t struct shallow_info *si)\n> >  {\n> > -\tint line_received = 0;\n> > +\tstruct oid_array *shallows;\n> > +\tint unshallow_received = 0;\n> > +\n> > +\tshallows = xcalloc(1, sizeof(*shallows));\n> \n> This has to be heap-allocated, since we pass off ownership to \"si\"\n> (sometimes). But in the v0 case, it comes from the transport's\n> &data->shallows of a local variable in cmd_fetch_pack(), and we never\n> free it. So I think this oid_array ends up getting leaked.\n\nThanks for the catch.\n\n> Perhaps it's worth passing down the shallows array we get from the\n> caller of fetch_pack(). Something like the patch below (I think it is\n> never NULL, which means in your patch 1 you can simplify the conditional\n> for the BUG).\n\n[snip patch]\n\nYou're right that it is never NULL - I have removed that check. As for\npassing down the shallows array that we get from the caller of\nfetch_pack(), that would get confusing because we end up modifying the\nshallows array in some code paths, and the transport is sometimes reused\n(for example, when backfilling tags). I have instead made a\nshallows_scratch variable in fetch_pack(), and made it pass it down\n(like in the diff you provided).\n"},{"id":"372534","messageId":"cover.1553622678.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553546216.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T17:53:24Z","receivedAt":"2019-03-26T17:53:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Updated to remove the unnecessary NULL check and memory leak that Peff\nnoticed [1].\n\n(Only commit 2/2 is changed, so the range-diff only shows 1 commit.)\n\n[1] https://public-inbox.org/git/20190326052011.GB1933@sigill.intra.peff.net/\n\nJonathan Tan (2):\n  fetch-pack: call prepare_shallow_info only if v0\n  fetch-pack: respect --no-update-shallow in v2\n\n commit.h     |  4 ++++\n fetch-pack.c | 51 +++++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 45 insertions(+), 10 deletions(-)\n\nRange-diff against v1:\n1:  c4d2f409e2 ! 1:  943b1cbc61 fetch-pack: respect --no-update-shallow in v2\n    @@ -33,13 +33,11 @@\n      static void receive_shallow_info(struct fetch_pack_args *args,\n     -\t\t\t\t struct packet_reader *reader)\n     +\t\t\t\t struct packet_reader *reader,\n    ++\t\t\t\t struct oid_array *shallows,\n     +\t\t\t\t struct shallow_info *si)\n      {\n     -\tint line_received = 0;\n    -+\tstruct oid_array *shallows;\n     +\tint unshallow_received = 0;\n    -+\n    -+\tshallows = xcalloc(1, sizeof(*shallows));\n      \n      \tprocess_section_header(reader, \"shallow-info\", 0);\n      \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n    @@ -77,8 +75,6 @@\n     +\n     +\t\tfor (i = 0; i < shallows->nr; i++)\n     +\t\t\tregister_shallow(the_repository, &shallows->oid[i]);\n    -+\t\toid_array_clear(shallows);\n    -+\t\tfree(shallows);\n      \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n      \t\t\t\t\tNULL);\n      \t\targs->deepen = 1;\n    @@ -95,14 +91,13 @@\n     +\t\telse\n     +\t\t\talternate_shallow_file = NULL;\n      \t} else {\n    -+\t\tfree(shallows);\n      \t\talternate_shallow_file = NULL;\n      \t}\n    - }\n     @@\n      \t\t\t\t    int fd[2],\n      \t\t\t\t    const struct ref *orig_ref,\n      \t\t\t\t    struct ref **sought, int nr_sought,\n    ++\t\t\t\t    struct oid_array *shallows,\n     +\t\t\t\t    struct shallow_info *si,\n      \t\t\t\t    char **pack_lockfile)\n      {\n    @@ -112,16 +107,36 @@\n      \t\t\t/* Check for shallow-info section */\n      \t\t\tif (process_section_header(&reader, \"shallow-info\", 1))\n     -\t\t\t\treceive_shallow_info(args, &reader);\n    -+\t\t\t\treceive_shallow_info(args, &reader, si);\n    ++\t\t\t\treceive_shallow_info(args, &reader, shallows, si);\n      \n      \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n      \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n     @@\n    + {\n    + \tstruct ref *ref_cpy;\n    + \tstruct shallow_info si;\n    ++\tstruct oid_array shallows_scratch = OID_ARRAY_INIT;\n    + \n    + \tfetch_pack_setup();\n    + \tif (nr_sought)\n    +@@\n    + \t\tdie(_(\"no matching remote head\"));\n    + \t}\n    + \tif (version == protocol_v2) {\n    +-\t\tif (shallow && shallow->nr)\n    ++\t\tif (shallow->nr)\n      \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n      \t\tmemset(&si, 0, sizeof(si));\n      \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n    --\t\t\t\t\t   pack_lockfile);\n    -+\t\t\t\t\t   &si, pack_lockfile);\n    ++\t\t\t\t\t   &shallows_scratch, &si,\n    + \t\t\t\t\t   pack_lockfile);\n      \t} else {\n      \t\tprepare_shallow_info(&si, shallow);\n    - \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n    +@@\n    + \tupdate_shallow(args, sought, nr_sought, &si);\n    + cleanup:\n    + \tclear_shallow_info(&si);\n    ++\toid_array_clear(&shallows_scratch);\n    + \treturn ref_cpy;\n    + }\n    + \n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372535","messageId":"d2eb101709cc9b300e38151b14c97f96a2b14188.1553622679.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553622678.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] fetch-pack: call prepare_shallow_info only if v0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T17:53:25Z","receivedAt":"2019-03-26T17:53:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In fetch_pack(), be clearer that there is no shallow information before\nthe fetch when v2 is used - memset the struct shallow_info to 0 instead\nof calling prepare_shallow_info().\n\nThis patch is in preparation for a future patch in which a v2 fetch\nmight call prepare_shallow_info() after shallow info has been retrieved\nduring the fetch, so I needed to ensure that prepare_shallow_info() is\nnot called before the fetch.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n commit.h     |  4 ++++\n fetch-pack.c | 10 +++++++---\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 42728c2906..a3f2b2eddb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -257,6 +257,10 @@ extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n extern const char *setup_temporary_shallow(const struct oid_array *extra);\n extern void advertise_shallow_grafts(int);\n \n+/*\n+ * Initialize with prepare_shallow_info() or zero-initialize (equivalent to\n+ * prepare_shallow_info with a NULL oid_array).\n+ */\n struct shallow_info {\n \tstruct oid_array *shallow;\n \tint *ours, nr_ours;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex e69993b2eb..a0eb268dfc 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1648,13 +1648,17 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\n \t}\n-\tprepare_shallow_info(&si, shallow);\n-\tif (version == protocol_v2)\n+\tif (version == protocol_v2) {\n+\t\tif (shallow && shallow->nr)\n+\t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n+\t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t   pack_lockfile);\n-\telse\n+\t} else {\n+\t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t&si, pack_lockfile);\n+\t}\n \treprepare_packed_git(the_repository);\n \n \tif (!args->cloning && args->deepen) {\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372536","messageId":"943b1cbc61dbcc498a9e34d7d39dc8617cb99195.1553622679.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553622678.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T17:53:26Z","receivedAt":"2019-03-26T17:53:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In protocol v0, when sending \"shallow\" lines, the server distinguishes\nbetween lines caused by the remote repo being shallow and lines caused\nby client-specified depth settings. Unless \"--update-shallow\" is\nspecified, there is a difference in behavior: refs that reach the former\n\"shallow\" lines, but not the latter, are rejected. But in v2, the server\ndoes not, and the client treats all \"shallow\" lines like lines caused by\nclient-specified depth settings.\n\nFull restoration of v0 functionality is not possible without protocol\nchange, but we can implement a heuristic: if we specify any depth\nsetting, treat all \"shallow\" lines like lines caused by client-specified\ndepth settings (that is, unaffected by \"--no-update-shallow\"), but\notherwise, treat them like lines caused by the remote repo being shallow\n(that is, affected by \"--no-update-shallow\"). This restores most of v0\nbehavior, except in the case where a client fetches from a shallow\nrepository with depth settings.\n\nThis patch causes a test that previously failed with\nGIT_TEST_PROTOCOL_VERSION=2 to pass.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c | 43 +++++++++++++++++++++++++++++++++++--------\n 1 file changed, 35 insertions(+), 8 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a0eb268dfc..4831eb6a25 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1253,9 +1253,11 @@ static int process_acks(struct fetch_negotiator *negotiator,\n }\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n-\t\t\t\t struct packet_reader *reader)\n+\t\t\t\t struct packet_reader *reader,\n+\t\t\t\t struct oid_array *shallows,\n+\t\t\t\t struct shallow_info *si)\n {\n-\tint line_received = 0;\n+\tint unshallow_received = 0;\n \n \tprocess_section_header(reader, \"shallow-info\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n@@ -1265,8 +1267,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\tif (skip_prefix(reader->line, \"shallow \", &arg)) {\n \t\t\tif (get_oid_hex(arg, &oid))\n \t\t\t\tdie(_(\"invalid shallow line: %s\"), reader->line);\n-\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\tline_received = 1;\n+\t\t\toid_array_append(shallows, &oid);\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(reader->line, \"unshallow \", &arg)) {\n@@ -1279,7 +1280,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\t\t\tdie(_(\"error in object: %s\"), reader->line);\n \t\t\tif (unregister_shallow(&oid))\n \t\t\t\tdie(_(\"no shallow found: %s\"), reader->line);\n-\t\t\tline_received = 1;\n+\t\t\tunshallow_received = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tdie(_(\"expected shallow/unshallow, got %s\"), reader->line);\n@@ -1289,10 +1290,31 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t    reader->status != PACKET_READ_DELIM)\n \t\tdie(_(\"error processing shallow info: %d\"), reader->status);\n \n-\tif (line_received) {\n+\tif (args->deepen || unshallow_received) {\n+\t\t/*\n+\t\t * Treat these as shallow lines caused by our depth settings.\n+\t\t * In v0, these lines cannot cause refs to be rejected; do the\n+\t\t * same.\n+\t\t */\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < shallows->nr; i++)\n+\t\t\tregister_shallow(the_repository, &shallows->oid[i]);\n \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n \t\t\t\t\tNULL);\n \t\targs->deepen = 1;\n+\t} else if (shallows->nr) {\n+\t\t/*\n+\t\t * Treat these as shallow lines caused by the remote being\n+\t\t * shallow. In v0, remote refs that reach these objects are\n+\t\t * rejected (unless --update-shallow is set); do the same.\n+\t\t */\n+\t\tprepare_shallow_info(si, shallows);\n+\t\tif (si->nr_ours || si->nr_theirs)\n+\t\t\talternate_shallow_file =\n+\t\t\t\tsetup_temporary_shallow(si->shallow);\n+\t\telse\n+\t\t\talternate_shallow_file = NULL;\n \t} else {\n \t\talternate_shallow_file = NULL;\n \t}\n@@ -1337,6 +1359,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t    int fd[2],\n \t\t\t\t    const struct ref *orig_ref,\n \t\t\t\t    struct ref **sought, int nr_sought,\n+\t\t\t\t    struct oid_array *shallows,\n+\t\t\t\t    struct shallow_info *si,\n \t\t\t\t    char **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1411,7 +1435,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_GET_PACK:\n \t\t\t/* Check for shallow-info section */\n \t\t\tif (process_section_header(&reader, \"shallow-info\", 1))\n-\t\t\t\treceive_shallow_info(args, &reader);\n+\t\t\t\treceive_shallow_info(args, &reader, shallows, si);\n \n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n@@ -1625,6 +1649,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n {\n \tstruct ref *ref_cpy;\n \tstruct shallow_info si;\n+\tstruct oid_array shallows_scratch = OID_ARRAY_INIT;\n \n \tfetch_pack_setup();\n \tif (nr_sought)\n@@ -1649,10 +1674,11 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tdie(_(\"no matching remote head\"));\n \t}\n \tif (version == protocol_v2) {\n-\t\tif (shallow && shallow->nr)\n+\t\tif (shallow->nr)\n \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n \t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n+\t\t\t\t\t   &shallows_scratch, &si,\n \t\t\t\t\t   pack_lockfile);\n \t} else {\n \t\tprepare_shallow_info(&si, shallow);\n@@ -1680,6 +1706,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tupdate_shallow(args, sought, nr_sought, &si);\n cleanup:\n \tclear_shallow_info(&si);\n+\toid_array_clear(&shallows_scratch);\n \treturn ref_cpy;\n }\n \n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372537","messageId":"20190326181806.GA24105@sigill.intra.peff.net","threadId":"50820","inReplyTo":"20190326173706.175638-1-jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T18:18:06Z","receivedAt":"2019-03-26T18:18:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2019 at 10:37:06AM -0700, Jonathan Tan wrote:\n\n> > Perhaps it's worth passing down the shallows array we get from the\n> > caller of fetch_pack(). Something like the patch below (I think it is\n> > never NULL, which means in your patch 1 you can simplify the conditional\n> > for the BUG).\n> \n> [snip patch]\n> \n> You're right that it is never NULL - I have removed that check. As for\n> passing down the shallows array that we get from the caller of\n> fetch_pack(), that would get confusing because we end up modifying the\n> shallows array in some code paths, and the transport is sometimes reused\n> (for example, when backfilling tags). I have instead made a\n> shallows_scratch variable in fetch_pack(), and made it pass it down\n> (like in the diff you provided).\n\nYeah, I confess to having spent quite a few minutes trying to figure out\nthe difference between \"shallows\" and \"shallow_info\", whether one\nwrote into the other, and who was responsible for filling each in. So I\nwill not complain if you have a way of writing it that is less\nconfusing. :)\n\n-Peff\n"},{"id":"372538","messageId":"20190326182047.GB24105@sigill.intra.peff.net","threadId":"50820","inReplyTo":"cover.1553622678.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T18:20:48Z","receivedAt":"2019-03-26T18:20:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2019 at 10:53:24AM -0700, Jonathan Tan wrote:\n\n> Updated to remove the unnecessary NULL check and memory leak that Peff\n> noticed [1].\n> \n> (Only commit 2/2 is changed, so the range-diff only shows 1 commit.)\n\nBut isn't this line:\n\n>     + \tif (version == protocol_v2) {\n>     +-\t\tif (shallow && shallow->nr)\n>     ++\t\tif (shallow->nr)\n>       \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n\nadded by patch 1? It's added with \"shallow &&\" in patch 1, and then\nmodified in patch 2.\n\nI think the \"it's never NULL\" property is true even before this series,\nright?\n\n-Peff\n"},{"id":"372539","messageId":"20190326182300.GC24105@sigill.intra.peff.net","threadId":"50820","inReplyTo":"943b1cbc61dbcc498a9e34d7d39dc8617cb99195.1553622679.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-26T18:23:00Z","receivedAt":"2019-03-26T18:23:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2019 at 10:53:26AM -0700, Jonathan Tan wrote:\n\n> @@ -1625,6 +1649,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n>  {\n>  \tstruct ref *ref_cpy;\n>  \tstruct shallow_info si;\n> +\tstruct oid_array shallows_scratch = OID_ARRAY_INIT;\n>  \n>  \tfetch_pack_setup();\n>  \tif (nr_sought)\n> @@ -1649,10 +1674,11 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n>  \t\tdie(_(\"no matching remote head\"));\n>  \t}\n>  \tif (version == protocol_v2) {\n> -\t\tif (shallow && shallow->nr)\n> +\t\tif (shallow->nr)\n>  \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n>  \t\tmemset(&si, 0, sizeof(si));\n>  \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n> +\t\t\t\t\t   &shallows_scratch, &si,\n>  \t\t\t\t\t   pack_lockfile);\n>  \t} else {\n>  \t\tprepare_shallow_info(&si, shallow);\n> @@ -1680,6 +1706,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n>  \tupdate_shallow(args, sought, nr_sought, &si);\n>  cleanup:\n>  \tclear_shallow_info(&si);\n> +\toid_array_clear(&shallows_scratch);\n>  \treturn ref_cpy;\n>  }\n\nYeah, this is much clearer than what I posted earlier. Thanks for\nthinking it through.\n\nThe patches look good to me, modulo the timing of the NULL thing I\nmentioned elsewhere.\n\n-Peff\n"},{"id":"372542","messageId":"cover.1553628494.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553546216.git.jonathantanmy@google.com","subject":"[PATCH v3 0/2] Last big GIT_TEST_PROTOCOL_VERSION=2 fix, hopefully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T19:31:19Z","receivedAt":"2019-03-26T19:31:30Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Peff says in [1]:\n\n> But isn't this line:\n> \n> >     + \tif (version == protocol_v2) {\n> >     +-\t\tif (shallow && shallow->nr)\n> >     ++\t\tif (shallow->nr)\n> >       \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n> \n> added by patch 1? It's added with \"shallow &&\" in patch 1, and then\n> modified in patch 2.\n> \n> I think the \"it's never NULL\" property is true even before this series,\n> right?\n\nAh...yes you're right. I've updated it here.\n\nThanks for your review.\n\n[1] https://public-inbox.org/git/20190326182047.GB24105@sigill.intra.peff.net/\n\nJonathan Tan (2):\n  fetch-pack: call prepare_shallow_info only if v0\n  fetch-pack: respect --no-update-shallow in v2\n\n commit.h     |  4 ++++\n fetch-pack.c | 51 +++++++++++++++++++++++++++++++++++++++++----------\n 2 files changed, 45 insertions(+), 10 deletions(-)\n\nRange-diff against v2:\n1:  d2eb101709 ! 1:  64f44a18ad fetch-pack: call prepare_shallow_info only if v0\n    @@ -38,7 +38,7 @@\n     -\tprepare_shallow_info(&si, shallow);\n     -\tif (version == protocol_v2)\n     +\tif (version == protocol_v2) {\n    -+\t\tif (shallow && shallow->nr)\n    ++\t\tif (shallow->nr)\n     +\t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n     +\t\tmemset(&si, 0, sizeof(si));\n      \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n2:  943b1cbc61 ! 2:  3f65698610 fetch-pack: respect --no-update-shallow in v2\n    @@ -120,11 +120,6 @@\n      \tfetch_pack_setup();\n      \tif (nr_sought)\n     @@\n    - \t\tdie(_(\"no matching remote head\"));\n    - \t}\n    - \tif (version == protocol_v2) {\n    --\t\tif (shallow && shallow->nr)\n    -+\t\tif (shallow->nr)\n      \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n      \t\tmemset(&si, 0, sizeof(si));\n      \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372543","messageId":"64f44a18ad02f68410b9a6637c543fc911de5f56.1553628494.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553628494.git.jonathantanmy@google.com","subject":"[PATCH v3 1/2] fetch-pack: call prepare_shallow_info only if v0","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T19:31:20Z","receivedAt":"2019-03-26T19:31:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In fetch_pack(), be clearer that there is no shallow information before\nthe fetch when v2 is used - memset the struct shallow_info to 0 instead\nof calling prepare_shallow_info().\n\nThis patch is in preparation for a future patch in which a v2 fetch\nmight call prepare_shallow_info() after shallow info has been retrieved\nduring the fetch, so I needed to ensure that prepare_shallow_info() is\nnot called before the fetch.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n commit.h     |  4 ++++\n fetch-pack.c | 10 +++++++---\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 42728c2906..a3f2b2eddb 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -257,6 +257,10 @@ extern void setup_alternate_shallow(struct lock_file *shallow_lock,\n extern const char *setup_temporary_shallow(const struct oid_array *extra);\n extern void advertise_shallow_grafts(int);\n \n+/*\n+ * Initialize with prepare_shallow_info() or zero-initialize (equivalent to\n+ * prepare_shallow_info with a NULL oid_array).\n+ */\n struct shallow_info {\n \tstruct oid_array *shallow;\n \tint *ours, nr_ours;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex e69993b2eb..ebb24ada24 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1648,13 +1648,17 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\n \t}\n-\tprepare_shallow_info(&si, shallow);\n-\tif (version == protocol_v2)\n+\tif (version == protocol_v2) {\n+\t\tif (shallow->nr)\n+\t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n+\t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t   pack_lockfile);\n-\telse\n+\t} else {\n+\t\tprepare_shallow_info(&si, shallow);\n \t\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n \t\t\t\t\t&si, pack_lockfile);\n+\t}\n \treprepare_packed_git(the_repository);\n \n \tif (!args->cloning && args->deepen) {\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372544","messageId":"3f65698610c1e3d1bc8acf76a24a6cabc554ff9b.1553628494.git.jonathantanmy@google.com","threadId":"50820","inReplyTo":"cover.1553628494.git.jonathantanmy@google.com","subject":"[PATCH v3 2/2] fetch-pack: respect --no-update-shallow in v2","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T19:31:21Z","receivedAt":"2019-03-26T19:31:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In protocol v0, when sending \"shallow\" lines, the server distinguishes\nbetween lines caused by the remote repo being shallow and lines caused\nby client-specified depth settings. Unless \"--update-shallow\" is\nspecified, there is a difference in behavior: refs that reach the former\n\"shallow\" lines, but not the latter, are rejected. But in v2, the server\ndoes not, and the client treats all \"shallow\" lines like lines caused by\nclient-specified depth settings.\n\nFull restoration of v0 functionality is not possible without protocol\nchange, but we can implement a heuristic: if we specify any depth\nsetting, treat all \"shallow\" lines like lines caused by client-specified\ndepth settings (that is, unaffected by \"--no-update-shallow\"), but\notherwise, treat them like lines caused by the remote repo being shallow\n(that is, affected by \"--no-update-shallow\"). This restores most of v0\nbehavior, except in the case where a client fetches from a shallow\nrepository with depth settings.\n\nThis patch causes a test that previously failed with\nGIT_TEST_PROTOCOL_VERSION=2 to pass.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c | 41 ++++++++++++++++++++++++++++++++++-------\n 1 file changed, 34 insertions(+), 7 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex ebb24ada24..4831eb6a25 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1253,9 +1253,11 @@ static int process_acks(struct fetch_negotiator *negotiator,\n }\n \n static void receive_shallow_info(struct fetch_pack_args *args,\n-\t\t\t\t struct packet_reader *reader)\n+\t\t\t\t struct packet_reader *reader,\n+\t\t\t\t struct oid_array *shallows,\n+\t\t\t\t struct shallow_info *si)\n {\n-\tint line_received = 0;\n+\tint unshallow_received = 0;\n \n \tprocess_section_header(reader, \"shallow-info\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n@@ -1265,8 +1267,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\tif (skip_prefix(reader->line, \"shallow \", &arg)) {\n \t\t\tif (get_oid_hex(arg, &oid))\n \t\t\t\tdie(_(\"invalid shallow line: %s\"), reader->line);\n-\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\tline_received = 1;\n+\t\t\toid_array_append(shallows, &oid);\n \t\t\tcontinue;\n \t\t}\n \t\tif (skip_prefix(reader->line, \"unshallow \", &arg)) {\n@@ -1279,7 +1280,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t\t\t\tdie(_(\"error in object: %s\"), reader->line);\n \t\t\tif (unregister_shallow(&oid))\n \t\t\t\tdie(_(\"no shallow found: %s\"), reader->line);\n-\t\t\tline_received = 1;\n+\t\t\tunshallow_received = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tdie(_(\"expected shallow/unshallow, got %s\"), reader->line);\n@@ -1289,10 +1290,31 @@ static void receive_shallow_info(struct fetch_pack_args *args,\n \t    reader->status != PACKET_READ_DELIM)\n \t\tdie(_(\"error processing shallow info: %d\"), reader->status);\n \n-\tif (line_received) {\n+\tif (args->deepen || unshallow_received) {\n+\t\t/*\n+\t\t * Treat these as shallow lines caused by our depth settings.\n+\t\t * In v0, these lines cannot cause refs to be rejected; do the\n+\t\t * same.\n+\t\t */\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < shallows->nr; i++)\n+\t\t\tregister_shallow(the_repository, &shallows->oid[i]);\n \t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n \t\t\t\t\tNULL);\n \t\targs->deepen = 1;\n+\t} else if (shallows->nr) {\n+\t\t/*\n+\t\t * Treat these as shallow lines caused by the remote being\n+\t\t * shallow. In v0, remote refs that reach these objects are\n+\t\t * rejected (unless --update-shallow is set); do the same.\n+\t\t */\n+\t\tprepare_shallow_info(si, shallows);\n+\t\tif (si->nr_ours || si->nr_theirs)\n+\t\t\talternate_shallow_file =\n+\t\t\t\tsetup_temporary_shallow(si->shallow);\n+\t\telse\n+\t\t\talternate_shallow_file = NULL;\n \t} else {\n \t\talternate_shallow_file = NULL;\n \t}\n@@ -1337,6 +1359,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\t    int fd[2],\n \t\t\t\t    const struct ref *orig_ref,\n \t\t\t\t    struct ref **sought, int nr_sought,\n+\t\t\t\t    struct oid_array *shallows,\n+\t\t\t\t    struct shallow_info *si,\n \t\t\t\t    char **pack_lockfile)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n@@ -1411,7 +1435,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\tcase FETCH_GET_PACK:\n \t\t\t/* Check for shallow-info section */\n \t\t\tif (process_section_header(&reader, \"shallow-info\", 1))\n-\t\t\t\treceive_shallow_info(args, &reader);\n+\t\t\t\treceive_shallow_info(args, &reader, shallows, si);\n \n \t\t\tif (process_section_header(&reader, \"wanted-refs\", 1))\n \t\t\t\treceive_wanted_refs(&reader, sought, nr_sought);\n@@ -1625,6 +1649,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n {\n \tstruct ref *ref_cpy;\n \tstruct shallow_info si;\n+\tstruct oid_array shallows_scratch = OID_ARRAY_INIT;\n \n \tfetch_pack_setup();\n \tif (nr_sought)\n@@ -1653,6 +1678,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t\tBUG(\"Protocol V2 does not provide shallows at this point in the fetch\");\n \t\tmemset(&si, 0, sizeof(si));\n \t\tref_cpy = do_fetch_pack_v2(args, fd, ref, sought, nr_sought,\n+\t\t\t\t\t   &shallows_scratch, &si,\n \t\t\t\t\t   pack_lockfile);\n \t} else {\n \t\tprepare_shallow_info(&si, shallow);\n@@ -1680,6 +1706,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tupdate_shallow(args, sought, nr_sought, &si);\n cleanup:\n \tclear_shallow_info(&si);\n+\toid_array_clear(&shallows_scratch);\n \treturn ref_cpy;\n }\n \n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"}]}