Re: [PATCH v2 5/8] fetch-pack: make negotiation-related vars local

[Brandon Williams <bmwill@google.com>]

On 06/06, Jonathan Tan wrote:
> Reduce the number of global variables by making the priority queue and
> the count of non-common commits in it local, passing them as a struct to
> various functions where necessary.
> 
> This also helps in the case that fetch_pack() is invoked twice in the
> same process (when tag following is required when using a transport that
> does not support tag following), in that different priority queues will
> now be used in each invocation, instead of reusing the possibly
> non-empty one.
> 
> The struct containing these variables is named "data" to ease review of
> a subsequent patch in this series - in that patch, this struct
> definition and several functions will be moved to a negotiation-specific
> file, and this allows the move to be verbatim.

Ok I spoke too soon in my other mail.  You've cleaned this up by moving
these things to local stack vars but you still don't clear the priority
queue meaning that memory is now leaking.  I think you should insert a
call to clear the priority queue in the earlier patch before returning
from the fetch code.  This way you have a clear place to update that
call to clear in this patch so that you don't forget any memory leaks.

I know this may still change later on in this series but it should still
get taken care of to make the review process easier.

> 
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  fetch-pack.c | 104 +++++++++++++++++++++++++++++----------------------
>  1 file changed, 59 insertions(+), 45 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 114207b8e..c31644bb9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -50,8 +50,12 @@ static int marked;
>   */
>  #define MAX_IN_VAIN 256
>  
> -static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband;
> +struct data {
> +	struct prio_queue rev_list;
> +	int non_common_revs;
> +};
> +
> +static int multi_ack, use_sideband;
>  /* 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). */
> @@ -93,7 +97,8 @@ static void cache_one_alternate(const char *refname,
>  	cache->items[cache->nr++] = obj;
>  }
>  
> -static void for_each_cached_alternate(void (*cb)(struct object *))
> +static void for_each_cached_alternate(struct data *data,
> +				      void (*cb)(struct data *, struct object *))
>  {
>  	static int initialized;
>  	static struct alternate_object_cache cache;
> @@ -105,10 +110,10 @@ static void for_each_cached_alternate(void (*cb)(struct object *))
>  	}
>  
>  	for (i = 0; i < cache.nr; i++)
> -		cb(cache.items[i]);
> +		cb(data, cache.items[i]);
>  }
>  
> -static void rev_list_push(struct commit *commit, int mark)
> +static void rev_list_push(struct data *data, struct commit *commit, int mark)
>  {
>  	if (!(commit->object.flags & mark)) {
>  		commit->object.flags |= mark;
> @@ -116,19 +121,20 @@ static void rev_list_push(struct commit *commit, int mark)
>  		if (parse_commit(commit))
>  			return;
>  
> -		prio_queue_put(&rev_list, commit);
> +		prio_queue_put(&data->rev_list, commit);
>  
>  		if (!(commit->object.flags & COMMON))
> -			non_common_revs++;
> +			data->non_common_revs++;
>  	}
>  }
>  
> -static int rev_list_insert_ref(const char *refname, const struct object_id *oid)
> +static int rev_list_insert_ref(struct data *data, const char *refname,
> +			       const struct object_id *oid)
>  {
>  	struct object *o = deref_tag(parse_object(oid), refname, 0);
>  
>  	if (o && o->type == OBJ_COMMIT)
> -		rev_list_push((struct commit *)o, SEEN);
> +		rev_list_push(data, (struct commit *)o, SEEN);
>  
>  	return 0;
>  }
> @@ -136,7 +142,7 @@ static int rev_list_insert_ref(const char *refname, const struct object_id *oid)
>  static int rev_list_insert_ref_oid(const char *refname, const struct object_id *oid,
>  				   int flag, void *cb_data)
>  {
> -	return rev_list_insert_ref(refname, oid);
> +	return rev_list_insert_ref(cb_data, refname, oid);
>  }
>  
>  static int clear_marks(const char *refname, const struct object_id *oid,
> @@ -156,7 +162,7 @@ static int clear_marks(const char *refname, const struct object_id *oid,
>     when only the server does not yet know that they are common).
>  */
>  
> -static void mark_common(struct commit *commit,
> +static void mark_common(struct data *data, struct commit *commit,
>  		int ancestors_only, int dont_parse)
>  {
>  	if (commit != NULL && !(commit->object.flags & COMMON)) {
> @@ -166,12 +172,12 @@ static void mark_common(struct commit *commit,
>  			o->flags |= COMMON;
>  
>  		if (!(o->flags & SEEN))
> -			rev_list_push(commit, SEEN);
> +			rev_list_push(data, commit, SEEN);
>  		else {
>  			struct commit_list *parents;
>  
>  			if (!ancestors_only && !(o->flags & POPPED))
> -				non_common_revs--;
> +				data->non_common_revs--;
>  			if (!o->parsed && !dont_parse)
>  				if (parse_commit(commit))
>  					return;
> @@ -179,7 +185,7 @@ static void mark_common(struct commit *commit,
>  			for (parents = commit->parents;
>  					parents;
>  					parents = parents->next)
> -				mark_common(parents->item, 0, dont_parse);
> +				mark_common(data, parents->item, 0, dont_parse);
>  		}
>  	}
>  }
> @@ -188,7 +194,7 @@ static void mark_common(struct commit *commit,
>    Get the next rev to send, ignoring the common.
>  */
>  
> -static const struct object_id *get_rev(void)
> +static const struct object_id *get_rev(struct data *data)
>  {
>  	struct commit *commit = NULL;
>  
> @@ -196,16 +202,16 @@ static const struct object_id *get_rev(void)
>  		unsigned int mark;
>  		struct commit_list *parents;
>  
> -		if (rev_list.nr == 0 || non_common_revs == 0)
> +		if (data->rev_list.nr == 0 || data->non_common_revs == 0)
>  			return NULL;
>  
> -		commit = prio_queue_get(&rev_list);
> +		commit = prio_queue_get(&data->rev_list);
>  		parse_commit(commit);
>  		parents = commit->parents;
>  
>  		commit->object.flags |= POPPED;
>  		if (!(commit->object.flags & COMMON))
> -			non_common_revs--;
> +			data->non_common_revs--;
>  
>  		if (commit->object.flags & COMMON) {
>  			/* do not send "have", and ignore ancestors */
> @@ -220,9 +226,9 @@ static const struct object_id *get_rev(void)
>  
>  		while (parents) {
>  			if (!(parents->item->object.flags & SEEN))
> -				rev_list_push(parents->item, mark);
> +				rev_list_push(data, parents->item, mark);
>  			if (mark & COMMON)
> -				mark_common(parents->item, 1, 0);
> +				mark_common(data, parents->item, 1, 0);
>  			parents = parents->next;
>  		}
>  	}
> @@ -296,9 +302,9 @@ static void send_request(struct fetch_pack_args *args,
>  		write_or_die(fd, buf->buf, buf->len);
>  }
>  
> -static void insert_one_alternate_object(struct object *obj)
> +static void insert_one_alternate_object(struct data *data, struct object *obj)
>  {
> -	rev_list_insert_ref(NULL, &obj->oid);
> +	rev_list_insert_ref(data, NULL, &obj->oid);
>  }
>  
>  #define INITIAL_FLUSH 16
> @@ -321,7 +327,7 @@ static int next_flush(int stateless_rpc, int count)
>  	return count;
>  }
>  
> -static int find_common(struct fetch_pack_args *args,
> +static int find_common(struct data *data, struct fetch_pack_args *args,
>  		       int fd[2], struct object_id *result_oid,
>  		       struct ref *refs)
>  {
> @@ -337,8 +343,8 @@ static int find_common(struct fetch_pack_args *args,
>  	if (args->stateless_rpc && multi_ack == 1)
>  		die(_("--stateless-rpc requires multi_ack_detailed"));
>  
> -	for_each_ref(rev_list_insert_ref_oid, NULL);
> -	for_each_cached_alternate(insert_one_alternate_object);
> +	for_each_ref(rev_list_insert_ref_oid, data);
> +	for_each_cached_alternate(data, insert_one_alternate_object);
>  
>  	fetching = 0;
>  	for ( ; refs ; refs = refs->next) {
> @@ -456,7 +462,7 @@ static int find_common(struct fetch_pack_args *args,
>  	retval = -1;
>  	if (args->no_dependents)
>  		goto done;
> -	while ((oid = get_rev())) {
> +	while ((oid = get_rev(data))) {
>  		packet_buf_write(&req_buf, "have %s\n", oid_to_hex(oid));
>  		print_verbose(args, "have %s", oid_to_hex(oid));
>  		in_vain++;
> @@ -514,7 +520,7 @@ static int find_common(struct fetch_pack_args *args,
>  					} else if (!args->stateless_rpc
>  						   || ack != ACK_common)
>  						in_vain = 0;
> -					mark_common(commit, 0, 1);
> +					mark_common(data, commit, 0, 1);
>  					retval = 0;
>  					got_continue = 1;
>  					if (ack == ACK_ready)
> @@ -704,7 +710,7 @@ static void filter_refs(struct fetch_pack_args *args,
>  	*refs = newlist;
>  }
>  
> -static void mark_alternate_complete(struct object *obj)
> +static void mark_alternate_complete(struct data *unused, struct object *obj)
>  {
>  	mark_complete(&obj->oid);
>  }
> @@ -741,7 +747,8 @@ static int add_loose_objects_to_set(const struct object_id *oid,
>   * earliest commit time of the objects in refs that are commits and that we know
>   * the commit time of.
>   */
> -static void mark_complete_and_common_ref(struct fetch_pack_args *args,
> +static void mark_complete_and_common_ref(struct data *data,
> +					 struct fetch_pack_args *args,
>  					 struct ref **refs)
>  {
>  	struct ref *ref;
> @@ -792,7 +799,7 @@ static void mark_complete_and_common_ref(struct fetch_pack_args *args,
>  	if (!args->no_dependents) {
>  		if (!args->deepen) {
>  			for_each_ref(mark_complete_oid, NULL);
> -			for_each_cached_alternate(mark_alternate_complete);
> +			for_each_cached_alternate(NULL, mark_alternate_complete);
>  			commit_list_sort_by_date(&complete);
>  			if (cutoff)
>  				mark_recent_complete_commits(args, cutoff);
> @@ -810,9 +817,10 @@ static void mark_complete_and_common_ref(struct fetch_pack_args *args,
>  				continue;
>  
>  			if (!(o->flags & SEEN)) {
> -				rev_list_push((struct commit *)o, COMMON_REF | SEEN);
> +				rev_list_push(data, (struct commit *)o,
> +					      COMMON_REF | SEEN);
>  
> -				mark_common((struct commit *)o, 1, 1);
> +				mark_common(data, (struct commit *)o, 1, 1);
>  			}
>  		}
>  	}
> @@ -995,6 +1003,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  	struct object_id oid;
>  	const char *agent_feature;
>  	int agent_len;
> +	struct data data = { { compare_commits_by_commit_date } };
>  
>  	sort_ref_list(&ref, ref_compare_name);
>  	QSORT(sought, nr_sought, cmp_ref_by_name);
> @@ -1070,13 +1079,13 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  	if (marked)
>  		for_each_ref(clear_marks, NULL);
>  	marked = 1;
> -	mark_complete_and_common_ref(args, &ref);
> +	mark_complete_and_common_ref(&data, args, &ref);
>  	filter_refs(args, &ref, sought, nr_sought);
>  	if (everything_local(args, &ref)) {
>  		packet_flush(fd[1]);
>  		goto all_done;
>  	}
> -	if (find_common(args, fd, &oid, ref) < 0)
> +	if (find_common(&data, args, fd, &oid, ref) < 0)
>  		if (!args->keep_pack)
>  			/* When cloning, it is not unusual to have
>  			 * no common commit.
> @@ -1157,13 +1166,14 @@ static void add_common(struct strbuf *req_buf, struct oidset *common)
>  	}
>  }
>  
> -static int add_haves(struct strbuf *req_buf, int *haves_to_send, int *in_vain)
> +static int add_haves(struct data *data, struct strbuf *req_buf,
> +		     int *haves_to_send, int *in_vain)
>  {
>  	int ret = 0;
>  	int haves_added = 0;
>  	const struct object_id *oid;
>  
> -	while ((oid = get_rev())) {
> +	while ((oid = get_rev(data))) {
>  		packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
>  		if (++haves_added >= *haves_to_send)
>  			break;
> @@ -1182,7 +1192,8 @@ static int add_haves(struct strbuf *req_buf, int *haves_to_send, int *in_vain)
>  	return ret;
>  }
>  
> -static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,
> +static int send_fetch_request(struct data *data, int fd_out,
> +			      const struct fetch_pack_args *args,
>  			      const struct ref *wants, struct oidset *common,
>  			      int *haves_to_send, int *in_vain)
>  {
> @@ -1238,7 +1249,7 @@ static int send_fetch_request(int fd_out, const struct fetch_pack_args *args,
>  		add_common(&req_buf, common);
>  
>  		/* Add initial haves */
> -		ret = add_haves(&req_buf, haves_to_send, in_vain);
> +		ret = add_haves(data, &req_buf, haves_to_send, in_vain);
>  	}
>  
>  	/* Send request */
> @@ -1275,7 +1286,8 @@ static int process_section_header(struct packet_reader *reader,
>  	return ret;
>  }
>  
> -static int process_acks(struct packet_reader *reader, struct oidset *common)
> +static int process_acks(struct data *data, struct packet_reader *reader,
> +			struct oidset *common)
>  {
>  	/* received */
>  	int received_ready = 0;
> @@ -1294,7 +1306,7 @@ static int process_acks(struct packet_reader *reader, struct oidset *common)
>  				struct commit *commit;
>  				oidset_insert(common, &oid);
>  				commit = lookup_commit(&oid);
> -				mark_common(commit, 0, 1);
> +				mark_common(data, commit, 0, 1);
>  			}
>  			continue;
>  		}
> @@ -1372,6 +1384,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  	struct packet_reader reader;
>  	int in_vain = 0;
>  	int haves_to_send = INITIAL_FLUSH;
> +	struct data data = { { compare_commits_by_commit_date } };
>  	packet_reader_init(&reader, fd[0], NULL, 0,
>  			   PACKET_READ_CHOMP_NEWLINE);
>  
> @@ -1392,18 +1405,19 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  			marked = 1;
>  
>  			/* Filter 'ref' by 'sought' and those that aren't local */
> -			mark_complete_and_common_ref(args, &ref);
> +			mark_complete_and_common_ref(&data, args, &ref);
>  			filter_refs(args, &ref, sought, nr_sought);
>  			if (everything_local(args, &ref))
>  				state = FETCH_DONE;
>  			else
>  				state = FETCH_SEND_REQUEST;
>  
> -			for_each_ref(rev_list_insert_ref_oid, NULL);
> -			for_each_cached_alternate(insert_one_alternate_object);
> +			for_each_ref(rev_list_insert_ref_oid, &data);
> +			for_each_cached_alternate(&data,
> +						  insert_one_alternate_object);
>  			break;
>  		case FETCH_SEND_REQUEST:
> -			if (send_fetch_request(fd[1], args, ref, &common,
> +			if (send_fetch_request(&data, fd[1], args, ref, &common,
>  					       &haves_to_send, &in_vain))
>  				state = FETCH_GET_PACK;
>  			else
> @@ -1411,7 +1425,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  			break;
>  		case FETCH_PROCESS_ACKS:
>  			/* Process ACKs/NAKs */
> -			switch (process_acks(&reader, &common)) {
> +			switch (process_acks(&data, &reader, &common)) {
>  			case 2:
>  				state = FETCH_GET_PACK;
>  				break;
> -- 
> 2.17.0.768.g1526ddbba1.dirty
> 

-- 
Brandon Williams