On 26/10/26 07:39AM, Patrick Steinhardt wrote:
On Fri, Oct 23, 2026 at 04:25:12PM -0500, Justin Tobler wrote:
quoted
On 26/10/23 02:19PM, Patrick Steinhardt wrote:
quoted
On Thu, Oct 22, 2026 at 06:40:07PM -0500, Justin Tobler wrote:
quoted
+ 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.
Your understanding is correct. This may be more defensive that
realistically neccessary. Probably doesn't matter too much either way
though.
quoted
quoted
quoted
+ 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.
Ok, I'll update to use 1000ms in the next version.
[snip]
quoted
quoted
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.
Ya, I completely agree. There are quite a few existing problems here.
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.
That's fair. I'll update in the next version to make writing the
"loose-object-idx" file here atomic. I'm not entirely sure if I should
go ahead and update the code in `write_one_object()` to also be atomic
though.
One concern I have is operations that may write many loose objects,
would rewrite the "loose-object-idx" file for each object written. If
the mapping file becomes quite large, I suppose this could create a
significant performance regression. If these loose object writes were
batched in a transaction, we could wait to write the final mapping file
at transaction commit, but that may not always be the case.
Right now this feature isn't really usable to begin with, so maybe it
doesn't matter too much either way. For now, I'll proceed with just
updating the mapping file writer adding in this patch to be atomic.
-Justin