-
Notifications
You must be signed in to change notification settings - Fork 451
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.
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
Mac: Support projecting symbolic links #298
Conversation
GVFS/GVFS.Virtualization/Projection/GitIndexProjection.GitIndexParser.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick code review pass for now. I'll try to look more deeply later, but don't block on me.
GVFS/GVFS.Virtualization/Projection/GitIndexProjection.GitIndexParser.cs
Outdated
Show resolved
Hide resolved
GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/SymbolicLinkTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kewillford thanks for the feedback 👍
GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/SymbolicLinkTests.cs
Show resolved
Hide resolved
GitLink = 0xE000, | ||
} | ||
|
||
public FileType Type => (FileType)(this.fileTypeAndMode & FileTypeMask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me, part of the value of creating a new type is to not pay the cost of masking every time we access a property. I would either store it as a ushort and add helper methods for masking (still keeping those details private to the parsing code), or else make this a class with separate fields. I also don't see much value in making this a struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't see much value in making this a struct.
The value I saw in using a struct if we avoid having to store reference + data in memory.
What do you see as the downside of using a struct?
GVFS/GVFS.Virtualization/Projection/GitIndexProjection.GitIndexParser.cs
Outdated
Show resolved
Hide resolved
@sanoursa @kewillford I just pushed a commit that simplifies the usage and implementation of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileTypeAndMode makes sense to me. LGTM assuming the rest of the tests come in the next iteration.
GVFS/GVFS.UnitTests/Platform.Mac/MacFileSystemVirtualizerTests.cs
Outdated
Show resolved
Hide resolved
GVFS/GVFS.UnitTests/Platform.Mac/MacFileSystemVirtualizerTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just a few clean up questions.
metadata.Add("bufferContents", Encoding.UTF8.GetString(buffer)); | ||
this.Context.Tracer.RelatedError(metadata, $"{nameof(this.TryGetSymLinkTarget)}: SymLink target exceeds buffer size"); | ||
|
||
throw new GetSymLinkTargetException("SymLink target exceeds buffer size");; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would an InvalidDataException
suffice here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with a custom exception here so that:
- This code would be consistent with the approach used in
OnGetFileStream
- We can differentiate this error from other
InvalidDataException
exceptions that might occur inside the lambda (seecatch
statements below).
Unfortunately this is one spot in the code we need to use exceptions to return errors to the caller.
case SymLinkFileIndexEntry: | ||
this.Type = FileType.SymLink; | ||
break; | ||
case GitLinkFileIndexEntry: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually support gitlinks right? If we ever see a repo with this data, we may want to fail hard as early as possible, rather than wait and see...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about making a change so that we'd throw when parsing the index if we encounter a git link (i.e. submodule).
However, doing so would allow someone to make their repo unmountable:
- Clone repo
- Add git submodule
- The next time VFS for Git parses the index it would throw an exception that results in GVFS.Mount.exe shutting down
- Repo cannot be mounted or repaired
This also seemed outside the scope of adding symlink support, and so if we do want to add this validation (and prevent a repo from becoming unmountable) it's probably a big enough task to put in its own PR.
Resolves #285
The changes in this PR include support for the following (on Mac):
The changes in this PR do not include support for:
(Support for these scenarios will be added in a follow up PR for issue #320)