Re: [PATCH 6/9] sha1-file: use an object_directory for the main object dir

[Jeff King <peff@peff.net>]

On Mon, Nov 12, 2018 at 10:48:36AM -0500, Derrick Stolee wrote:

> > If the "the first one is the main store, the rest are alternates" bit is
> > too subtle, we could mark each "struct object_directory" with a bit for
> > "is_local".
> 
> This is probably a good thing to do proactively. We have the equivalent in
> the packed_git struct, but that's also because they get out of order. At the
> moment, I can't think of a read-only action that needs to treat the local
> object directory more carefully. The closest I know about is 'git
> pack-objects --local', but that also writes a pack-file.
> 
> I assume that when we write a pack-file to the "default location" we use
> get_object_directory() instead of referring to the default object_directory?

Generally, yes, though that should eventually be going away in favor of
accessing it via a "struct repository". And after my series,
get_object_directory() is just returning the_repository->objects->odb->path
(i.e., using the "first one is main" rule).

One thing that makes me nervous about a "local" flag in each struct is
that it implies that it's the source of truth for where to write to. So
what does git_object_directory() look like after that? Do we leave it
with the "first one is main" rule? Or does it become:

  for (odb = the_repository->objects->odb; odb; odb = odb->next) {
	if (odb->local)
		return odb->path;
  }
  return NULL; /* yikes? */

? That feels like it's making things more complicated, not less.

> > diff --git a/packfile.c b/packfile.c
> > index d6d511cfd2..1eda33247f 100644
> > --- a/packfile.c
> > +++ b/packfile.c
> > @@ -970,12 +970,12 @@ static void prepare_packed_git(struct repository *r)
> >   	if (r->objects->packed_git_initialized)
> >   		return;
> > -	prepare_multi_pack_index_one(r, r->objects->objectdir, 1);
> > -	prepare_packed_git_one(r, r->objects->objectdir, 1);
> > +
> >   	prepare_alt_odb(r);
> > -	for (odb = r->objects->alt_odb_list; odb; odb = odb->next) {
> > -		prepare_multi_pack_index_one(r, odb->path, 0);
> > -		prepare_packed_git_one(r, odb->path, 0);
> > +	for (odb = r->objects->odb; odb; odb = odb->next) {
> > +		int local = (odb == r->objects->odb);
> 
> Here seems to be a place where `odb->is_local` would help.

Yes, though I don't mind this spot in particular, as the check is pretty
straight-forward.

I think an example that would benefit more is the check_and_freshen()
stuff. There we have two almost-the-same wrappers, one of which operates
on just the first element of the list, and the other of which operates
on all of the elements after the first.

It could become:

  static int check_and_freshen_odb(struct object_directory *odb_list,
				   const struct object_id *oid,
				   int freshen,
				   int local)
  {
	struct object_directory *odb;

	for (odb = odb_list; odb; odb = odb->next) {
		static struct strbuf path = STRBUF_INIT;

		if (odb->local != local)
			continue;

		odb_loose_path(odb, &path, oid->hash);
		return check_and_freshen_file(path.buf, freshen);
	}
  }

  int check_and_freshen_local(const struct object_id *oid, int freshen)
  {
	return check_and_freshen_odb(the_repository->objects->odb, oid,
				     freshen, 1);
  }

  int check_and_freshen_nonlocal(const struct object_id *oid, int freshen)
  {
	return check_and_freshen_odb(the_repository->objects->odb, oid,
				     freshen, 0);
  }

I'm not sure that is a big improvement over the patch we're replying to,
though.

-Peff