From c01df3012e3fd1dddd8312459d818878f803e414 Mon Sep 17 00:00:00 2001 From: Canon Date: Fri, 22 May 2026 15:00:03 +0800 Subject: [PATCH] Improve git update error reporting --- Commands/UpdateCommand.cs | 14 +-- .../Services/GitModUpdaterTests.cs | 106 ++++++++++++++++++ .../Services/GitOperationResultTests.cs | 34 ++++++ .../Services/ModDiscoveryServiceTests.cs | 8 +- Models/GitOperationResult.cs | 23 ++++ Services/GitModUpdater.cs | 59 ++++++++++ Services/GitService.cs | 34 ++++-- Services/IGitService.cs | 4 +- UI/InteractiveMenu.cs | 14 +-- 9 files changed, 252 insertions(+), 44 deletions(-) create mode 100644 FCPModUpdater.Tests/Services/GitModUpdaterTests.cs create mode 100644 FCPModUpdater.Tests/Services/GitOperationResultTests.cs create mode 100644 Models/GitOperationResult.cs create mode 100644 Services/GitModUpdater.cs diff --git a/Commands/UpdateCommand.cs b/Commands/UpdateCommand.cs index 77c0029..f17db24 100644 --- a/Commands/UpdateCommand.cs +++ b/Commands/UpdateCommand.cs @@ -53,19 +53,7 @@ public class UpdateCommand( "Updating mods", updateableMods, installedMod => installedMod.Name, - async (mod, progress) => - { - progress.Report(25); - var fetchOk = await gitService.FetchAsync(mod.Path, ct: ct); - if (!fetchOk) - return (false, "Fetch failed"); - - progress.Report(50); - var pullOk = await gitService.PullAsync(mod.Path, ct: ct); - progress.Report(100); - - return (pullOk, pullOk ? null : "Pull failed"); - }); + async (mod, progress) => await GitModUpdater.UpdateAsync(gitService, mod, progress, ct)); ModTableRenderer.RenderUpdateSummary(results); diff --git a/FCPModUpdater.Tests/Services/GitModUpdaterTests.cs b/FCPModUpdater.Tests/Services/GitModUpdaterTests.cs new file mode 100644 index 0000000..bf96ad6 --- /dev/null +++ b/FCPModUpdater.Tests/Services/GitModUpdaterTests.cs @@ -0,0 +1,106 @@ +using NSubstitute; + +namespace FCPModUpdater.Tests.Services; + +public class GitModUpdaterTests +{ + private readonly IGitService _gitService = Substitute.For(); + + private static InstalledMod MakeMod() => new() + { + Name = "TestMod", + Path = "/mods/TestMod", + Source = ModSource.Git, + Status = ModStatus.Behind, + CommitsBehind = 1 + }; + + [Fact] + public async Task UpdateAsync_FetchFailure_ReturnsModSpecificError() + { + CancellationToken token = TestContext.Current.CancellationToken; + InstalledMod mod = MakeMod(); + _gitService.FetchAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(false, 128, "fatal: fetch failed")); + _gitService.GetCommitDifferenceAsync(mod.Path, token) + .Returns((Behind: 1, Ahead: 0)); + + var result = await GitModUpdater.UpdateAsync(_gitService, mod, ct: token); + + Assert.False(result.Success); + Assert.Equal("Fetch failed for TestMod: fatal: fetch failed", result.Error); + } + + [Fact] + public async Task UpdateAsync_PullFailure_ReturnsModSpecificError() + { + CancellationToken token = TestContext.Current.CancellationToken; + InstalledMod mod = MakeMod(); + _gitService.FetchAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(true, 0, null)); + _gitService.PullAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(false, 128, "fatal: pull failed")); + _gitService.GetCommitDifferenceAsync(mod.Path, token) + .Returns((Behind: 1, Ahead: 0)); + + var result = await GitModUpdater.UpdateAsync(_gitService, mod, ct: token); + + Assert.False(result.Success); + Assert.Equal("Pull failed for TestMod: fatal: pull failed", result.Error); + } + + [Fact] + public async Task UpdateAsync_PullFailureButNoLongerBehind_ReturnsSuccess() + { + CancellationToken token = TestContext.Current.CancellationToken; + InstalledMod mod = MakeMod(); + _gitService.FetchAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(true, 0, null)); + _gitService.PullAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(false, 1, "fatal: transient failure")); + _gitService.GetCommitDifferenceAsync(mod.Path, token) + .Returns((Behind: 0, Ahead: 0)); + + var result = await GitModUpdater.UpdateAsync(_gitService, mod, ct: token); + + Assert.True(result.Success); + Assert.Null(result.Error); + } + + [Fact] + public async Task UpdateAsync_ExitCodeOneFourOne_ReturnsSuccessWhenNoLongerBehind() + { + CancellationToken token = TestContext.Current.CancellationToken; + InstalledMod mod = MakeMod(); + _gitService.FetchAsync(mod.Path, Arg.Any>(), token) + .Returns(GitOperationResult.FromExitCode(141, "progress pipe closed")); + _gitService.PullAsync(mod.Path, Arg.Any>(), token) + .Returns(GitOperationResult.FromExitCode(141, "progress pipe closed")); + _gitService.GetCommitDifferenceAsync(mod.Path, token) + .Returns((Behind: 0, Ahead: 0)); + + var result = await GitModUpdater.UpdateAsync(_gitService, mod, ct: token); + + Assert.True(result.Success); + Assert.Null(result.Error); + await _gitService.Received(1).GetCommitDifferenceAsync(mod.Path, token); + } + + [Fact] + public async Task UpdateAsync_PullSuccessButStillBehind_ReturnsFailure() + { + CancellationToken token = TestContext.Current.CancellationToken; + InstalledMod mod = MakeMod(); + _gitService.FetchAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(true, 0, null)); + _gitService.PullAsync(mod.Path, Arg.Any>(), token) + .Returns(new GitOperationResult(true, 0, null)); + _gitService.GetCommitDifferenceAsync(mod.Path, token) + .Returns((Behind: 2, Ahead: 0)); + + var result = await GitModUpdater.UpdateAsync(_gitService, mod, ct: token); + + Assert.False(result.Success); + Assert.Equal("Pull completed for TestMod, but it is still 2 commit(s) behind", result.Error); + } +} diff --git a/FCPModUpdater.Tests/Services/GitOperationResultTests.cs b/FCPModUpdater.Tests/Services/GitOperationResultTests.cs new file mode 100644 index 0000000..ce91e8f --- /dev/null +++ b/FCPModUpdater.Tests/Services/GitOperationResultTests.cs @@ -0,0 +1,34 @@ +namespace FCPModUpdater.Tests.Services; + +public class GitOperationResultTests +{ + [Fact] + public void FromExitCode_Zero_IsSuccessful() + { + GitOperationResult result = GitOperationResult.FromExitCode(0, null); + + Assert.True(result.Success); + Assert.Equal(0, result.ExitCode); + Assert.Null(result.Error); + } + + [Fact] + public void FromExitCode_OneFourOne_IsSuccessful() + { + GitOperationResult result = GitOperationResult.FromExitCode(141, "progress pipe closed"); + + Assert.True(result.Success); + Assert.Equal(141, result.ExitCode); + Assert.Equal("progress pipe closed", result.Error); + } + + [Fact] + public void FromExitCode_NonZero_IsFailure() + { + GitOperationResult result = GitOperationResult.FromExitCode(128, "fatal: failed"); + + Assert.False(result.Success); + Assert.Equal(128, result.ExitCode); + Assert.Equal("fatal: failed", result.Error); + } +} diff --git a/FCPModUpdater.Tests/Services/ModDiscoveryServiceTests.cs b/FCPModUpdater.Tests/Services/ModDiscoveryServiceTests.cs index 4e5b4d1..1e82418 100644 --- a/FCPModUpdater.Tests/Services/ModDiscoveryServiceTests.cs +++ b/FCPModUpdater.Tests/Services/ModDiscoveryServiceTests.cs @@ -159,7 +159,7 @@ public class DiscoverModsAsyncTests : IDisposable _gitService.HasLocalChangesAsync(Arg.Any(), Arg.Any()) .Returns(false); _gitService.FetchAsync(Arg.Any(), Arg.Any>(), Arg.Any()) - .Returns(true); + .Returns(new GitOperationResult(true, 0, null)); _gitService.GetCommitDifferenceAsync(Arg.Any(), Arg.Any()) .Returns((Behind: 0, Ahead: 0)); @@ -207,7 +207,7 @@ public class DiscoverModsAsyncTests : IDisposable _gitService.HasLocalChangesAsync(Arg.Any(), Arg.Any()) .Returns(false); _gitService.FetchAsync(Arg.Any(), Arg.Any>(), Arg.Any()) - .Returns(true); + .Returns(new GitOperationResult(true, 0, null)); _gitService.GetCommitDifferenceAsync(Arg.Any(), Arg.Any()) .Returns((Behind: 0, Ahead: 0)); @@ -250,7 +250,7 @@ public class DiscoverModsAsyncTests : IDisposable _gitService.HasLocalChangesAsync(Arg.Any(), Arg.Any()) .Returns(false); _gitService.FetchAsync(Arg.Any(), Arg.Any>(), Arg.Any()) - .Returns(true); + .Returns(new GitOperationResult(true, 0, null)); IReadOnlyList result = await _service.DiscoverModsAsync(_tempDir, ct: token); @@ -297,7 +297,7 @@ public class DiscoverModsAsyncTests : IDisposable _gitService.HasLocalChangesAsync(Arg.Any(), Arg.Any()) .Returns(false); _gitService.FetchAsync(Arg.Any(), Arg.Any>(), Arg.Any()) - .Returns(true); + .Returns(new GitOperationResult(true, 0, null)); _gitService.GetCommitDifferenceAsync(Arg.Any(), Arg.Any()) .Returns((Behind: 3, Ahead: 0)); diff --git a/Models/GitOperationResult.cs b/Models/GitOperationResult.cs new file mode 100644 index 0000000..4b6bca3 --- /dev/null +++ b/Models/GitOperationResult.cs @@ -0,0 +1,23 @@ +namespace FCPModUpdater.Models; + +public sealed record GitOperationResult( + bool Success, + int ExitCode, + string? Error) +{ + public static GitOperationResult FromExitCode(int exitCode, string? error) + { + return new GitOperationResult(IsSuccessExitCode(exitCode), exitCode, NormalizeError(error)); + } + + private static bool IsSuccessExitCode(int exitCode) + { + // Exit code 141 = SIGPIPE (128+13), harmless when git's progress pipe closes early. + return exitCode is 0 or 141; + } + + private static string? NormalizeError(string? error) + { + return string.IsNullOrWhiteSpace(error) ? null : error.Trim(); + } +} diff --git a/Services/GitModUpdater.cs b/Services/GitModUpdater.cs new file mode 100644 index 0000000..054dbd0 --- /dev/null +++ b/Services/GitModUpdater.cs @@ -0,0 +1,59 @@ +using FCPModUpdater.Models; + +namespace FCPModUpdater.Services; + +public static class GitModUpdater +{ + public static async Task<(bool Success, string? Error)> UpdateAsync( + IGitService gitService, + InstalledMod mod, + IProgress? progress = null, + CancellationToken ct = default) + { + progress?.Report(25); + GitOperationResult fetchResult = await gitService.FetchAsync(mod.Path, ct: ct); + if (!fetchResult.Success) + { + return await VerifyOrFailAsync(gitService, mod, "Fetch", fetchResult, ct); + } + + progress?.Report(50); + GitOperationResult pullResult = await gitService.PullAsync(mod.Path, ct: ct); + progress?.Report(100); + + (int behind, _) = await gitService.GetCommitDifferenceAsync(mod.Path, ct); + if (behind == 0) + { + return (Success: true, Error: null); + } + + if (pullResult.Success) + { + return (Success: false, + Error: $"Pull completed for {mod.Name}, but it is still {behind} commit(s) behind"); + } + + return (Success: false, Error: $"Pull failed for {mod.Name}: {FormatGitFailure(pullResult)}"); + } + + private static async Task<(bool Success, string? Error)> VerifyOrFailAsync( + IGitService gitService, + InstalledMod mod, + string operation, + GitOperationResult result, + CancellationToken ct) + { + (int behind, _) = await gitService.GetCommitDifferenceAsync(mod.Path, ct); + if (behind == 0) + { + return (Success: true, Error: null); + } + + return (Success: false, Error: $"{operation} failed for {mod.Name}: {FormatGitFailure(result)}"); + } + + private static string FormatGitFailure(GitOperationResult result) + { + return result.Error ?? $"git exited with code {result.ExitCode}"; + } +} diff --git a/Services/GitService.cs b/Services/GitService.cs index 96bce79..a0f14c1 100644 --- a/Services/GitService.cs +++ b/Services/GitService.cs @@ -108,7 +108,7 @@ public partial class GitService : IGitService return ParseCommitLog(output); } - public async Task FetchAsync(string path, IProgress? progress = null, + public async Task FetchAsync(string path, IProgress? progress = null, CancellationToken ct = default) { progress?.Report("Fetching from remote..."); @@ -119,13 +119,15 @@ public partial class GitService : IGitService ct, idleTimeoutMs: NetworkCommandIdleTimeoutMs); - if (exitCode != 0) - progress?.Report($"Fetch failed: {error}"); + var result = GitOperationResult.FromExitCode(exitCode, error); - return exitCode == 0; + if (!result.Success) + progress?.Report($"Fetch failed: {result.Error ?? $"git exited with code {result.ExitCode}"}"); + + return result; } - public async Task PullAsync(string path, IProgress? progress = null, + public async Task PullAsync(string path, IProgress? progress = null, CancellationToken ct = default) { progress?.Report("Pulling changes..."); @@ -136,10 +138,12 @@ public partial class GitService : IGitService ct, idleTimeoutMs: NetworkCommandIdleTimeoutMs); - if (exitCode != 0) - progress?.Report($"Pull failed: {error}"); + var result = GitOperationResult.FromExitCode(exitCode, error); - return exitCode == 0; + if (!result.Success) + progress?.Report($"Pull failed: {result.Error ?? $"git exited with code {result.ExitCode}"}"); + + return result; } public async Task<(bool Success, string? Error)> CloneAsync(string url, string targetPath, IProgress? progress = null, @@ -157,10 +161,10 @@ public partial class GitService : IGitService ct, idleTimeoutMs: NetworkCommandIdleTimeoutMs); - // Exit code 141 = SIGPIPE (128+13), harmless when git's progress pipe closes early - if (exitCode != 0 && exitCode != 141) + var result = GitOperationResult.FromExitCode(exitCode, error); + if (!result.Success) { - var errorMessage = $"Clone failed (exit code {exitCode}): url={url}, target={targetPath}, git output: {error}"; + var errorMessage = $"Clone failed for {targetPath}: {FormatGitFailure(result)}"; progress?.Report(errorMessage); return (Success: false, Error: errorMessage); } @@ -168,7 +172,8 @@ public partial class GitService : IGitService // Verify the clone actually produced a valid git repo if (!Directory.Exists(targetPath) || !await IsGitRepositoryAsync(targetPath, ct)) { - var errorMessage = $"Clone appeared to finish but target is not a valid git repo: url={url}, target={targetPath}, git output: {error}"; + var errorMessage = + $"Clone appeared to finish but target is not a valid git repo: url={url}, target={targetPath}, git output: {result.Error}"; progress?.Report(errorMessage); return (Success: false, Error: errorMessage); } @@ -176,6 +181,11 @@ public partial class GitService : IGitService return (Success: true, Error: null); } + internal static string FormatGitFailure(GitOperationResult result) + { + return result.Error ?? $"git exited with code {result.ExitCode}"; + } + public async Task> GetRemoteBranchesAsync(string path, CancellationToken ct = default) { var (exitCode, output, _) = await RunGitCommandAsync(path, "branch -r", ct); diff --git a/Services/IGitService.cs b/Services/IGitService.cs index 1518d56..3b3928d 100644 --- a/Services/IGitService.cs +++ b/Services/IGitService.cs @@ -10,8 +10,8 @@ public interface IGitService Task GetCurrentBranchAsync(string path, CancellationToken ct = default); Task<(int Behind, int Ahead)> GetCommitDifferenceAsync(string path, CancellationToken ct = default); Task> GetIncomingCommitsAsync(string path, int limit = 10, CancellationToken ct = default); - Task FetchAsync(string path, IProgress? progress = null, CancellationToken ct = default); - Task PullAsync(string path, IProgress? progress = null, CancellationToken ct = default); + Task FetchAsync(string path, IProgress? progress = null, CancellationToken ct = default); + Task PullAsync(string path, IProgress? progress = null, CancellationToken ct = default); Task<(bool Success, string? Error)> CloneAsync(string url, string targetPath, IProgress? progress = null, IProgress? percentProgress = null, CancellationToken ct = default); Task> GetRemoteBranchesAsync(string path, CancellationToken ct = default); Task CheckoutAsync(string path, string branchOrCommit, CancellationToken ct = default); diff --git a/UI/InteractiveMenu.cs b/UI/InteractiveMenu.cs index c5acb44..95e9c4d 100644 --- a/UI/InteractiveMenu.cs +++ b/UI/InteractiveMenu.cs @@ -156,19 +156,7 @@ public class InteractiveMenu "Updating mods", selected.ToList(), mod => mod.Name, - async (mod, progress) => - { - progress.Report(25); - var fetchOk = await _gitService.FetchAsync(mod.Path, ct: ct); - if (!fetchOk) - return (Success: false, Error: "Fetch failed"); - - progress.Report(50); - var pullOk = await _gitService.PullAsync(mod.Path, ct: ct); - progress.Report(100); - - return (Success: pullOk, Error: pullOk ? null : "Pull failed"); - }); + async (mod, progress) => await GitModUpdater.UpdateAsync(_gitService, mod, progress, ct)); ModTableRenderer.RenderUpdateSummary(results); WaitForKey();