On Mon, Dec 30, 2024 at 06:40:53AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > By definition, two files cannot collide with each other when one of them
> > has been removed. We can thus trivially fix the issue by ignoring ENOENT
> > when opening either of the files we're about to check for collision.
>
> Thanks for digging it down to the cause.
>
> It is more like even if these two files collided (i.e. have the same
> name based on what the hash function says, with different contents),
> when one of them has been removed, we have no way to check if the
> collision is benign, and even if it were not, we cannot do anything
> about it, isn't it?
Depends on what "benign" means in this context, I guess. We can only
assert the most trivial case of it being "benign", namely that we have
computed a packfile that actually is the exact same. This is also going
to be the most common case, as everything else would depend on a
cryptographic collision of the packfile contents. And in that case... we
cannot do anything about it, yes.
> I do like the simplicity of the solution. I wonder given bad enough
> race, we could fall into a case where both files are missing?
I was wondering about that, too, but it would very much feel like a bug
to me if that were ever to happen. So I briefly considered whether I
should treat the passed-in filenames differently:
- One that must exist non-racily. This is our temporary object or
packfile that we want to move into place.
- And one that may have been removed racily. This is our target file
path that we want to overwrite, unless there is a collision.
The idea would be to only handle ENOENT for the second case. But in the
end I don't think it's worth the complexity because `check_collision()`
is used before rename(3p)ing the former into place, and that function
would already notice ENOENT anyway. So we would eventually just die the
same.
Patrick
