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

> 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:
> > > > +	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.

> > > 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.