Re: [RFC][PATCH] object.c: use has_object() instead of repo_has_object_file()

[Jeff King <peff@peff.net>]

On Wed, Nov 16, 2022 at 10:09:56PM +0530, Kousik Sanagavarapu wrote:

> It is mentioned in object-store.h that the function
> repo_has_object_file() is deprecated. One possible alternative for this
> function is has_object() (or atleast that is how I understood it).
> 
> The file object-store.h also mentions that repo_has_object_file() and
> its fellow functions and macros can be removed once the migrations take
> place. This patch therefore is an attempt to reduce the usage of these
> functions and macros.
> 
> I request for comments as I'm not really sure about the "flags" argument
> of the has_object() function and its usage in this patch.

So you've stumbled into quite a tricky spot. :)

Yes, without specifying new flags, this patch has a change of behavior
which I think is not what we want. From 1d8d9cb620 (sha1-file: introduce
no-lazy-fetch has_object(), 2020-08-05):

  There have been a few bugs wherein Git fetches missing objects
  whenever the existence of an object is checked, even though it does
  not need to perform such a fetch. To resolve these bugs, we could look
  at all the places that has_object_file() (or a similar function) is
  used. As a first step, introduce a new function has_object() that
  checks for the existence of an object, with a default behavior of not
  fetching if the object is missing and the repository is a partial
  clone. As we verify each has_object_file() (or similar) usage, we can
  replace it with has_object(), and we will know that we are done when
  we can delete has_object_file() (and the other similar functions).
  
  Also, the new function has_object() has more appropriate defaults:
  besides not fetching, it also does not recheck packed storage.

So the new function will:

  - not recheck packed objects unless we pass HAS_OBJECT_RECHECK_PACKED;
    this is done in the default paths because a simultaneous git-gc may
    be repacking objects (and we would rather double-check than racily
    miss it). So it's appropriate behavior if the caller is
    speculatively asking "hey, we _might_ have this object", but not if
    we expect to have it.

  - not lazily fetch objects in a partial-clone repository. This again
    depends on the caller being in a situation where they are OK saying
    "no we don't have it" for an object we _could_ get if it was worth
    spending effort

The call you're touching here is in parse_object_with_flags(), which is
using the check as part of the "is it a blob that we should stream?"
check. If this call returns "no we don't have it", when we could get it
(either due to racy repack or by fetching), then we'll hit the non-blob
path that calls repo_read_object_file(). Where we would do a fresh
lookup, including re-checking packs and/or lazily fetching!

So the new behavior seems strictly worse to me. We don't avoid those
behaviors, _and_ we fail to follow the streaming-blob code path (which
means we may accidentally read a huge blob into memory. I think we'd
want to leave it as-is, or if we really want to eventually drop to a
single interface, we need has_object() to learn a new flag to enable the
lazy-fetch (and then use it along with RECHECK_PACKED).

Now all of that said, I am skeptical that these calls to
repo_has_object_file() are even doing anything useful at all. Looking at
the existing code (dropping the "+" lines from your patch):

> -	if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||
> -	    (!obj && repo_has_object_file(r, oid) &&
>  	     oid_object_info(r, oid, NULL) == OBJ_BLOB)) {
>  		if (!skip_hash && stream_object_signature(r, repl) < 0) {
>  			error(_("hash mismatch %s"), oid_to_hex(oid));

we are checking that it exits if either:

  - another object (e.g., a tree) referred to it as a blob during this
    process, and we created an in-memory "struct object" with that type

  - nobody has referred to it, and we want to check its type via
    oid_object_info()

In the second case, this seems totally pointless. We can just ask
oid_object_info() what it's type is, and it will say "no, we don't have
it" if appropriate. It will do the usual recheck-pack and lazy-fetch,
but so is repo_has_object_file(), so the short-circuit "&&" is not
helping. _If_ we were to switch to has_object() it would start to do
something, but I think that's a bad idea for the reasons given above.

In the first case, I'd likewise argue it's not doing anything useful. It
is confirming that we have the object, but so would the call to
stream_object_signature() immediately below (assuming skip_hash is not
set). If skip_hash is set, then we could either:

  1. Just assume we have it. The point of the caller passing skip_hash
     is that we don't care about checking the integrity for this use
     case, and "missing" is not really any different than "there's a
     file on disk but it might contain garbage bytes".

  2. Check repo_has_object_file() in this code path only when skip_hash
     is set. That retains the same "do we even have it" check for
     skip_hash that is performed now.

I.e., I'd suggest this patch to remove both calls entirely:

diff --git a/object.c b/object.c
index 248530ba7b..a370545405 100644
--- a/object.c
+++ b/object.c
@@ -285,9 +285,8 @@ struct object *parse_object_with_flags(struct repository *r,
 			return &commit->object;
 	}
 
-	if ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||
-	    (!obj && repo_has_object_file(r, oid) &&
-	     oid_object_info(r, oid, NULL) == OBJ_BLOB)) {
+	if ((obj && obj->type == OBJ_BLOB) ||
+	    (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {
 		if (!skip_hash && stream_object_signature(r, repl) < 0) {
 			error(_("hash mismatch %s"), oid_to_hex(oid));
 			return NULL;

But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who
added has_object(), and who added the top call to repo_has_object_file()
via df11e19648 (rev-list: support termination at promisor objects,
2017-12-08). The second call is from 090ea12671 (parse_object: avoid
putting whole blob in core, 2012-03-07), but he's no longer active on
the project. Looking at the commit, I think it was just a case of "let's
be extra careful". But as far as I can tell, it's not helping anything,
and both calls are introducing extra work doing object lookups.

-Peff