Jeff King <peff@xxxxxxxx> writes: > That changed in cfea2f2da8 (packed-backend: check whether the > "packed-refs" is regular file, 2025-02-28), which uses open_nofollow() > to check for symlinks while we open it. But it feels like it would be > more direct to just lstat() the file in the first place (which we end up > doing anyway to check for other things besides symlinks!). > ... > It's not as "atomic" as open_nofollow() and fstat(), but I don't think > we care about that for fsck. This is about consistency checking, not > trying to beat races against active adversaries (not to mention that our > open_nofollow() is best-effort anyway, and may be racy). True. Atomicity, which the use of open_nofollow() and fstat() tries to achieve, may not matter in fsck. We can think of the use of open_nofollow() in this particular codepath merely as a convenient helper function, and I do not think we have any problem with such a helper function. But open_nofollow() and its emulation can be called from other codepaths that may care about atomicity, and I am not sure what our attitude towards atomicity requirements vs platform capabilities should be. If an atomicity (or any other) requirement in a particular codepath has a simple and obvious way to solve on common platforms, but that the mechanism to implement the simple and obvious way is unavailable on other platforms, where does it lead us? For some kind of requirements, we can treat it merely as a quality of implementation issue, similar to how finalize_object_file() ideally wants to do the create(TMP) then link(TMP->FINAL) then unlink(TMP) dance (because we want to detect collisions when able) but has fallback implementation to create(TMP) then rename(TMP->FINAL) (which punts on collision detection) on platforms where the preferred way does not work. It falls into this category, I would think, to think of use of open_nofollow() in this codepath as a mere helper function that makes the code in fsck shorter. But for other kind of requirements, we want to fulfill them on all platforms that we claim to support. Using open_nofollow() to achieve hard atomicity requirement would be a bug in such a situation. Should we somehow warn our developers against its use? Idealists in us first try hard to find the right abstraction that would work everywhere, and use compat/ layer to implement that abstraction, but we of course are often not successful, and end up with a series of #ifdef for pieces of platform-specific code in fairly higher layer. It feels that open_nofollow() that is not necesarily atomic is the latter but that is done at a level that is a bit too low. I dunno.
