Re: [PATCH v2] push: don't fetch commit object when checking existence

[Jeff King <peff@peff.net>]

On Wed, May 22, 2024 at 09:15:40PM +0100, Tom Hughes wrote:

> diff --git a/remote.c b/remote.c
> index 2b650b813b..20395bbbd0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1773,7 +1773,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>  		if (!reject_reason && !ref->deletion && !is_null_oid(&ref->old_oid)) {
>  			if (starts_with(ref->name, "refs/tags/"))
>  				reject_reason = REF_STATUS_REJECT_ALREADY_EXISTS;
> -			else if (!repo_has_object_file(the_repository, &ref->old_oid))
> +			else if (!repo_has_object_file_with_flags(the_repository, &ref->old_oid, OBJECT_INFO_SKIP_FETCH_OBJECT))
>  				reject_reason = REF_STATUS_REJECT_FETCH_FIRST;
>  			else if (!lookup_commit_reference_gently(the_repository, &ref->old_oid, 1) ||
>  				 !lookup_commit_reference_gently(the_repository, &ref->new_oid, 1))

This makes sense to me, as we're just speculatively asking "do we have
the object". I think for that reason it would also be reasonable to use
OBJECT_INFO_QUICK here, which would avoid a fruitless re-scan of the
local objects/ directory. We often pair the two[1].

In practice, though, I think fetching the missing object is going to be
much more expensive than a local re-scan. We tend to notice the latter
only when you have a large number of objects to check, and here we're
basically limited by the number of non-fast-forward refs you're trying
to push.

So I also think it would be OK to leave it here and only do QUICK if
somebody ever notices it.

-Peff

[1] We've talked about unifying those two flags, since they so often
    come together. There's some discussion in:

      https://lore.kernel.org/git/20191011220822.154063-1-jonathantanmy@google.com/

    that they could become one flag, but these two:

      https://lore.kernel.org/git/20190909222101.GB31319@sigill.intra.peff.net/

      https://lore.kernel.org/git/20200322054916.GB578498@coredump.intra.peff.net/

    argue that QUICK implies SKIP_FETCH, but not always the other way
    around. (Obviously getting a bit off topic for your patch; if
    anything, I think this call site would just use both for now).