Re: [PATCH v2 16/17] midx: refactor interfaces to work on "packed" source

[Justin Tobler <jltobler@gmail.com>]

On 26/06/09 10:51AM, Patrick Steinhardt wrote:
> Our interfaces used to interact with MIDXs all work on top of the
> generic `struct odb_source`. This doesn't make much sense though: a MIDX
> is strictly tied to the "packed" source, so passing in a generic source
> gives the false sense that it may also work with a different type of
> source.

A `struct odb_source_packed` now acts as the source specific to all
packfiles in the repository. Being that MIDXs only work with packfiles,
it also makes sense to me to use the appropriate concrete source here.

> Fix this conceptual weirdness and instead require the caller to pass in
> a "packed" source explicitly. This also makes the next commit easier to
> implement, where we drop the pointer to the "files" source in the
> "packed" source.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/multi-pack-index.c |  29 +++++------
>  builtin/pack-objects.c     |   3 +-
>  builtin/repack.c           |   8 ++-
>  midx-write.c               |  34 ++++++-------
>  midx.c                     | 118 ++++++++++++++++++++++-----------------------
>  midx.h                     |  30 ++++++------
>  odb/source-packed.c        |  12 ++---
>  pack-bitmap.c              |   8 +--
>  pack-revindex.c            |   6 +--
>  repack-geometry.c          |   3 +-
>  repack-midx.c              |   9 ++--
>  repack.c                   |   6 +--
>  t/helper/test-read-midx.c  |   7 ++-
>  13 files changed, 144 insertions(+), 129 deletions(-)
> 
> diff --git a/builtin/multi-pack-index.c b/builtin/multi-pack-index.c
> index 00ffb36394..6e73c85cde 100644
> --- a/builtin/multi-pack-index.c
> +++ b/builtin/multi-pack-index.c
> @@ -10,6 +10,7 @@
>  #include "trace2.h"
>  #include "odb.h"
>  #include "odb/source.h"
> +#include "odb/source-files.h"
>  #include "replace-object.h"
>  #include "repository.h"
>  
> @@ -85,12 +86,12 @@ static int parse_object_dir(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> -static struct odb_source *handle_object_dir_option(struct repository *repo)
> +static struct odb_source_files *handle_object_dir_option(struct repository *repo)
>  {
>  	struct odb_source *source = odb_find_source(repo->objects, opts.object_dir);
>  	if (!source)
>  		source = odb_add_to_alternates_memory(repo->objects, opts.object_dir);
> -	return source;
> +	return odb_source_files_downcast(source);

Now that we are passing around concrete ODB sources to many of these
interfaces, callers may have to handle downcasting explicitly. Since now
the downcasting may happen a bit ealier than previously in some cases, I
wondered if this would alter any error flows. I don't think this would
be the case though because ultimately we only have the "files" source
currently anyways.

The updates to the function signatures and call sites accordingly in
this patch all look good to me.

-Justin