Junio C Hamano <gitster@xxxxxxxxx> writes: > Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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.
