CloneVerb: Force download of commit and trees on failed initial checkout #313
Conversation
GVFS/GVFS/CommandLine/GVFSVerb.cs
Outdated
| return true; | ||
| } | ||
|
|
||
| protected bool ForceTryDownloadCommit( |
There was a problem hiding this comment.
I don't see anything being "forced" here. This is really referring to the other method, which first checks the cache. I would suggest just adding a skipLocalCacheCheck param to the existing method instead.
GVFS/GVFS/CommandLine/GVFSVerb.cs
Outdated
| GVFSGitObjects gitObjects, | ||
| out string error) | ||
| { | ||
| if (!gitObjects.TryDownloadCommit(commitId)) |
There was a problem hiding this comment.
Does the objects endpoint always respond with all of the trees reachable by commitId?
There was a problem hiding this comment.
All trees reachable from its root tree, yes.
I'm trying to make sure I understand this scenario. If this were to happen, is the problem that not all reachable trees were downloaded? Or is the issue that they were downloaded as loose objects? |
The issue is that we don't have all reachable trees. When we ask for objects one-at-a-time, we don't close by reachability. But on clone time, we ask for and expect a reachable closure of trees. Our condition of checking just the commit and root tree is insufficient for this case. |
derrickstolee
force-pushed
the
checkout-gaps
branch
from
f01f433
to
49a55aa
Compare
derrickstolee
force-pushed
the
checkout-gaps
branch
3 times, most recently
from
c7e4cdb
to
c9e3c2b
Compare
GVFS/GVFS.FunctionalTests/Tests/MultiEnlistmentTests/SharedCacheTests.cs
Outdated
Show resolved
Hide resolved
| string[] packDirFiles = Directory.EnumerateFiles(packDir, "*", SearchOption.AllDirectories).ToArray(); | ||
| for (int i = 0; i < packDirFiles.Length; i++) | ||
| { | ||
| this.fileSystem.DeleteFile(Path.Combine(packDir, packDirFiles[i])); |
There was a problem hiding this comment.
Your other option would be to use a new cache location for this test case only, and clone with --no-prefetch
There was a problem hiding this comment.
I'll use a new location.
GVFS/GVFS/CommandLine/CloneVerb.cs
Outdated
| // It is possible to have the above TryDownloadCommit() fail because we | ||
| // already have the commit and root tree we intend to check out, but | ||
| // don't have a tree further down the working directory. If we fail | ||
| // checkout here, it may be due to not having these trees and the |
There was a problem hiding this comment.
It's no longer "may be" since you're now checking the specific error
GVFS/GVFS/CommandLine/GVFSVerb.cs
Outdated
| @@ -401,9 +401,10 @@ protected void ValidatePathParameter(string path) | |||
| GitObjectsHttpRequestor objectRequestor, | |||
| GVFSGitObjects gitObjects, | |||
| GitRepo repo, | |||
| out string error) | |||
| out string error, | |||
| bool ignoreIfRootExists = true) | |||
There was a problem hiding this comment.
This name is confusing, and results in a double-negative check down below with !ignore. Rename it to checkLocalObjectCache?
derrickstolee
force-pushed
the
checkout-gaps
branch
from
c9e3c2b
to
f97b5cd
Compare
On Clone, we need to create the
.git/indexfile during a checkout, which requires having all trees reachable from the root tree of the tip commit. To ensure we have all of the objects, we callTryDownloadCommit(). However, this method doesn't actually try downloading a pack of trees if we already have the commit and root tree.Due to the shared object cache, we may have these objects for a few reasons:
This fixes the issue by forcibly downloading the packfile if the checkout fails and trying again.
Resolves #306.