Re: [PATCH 1/7] odb/source: discern missing and corrupt objects

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

Patrick Steinhardt <ps@pks.im> writes:

> The `read_object_info()` callback of `struct odb_source` is documented
> to return a negative error code in case reading the object has failed,
> and zero otherwise. This is overly broad though, as there are two very
> different kinds of failures:
>
>   - The object may not exist in the source at all.
>
>   - The object exists, but reading it has failed, for example because
>     its on-disk state is corrupt.
>
> This distinction matters to callers: when an object is corrupt in one
> source we may still find a good copy of it in another source, so we may
> still be able to proceed with a given operation.
>
> The "packed" source already distinguishes these cases by returning a
> positive value for missing objects and a negative value in case reading
> the object has failed. But all the other sources conflate them into a
> single negative return value.

In other words, "packed" did not honor the documented contract with
the callers and nobody noticed?  It gives us a usable escape hatch ;-)

Do we need to support many other "it is an error but we treat as non
error in some context" values, like the "does not exist"?  If so, it
does make sense to say 0 is absolute success, positive values are
such half-errors, and negative values are absolute failures.  If
not, it would have been much nicer if "you asked me about this
information but there is no such object" were still signalled as an
error (i.e., negative return value) that is distinct from other
kinds of errors like I/O error (which also should be signalled by a
negative return value), instead of a positive value whose meanings
were not defined, though.

> Adapt the documentation to explicitly require the semantics of the
> "packed" backend, where we return a positive value for missing objects
> and a negative value for corrupt ones. Subsequent commits will adapt all
> the other implementations to respect those new semantics.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source.h | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/odb/source.h b/odb/source.h
> index d69f8e2d1c..4ae6cc160e 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -110,8 +110,17 @@ struct odb_source {
>  	 *     second read in case they know that the first read would have
>  	 *     already surfaced the object without reloading any on-disk state.
>  	 *
> -	 * The callback is expected to return a negative error code in case
> -	 * reading the object has failed, 0 otherwise.
> +	 * The callback is expected to return one of the following values:
> +	 *
> +	 *   - Zero in case the object has been found and its object info has
> +	 *     been read successfully.
> +	 *
> +	 *   - A positive value in case the object does not exist in this
> +	 *     source.
> +	 *
> +	 *   - A negative value in case the object exists in this source, but
> +	 *     reading its object info has failed, for example because its
> +	 *     on-disk state is corrupt.
>  	 */
>  	int (*read_object_info)(struct odb_source *source,
>  				const struct object_id *oid,
> @@ -340,7 +349,9 @@ static inline void odb_source_prepare(struct odb_source *source,
>  
>  /*
>   * Read an object from the object database source identified by its object ID.
> - * Returns 0 on success, a negative error code otherwise.
> + * Returns 0 on success, a positive value in case the object is missing in the
> + * source and a negative value in case the object exists, but reading it has
> + * failed.
>   */
>  static inline int odb_source_read_object_info(struct odb_source *source,
>  					      const struct object_id *oid,