GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Use GVFS Helper transport instead of read object hook by derrickstolee · Pull Request #122 · microsoft/scalar

Conversation

@derrickstolee

@derrickstolee Derrick Stolee (derrickstolee) commented Sep 20, 2019 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

See microsoft/git#191 for the Git code regarding the GVFS transport layer. This can work in tandem with the read-object hook, but it should make the read-object hook irrelevant.

  • Delete all references to the read-object hook in the product code. This unblocks Finish conversion to .NET Core #4 and the [Mount Removal] tasks [Mount Removal] Remove disk layout upgrade code #15, [Mount Removal] Delete Scalar.Mount.* projects #132, [Mount Removal] Remove GitObjectsRoot and LocalCacheRoot from RepoMetadata.dat and switch to 'gvfs.sharedcache' #133, [Mount Removal] Remove shared cache mapping file #135, [Mount Removal] Improve the layout of files owned by Scalar.Service and audit their ACLs #136, and possible others.

  • Delete the Sparse Verb in favor of git sparse-checkout set.

  • Delete the BlobPrefetcher and all references to --files or --folders prefetching.

Resolves #6, #7, #36.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@derrickstolee
Derrick Stolee (derrickstolee) requested review from Garima Singh (garimasi514), Jeff Hostetler (jeffhostetler), John Briggs (jrbriggs) and William Baker (wilbaker) September 20, 2019 18:22
@derrickstolee Derrick Stolee (derrickstolee) added cleanup performance git labels Sep 20, 2019
@derrickstolee Derrick Stolee (derrickstolee) added this to the MVP milestone Sep 20, 2019
@derrickstolee Derrick Stolee (derrickstolee) mentioned this pull request Oct 1, 2019
SparseVerb: the progress in 'git read-tree' is not passed through to user #130
Closed
@wilbaker William Baker (wilbaker) mentioned this pull request Oct 1, 2019
[Mount Removal] Remove GitObjectsRoot and LocalCacheRoot from RepoMetadata.dat and switch to 'gvfs.sharedCache' #137
Merged
Derrick Stolee (derrickstolee) added 2 commits October 3, 2019 12:00
@derrickstolee
Remove ReadObjectHook in favor of gvfs helper
1688026 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
SharedCacheTests: show errors on failed Git process
98a228e 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee

Derrick Stolee (derrickstolee) commented Oct 3, 2019

Copy link
Copy Markdown
Contributor Author

After Jeff Hostetler (@jeffhostetler) merges microsoft/git#191, I'll generate a -sc installer and re-push these commits.

In the meantime, the rest is available for review.

cc/ William Baker (@wilbaker), John Briggs (@jrbriggs), Kevin Willford (@kewillford)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Derrick Stolee (derrickstolee) added 8 commits October 3, 2019 15:50
@derrickstolee
Update Git to include gvfs-helper
cca637a 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
Remove more references to ReadObjectHook
eea8c54 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
Don't prefetch in sparse verb, rely on gvfs helper instead
89dba1a 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
Remove SparseVerb in favor of 'git sparse-checkout set'
a492c88 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
Remove folder and file prefetch
d7bc06a 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
Remove symlink tests
d3f6d01 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
Run SharedCacheTests without --extra-coverage
48568da 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
SharedCacheTests: reduce time spent in test by exploring one tree only
2780b39 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee Derrick Stolee (derrickstolee) mentioned this pull request Oct 3, 2019
'sparse' verb should record how long the git sparse-checkout took to run #114
Closed
@derrickstolee Derrick Stolee (derrickstolee) changed the title [PR Build] Use GVFS Helper transport instead of read object hook Use GVFS Helper transport instead of read object hook Oct 3, 2019
wilbaker
William Baker (wilbaker) suggested changes Oct 3, 2019
View reviewed changes

@wilbaker William Baker (wilbaker) left a comment

