Re: [PATCH 10/27] upload-pack: move rev-list code out of check_non_tip()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2016, 20:36 UTC
Message-ID: <xmqqshwkeq7z.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160610122714.3341-11-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 15 quoted lines

> -static void check_non_tip(void)
> +static int check_unreachable(struct object_array *src)
>  {
>...
>  	/* All the non-tip ones are ancestors of what we advertised */
> -	return;
> +	return 1;
>  
>  error:
>  	if (cmd.in >= 0)
>  		close(cmd.in);
>  	if (cmd.out >= 0)
>  		close(cmd.out);
> +	return 0;
> +}

"zero is bad, one is good" is OK as a helper function that is local. The convention needs to be documented as a comment before the function.

Perhaps if you avoid using "check_", which does not hint the return value, both the function and the caller would become easier to read.

How about calling it has_unreachable() and make the caller say

	if (!has_unreachable(&want_obj))
        	/* all requested tips are reachable from what we advertised */
		return;

instead?

Show 23 quoted lines

> +static void check_non_tip(void)
> +{
> +	int i;
> +
> +	/*
> +	 * In the normal in-process case without
> +	 * uploadpack.allowReachableSHA1InWant,
> +	 * non-tip requests can never happen.
> +	 */
> +	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
> +		goto error;
> +	if (check_unreachable(&want_obj))
> +		/* All the non-tip ones are ancestors of what we advertised */
> +		return;
> +
> +error:
>  	/* Pick one of them (we know there at least is one) */
>  	for (i = 0; i < want_obj.nr; i++) {
> -		o = want_obj.objects[i].item;
> +		struct object *o = want_obj.objects[i].item;
>  		if (!is_our_ref(o))
>  			die("git upload-pack: not our ref %s",
>  			    oid_to_hex(&o->oid));