Skip to content

Commit 2bee9a3

Browse files
Tyrie VellaCopilot
andcommitted
Address PR #1856 review feedback: fix fail-open, tree-ish parsing, pathspec-from-file
Fix three issues identified in code review: 1. HIGH: AddStagedFilesToModifiedPaths now returns failure when git diff --cached exits non-zero, instead of silently succeeding with addedCount=0 (which re-opened the corruption this PR fixes). 2. MEDIUM: GetRestorePathspec now strips the tree-ish argument for checkout commands (e.g. HEAD in 'checkout HEAD -- file'), and skips --source/-s values for restore commands, preventing accidental pathspec scope widening. 3. MEDIUM: --pathspec-from-file and --pathspec-file-nul are now forwarded through the IPC protocol to the mount process, which passes them directly to git diff --cached. The hook never reads the file itself -- git handles all pathspec parsing on both sides. Stdin (-) is unsupported in hook context and fails closed. Refactors parsing logic into UnstageCommandParser (public static class in a separate file) so it can be linked into GVFS.UnitTests without pulling in the full GVFS.Hooks Exe assembly. DiffCachedNameStatus now accepts optional pathspecFromFile and pathspecFileNul parameters. IPC message body format extended: null/empty -> all staged files path1\0path2 -> inline pathspecs (unchanged) \nF\n<filepath> -> --pathspec-from-file \nFZ\n<filepath> -> --pathspec-from-file + --pathspec-file-nul Adds 28 unit tests covering IsUnstageOperation and GetRestorePathspec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 9aa9eda commit 2bee9a3

7 files changed

Lines changed: 562 additions & 64 deletions

File tree

