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

Jonathan Tan <jonathantanmy@google.com> writes:

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".