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

Rename PostFetchStep to CommitGraphStep #170

Merged
merged 1 commit into from
Oct 10, 2019
Merged

Rename PostFetchStep to CommitGraphStep #170

merged 1 commit into from
Oct 10, 2019

Conversation

derrickstolee
Copy link
Contributor

@derrickstolee derrickstolee commented Oct 9, 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 added this to the MVP milestone Oct 9, 2019
@derrickstolee
Copy link
Contributor Author

(I'm going to undo the status/mount/unmount change in this PR as they are too early.)

@derrickstolee derrickstolee changed the title [WIP] Rename PostFetchStep to CommitGraphStep, drop some verbs [WIP] Rename PostFetchStep to CommitGraphStep Oct 10, 2019
@derrickstolee derrickstolee changed the title [WIP] Rename PostFetchStep to CommitGraphStep Rename PostFetchStep to CommitGraphStep Oct 10, 2019
@derrickstolee derrickstolee marked this pull request as ready for review October 10, 2019 01:42
@derrickstolee
Copy link
Contributor Author

@wilbaker The pipeline is failing for reasons unrelated to this PR. I have no reason to believe they would not succeed in a second run.

I hope this doesn't collide too much with #172. I believe they will merge pretty cleanly.

@derrickstolee
Copy link
Contributor Author

/azp run microsoft.scalar

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@derrickstolee
Copy link
Contributor Author

/azp run microsoft.scalar

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Approved with suggestion

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee derrickstolee merged commit 9244e87 into microsoft:master Oct 10, 2019
@derrickstolee derrickstolee deleted the commit-graph-step branch November 18, 2019 15:16
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 3, 2020
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional
test fails at present, as it looks for specific output from the
FetchStep maintenance command in the form of an "Unable to delete"
message.  This string is no longer written to standard output by
the command, however, due to the changes in PR microsoft#295 which introduced
a standard progress meter.  Instead, that string is only written
using this.Context.Tracer.RelatedWarning() in the PerformMaintenance()
method of Scalar.Common.Maintenance.FetchStep, and thus appears only
in the maintenance log files.

The breakage of the test was not noticed because it belongs to the
ExtraCoverage category, and therefore has not been running as part
of the CI suite on Windows because the RunFunctionalTests.bat
script does not supply the --full-suite option.

To resolve the test failure we simply check for the continued
presence of the locked bad pack file after the first fetch attempt
instead of parsing for a specific text string.

The test is also Windows-specific because it depends on an open
file handle with a FileShare.None attribute blocking attempts by
other processes to delete the file, and this only works in C# on
the Windows platform.  On Unix platforms, the C# core libraries
implement file sharing in an advisory manner only; see:

https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88

We therefore mark this test as WindowsOnly, since it effectively
does not apply to other platforms.  Note that the previous category
assigned to the test, MacTODO.TestNeedsToLockFile, dates from
VFSForGit and has been migrated to just this one test as a
result of changes in PR microsoft#170.  However, the meaning of that
category varies, as the one other functional test in the category
uses Scalar's FileBasedLock and not C# FileShare settings, so we
clarify the situation by just marking this WindowsOnly.
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 3, 2020
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional
test fails at present, as it looks for specific output from the
FetchStep maintenance command in the form of an "Unable to delete"
message.  This string is no longer written to standard output by
the command, however, due to the changes in PR microsoft#295 which introduced
a standard progress meter.  Instead, that string is only written
using this.Context.Tracer.RelatedWarning() in the PerformMaintenance()
method of Scalar.Common.Maintenance.FetchStep, and thus appears only
in the maintenance log files.

The breakage of the test was not noticed because it belongs to the
ExtraCoverage category, and therefore has not been running as part
of the CI suite on Windows because the RunFunctionalTests.bat
script does not supply the --full-suite option.

To resolve the test failure we simply check for the continued
presence of the locked bad pack file after the first fetch attempt
instead of parsing for a specific text string.

The test is also Windows-specific because it depends on an open
file handle with a FileShare.None attribute blocking attempts by
other processes to delete the file, and this only works in C# on
the Windows platform.  On Unix platforms, the C# core libraries
implement file sharing in an advisory manner only; see:

https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88

We therefore mark this test as WindowsOnly, since it effectively
does not apply to other platforms.  Note that the previous category
assigned to the test, MacTODO.TestNeedsToLockFile, dates from
VFSForGit and has been migrated to just this one test as a
result of changes in PR microsoft#170.  However, the meaning of that
category varies, as the one other functional test in the category
uses Scalar's FileBasedLock and not C# FileShare settings, so we
clarify the situation by just marking this WindowsOnly.
chrisd8088 added a commit to chrisd8088/scalar that referenced this pull request Sep 4, 2020
The FetchStepFailsWhenItCannotRemoveABadPrefetchPack() functional
test fails at present, as it looks for specific output from the
FetchStep maintenance command in the form of an "Unable to delete"
message.  This string is no longer written to standard output by
the command, however, due to the changes in PR microsoft#295 which introduced
a standard progress meter.  Instead, that string is only written
using this.Context.Tracer.RelatedWarning() in the PerformMaintenance()
method of Scalar.Common.Maintenance.FetchStep, and thus appears only
in the maintenance log files.

The breakage of the test was not noticed because it belongs to the
ExtraCoverage category, and therefore has not been running as part
of the CI suite on Windows because the RunFunctionalTests.bat
script does not supply the --full-suite option.

To resolve the test failure we simply check for the continued
presence of the locked bad pack file after the first fetch attempt
instead of parsing for a specific text string.

The test is also Windows-specific because it depends on an open
file handle with a FileShare.None attribute blocking attempts by
other processes to delete the file, and this only works in C# on
the Windows platform.  On Unix platforms, the C# core libraries
implement file sharing in an advisory manner only; see:

https://github.com/dotnet/runtime/blob/341b8b22280e297daa7d96b4b8a9fa1d40d28aa9/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.Unix.cs#L73-L88

We therefore mark this test as WindowsOnly, since it effectively
does not apply to other platforms.  Note that the previous category
assigned to the test, MacTODO.TestNeedsToLockFile, dates from
VFSForGit and has been migrated to just this one test as a
result of changes in PR microsoft#170.  However, the meaning of that
category varies, as the one other functional test in the category
uses Scalar's FileBasedLock and not C# FileShare settings, so we
clarify the situation by just marking this WindowsOnly.
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.

[Mount Removal] Change behavior of PostFetchJob
2 participants