Re: [PATCH 5/5] odb/source: drop `local` flag

[Justin Tobler <jltobler@gmail.com>]

On 26/10/13 09:36AM, Patrick Steinhardt wrote:
> The `local` flag indicates whether a given `struct odb_source` is a
> local source or not, where "local" is defined as being the object
> directory that is directly attached to the current repository. But now
> that alternates are handled in the "files" backend, this property is not
> something that even makes sense anymore to have on the generic source.
> 
> In fact, we already have a partial replacement for this field in `struct
> odb_files_dir`. The only other site that requires it is the packed
> object database source, because it propagates the bit into the packfiles
> it is opening.
> 
> Drop the flag from `struct odb_source` and instead move it into the
> packed source.
> 
> Note that we stop considering the `local` bit when iterating through all
> loose objects with the `ODB_FOR_EACH_OBJECT_LOCAL_ONLY` flag. This is
> fine though, as we already handle the flag in the "files" backend
> itself, where we skip over any object directory that is not marked as
> local.

I do wish we could also drop ODB_FOR_EACH_OBJECT_LOCAL_ONLY. This flag
also feels rather leaky to me. I guess though we still need a way to
ignore objects from certain sources when iterating over all sources such
as the inmemory source.

Overall, I happy to see the `local` field dropped from the generic ODB
source structure, but IMO is does make each `for_each_object` callback
handling of the ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag a bit ambiguous and
awkward.

Naive question: Do all odb_for_each_object() call sites that use the
ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag have "files" backend specific
reasons to do so?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip]
> diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
> index 868e723c81..75cf29fa9d 100644
> --- a/odb/source-inmemory.c
> +++ b/odb/source-inmemory.c
> @@ -161,8 +161,12 @@ static int odb_source_inmemory_for_each_object(struct odb_source *source,
>  	};
>  	struct object_id null_oid = { 0 };
>  
> -	if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) ||
> -	    (opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY && !source->local))
> +	/*
> +	 * In-memory objects are neither promisor objects nor do they live in
> +	 * the local object directory, so there is nothing to iterate over.
> +	 */
> +	if (opts->flags & (ODB_FOR_EACH_OBJECT_PROMISOR_ONLY |
> +			   ODB_FOR_EACH_OBJECT_LOCAL_ONLY))

So for the inmemory source, we don't want to iterate over the objects at
all if we are only interested in "local" objects.

Naive question: Are there any scenarios where we even would iterate over
an inmemory source and only be interested in "local" objects?

>  		return 0;
>  	if (!inmemory->objects)
>  		return 0;
> @@ -369,7 +373,7 @@ struct odb_source_inmemory *odb_source_inmemory_new(struct object_database *odb)
>  	struct odb_source_inmemory *source;
>  
>  	CALLOC_ARRAY(source, 1);
> -	odb_source_init(&source->base, odb, ODB_SOURCE_INMEMORY, "source", false);
> +	odb_source_init(&source->base, odb, ODB_SOURCE_INMEMORY, "source");
>  
>  	source->base.free = odb_source_inmemory_free;
>  	source->base.close = odb_source_inmemory_close;
> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index 180ce7403f..50cb388c35 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -464,8 +464,6 @@ static int odb_source_loose_for_each_object(struct odb_source *source,
>  	/* There are no loose promisor objects, so we can return immediately. */
>  	if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY))
>  		return 0;
> -	if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !source->local)
> -		return 0;

Ok, the "loose" backend callback already doesn't need to care about the
ODB_FOR_EACH_OBJECT_LOCAL_ONLY flag because it is handled at the "files"
layer.

>  	if (opts->prefix)
>  		return oidtree_each(odb_source_loose_cache(loose, opts->prefix),
> @@ -1108,13 +1106,12 @@ static int odb_source_loose_fsck(struct odb_source *source,
>  }
>  
>  struct odb_source_loose *odb_source_loose_new(struct object_database *odb,
> -					      const char *path,
> -					      bool local)
> +					      const char *path)
>  {
>  	struct odb_source_loose *loose;
>  
>  	CALLOC_ARRAY(loose, 1);
> -	odb_source_init(&loose->base, odb, ODB_SOURCE_LOOSE, path, local);
> +	odb_source_init(&loose->base, odb, ODB_SOURCE_LOOSE, path);
>  
>  	loose->base.free = odb_source_loose_free;
>  	loose->base.close = odb_source_loose_close;
[snip]
> diff --git a/odb/source-packed.h b/odb/source-packed.h
> index a0f6b5096d..e8a7b08fbb 100644
> --- a/odb/source-packed.h
> +++ b/odb/source-packed.h
> @@ -41,6 +41,13 @@ struct odb_source_packed {
>  	 */
>  	struct strmap packs_by_path;
>  
> +	/*
> +	 * Whether this store is part of the local object directory of the
> +	 * owning repository. Packfiles loaded via this store inherit this
> +	 * flag via `pack_local`. Stores for alternates are not local.
> +	 */
> +	bool local;

Here the local flag is moved to the "packed" backend...

> +
>  	/*
>  	 * Whether packfiles have already been populated with this store's
>  	 * packs.
> diff --git a/odb/source.c b/odb/source.c
> index b25ef14df8..4155fc22da 100644
[snip]
> diff --git a/odb/source.h b/odb/source.h
> index 0cfe762376..8b839caa39 100644
> --- a/odb/source.h
> +++ b/odb/source.h
> @@ -58,14 +58,6 @@ struct odb_source {
>  	/* The type used by this source. */
>  	enum odb_source_type type;
>  
> -	/*
> -	 * Figure out whether this is the local source of the owning
> -	 * repository, which would typically be its ".git/objects" directory.
> -	 * This local object directory is usually where objects would be
> -	 * written to.
> -	 */
> -	bool local;

...and here the local flag is dropped from the generic source layer.

The patch overall looks reasonable and seems to be moving us in the
right direction.

-Justin