Fix file locking on POSIX OS#5257
Conversation
henkesn
changed the title
|
Ready to merge. |
pks-t
left a comment
pks-t
left a comment
There was a problem hiding this comment.
Thanks a lot for your pull request! I'm very happy to see the force-flag being disambiguated, especially as it seems like none of the previous callers actually wanted to really override any pre-existing locks.
I'd be very grateful to see some reasoning inside of the commit messages, most importantly why you're making the changes and what is actually getting fixed. Otherwise it'll be much harder to dig back in history if one encounters these commits without any rationale at all, as one will have to read the complete commit first in order to get what it's about.
All in all I think that the overall goal is perfectly sound and I guess we can want to merge this after a few small touchups. Please let me know if you need any further help.
The flag GIT_FILEBUF_FORCE currently does two things:
1. It will cause the filebuf to create non-existing leading
directories for the file that is about to be written.
2. It will forcibly remove any pre-existing locks.
While most call sites actually do want (1), they do not want to
remove pre-existing locks, as that renders the locking mechanisms
effectively useless.
Introduce a new flag `GIT_FILEBUF_CREATE_LEADING_DIRS` to
separate both behaviours cleanly from each other and convert
callers to use it instead of `GIT_FILEBUF_FORCE` to have them
honor locked files correctly.
As this conversion removes all current users of `GIT_FILEBUF_FORCE`,
this commit removes the flag altogether.
|
I'm curious to know which - if any - of these actually need the create leading directory semantics? It looks like several of these are writing to the gitdir, which probably should fail if it doesn't exist. The only time that should happen is in a race condition where somebody deletes it in the middle of a libgit2 function? In that case we probably shouldn't create the gitdir. |
|
@ethomson, that's a question which also came to my mind and I tried to comment out the if (flags & GIT_FILEBUF_CREATE_LEADING_DIRS)part. I think at least all new files in new leading directories, e. g. |
Yeah. I agree that for this change that we should just update the semantics and revisit if we want on the recreating directories. Thanks for catching this and creating a fix. I'll let @pks-t take one more review. |
3 participants
See #5256
Should fail on Linux but be ok on Windows.