Re: [PATCH 2/2] promisor-remote: die upon failing fetch

[Junio C Hamano <gitster@pobox.com>]

Jonathan Tan <jonathantanmy@google.com> writes:

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

> +	}
> +
> +	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);
>  }