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

[Jonathan Tan <jonathantanmy@google.com>]

Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Tan <jonathantanmy@google.com> 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.  

True - in all cases, we either fall back to a code path where we try to
read a single object (if promisor_remote_get_direct() was called by a
prefetch) or the same code path that is taken when a non-partial-clone
tries to read a missing object. So it's not fully clear what's going on,
but it is safe. I should have added a note about this in the commit
message and will do so in the next version.

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

I'll add a note that in a subsequent patch, we will add a message that
can indeed distinguish this case.