Re: [PATCH v3 09/25] midx: avoid opening multiple MIDXs when writing
From: Taylor Blau <hidden>
Date: 2021-07-29 19:30:54
On Tue, Jul 27, 2021 at 05:19:46PM -0400, Taylor Blau wrote:
quoted hunk ↗ jump to hunk
@@ -914,10 +915,14 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * die_errno(_("unable to create leading directories of %s"), midx_name); - if (m) - ctx.m = m; - else - ctx.m = load_multi_pack_index(object_dir, 1); + for (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) { + if (!strcmp(object_dir, cur->object_dir)) { + ctx.m = cur; + break; + } + } + if (!ctx.m) + ctx.m = get_local_multi_pack_index(the_repository);
Oops, the `if (!ctx.m)` part of this diff is just plain wrong.
I think that I had in my mind that some callers don't pass object_dir,
and so that we should fall-back to the local MIDX in that case. And so I
probably meant to write `if (!object_dir && !ctx.m)` instead.
But, all of the callers *do* pass the result of get_object_directory(),
so we don't need to do anything of the sort.
On a related note, though, a side-effect of this change is that this
makes it no longer possible to do
git multi-pack-index write --object-dir=/not/an/alternate.git/objects
since get_local_multi_pack_index() will only populate the MIDXs in
alternate object stores. We never enforced that `--object-dir` must
point to an alternate, but the documentation uses `<alt>` to describe
the argument to this flag, and accepting arbitrary non-alternate paths
seems like a footgun to me.
So I'm OK with "breaking" that behavior, as long as nobody complains
loudly. Obviously it makes the fix easier to write, but I'd argue that
the behavior we're losing is worth getting rid of anyway.
Thanks,
Taylor