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.

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

Use GVFS Helper transport instead of read object hook #122

Merged
merged 12 commits into from
Oct 4, 2019
Merged

Use GVFS Helper transport instead of read object hook #122

merged 12 commits into from
Oct 4, 2019

Conversation

derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Sep 20, 2019

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.

Resolves #6, #7, #36.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
Copy link
Contributor Author

After @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/ @wilbaker, @jrbriggs, @kewillford

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee 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
Copy link
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.

Just some minor comments and questions, overall looking good!

Scalar.Common/FileSystem/HooksInstaller.cs Show resolved Hide resolved
Scalar.Common/Git/GitProcess.cs Outdated Show resolved Hide resolved
Scripts/Mac/BuildScalarForMac.sh Show resolved Hide resolved
Scripts/Mac/BuildScalarForMac.sh Outdated Show resolved Hide resolved
Scalar/CommandLine/ScalarVerb.cs Outdated Show resolved Hide resolved
Scalar/CommandLine/ScalarVerb.cs Show resolved Hide resolved
Scalar/CommandLine/PrefetchVerb.cs Show resolved Hide resolved
Signed-off-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Derrick Stolee <stolee@gmail.com>
@@ -36,6 +36,7 @@ public static class GitConfig
public const string ScalarTelemetryPipe = GitConfig.ScalarPrefix + "telemetry-pipe";
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

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'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.

@derrickstolee derrickstolee merged commit a8b0b08 into microsoft:master Oct 4, 2019
derrickstolee added a commit that referenced this pull request Oct 10, 2019
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 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove reliance on mount process from read-object hook
4 participants