Skip to content

Allow --files-list option for prefetch#588

Merged
derrickstolee merged 12 commits into
microsoft:masterfrom
turbonaitis:fetchFilesFromList
Dec 17, 2018
Merged

Allow --files-list option for prefetch#588
derrickstolee merged 12 commits into
microsoft:masterfrom
turbonaitis:fetchFilesFromList

Conversation

@turbonaitis
Copy link
Copy Markdown
Contributor

This PR addresses #580 and adds --files-list option to prefetch verb. It also adds a magic file name, to allow passing in both --files-list and --folders-list via stdin.

Copy link
Copy Markdown
Contributor

@derrickstolee derrickstolee 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 contribution! I think the command-line options need some tweaks, and a lot of style/small fixes in the implementation.

Comment thread GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/PrefetchVerbTests.cs Outdated
Comment thread GVFS/GVFS/CommandLine/PrefetchVerb.cs
Comment thread GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/PrefetchVerbTests.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
@turbonaitis
Copy link
Copy Markdown
Contributor Author

As per @derrickstolee suggestions, added separate flags to trigger file or folder list loading from stdin

@turbonaitis
Copy link
Copy Markdown
Contributor Author

Fixed the build issue, by adding the stdin option to fastfetch as well.

@derrickstolee
Copy link
Copy Markdown
Contributor

@turbonaitis Please leave FastFetch alone. That's kind of a separate project and should not be taken into account just for consistency-sake.

@turbonaitis
Copy link
Copy Markdown
Contributor Author

@derrickstolee removed. Out of interest, why do you want to prevent new features from being added to FastFetch?

@jrbriggs
Copy link
Copy Markdown
Member

@turbonaitis The fact that FastFetch lives in the VFS for Git codebase is a bit of a historical oddity. The projects try to do very different things. VFS for Git wants to virtualize an repository, only hydrating files on-demand. FastFetch wants to hydrate an entire enlistment as fast as it possibly can. The only reason it's here is because it depends on the /gvfs/objects endpoint.

In the future, I see FastFetch lifting out of the VFS for Git repository.

/cc @derrickstolee

@turbonaitis
Copy link
Copy Markdown
Contributor Author

@turbonaitis The fact that FastFetch lives in the VFS for Git codebase is a bit of a historical oddity. The projects try to do very different things. VFS for Git wants to virtualize an repository, only hydrating files on-demand. FastFetch wants to hydrate an entire enlistment as fast as it possibly can. The only reason it's here is because it depends on the /gvfs/objects endpoint.

In the future, I see FastFetch lifting out of the VFS for Git repository.

/cc @derrickstolee

I understand, that it's a separate tool with a different purpose, but I fail to understand, how adding features to it is a bad thing. In any case, it's your decision - I've updated my PR to not include changes to FastFetch.

@turbonaitis
Copy link
Copy Markdown
Contributor Author

@derrickstolee are there any more outstanding changes for this PR?

Copy link
Copy Markdown
Contributor

@derrickstolee derrickstolee left a comment

Choose a reason for hiding this comment

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

I just found a few style things at this point.

@wilbaker do you mind taking a look? Most of the low-hanging comments should be covered.

Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
@wilbaker
Copy link
Copy Markdown
Member

@wilbaker do you mind taking a look? Most of the low-hanging comments should be covered.

No problem, taking a look now

Copy link
Copy Markdown
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

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

Overall the changes are looking good, I've left some comments (mostly minor).

Comment thread GVFS/GVFS/CommandLine/PrefetchVerb.cs Outdated
Comment thread GVFS/GVFS/CommandLine/PrefetchVerb.cs Outdated
Comment thread GVFS/GVFS/CommandLine/PrefetchVerb.cs Outdated
Comment thread GVFS/GVFS/CommandLine/PrefetchVerb.cs
Comment thread GVFS/GVFS/CommandLine/PrefetchVerb.cs Outdated
Comment thread GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/PrefetchVerbTests.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs Outdated
Comment thread GVFS/GVFS.Common/Prefetch/BlobPrefetcher.cs
Copy link
Copy Markdown
Member

@wilbaker wilbaker left a comment

Choose a reason for hiding this comment

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

Looks good, I've kicked off the Mac functional tests for this PR

@turbonaitis
Copy link
Copy Markdown
Contributor Author

@wilbaker merged in changes from master to prevent merge conflicts.

@derrickstolee derrickstolee merged commit 9049ba4 into microsoft:master Dec 17, 2018
@derrickstolee
Copy link
Copy Markdown
Contributor

Thanks, @turbonaitis!

@turbonaitis
Copy link
Copy Markdown
Contributor Author

Thanks for your comments!

@turbonaitis turbonaitis deleted the fetchFilesFromList branch December 17, 2018 14:48
@jrbriggs jrbriggs added this to the S147 milestone Feb 7, 2019
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.

4 participants