Subject: Re: [PATCH 1/2] promisor-remote: remove a return value

From: Junio C Hamano <[email-masked]>
Date: Thu, 29 Sep 2022 12:23:53 -0700

Jonathan Tan <[email-masked]> writes:

> No caller of promisor_remote_get_direct() is checking its return value,
> so remove it.

The above is a good explanation why this change will not hurt the
current users, but is it a good thing to do in the first place?

Isn't it an indication that all the existing callers are sloppy?  Or
does it mean that the value returned from this function is not a
useful signal to the caller?

Looking at the implementation, the function seems to want to
indicate if the fetching succeeded (with 0) or failed (with -1).
What is curious is that the function seems to consider it a success
even if some requested objects were never downloaded after
consulting all the promisor remotes.  There is no way for the caller
to obtain the list of objects it wanted to have but the function
failed to deliver, either (other than obviously redoing what
promisor-remote.c::remove_fetched_oids() does itself).

Are these what makes the returned value from the function useless
and the callers ignore it?  If these are fixed, would it make the
indication of error more useful?

Looking at some of the existing callers:

 * builtin/index-pack.c calls it to fill the "gap" and after giving
   the function a chance to download "missing" objects, goes on its
   prosessing as if nothing has happened.  We will properly die if
   any object we need is still missing, so unless we are interested
   in failing early, checking the return value of the function does
   not help all that much.

 * The caller in builtin/pack-objects.c is similar.  We have a list
   of bunch of objects we want to pack, some of which may be
   missing.  We ask all missing ones from the list in bulk before
   doing anything, but we rely on the regular codepath to notice if
   the promisor remote did not give us necessary objects.

I didn't look at others, but the above two are probably OK, unless
we want to get a signal to be cautious about poorly managed promisor
remotes.  Not distinguishing the reason why we do not have objects
between a corrupt repository and a promisor remote that breaks its
earlier promises makes it impossible to say "hey, that promisor
remote is failing its expectation, perhaps we should wean us off
from it".