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

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

> > > +	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?

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

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