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@xxxxxxxxxx/
that they could become one flag, but these two:
https://lore.kernel.org/git/20190909222101.GB31319@xxxxxxxxxxxxxxxxxxxxx/
https://lore.kernel.org/git/20200322054916.GB578498@xxxxxxxxxxxxxxxxxxxxxxx/
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).
