[08/13] upload-pack: move symref to upload_pack_data

Commit Message

Christian Couder May 15, 2020, 10:04 a.m. UTC

As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
more thoroughly, we are passing around that struct to many
functions, so let's also pass 'struct string_list symref' around
at the same time by moving it from a local variable in
upload_pack() into a field of 'struct upload_pack_data'.

Signed-off-by: Christian Couder <[e-mail]>
---
 upload-pack.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Jeff King May 15, 2020, 6:28 p.m. UTC | #1

On Fri, May 15, 2020 at 12:04:49PM +0200, Christian Couder wrote:

> As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
> more thoroughly, we are passing around that struct to many
> functions, so let's also pass 'struct string_list symref' around
> at the same time by moving it from a local variable in
> upload_pack() into a field of 'struct upload_pack_data'.

Hmm, do we really benefit here at all? The symref list is a local
implementation detail of upload_pack(), and v2 would not use it at all.
Nor does it ever survive past the function in which it's declared.

Perhaps it would help if we were able to send upload_pack_data along to
for_each_ref(), as you do in the next patch, and that let us make use of
upload_pack_data to access other variables. But it doesn't look like we
ever do.

In light of that, is it worth it?

I think we _could_ switch send_ref() (the callback for for_each_ref())
to use the packet-writer struct. Perhaps that would make it worth doing.

-Peff

Patch

diff --git a/upload-pack.c b/upload-pack.c
index 399ec60ade..c7e35a7fc9 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -73,6 +73,7 @@  static int allow_ref_in_want;
 static int allow_sideband_all;
 
 struct upload_pack_data {
+	struct string_list symref;
 	struct string_list wanted_refs;
 	struct object_array want_obj;
 	struct object_array have_obj;
@@ -100,6 +101,7 @@  struct upload_pack_data {
 
 static void upload_pack_data_init(struct upload_pack_data *data)
 {
+	struct string_list symref = STRING_LIST_INIT_DUP;
 	struct string_list wanted_refs = STRING_LIST_INIT_DUP;
 	struct object_array want_obj = OBJECT_ARRAY_INIT;
 	struct object_array have_obj = OBJECT_ARRAY_INIT;
@@ -108,6 +110,7 @@  static void upload_pack_data_init(struct upload_pack_data *data)
 	struct string_list deepen_not = STRING_LIST_INIT_DUP;
 
 	memset(data, 0, sizeof(*data));
+	data->symref = symref;
 	data->wanted_refs = wanted_refs;
 	data->want_obj = want_obj;
 	data->have_obj = have_obj;
@@ -119,6 +122,7 @@  static void upload_pack_data_init(struct upload_pack_data *data)
 
 static void upload_pack_data_clear(struct upload_pack_data *data)
 {
+	string_list_clear(&data->symref, 1);
 	string_list_clear(&data->wanted_refs, 1);
 	object_array_clear(&data->want_obj);
 	object_array_clear(&data->have_obj);
@@ -1142,7 +1146,6 @@  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 packet_reader reader;
 	struct upload_pack_data data;
 
@@ -1154,19 +1157,18 @@  void upload_pack(struct upload_pack_options *options)
 
 	upload_pack_data_init(&data);
 
-	head_ref_namespaced(find_symref, &symref);
+	head_ref_namespaced(find_symref, &data.symref);
 
 	if (options->advertise_refs || !stateless_rpc) {
 		reset_timeout();
-		head_ref_namespaced(send_ref, &symref);
-		for_each_namespaced_ref(send_ref, &symref);
+		head_ref_namespaced(send_ref, &data.symref);
+		for_each_namespaced_ref(send_ref, &data.symref);
 		advertise_shallow_grafts(1);
 		packet_flush(1);
 	} else {
 		head_ref_namespaced(check_ref, NULL);
 		for_each_namespaced_ref(check_ref, NULL);
 	}
-	string_list_clear(&symref, 1);
 
 	if (!options->advertise_refs) {
 		packet_reader_init(&reader, 0, NULL, 0,