Re: [PATCH v7 1/9] object-file.c: do not rename in a temp odb

On Tue, Sep 28, 2021 at 4:55 PM Jeff King [off-list ref] wrote:

On Tue, Sep 28, 2021 at 11:32:43PM +0000, Neeraj Singh via GitGitGadget wrote:

quoted

If a temporary ODB is active, as determined by GIT_QUARANTINE_PATH
being set, create object files with their final names. This avoids
an extra rename beyond what is needed to merge the temporary ODB in
tmp_objdir_migrate.

What's our goal here? Is it the performance of avoiding the extra
rename()? Or do we benefit from the simplicity of avoiding it?

If the former, do we have measurements on how much this matters?

If the latter, what does the simplicity buy us? I thought maybe it would
make reasoning about fsync() easier, because we don't have to worry
about fsyncing the rename. But we'd eventually have to rename() into the
real object directory anyway.

The reason I want to push back is...

quoted

Creating an object file with the expected final name should be okay
since the git process writing to the temporary object store is the
only writer, and it only invokes write_loose_object/create_object_file
after checking that the object doesn't exist.

...this seems like a kind-of dangerous assumption. Most of the time,
yeah, I'd expect just a single process to be writing. But one of the
things that happens during the receive-pack quarantine is that we run
hooks, which can run any set of arbitrary Git commands, including
simultaneous readers and writers. It seems like we might be introducing
subtle races there.

-Peff

Yes, the main goal was to avoid an extra rename. I see your concern
and I guess we have no way of knowing if someone is really going to
get bitten by this or not. On the other hand, we do know in the case
of batch_fsync that only Git is running against the objdir at that
time.  I'll remove this change since it's not where the real perf
benefit is.

Thanks,
Neeraj