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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jun 6, 2018, 02:12 UTC
Message-ID: <CAGf8dgL2i2R50aRM9OoKGmVgS6XOSY+rD3F=RV8qd=Ozh=ONUA@mail.gmail.com>
In-Reply-To: <20180605233504.GG9266@aiede.svl.corp.google.com>

On Tue, Jun 5, 2018 at 4:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 21 quoted lines

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

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

I'll add a comment.