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

General improvements to memfs #50

Merged
merged 7 commits into from
Apr 27, 2024
Merged

General improvements to memfs #50

merged 7 commits into from
Apr 27, 2024

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Apr 19, 2024

No description provided.

@pjbgf pjbgf requested a review from hiddeco April 19, 2024 20:02
pjbgf added 6 commits April 19, 2024 21:02
@pjbgf
memfs: Ensure OpenFile and ReadDir returns self-targeted symlink
836d445 
A recent change (9a476c0) short-circuited when
trying to open symlink file that pointed to self by returning an os.ErrNotExist.
This change stops returning the error, returning the original symlink file instead

Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf
build: Bump Go to 1.20
6552f48 
Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf
build: Enable dependabot
d0b81f2 
Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf
memfs: Increase memfs test coverage
923ba4e 
Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf
memfs: Align Symlink behaviour with other filesystems
c6918c2 
This change ensures that symlinks are not resolved when creating a new
symlink. This aligns with the behaviour of osfs.

Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf
memfs: Handle error checks with error.Is
65c61a1 
Signed-off-by: Paulo Gomes <[email protected]>
@pjbgf pjbgf mentioned this pull request Apr 19, 2024
Merged
aymanbagabas
aymanbagabas approved these changes Apr 24, 2024
View reviewed changes
Copy link
Member

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

I wonder if we should drop the dependency on os and filepath? The implementation should be OS-independent since memory across all is the same. We could replace filepath with path, use the same separator = '/', and replace os errors with fs

.github/workflows/test.yml
@@ -5,15 +5,15 @@ jobs:
test:
strategy:
matrix:
go-version: [1.20.x,1.21.x]
go-version: [1.20.x,1.21.x,1.22.x]
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps ^1 to use the latest cached version?

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean, have a single ^1.20.x for the latest cached version or to do that across the matrix range?

In the past, go-billy broke on Windows for specific Go minors, so it would be good to ensure we keep the last 3 minors.

Copy link
Member

Choose a reason for hiding this comment

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

But then we'd need to constantly update the workflows. Instead, we can use [^1, ...] and whatever specific Go versions we need to test against (i.e. the ones that broke in the past)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, let's give it a go on a follow-up PR.

.github/workflows/test_js.yml
@@ -5,9 +5,12 @@ jobs:
test:
strategy:
matrix:
go-version: [1.20.x,1.21.x]
go-version: [1.21.x,1.22.x]
Copy link
Member

Choose a reason for hiding this comment

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

ditto

.github/workflows/test_wasip1.yml
@@ -5,9 +5,12 @@ jobs:
test:
strategy:
matrix:
go-version: [1.21.x]
go-version: [1.21.x,1.22.x]
Copy link
Member

Choose a reason for hiding this comment

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

ditto

memfs/memory.go Outdated Show resolved Hide resolved
memfs/memory_test.go Show resolved Hide resolved
@pjbgf
Copy link
Member Author

pjbgf commented Apr 26, 2024

I wonder if we should drop the dependency on os and filepath? The implementation should be OS-independent since memory across all is the same. We could replace filepath with path, use the same separator = '/', and replace os errors with fs

Completely agree with that - in memfs ideally would be OS-agnostic. But we would probably only be able to do that on V6 due to backwards compatibility.

I looked into this and kept the existing behaviour behind a WithLegacy option, but it broke a range of tests in Windows given the existing hybrid behaviour, so will be revisiting it later.

@pjbgf pjbgf requested a review from aymanbagabas April 26, 2024 06:01
@pjbgf pjbgf merged commit 8453aa9 into go-git:master Apr 27, 2024
15 checks passed
@pjbgf pjbgf deleted the fix-sl branch April 27, 2024 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aymanbagabas aymanbagabas aymanbagabas approved these changes

@hiddeco hiddeco Awaiting requested review from hiddeco

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pjbgf @aymanbagabas