Re: [PATCH v2 5/6] odb: don't try re-fetching corrupt objects

[Justin Tobler <jltobler@gmail.com>]

On 26/10/14 10:53AM, Patrick Steinhardt wrote:
> When we failed reading an object in a repository with a promisor remote
> we try to fetch it from the promisor and then re-read it. This logic
> kicks in not only when the object couldn't be found, but also in the
> case where it could be found but the object is corrupt. It feels dubious
> though to just paper over corruption and proceed as if nothing has
> happened, happily fetching data from the promisor.

Agreed. If we know the object is corrupt, it makes sense to stop early
and report the finding.

> Adapt the code so that we only fetch objects in case they are missing.
> 
> Suggested-by: Justin Tobler <jltobler@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.c | 28 ++++++++--------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index 095183aec0..2c93a9490c 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -265,7 +265,6 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  	struct strbuf corrupt_err = STRBUF_INIT;
>  	const struct object_id *real = oid;
>  	enum odb_read_status ret;
> -	bool corrupt = false;
>  
>  	if (flags & OBJECT_INFO_LOOKUP_REPLACE)
>  		real = lookup_replace_object(odb->repo, oid);
> @@ -278,38 +277,27 @@ static enum odb_read_status do_oid_object_info_extended(struct object_database *
>  
>  	ret = odb_source_read_object_info(odb->source, real, oi, flags,
>  					  &corrupt_err);
> -	if (!ret)
> -		goto out;
> -	if (ret != ODB_READ_NOT_FOUND)
> -		corrupt = true;
> -
> -	/* Try to fetch the missing object from a promisor remote. */
> -	if (odb->repo->fetch_if_missing && repo_has_promisor_remote(odb->repo) &&
> -	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {
> +	if (ret == ODB_READ_NOT_FOUND &&
> +	    odb->repo->fetch_if_missing &&
> +	    !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT) &&
> +	    repo_has_promisor_remote(odb->repo)) {

Ok, now we only try to fetch a missing object specifically if it was
ODB_READ_NOT_FOUND in the first lookup. This means we now skip this
logic if the object is corrupt. This makes more sense to me and looks
good.

> +		/* Try to fetch the missing object from a promisor remote. */
>  		promisor_remote_get_direct(odb->repo, real, 1);
> -
>  		ret = odb_source_read_object_info(odb->source, real, oi, flags,
> -						  corrupt_err.len ? NULL : &corrupt_err);
> -		if (!ret)
> -			goto out;
> -		if (ret != ODB_READ_NOT_FOUND)
> -			corrupt = true;
> +						  &corrupt_err);
>  	}
>  
> -	if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
> +	if (ret < 0 && (flags & OBJECT_INFO_DIE_IF_CORRUPT)) {
>  		if ((flags & OBJECT_INFO_LOOKUP_REPLACE) && !oideq(real, oid))
>  			die(_("replacement %s not found for %s"),
>  			    oid_to_hex(real), oid_to_hex(oid));
> -		if (corrupt) {
> +		if (ret != ODB_READ_NOT_FOUND) {
>  			if (corrupt_err.len)
>  				die("%s", corrupt_err.buf);
>  			die(_("object %s is corrupt"), oid_to_hex(real));
>  		}
>  	}
>  
> -	ret = corrupt ? ODB_READ_ERROR : ODB_READ_NOT_FOUND;
> -
> -out:

The remainder of this patch is functionally identical and looks good.

-Justin