Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

handle write-collisions and permissions #821

Merged
merged 8 commits into from
Apr 20, 2023
Merged

handle write-collisions and permissions #821

merged 8 commits into from
Apr 20, 2023

Conversation

Byron
Copy link
Owner

@Byron Byron commented Apr 20, 2023 •
edited
Loading

Tasks

  • reproduce collisions
  • fix collisions
  • adjust permission bits to be similar to what git does after creating new loose objects

Fixes #819

@Byron
Add information about configuration flags related to fsync usage
8d5762c
@Byron
fix: collisions of newly written object's don't trigger collisions an…
416cecc 
…ymore. (#819)

It's solved by special-casing windows and assume that certain kinds of filesystem errors
are the result of a collision (with some degree of concurrency/contention).
@Byron
feat: add Object::into_tag() and Tag::decode() methods.
037f52d 
This makes the API more symmetric as similar methods exist for commits
and trees.
@Byron
make empty sink module non-public.
45a0ac1 
Not breaking as it can't be used anyway.
@Byron
feat: add encode::compute_hash() for an easy way to obtain object h…
ebf3fd4 
…ashes.
@Byron
adjust to improvements in gix-object
efeb29d
@Byron
feat: Before writing new objects, check if they exist.
afe7faa 
That way we safe expensive IO at the cost of some CPU.
@Byron
fix: set permissions of newly written loose objects to be similar to …
07e11cf 
…`git`.

Note that the current implementation lacks all of the sophistication that git
applies, and doing this properly definitely takes more work as we would need
to support `core.sharedRepository`.

Further, our tempfile implementation doesn't allow the setup of file modes
right when it matters, so that could mean quite some work to either workaround
or contribute.
@Byron
Copy link
Owner Author

Byron commented Apr 20, 2023

Permissions are quite complex in detail, but are now fixed to the point where they look similar to what git produces with default settings.

On the bright side, gitoxide will now do what git does to avoid writing duplicate loose objects to disk, which should prevent typical collisions on windows all by itself. If the object DB is used directly, it will now ignore access-denied errors on windows, assuming that there is no actual hash collision, but an issue with the filesystem. This is equivalent to the unix implementation which would also overwrite possibly existing files by renaming.

@Byron Byron merged commit 69faad0 into main Apr 20, 2023
@Byron Byron deleted the fix-819 branch April 20, 2023 16:27
@jpgrayson
Copy link
Contributor

Thanks, @Byron. This is great! I really appreciate it.

@Byron
Copy link
Owner Author

Byron commented Apr 20, 2023

I really appreciate it too :) - when investigating this issue I noticed a lot of rw objects which were clearly written by gitoxide, and I also want to see less of that. Furthermore I'd hope that overall write speeds are now increased by preventing duplication, let's see if it makes a difference for stg workloads.

Please note that the implementation might well be slower now as the additional chmod call is something git doesn't have to do. I checked with the tempfile crate if this is something that could change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Loose object tempfile failure on Windows
2 participants
@Byron @jpgrayson