| Message ID | 20200515100454.14486-5-chriscool@tuxfamily.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | upload-pack: use 'struct upload_pack_data' thoroughly, part 1 | expand |
On 5/15/2020 6:04 AM, Christian Couder wrote: > As we cleanup 'upload-pack.c' by using 'struct upload_pack_data' > more thoroughly, let's use 'struct upload_pack_data' in > upload_pack(). > > This will make it possible in followup commits to remove a lot > of static variables and local variables that have the same name > and purpose as fields in 'struct upload_pack_data'. This will > also make upload_pack() work in a more similar way as > upload_pack_v2(). > > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > --- > upload-pack.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/upload-pack.c b/upload-pack.c > index 9aeb3477c9..cb336c5713 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -1144,18 +1144,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused) > 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; > + struct upload_pack_data data; > > stateless_rpc = options->stateless_rpc; > timeout = options->timeout; > daemon_mode = options->daemon_mode; > > - memset(&filter_options, 0, sizeof(filter_options)); > - I checked that upload_pack_data_init() runs memset(), which initializes the memory for data.filter_options. Thanks. > git_config(upload_pack_config, NULL); > > + upload_pack_data_init(&data); > + > head_ref_namespaced(find_symref, &symref); > > if (options->advertise_refs || !stateless_rpc) { > @@ -1169,21 +1168,24 @@ void upload_pack(struct upload_pack_options *options) The control flow below was hard to parse from the diff because a whitespace line split up the "-" lines and the "+" lines. I reorder them here: Old code: > - if (options->advertise_refs) > - return; > > - packet_reader_init(&reader, 0, NULL, 0, > - PACKET_READ_CHOMP_NEWLINE | > - PACKET_READ_DIE_ON_ERR_PACKET); > > - 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, &filter_options); > } > - list_objects_filter_release(&filter_options); > } New code: > + if (!options->advertise_refs) { > + packet_reader_init(&reader, 0, NULL, 0, > + PACKET_READ_CHOMP_NEWLINE | > + PACKET_READ_DIE_ON_ERR_PACKET); > > + receive_needs(&reader, &data.want_obj, &data.filter_options); > + if (data.want_obj.nr) { > + get_common_commits(&reader, > + &data.have_obj, > + &data.want_obj); > + create_pack_file(&data.have_obj, > + &data.want_obj, > + &data.filter_options); > + } > } > > + upload_pack_data_clear(&data); > } The major change is that the "options->advertise_refs" case now clears the data when before it did not. This seems like a good change to make. Also, the code that has now been surrounded by an if block isn't so large as to justify a "goto cleanup" case. Thanks, -Stolee
On Fri, May 15, 2020 at 06:30:23AM -0400, Derrick Stolee wrote: > The control flow below was hard to parse from the diff because > a whitespace line split up the "-" lines and the "+" lines. > I reorder them here: Using "-w" improves it a bit, because the packet_reader_init() becomes an anchor. But sadly the rest is hard to follow because of adding "data." to each variable reference. I hoped --patience or --histogram might do better, but they don't. I think breaking it into a block like you did is the simplest way to see it. > The major change is that the "options->advertise_refs" case > now clears the data when before it did not. This seems like > a good change to make. Yeah, I think so, too. It looks like we were simply leaking the filter options before. I think we still leak the symref list, but it looks like that will get moved into upload_pack_data in a later patch, too. So still looking good so far. -Peff
diff --git a/upload-pack.c b/upload-pack.c index 9aeb3477c9..cb336c5713 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -1144,18 +1144,17 @@ static int upload_pack_config(const char *var, const char *value, void *unused) 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; + struct upload_pack_data data; 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); + upload_pack_data_init(&data); + head_ref_namespaced(find_symref, &symref); if (options->advertise_refs || !stateless_rpc) { @@ -1169,21 +1168,24 @@ void upload_pack(struct upload_pack_options *options) for_each_namespaced_ref(check_ref, NULL); } string_list_clear(&symref, 1); - if (options->advertise_refs) - return; - packet_reader_init(&reader, 0, NULL, 0, - PACKET_READ_CHOMP_NEWLINE | - PACKET_READ_DIE_ON_ERR_PACKET); + if (!options->advertise_refs) { + packet_reader_init(&reader, 0, NULL, 0, + PACKET_READ_CHOMP_NEWLINE | + PACKET_READ_DIE_ON_ERR_PACKET); - 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, &filter_options); + receive_needs(&reader, &data.want_obj, &data.filter_options); + if (data.want_obj.nr) { + get_common_commits(&reader, + &data.have_obj, + &data.want_obj); + create_pack_file(&data.have_obj, + &data.want_obj, + &data.filter_options); + } } - list_objects_filter_release(&filter_options); + upload_pack_data_clear(&data); } static int parse_want(struct packet_writer *writer, const char *line,
As we cleanup 'upload-pack.c' by using 'struct upload_pack_data' more thoroughly, let's use 'struct upload_pack_data' in upload_pack(). This will make it possible in followup commits to remove a lot of static variables and local variables that have the same name and purpose as fields in 'struct upload_pack_data'. This will also make upload_pack() work in a more similar way as upload_pack_v2(). Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- upload-pack.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)