fix(diff): present submodules also if no local path (gitextensions#12467)

* Use git-diff --raw output to get if a path is a submodule,
instead of parsing .gitmodules (info was not available with --name-status).
This allows submodules where the local path does not exist to be
presented as added/removed.

* Remove support for git-status -porcelain=1, no longer in use,
to simplify the git-diff parsing code

* Move GetDiffChangedFilesFromString tests from GitModuleTests to
the related tests in GetAllChangedFilesOutputParser

* A few Span<> optimizations to reduce allocations.

* Remove handling of error strings in the parsed text,
as stderr is no longer included in the output.

* Remove a few unused test files.

Author: gerhardol

src/app/GitCommands/Git/GetAllChangedFilesOutputParser.cs

@@ -1,4 +1,5 @@
-using GitExtensions.Extensibility;
+using System.Diagnostics;
+using GitExtensions.Extensibility;
 using GitExtensions.Extensibility.Git;
 
 namespace GitCommands.Git
@@ -28,14 +29,13 @@ public IReadOnlyList<GitItemStatus> Parse(string getAllChangedFilesCommandOutput
         }
 
         /// <summary>
-        /// Parse git-status --porcelain=1 and git-diff --name-status
-        /// Outputs are similar, except that git-status has status for both worktree and index.
+        /// Parse git-diff --raw
+        /// format is similar to "git-status --porcelain=1 -z" and "git diff --name-status", no longer supported
         /// </summary>
         /// <param name="getAllChangedFilesCommandOutput">An output of <see cref="Commands.GetAllChangedFiles"/> command.</param>
-        /// <param name="fromDiff">Parse git-diff.</param>
         /// <param name="staged">The staged status <see cref="GitItemStatus"/>, only relevant for git-diff (parsed for git-status).</param>
         /// <returns>list with the git items.</returns>
-        internal List<GitItemStatus> GetAllChangedFilesFromString_v1(string getAllChangedFilesCommandOutput, bool fromDiff, StagedStatus staged)
+        internal List<GitItemStatus> GetDiffChangedFilesFromString(string getAllChangedFilesCommandOutput, StagedStatus staged)
         {
             List<GitItemStatus> diffFiles = [];
 
@@ -44,113 +44,69 @@ internal List<GitItemStatus> GetAllChangedFilesFromString_v1(string getAllChange
                 return diffFiles;
             }
 
-            string trimmedStatus = RemoveWarnings(getAllChangedFilesCommandOutput);
-
-            // Doesn't work with removed submodules
-            IReadOnlyList<string> submodules = GetModule().GetSubmodulesLocalPaths();
-
             // Split all files on '\0' (WE NEED ALL COMMANDS TO BE RUN WITH -z! THIS IS ALSO IMPORTANT FOR ENCODING ISSUES!)
-            string[] files = trimmedStatus.Split(Delimiters.Null, StringSplitOptions.RemoveEmptyEntries);
-            for (int n = 0; n < files.Length; n++)
+            string[] files = getAllChangedFilesCommandOutput.Split(Delimiters.Null, StringSplitOptions.RemoveEmptyEntries);
+            for (int n = 0; n < files.Length; ++n)
             {
                 if (string.IsNullOrEmpty(files[n]))
                 {
                     continue;
                 }
 
-                int splitIndex;
-                if (fromDiff)
-                {
-                    splitIndex = -1;
-                }
-                else
+                if (n >= files.Length - 1)
                 {
-                    // Note that this fails for files with spaces (git-status --porcelain=1 is deprecated)
-                    splitIndex = files[n].IndexOfAny(Delimiters.TabAndSpace, 1);
+                    // no file name for last entry, ignore
+                    Trace.WriteLine($"No filename for last entry, ignoring: {files[n]}");
+                    continue;
                 }
 
-                string status;
-                string fileName;
+                ReadOnlySpan<char> status = files[n].AsSpan();
+                ++n;
+                ReadOnlySpan<char> fileName = files[n].AsSpan();
 
-                if (splitIndex < 0)
+                if (status[0] != ':' || status.Length < 15)
                 {
-                    if (n >= files.Length - 1)
-                    {
-                        // Illegal, ignore
-                        continue;
-                    }
-
-                    status = files[n];
-                    fileName = files[n + 1];
-                    n++;
-                }
-                else
-                {
-                    status = files[n][..splitIndex];
-                    fileName = files[n][splitIndex..];
+                    Trace.WriteLine($"Illegal status line: {fileName}");
+                    continue;
                 }
 
-                char x = status[0];
-                char y = status.Length > 1 ? status[1] : GitItemStatusConverter.UnmodifiedStatus_v1;
+                int statusIndex = status.LastIndexOfAnyExceptInRange('0', '9');
+                char x = status[statusIndex];
 
-                if (fromDiff && staged == StagedStatus.WorkTree && x == GitItemStatusConverter.UnmergedStatus)
+                if (staged == StagedStatus.WorkTree && x == GitItemStatusConverter.UnmergedStatus)
                 {
                     // git-diff has two lines to inform that a file is modified and has a merge conflict
                     continue;
                 }
 
-                // Skip unmerged where both are modified: Only worktree interesting.
-                if ((x != GitItemStatusConverter.UntrackedStatus && x != GitItemStatusConverter.IgnoredStatus && x != GitItemStatusConverter.UnmodifiedStatus_v1)
-                    || x != GitItemStatusConverter.UnmergedStatus
-                    || y != GitItemStatusConverter.UnmergedStatus)
+                GitItemStatus gitItemStatusX = GitItemStatusConverter.FromStatusCharacter(staged, fileName.ToString(), x);
+                if (x is GitItemStatusConverter.RenamedStatus or GitItemStatusConverter.CopiedStatus)
                 {
-                    GitItemStatus gitItemStatusX;
-                    StagedStatus stagedX = fromDiff ? staged : StagedStatus.Index;
-                    if (x == GitItemStatusConverter.RenamedStatus || x == GitItemStatusConverter.CopiedStatus)
+                    gitItemStatusX.RenameCopyPercentage = status[(statusIndex + 1)..].ToString();
+                    gitItemStatusX.OldName = gitItemStatusX.Name;
+
+                    if (n + 1 >= files.Length)
                     {
-                        // Find renamed files...
-                        string nextFile = n + 1 < files.Length ? files[n + 1] : "";
-                        gitItemStatusX = GitItemStatusFromCopyRename(stagedX, fromDiff, nextFile, fileName, x, status);
-                        n++;
+                        Trace.WriteLine($"No next file for renamed entry: {status}::{gitItemStatusX.Name}");
                     }
                     else
                     {
-                        gitItemStatusX = GitItemStatusConverter.FromStatusCharacter(stagedX, fileName, x);
+                        // Renamed file in an extra entry
+                        // No check if the entry seem to be a file (if it starts with '.' it is likely next status)
+                        ++n;
+                        gitItemStatusX.Name = files[n].ToString();
                     }
-
-                    if (submodules.Contains(gitItemStatusX.Name))
-                    {
-                        gitItemStatusX.IsSubmodule = true;
-                    }
-
-                    diffFiles.Add(gitItemStatusX);
-                }
-
-                if (fromDiff || y == GitItemStatusConverter.UnmodifiedStatus_v1)
-                {
-                    continue;
                 }
 
-                GitItemStatus gitItemStatusY;
-                StagedStatus stagedY = StagedStatus.WorkTree;
-                if (y == GitItemStatusConverter.RenamedStatus || y == GitItemStatusConverter.CopiedStatus)
-                {
-                    // Find renamed files...
-                    string nextFile = n + 1 < files.Length ? files[n + 1] : "";
-                    gitItemStatusY = GitItemStatusFromCopyRename(stagedY, false, nextFile, fileName, y, status);
-                    n++;
-                }
-                else
+                const string subMode = "160000";
+                if (status.Slice(1, 6).SequenceEqual(subMode) || status.Slice(8, 6).SequenceEqual(subMode))
                 {
-                    gitItemStatusY = GitItemStatusConverter.FromStatusCharacter(stagedY, fileName, y);
+                    gitItemStatusX.IsSubmodule = true;
                 }
 
-                if (submodules.Contains(gitItemStatusY.Name))
-                {
-                    gitItemStatusY.IsSubmodule = true;
-                }
+                // ignore the sha in status (could be used to set TreeGuid)
 
-                diffFiles.Add(gitItemStatusY);
+                diffFiles.Add(gitItemStatusX);
             }
 
             return diffFiles;
@@ -170,16 +126,15 @@ private static IReadOnlyList<GitItemStatus> GetAllChangedFilesFromString_v2(stri
                 return diffFiles;
             }
 
-            string trimmedStatus = RemoveWarnings(getAllChangedFilesCommandOutput);
-
             // Split all files on '\0'
-            string[] files = trimmedStatus.Split(Delimiters.Null, StringSplitOptions.RemoveEmptyEntries);
-            for (int n = 0; n < files.Length; n++)
+            string[] files = getAllChangedFilesCommandOutput.Split(Delimiters.Null, StringSplitOptions.RemoveEmptyEntries);
+            for (int n = 0; n < files.Length; ++n)
             {
-                string line = files[n];
-                if (line.Length <= 2 || line[1] != ' ')
+                ReadOnlySpan<char> line = files[n].AsSpan();
+                if (line.Length <= 2 || line[1] != ' ' || line[0] == '#')
                 {
-                    // Illegal info, like error output
+                    // Illegal info, like error output,
+                    // except '#' that is an ignored header
                     continue;
                 }
 
@@ -188,7 +143,7 @@ private static IReadOnlyList<GitItemStatus> GetAllChangedFilesFromString_v2(stri
                 if (entryType == GitItemStatusConverter.UntrackedStatus || entryType == GitItemStatusConverter.IgnoredStatus)
                 {
                     // Untracked and ignored with just the path following, supply dummy data for most info.
-                    string otherFileName = line[2..];
+                    string otherFileName = line[2..].ToString();
                     const string NotSumoduleEntry = "N...";
                     UpdateItemStatus(entryType, false, NotSumoduleEntry, otherFileName, null, null);
                     continue;
@@ -217,27 +172,28 @@ private static IReadOnlyList<GitItemStatus> GetAllChangedFilesFromString_v2(stri
                 string fileName;
                 string? oldFileName = null;
                 string? renamePercent = null;
-                string subm = line.Substring(5, 4);
+                string subm = line[5..9].ToString();
 
                 if (entryType == OrdinaryEntry)
                 {
-                    DebugHelpers.Assert(line.Length > 113, "Cannot parse line:" + line);
-                    fileName = line[113..];
+                    DebugHelpers.Assert(line.Length > 113, $"Cannot parse line: {line}");
+                    fileName = line[113..].ToString();
                 }
                 else if (entryType == RenamedEntry)
                 {
-                    DebugHelpers.Assert(n + 1 < files.Length, "Cannot parse renamed:" + line);
+                    DebugHelpers.Assert(n + 1 < files.Length, "Cannot parse renamed: {line}");
 
                     // Find renamed files...
-                    string[] renames = line[114..].Split(Delimiters.Space, 2);
-                    renamePercent = renames[0];
-                    fileName = renames[1];
+                    int pos = line[114..].IndexOfAnyExceptInRange('0', '9');
+                    DebugHelpers.Assert(pos >= 0, "Cannot find space separating rename: {line}");
+                    renamePercent = line[114..(114 + pos)].ToString();
+                    fileName = line[(114 + pos + 1)..].ToString();
                     oldFileName = files[++n];
                 }
                 else if (entryType == UnmergedEntry)
                 {
-                    DebugHelpers.Assert(line.Length > 161, "Cannot parse unmerged:" + line);
-                    fileName = line[161..];
+                    DebugHelpers.Assert(line.Length > 161, $"Cannot parse line: {line}");
+                    fileName = line[161..].ToString();
                 }
                 else
                 {
@@ -292,79 +248,5 @@ void UpdateItemStatus(char x, bool isIndex, string subm, string fileName, string
                 diffFiles.Add(gitItemStatus);
             }
         }
-
-        private static GitItemStatus GitItemStatusFromCopyRename(StagedStatus staged, bool fromDiff, string nextFile, string fileName, char x, string status)
-        {
-            GitItemStatus gitItemStatus;
-
-            // Find renamed files...
-            if (fromDiff)
-            {
-                gitItemStatus = new GitItemStatus(name: nextFile);
-                gitItemStatus.OldName = fileName;
-            }
-            else
-            {
-                gitItemStatus = new GitItemStatus(name: fileName);
-                gitItemStatus.OldName = nextFile;
-            }
-
-            gitItemStatus.IsNew = false;
-            gitItemStatus.IsChanged = false;
-            gitItemStatus.IsDeleted = false;
-            if (x == GitItemStatusConverter.RenamedStatus)
-            {
-                gitItemStatus.IsRenamed = true;
-            }
-            else
-            {
-                gitItemStatus.IsCopied = true;
-            }
-
-            gitItemStatus.IsTracked = true;
-            if (status.Length > 2)
-            {
-                gitItemStatus.RenameCopyPercentage = status[1..];
-            }
-
-            gitItemStatus.Staged = staged;
-
-            return gitItemStatus;
-        }
-
-        private IGitModule GetModule()
-        {
-            IGitModule module = _getModule();
-
-            if (module is null)
-            {
-                throw new ArgumentException($"Require a valid instance of {nameof(IGitModule)}");
-            }
-
-            return module;
-        }
-
-        private static string RemoveWarnings(string statusString)
-        {
-            // The status string from git-diff can show warnings. See tests
-            string trimmedStatus = statusString;
-            int lastNewLinePos = trimmedStatus.LastIndexOfAny(Delimiters.LineFeedAndCarriageReturn);
-            while (lastNewLinePos >= 0)
-            {
-                if (lastNewLinePos == 0)
-                {
-                    trimmedStatus = trimmedStatus.Remove(0, 1);
-                    break;
-                }
-
-                // Error always end with \n and start at previous index
-                int ind = trimmedStatus.LastIndexOfAny(Delimiters.LineFeedAndCarriageReturnAndNull, lastNewLinePos - 1);
-
-                trimmedStatus = trimmedStatus.Remove(ind + 1, lastNewLinePos - ind);
-                lastNewLinePos = trimmedStatus.LastIndexOfAny(Delimiters.LineFeedAndCarriageReturn);
-            }
-
-            return trimmedStatus;
-        }
     }
 }

src/app/GitCommands/Git/GitModule.cs

@@ -2430,7 +2430,7 @@ public async Task<ExecutionResult> GetGrepFileAsync(
                 cancellationToken: cancellationToken);
         }
 
-        public ExecutionResult GetDiffFiles(string? firstRevision, string? secondRevision, bool noCache = false, bool nullSeparated = false, CancellationToken cancellationToken = default)
+        public ExecutionResult GetDiffFiles(string? firstRevision, string? secondRevision, bool noCache = false, bool rawParsable = false, CancellationToken cancellationToken = default)
         {
             noCache = noCache || firstRevision.IsArtificial() || secondRevision.IsArtificial();
 
@@ -2447,8 +2447,8 @@ public ExecutionResult GetDiffFiles(string? firstRevision, string? secondRevisio
                     "--no-ext-diff",
                     "--find-renames",
                     "--find-copies",
-                    "--name-status",
-                    { nullSeparated, "-z" },
+                    { rawParsable, "--raw", "--name-status" },
+                    { rawParsable, "-z" },
                     _revisionDiffProvider.Get(firstRevision, secondRevision)
                 },
                 cache: noCache ? null : GitCommandCache,
@@ -2505,7 +2505,7 @@ public IReadOnlyList<GitItemStatus> GetDiffFilesWithUntracked(string? firstRevis
                 return status.Where(x => x.Staged == stagedStatus || x.IsStatusOnly).ToList();
             }
 
-            ExecutionResult exec = GetDiffFiles(firstRevision, secondRevision, noCache: noCache, nullSeparated: true, cancellationToken);
+            ExecutionResult exec = GetDiffFiles(firstRevision, secondRevision, noCache: noCache, rawParsable: true, cancellationToken);
             List<GitItemStatus> result = GetDiffChangedFilesFromString(exec.StandardOutput, stagedStatus);
             if (!exec.ExitedSuccessfully)
             {
@@ -2705,7 +2705,7 @@ public IReadOnlyList<GitItemStatus> GetIndexFiles()
                 "--find-renames",
                 "--find-copies",
                 "-z",
-                "--name-status",
+                "--raw",
                 "--cached"
             };
             ExecutionResult exec = _gitExecutable.Execute(args, throwOnErrorExit: false);
@@ -2730,14 +2730,14 @@ public IReadOnlyList<GitItemStatus> GetIndexFiles()
         }
 
         /// <summary>
-        /// Parse the output from git-diff --name-status.
+        /// Parse the output from git-diff --raw.
         /// </summary>
         /// <param name="statusString">output from the git command.</param>
         /// <param name="staged">required to determine if <see cref="StagedStatus"/> allows stage/unstage.</param>
         /// <returns>list with the parsed GitItemStatus.</returns>
         /// <seealso href="https://git-scm.com/docs/git-diff"/>
         private List<GitItemStatus> GetDiffChangedFilesFromString(string statusString, StagedStatus staged)
-            => _getAllChangedFilesOutputParser.GetAllChangedFilesFromString_v1(statusString, true, staged);
+            => _getAllChangedFilesOutputParser.GetDiffChangedFilesFromString(statusString, staged);
 
         public IReadOnlyList<GitItemStatus> GetIndexFilesWithSubmodulesStatus()
         {
@@ -2774,10 +2774,16 @@ public bool IsDirtyDir()
 
         public async Task<Patch?> GetCurrentChangesAsync(string? fileName, string? oldFileName, bool staged, string extraDiffArguments, Encoding? encoding = null, bool noLocks = false)
         {
-            string output = await _gitExecutable.GetOutputAsync(Commands.GetCurrentChanges(fileName, oldFileName, staged, extraDiffArguments, noLocks),
-                outputEncoding: LosslessEncoding).ConfigureAwait(false);
+            ExecutionResult result = await _gitExecutable.ExecuteAsync(Commands.GetCurrentChanges(fileName, oldFileName, staged, extraDiffArguments, noLocks),
+                outputEncoding: LosslessEncoding, throwOnErrorExit: false).ConfigureAwait(false);
+            if (!result.ExitedSuccessfully)
+            {
+                // occurs if a submodule in a submodule does not exist
+                Trace.WriteLine($"Failure to get current changes: {result.AllOutput}");
+                return null;
+            }
 
-            IReadOnlyList<Patch> patches = PatchProcessor.CreatePatchesFromString(output, new Lazy<Encoding>(() => encoding ?? FilesEncoding)).ToList();
+            IReadOnlyList<Patch> patches = PatchProcessor.CreatePatchesFromString(result.StandardOutput, new Lazy<Encoding>(() => encoding ?? FilesEncoding)).ToList();
 
             return GetPatch(patches, fileName, oldFileName);
         }