{"thread":{"id":"53422","subject":"[RFC PATCH] upload-pack: fix filter options scope","startedAt":"2020-05-07T09:58:49Z","lastAt":"2020-05-08T18:11:06Z","messageCount":12,"participants":["Christian Couder","Jeff King","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397264","messageId":"20200507095829.16894-1-chriscool@tuxfamily.org","threadId":"53422","inReplyTo":null,"subject":"[RFC PATCH] upload-pack: fix filter options scope","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-07T09:58:29Z","receivedAt":"2020-05-07T09:58:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"The upload_pack_v2() function is sometimes called twice in the same\nprocess while 'struct list_objects_filter_options filter_options' was\ndeclared as static at the beginning of 'upload-pack.c'.\n\nThis made the check in list_objects_filter_die_if_populated(), which\nis called by process_args(), fail the second time upload_pack_v2() is\ncalled, as filter_options had already been populated the first time.\n\nTo fix that, filter_options is not static any more, but instead it's\nnow owned by upload_pack_v2() and upload_pack().\n\nThis fixes the first of the 2 bugs documented by d0badf8797\n(partial-clone: demonstrate bugs in partial fetch, 2020-02-21).\n\nHelped-by: Derrick Stolee \nSigned-off-by: Christian Couder \n---\nThe issue was originally discussed in:\n\nhttps://lore.kernel.org/git/dbc1bdcae16f8b9941add514264b0fe04cda48c0.1582129312.git.gitgitgadget@gmail.com/\n\nI am not sure why upload_pack_v2() is called twice. Also it seems\nthat another similar issue might not be fixed by this. So this patch\nis RFC for now. \n\n t/t5616-partial-clone.sh | 2 +-\n upload-pack.c | 40 ++++++++++++++++++++++++++--------------\n 2 files changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 88002b24af..b73c38c29a 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -389,7 +389,7 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n # repository did not have tags during clone, but has tags\n # in the fetch.\n \n-test_expect_failure 'verify fetch succeeds when asking for new tags' '\n+test_expect_success 'verify fetch succeeds when asking for new tags' '\n \tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n \tfor i in I J K\n \tdo\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 902d0ad5e1..4e22e46294 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -68,7 +68,6 @@ static const char *pack_objects_hook;\n static int filter_capability_requested;\n static int allow_filter;\n static int allow_ref_in_want;\n-static struct list_objects_filter_options filter_options;\n \n static int allow_sideband_all;\n \n@@ -103,7 +102,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n }\n \n static void create_pack_file(const struct object_array *have_obj,\n-\t\t\t const struct object_array *want_obj)\n+\t\t\t const struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tchar data[8193], progress[128];\n@@ -140,9 +140,9 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\targv_array_push(&pack_objects.args, \"--delta-base-offset\");\n \tif (use_include_tag)\n \t\targv_array_push(&pack_objects.args, \"--include-tag\");\n-\tif (filter_options.choice) {\n+\tif (filter_options->choice) {\n \t\tconst char *spec =\n-\t\t\texpand_list_objects_filter_spec(&filter_options);\n+\t\t\texpand_list_objects_filter_spec(filter_options);\n \t\tif (pack_objects.use_shell) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tsq_quote_buf(&buf, spec);\n@@ -848,7 +848,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n-static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)\n+static void receive_needs(struct packet_reader *reader,\n+\t\t\t struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n@@ -883,8 +885,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \t\tif (skip_prefix(reader->line, \"filter \", &arg)) {\n \t\t\tif (!filter_capability_requested)\n \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n-\t\t\tlist_objects_filter_die_if_populated(&filter_options);\n-\t\t\tparse_list_objects_filter(&filter_options, arg);\n+\t\t\tlist_objects_filter_die_if_populated(filter_options);\n+\t\t\tparse_list_objects_filter(filter_options, arg);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1087,11 +1089,14 @@ void upload_pack(struct upload_pack_options *options)\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct packet_reader reader;\n+\tstruct list_objects_filter_options filter_options;\n \n \tstateless_rpc = options->stateless_rpc;\n \ttimeout = options->timeout;\n \tdaemon_mode = options->daemon_mode;\n \n+\tmemset(&filter_options, 0, sizeof(filter_options));\n+\n \tgit_config(upload_pack_config, NULL);\n \n \thead_ref_namespaced(find_symref, &symref);\n@@ -1114,12 +1119,14 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n-\treceive_needs(&reader, &want_obj);\n+\treceive_needs(&reader, &want_obj, &filter_options);\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \t\tget_common_commits(&reader, &have_obj, &want_obj);\n-\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\tcreate_pack_file(&have_obj, &want_obj, &filter_options);\n \t}\n+\n+\tlist_objects_filter_release(&filter_options);\n }\n \n struct upload_pack_data {\n@@ -1250,7 +1257,8 @@ static int parse_have(const char *line, struct oid_array *haves)\n \n static void process_args(struct packet_reader *request,\n \t\t\t struct upload_pack_data *data,\n-\t\t\t struct object_array *want_obj)\n+\t\t\t struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \twhile (packet_reader_read(request) == PACKET_READ_NORMAL) {\n \t\tconst char *arg = request->line;\n@@ -1306,8 +1314,8 @@ static void process_args(struct packet_reader *request,\n \t\t}\n \n \t\tif (allow_filter && skip_prefix(arg, \"filter \", &p)) {\n-\t\t\tlist_objects_filter_die_if_populated(&filter_options);\n-\t\t\tparse_list_objects_filter(&filter_options, p);\n+\t\t\tlist_objects_filter_die_if_populated(filter_options);\n+\t\t\tparse_list_objects_filter(filter_options, p);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1470,6 +1478,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \tstruct upload_pack_data data;\n \tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n+\tstruct list_objects_filter_options filter_options;\n \n \tclear_object_flags(ALL_FLAGS);\n \n@@ -1478,10 +1487,12 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \tupload_pack_data_init(&data);\n \tuse_sideband = LARGE_PACKET_MAX;\n \n+\tmemset(&filter_options, 0, sizeof(filter_options));\n+\n \twhile (state != FETCH_DONE) {\n \t\tswitch (state) {\n \t\tcase FETCH_PROCESS_ARGS:\n-\t\t\tprocess_args(request, &data, &want_obj);\n+\t\t\tprocess_args(request, &data, &want_obj, &filter_options);\n \n \t\t\tif (!want_obj.nr) {\n \t\t\t\t/*\n@@ -1514,7 +1525,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n \t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\t\tcreate_pack_file(&have_obj, &want_obj, &filter_options);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n@@ -1525,6 +1536,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \tupload_pack_data_clear(&data);\n \tobject_array_clear(&have_obj);\n \tobject_array_clear(&want_obj);\n+\tlist_objects_filter_release(&filter_options);\n \treturn 0;\n }\n \n-- \n2.26.2.562.ga2ab1ba363\n\n"},{"id":"397282","messageId":"20200507113624.GA3027470@coredump.intra.peff.net","threadId":"53422","inReplyTo":"20200507095829.16894-1-chriscool@tuxfamily.org","subject":"Re: [RFC PATCH] upload-pack: fix filter options scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T11:36:24Z","receivedAt":"2020-05-07T11:36:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 07, 2020 at 11:58:29AM +0200, Christian Couder wrote:\n\n> I am not sure why upload_pack_v2() is called twice. Also it seems\n> that another similar issue might not be fixed by this. So this patch\n> is RFC for now.\n\nIt's because of the request/response model of v2. The client side of the\nconversation looks something like:\n\n - client connects transport to server\n\n - client issues command=ls-refs to get the ref advertisement\n\n - client issues command=fetch and sends wants/have; the server may\n respond with a pack when the negotiation is finished, but may also\n respond with acks, etc, asking for more rounds of negotiation\n\n - if there's no pack, the client then issues another command=fetch,\n repeating until we get a pack\n\nIn stateless git-over-http, each of those request/response pairs happens in\nits own server-side process, because the webserver kicks off a separate\nCGI for each request. But v2 adapts git://, ssh://, and direct-pipe\nconnections to use the same request/response model, but all connected to\na single Git process on the other side.\n\nSo each upload_pack_v2() call needs to start with a clean slate; it\ncan't assume anything about previous rounds, and it needs to clean up\nany allocated data from those rounds.\n\nSo your patch is going in the right direction, but we already have other\nsimilar data handled by upload_pack_data, which has its own init/clear\nfunctions. Probably the filter data should go in there, too.\n\nThis is an easy bug to introduce, and I suspect we'll see equivalent\nones in the future (if there aren't already more lurking; there's a lot\nof global data in Git, and I really have no idea what subtle\ninteractions one could see). One thing we could do is to truly run each\nv2 command request in its own process, just like git-over-http does. But\nthere are a lot of complications there around holding the pipe/socket\nopen, how the parent v2 serving process interacts with the child\nrequests (e.g., noticing errors), etc. So I'd worry that going that\ndirection would be just as likely to introduce bugs as fix them.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 902d0ad5e1..4e22e46294 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -68,7 +68,6 @@ static const char *pack_objects_hook;\n> static int filter_capability_requested;\n> static int allow_filter;\n> static int allow_ref_in_want;\n> -static struct list_objects_filter_options filter_options;\n\nJust looking at nearby bits of global data that might want similar\ntreatment:\n\n - allow_filter and allow_ref_in_want are set by the server-side config.\n So they're persisted between upload_pack_v2() calls, but we'd\n overwrite them by calling git_config() in each incarnation\n\n - filter_capability_requested is set based on client input, but it's\n only used in v1\n\nBut boy there are a lot of global variables there to trace through.\nProbably a more fruitful way to find similar problems is to look at the\nv2 process_args() to see what it touches. It looks like options like\nofs_delta probably ought to be part of upload_pack_data, too. We didn't\nnotice because it would require a client doing something like:\n\n command=fetch\n ofs-delta [claims to support ofs-delta]\n 0001\n [wants and haves that don't result in a pack yet...]\n 0000\n command=fetch\n [do not claim to support ofs-delta!]\n 0001\n [wants and haves that result in a pack]\n 0000\n\nThe server _shouldn't_ use ofs-delta there (and wouldn't over http,\nwhere the stateless server side that generates the pack knows only about\nthat second request). But in git-over-ssh, it would.\n\nA client who does this is stupid and wrong (and I didn't check the\nprotocol spec, but it could very well be violating the spec). So I don't\nthink it's _that_ big a deal. But it would be nice if all of those\nglobals got moved into upload_pack_data, and both v1 and v2 just used it\nconsistently.\n\n-Peff\n"},{"id":"397292","messageId":"CAP8UFD3AqUGrE2W3Qg_+z6HnobmRpOEyNEdspmDPGggKUqyK+w@mail.gmail.com","threadId":"53422","inReplyTo":"20200507113624.GA3027470@coredump.intra.peff.net","subject":"Re: [RFC PATCH] upload-pack: fix filter options scope","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-07T12:32:39Z","receivedAt":"2020-05-07T12:32:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, May 7, 2020 at 1:36 PM Jeff King wrote:\n>\n> On Thu, May 07, 2020 at 11:58:29AM +0200, Christian Couder wrote:\n>\n> > I am not sure why upload_pack_v2() is called twice. Also it seems\n> > that another similar issue might not be fixed by this. So this patch\n> > is RFC for now.\n>\n> It's because of the request/response model of v2. The client side of the\n> conversation looks something like:\n>\n> - client connects transport to server\n>\n> - client issues command=ls-refs to get the ref advertisement\n>\n> - client issues command=fetch and sends wants/have; the server may\n> respond with a pack when the negotiation is finished, but may also\n> respond with acks, etc, asking for more rounds of negotiation\n>\n> - if there's no pack, the client then issues another command=fetch,\n> repeating until we get a pack\n>\n> In stateless git-over-http, each of those request/response pairs happens in\n> its own server-side process, because the webserver kicks off a separate\n> CGI for each request. But v2 adapts git://, ssh://, and direct-pipe\n> connections to use the same request/response model, but all connected to\n> a single Git process on the other side.\n>\n> So each upload_pack_v2() call needs to start with a clean slate; it\n> can't assume anything about previous rounds, and it needs to clean up\n> any allocated data from those rounds.\n>\n> So your patch is going in the right direction, but we already have other\n> similar data handled by upload_pack_data, which has its own init/clear\n> functions. Probably the filter data should go in there, too.\n\nThank you for the detailed explanations!\n\nIt looks like 'struct upload_pack_data' is indeed used by\nupload_pack_v2(). It's not used by upload_pack() though. So if I move\n'struct list_objects_filter_options filter_options' there, I would\nneed to also make upload_pack() either have its own filter_options or\nuse 'struct upload_pack_data' too.\n\n> This is an easy bug to introduce, and I suspect we'll see equivalent\n> ones in the future (if there aren't already more lurking; there's a lot\n> of global data in Git, and I really have no idea what subtle\n> interactions one could see). One thing we could do is to truly run each\n> v2 command request in its own process, just like git-over-http does. But\n> there are a lot of complications there around holding the pipe/socket\n> open, how the parent v2 serving process interacts with the child\n> requests (e.g., noticing errors), etc. So I'd worry that going that\n> direction would be just as likely to introduce bugs as fix them.\n\nI agree that it seems better to just get rid of the global data used\nby the upload pack functions.\n\n> > diff --git a/upload-pack.c b/upload-pack.c\n> > index 902d0ad5e1..4e22e46294 100644\n> > --- a/upload-pack.c\n> > +++ b/upload-pack.c\n> > @@ -68,7 +68,6 @@ static const char *pack_objects_hook;\n> > static int filter_capability_requested;\n> > static int allow_filter;\n> > static int allow_ref_in_want;\n> > -static struct list_objects_filter_options filter_options;\n>\n> Just looking at nearby bits of global data that might want similar\n> treatment:\n>\n> - allow_filter and allow_ref_in_want are set by the server-side config.\n> So they're persisted between upload_pack_v2() calls, but we'd\n> overwrite them by calling git_config() in each incarnation\n>\n> - filter_capability_requested is set based on client input, but it's\n> only used in v1\n>\n> But boy there are a lot of global variables there to trace through.\n\nYeah, unfortunately.\n\n> Probably a more fruitful way to find similar problems is to look at the\n> v2 process_args() to see what it touches. It looks like options like\n> ofs_delta probably ought to be part of upload_pack_data, too. We didn't\n> notice because it would require a client doing something like:\n>\n> command=fetch\n> ofs-delta [claims to support ofs-delta]\n> 0001\n> [wants and haves that don't result in a pack yet...]\n> 0000\n> command=fetch\n> [do not claim to support ofs-delta!]\n> 0001\n> [wants and haves that result in a pack]\n> 0000\n>\n> The server _shouldn't_ use ofs-delta there (and wouldn't over http,\n> where the stateless server side that generates the pack knows only about\n> that second request). But in git-over-ssh, it would.\n>\n> A client who does this is stupid and wrong (and I didn't check the\n> protocol spec, but it could very well be violating the spec). So I don't\n> think it's _that_ big a deal. But it would be nice if all of those\n> globals got moved into upload_pack_data, and both v1 and v2 just used it\n> consistently.\n\nUnfortunately as I discuss a bit above 'struct upload_pack_data' is\nnot used by v1, only by v2. I think making v1 use upload_pack_data\nmight be nice, but it's a separate issue. So I am tempted to just\nimprove the commit message, adding some information from you and from\nthis discussion, and then re-sending.\n\nAnother intermediate solution would be to add filter_options to\n'struct upload_pack_data' for v2 and to use filter_options directly\nfor v1.\n"},{"id":"397311","messageId":"20200507144710.GA3063766@coredump.intra.peff.net","threadId":"53422","inReplyTo":"CAP8UFD3AqUGrE2W3Qg_+z6HnobmRpOEyNEdspmDPGggKUqyK+w@mail.gmail.com","subject":"Re: [RFC PATCH] upload-pack: fix filter options scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T14:47:10Z","receivedAt":"2020-05-07T14:47:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 07, 2020 at 02:32:39PM +0200, Christian Couder wrote:\n\n> > A client who does this is stupid and wrong (and I didn't check the\n> > protocol spec, but it could very well be violating the spec). So I don't\n> > think it's _that_ big a deal. But it would be nice if all of those\n> > globals got moved into upload_pack_data, and both v1 and v2 just used it\n> > consistently.\n> \n> Unfortunately as I discuss a bit above 'struct upload_pack_data' is\n> not used by v1, only by v2. I think making v1 use upload_pack_data\n> might be nice, but it's a separate issue. So I am tempted to just\n> improve the commit message, adding some information from you and from\n> this discussion, and then re-sending.\n> \n> Another intermediate solution would be to add filter_options to\n> 'struct upload_pack_data' for v2 and to use filter_options directly\n> for v1.\n\nI think we do want the v1 path to use upload_pack_data in the long run,\njust to keep things less confusing.\n\nBut if you don't want to go that far now, I'd strongly prefer the second\noption, pushing v2 towards how we ultimately want it to look, and\nplumbing a local variable through v1 paths as necessary. Or perhaps just\nrenaming the global to filter_options_v1 or something, so that v2 code\npaths aren't tempted to use it.\n\n-Peff\n"},{"id":"397323","messageId":"20200507164206.GB26677@syl.local","threadId":"53422","inReplyTo":"20200507144710.GA3063766@coredump.intra.peff.net","subject":"Re: [RFC PATCH] upload-pack: fix filter options scope","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-07T16:42:06Z","receivedAt":"2020-05-07T16:42:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, May 07, 2020 at 10:47:10AM -0400, Jeff King wrote:\n> On Thu, May 07, 2020 at 02:32:39PM +0200, Christian Couder wrote:\n>\n> > > A client who does this is stupid and wrong (and I didn't check the\n> > > protocol spec, but it could very well be violating the spec). So I don't\n> > > think it's _that_ big a deal. But it would be nice if all of those\n> > > globals got moved into upload_pack_data, and both v1 and v2 just used it\n> > > consistently.\n> >\n> > Unfortunately as I discuss a bit above 'struct upload_pack_data' is\n> > not used by v1, only by v2. I think making v1 use upload_pack_data\n> > might be nice, but it's a separate issue. So I am tempted to just\n> > improve the commit message, adding some information from you and from\n> > this discussion, and then re-sending.\n> >\n> > Another intermediate solution would be to add filter_options to\n> > 'struct upload_pack_data' for v2 and to use filter_options directly\n> > for v1.\n>\n> I think we do want the v1 path to use upload_pack_data in the long run,\n> just to keep things less confusing.\n>\n> But if you don't want to go that far now, I'd strongly prefer the second\n> option, pushing v2 towards how we ultimately want it to look, and\n> plumbing a local variable through v1 paths as necessary. Or perhaps just\n> renaming the global to filter_options_v1 or something, so that v2 code\n> paths aren't tempted to use it.\n\nI would be very much in favor of that direction. Thanks for CC-ing me on\nthis thread, since any changes here will obviously create merge\nconflicts on my end when I send the \"configure allowed object filters\"\npatches.\n\n> -Peff\n\nThanks,\nTaylor\n"},{"id":"397395","messageId":"20200508080115.15616-1-chriscool@tuxfamily.org","threadId":"53422","inReplyTo":"20200507095829.16894-1-chriscool@tuxfamily.org","subject":"[PATCH v2] upload-pack: fix filter options scope","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-08T08:01:15Z","receivedAt":"2020-05-08T08:01:44Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Because of the request/response model of protocol v2, the\nupload_pack_v2() function is sometimes called twice in the same\nprocess, while 'struct list_objects_filter_options filter_options'\nwas declared as static at the beginning of 'upload-pack.c'.\n\nThis made the check in list_objects_filter_die_if_populated(), which\nis called by process_args(), fail the second time upload_pack_v2() is\ncalled, as filter_options had already been populated the first time.\n\nTo fix that, filter_options is not static any more. It's now owned\ndirectly by upload_pack(). It's now also part of 'struct\nupload_pack_data', so that it's owned indirectly by upload_pack_v2().\n\nIn the long term, the goal is to also have upload_pack() use\n'struct upload_pack_data', so adding filter_options to this struct\nmakes more sense than to have it owned directly by upload_pack_v2().\n\nThis fixes the first of the 2 bugs documented by d0badf8797\n(partial-clone: demonstrate bugs in partial fetch, 2020-02-21).\n\nHelped-by: Derrick Stolee \nHelped-by: Jeff King \nHelped-by: Taylor Blau \nSigned-off-by: Christian Couder \n---\nThe changes since the previous RFC version are the following:\n\n - now filter_options is part of struct upload_pack_data as\n suggested by Peff and Taylor\n - improved commit message\n - updated comment before the test that used to fail\n\n t/t5616-partial-clone.sh | 7 +++----\n upload-pack.c | 34 ++++++++++++++++++++++------------\n 2 files changed, 25 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 88002b24af..8a27452a51 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -384,12 +384,11 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n-# The following two tests must be in this order, or else\n-# the first will not fail. It is important that the srv.bare\n-# repository did not have tags during clone, but has tags\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 \n-test_expect_failure 'verify fetch succeeds when asking for new tags' '\n+test_expect_success 'verify fetch succeeds when asking for new tags' '\n \tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n \tfor i in I J K\n \tdo\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 902d0ad5e1..4d4dd7cd41 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -68,7 +68,6 @@ static const char *pack_objects_hook;\n static int filter_capability_requested;\n static int allow_filter;\n static int allow_ref_in_want;\n-static struct list_objects_filter_options filter_options;\n \n static int allow_sideband_all;\n \n@@ -103,7 +102,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n }\n \n static void create_pack_file(const struct object_array *have_obj,\n-\t\t\t const struct object_array *want_obj)\n+\t\t\t const struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tchar data[8193], progress[128];\n@@ -140,9 +140,9 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\targv_array_push(&pack_objects.args, \"--delta-base-offset\");\n \tif (use_include_tag)\n \t\targv_array_push(&pack_objects.args, \"--include-tag\");\n-\tif (filter_options.choice) {\n+\tif (filter_options->choice) {\n \t\tconst char *spec =\n-\t\t\texpand_list_objects_filter_spec(&filter_options);\n+\t\t\texpand_list_objects_filter_spec(filter_options);\n \t\tif (pack_objects.use_shell) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tsq_quote_buf(&buf, spec);\n@@ -848,7 +848,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n-static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)\n+static void receive_needs(struct packet_reader *reader,\n+\t\t\t struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n@@ -883,8 +885,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \t\tif (skip_prefix(reader->line, \"filter \", &arg)) {\n \t\t\tif (!filter_capability_requested)\n \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n-\t\t\tlist_objects_filter_die_if_populated(&filter_options);\n-\t\t\tparse_list_objects_filter(&filter_options, arg);\n+\t\t\tlist_objects_filter_die_if_populated(filter_options);\n+\t\t\tparse_list_objects_filter(filter_options, arg);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1087,11 +1089,14 @@ void upload_pack(struct upload_pack_options *options)\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct packet_reader reader;\n+\tstruct list_objects_filter_options filter_options;\n \n \tstateless_rpc = options->stateless_rpc;\n \ttimeout = options->timeout;\n \tdaemon_mode = options->daemon_mode;\n \n+\tmemset(&filter_options, 0, sizeof(filter_options));\n+\n \tgit_config(upload_pack_config, NULL);\n \n \thead_ref_namespaced(find_symref, &symref);\n@@ -1114,12 +1119,14 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n-\treceive_needs(&reader, &want_obj);\n+\treceive_needs(&reader, &want_obj, &filter_options);\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \t\tget_common_commits(&reader, &have_obj, &want_obj);\n-\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\tcreate_pack_file(&have_obj, &want_obj, &filter_options);\n \t}\n+\n+\tlist_objects_filter_release(&filter_options);\n }\n \n struct upload_pack_data {\n@@ -1134,6 +1141,8 @@ struct upload_pack_data {\n \tint deepen_rev_list;\n \tint deepen_relative;\n \n+\tstruct list_objects_filter_options filter_options;\n+\n \tstruct packet_writer writer;\n \n \tunsigned stateless_rpc : 1;\n@@ -1169,6 +1178,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \toid_array_clear(&data->haves);\n \tobject_array_clear(&data->shallows);\n \tstring_list_clear(&data->deepen_not, 0);\n+\tlist_objects_filter_release(&data->filter_options);\n }\n \n static int parse_want(struct packet_writer *writer, const char *line,\n@@ -1306,8 +1316,8 @@ static void process_args(struct packet_reader *request,\n \t\t}\n \n \t\tif (allow_filter && skip_prefix(arg, \"filter \", &p)) {\n-\t\t\tlist_objects_filter_die_if_populated(&filter_options);\n-\t\t\tparse_list_objects_filter(&filter_options, p);\n+\t\t\tlist_objects_filter_die_if_populated(&data->filter_options);\n+\t\t\tparse_list_objects_filter(&data->filter_options, p);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1514,7 +1524,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n \t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\t\tcreate_pack_file(&have_obj, &want_obj, &data.filter_options);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n-- \n2.26.2.561.g07d8ea56f2.dirty\n\n"},{"id":"397402","messageId":"20200508130616.GA631018@coredump.intra.peff.net","threadId":"53422","inReplyTo":"20200508080115.15616-1-chriscool@tuxfamily.org","subject":"Re: [PATCH v2] upload-pack: fix filter options scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-08T13:06:16Z","receivedAt":"2020-05-08T13:06:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 08, 2020 at 10:01:15AM +0200, Christian Couder wrote:\n\n> The changes since the previous RFC version are the following:\n> \n> - now filter_options is part of struct upload_pack_data as\n> suggested by Peff and Taylor\n> - improved commit message\n> - updated comment before the test that used to fail\n\nThanks, this version looks good to me.\n\n> static void create_pack_file(const struct object_array *have_obj,\n> -\t\t\t const struct object_array *want_obj)\n> +\t\t\t const struct object_array *want_obj,\n> +\t\t\t struct list_objects_filter_options *filter_options)\n\nI had hoped that stuffing it into upload_pack_data would require fewer\nchanges passing it around, but I guess many of these functions don't\nknow about upload_pack_data in the first place. Oh well. I think this is\nthe best we can do for now. In the long run we'd perhaps want to take\nupload_pack_data there, but it wouldn't help until the v0 path also uses\nthat struct.\n\n-Peff\n"},{"id":"397413","messageId":"xmqqtv0qzax9.fsf@gitster.c.googlers.com","threadId":"53422","inReplyTo":"20200508130616.GA631018@coredump.intra.peff.net","subject":"Re: [PATCH v2] upload-pack: fix filter options scope","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T15:46:58Z","receivedAt":"2020-05-08T15:47:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 08, 2020 at 10:01:15AM +0200, Christian Couder wrote:\n>\n>> The changes since the previous RFC version are the following:\n>> \n>> - now filter_options is part of struct upload_pack_data as\n>> suggested by Peff and Taylor\n>> - improved commit message\n>> - updated comment before the test that used to fail\n>\n> Thanks, this version looks good to me.\n>\n>> static void create_pack_file(const struct object_array *have_obj,\n>> -\t\t\t const struct object_array *want_obj)\n>> +\t\t\t const struct object_array *want_obj,\n>> +\t\t\t struct list_objects_filter_options *filter_options)\n>\n> I had hoped that stuffing it into upload_pack_data would require fewer\n> changes passing it around, but I guess many of these functions don't\n> know about upload_pack_data in the first place. Oh well. I think this is\n> the best we can do for now. In the long run we'd perhaps want to take\n> upload_pack_data there, but it wouldn't help until the v0 path also uses\n> that struct.\n\nYup, I agree that this v2-only fix is a good first step.\n\nEven though the log message itself got a lot better explaining the\nnature of the issue, I do not think the title of the patch does a\ngood job explaining what it is about to the readers of shortlog.\n\n\"fix\" is a meaningless word in a bugfix patch, and it does not make\nit clear what bad effect of the original code had by not giving a\nclean-slate \"options\" variable to the second invocation of the\ncallchain.\n\nIs it that the server side was incapable of serving a follow-up\nfetch request in the same process when protocol v2 was in use?\nPerhaps\n\n upload-pack: allow follow-up fetch in protocol v2\n\nor something?\n\nI care about singling out protocol v2 because then we would\nimmediately know that backporting this to older maintenance track is\nof lower priority as the plan is to flip the default back to v0.\n\nThanks.\n"},{"id":"397424","messageId":"20200508171635.GB637136@coredump.intra.peff.net","threadId":"53422","inReplyTo":"xmqqtv0qzax9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] upload-pack: fix filter options scope","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-08T17:16:35Z","receivedAt":"2020-05-08T17:16:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 08, 2020 at 08:46:58AM -0700, Junio C Hamano wrote:\n\n> Even though the log message itself got a lot better explaining the\n> nature of the issue, I do not think the title of the patch does a\n> good job explaining what it is about to the readers of shortlog.\n> \n> \"fix\" is a meaningless word in a bugfix patch, and it does not make\n> it clear what bad effect of the original code had by not giving a\n> clean-slate \"options\" variable to the second invocation of the\n> callchain.\n> \n> Is it that the server side was incapable of serving a follow-up\n> fetch request in the same process when protocol v2 was in use?\n> Perhaps\n> \n> upload-pack: allow follow-up fetch in protocol v2\n> \n> or something?\n\nYeah, I agree that's better. Maybe even:\n\n upload-pack: clear filter_options for each v2 fetch command\n\nwhich is more direct (we already allow follow-up fetches; they just\ndon't work sometimes) and makes it clear that this is about fixing the\nfilter feature with v2.\n\n-Peff\n"},{"id":"397431","messageId":"CAP8UFD2-8FwEjyHromp-=MSSeFo=mn9GhkPvtFnF9MRx-4cjPQ@mail.gmail.com","threadId":"53422","inReplyTo":"20200508171635.GB637136@coredump.intra.peff.net","subject":"Re: [PATCH v2] upload-pack: fix filter options scope","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-08T18:00:27Z","receivedAt":"2020-05-08T18:00:42Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 8, 2020 at 7:16 PM Jeff King wrote:\n>\n> On Fri, May 08, 2020 at 08:46:58AM -0700, Junio C Hamano wrote:\n\n> > Is it that the server side was incapable of serving a follow-up\n> > fetch request in the same process when protocol v2 was in use?\n> > Perhaps\n> >\n> > upload-pack: allow follow-up fetch in protocol v2\n> >\n> > or something?\n>\n> Yeah, I agree that's better. Maybe even:\n>\n> upload-pack: clear filter_options for each v2 fetch command\n\nOk, I will resend soon with the above title.\n\n> which is more direct (we already allow follow-up fetches; they just\n> don't work sometimes) and makes it clear that this is about fixing the\n> filter feature with v2.\n\nThanks,\nChristian.\n"},{"id":"397433","messageId":"20200508180935.23142-1-chriscool@tuxfamily.org","threadId":"53422","inReplyTo":"20200508080115.15616-1-chriscool@tuxfamily.org","subject":"[PATCH v3] upload-pack: clear filter_options for each v2 fetch command","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2020-05-08T18:09:35Z","receivedAt":"2020-05-08T18:09:53Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Because of the request/response model of protocol v2, the\nupload_pack_v2() function is sometimes called twice in the same\nprocess, while 'struct list_objects_filter_options filter_options'\nwas declared as static at the beginning of 'upload-pack.c'.\n\nThis made the check in list_objects_filter_die_if_populated(), which\nis called by process_args(), fail the second time upload_pack_v2() is\ncalled, as filter_options had already been populated the first time.\n\nTo fix that, filter_options is not static any more. It's now owned\ndirectly by upload_pack(). It's now also part of 'struct\nupload_pack_data', so that it's owned indirectly by upload_pack_v2().\n\nIn the long term, the goal is to also have upload_pack() use\n'struct upload_pack_data', so adding filter_options to this struct\nmakes more sense than to have it owned directly by upload_pack_v2().\n\nThis fixes the first of the 2 bugs documented by d0badf8797\n(partial-clone: demonstrate bugs in partial fetch, 2020-02-21).\n\nHelped-by: Derrick Stolee \nHelped-by: Jeff King \nHelped-by: Taylor Blau \nSigned-off-by: Christian Couder \n---\nThe only change compared to V2 is an improved commit title, thanks\nto Peff and Junio.\n\n t/t5616-partial-clone.sh | 7 +++----\n upload-pack.c | 34 ++++++++++++++++++++++------------\n 2 files changed, 25 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 88002b24af..8a27452a51 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -384,12 +384,11 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '\n \tgrep \"want $(cat hash)\" trace\n '\n \n-# The following two tests must be in this order, or else\n-# the first will not fail. It is important that the srv.bare\n-# repository did not have tags during clone, but has tags\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 \n-test_expect_failure 'verify fetch succeeds when asking for new tags' '\n+test_expect_success 'verify fetch succeeds when asking for new tags' '\n \tgit clone --filter=blob:none \"file://$(pwd)/srv.bare\" tag-test &&\n \tfor i in I J K\n \tdo\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 902d0ad5e1..4d4dd7cd41 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -68,7 +68,6 @@ static const char *pack_objects_hook;\n static int filter_capability_requested;\n static int allow_filter;\n static int allow_ref_in_want;\n-static struct list_objects_filter_options filter_options;\n \n static int allow_sideband_all;\n \n@@ -103,7 +102,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)\n }\n \n static void create_pack_file(const struct object_array *have_obj,\n-\t\t\t const struct object_array *want_obj)\n+\t\t\t const struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tchar data[8193], progress[128];\n@@ -140,9 +140,9 @@ static void create_pack_file(const struct object_array *have_obj,\n \t\targv_array_push(&pack_objects.args, \"--delta-base-offset\");\n \tif (use_include_tag)\n \t\targv_array_push(&pack_objects.args, \"--include-tag\");\n-\tif (filter_options.choice) {\n+\tif (filter_options->choice) {\n \t\tconst char *spec =\n-\t\t\texpand_list_objects_filter_spec(&filter_options);\n+\t\t\texpand_list_objects_filter_spec(filter_options);\n \t\tif (pack_objects.use_shell) {\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tsq_quote_buf(&buf, spec);\n@@ -848,7 +848,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,\n \treturn 0;\n }\n \n-static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)\n+static void receive_needs(struct packet_reader *reader,\n+\t\t\t struct object_array *want_obj,\n+\t\t\t struct list_objects_filter_options *filter_options)\n {\n \tstruct object_array shallows = OBJECT_ARRAY_INIT;\n \tstruct string_list deepen_not = STRING_LIST_INIT_DUP;\n@@ -883,8 +885,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan\n \t\tif (skip_prefix(reader->line, \"filter \", &arg)) {\n \t\t\tif (!filter_capability_requested)\n \t\t\t\tdie(\"git upload-pack: filtering capability not negotiated\");\n-\t\t\tlist_objects_filter_die_if_populated(&filter_options);\n-\t\t\tparse_list_objects_filter(&filter_options, arg);\n+\t\t\tlist_objects_filter_die_if_populated(filter_options);\n+\t\t\tparse_list_objects_filter(filter_options, arg);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1087,11 +1089,14 @@ void upload_pack(struct upload_pack_options *options)\n \tstruct string_list symref = STRING_LIST_INIT_DUP;\n \tstruct object_array want_obj = OBJECT_ARRAY_INIT;\n \tstruct packet_reader reader;\n+\tstruct list_objects_filter_options filter_options;\n \n \tstateless_rpc = options->stateless_rpc;\n \ttimeout = options->timeout;\n \tdaemon_mode = options->daemon_mode;\n \n+\tmemset(&filter_options, 0, sizeof(filter_options));\n+\n \tgit_config(upload_pack_config, NULL);\n \n \thead_ref_namespaced(find_symref, &symref);\n@@ -1114,12 +1119,14 @@ void upload_pack(struct upload_pack_options *options)\n \t\t\t PACKET_READ_CHOMP_NEWLINE |\n \t\t\t PACKET_READ_DIE_ON_ERR_PACKET);\n \n-\treceive_needs(&reader, &want_obj);\n+\treceive_needs(&reader, &want_obj, &filter_options);\n \tif (want_obj.nr) {\n \t\tstruct object_array have_obj = OBJECT_ARRAY_INIT;\n \t\tget_common_commits(&reader, &have_obj, &want_obj);\n-\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\tcreate_pack_file(&have_obj, &want_obj, &filter_options);\n \t}\n+\n+\tlist_objects_filter_release(&filter_options);\n }\n \n struct upload_pack_data {\n@@ -1134,6 +1141,8 @@ struct upload_pack_data {\n \tint deepen_rev_list;\n \tint deepen_relative;\n \n+\tstruct list_objects_filter_options filter_options;\n+\n \tstruct packet_writer writer;\n \n \tunsigned stateless_rpc : 1;\n@@ -1169,6 +1178,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data)\n \toid_array_clear(&data->haves);\n \tobject_array_clear(&data->shallows);\n \tstring_list_clear(&data->deepen_not, 0);\n+\tlist_objects_filter_release(&data->filter_options);\n }\n \n static int parse_want(struct packet_writer *writer, const char *line,\n@@ -1306,8 +1316,8 @@ static void process_args(struct packet_reader *request,\n \t\t}\n \n \t\tif (allow_filter && skip_prefix(arg, \"filter \", &p)) {\n-\t\t\tlist_objects_filter_die_if_populated(&filter_options);\n-\t\t\tparse_list_objects_filter(&filter_options, p);\n+\t\t\tlist_objects_filter_die_if_populated(&data->filter_options);\n+\t\t\tparse_list_objects_filter(&data->filter_options, p);\n \t\t\tcontinue;\n \t\t}\n \n@@ -1514,7 +1524,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,\n \t\t\tsend_shallow_info(&data, &want_obj);\n \n \t\t\tpacket_writer_write(&data.writer, \"packfile\\n\");\n-\t\t\tcreate_pack_file(&have_obj, &want_obj);\n+\t\t\tcreate_pack_file(&have_obj, &want_obj, &data.filter_options);\n \t\t\tstate = FETCH_DONE;\n \t\t\tbreak;\n \t\tcase FETCH_DONE:\n-- \n2.26.2.562.g0c5c0b446d.dirty\n\n"},{"id":"397434","messageId":"xmqqr1vuxpor.fsf@gitster.c.googlers.com","threadId":"53422","inReplyTo":"20200508171635.GB637136@coredump.intra.peff.net","subject":"Re: [PATCH v2] upload-pack: fix filter options scope","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-08T18:11:00Z","receivedAt":"2020-05-08T18:11:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Maybe even:\n>\n> upload-pack: clear filter_options for each v2 fetch command\n>\n> which is more direct (we already allow follow-up fetches; they just\n> don't work sometimes) and makes it clear that this is about fixing the\n> filter feature with v2.\n\nChristian may want to send in an updated one, but will queue with\nthat title in the meantime.\n\nThanks.\n"}]}