{"thread":{"id":"50199","subject":"[PATCH] fetch-pack: do not take shallow lock unnecessarily","startedAt":"2019-01-10T19:36:55Z","lastAt":"2019-01-14T19:08:58Z","messageCount":3,"participants":["Jonathan Tan","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"366527","messageId":"20190110193645.34080-1-jonathantanmy@google.com","threadId":"50199","inReplyTo":null,"subject":"[PATCH] fetch-pack: do not take shallow lock unnecessarily","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-10T19:36:45Z","receivedAt":"2019-01-10T19:36:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching using protocol v2, the remote may send a \"shallow-info\"\nsection if the client is shallow. If so, Git as the client currently\ntakes the shallow file lock, even if the \"shallow-info\" section is\nempty.\n\nThis is not a problem except that Git does not support taking the\nshallow file lock after modifying the shallow file, because\nis_repository_shallow() stores information that is never cleared. And\nthis take-after-modify occurs when Git does a tag-following fetch from a\nshallow repository on a transport that does not support tag following\n(since in this case, 2 fetches are performed).\n\nTo solve this issue, take the shallow file lock (and perform all other\nshallow processing) only if the \"shallow-info\" section is non-empty;\notherwise, behave as if it were empty.\n\nA full solution (probably, ensuring that any action of committing\nshallow file locks also includes clearing the information stored by\nis_repository_shallow()) would solve the issue without need for this\npatch, but this patch is independently useful (as an optimization to\nprevent writing a file in an unnecessary case), hence why I wrote it. I\nhave included a NEEDSWORK outlining the full solution.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nSorry for sending out multiple solutions to this issue, but I think I\nhave found the simplest way to do this. Even if we end up needing one of\nthe more complicated ways, I think that this is independently useful (as\nstated above), so I am sending out this patch for your consideration.\n\nFor reference, the other solutions I sent out are:\n\n(1) https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/\nThis is the full solution described in the commit message above. Locking\nand committing the shallow file is now abstracted behind an interface\nthat ensures that anything done by is_repository_shallow() is cleared\nwhen the shallow file is committed.\n\n(2) https://public-inbox.org/git/20181220195349.92214-1-jonathantanmy@google.com/\nA partial solution - if the client did not make any depth requests (as\nis the case above - the client is shallow but made a normal fetch\nrequest), any \"shallow\" lines are first filtered before determining if\nthe lock needs to be taken. This solves the issue in practice because\nthere are no \"shallow\"s, so no lock is taken (and the filter is a\nno-op).\n\nThe two prior versions do not have the annotated tag in the test. I\nnoticed that the second fetch occurs regardless of whether the annotated\ntag is present, but I included it anyway - the second fetch is possibly\na bug if the annotated tag is not present, but that issue is outside the\nscope of this patch.\n---\n fetch-pack.c           | 11 +++++++++--\n shallow.c              |  7 +++++++\n t/t5702-protocol-v2.sh | 18 ++++++++++++++++++\n 3 files changed, 34 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex dd6700bda9..5885623ece 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1232,6 +1232,8 @@ static int process_acks(struct fetch_negotiator *negotiator,\n static void receive_shallow_info(struct fetch_pack_args *args,\n \t\t\t\t struct packet_reader *reader)\n {\n+\tint line_received = 0;\n+\n \tprocess_section_header(reader, \"shallow-info\", 0);\n \twhile (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n \t\tconst char *arg;\n@@ -1241,6 +1243,7 @@ static void receive_shallow_info(struct fetch_pack_args *args,\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\tcontinue;\n \t\t}\n \t\tif (skip_prefix(reader->line, \"unshallow \", &arg)) {\n@@ -1253,6 +1256,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\tcontinue;\n \t\t}\n \t\tdie(_(\"expected shallow/unshallow, got %s\"), reader->line);\n@@ -1262,8 +1266,11 @@ 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-\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file, NULL);\n-\targs->deepen = 1;\n+\tif (line_received) {\n+\t\tsetup_alternate_shallow(&shallow_lock, &alternate_shallow_file,\n+\t\t\t\t\tNULL);\n+\t\targs->deepen = 1;\n+\t}\n }\n \n static void receive_wanted_refs(struct packet_reader *reader,\ndiff --git a/shallow.c b/shallow.c\nindex 02fdbfc554..ce45297940 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -43,6 +43,13 @@ int register_shallow(struct repository *r, const struct object_id *oid)\n \n int is_repository_shallow(struct repository *r)\n {\n+\t/*\n+\t * NEEDSWORK: This function updates\n+\t * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but\n+\t * there is no corresponding function to clear them when the shallow\n+\t * file is updated.\n+\t */\n+\n \tFILE *fp;\n \tchar buf[1024];\n \tconst char *path = r->parsed_objects->alternate_shallow_file;\ndiff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh\nindex 0f2b09ebb8..fd164d414d 100755\n--- a/t/t5702-protocol-v2.sh\n+++ b/t/t5702-protocol-v2.sh\n@@ -471,6 +471,24 @@ test_expect_success 'upload-pack respects client shallows' '\n \tgrep \"fetch< version 2\" trace\n '\n \n+test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' '\n+\trm -rf server client trace &&\n+\n+\ttest_create_repo server &&\n+\ttest_commit -C server one &&\n+\ttest_commit -C server two &&\n+\ttest_commit -C server three &&\n+\tgit clone --shallow-exclude two \"file://$(pwd)/server\" client &&\n+\n+\tgit -C server tag -a -m \"an annotated tag\" twotag two &&\n+\n+\t# Triggers tag following (thus, 2 fetches in one process)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n+\t\tfetch --shallow-exclude one origin &&\n+\t# Ensure that protocol v2 is used\n+\tgrep \"fetch< version 2\" trace\n+'\n+\n # Test protocol v2 with 'http://' transport\n #\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.19.0.271.gfe8321ec05.dirty\n\n"},{"id":"366545","messageId":"CAGZ79kZ8U6xWKQrmBW-G5HrZC0DN3AroxLCnkN2FPC70rQGYyg@mail.gmail.com","threadId":"50199","inReplyTo":"20190110193645.34080-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: do not take shallow lock unnecessarily","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-10T23:17:31Z","receivedAt":"2019-01-10T23:17:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jan 10, 2019 at 11:36 AM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> When fetching using protocol v2, the remote may send a \"shallow-info\"\n> section if the client is shallow. If so, Git as the client currently\n> takes the shallow file lock, even if the \"shallow-info\" section is\n> empty.\n>\n> This is not a problem except that Git does not support taking the\n> shallow file lock after modifying the shallow file, because\n> is_repository_shallow() stores information that is never cleared. And\n> this take-after-modify occurs when Git does a tag-following fetch from a\n> shallow repository on a transport that does not support tag following\n> (since in this case, 2 fetches are performed).\n>\n> To solve this issue, take the shallow file lock (and perform all other\n> shallow processing) only if the \"shallow-info\" section is non-empty;\n> otherwise, behave as if it were empty.\n\nIn other parts of the code we often have an early exit instead of\nsetting a variable and reacting to that in the end, i.e. what\ndo you think about:\n\nstatic void receive_shallow_info(struct fetch_pack_args *args,\n    struct packet_reader *reader)\n{\n     process_section_header(reader, \"shallow-info\", 0);\n+    if (reader->status == PACKET_READ_FLUSH ||\n+        reader->status == PACKET_READ_DELIM)\n+            /* useful comment why empty sections appear */\n+            return;\n    while (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n    ...\n\ninstead? This would allow us to keep the rest of the function\nrelatively simple as well as we'd have a dedicated space where\nwe can explain why empty sections need to be treated specially.\n\n> A full solution (probably, ensuring that any action of committing\n> shallow file locks also includes clearing the information stored by\n> is_repository_shallow()) would solve the issue without need for this\n> patch, but this patch is independently useful (as an optimization to\n> prevent writing a file in an unnecessary case), hence why I wrote it. I\n> have included a NEEDSWORK outlining the full solution.\n\nI like this patch..\n\n> +test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' '\n> +       rm -rf server client trace &&\n> +\n> +       test_create_repo server &&\n> +       test_commit -C server one &&\n> +       test_commit -C server two &&\n> +       test_commit -C server three &&\n> +       git clone --shallow-exclude two \"file://$(pwd)/server\" client &&\n> +\n> +       git -C server tag -a -m \"an annotated tag\" twotag two &&\n> +\n> +       # Triggers tag following (thus, 2 fetches in one process)\n> +       GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> +               fetch --shallow-exclude one origin &&\n> +       # Ensure that protocol v2 is used\n> +       grep \"fetch< version 2\" trace\n> +'\n\nWould we also need to ensure tags 'one' and 'three',\nbut not 'two' are present?\n(What error condition do we see without this patch?)\n"},{"id":"366674","messageId":"20190114190851.63976-1-jonathantanmy@google.com","threadId":"50199","inReplyTo":"CAGZ79kZ8U6xWKQrmBW-G5HrZC0DN3AroxLCnkN2FPC70rQGYyg@mail.gmail.com","subject":"Re: [PATCH] fetch-pack: do not take shallow lock unnecessarily","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-01-14T19:08:51Z","receivedAt":"2019-01-14T19:08:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> In other parts of the code we often have an early exit instead of\n> setting a variable and reacting to that in the end, i.e. what\n> do you think about:\n> \n> static void receive_shallow_info(struct fetch_pack_args *args,\n>     struct packet_reader *reader)\n> {\n>      process_section_header(reader, \"shallow-info\", 0);\n> +    if (reader->status == PACKET_READ_FLUSH ||\n> +        reader->status == PACKET_READ_DELIM)\n> +            /* useful comment why empty sections appear */\n> +            return;\n>     while (packet_reader_read(reader) == PACKET_READ_NORMAL) {\n>     ...\n> \n> instead? This would allow us to keep the rest of the function\n> relatively simple as well as we'd have a dedicated space where\n> we can explain why empty sections need to be treated specially.\n\nGood idea. I'll do something like this in the next version, which will\nbe combined with another patch of mine into a series [1].\n\n[1] https://public-inbox.org/git/xmqqwoncyvh5.fsf@gitster-ct.c.googlers.com/\n\n> I like this patch..\n\nThanks.\n\n> > +test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' '\n> > +       rm -rf server client trace &&\n> > +\n> > +       test_create_repo server &&\n> > +       test_commit -C server one &&\n> > +       test_commit -C server two &&\n> > +       test_commit -C server three &&\n> > +       git clone --shallow-exclude two \"file://$(pwd)/server\" client &&\n> > +\n> > +       git -C server tag -a -m \"an annotated tag\" twotag two &&\n> > +\n> > +       # Triggers tag following (thus, 2 fetches in one process)\n> > +       GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client -c protocol.version=2 \\\n> > +               fetch --shallow-exclude one origin &&\n> > +       # Ensure that protocol v2 is used\n> > +       grep \"fetch< version 2\" trace\n> > +'\n> \n> Would we also need to ensure tags 'one' and 'three',\n> but not 'two' are present?\n> (What error condition do we see without this patch?)\n\nWell, both \"two\" and \"three\" should be present, but not \"one\". The error\ncondition is that, previously, this would fail with \"fatal: shallow file\nhas changed since we read it\". I'll add a note about this in the commit\nmessage.\n"}]}