| Message ID | 7cef8088cebe368e66237837eacec71381182ec1.1664316642.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fail early when partial clone prefetch fails | expand |
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. 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".
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.
diff --git a/promisor-remote.c b/promisor-remote.c index 68f46f5ec7..8b4d650b4c 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -230,18 +230,17 @@ static int remove_fetched_oids(struct repository *repo, return remaining_nr; } -int promisor_remote_get_direct(struct repository *repo, - const struct object_id *oids, - int oid_nr) +void promisor_remote_get_direct(struct repository *repo, + const struct object_id *oids, + int oid_nr) { struct promisor_remote *r; struct object_id *remaining_oids = (struct object_id *)oids; int remaining_nr = oid_nr; int to_free = 0; - int res = -1; if (oid_nr == 0) - return 0; + return; promisor_remote_init(repo); @@ -256,12 +255,9 @@ int promisor_remote_get_direct(struct repository *repo, continue; } } - res = 0; break; } if (to_free) free(remaining_oids); - - return res; } diff --git a/promisor-remote.h b/promisor-remote.h index edc45ab0f5..df36eb08ef 100644 --- a/promisor-remote.h +++ b/promisor-remote.h @@ -39,13 +39,12 @@ static inline int has_promisor_remote(void) /* * Fetches all requested objects from all promisor remotes, trying them one at - * a time until all objects are fetched. Returns 0 upon success, and non-zero - * otherwise. + * a time until all objects are fetched. * - * If oid_nr is 0, this function returns 0 (success) immediately. + * If oid_nr is 0, this function returns immediately. */ -int promisor_remote_get_direct(struct repository *repo, - const struct object_id *oids, - int oid_nr); +void promisor_remote_get_direct(struct repository *repo, + const struct object_id *oids, + int oid_nr); #endif /* PROMISOR_REMOTE_H */
No caller of promisor_remote_get_direct() is checking its return value, so remove it. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- promisor-remote.c | 12 ++++-------- promisor-remote.h | 11 +++++------ 2 files changed, 9 insertions(+), 14 deletions(-)