Re: [PATCH] tmp-objdir: merge loose object index when migrating tmpdir

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:
> On 26/10/23 02:19PM, Patrick Steinhardt wrote:
> > On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:
[snip]
> > > diff --git a/loose.c b/loose.c
> > > index d6a2bb5ba3..79d4c256e0 100644
> > > --- a/loose.c
> > > +++ b/loose.c
> > > @@ -265,3 +265,75 @@ void loose_object_map_clear(struct loose_object_map **map)
> > >  	free(m);
> > >  	*map = NULL;
> > >  }
> > > +
> > > +int loose_object_idx_merge(struct repository *repo, const char *src,
> > > +			   const char *dst)
> > > +{
> > > +	struct lock_file lock = LOCK_INIT;
> > > +	struct strbuf buf = STRBUF_INIT;
> > > +	struct stat st;
> > > +	FILE *fp;
> > > +	int fd = -1;
> > > +	int ret;
> > > +
> > > +	fp = fopen(src, "r");
> > > +	if (!fp) {
> > > +		ret = error_errno(_("unable to open %s"), src);
> > > +		goto out;
> > > +	}
> > > +	if (strbuf_getwholeline(&buf, fp, '\n') || strcmp(buf.buf, loose_object_header)) {
> > > +		ret = error(_("invalid loose object index %s"), src);
> > > +		goto out;
> > > +	}
> > 
> > Curious. Why do we even expect that this file may be broken?
> 
> For our intended use case of reading the "loose-object-idx" file from
> the quarantine directory and merging its entries to the mapping file in
> the primary object directory, I wouldn't expect it to ever be broken.
> That is because realistically only a "loose-object-idx" that we create
> during the transaction should be written into the quarantine directory.
> 
> I chose to be more defensive here because the `loose_object_idx_merge()`
> function itself isn't aware of the circumstances regarding its use case.
> In theory, another caller could use any mapping file as the source.
> Ultimately though I don't feel too strongly either way and am happy to
> remove it if we don't like it.

Ah. You mean because we're called with arbitrary paths we want to verify
whether our callers know what they're doing, and consequently we check
whether the files look like a mapping file? Still a bit funny, but with
that context it at least doesn't feel too unreasonable.

> > > +	if (repo_hold_lock_file_for_update_timeout(repo, &lock, dst, 0, -1) < 0) {
> > > +		ret = error_errno(_("unable to lock %s"), dst);
> > > +		goto out;
> > > +	}
> > 
> > With a timeout of -1 we'll block indefinitely. That doesn't feel like
> > the best option to me, as the lockfile may be left over from a previous
> > process. And Git locking up indefinitely feels suboptimal to me.
> 
> That's fair. I picked this value purely because I was closely following
> how `write_one_object()` implements its logic to update the
> "loose-object-idx" file.
> 
> Would it make sense to make this value configurable? Or would it be fine
> to just set it to 1000ms? If we change this here, maybe we should also
> change it for `write_one_object()` to match?

Yeah, I think that'd be a good idea.

> > > +	fd = open(dst, O_WRONLY | O_CREAT | O_APPEND, 0666);
> > 
> > I don't think it's a good idea to append data under the lock. It may
> > easily cause us to corrupt the file in case the write succeeds
> > partially, only.
> > 
> > Our usual pattern here is to create the lockfile, write all data into
> > the new file and then rename it into place.
> 
> So here I was also following how `write_one_object()` handles writing
> its mapping entries. I was trying to keep the `loose_object_idx_merge()`
> and `write_one_object()` approaches similar. If this really isn't the
> write approach though, I can explore adapting the patch to follow our
> more typical pattern.

I think this is quite broken. Right now nobody would care because we
don't really use the compatibility hash algorithm yet. But once it
starts becoming useful this _will_ lead to corrupted files. So I'd
prefer to fix it rather than doubling down on the broken pattern.

> > I'm also a bit surprised that the file isn't sorted. Is a reader
> > expected to always read _all_ mappings?
> 
> I was surprised by this too. From my understanding, readers are not
> expected to consult the "loose-object-idx" file directly, but instead
> consult an in-memory hash map that is loaded from the file. When an
> entry is not found in map, the file is reloaded to try to pick up any
> missing entries.

Sure. But that'd still require a linear scan through all mappings in
here.

> I see this a preexisting quirk though and am not sure it is something
> that needs to be addressed as part of this change.

Yeah, I think it's fine to ignore this for now. This part here is
certainly outside the scope of the patch series.

> > > +	if (fd < 0) {
> > > +		ret = error_errno(_("unable to open %s"), dst);
> > > +		goto out;
> > > +	}
> > > +	if (fstat(fd, &st) < 0) {
> > > +		ret = error_errno(_("unable to stat %s"), dst);
> > > +		goto out;
> > > +	}
> > > +
> > > +	/* Create the loose-object-idx if one does not already exist. */
> > > +	if (!st.st_size && write_in_full(fd, loose_object_header,
> > > +					 strlen(loose_object_header)) < 0) {
> > > +		ret = error_errno(_("failed to write loose object index header"));
> > > +		goto out;
> > > +	}
> > > +
> > > +	while (!strbuf_getwholeline(&buf, fp, '\n')) {
> > > +		if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > > +			ret = error_errno(_("failed to write loose object index %s"),
> > > +					  dst);
> > > +			goto out;
> > > +		}
> > > +	}
> > 
> > When merging, wouldn't we have to verify that the object IDs that we're
> > adding don't already exist?
> 
> So typically a loose object doesn't get written to the object directory
> at all if it already exists and thus doesn't create a new mapping entry.
> There is a race though where concurrent writes of the same object could
> cause the same entry to be appended twice. This is already an existing
> problem though as `write_one_object()` also doesn't verify whether the
> object is already in the file.
> 
> With this patch, I mostly avoided many of these preexisting quirks and
> just focused on addressing the underlying bug. If we feel strongly about
> it though, I can change the approach.

I feel strongly that this mechanism is quite broken in its current form:

  - The file is unsorted, making lookups inefficient.

  - We don't deduplicate at all.

  - We never prune entries.

  - Writes are not atomic.

The first three are conceptual issues that we'd have to fix to ever make
this feature usable. But that certainly doesn't have to happen as part
of this patch series.

The last part though shouldn't be too hard to fix, and you make the
problem worse because you add a second writer that uses the same broken
pattern. So that's a part I'd like to see fixd as part of this series,
but I won't insist on it.

Thanks!

Patrick