Re: [PATCH 2/2] promisor-remote: die upon failing fetch
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 29, 2022, 20:14 UTC
- Message-ID
- <xmqqh70qndk1.fsf@gitster.g>
- In-Reply-To
- <7f6412eb8ce0c47a7645b89fab171a212353f8b2.1664316642.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
Show 41 quoted lines
> diff --git a/object-file.c b/object-file.c
> index 5b270f046d..5e30960234 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1599,10 +1599,6 @@ static int do_oid_object_info_extended(struct repository *r,
> if (fetch_if_missing && repo_has_promisor_remote(r) &&
> !already_retried &&
> !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
> - /*
> - * TODO Investigate checking promisor_remote_get_direct()
> - * TODO return value and stopping on error here.
> - */
> promisor_remote_get_direct(r, real, 1);
> already_retried = 1;
> continue;
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 8b4d650b4c..faa7612941 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -4,6 +4,7 @@
> #include "config.h"
> #include "transport.h"
> #include "strvec.h"
> +#include "packfile.h"
>
> struct promisor_remote_config {
> struct promisor_remote *promisors;
> @@ -238,6 +239,7 @@ void promisor_remote_get_direct(struct repository *repo,
> struct object_id *remaining_oids = (struct object_id *)oids;
> int remaining_nr = oid_nr;
> int to_free = 0;
> + int i;
>
> if (oid_nr == 0)
> return;
> @@ -255,9 +257,16 @@ void promisor_remote_get_direct(struct repository *repo,
> continue;
> }
> }
> - break;
> + goto all_fetched;OK. So when fetch_object() says it got everything we asked it to give, there is no behaviour change. But if invocations of fetch_objects() on all promisor remotes end unsuccessfully, we see if some are supposed to be available from the promisor remote and die.
An obvious alternative would be to do without the previous step, and instead tighten the callers to react to error return, and then from the new code, return a different error return value to allow the caller to tell what kind of breakage it got. But this extra die() with more sloppy callers would probably work just fine in practice.
Show 12 quoted lines
> + }
> +
> + for (i = 0; i < remaining_nr; i++) {
> + if (is_promisor_object(&remaining_oids[i]))
> + die(_("could not fetch %s from promisor remote"),
> + oid_to_hex(&remaining_oids[i]));
> }
>
> +all_fetched:
> if (to_free)
> free(remaining_oids);
> }