-
Notifications
You must be signed in to change notification settings - Fork 451
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
stop blocking unsupported git commands #597
Conversation
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.
You need to update the git version in GVFS.props
. Did we have any functional tests for these commands? Might be good to add some if we don't.
Could you also include a bit more detail on how someone could find out more about the corresponding Git changes? I think a link to the git commit with the changes (or at least the initial git commit with these changes) would help someone who wants to learn more about this. Also, as Kevin mentioned, do we need to update the associated version of Git to one that includes these changes? or do we already have them? I don't think it would be much work to include functional tests here as well. |
Please update GVFS.props to 2.20181213.2 (when this build completes.) |
Please mention that your Git update includes two fixes: |
Now that git has learned to block unsupported commands on VFS4G repos, we can remove that logic from the pre-command hook. Signed-off-by: Ben Peart <benpeart@microsoft.com>
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.
Approved with some suggestions.
this.fileSystem = fileSystem; | ||
} | ||
|
||
[TestCase, Order(1)] |
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.
Does order really matter in these tests?
@@ -1036,10 +1036,10 @@ public void RenameOnlyFileInFolder() | |||
public void UpdateIndexCannotModifySkipWorktreeBit() |
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.
This seems like a duplicate test to the ones you added? If so I would just delete it.
ProcessResult result = GitHelpers.InvokeGitAgainstGVFSRepo( | ||
this.Enlistment.RepoRoot, | ||
"fsck"); | ||
result.ExitCode.ShouldNotEqual(0, result.Errors); |
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.
I would maybe add a function that is:
private void CommandBlocked(string command)
{
ProcessResult result = GitHelpers.InvokeGitAgainstGVFSRepo(
this.Enlistment.RepoRoot,
command);
result.ExitCode.ShouldNotEqual(0, $"Command {command} not blocked when it should be. Errors: {result.Errors}");
}
And have a similar method for CommandNotBlocked
Then you could have one test case that is:
CommandBlocked("fsck");
CommandBlocked("gc");
CommandNotBlocked("gc --auto");
CommandBlocked("prune");
etc...
|
||
namespace GVFS.FunctionalTests.Tests.EnlistmentPerFixture | ||
{ | ||
[TestFixtureSource(typeof(FileSystemRunner), nameof(FileSystemRunner.Runners))] |
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.
There is no need to parameterize these tests on FileSystemRunner
as they never use this.fileSystem
.
[TestFixtureSource(typeof(FileSystemRunner), nameof(FileSystemRunner.Runners))]
can be replaced with [TestFixture]
and the FileSystemRunner fileSystem
parameter can be removed from the constructor.
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.
@benpeart I see I was too late with this comment, I will create a PR with this change.
Now that git has learned to block unsupported commands on VFS4G repos, we can remove that logic from the pre-command hook.
The corresponding changes to git can be found in microsoft/git@cfe88be
The version of git specified includes two fixes:
Signed-off-by: Ben Peart benpeart@microsoft.com