Re: [PATCH] rev-list: allow cached objects in existence check

> This fixes a regression in 7c0fe330d5 (rev-list: handle missing tree
> objects properly, 2018-10-05) where rev-list will now complain about the
> empty tree when it doesn't physically exist on disk.
>
> Before that commit, we relied on the traversal code in list-objects.c to
> walk through the trees. Since it uses parse_tree(), we'd do a normal
> object lookup that includes looking in the set of "cached" objects
> (which is where our magic internal empty-tree kicks in).
>
> After that commit, we instead tell list-objects.c not to die on any
> missing trees, and we check them ourselves using has_object_file(). But
> that function uses OBJECT_INFO_SKIP_CACHED, which means we won't use our
> internal empty tree.

> This patch makes the minimal fix, which is to swap out a direct call to
> oid_object_info_extended(), minus the SKIP_CACHED flag, instead of
> calling has_object_file(). This is all that has_object_file() is doing
> under the hood. And there's little danger of unrelated fallout from
> other unexpected "cached" objects, since there's only one call site that
> ends such a cached object, and it's in git-blame.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I prepared this directly on top of 7c0fe330d5, but it should merge
> cleanly into the current tip of master.
>
> I think we might also consider just having has_object_file() respect
> cached objects. The SKIP_CACHED flag comes from Jonathan Tan's
> e83e71c5e1 (sha1_file: refactor has_sha1_file_with_flags, 2017-06-21).
> But it was just matching the old behavior; it's not clear to me that we
> particularly care about that, and it wasn't simply that nobody bothered
> to put the cached-object check into has_sha1_file().

> Some concerns/arguments against it:
>
>   - we probably would want to make sure we do not short-cut
>     write_sha1_file(). I.e., we should still write it to disk when
>     somebody wants it. But I think that works, because that function
>     uses its own check-and-freshen infrastructure.
>
>   - some callers of has_sha1_file() might care about durability between
>     processes. Because it's baked in, the empty tree is safe for that
>     (whatever follow-on process runs, it will also be baked in there).
>     But that's not necessarily true for other "cached" objects. I'm not
>     really that worried about it because we use it sparingly (the only
>     call to pretend_sha1_file() is in git-blame, and if it ever did ask
>     "do we have this object", I actually think the right answer would be
>     "yes").

>     But if this is a concern, we could perhaps have two levels of flags:
>     SKIP_CACHED and SKIP_INTERNAL.
>
>  builtin/rev-list.c           |  2 +-
>  t/t1060-object-corruption.sh | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 49d6deed70..877b6561f4 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -237,7 +237,7 @@ static inline void finish_object__ma(struct object *obj)
>  static int finish_object(struct object *obj, const char *name, void *cb_data)
>  {
>  	struct rev_list_info *info = cb_data;
> -	if (!has_object_file(&obj->oid)) {
> +	if (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {
>  		finish_object__ma(obj);
>  		return 1;
>  	}
> diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
> index ac1f189fd2..807b63b473 100755
> --- a/t/t1060-object-corruption.sh
> +++ b/t/t1060-object-corruption.sh
> @@ -125,4 +125,14 @@ test_expect_success 'fetch into corrupted repo with index-pack' '
>  	)
>  '
>  
> +test_expect_success 'internal tree objects are not "missing"' '
> +	git init missing-empty &&
> +	(
> +		cd missing-empty &&
> +		empty_tree=$(git hash-object -t tree /dev/null) &&
> +		commit=$(echo foo | git commit-tree $empty_tree) &&
> +		git rev-list --objects $commit
> +	)
> +'
> +
>  test_done