Finish a sibling comparison when a shared entry cannot be read [patch]

ProjectDirector.Test/SimilarReposTests.cs

@@ -364,6 +364,139 @@ public void RefreshingOneFileRejectsARepositoryThatIsNotOnGitHub()
 			() => ProjectDirector.RefreshFileDiff(repoA, other, RelativeFilePath.Create<RelativeFilePath>("shared.txt")));
 	}
 
+	[TestMethod]
+	public async Task SiblingsSharingASubmodulePathCompareWithTheGitlinkLeftOut()
+	{
+		string a = CreateRepository([("shared.txt", "one\n")]);
+		string b = CreateRepository([("shared.txt", "two\n")]);
+
+		try
+		{
+			// A submodule is tracked as a gitlink, and its path is a directory on disk even before
+			// the submodule is initialised. Reading it as a file threw out of the background task.
+			AddGitlink(a, "external");
+			AddGitlink(b, "external");
+
+			GitHubRepository repoA = Repository(a, "A");
+			ConcurrentQueue<string> log = [];
+
+			await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, Repository(b, "B")], log.Enqueue).ConfigureAwait(false);
+
+			Assert.IsFalse(repoA.SimilarReposPending, "The comparison should finish rather than stay pending.");
+			Assert.IsNotNull(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create<RelativeFilePath>("shared.txt")));
+			Assert.IsNull(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create<RelativeFilePath>("external")), "A gitlink has no content to diff.");
+			Assert.Contains("Compared", log.Single());
+		}
+		finally
+		{
+			TryDeleteDirectory(a);
+			TryDeleteDirectory(b);
+		}
+	}
+
+	[TestMethod]
+	public async Task ASharedFileThatCannotBeReadIsLeftOutAndLogged()
+	{
+		string a = CreateRepository([("shared.txt", "one\n"), ("locked.txt", "a\n")]);
+		string b = CreateRepository([("shared.txt", "two\n"), ("locked.txt", "b\n")]);
+
+		try
+		{
+			GitHubRepository repoA = Repository(a, "A");
+			ConcurrentQueue<string> log = [];
+
+			// An exclusive handle stands in for a file another process holds open, and unlike a
+			// permission bit it is refused under root as well.
+			using (new FileStream(Path.Join(b, "locked.txt"), FileMode.Open, FileAccess.ReadWrite, FileShare.None))
+			{
+				await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, Repository(b, "B")], log.Enqueue).ConfigureAwait(false);
+			}
+
+			Assert.IsFalse(repoA.SimilarReposPending, "An unreadable file must not leave the comparison pending.");
+			Assert.IsNotNull(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create<RelativeFilePath>("shared.txt")), "The readable files are still compared.");
+			Assert.IsNull(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create<RelativeFilePath>("locked.txt")));
+			Assert.Contains(line => line.Contains("Left locked.txt out", StringComparison.Ordinal), log, "The log should say which file was left out.");
+		}
+		finally
+		{
+			TryDeleteDirectory(a);
+			TryDeleteDirectory(b);
+		}
+	}
+
+	[TestMethod]
+	public async Task ASharedFileMissingFromTheWorkingTreeIsComparedAsEmpty()
+	{
+		string a = CreateRepository([("gone.txt", "a\n")]);
+		string b = CreateRepository([("gone.txt", "b\n")]);
+
+		try
+		{
+			AddNestedFile(a, "a\n");
+			AddNestedFile(b, "b\n");
+
+			// A tracked file can be absent on disk, directly or because its whole folder is.
+			File.Delete(Path.Join(b, "gone.txt"));
+			Directory.Delete(Path.Join(b, "sub"), recursive: true);
+
+			GitHubRepository repoA = Repository(a, "A");
+			await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, Repository(b, "B")], _ => { }).ConfigureAwait(false);
+
+			Assert.IsFalse(repoA.SimilarReposPending);
+			Assert.IsNotEmpty(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create<RelativeFilePath>("gone.txt"))!.DiffBlocks, "A missing file diffs against nothing.");
+			Assert.IsNotEmpty(ProjectDirector.FindDiff(repoA, Name("B"), RelativeFilePath.Create<RelativeFilePath>("sub/nested.txt"))!.DiffBlocks, "A file in a missing folder diffs against nothing.");
+		}
+		finally
+		{
+			TryDeleteDirectory(a);
+			TryDeleteDirectory(b);
+		}
+	}
+
+	// Windows file systems cannot hold a file name containing '<' or '>'.
+	[TestMethod]
+	[OSCondition(ConditionMode.Exclude, OperatingSystems.Windows)]
+	public async Task AComparisonThatFailsStillEndsThePendingStateAndSaysWhy()
+	{
+		// A path that git tracks happily but the semantic path types refuse, which throws partway
+		// through building the comparison.
+		string a = CreateRepository([("a<b>.txt", "one\n")]);
+		string b = CreateRepository([("a<b>.txt", "two\n")]);
+
+		try
+		{
+			GitHubRepository repoA = Repository(a, "A");
+			ConcurrentQueue<string> log = [];
+
+			await ProjectDirector.CompareSiblingsAsync(repoA, [repoA, Repository(b, "B")], log.Enqueue).ConfigureAwait(false);
+
+			Assert.IsFalse(repoA.SimilarReposPending, "A failed comparison must not leave the panels pending.");
+			Assert.IsEmpty(repoA.SimilarRepoDiffs);
+			Assert.Contains("failed", log.Single());
+		}
+		finally
+		{
+			TryDeleteDirectory(a);
+			TryDeleteDirectory(b);
+		}
+	}
+
+	private static void AddNestedFile(string root, string contents)
+	{
+		_ = Directory.CreateDirectory(Path.Join(root, "sub"));
+		File.WriteAllText(Path.Join(root, "sub", "nested.txt"), contents);
+		Assert.IsTrue(GitCli.RunIn(root, "add", "--all").Succeeded, "git add failed.");
+		Assert.IsTrue(GitCli.RunIn(root, "commit", "-m", "Add nested file").Succeeded, "git commit failed.");
+	}
+
+	private static void AddGitlink(string root, string path)
+	{
+		string head = GitCli.RunIn(root, "rev-parse", "HEAD").OutputText;
+		Assert.IsTrue(GitCli.RunIn(root, "update-index", "--add", "--cacheinfo", $"160000,{head},{path}").Succeeded, "Adding the gitlink failed.");
+		Assert.IsTrue(GitCli.RunIn(root, "commit", "-m", "Add submodule").Succeeded, "Committing the gitlink failed.");
+		_ = Directory.CreateDirectory(Path.Join(root, path));
+	}
+
 	private static string CreateRepository(IEnumerable<(string RelativePath, string Contents)> files)
 	{
 		string root = Path.Join(Path.GetTempPath(), $"ktsu_pd_similar_{Guid.NewGuid():N}");

ProjectDirector/ProjectDirector.cs

@@ -18,7 +18,7 @@
 using ktsu.ImGui.Widgets;
 using ktsu.ImGui.Styler;
 using Octokit;
 // using OpenAI.Chat;
 using Semantics.Paths;
 
 #pragma warning disable CA1506
@@ -56,7 +56,7 @@
 	/// </summary>
 	private GitHubOwnerName? OwnerPendingTokenPopup { get; set; }
 
 	// private ChatClient ChatClient { get; init; }
 
 	private static void Main(string[] _)
 	{
@@ -79,7 +79,7 @@
 		_ = MakeLoadedOptionsSafe(Options, QueueLog);
 
 		Options.Save();
 		// ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken));
 		DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerCols = new("VerticalDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerRows = new("HorizontalDivider", DividerResized, ImGuiWidgets.DividerLayout.Rows);
@@ -738,7 +738,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -1336,7 +1336,24 @@
 		int token = repo.RequestSimilarRepoDiffs();
 		Task task = new(() =>
 		{
-			Dictionary<FullyQualifiedGitHubRepoName, Dictionary<RelativeFilePath, DiffResult>> diffs = DiffAgainstAll(repo, others);
+			Dictionary<FullyQualifiedGitHubRepoName, Dictionary<RelativeFilePath, DiffResult>> diffs;
+			try
+			{
+				diffs = DiffAgainstAll(repo, others, log);
+			}
+			catch (Exception e) when (e is IOException or UnauthorizedAccessException or InvalidOperationException or ArgumentException)
+			{
+				// Nothing observes this task, so a throw out of it would leave the panels on
+				// "Comparing repositories..." for good. Publishing an empty answer ends the pending
+				// state, and the log says why there is nothing to show.
+				if (repo.TryApplySimilarRepoDiffs(token, []))
+				{
+					log($"[{DateTimeOffset.Now}] Comparing {repo.RemotePath} failed: {e.Message}");
+				}
+
+				return;
+			}
+
 			if (repo.TryApplySimilarRepoDiffs(token, diffs))
 			{
 				log($"[{DateTimeOffset.Now}] Compared {repo.RemotePath} against {others.Count} other {(others.Count == 1 ? "repository" : "repositories")}");
@@ -1352,6 +1369,7 @@
 	/// </summary>
 	/// <param name="repo">The repository every sibling is compared against.</param>
 	/// <param name="others">The siblings, paired with the names to key the result by.</param>
+	/// <param name="log">Reports each shared file that could not be read and so was left out.</param>
 	/// <returns>The diffs for each sibling.</returns>
 	/// <remarks>
 	/// The base repository's tracked-file list is read once for the whole run rather than once per
@@ -1361,7 +1379,8 @@
 	/// </remarks>
 	internal static Dictionary<FullyQualifiedGitHubRepoName, Dictionary<RelativeFilePath, DiffResult>> DiffAgainstAll(
 		GitRepository repo,
-		IEnumerable<KeyValuePair<FullyQualifiedGitHubRepoName, GitRepository>> others)
+		IEnumerable<KeyValuePair<FullyQualifiedGitHubRepoName, GitRepository>> others,
+		Action<string>? log = null)
 	{
 		Collection<string> trackedFiles = GitCli.IsRepository(repo.LocalPath)
 			? GitCli.ListTrackedFiles(repo.LocalPath)
@@ -1370,13 +1389,13 @@
 		Dictionary<FullyQualifiedGitHubRepoName, Dictionary<RelativeFilePath, DiffResult>> diffs = [];
 		foreach ((FullyQualifiedGitHubRepoName otherRepoName, GitRepository otherRepo) in others)
 		{
-			diffs[otherRepoName] = DiffRepos(repo, trackedFiles, otherRepo);
+			diffs[otherRepoName] = DiffRepos(repo, trackedFiles, otherRepo, log);
 		}
 
 		return diffs;
 	}
 
-	private static Dictionary<RelativeFilePath, DiffResult> DiffRepos(GitRepository repoA, IEnumerable<string> trackedFilesA, GitRepository repoB)
+	private static Dictionary<RelativeFilePath, DiffResult> DiffRepos(GitRepository repoA, IEnumerable<string> trackedFilesA, GitRepository repoB, Action<string>? log)
 	{
 		Dictionary<RelativeFilePath, DiffResult> diffs = [];
 
@@ -1389,12 +1408,22 @@
 			.Intersect(GitCli.ListTrackedFiles(repoB.LocalPath))
 			.ToCollection();
 
-		Dictionary<string, string> fileContents = matches.ToDictionary(x => x, x => ReadFileOrEmpty(repoA.LocalPath, x));
-		Dictionary<string, string> otherFileContents = matches.ToDictionary(x => x, x => ReadFileOrEmpty(repoB.LocalPath, x));
-
 		foreach (string match in matches)
 		{
-			diffs[RelativeFilePath.Create<RelativeFilePath>(match)] = Differ.Instance.CreateLineDiffs(fileContents[match], otherFileContents[match], ignoreWhitespace: false, ignoreCase: false);
+			// A submodule's gitlink is tracked but is a directory on disk, as is a tracked link to
+			// one. Neither has content to diff, so the pair is left out rather than read.
+			if (!TryReadTrackedFile(repoA.LocalPath, match, out string fileContents, out string? reason)
+				|| !TryReadTrackedFile(repoB.LocalPath, match, out string otherFileContents, out reason))
+			{
+				if (reason is not null)
+				{
+					log?.Invoke($"[{DateTimeOffset.Now}] Left {match} out of the comparison with {repoB.RemotePath}: {reason}");
+				}
+
+				continue;
+			}
+
+			diffs[RelativeFilePath.Create<RelativeFilePath>(match)] = Differ.Instance.CreateLineDiffs(fileContents, otherFileContents, ignoreWhitespace: false, ignoreCase: false);
 		}
 
 		return diffs;
@@ -1404,22 +1433,55 @@
 	/// Reads a tracked file from a working tree, treating anything missing on disk as empty. A file
 	/// can be tracked and still be absent, and a diff against nothing is the useful answer.
 	/// </summary>
-	private static string ReadFileOrEmpty(string repoPath, string relativePath)
+	/// <param name="repoPath">The working tree.</param>
+	/// <param name="relativePath">The tracked path, relative to <paramref name="repoPath"/>.</param>
+	/// <param name="contents">The file's text, or empty when it is missing or cannot be read.</param>
+	/// <param name="reason">
+	/// Why an existing file could not be read, or <see langword="null"/> when there is nothing to
+	/// report: the file was read, was missing, or is a directory.
+	/// </param>
+	/// <returns>Whether there is file content to diff, which a directory never has.</returns>
+	/// <remarks>
+	/// Every failure is answered rather than thrown. The whole-repository comparison runs on a task
+	/// nobody observes, and the single-file re-diff runs on the render thread.
+	/// </remarks>
+	private static bool TryReadTrackedFile(string repoPath, string relativePath, out string contents, out string? reason)
 	{
+		contents = string.Empty;
+		reason = null;
+
+		string fullPath = Path.Join(repoPath, relativePath);
+		if (Directory.Exists(fullPath))
+		{
+			return false;
+		}
+
 		try
 		{
-			return File.ReadAllText(Path.Combine(repoPath, relativePath));
+			contents = File.ReadAllText(fullPath);
+			return true;
 		}
 		catch (FileNotFoundException)
 		{
-			return string.Empty;
+			return true;
 		}
 		catch (DirectoryNotFoundException)
 		{
-			return string.Empty;
+			return true;
+		}
+		catch (Exception e) when (e is IOException or UnauthorizedAccessException)
+		{
+			reason = e.Message;
+			return false;
 		}
 	}
 
+	private static string ReadFileOrEmpty(string repoPath, string relativePath)
+	{
+		_ = TryReadTrackedFile(repoPath, relativePath, out string contents, out _);
+		return contents;
+	}
+
 	/// <summary>
 	/// Looks up one file's diff against a sibling repository.
 	/// </summary>
@@ -2111,7 +2173,7 @@
 
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;