Skip to content

Conversation

@georg
Copy link
Contributor

@georg georg commented Oct 14, 2025

Adds a call to file.Sync() to util.WriteFile() (right before closing the file, so it should be functionally equivalent) to ensure test coverage.

This is a continuation of PR #126 and implements the suggested changes.

Fixes: #86
Closes: #126

0xDEC0DE and others added 2 commits April 24, 2025 18:05
Adds a call to `file.Sync()` to `util.WriteFile()` (right before
closing the file, so it should be functionally equivalent) to ensure
test coverage.

Fixes: Issue go-git#86
Also add CallLogger to mock file system to record file system calls.
The current implementation only logs sync calls but can be extended as
needed.
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@georg thanks for following up on this.

@georg
Copy link
Contributor Author

georg commented Oct 14, 2025

@pjbgf Thanks for the review. I've made the changes. Only BoundOS now reports the SyncCapability.

For ChrootOS it's a bit trickier.
I think it would report the SyncCapability correctly if the underlying FS reports it but its file implementation does not have a Sync method. I could change the chroot.newFile() implementation to return a different file type that implements Syncer if the underlying FS has the SyncCapability. What do you think?

Then there's also polyfill.Polyfill which ChrootOS uses when you initialise a new FS with os.New(dir). Polyfill has no way of finding out the underlying OS capabilities and just returns the DefaultCapabilities - see billy.Capabilities(). The comment of billy.Capabilities() is not correct at the moment because it is not returning all capabilities but just the default capabilities. We could return all capabilities here which would make the ChrootOS case work when polyfill is used, but I haven't looked into other side effects of changing billy.Capabilities().

@georg georg requested a review from pjbgf October 15, 2025 10:24
@pjbgf
Copy link
Member

pjbgf commented Oct 15, 2025

@georg thanks for looking into this. Don't worry about the ChrootOS implementation for the time being. As part of the V6 there may be some restructuring around osfs. Historically we only added a second implementation of osfs to not break backwards compatibility, so one of them may cease to exist before the first version for v6 is cut. I will create an issue to review this.

@pjbgf pjbgf merged commit d6fa0e9 into go-git:main Oct 15, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

file.Sync is not implemented

3 participants