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

On Tue, Jun 5, 2018 at 4:35 PM, Jonathan Nieder [off-list ref] wrote:

Jonathan Tan wrote:

quoted

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/

quoted

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?

Yes.

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

I wanted to make it easier to review the subsequent patch, in that
entire functions, including the signature, can be moved verbatim. If
the project prefers that I don't do that, let me know.

[...]

quoted

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

I'll add a comment.