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

[Jonathan Tan]

On Tue, Jun 5, 2018 at 4:35 PM, Jonathan Nieder  wrote:
> 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.

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

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

[Jonathan Nieder]

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?

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

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

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

[Jonathan Tan]

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.

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c | 104 +--
 1 file changed, 59 insertions(+), 45 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 192771a8f..ec92929bc 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(_list, commit);
+   prio_queue_put(>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 =