diff --git a/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs b/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs index b8362179c7..083ff033fc 100644 --- a/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs +++ b/GVFS/GVFS.FunctionalTests.LockHolder/Program.cs @@ -8,6 +8,12 @@ public class Program { public static void Main(string[] args) { + if (args.Length > 0 && args[0] == "--echo-arguments") + { + Console.Write(string.Join("\0", args, 1, args.Length - 1)); + return; + } + if (args.Length > 0 && args[0] == "--wait-for-event") { string eventName = Environment.GetEnvironmentVariable("GVFS_TEST_HOOK_WAIT_EVENT_NAME"); diff --git a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs index 0427576b70..815efe1216 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/GitReadAndGitLockTests.cs @@ -110,6 +110,21 @@ public void GitCommandWaitsWhileAnotherIsRunning(string command) [TestCase("count-objects -vH")] [TestCase("ls-remote --get-url origin")] [TestCase("range-diff HEAD~1..HEAD HEAD~1..HEAD")] + [TestCase("reflog")] + [TestCase("reflog show --all")] + [TestCase("reflog list")] + [TestCase("reflog exists HEAD")] + [TestCase("describe --always HEAD")] + [TestCase("grep -n -e . HEAD -- Readme.md")] + [TestCase("grep --cached -e . -- Readme.md")] + [TestCase("refs list --count=1")] + [TestCase("merge-file -p .git/HEAD .git/HEAD .git/HEAD")] + [TestCase("merge-file --object-id HEAD:Readme.md HEAD:Readme.md HEAD:Readme.md")] + [TestCase("merge-tree --write-tree HEAD HEAD")] + [TestCase("notes list")] + [TestCase("replace -l")] + [TestCase("format-patch --stdout -1 HEAD")] + [TestCase("var GIT_AUTHOR_IDENT")] [TestCase("shortlog -sn HEAD")] [TestCase("show-branch --list")] public void ReadOnlyCommandDoesNotWaitForLock(string command) @@ -128,6 +143,188 @@ public void ReadOnlyCommandDoesNotWaitForLock(string command) }); } + [TestCase("describe --dirty --always")] + [TestCase("describe --broken --always")] + [TestCase("describe --di --always")] + [TestCase("grep -nOcat -e . -- Readme.md")] + [TestCase("grep --open=cat -e . -- Readme.md")] + [TestCase("refs migrate -h")] + public void UnsafeCommandModeStillWaitsForLock(string command) + { + File.ReadAllText(this.Enlistment.GetVirtualPathTo("Readme.md")); + ProcessResult result = this.RunWithLockHolder(command); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase(false)] + [TestCase(true)] + public void MergeFileOnlyBypassesLockWhenOutputModeRemainsEnabled(bool disableStdout) + { + string inputPath = Path.Combine(this.Enlistment.EnlistmentRoot, "merge-file-" + Guid.NewGuid().ToString("N")); + File.WriteAllText(inputPath, "merge input\n"); + try + { + string command = $"merge-file --stdout {(disableStdout ? "--no-stdout" : string.Empty)} \"{inputPath}\" \"{inputPath}\" \"{inputPath}\""; + ProcessResult result = this.RunWithLockHolder(command, shouldWait: disableStdout); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.Contains(ExpectedStatusWaitingText).ShouldEqual(disableStdout); + File.ReadAllText(inputPath).ShouldEqual("merge input\n"); + } + finally + { + File.Delete(inputPath); + } + } + + [TestCase] + public void ReflogWritesPreserveIndexAndTrackedContentUnderLockContention() + { + string refName = "refs/heads/gvfs-reflog-test-" + Guid.NewGuid().ToString("N"); + string filePath = this.Enlistment.GetVirtualPathTo("Readme.md"); + string indexPath = this.Enlistment.GetDotGitPath("index"); + string originalContent = File.ReadAllText(filePath); + string oldObject = GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse HEAD~1").Trim(); + string newObject = GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse HEAD").Trim(); + try + { + File.WriteAllText(filePath, originalContent + "\nstaged reflog test\n"); + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "add -- Readme.md"); + string dirtyContent = originalContent + "\nunstaged reflog test\n"; + File.WriteAllText(filePath, dirtyContent); + GitHelpers.CheckGitCommandAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"update-ref --create-reflog -m first {refName} {oldObject}"); + GitHelpers.CheckGitCommandAgainstGVFSRepo( + this.Enlistment.RepoRoot, + $"update-ref -m second {refName} {newObject}"); + + GitHelpers.RunWithGVFSLock( + this.Enlistment, + () => + { + byte[] index = File.ReadAllBytes(indexPath); + DateTime indexWriteTime = File.GetLastWriteTimeUtc(indexPath); + (string Command, string ExpectedRef, int ReflogEntries)[] commands = + { + ($"reflog write {refName} {newObject} {newObject} appended", newObject, 3), + ($"reflog delete --rewrite --updateref {refName}@{{0}}", newObject, 2), + ($"reflog delete --rewrite --updateref {refName}@{{0}}", oldObject, 1), + ($"reflog expire --stale-fix --rewrite --updateref --expire=never --expire-unreachable=never {refName}", oldObject, 1), + ($"reflog expire --stale-fix --rewrite --updateref --expire=all {refName}", oldObject, 0), + ($"reflog drop {refName}", oldObject, -1), + }; + + foreach ((string command, string expectedRef, int reflogEntries) in commands) + { + ProcessResult result = GitProcess.InvokeProcess( + this.Enlistment.RepoRoot, + command, + timeoutMs: AcquireGVFSLockTimeout); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldNotContain(false, ExpectedStatusWaitingText); + GitProcess.Invoke(this.Enlistment.RepoRoot, "rev-parse " + refName).Trim().ShouldEqual(expectedRef); + if (reflogEntries < 0) + { + GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "reflog exists " + refName).ExitCode.ShouldEqual(1); + } + else + { + ProcessResult reflog = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "reflog show --format=%H " + refName); + reflog.ExitCode.ShouldEqual(0, reflog.Errors); + reflog.Output.Split(new[] { '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries).Length.ShouldEqual(reflogEntries); + } + + File.ReadAllBytes(indexPath).ShouldMatchInOrder(index); + File.GetLastWriteTimeUtc(indexPath).ShouldEqual(indexWriteTime); + File.ReadAllText(filePath).ShouldEqual(dirtyContent); + } + }); + } + finally + { + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "reset --hard"); + GitHelpers.CheckGitCommandAgainstGVFSRepo(this.Enlistment.RepoRoot, "update-ref -d " + refName); + } + } + + [TestCase("")] + [TestCase("with space")] + [TestCase("with\ttab")] + [TestCase("with\"quote")] + [TestCase("trailing space slash\\")] + [TestCase("before\\\\\"quote")] + public void HookLoaderPreservesArgumentBoundaries(string argument) + { + string loaderPath = this.Enlistment.GetDotGitPath("hooks", "pre-command.exe"); + string hookListPath = this.Enlistment.GetDotGitPath("hooks", "pre-command.hooks"); + string originalHookList = File.ReadAllText(hookListPath); + try + { + File.WriteAllText(hookListPath, $"\"{GitHelpers.LockHolderCommandPath}\" --echo-arguments"); + ProcessStartInfo loader = new ProcessStartInfo(loaderPath) + { + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + loader.ArgumentList.Add("first"); + loader.ArgumentList.Add(argument); + loader.ArgumentList.Add("last"); + loader.Environment.Remove("GITHOOKSLOADER_PERFTRACE"); + ProcessResult result = ProcessHelper.Run(loader, timeoutMs: AcquireGVFSLockTimeout); + result.ExitCode.ShouldEqual(0, result.Errors); + result.Output.Split('\0').ShouldMatchInOrder("pre-command", "first", argument, "last"); + } + finally + { + File.WriteAllText(hookListPath, originalHookList); + } + } + + [TestCase("rm -f")] + [TestCase("foo --")] + public void GrepOptionValuesCannotHidePagerDispatch(string pattern) + { + File.ReadAllText(this.Enlistment.GetVirtualPathTo("Readme.md")); + string[] arguments = { "grep", "-e", pattern, "-Ocat", "-e", ".", "--", "Readme.md" }; + ProcessResult result = this.RunWithLockHolder(string.Empty, argumentList: arguments); + // Git grep returns 1 when no line matches. + Assert.That(result.ExitCode, Is.InRange(0, 1), result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("v1* --")] + [TestCase("v\"")] + public void DescribeOptionValuesCannotHideIndexRefresh(string pattern) + { + string[] arguments = { "describe", "--match", pattern, "--dirty", "--always" }; + ProcessResult result = this.RunWithLockHolder(string.Empty, argumentList: arguments); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + } + + [TestCase("ours -p")] + [TestCase("ours\t-p")] + [TestCase("ours\"")] + public void MergeFileLabelsCannotEnableAnOutputMode(string label) + { + string inputPath = Path.Combine(this.Enlistment.EnlistmentRoot, "merge-label-" + Guid.NewGuid().ToString("N")); + File.WriteAllText(inputPath, "merge input\n"); + try + { + string[] arguments = { "merge-file", "-L", label, inputPath, inputPath, inputPath }; + ProcessResult result = this.RunWithLockHolder(string.Empty, argumentList: arguments); + result.ExitCode.ShouldEqual(0, result.Output + result.Errors); + result.Errors.ShouldContain(ExpectedStatusWaitingText); + File.ReadAllText(inputPath).ShouldEqual("merge input\n"); + } + finally + { + File.Delete(inputPath); + } + } + [TestCase, Order(5)] public void GitAliasNamedAfterKnownCommandAcquiresLock() { @@ -567,7 +764,11 @@ private ProcessStartInfo CreateGitStartInfo(string command, Dictionary environment = null, bool shouldWait = true) + private ProcessResult RunWithLockHolder( + string command, + Dictionary environment = null, + bool shouldWait = true, + string[] argumentList = null) { using (Process holder = new Process()) using (Process git = new Process()) @@ -593,6 +794,14 @@ private ProcessResult RunWithLockHolder(string command, Dictionary { if (args.Data != null) diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs index 01b9ce3517..802966dee6 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CreatePlaceholderTests.cs @@ -41,6 +41,7 @@ public void AllowsPlaceholderCreationForDefaultAnnotate() [TestCase("hash-object --stdin")] [TestCase("index-pack --stdin")] [TestCase("name-rev --stdin")] + [TestCase("notes copy --stdin")] [TestCase("rev-list --stdin --quiet --all")] [TestCase("update-ref --stdin")] public void AllowsPlaceholderCreationWhileGitCommandIsRunning(string commandToRun) @@ -50,7 +51,6 @@ public void AllowsPlaceholderCreationWhileGitCommandIsRunning(string commandToRu [TestCase("checkout-index --stdin")] [TestCase("fetch-pack --stdin URL")] - [TestCase("notes copy --stdin")] [TestCase("reset --stdin")] [TestCase("send-pack --stdin URL")] [TestCase("update-index --stdin")] @@ -62,11 +62,16 @@ public void BlocksPlaceholderCreationWhileGitCommandIsRunning(string commandToRu [TestCase("worktree list")] [TestCase("worktree list --porcelain")] [TestCase("worktree list --porcelain -z")] - public void AllowsPlaceholderCreationWhileWorktreeListIsRunning(string command) + [TestCase("reflog")] + [TestCase("describe --always HEAD")] + [TestCase("grep -q -e . HEAD -- Readme.md")] + [TestCase("refs list --count=1")] + [TestCase("merge-file --object-id HEAD:Readme.md HEAD:Readme.md HEAD:Readme.md")] + public void AllowsPlaceholderCreationWhileExemptCommandIsRunning(string command) { string hookListPath = this.Enlistment.GetVirtualPathTo(".git", "hooks", "post-command.hooks"); string originalHookList = File.ReadAllText(hookListPath); - string eventName = "GVFS.WorktreeList." + Guid.NewGuid().ToString("N"); + string eventName = "GVFS.ExemptCommand." + Guid.NewGuid().ToString("N"); string expectedContent = File.ReadAllText(Path.Combine(this.ControlGitRepo.RootPath, FileToRead)); using (EventWaitHandle ready = new EventWaitHandle(false, EventResetMode.ManualReset, eventName + ".Ready")) diff --git a/GVFS/GVFS.Hooks/Program.CommandLocking.cs b/GVFS/GVFS.Hooks/Program.CommandLocking.cs index b016a7d9ea..5096bea247 100644 --- a/GVFS/GVFS.Hooks/Program.CommandLocking.cs +++ b/GVFS/GVFS.Hooks/Program.CommandLocking.cs @@ -8,10 +8,12 @@ public partial class Program { // Hooks run on the git command path, so config reads retry at most once. private const int ConfigReadMaxAttempts = 2; + private const string GitPidArg = "--git-pid="; // The lock serializes changes to the enlistment's index and tracked working tree with GVFS projection updates. // Before adding an exemption, check all supported options, subcommands, and child-command paths. - // Ref and object writes alone do not require this lock. Index refreshes do. + // Ref and object writes alone do not require this lock. + // Keep index refresh modes locked. RequiredGitConfig enables opportunistic stat refreshes for diff and difftool. // Keep arbitrary Git dispatchers locked because nested command hooks can be suppressed. // Excluded readers can observe concurrent working-tree changes. They do not receive snapshot isolation. internal static bool TryShouldLock( @@ -55,47 +57,70 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) case "annotate": case "blame": case "branch": + case "bundle": case "cat-file": case "check-attr": case "check-ignore": case "check-mailmap": case "check-ref-format": case "cherry": + case "column": case "commit-graph": + case "commit-tree": case "config": case "count-objects": case "credential": case "diff": case "diff-files": case "diff-index": + case "diff-pairs": case "diff-tree": case "difftool": case "fetch": + case "fmt-merge-msg": case "for-each-ref": + case "format-patch": + case "format-rev": + case "get-tar-commit-id": case "help": case "hash-object": case "index-pack": + case "last-modified": case "log": case "ls-files": case "ls-remote": case "ls-tree": case "merge-base": + case "merge-tree": + case "mktag": + case "mktree": case "multi-pack-index": case "name-rev": + case "notes": case "pack-objects": + case "pack-refs": + case "patch-id": + case "pickaxe": case "push": case "range-diff": + case "reflog": case "remote": + case "replace": case "rev-list": case "rev-parse": case "shortlog": case "show": case "show-branch": + case "show-index": case "show-ref": + case "stripspace": case "symbolic-ref": case "tag": case "unpack-objects": case "update-ref": + case "update-server-info": + case "url-parse": + case "var": case "verify-commit": case "verify-pack": case "verify-tag": @@ -103,6 +128,31 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) case "web--browse": return false; + case "describe": + return !HasOnlySafeOptions(args, GetDescribeOptionType, out _); + + case "grep": + return !HasOnlySafeOptions(args, GetGrepOptionType, out _); + + case "merge-file": + return !HasOnlySafeOptions(args, GetMergeFileOptionType, out bool hasSafeOutputMode) || !hasSafeOutputMode; + + case "refs": + switch (args.Length > 2 ? args[2] : null) + { + case "list": + case "exists": + case "verify": + case "optimize": + case "create": + case "delete": + case "update": + case "rename": + return false; + } + + return true; + case "worktree": return WorktreeCommandParser.GetSubcommand(args) != "list"; @@ -127,6 +177,219 @@ private static bool ShouldLockBuiltIn(string gitCommand, string[] args) return true; } + private static bool HasOnlySafeOptions(string[] args, Func getOptionType, out bool hasSafeOutputMode) + { + hasSafeOutputMode = false; + int end = args.Length; + + // Only the appended hook metadata is excluded. User arguments with these names remain unknown options. + if (args[0] == "post-command") + { + if (end <= 2 || !args[end - 1].StartsWith("--exit_code=", StringComparison.Ordinal)) + { + return false; + } + + --end; + } + + if (end <= 2 || !args[end - 1].StartsWith(GitPidArg, StringComparison.Ordinal)) + { + return false; + } + + --end; + for (int i = 2; i < end; ++i) + { + string argument = args[i]; + if (argument == "--") + { + return true; + } + + if (!argument.StartsWith("-", StringComparison.Ordinal) || argument == "-") + { + continue; + } + + if (argument.StartsWith("--", StringComparison.Ordinal)) + { + int equals = argument.IndexOf('='); + string option = equals < 0 ? argument : argument.Substring(0, equals); + CommandOptionType type = getOptionType(option); + if (type == CommandOptionType.Unknown || + (equals >= 0 && type != CommandOptionType.Value && type != CommandOptionType.OptionalValue)) + { + return false; + } + + hasSafeOutputMode |= type == CommandOptionType.SafeOutputMode; + if (type == CommandOptionType.Value && equals < 0 && ++i >= end) + { + return false; + } + } + else + { + for (int j = 1; j < argument.Length; ++j) + { + CommandOptionType type = getOptionType("-" + argument[j]); + if (type == CommandOptionType.Unknown) + { + return false; + } + + hasSafeOutputMode |= type == CommandOptionType.SafeOutputMode; + if (type == CommandOptionType.Value) + { + if (j == argument.Length - 1 && ++i >= end) + { + return false; + } + + break; + } + } + } + } + + return true; + } + + private static CommandOptionType GetDescribeOptionType(string option) + { + switch (option) + { + case "--contains": + case "--debug": + case "--all": + case "--tags": + case "--long": + case "--first-parent": + case "--exact-match": + case "--always": + return CommandOptionType.Flag; + + case "--candidates": + case "--match": + case "--exclude": + return CommandOptionType.Value; + + case "--abbrev": + return CommandOptionType.OptionalValue; + } + + return CommandOptionType.Unknown; + } + + private static CommandOptionType GetGrepOptionType(string option) + { + switch (option) + { + case "-v": + case "-i": + case "-w": + case "-a": + case "-I": + case "-r": + case "-E": + case "-G": + case "-F": + case "-P": + case "-n": + case "-h": + case "-H": + case "-l": + case "-L": + case "-z": + case "-o": + case "-c": + case "-p": + case "-W": + case "-q": + case "--cached": + case "--no-index": + case "--untracked": + case "--exclude-standard": + case "--recurse-submodules": + case "--invert-match": + case "--ignore-case": + case "--word-regexp": + case "--text": + case "--textconv": + case "--recursive": + case "--extended-regexp": + case "--basic-regexp": + case "--fixed-strings": + case "--perl-regexp": + case "--line-number": + case "--column": + case "--full-name": + case "--files-with-matches": + case "--name-only": + case "--files-without-match": + case "--null": + case "--only-matching": + case "--count": + case "--break": + case "--heading": + case "--show-function": + case "--function-context": + case "--and": + case "--or": + case "--not": + case "--quiet": + case "--all-match": + return CommandOptionType.Flag; + + case "-e": + case "-f": + case "-A": + case "-B": + case "-C": + case "-m": + case "--max-depth": + case "--context": + case "--before-context": + case "--after-context": + case "--threads": + case "--max-count": + return CommandOptionType.Value; + + case "--color": + return CommandOptionType.OptionalValue; + } + + return CommandOptionType.Unknown; + } + + private static CommandOptionType GetMergeFileOptionType(string option) + { + switch (option) + { + case "-p": + case "--stdout": + case "--object-id": + return CommandOptionType.SafeOutputMode; + + case "-q": + case "--quiet": + case "--diff3": + case "--zdiff3": + case "--ours": + case "--theirs": + case "--union": + return CommandOptionType.Flag; + + case "-L": + case "--diff-algorithm": + case "--marker-size": + return CommandOptionType.Value; + } + + return CommandOptionType.Unknown; + } + private static string GetGitCommand(string[] args) { string command = args[1].ToLowerInvariant(); @@ -137,5 +400,14 @@ private static string GetGitCommand(string[] args) return command; } + + private enum CommandOptionType + { + Unknown, + Flag, + Value, + OptionalValue, + SafeOutputMode, + } } } diff --git a/GVFS/GVFS.Hooks/Program.cs b/GVFS/GVFS.Hooks/Program.cs index 600ee823a7..4c51a0c63c 100644 --- a/GVFS/GVFS.Hooks/Program.cs +++ b/GVFS/GVFS.Hooks/Program.cs @@ -17,7 +17,6 @@ public partial class Program private const string PreCommandHook = "pre-command"; private const string PostCommandHook = "post-command"; - private const string GitPidArg = "--git-pid="; private const int InvalidProcessId = -1; private const int PostCommandSpinnerDelayMs = 500; diff --git a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs index 433ee028e6..40ab5c07f6 100644 --- a/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs +++ b/GVFS/GVFS.UnitTests/Hooks/CommandLockingTests.cs @@ -13,6 +13,7 @@ public class CommandLockingTests [TestCase("annotate --contents mock-file HEAD")] [TestCase("blame")] [TestCase("branch")] + [TestCase("bundle create mock.bundle HEAD")] [TestCase("cat-file")] [TestCase("check-attr")] [TestCase("check-ignore")] @@ -21,33 +22,89 @@ public class CommandLockingTests [TestCase("check-ref-format --normalize refs/heads/topic")] [TestCase("check-ref-format --refspec-pattern refs/heads/*")] [TestCase("cherry -v HEAD~2 HEAD HEAD~1")] + [TestCase("column --mode=column")] [TestCase("commit-graph")] + [TestCase("commit-tree HEAD^{tree}")] [TestCase("config")] [TestCase("count-objects")] [TestCase("count-objects -vH")] [TestCase("credential")] + [TestCase("describe")] + [TestCase("describe --tags --always --abbrev=7 HEAD")] + [TestCase("describe --abbrev")] + [TestCase("describe --abbrev 7")] + [TestCase("describe --contains --all --long --first-parent --exact-match")] + [TestCase("describe --match mock-* --exclude=excluded-* --candidates 5")] + [TestCase("describe --match --dirty")] + [TestCase("describe -- --dirty")] [TestCase("diff")] [TestCase("diff-files")] [TestCase("diff-index")] + [TestCase("diff-pairs")] [TestCase("diff-tree")] [TestCase("difftool")] [TestCase("fetch")] + [TestCase("fmt-merge-msg")] [TestCase("for-each-ref")] + [TestCase("format-patch --stdout --base=auto HEAD~1")] + [TestCase("format-rev HEAD")] + [TestCase("get-tar-commit-id")] + [TestCase("grep pattern")] + [TestCase("grep -in -e pattern -- Readme.md")] + [TestCase("grep -nepattern")] + [TestCase("grep -e -O -f patterns --cached")] + [TestCase("grep -e-O")] + [TestCase("grep --context=2 --threads 1 --color=never pattern")] + [TestCase("grep --color pattern")] + [TestCase("grep -- --open-files-in-pager")] + [TestCase("grep -- -O")] [TestCase("help")] [TestCase("hash-object")] [TestCase("index-pack")] + [TestCase("last-modified HEAD -- Readme.md")] [TestCase("log")] [TestCase("ls-files")] [TestCase("ls-remote --symref origin")] [TestCase("ls-remote --get-url origin")] [TestCase("ls-tree")] [TestCase("merge-base")] + [TestCase("merge-file -p ours base theirs")] + [TestCase("merge-file --stdout --diff3 -L ours ours base theirs")] + [TestCase("merge-file -qpLlabel ours base theirs")] + [TestCase("merge-file -p -L --no-stdout ours base theirs")] + [TestCase("merge-file --object-id HEAD:a HEAD:b HEAD:c")] + [TestCase("merge-file --object-id --marker-size=7 --diff-algorithm histogram a b c")] + [TestCase("merge-file --stdout -- ours base --no-stdout")] + [TestCase("merge-tree --write-tree HEAD HEAD")] + [TestCase("mktag")] + [TestCase("mktree")] [TestCase("multi-pack-index")] [TestCase("name-rev")] + [TestCase("notes merge --commit")] [TestCase("pack-objects")] + [TestCase("pack-refs --all")] + [TestCase("patch-id --stable")] + [TestCase("pickaxe HEAD -- Readme.md")] [TestCase("push")] [TestCase("range-diff HEAD~2..HEAD~1 HEAD~1..HEAD")] + [TestCase("reflog")] + [TestCase("reflog show --all")] + [TestCase("reflog list")] + [TestCase("reflog exists HEAD")] + [TestCase("reflog write refs/heads/topic old new message")] + [TestCase("reflog delete --rewrite --updateref topic@{0}")] + [TestCase("reflog drop --all --single-worktree")] + [TestCase("reflog expire --stale-fix --rewrite --updateref --all")] + [TestCase("refs list")] + [TestCase("refs exists refs/heads/topic")] + [TestCase("refs verify --strict")] + [TestCase("refs optimize --all")] + [TestCase("refs create refs/heads/topic HEAD")] + [TestCase("refs delete refs/heads/topic")] + [TestCase("refs update refs/heads/topic HEAD")] + [TestCase("refs rename refs/heads/topic refs/heads/new-topic")] [TestCase("remote")] + [TestCase("replace --edit HEAD")] [TestCase("reset --soft")] [TestCase("rev-list")] [TestCase("rev-parse")] @@ -56,13 +113,18 @@ public class CommandLockingTests [TestCase("show-branch --list")] [TestCase("show-branch --merge-base")] [TestCase("show-branch --reflog")] + [TestCase("show-index")] [TestCase("show-ref")] + [TestCase("stripspace")] [TestCase("submodule")] [TestCase("git-submodule")] [TestCase("symbolic-ref")] [TestCase("tag")] [TestCase("unpack-objects")] [TestCase("update-ref")] + [TestCase("update-server-info")] + [TestCase("url-parse https://example.invalid/repo")] + [TestCase("var GIT_EDITOR")] [TestCase("verify-commit --raw -v HEAD")] [TestCase("verify-pack -v mock-pack.idx")] [TestCase("verify-pack -s mock-pack.idx")] @@ -89,15 +151,49 @@ public void CommandsWithoutLockPreserveTheirPolicy(string command) [TestCase("cherry-pick")] [TestCase("clean")] [TestCase("commit")] - [TestCase("describe")] [TestCase("describe --dirty")] + [TestCase("describe --dirty=modified")] + [TestCase("describe --abbrev --dirty")] + [TestCase("describe --di")] + [TestCase("describe --dir=modified")] [TestCase("describe --broken")] + [TestCase("describe --br=broken")] + [TestCase("describe --tags --match mock-* --broken")] + [TestCase("describe --unknown-option")] + [TestCase("describe --match")] + [TestCase("describe --git-pid=123")] + [TestCase("grep -O pattern")] + [TestCase("grep --color -O pattern")] + [TestCase("grep --cached=value pattern")] + [TestCase("grep -Omock-pager pattern")] + [TestCase("grep -inO pattern")] + [TestCase("grep -nOmock-pager pattern")] + [TestCase("grep --open pattern")] [TestCase("grep --open-files-in-pager=mock-pager pattern")] + [TestCase("grep -e pattern --open-files-in-pager")] + [TestCase("grep --unknown-option pattern")] + [TestCase("grep -e")] + [TestCase("grep --threads")] + [TestCase("grep --exit_code=0 pattern")] [TestCase("merge")] + [TestCase("merge-file ours base theirs")] + [TestCase("merge-file -- -p base theirs")] + [TestCase("merge-file -L -p ours base theirs")] + [TestCase("merge-file -p --no-stdout ours base theirs")] + [TestCase("merge-file --object-id --no-object-id ours base theirs")] + [TestCase("merge-file -p --no-st ours base theirs")] + [TestCase("merge-file --obj ours base theirs")] + [TestCase("merge-file -p --unknown-option ours base theirs")] + [TestCase("merge-file -p -L")] [TestCase("mv")] [TestCase("pull")] [TestCase("read-tree")] [TestCase("rebase")] + [TestCase("refs")] + [TestCase("refs migrate --ref-format=reftable")] + [TestCase("refs unknown list")] + [TestCase("refs --unknown-option list")] + [TestCase("refs LIST")] [TestCase("reset")] [TestCase("reset --mixed")] [TestCase("reset --hard")] @@ -123,9 +219,26 @@ public void CommandsWithLockPreserveTheirPolicy(string command) AssertLockPolicy(command, shouldLock: true); } + [TestCase("describe")] + [TestCase("describe --match value")] + [TestCase("grep pattern")] + [TestCase("grep --color pattern")] + [TestCase("merge-file -p a b c")] + [TestCase("merge-file --object-id a b c")] + public void PartialExemptionsRequireSeparateHookMetadata(string command) + { + AssertLockPolicy(command, shouldLock: true, includeHookMetadata: false); + } + [TestCase("git-annotate")] [TestCase("git-DIFF")] [TestCase("git-LOG")] + [TestCase("git-reflog")] + [TestCase("git-merge-tree")] + [TestCase("git-describe")] + [TestCase("git-grep pattern")] + [TestCase("git-merge-file -p ours base theirs")] + [TestCase("git-refs list")] [TestCase("git-worktree list")] [TestCase("git-worktree add list")] [TestCase("git-reset --soft")] @@ -194,7 +307,7 @@ private static string[] CreatePreCommandArgs(string command) return new[] { "pre-command" }.Concat(command.Split(' ')).Concat(new[] { "--git-pid=456" }).ToArray(); } - private static void AssertLockPolicy(string command, bool shouldLock) + private static void AssertLockPolicy(string command, bool shouldLock, bool includeHookMetadata = true) { // The hook sends post-command for git- to the lock-ownership check, not to this policy. string[] hooks = command.StartsWith("git-", System.StringComparison.Ordinal) @@ -204,7 +317,9 @@ private static void AssertLockPolicy(string command, bool shouldLock) { string[] args = new[] { hook } .Concat(command.Split(' ')) - .Concat(hook == "post-command" + .Concat(!includeHookMetadata + ? System.Array.Empty() + : hook == "post-command" ? new[] { "--git-pid=456", "--exit_code=0" } : new[] { "--git-pid=456" }) .ToArray(); diff --git a/GVFS/GitHooksLoader/GitHooksLoader.cpp b/GVFS/GitHooksLoader/GitHooksLoader.cpp index 9f1619d0a6..e9ddc6ed81 100644 --- a/GVFS/GitHooksLoader/GitHooksLoader.cpp +++ b/GVFS/GitHooksLoader/GitHooksLoader.cpp @@ -2,6 +2,7 @@ // #include "stdafx.h" +#include #include #include @@ -91,6 +92,36 @@ int wmain(int argc, WCHAR *argv[]) return 0; } +static void AppendCommandLineArgument(std::wstring &commandLine, const wchar_t *argument) +{ + commandLine += L' '; + if (*argument != L'\0' && std::wcspbrk(argument, L" \t\"") == nullptr) + { + commandLine += argument; + return; + } + + commandLine += L'"'; + size_t backslashes = 0; + for (; *argument != L'\0'; ++argument) + { + if (*argument == L'\\') + { + ++backslashes; + } + else + { + // Double backslashes before quotes, then escape the literal quote. + commandLine.append(*argument == L'"' ? backslashes * 2 + 1 : backslashes, L'\\'); + commandLine += *argument; + backslashes = 0; + } + } + + commandLine.append(backslashes * 2, L'\\'); + commandLine += L'"'; +} + int ExecuteHook(const std::wstring &applicationName, wchar_t *hookName, int argc, WCHAR *argv[]) { wchar_t expandedPath[MAX_PATH + 1]; @@ -101,10 +132,11 @@ int ExecuteHook(const std::wstring &applicationName, wchar_t *hookName, int argc exit(6); } - std::wstring commandLine = std::wstring(expandedPath) + L" " + hookName; + std::wstring commandLine = std::wstring(expandedPath); + AppendCommandLineArgument(commandLine, hookName); for (int x = 1; x < argc; x++) { - commandLine += L" " + std::wstring(argv[x]); + AppendCommandLineArgument(commandLine, argv[x]); } // Start the child process.