[PATCH] connected: distinguish local/remote bad objects

[Jonathan Tan <jonathantanmy@google.com>]

When the connectivity check after a fetch fails, an error message
"<remote> did not send all necessary objects" is printed. That error
message is printed regardless of the reason of failure: in particular,
that message may be printed if the connectivity check fails because a
local object is missing. (The connectivity check reads local objects too
because it compares the set of objects that the remote claims to send
against the set of objects that our refs directly or indirectly
reference.)

The connectivity check passes, to "git rev-list", remote objects
directly and local objects through "--not". And internally, the latter
are marked with the UNINTERESTING flag. When reading a commit during the
commit walk, we know whether the commit came from an UNINTERESTING
commit or not. Therefore, use this flag to produce a clearer error
message when a bad object is read.

This necessitates changes in revision.c which is used by components
other than the connectivity check and may have different meanings for
objects passe with and without "--not", so guard the extra diagnostics
behind a CLI argument.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
We noticed, at $DAYJOB, some of these messages that were likely caused
by missing objects in the local repository instead, so here is a patch
that will make it easier to diagnose such issues.
---
 builtin/fetch.c              |  2 +-
 connected.c                  |  1 +
 revision.c                   | 16 ++++++++++++--
 revision.h                   |  3 +++
 t/t5518-fetch-exit-status.sh | 43 ++++++++++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index ac29c2b1ae..6f43b2bf8d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1133,7 +1133,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 
 		rm = ref_map;
 		if (check_connected(iterate_ref_map, &rm, &opt)) {
-			rc = error(_("%s did not send all necessary objects\n"), url);
+			rc = error(_("connectivity check failed for %s\n"), url);
 			goto abort;
 		}
 	}
diff --git a/connected.c b/connected.c
index ed3025e7a2..ea773f25db 100644
--- a/connected.c
+++ b/connected.c
@@ -94,6 +94,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		strvec_push(&rev_list.args, opt->shallow_file);
 	}
 	strvec_push(&rev_list.args,"rev-list");
+	strvec_push(&rev_list.args, "--detailed-bad-object");
 	strvec_push(&rev_list.args, "--objects");
 	strvec_push(&rev_list.args, "--stdin");
 	if (has_promisor_remote())
diff --git a/revision.c b/revision.c
index 090a967bf4..777e762373 100644
--- a/revision.c
+++ b/revision.c
@@ -367,6 +367,16 @@ void add_head_to_pending(struct rev_info *revs)
 	add_pending_object(revs, obj, "HEAD");
 }
 
+static void NORETURN bad_object(struct rev_info *revs, const char *name,
+				unsigned int flags)
+{
+	if (!revs->detailed_bad_object)
+		die("bad object %s", name);
+	if (flags & UNINTERESTING)
+		die("bad object %s (from local object store)", name);
+	die("bad object %s (from remote)", name);
+}
+
 static struct object *get_reference(struct rev_info *revs, const char *name,
 				    const struct object_id *oid,
 				    unsigned int flags)
@@ -390,7 +400,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
 			return object;
 		if (revs->exclude_promisor_objects && is_promisor_object(oid))
 			return NULL;
-		die("bad object %s", name);
+		bad_object(revs, name, flags);
 	}
 	object->flags |= flags;
 	return object;
@@ -426,7 +436,7 @@ static struct commit *handle_commit(struct rev_info *revs,
 			if (revs->exclude_promisor_objects &&
 			    is_promisor_object(&tag->tagged->oid))
 				return NULL;
-			die("bad object %s", oid_to_hex(&tag->tagged->oid));
+			bad_object(revs, oid_to_hex(&tag->tagged->oid), flags);
 		}
 		object->flags |= flags;
 		/*
@@ -2537,6 +2547,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		if (fetch_if_missing)
 			BUG("exclude_promisor_objects can only be used when fetch_if_missing is 0");
 		revs->exclude_promisor_objects = 1;
+	} else if (!strcmp(arg, "--detailed-bad-object")) {
+		revs->detailed_bad_object = 1;
 	} else {
 		int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
 		if (!opts)
diff --git a/revision.h b/revision.h
index e80c148b19..7f685dd5bb 100644
--- a/revision.h
+++ b/revision.h
@@ -328,6 +328,9 @@ struct rev_info {
 
 	/* Location where temporary objects for remerge-diff are written. */
 	struct tmp_objdir *remerge_objdir;
+
+	/* Error reporting info */
+	unsigned detailed_bad_object : 1;
 };
 
 int ref_excluded(struct string_list *, const char *path);
diff --git a/t/t5518-fetch-exit-status.sh b/t/t5518-fetch-exit-status.sh
index 5c4ac2556e..f1adac1dd6 100755
--- a/t/t5518-fetch-exit-status.sh
+++ b/t/t5518-fetch-exit-status.sh
@@ -37,4 +37,47 @@ test_expect_success 'forced update' '
 
 '
 
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'connectivity check failure due to missing local object' '
+	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+	test_when_finished "rm -rf \"$SERVER\" client" &&
+	test_create_repo "$SERVER" &&
+	test_commit -C "$SERVER" foo &&
+
+	test_create_repo client &&
+	test_commit -C client bar &&
+
+	# Simulate missing client objects.
+	rm -rf client/.git/objects/* &&
+	test_must_fail git -C client fetch $HTTPD_URL/smart/server 2>err &&
+	grep "(from local object store)" err &&
+	! grep "(from remote)" err &&
+	grep "error: connectivity check failed for" err
+'
+
+test_expect_success 'connectivity check failure due to missing remote object' '
+	SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+	test_when_finished "rm -rf \"$SERVER\" client" &&
+	test_create_repo "$SERVER" &&
+	test_commit -C "$SERVER" foo &&
+	git -C "$SERVER" config uploadpack.allowRefInWant true &&
+	SERVER_HEAD=$(git -C "$SERVER" rev-parse HEAD) &&
+	SERVER_FAKE_HEAD=$(echo $SERVER_HEAD | tr "0123456789abcdef" "123456789abcdef0") &&
+
+	test_create_repo client &&
+
+	# Make the server claim that it has $SERVER_FAKE_HEAD as
+	# refs/heads/main. The server still sends $SERVER_HEAD in the packfile,
+	# so the client will see $SERVER_FAKE_HEAD as missing.
+	echo "s=$SERVER_HEAD refs/heads/main=$SERVER_FAKE_HEAD refs/heads/main= if /wanted-refs/../packfile/" >"$HTTPD_ROOT_PATH/one-time-perl" &&
+
+	test_must_fail git -C client fetch $HTTPD_URL/one_time_perl/server refs/heads/main 2>err &&
+	grep "(from remote)" err &&
+	! grep "(from local object store)" err &&
+	grep "error: connectivity check failed for" err
+'
+
 test_done
+
-- 
2.36.1.255.ge46751e96f-goog