Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement

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

Jeff King <peff@peff.net> writes:

> When we advertise a ref, the first thing we do is parse the
> pointed-to object. This gives us two things:
>
>   1. a "struct object" we can use to store flags
>
>   2. the type of the object, so we know whether we need to
>      dereference it as a tag
>
> Instead, we can just use lookup_unknown_object to get an
> object struct, and then fill in just the type field using
> sha1_object_info (which, in the case of packed files, can
> find the information without actually inflating the object
> data).
>
> This can save time if you have a large number of refs, and
> the client isn't actually going to request those refs (e.g.,
> because most of them are already up-to-date).
>
> The downside is that we are no longer verifying objects that
> we advertise by fully parsing them (however, we do still
> know we actually have them, because sha1_object_info must
> find them to get the type). While we might fail to detect a
> corrupt object here, if the client actually fetches the
> object, we will parse (and verify) it then.
>...

This is an old news, but do you recall why this patch did not update
the code in mark_our_ref() that gets "struct object *o" from parse_object()
the same way and mark them with OUR_REF flag?

I was wondering about code consolidation like this.

 upload-pack.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 95d8313..609cd6c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -722,15 +722,18 @@ static void receive_needs(void)
 	free(shallows.objects);
 }
 
+static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data);
+
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
 		" side-band-64k ofs-delta shallow no-progress"
 		" include-tag multi_ack_detailed";
-	struct object *o = lookup_unknown_object(sha1);
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
+	mark_our_ref(refname, sha1, flag, cb_data);
+
 	if (capabilities)
 		packet_write(1, "%s %s%c%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
@@ -740,10 +743,6 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	else
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
 	capabilities = NULL;
-	if (!(o->flags & OUR_REF)) {
-		o->flags |= OUR_REF;
-		nr_our_refs++;
-	}
 	if (!peel_ref(refname, peeled))
 		packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons);
 	return 0;
@@ -751,7 +750,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 
 static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
-	struct object *o = parse_object(sha1);
+	struct object *o = parse_object(sha1); /* lookup-unknown??? */
 	if (!o)
 		die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
 	if (!(o->flags & OUR_REF)) {