| Message ID | 20200508080115.15616-1-chriscool@tuxfamily.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] upload-pack: fix filter options scope | expand |
On Fri, May 08, 2020 at 10:01:15AM +0200, Christian Couder wrote: > The changes since the previous RFC version are the following: > > - now filter_options is part of struct upload_pack_data as > suggested by Peff and Taylor > - improved commit message > - updated comment before the test that used to fail Thanks, this version looks good to me. > static void create_pack_file(const struct object_array *have_obj, > - const struct object_array *want_obj) > + const struct object_array *want_obj, > + struct list_objects_filter_options *filter_options) I had hoped that stuffing it into upload_pack_data would require fewer changes passing it around, but I guess many of these functions don't know about upload_pack_data in the first place. Oh well. I think this is the best we can do for now. In the long run we'd perhaps want to take upload_pack_data there, but it wouldn't help until the v0 path also uses that struct. -Peff
Jeff King <peff@peff.net> writes: > On Fri, May 08, 2020 at 10:01:15AM +0200, Christian Couder wrote: > >> The changes since the previous RFC version are the following: >> >> - now filter_options is part of struct upload_pack_data as >> suggested by Peff and Taylor >> - improved commit message >> - updated comment before the test that used to fail > > Thanks, this version looks good to me. > >> static void create_pack_file(const struct object_array *have_obj, >> - const struct object_array *want_obj) >> + const struct object_array *want_obj, >> + struct list_objects_filter_options *filter_options) > > I had hoped that stuffing it into upload_pack_data would require fewer > changes passing it around, but I guess many of these functions don't > know about upload_pack_data in the first place. Oh well. I think this is > the best we can do for now. In the long run we'd perhaps want to take > upload_pack_data there, but it wouldn't help until the v0 path also uses > that struct. Yup, I agree that this v2-only fix is a good first step. Even though the log message itself got a lot better explaining the nature of the issue, I do not think the title of the patch does a good job explaining what it is about to the readers of shortlog. "fix" is a meaningless word in a bugfix patch, and it does not make it clear what bad effect of the original code had by not giving a clean-slate "options" variable to the second invocation of the callchain. Is it that the server side was incapable of serving a follow-up fetch request in the same process when protocol v2 was in use? Perhaps upload-pack: allow follow-up fetch in protocol v2 or something? I care about singling out protocol v2 because then we would immediately know that backporting this to older maintenance track is of lower priority as the plan is to flip the default back to v0. Thanks.
On Fri, May 08, 2020 at 08:46:58AM -0700, Junio C Hamano wrote: > Even though the log message itself got a lot better explaining the > nature of the issue, I do not think the title of the patch does a > good job explaining what it is about to the readers of shortlog. > > "fix" is a meaningless word in a bugfix patch, and it does not make > it clear what bad effect of the original code had by not giving a > clean-slate "options" variable to the second invocation of the > callchain. > > Is it that the server side was incapable of serving a follow-up > fetch request in the same process when protocol v2 was in use? > Perhaps > > upload-pack: allow follow-up fetch in protocol v2 > > or something? Yeah, I agree that's better. Maybe even: upload-pack: clear filter_options for each v2 fetch command which is more direct (we already allow follow-up fetches; they just don't work sometimes) and makes it clear that this is about fixing the filter feature with v2. -Peff
On Fri, May 8, 2020 at 7:16 PM Jeff King <peff@peff.net> wrote: > > On Fri, May 08, 2020 at 08:46:58AM -0700, Junio C Hamano wrote: > > Is it that the server side was incapable of serving a follow-up > > fetch request in the same process when protocol v2 was in use? > > Perhaps > > > > upload-pack: allow follow-up fetch in protocol v2 > > > > or something? > > Yeah, I agree that's better. Maybe even: > > upload-pack: clear filter_options for each v2 fetch command Ok, I will resend soon with the above title. > which is more direct (we already allow follow-up fetches; they just > don't work sometimes) and makes it clear that this is about fixing the > filter feature with v2. Thanks, Christian.
Jeff King <peff@peff.net> writes: > Maybe even: > > upload-pack: clear filter_options for each v2 fetch command > > which is more direct (we already allow follow-up fetches; they just > don't work sometimes) and makes it clear that this is about fixing the > filter feature with v2. Christian may want to send in an updated one, but will queue with that title in the meantime. Thanks.
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh index 88002b24af..8a27452a51 100755 --- a/t/t5616-partial-clone.sh +++ b/t/t5616-partial-clone.sh @@ -384,12 +384,11 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' ' grep "want $(cat hash)" trace ' -# The following two tests must be in this order, or else -# the first will not fail. It is important that the srv.bare -# repository did not have tags during clone, but has tags +# The following two tests must be in this order. It is important that +# the srv.bare repository did not have tags during clone, but has tags # in the fetch. -test_expect_failure 'verify fetch succeeds when asking for new tags' ' +test_expect_success 'verify fetch succeeds when asking for new tags' ' git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test && for i in I J K do diff --git a/upload-pack.c b/upload-pack.c index 902d0ad5e1..4d4dd7cd41 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -68,7 +68,6 @@ static const char *pack_objects_hook; static int filter_capability_requested; static int allow_filter; static int allow_ref_in_want; -static struct list_objects_filter_options filter_options; static int allow_sideband_all; @@ -103,7 +102,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data) } static void create_pack_file(const struct object_array *have_obj, - const struct object_array *want_obj) + const struct object_array *want_obj, + struct list_objects_filter_options *filter_options) { struct child_process pack_objects = CHILD_PROCESS_INIT; char data[8193], progress[128]; @@ -140,9 +140,9 @@ static void create_pack_file(const struct object_array *have_obj, argv_array_push(&pack_objects.args, "--delta-base-offset"); if (use_include_tag) argv_array_push(&pack_objects.args, "--include-tag"); - if (filter_options.choice) { + if (filter_options->choice) { const char *spec = - expand_list_objects_filter_spec(&filter_options); + expand_list_objects_filter_spec(filter_options); if (pack_objects.use_shell) { struct strbuf buf = STRBUF_INIT; sq_quote_buf(&buf, spec); @@ -848,7 +848,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not, return 0; } -static void receive_needs(struct packet_reader *reader, struct object_array *want_obj) +static void receive_needs(struct packet_reader *reader, + struct object_array *want_obj, + struct list_objects_filter_options *filter_options) { struct object_array shallows = OBJECT_ARRAY_INIT; struct string_list deepen_not = STRING_LIST_INIT_DUP; @@ -883,8 +885,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan if (skip_prefix(reader->line, "filter ", &arg)) { if (!filter_capability_requested) die("git upload-pack: filtering capability not negotiated"); - list_objects_filter_die_if_populated(&filter_options); - parse_list_objects_filter(&filter_options, arg); + list_objects_filter_die_if_populated(filter_options); + parse_list_objects_filter(filter_options, arg); continue; } @@ -1087,11 +1089,14 @@ void upload_pack(struct upload_pack_options *options) struct string_list symref = STRING_LIST_INIT_DUP; struct object_array want_obj = OBJECT_ARRAY_INIT; struct packet_reader reader; + struct list_objects_filter_options filter_options; stateless_rpc = options->stateless_rpc; timeout = options->timeout; daemon_mode = options->daemon_mode; + memset(&filter_options, 0, sizeof(filter_options)); + git_config(upload_pack_config, NULL); head_ref_namespaced(find_symref, &symref); @@ -1114,12 +1119,14 @@ void upload_pack(struct upload_pack_options *options) PACKET_READ_CHOMP_NEWLINE | PACKET_READ_DIE_ON_ERR_PACKET); - receive_needs(&reader, &want_obj); + receive_needs(&reader, &want_obj, &filter_options); if (want_obj.nr) { struct object_array have_obj = OBJECT_ARRAY_INIT; get_common_commits(&reader, &have_obj, &want_obj); - create_pack_file(&have_obj, &want_obj); + create_pack_file(&have_obj, &want_obj, &filter_options); } + + list_objects_filter_release(&filter_options); } struct upload_pack_data { @@ -1134,6 +1141,8 @@ struct upload_pack_data { int deepen_rev_list; int deepen_relative; + struct list_objects_filter_options filter_options; + struct packet_writer writer; unsigned stateless_rpc : 1; @@ -1169,6 +1178,7 @@ static void upload_pack_data_clear(struct upload_pack_data *data) oid_array_clear(&data->haves); object_array_clear(&data->shallows); string_list_clear(&data->deepen_not, 0); + list_objects_filter_release(&data->filter_options); } static int parse_want(struct packet_writer *writer, const char *line, @@ -1306,8 +1316,8 @@ static void process_args(struct packet_reader *request, } if (allow_filter && skip_prefix(arg, "filter ", &p)) { - list_objects_filter_die_if_populated(&filter_options); - parse_list_objects_filter(&filter_options, p); + list_objects_filter_die_if_populated(&data->filter_options); + parse_list_objects_filter(&data->filter_options, p); continue; } @@ -1514,7 +1524,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys, send_shallow_info(&data, &want_obj); packet_writer_write(&data.writer, "packfile\n"); - create_pack_file(&have_obj, &want_obj); + create_pack_file(&have_obj, &want_obj, &data.filter_options); state = FETCH_DONE; break; case FETCH_DONE:
Because of the request/response model of protocol v2, the upload_pack_v2() function is sometimes called twice in the same process, while 'struct list_objects_filter_options filter_options' was declared as static at the beginning of 'upload-pack.c'. This made the check in list_objects_filter_die_if_populated(), which is called by process_args(), fail the second time upload_pack_v2() is called, as filter_options had already been populated the first time. To fix that, filter_options is not static any more. It's now owned directly by upload_pack(). It's now also part of 'struct upload_pack_data', so that it's owned indirectly by upload_pack_v2(). In the long term, the goal is to also have upload_pack() use 'struct upload_pack_data', so adding filter_options to this struct makes more sense than to have it owned directly by upload_pack_v2(). This fixes the first of the 2 bugs documented by d0badf8797 (partial-clone: demonstrate bugs in partial fetch, 2020-02-21). Helped-by: Derrick Stolee <dstolee@microsoft.com> Helped-by: Jeff King <peff@peff.net> Helped-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- The changes since the previous RFC version are the following: - now filter_options is part of struct upload_pack_data as suggested by Peff and Taylor - improved commit message - updated comment before the test that used to fail t/t5616-partial-clone.sh | 7 +++---- upload-pack.c | 34 ++++++++++++++++++++++------------ 2 files changed, 25 insertions(+), 16 deletions(-)