Re: [PATCH 4/6] fetch-pack: make negotiation-related vars local

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 5, 2018, 23:35 UTC
Message-ID: <20180605233504.GG9266@aiede.svl.corp.google.com>
In-Reply-To: <8bda254e73fb00ff8448031002c7619bbe1dc285.1527894919.git.jonathantanmy@google.com>

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.

\o/

Show 10 quoted lines

> 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.

Hm. Is the idea that 'struct data' gets stored in the opaque 'data' member of the fetch_negotiator?

'struct data' is a quite vague type name --- it's almost equivalent to 'void' (which I suppose is the idea). How about something like 'struct negotiation_data' or 'fetch_negotiator_data' in this patch? That way this last paragraph of the commit message wouldn't be needed.

[...]

Show 12 quoted lines

> --- 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;
> +};

How does this struct get used? What does it represent? A comment might help.

The rest looks good.

Thanks, Jonathan