Re: [PATCH v4] fetch-pack: always allow fetching of literal SHA1s

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 11, 2017, 21:35 UTC
Message-ID: <20170511213517.GA21723@aiede.svl.corp.google.com>
In-Reply-To: <20170511211403.5252-1-jonathantanmy@google.com>

Hi,

Jonathan Tan wrote:

Show 7 quoted lines

> Thanks, peff. I've incorporated your suggestions - I don't feel very
> strongly about this, but I guess it's worthwhile to avoid the quadratic
> behavior if we can.
>
> Also incorporated Jonathan Nieder's suggestion about the placement of
> the last line. The relevant function is also factored out (following
> peff's suggestion).

Thanks. The structure still seems more complicated than it needs to be. More details below.

[...]

> +++ b/fetch-pack.c

[...]

Show 8 quoted lines

> @@ -592,13 +593,22 @@ static void mark_recent_complete_commits(struct fetch_pack_args *args,
>  	}
>  }
>  
> +static void add_refs_to_oidset(struct oidset *oids, const struct ref *refs)
> +{
> +	for (; refs; refs = refs->next)
> +		oidset_insert(oids, &refs->old_oid);

Makes sense.

[...]

Show 26 quoted lines

>  	/* Append unmatched requests to the list */
>  	for (i = 0; i < nr_sought; i++) {
>  		unsigned char sha1[20];
> +		int can_append = 0;
>  
>  		ref = sought[i];
>  		if (ref->match_status != REF_NOT_MATCHED)
> @@ -649,6 +661,21 @@ static void filter_refs(struct fetch_pack_args *args,
>  
>  		if ((allow_unadvertised_object_request &
>  		    (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
> +			can_append = 1;
> +		} else {
> +			if (!tip_oids_initialized) {
> +				/*
> +				 * Check all refs, including those already
> +				 * matched
> +				 */
> +				add_refs_to_oidset(&tip_oids, unmatched);
> +				add_refs_to_oidset(&tip_oids, newlist);
> +				tip_oids_initialized = 1;
> +			}
> +			can_append = oidset_contains(&tip_oids, &ref->old_oid);
> +		}
> +
> +		if (can_append) {

This structure could be simplified by putting the lazy-initializing tip_oids lookup in a separate function. For example:

	int tip_oids_contain(struct oidset *tip_oids,
		struct ref *unmatched, struct ref *newlist,
		const struct oid *id)
	{
		if (oidset_is_empty(tip_oids)) {
			add_refs_to_oidset(tip_oids, unmatched);
			add_refs_to_oidset(tip_oids, newlist);
		}
		return oidset_contains(tip_oids, id);
	}

That way, the caller could be kept simple (eliminating can_append and the repeated if).

Thanks, Jonathan