Copy link
Copy Markdown
Member

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 some minor comments and questions, overall looking good!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Scalar.Common/FileSystem/HooksInstaller.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scalar.Common/Git/GitProcess.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scripts/Mac/BuildScalarForMac.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scripts/Mac/BuildScalarForMac.sh Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scalar/CommandLine/ScalarVerb.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scalar/CommandLine/ScalarVerb.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scalar.FunctionalTests/Tests/EnlistmentPerFixture/PrefetchVerbTests.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scalar/CommandLine/PrefetchVerb.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Scalar.FunctionalTests/Tests/MultiEnlistmentTests/SharedCacheTests.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.
@wilbaker William Baker (wilbaker) mentioned this pull request Oct 3, 2019
Git should recreate 'gvfs.sharedCache` when it's missing, and re-enable MountReusesLocalCacheKeyWhenGitObjectsRootDeleted test #147
Closed
Derrick Stolee (derrickstolee) added 2 commits October 3, 2019 20:25
@derrickstolee
Delete Scalar.NativeHooks.Common
636919b 
Signed-off-by: Derrick Stolee <[email protected]>
@derrickstolee
A few more cleanups related to gvfs-helper
89e66d1 
Signed-off-by: Derrick Stolee <[email protected]>
This was referenced Oct 4, 2019
ScalarVerb: Remove EnsureLocalCacheIsHealthy #146
Merged
[WIP] More cleanups following gvfs-helper #148
Closed
jeffhostetler
Jeff Hostetler (jeffhostetler) reviewed Oct 4, 2019
View reviewed changes
Comment thread Scalar.Common/ScalarConstants.cs
@@ -36,6 +36,7 @@ public static class GitConfig
public const string ScalarTelemetryPipe = GitConfig.ScalarPrefix + "telemetry-pipe";

@jeffhostetler Jeff Hostetler (jeffhostetler) Oct 4, 2019

Copy link
Copy Markdown
Contributor

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.

Wondering if we should have a comment on the "gvfs.cache-server" and "gvfs.sharedCache" stating that we intended/need to leave the config prefix as "gvfs" and that it wasn't an oversight during the conversion.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@derrickstolee Derrick Stolee (derrickstolee) Oct 4, 2019

Copy link
Copy Markdown
Contributor Author

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.

Interesting... probably not? I did a pretty thorough find-and-replace, so we should assume from now on that every "gvfs" in the codebase is intended to be there.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@wilbaker William Baker (wilbaker) Oct 4, 2019

Copy link
Copy Markdown
Member

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.

that we intended/need to leave the config prefix as "gvfs"

Why do we need to use "gvfs" for the new settings? Is the long term plan to remove the read-object hook in GVFS as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@derrickstolee Derrick Stolee (derrickstolee) Oct 4, 2019

Copy link
Copy Markdown
Contributor Author

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.

It's because it is related to the "gvfs" protocol (the endpoints have "gvfs" in the name).

There is a possibility that we can move VFS for Git onto the gvfs-helper, too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jeffhostetler
Jeff Hostetler (jeffhostetler) approved these changes Oct 4, 2019
View reviewed changes
jrbriggs
John Briggs (jrbriggs) approved these changes Oct 4, 2019
View reviewed changes
wilbaker
William Baker (wilbaker) approved these changes Oct 4, 2019
View reviewed changes
@derrickstolee
Derrick Stolee (derrickstolee) merged commit a8b0b08 into microsoft:master Oct 4, 2019
@wilbaker William Baker (wilbaker) mentioned this pull request Oct 4, 2019
Cleanups following gvfs-helper #150
Merged
This was referenced Oct 4, 2019
[GVFS Helper] Fix parallel load test #156
Closed
Sparse: Check for superceding parents in input #125
Closed
Investigate having the 'sparse' verb call 'git status' before finishing to prime the untracked cache #116
Closed
Rename PostFetchStep to CommitGraphStep #170
Merged
Derrick Stolee (derrickstolee) added a commit that referenced this pull request Oct 10, 2019
@derrickstolee
Merge pull request #170: Rename PostFetchStep to CommitGraphStep
9244e87 
Resolves #164.

Rename `PostFetchStep` to `CommitGraphStep` because it only writes the commit-graph files. Convert the step to use the `--reachable` option instead of `--stdin-packs`. This helps in multiple ways:

1. We don't need to communicate a list of downloaded packs.
2. It's faster to start from refs than scanning packs.
3. This will work on vanilla Git repos.
4. We can now run the commit-graph step in the background at a standard cadence instead of running it after a prefetch.

Cleans up some messages that are no longer useful, including the download-object request that was made irrelevant in #122.
@derrickstolee
Derrick Stolee (derrickstolee) deleted the gvfs-helper branch November 18, 2019 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garimasi514 Garima Singh (garimasi514) Awaiting requested review from garimasi514
3 more reviewers
@wilbaker William Baker (wilbaker) wilbaker approved these changes
@jrbriggs John Briggs (jrbriggs) jrbriggs approved these changes
@jeffhostetler Jeff Hostetler (jeffhostetler) jeffhostetler approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

cleanup git performance

Projects

None yet

Milestone

MVP

Development

Successfully merging this pull request may close these issues.

Remove reliance on mount process from read-object hook

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@derrickstolee @jrbriggs @jeffhostetler @wilbaker
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.