Re: [PATCH 1/9] odb: compute compat object ID in `odb_write_object_ext()`

On 26/07/17 11:32AM, Patrick Steinhardt wrote:
> Repositories can have a compatibility hash configured, which means that
> such a repository is expected to maintain a mapping between canonical
> and compatibility object hashes. Maintaining this mapping is the
> responsibility of the object database sources, where we either store
> them as part of the loose objects map or in packfile indices v3 (once we
> gain support for this feature).

Makes sense. Each ODB source should be responsible to tracking how an
objects maps from one hash to another for compatibility.

> But besides storing these compatibility hashes, the sources are also
> responsible for generating the compatibility hash in the first place.
> This is somewhat unnecessary though, as the compatibility hash should be
> computed the same no matter which source is being used. The consequence
> is that we need to duplicate this functionality across the different
> backends, which does not make a lot of sense.

Agreed, there is no need to duplicate logic as the hashes that get
generated should be the same regardless of the backend.

> Refactor the code so that we instead compute the compatibility hash in
> `odb_write_object_ext()` and then pass the computed value to the
> sources. No callers need adjustment as there are none that write objects
> via the source interfaces directly.
> 
> Signed-off-by: Patrick Steinhardt <[email protected]>
> ---
>  odb.c                 | 26 ++++++++++++++++++++++++--
>  odb.h                 | 10 ++++++----
>  odb/source-files.c    |  2 +-
>  odb/source-inmemory.c |  2 +-
>  odb/source-loose.c    | 24 +++---------------------
>  odb/source-packed.c   |  2 +-
>  odb/source.h          |  4 ++--
>  7 files changed, 38 insertions(+), 32 deletions(-)
> 
> diff --git a/odb.c b/odb.c
> index cf6e7938c0..1d6538163b 100644
> --- a/odb.c
> +++ b/odb.c
> @@ -989,11 +989,33 @@ int odb_write_object_ext(struct object_database *odb,
>  			 const void *buf, unsigned long len,
>  			 enum object_type type,
>  			 struct object_id *oid,
> -			 struct object_id *compat_oid,
> +			 const struct object_id *compat_oid_in,
>  			 enum odb_write_object_flags flags)
>  {
> +	const struct git_hash_algo *compat = odb->repo->compat_hash_algo;
> +	struct object_id compat_oid, *compat_oid_p = NULL;
> +
> +	if (compat) {
> +		const struct git_hash_algo *algo = odb->repo->hash_algo;
> +
> +		if (compat_oid_in) {
> +			oidcpy(&compat_oid, compat_oid_in);
> +		} else if (type == OBJ_BLOB) {
> +			hash_object_file(compat, buf, len, type, &compat_oid);
> +		} else {
> +			struct strbuf converted = STRBUF_INIT;
> +			convert_object_file(odb->repo, &converted, algo, compat,
> +					    buf, len, type, 0);
> +			hash_object_file(compat, converted.buf, converted.len,
> +					 type, &compat_oid);
> +			strbuf_release(&converted);
> +		}
> +
> +		compat_oid_p = &compat_oid;
> +	}

Here we lift up the logic to gnerate the compat hash out of the backend
and into `odb_write_object_ext()` so the resulting hash can be wired to
the ODB source callback to write the object. The logic itself is the
same and looks good.

The rest of this patch is mainly just updating the callsites accordingly
and also looks good.

-Justin