GVFS/GVFS.Common/Git/GitProcess.cs

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -514,9 +514,28 @@ public Result StatusPorcelain()
514514
/// status and path: "A\0path1\0M\0path2\0D\0path3\0".
515515
/// Status codes: A=added, M=modified, D=deleted, R=renamed, C=copied.
516516
/// </summary>
517-
public Result DiffCachedNameStatus(string[] pathspecs = null)
517+
/// <param name="pathspecs">Inline pathspecs to scope the diff, or null for all.</param>
518+
/// <param name="pathspecFromFile">
519+
/// Path to a file containing additional pathspecs (one per line), forwarded
520+
/// as --pathspec-from-file to git. Null if not used.
521+
/// </param>
522+
/// <param name="pathspecFileNul">
523+
/// When true and pathspecFromFile is set, pathspec entries in the file are
524+
/// separated by NUL instead of newline (--pathspec-file-nul).
525+
/// </param>
526+
public Result DiffCachedNameStatus(string[] pathspecs = null, string pathspecFromFile = null, bool pathspecFileNul = false)
518527
{
519528
string command = "diff --cached --name-status -z --no-renames";
529+
530+
if (pathspecFromFile != null)
531+
{
532+
command += " --pathspec-from-file=" + QuoteGitPath(pathspecFromFile);
533+
if (pathspecFileNul)
534+
{
535+
command += " --pathspec-file-nul";
536+
}
537+
}
538+
520539
if (pathspecs != null && pathspecs.Length > 0)
521540
{
522541
command += " -- " + string.Join(" ", pathspecs.Select(p => QuoteGitPath(p)));

GVFS/GVFS.Hooks/Program.Unstage.cs

Lines changed: 32 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
using GVFS.Common.NamedPipes;
22
using System;
3-
using System.Collections.Generic;
4-
using System.Linq;
53

64
namespace GVFS.Hooks
75
{
@@ -14,74 +12,51 @@ namespace GVFS.Hooks
1412
public partial class Program
1513
{
1614
/// <summary>
17-
/// Detects whether the git command is an unstage operation that may need
18-
/// special handling for VFS projections.
19-
/// Matches: "restore --staged", "restore -S", "checkout HEAD --"
15+
/// Sends a PrepareForUnstage message to the GVFS mount process, which will
16+
/// add staged files matching the pathspec to ModifiedPaths so that git will
17+
/// clear skip-worktree and process them.
2018
/// </summary>
21-
private static bool IsUnstageOperation(string command, string[] args)
19+
private static void SendPrepareForUnstageMessage(string command, string[] args)
2220
{
23-
if (command == "restore")
24-
{
25-
return args.Any(arg =>
26-
arg.Equals("--staged", StringComparison.OrdinalIgnoreCase) ||
27-
// -S is --staged; char overload of IndexOf is case-sensitive,
28-
// which is required because lowercase -s means --source
29-
(arg.StartsWith("-") && !arg.StartsWith("--") && arg.IndexOf('S') >= 0));
30-
}
21+
UnstageCommandParser.PathspecResult pathspecResult = UnstageCommandParser.GetRestorePathspec(command, args);
3122

32-
if (command == "checkout")
23+
if (pathspecResult.Failed)
3324
{
34-
// "checkout HEAD -- <paths>" is an unstage+restore operation
35-
bool hasHead = args.Any(arg => arg.Equals("HEAD", StringComparison.OrdinalIgnoreCase));
36-
bool hasDashDash = args.Any(arg => arg == "--");
37-
return hasHead && hasDashDash;
25+
ExitWithError(
26+
"VFS for Git was unable to determine the pathspecs for this unstage operation.",
27+
"This can happen when --pathspec-from-file=- (stdin) is used.",
28+
"",
29+
"Instead, pass the paths directly on the command line:",
30+
" git restore --staged <path1> <path2> ...");
31+
return;
3832
}
3933

40-
return false;
41-
}
42-
43-
/// <summary>
44-
/// Extracts pathspec arguments from a restore --staged command.
45-
/// Returns null-separated pathspecs, or empty string for all staged files.
46-
/// </summary>
47-
private static string GetRestorePathspec(string command, string[] args)
48-
{
49-
// args[0] = hook type, args[1] = git command, rest are arguments
50-
// Skip flags (--staged, -S, --source=, -s, etc.) and extract paths
51-
List<string> paths = new List<string>();
52-
bool pastDashDash = false;
53-
54-
for (int i = 2; i < args.Length; i++)
34+
// Build the message body. Format:
35+
// null/empty → all staged files (no pathspec)
36+
// "path1\0path2" → inline pathspecs (null-separated)
37+
// "\nF\n<filepath>" → --pathspec-from-file (mount forwards to git)
38+
// "\nFZ\n<filepath>" → --pathspec-from-file with --pathspec-file-nul
39+
// The leading \n distinguishes file-reference bodies from inline pathspecs.
40+
string body;
41+
if (pathspecResult.PathspecFromFile != null)
5542
{
56-
string arg = args[i];
43+
string prefix = pathspecResult.PathspecFileNul ? "\nFZ\n" : "\nF\n";
44+
body = prefix + pathspecResult.PathspecFromFile;
5745

58-
if (arg.StartsWith("--git-pid="))
59-
continue;
60-
if (arg == "--")
46+
// If there are also inline pathspecs, append them after another \n
47+
if (!string.IsNullOrEmpty(pathspecResult.InlinePathspecs))
6148
{
62-
pastDashDash = true;
63-
continue;
49+
body += "\n" + pathspecResult.InlinePathspecs;
6450
}
65-
if (!pastDashDash && arg.StartsWith("-"))
66-
continue;
67-
68-
paths.Add(arg);
51+
}
52+
else
53+
{
54+
body = pathspecResult.InlinePathspecs;
6955
}
7056

71-
return paths.Count > 0 ? string.Join("\0", paths) : "";
72-
}
73-
74-
/// <summary>
75-
/// Sends a PrepareForUnstage message to the GVFS mount process, which will
76-
/// add staged files matching the pathspec to ModifiedPaths so that git will
77-
/// clear skip-worktree and process them.
78-
/// </summary>
79-
private static void SendPrepareForUnstageMessage(string command, string[] args)
80-
{
81-
string pathspec = GetRestorePathspec(command, args);
82-
string message = string.IsNullOrEmpty(pathspec)
57+
string message = string.IsNullOrEmpty(body)
8358
? NamedPipeMessages.PrepareForUnstage.Request
84-
: NamedPipeMessages.PrepareForUnstage.Request + "|" + pathspec;
59+
: NamedPipeMessages.PrepareForUnstage.Request + "|" + body;
8560

8661
bool succeeded = false;
8762
string failureMessage = null;

GVFS/GVFS.Hooks/Program.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ private static void RunPreCommands(string[] args)
102102
break;
103103
case "restore":
104104
case "checkout":
105-
if (IsUnstageOperation(command, args))
105+
if (UnstageCommandParser.IsUnstageOperation(command, args))
106106
{
107107
SendPrepareForUnstageMessage(command, args);
108108
}
Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,173 @@
1+
using System;
2+
using System.Collections.Generic;
3+
using System.Linq;
4+
5+
namespace GVFS.Hooks
6+
{
7+
/// <summary>
8+
/// Pure parsing logic for detecting and extracting pathspecs from
9+
/// git unstage commands. Separated from Program.Unstage.cs so it
10+
/// can be linked into the unit test project without pulling in the
11+
/// rest of the Hooks assembly.
12+
/// </summary>
13+
public static class UnstageCommandParser
14+
{
15+
/// <summary>
16+
/// Result of parsing pathspec arguments from a git unstage command.
17+
/// </summary>
18+
public class PathspecResult
19+
{
20+
/// <summary>Null-separated inline pathspecs, or empty for all staged files.</summary>
21+
public string InlinePathspecs { get; set; }
22+
23+
/// <summary>Path to a --pathspec-from-file, or null if not specified.</summary>
24+
public string PathspecFromFile { get; set; }
25+
26+
/// <summary>Whether --pathspec-file-nul was specified.</summary>
27+
public bool PathspecFileNul { get; set; }
28+
29+
/// <summary>True if parsing failed and the command should be blocked.</summary>
30+
public bool Failed { get; set; }
31+
}
32+
33+
/// <summary>
34+
/// Detects whether the git command is an unstage operation that may need
35+
/// special handling for VFS projections.
36+
/// Matches: "restore --staged", "restore -S", "checkout HEAD --"
37+
/// </summary>
38+
public static bool IsUnstageOperation(string command, string[] args)
39+
{
40+
if (command == "restore")
41+
{
42+
return args.Any(arg =>
43+
arg.Equals("--staged", StringComparison.OrdinalIgnoreCase) ||
44+
// -S is --staged; char overload of IndexOf is case-sensitive,
45+
// which is required because lowercase -s means --source
46+
(arg.StartsWith("-") && !arg.StartsWith("--") && arg.IndexOf('S') >= 0));
47+
}
48+
49+
if (command == "checkout")
50+
{
51+
// "checkout HEAD -- <paths>" is an unstage+restore operation.
52+
// TODO: investigate whether "checkout <non-HEAD-tree> -- <paths>" also
53+
// needs PrepareForUnstage protection. It re-stages files (sets index to
54+
// a different tree-ish) and could hit the same skip-worktree interference
55+
// if the target files were staged by cherry-pick -n / merge and aren't in
56+
// ModifiedPaths. Currently scoped to HEAD only as the common unstage case.
57+
bool hasHead = args.Any(arg => arg.Equals("HEAD", StringComparison.OrdinalIgnoreCase));
58+
bool hasDashDash = args.Any(arg => arg == "--");
59+
return hasHead && hasDashDash;
60+
}
61+
62+
return false;
63+
}
64+
65+
/// <summary>
66+
/// Extracts pathspec arguments from a restore/checkout unstage command.
67+
/// Returns a <see cref="PathspecResult"/> containing either inline pathspecs,
68+
/// a --pathspec-from-file reference, or a failure indicator.
69+
///
70+
/// When --pathspec-from-file is specified, the file path is returned so the
71+
/// caller can forward it through IPC to the mount process, which passes it
72+
/// to git diff --cached --pathspec-from-file.
73+
/// </summary>
74+
public static PathspecResult GetRestorePathspec(string command, string[] args)
75+
{
76+
// args[0] = hook type, args[1] = git command, rest are arguments
77+
List<string> paths = new List<string>();
78+
bool pastDashDash = false;
79+
bool skipNext = false;
80+
bool isCheckout = command == "checkout";
81+
82+
// For checkout, the first non-option arg before -- is the tree-ish (e.g. HEAD),
83+
// not a pathspec. Track whether we've consumed it.
84+
bool treeishConsumed = false;
85+
86+
// --pathspec-from-file support: collect the file path and nul flag
87+
string pathspecFromFile = null;
88+
bool pathspecFileNul = false;
89+
bool captureNextAsPathspecFile = false;
90+
91+
for (int i = 2; i < args.Length; i++)
92+
{
93+
string arg = args[i];
94+
95+
if (captureNextAsPathspecFile)
96+
{
97+
pathspecFromFile = arg;
98+
captureNextAsPathspecFile = false;
99+
continue;
100+
}
101+
102+
if (skipNext)
103+
{
104+
skipNext = false;
105+
continue;
106+
}
107+
108+
if (arg.StartsWith("--git-pid="))
109+
continue;
110+
111+
// Capture --pathspec-from-file value
112+
if (arg.StartsWith("--pathspec-from-file="))
113+
{
114+
pathspecFromFile = arg.Substring("--pathspec-from-file=".Length);
115+
continue;
116+
}
117+
118+
if (arg == "--pathspec-from-file")
119+
{
120+
captureNextAsPathspecFile = true;
121+
continue;
122+
}
123+
124+
if (arg == "--pathspec-file-nul")
125+
{
126+
pathspecFileNul = true;
127+
continue;
128+
}
129+
130+
if (arg == "--")
131+
{
132+
pastDashDash = true;
133+
continue;
134+
}
135+
136+
if (!pastDashDash && arg.StartsWith("-"))
137+
{
138+
// For restore: --source and -s take a following argument
139+
if (!isCheckout &&
140+
(arg == "--source" || arg == "-s"))
141+
{
142+
skipNext = true;
143+
}
144+
145+
continue;
146+
}
147+
148+
// For checkout, the first positional arg before -- is the tree-ish
149+
if (isCheckout && !pastDashDash && !treeishConsumed)
150+
{
151+
treeishConsumed = true;
152+
continue;
153+
}
154+
155+
paths.Add(arg);
156+
}
157+
158+
// stdin ("-") is not supported in hook context — the hook's stdin
159+
// is not connected to the user's terminal
160+
if (pathspecFromFile == "-")
161+
{
162+
return new PathspecResult { Failed = true };
163+
}
164+
165+
return new PathspecResult
166+
{
167+
InlinePathspecs = paths.Count > 0 ? string.Join("\0", paths) : "",
168+
PathspecFromFile = pathspecFromFile,
169+
PathspecFileNul = pathspecFileNul,
170+
};
171+
}
172+
}
173+
}

GVFS/GVFS.UnitTests/GVFS.UnitTests.csproj

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@
3737
</None>
3838
</ItemGroup>
3939

40+
<ItemGroup>
41+
<Compile Include="..\GVFS.Hooks\UnstageCommandParser.cs">
42+
<Link>Hooks\UnstageCommandParser.cs</Link>
43+
</Compile>
44+
</ItemGroup>
45+
4046
<ItemGroup>
4147
<None Include="Data\backward.txt">
4248
<CopyToOutputDirectory>Always</CopyToOutputDirectory>

0 commit comments

Comments
 (0)