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

[Junio C Hamano <gitster@pobox.com>]

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

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

> +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));