Merge milestones/M139 to releases/shipped#180
Conversation
Update the readme to indicate the rename is in progress
Handle IOExceptions when setting Console.InputEncoding Only add to the projection if the index entry has the skipworktree bit and is Update Git for Windows for status cache fixes Stand up script to enable Mac functional tests to run as a build definition Native hooks on Mac Update the readme to indicate the rename is in progress Remove GC.Collect, update allocation strategy, and add logging for index parsing Update to ProjFS version 2018.719.1 Remove the string split from upgrading to modified paths Update git for windows with version that uses new msys2 runtime add utility method to convert from Windows to Unix paths Run prefetch in the background Update to ProjFS version 2018.706.1 Tweak logic for determining the current test directory Update GVFS.Hooks to work on Mac, install GVFS.Hooks on Mac, and fix clone+mount with GVFS aware Git Fixes incremental builds when changing props files in GVFS.Build
Update SHA values in functional tests to work against test repo Install GVFS-Aware Git on MacOS (without standing up PreBuild) Only start the background prefetch if the operation would use a cache server Prevent NullReferenceException during enumeration callbacks Update GVFS VC Runtime platform and remove .NET 3.5 dependency Fix build error on VS2017 Update 8 Merge features/status_cache to master
sanoursa
left a comment
sanoursa
left a comment
There was a problem hiding this comment.
I would like to make sure we're rock solid on the enable-after-reboot behavior, because a bug in that code will result in incorrect behavior for git status. We also need to make sure that nothing in #170 is a product issue.
| /// <returns>True on success, False on failure</returns> | ||
| private bool MoveCacheFileToFinalLocation(string tmpStatusFilePath) | ||
| { | ||
| Debug.Assert(Monitor.IsEntered(this.cacheFileLock), "Attempting to update the git status cache file without the cacheFileLock"); |
There was a problem hiding this comment.
@jamill we tend to not use debug asserts. This seems like a serious enough issue to warrant throwing, even in production.
| metadata: null, | ||
| message: "Failed to connect to GVFS. Skipping post-fetch job request.", | ||
| keywords: Keywords.Telemetry); | ||
| return true; |
There was a problem hiding this comment.
@derrickstolee why is this returning true? We actually failed to prefetch here, correct?
There was a problem hiding this comment.
The comment above says "best effort" Just because we failed to trigger a background midx/graph computation doesn't mean we should fail the prefetch.
There was a problem hiding this comment.
I get that we want it to be best effort, but the code is currently inconsistent. We return true if we fail to connect, but we return false if we fail to get a response. Why do those two code paths behave differently?
Shouldn't this method simply return an honest answer about whether it succeeded or not? The decision of whether to fail the prefetch or not is a business decision that should be driven by PrefetchVerb, e.g. by ignoring the return value.
There was a problem hiding this comment.
@derrickstolee I'm happy to address this, but I'll do it in a separate PR
GVFS/GVFS.Service/GvfsService.cs
Outdated
| // When a version of GVFS that supports the GitStatusCache is installed, it will create | ||
| // the following file. By checking the time the file was created, we know when that | ||
| // version of GVFS was installed. | ||
| string fileToCheck = Path.Combine(Configuration.AssemblyPath, "GitStatusCacheAvailable"); |
There was a problem hiding this comment.
@jamill why is this file name a literal, and the other file above a constant? I would make them consistent.
I also don't really understand the name "GitStatusCacheAvailable". The purpose of this file is to record the install time of GVFS, correct? Why not name it for that behavior? This could end up being useful in future upgrades as well, and I don't want to create a pattern where we have to drop a new file for each feature that might require a reboot.
I think we need to clean up this pattern before releasing, because we will have to preserve this behavior once it gets deployed on customer machines.
GVFS/GVFS.Installer/Setup.iss
Outdated
| var | ||
| TokenFilePath: string; | ||
| begin | ||
| TokenFilePath := ExpandConstant('{app}\GitStatusCacheAvailable'); |
There was a problem hiding this comment.
@jamill @jrbriggs As I mentioned in another comment, this pattern of having the installer write out a file that is specific to a single feature does not seem like a sustainable model. I think all you really want is to write out a file indicating when the upgrade happened, because the question you're trying to answer is: was there a reboot since the last upgrade? That question is not specific to the GitStatusCache.
To be more precise, what the GitStatusCache cares about is: did the last upgrade cross over version X (16 in this case) and has there been a reboot since then?
I think you can do this with an upgrades.dat file (in the format of our FileBasedDictionary) that records the time of each upgrade. The GitStatusCache can then query this file and the system reboot time, find the upgrade date/time for the first upgrade that got to 16 or greater, and make sure there has been a reboot since then. There doesn't seem to be a need for a second token file either.
Thoughts on that alternative approach?
There was a problem hiding this comment.
I am open to changes - I just want to clarify some things that might not be clear from the naming, as I think the current names are overloaded, and that might be causing confusion.
Concepts:
-
When was a version of GVFS that supports the GitStatusCache installed - this is currently recorded by the creation time of
{app}\GitStatusCacheAvailable. -
Is the GitStatusCache feature available (has the system been rebooted since a version of GVFS that supports the GitStatusCache was installed). This is
EnableGitStatusCacheToken.datthat is in the GVFS.Service app data directory.
The installer writes out a file so we can get an accurate time of when a version of GVFS that supports the GitStatusCache was installed (concept 1). That is the purpose of this file - my intent with the naming here was "a file that is created when a version of GVFS that supports GitStatusCache was installed", or "the 1st time a version of GVFS 16 or higher was installed".
The GVFS.Service will query when the a suitable version of GVFS was installed (by reading the time stamp off the file laid down by the installer), and the last time the system was rebooted. If the system has been rebooted since GVFS was installed, then it will write out a file indicating the GitStatusCache is available (i.e.. concept 2).
The GVFS mount process only checks for the existence of the EnableGitStatusCacheToken.dat to make decisions on the availability of the GitStatusCache. We can add functional tests around that GitStatusCache correctly respects the existence of this file with the current approach. Based on previous discussions, the consensus was to keep the logic for comparing the installation time with the reboot time in the GVFS.Service, and having the logic in the mount processes be as simple as possible (i.e. only check for existence of the EnableGitStatusCacheToken.dat file. There was other discussion on whether this logic should be platform aware).
Questions:
In your suggested approach, is upgrades.dat a repository specific file, or is there 1 per GVFS installation? Who would update / maintain this file?
There was a problem hiding this comment.
Thanks, yea another point of confusion that we have is that we often mix up "product upgrades", meaning updating the bits installed on the box, and "repo upgrades", meaning updating the file formats within a single repo.
In my suggestion, I was referring to product upgrades, and so I would rephrase that as: create a single ProductUpgrades.dat file in the service's ProgramData folder. The installer would update this file, and then GVFS.Mount would query it to decide if there has been a reboot since we got to version 16.
@jrbriggs just suggested a simplification of that approach. Instead of maintaining a database of every upgrade, we can just drop a file every time we get a feature that requires a reboot. In this case, the file we drop would be something like OnDiskVersion16CapableInstallation.dat, and its create time would indicate when we got from a pre-16 version to a 16-or-greater version. The benefit of this is that the installer doesn't have to do this work on every single upgrade, but on the off chance that we get this sort of requirement again in the future, we have a generic pattern to use.
With both of these new approaches, there is no need for a second token file. The mount process can just query for "when did we get to a version 16 capable installation" and "when did we reboot" and compare those times.
You can still enable functional testing by simply querying the actual reboot time, and then constructing that version-16 file with an appropriate timestamp on it.
GVFS/GVFS.Installer/Setup.iss
Outdated
| end; | ||
| end; | ||
|
|
||
| procedure WriteGitStatusCacheAvailableFile(); |
There was a problem hiding this comment.
In general, I think this pattern is currently too complex, and unfortunately not something we can add functional tests for because of the need to reboot to trigger the behavior. I would definitely like to see more manual testing to ensure that it is working, and it would be nice to add functional tests that at least cover all of the logic. E.g. query the last reboot time, and then (following my upgrades.dat pattern), set up the last upgrade time to be before the actual reboot time vs after the actual reboot time, and confirm that the GitStatusCache responds correctly.
This is to fix an issue that happened (intermittently) in the functional tests. There is a race condition where the test is attempting to delete the status cache file, but the status cache might not have been generated for the test repository. To fix this, the test setup will wait for the initial status cache to be generated, so it can proceed from a known state. (cherry picked from commit 97975e4)
Tweak the order that components are shutdown, to shutdown the GitStatusCache before other components that it depends on. This is to prevent problems that might arise from a component being disabled that GitStatusCache depends on. (cherry picked from commit 0c09ed4)
This fixes a bug in TryAcquireGVFSLockForProcess where it does not correctly report whether it was able to acquire the GVFS lock. In cases where this method retries to acquire the lock, it would report success, even if it was not able to acquire the lock. This could lead to problems where commands that depended on the GVFS could run, even if it did not actually have the lock. (cherry picked from commit dfbe657)
…etup GitStatusCache: fix race condition in test setup
…g repository Fix lock issue causing problems in unmounting repository
…checks in GetObjectRoot [M139 Port] Fix test reliability issues with checks in GetObjectRoot
…sion Update name of file that GVFS installer creates when an on disk version 16 capable installation is first installed. (cherry picked from commit 120eeba)
…s installation version Update name of file that indicates installation version
5 participants
Release Notes
Performance Improvements
General Improvements