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

Nguyễn Thái Ngọc Duy  [off-list ref] 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));