Skip to content

Conversation

@evankanderson
Copy link
Contributor

Fixes #11

I did not implement SubFS or GlobFS, because those seemed a bit trickier, and I didn't need them for my use case. 😁 Another option would be to extend the billy.File interface to add a Stat() method, but that seemed like a breaking ("v6") change, at which point you might simply want to rewrite towards io/fs, which has a testing memory filesystem already.

It turns out that the Go fstest.TestFS is a bit more stringent than the billy tests,
and it checks to see whether two Stat() calls return the same results on the same file. I disabled those tests in the unit test, but we could also adjust the Memfs implementation to store the ModTimes on each file when they are created. I thought the extra in-memory storage probably wasn't worth it, but I have a patch if you'd prefer that approach.

ok	github.com/go-git/go-billy/v5/helper/iofs	0.244s	coverage: 93.3% of statements 

Note that you'll probably need Go 1.23.x to pick up this change to the errors returned by fstest.TestFS().

Signed-off-by: Evan Anderson <evan@stacklok.com>
@evankanderson
Copy link
Contributor Author

So, it looks like the FS test fails on 1.22 and earlier (as I mentioned in the original PR) because the ModTime on Stat() is always set to Now(). Would you like me to roll in a PR to fix that, or to add filtering for earlier versions of Go's errors?

@evankanderson
Copy link
Contributor Author

I added some line filtering that I hope will work for go <= 1.22. I haven't specifically set up a test environment, but I may try doing so if the workflows don't run in the next day or so.

@evankanderson
Copy link
Contributor Author

I got the chance to test on go 1.20.3 this evening, and discovered that the pre-1.23 error includes the line TestFS found errors:, so I handled that case as well.

@evankanderson
Copy link
Contributor Author

(I think this should pass CI now)

@pjbgf
Copy link
Member

pjbgf commented Sep 19, 2024

@evankanderson thanks for putting this PR together.

I got the chance to test on go 1.20.3 this evening, and discovered that the pre-1.23 error includes the line TestFS found errors:, so I handled that case as well.

go-git and go-billy are meant to only support the last 3 stable Go versions, so we don't need to worry about v1.20. However, we need a bump to get go.mod and tests to run on Go v1.21-v1.23. Feel free to add a commit to that effect.

files := map[string]string{
"foo.txt": "hello, world",
"bar.txt": "goodbye, world",
"dir/baz.txt": "こんにちわ, world",
Copy link
Member

Choose a reason for hiding this comment

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

I would be good that tests take into account OS specific things, such as path separators:

Suggested change
"dir/baz.txt": "こんにちわ, world",
filepath.Join("dir","baz.txt"): "こんにちわ, world",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that fstest.testFS isn't written in a Windows-aware way (e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/testing/fstest/testfs.go;l=147), so I skipped this test on Windows for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me sad, but not sad enough to re-implement fstest.testFS in this file, given that we have Linux coverage and billy coverage.

Copy link
Contributor Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback, I think I addressed all of it (and it should pass the Windows CI leg now).

files := map[string]string{
"foo.txt": "hello, world",
"bar.txt": "goodbye, world",
"dir/baz.txt": "こんにちわ, world",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that fstest.testFS isn't written in a Windows-aware way (e.g. https://cs.opensource.google/go/go/+/refs/tags/go1.23.1:src/testing/fstest/testfs.go;l=147), so I skipped this test on Windows for now.

files := map[string]string{
"foo.txt": "hello, world",
"bar.txt": "goodbye, world",
"dir/baz.txt": "こんにちわ, world",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me sad, but not sad enough to re-implement fstest.testFS in this file, given that we have Linux coverage and billy coverage.

@evankanderson
Copy link
Contributor Author

I'm willing to bump go.mod and the tests, but I'd prefer to do it in a separate PR. (And this should pass on 1.20 if someone is using something that old).

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.

@evankanderson small change on the API to align with the rest of helpers, apart from that LGTM.

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.

LGTM. @evankanderson thanks for working on this. 🙇

@pjbgf pjbgf merged commit c1ee0b9 into go-git:master Sep 27, 2024
15 checks passed
pjbgf added a commit to pjbgf/go-billy that referenced this pull request Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants