diff mbox series

[02/12] upload-pack: move static vars to upload_pack_data

Message ID 20200527164742.23067-3-chriscool@tuxfamily.org (mailing list archive)
State New, archived
Headers show
Series upload-pack: use 'struct upload_pack_data' thoroughly, part 2 | expand

Commit Message

Christian Couder May 27, 2020, 4:47 p.m. UTC 
As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
more thoroughly, let's move the 'no_done', 'daemon_mode' and
'timeout' variables into this struct.

They are only used by protocol v0 code since protocol v2 assumes
certain baseline capabilities, but rolling them into
upload_pack_data and just letting v2 code ignore them as it does
now is more coherent and cleaner.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 upload-pack.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

Comments

Jeff King May 27, 2020, 6:06 p.m. UTC | #1 
On Wed, May 27, 2020 at 06:47:32PM +0200, Christian Couder wrote:

> As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
> more thoroughly, let's move the 'no_done', 'daemon_mode' and
> 'timeout' variables into this struct.

Makes sense.

> They are only used by protocol v0 code since protocol v2 assumes
> certain baseline capabilities, but rolling them into
> upload_pack_data and just letting v2 code ignore them as it does
> now is more coherent and cleaner.

Is it perhaps worth keeping these v0-only fields grouped together within
the struct, along with a comment? That way nobody has to repeat your
research later into which ones are used and which ones are not.

> -static unsigned int timeout;

This was unsigned, but gets moved as...

> @@ -83,18 +80,21 @@ struct upload_pack_data {
>  	timestamp_t deepen_since;
>  	int deepen_rev_list;
>  	int deepen_relative;
> +	int timeout;

...a signed int.

It probably doesn't matter much in practice for the values we'd give it,
but the alarm() to which it's ultimately fed takes an unsigned int.

-Peff
Christian Couder June 2, 2020, 4:19 a.m. UTC | #2 
On Wed, May 27, 2020 at 8:06 PM Jeff King <peff@peff.net> wrote:
>
> On Wed, May 27, 2020 at 06:47:32PM +0200, Christian Couder wrote:

> > They are only used by protocol v0 code since protocol v2 assumes
> > certain baseline capabilities, but rolling them into
> > upload_pack_data and just letting v2 code ignore them as it does
> > now is more coherent and cleaner.
>
> Is it perhaps worth keeping these v0-only fields grouped together within
> the struct, along with a comment?

Ok, I will try to do that in part 2 and part 3.

> That way nobody has to repeat your
> research later into which ones are used and which ones are not.
>
> > -static unsigned int timeout;
>
> This was unsigned, but gets moved as...
>
> > @@ -83,18 +80,21 @@ struct upload_pack_data {
> >       timestamp_t deepen_since;
> >       int deepen_rev_list;
> >       int deepen_relative;
> > +     int timeout;
>
> ...a signed int.

Yeah, not sure what happened. Maybe I needed some coffee when I did that.

It's fixed in my current version.

Thanks,
Christian.
diff mbox series

Patch

diff --git a/upload-pack.c b/upload-pack.c
index 2fa645834a..c83c5a619b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -45,8 +45,6 @@ 
 static timestamp_t oldest_have;
 
 static int multi_ack;
-static int no_done;
-static int daemon_mode;
 /* Allow specifying sha1 if it is a ref tip. */
 #define ALLOW_TIP_SHA1	01
 /* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
@@ -56,7 +54,6 @@  static int daemon_mode;
 static unsigned int allow_unadvertised_object_request;
 static int shallow_nr;
 static struct object_array extra_edge_obj;
-static unsigned int timeout;
 static int keepalive = 5;
 /* 0 for no sideband,
  * otherwise maximum packet size (up to 65520 bytes).
@@ -83,18 +80,21 @@  struct upload_pack_data {
 	timestamp_t deepen_since;
 	int deepen_rev_list;
 	int deepen_relative;
+	int timeout;
 
 	struct list_objects_filter_options filter_options;
 
 	struct packet_writer writer;
 
 	unsigned stateless_rpc : 1;
+	unsigned daemon_mode : 1;
 
 	unsigned use_thin_pack : 1;
 	unsigned use_ofs_delta : 1;
 	unsigned no_progress : 1;
 	unsigned use_include_tag : 1;
 	unsigned done : 1;
+	unsigned no_done : 1;
 };
 
 static void upload_pack_data_init(struct upload_pack_data *data)
@@ -130,7 +130,7 @@  static void upload_pack_data_clear(struct upload_pack_data *data)
 	list_objects_filter_release(&data->filter_options);
 }
 
-static void reset_timeout(void)
+static void reset_timeout(int timeout)
 {
 	alarm(timeout);
 }
@@ -246,7 +246,7 @@  static void create_pack_file(struct upload_pack_data *pack_data)
 		int pe, pu, pollsize;
 		int ret;
 
-		reset_timeout();
+		reset_timeout(pack_data->timeout);
 
 		pollsize = 0;
 		pe = pu = -1;
@@ -428,7 +428,7 @@  static int get_common_commits(struct upload_pack_data *data,
 	for (;;) {
 		const char *arg;
 
-		reset_timeout();
+		reset_timeout(data->timeout);
 
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
 			if (multi_ack == 2
@@ -441,7 +441,7 @@  static int get_common_commits(struct upload_pack_data *data,
 			if (data->have_obj.nr == 0 || multi_ack)
 				packet_write_fmt(1, "NAK\n");
 
-			if (no_done && sent_ready) {
+			if (data->no_done && sent_ready) {
 				packet_write_fmt(1, "ACK %s\n", last_hex);
 				return 0;
 			}
@@ -919,7 +919,7 @@  static void receive_needs(struct upload_pack_data *data,
 		struct object_id oid_buf;
 		const char *arg;
 
-		reset_timeout();
+		reset_timeout(data->timeout);
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
 			break;
 
@@ -952,7 +952,7 @@  static void receive_needs(struct upload_pack_data *data,
 		else if (parse_feature_request(features, "multi_ack"))
 			multi_ack = 1;
 		if (parse_feature_request(features, "no-done"))
-			no_done = 1;
+			data->no_done = 1;
 		if (parse_feature_request(features, "thin-pack"))
 			data->use_thin_pack = 1;
 		if (parse_feature_request(features, "ofs-delta"))
@@ -995,7 +995,7 @@  static void receive_needs(struct upload_pack_data *data,
 	if (has_non_tip)
 		check_non_tip(data);
 
-	if (!use_sideband && daemon_mode)
+	if (!use_sideband && data->daemon_mode)
 		data->no_progress = 1;
 
 	if (data->depth == 0 && !data->deepen_rev_list && data->shallows.nr == 0)
@@ -1144,19 +1144,18 @@  void upload_pack(struct upload_pack_options *options)
 	struct packet_reader reader;
 	struct upload_pack_data data;
 
-	timeout = options->timeout;
-	daemon_mode = options->daemon_mode;
-
 	git_config(upload_pack_config, NULL);
 
 	upload_pack_data_init(&data);
 
 	data.stateless_rpc = options->stateless_rpc;
+	data.daemon_mode = options->daemon_mode;
+	data.timeout = options->timeout;
 
 	head_ref_namespaced(find_symref, &data.symref);
 
 	if (options->advertise_refs || !data.stateless_rpc) {
-		reset_timeout();
+		reset_timeout(data.timeout);
 		head_ref_namespaced(send_ref, &data);
 		for_each_namespaced_ref(send_ref, &data);
 		advertise_shallow_grafts(1);