Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion GVFS/GVFS.Build/GVFS.props
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

<PropertyGroup Label="Parameters">
<GVFSVersion>0.2.173.2</GVFSVersion>
<GitPackageVersion>2.20180730.3</GitPackageVersion>
<GitPackageVersion>2.20180814.4</GitPackageVersion>
</PropertyGroup>

<PropertyGroup Label="DefaultSettings">
Expand Down
27 changes: 6 additions & 21 deletions GVFS/GVFS.FunctionalTests/Tests/GitCommands/MergeConflictTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public void MergeConflict_UsingStrategyOurs()
public void MergeConflictEnsureStatusFailsDueToConfig()
{
// This is compared against the message emitted by GVFS.Hooks\Program.cs
string expectedErrorMessagePart = "--no-renames --no-breaks";
string expectedErrorMessagePart = "--no-renames";

this.ValidateGitCommand("checkout " + GitRepoTests.ConflictTargetBranch);
this.RunGitCommand("merge " + GitRepoTests.ConflictSourceBranch, checkStatus: false);
Expand All @@ -95,25 +95,8 @@ public void MergeConflictEnsureStatusFailsDueToConfig()
ProcessResult result2 = GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status --no-renames");
result2.Errors.Contains(expectedErrorMessagePart);

ProcessResult result3 = GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status --no-breaks");
result3.Errors.Contains(expectedErrorMessagePart);

// only renames in config
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local status.renames false");
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status --no-breaks").Errors.ShouldBeEmpty();
ProcessResult result4 = GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status");
result4.Errors.Contains(expectedErrorMessagePart);

// only breaks in config
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local --unset status.renames");
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local status.breaks false");
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status --no-renames").Errors.ShouldBeEmpty();
ProcessResult result5 = GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "status");
result5.Errors.Contains(expectedErrorMessagePart);

// Complete setup to ensure teardown succeeds
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local status.renames false");
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local status.breaks false");
GitHelpers.InvokeGitAgainstGVFSRepo(this.Enlistment.RepoRoot, "config --local test.renames false");
}

protected override void CreateEnlistment()
Expand All @@ -127,8 +110,10 @@ protected override void CreateEnlistment()

private void SetupRenameDetectionAvoidanceInConfig()
{
this.ValidateGitCommand("config --local status.renames false");
this.ValidateGitCommand("config --local status.breaks false");
// Tell the pre-command hook that it shouldn't check for "--no-renames" when runing "git status"
// as the control repo won't do that. When the pre-command hook has been updated to properly
// check for "status.renames" we can set that value here instead.
this.ValidateGitCommand("config --local test.renames false");
}
}
}
1 change: 1 addition & 0 deletions GVFS/GVFS.FunctionalTests/Tools/ControlGitRepo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public void Initialize()
GitProcess.Invoke(this.RootPath, "init");
GitProcess.Invoke(this.RootPath, "config core.autocrlf false");
GitProcess.Invoke(this.RootPath, "config merge.stat false");
GitProcess.Invoke(this.RootPath, "config merge.renames false");
GitProcess.Invoke(this.RootPath, "config advice.statusUoption false");
GitProcess.Invoke(this.RootPath, "config core.abbrev 40");
GitProcess.Invoke(this.RootPath, "config status.aheadbehind false");
Expand Down
27 changes: 10 additions & 17 deletions GVFS/GVFS.Hooks/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -159,35 +159,28 @@ private static void VerifyRenameDetectionSettings(string[] args)
if (File.Exists(Path.Combine(srcRoot, GVFSConstants.DotGit.MergeHead)) ||
File.Exists(Path.Combine(srcRoot, GVFSConstants.DotGit.RevertHead)))
{
// If no-renames and no-breaks are specified, avoid reading config.
if (!args.Contains("--no-renames") || !args.Contains("--no-breaks"))
// If no-renames is specified, avoid reading config.
if (!args.Contains("--no-renames"))
{
// To behave properly, this needs to check for the status.renames setting the same
// way that git does including global and local config files, setting inheritance from
// diff.renames, etc. This is probably best accomplished by calling "git config --get status.renames"
// to ensure we are getting the correct value and then checking for "true" (rather than
// just existance like below).
Dictionary<string, GitConfigSetting> statusConfig = GitConfigHelper.GetSettings(
File.ReadAllLines(Path.Combine(srcRoot, GVFSConstants.DotGit.Config)),
"status");
"test");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between test.renames and merge.renames?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benpeart can elaborate but this is so the tests will pass when someone has status.renames=true in their global config because our code is not following proper git config hierarchy

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check just for our tests? Or is it needed for customer machines as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so that the test cases can pass without enabling someone to bypass the "--no-renames" test in the pre-command hook by setting status.renames=true in their global config.

If I leave this as "status" then if someone set status.renames=true in their global config (an entirely valid setting) we would allow the command to go through instead of blocking it and requiring --no-renames. I only left this in as "test" so that I can make the test cases pass until we get a proper fix that checks all config files, deals with inheritance from diff.renames, etc.

Details on the various config settings, and how they are inherited/overwritten by other settings and command line options are in the patch to git (commit e8b2dc2c)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the details!


if (!IsRunningWithParamOrSetting(args, statusConfig, "--no-renames", "renames") ||
!IsRunningWithParamOrSetting(args, statusConfig, "--no-breaks", "breaks"))
if (!statusConfig.ContainsKey("renames"))
{
ExitWithError(
"git status requires rename detection to be disabled during a merge or revert conflict.",
"Run 'git status --no-renames --no-breaks'");
"Run 'git status --no-renames'");
}
}
}
}

private static bool IsRunningWithParamOrSetting(
string[] args,
Dictionary<string, GitConfigSetting> configSettings,
string expectedArg,
string expectedSetting)
{
return
args.Contains(expectedArg) ||
configSettings.ContainsKey(expectedSetting);
}

private static void RunLockRequest(string[] args, bool unattended, LockRequestDelegate requestToRun)
{
try
Expand Down
1 change: 1 addition & 0 deletions GVFS/GVFS/CommandLine/GVFSVerb.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ public static bool TrySetRequiredGitConfigSettings(Enlistment enlistment)
{ "gui.gcwarning", "false" },
{ "index.version", "4" },
{ "merge.stat", "false" },
{ "merge.renames", "false" },
{ "receive.autogc", "false" },
{ "status.deserializePath", gitStatusCachePath },
};
Expand Down