Skip to content

Commit f0be692

Browse files
NyanHeartNyanHeart
committed
Fix: Address PR review comments (threading, cleanup & error handling)
- Threading: Wrapped Status Center UI updates and dialogs in `DispatcherQueue.TryEnqueue` to prevent `COMException` (RPC_E_WRONG_THREAD) during background fetches. - Cleanup: Removed unused `_fetchOptions` field and restored `Prune = true` for fetch operations. - Error Handling: - Catch `CheckoutConflictException` specifically for uncommitted changes during pull, providing a helpful localized error message. - Catch generic `LibGit2SharpException` to surface authentic underlying errors (e.g. ssh-agent failures, missing files) to the user via dialog. - Localization: Added `GitError_UncommittedChanges` resource string.
1 parent bfbc9a2 commit f0be692

File tree

2 files changed

+153
-95
lines changed

2 files changed

+153
-95
lines changed

‎src/Files.App/Strings/en-US/Resources.resw‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3823,7 +3823,10 @@
38233823
<comment>Shown in a StatusCenter card.</comment>
38243824
</data>
38253825
<data name="StatusCenter_GitPullFailed_SubHeader" xml:space="preserve">
3826-
<value>Failed to pull branch "{0}" from "{1}"</value>
3826+
<value>We couldn't pull the latest changes from the remote right now.</value>
3827+
</data>
3828+
<data name="GitError_UncommittedChanges" xml:space="preserve">
3829+
<value>Please commit or stash your changes before pulling.</value>
38273830
<comment>Shown in a StatusCenter card.</comment>
38283831
</data>
38293832
<data name="StatusCenter_GitPullInProgress_Header" xml:space="preserve">

‎src/Files.App/Utils/Git/GitHelpers.cs‎

Lines changed: 149 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,6 @@ internal static partial class GitHelpers
2929

3030
private static readonly IDialogService _dialogService = Ioc.Default.GetRequiredService<IDialogService>();
3131

32-
private static readonly FetchOptions _fetchOptions = new()
33-
{
34-
Prune = true
35-
};
36-
3732
private static readonly PullOptions _pullOptions = new();
3833

3934
private static readonly string _clientId = AppLifecycleHelper.AppEnvironment is AppEnvironment.Dev
@@ -363,34 +358,39 @@ public static async Task FetchOrigin(string? repositoryPath, CancellationToken c
363358
using var repository = new Repository(repositoryPath);
364359
var signature = repository.Config.BuildSignature(DateTimeOffset.Now);
365360

366-
var remoteName = repository.Network.Remotes.FirstOrDefault()?.Name ?? "origin";
367-
368-
// Add Status Center Card
369-
var banner = StatusCenterHelper.AddCard_GitFetch(remoteName, ReturnResult.InProgress);
370-
var fsProgress = new StatusCenterItemProgressModel(banner.ProgressEventSource, enumerationCompleted: true, FileSystemStatusCode.InProgress);
371-
372-
// Link CancellationTokens
373-
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, banner.CancellationToken);
374-
var token = linkedCts.Token;
375-
376-
var fetchOptions = new FetchOptions
377-
{
378-
CredentialsProvider = CredentialsHandler,
379-
OnTransferProgress = (progress) =>
380-
{
381-
if (token.IsCancellationRequested)
382-
return false;
383-
384-
if (progress.TotalObjects > 0)
385-
{
386-
fsProgress.ItemsCount = progress.TotalObjects;
387-
fsProgress.SetProcessedSize(progress.ReceivedBytes);
388-
fsProgress.AddProcessedItemsCount(progress.ReceivedObjects);
389-
fsProgress.Report((int)((progress.ReceivedObjects / (double)progress.TotalObjects) * 100));
390-
}
391-
return true;
392-
}
393-
};
361+
var remoteName = repository.Network.Remotes.FirstOrDefault()?.Name ?? "origin";
362+
363+
// Add Status Center Card
364+
var banner = StatusCenterHelper.AddCard_GitFetch(remoteName, ReturnResult.InProgress);
365+
var fsProgress = new StatusCenterItemProgressModel(banner.ProgressEventSource, enumerationCompleted: true, FileSystemStatusCode.InProgress);
366+
367+
// Link CancellationTokens
368+
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, banner.CancellationToken);
369+
var token = linkedCts.Token;
370+
371+
var fetchOptions = new FetchOptions
372+
{
373+
CredentialsProvider = CredentialsHandler,
374+
OnTransferProgress = (progress) =>
375+
{
376+
if (token.IsCancellationRequested)
377+
return false;
378+
379+
// Ensure StatusCenter updates are dispatched to the UI thread
380+
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
381+
{
382+
if (progress.TotalObjects > 0)
383+
{
384+
fsProgress.ItemsCount = progress.TotalObjects;
385+
fsProgress.SetProcessedSize(progress.ReceivedBytes);
386+
fsProgress.AddProcessedItemsCount(progress.ReceivedObjects);
387+
fsProgress.Report((int)((progress.ReceivedObjects / (double)progress.TotalObjects) * 100));
388+
}
389+
});
390+
return true;
391+
},
392+
Prune = true // Restore Prune behavior as requested in PR review
393+
};
394394

395395
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
396396
{
@@ -403,7 +403,7 @@ public static async Task FetchOrigin(string? repositoryPath, CancellationToken c
403403
{
404404
token.ThrowIfCancellationRequested();
405405

406-
// Iterate remotes (though usually only one matters for progress, we'll fetch all)
406+
// Iterate remotes (though usually only one matters for progress, we'll fetch all)
407407
foreach (var remote in repository.Network.Remotes)
408408
{
409409
token.ThrowIfCancellationRequested();
@@ -417,7 +417,22 @@ public static async Task FetchOrigin(string? repositoryPath, CancellationToken c
417417
}
418418

419419
token.ThrowIfCancellationRequested();
420-
return GitOperationResult.Success;
420+
return GitOperationResult.Success;
421+
}
422+
catch (LibGit2SharpException e)
423+
{
424+
MainWindow.Instance.DispatcherQueue.TryEnqueue(async () =>
425+
{
426+
var dialog = new DynamicDialog(new DynamicDialogViewModel
427+
{
428+
TitleText = Strings.GitError.GetLocalizedResource(),
429+
SubtitleText = e.Message,
430+
CloseButtonText = Strings.Close.GetLocalizedResource(),
431+
DynamicButtons = DynamicDialogButtons.Cancel
432+
});
433+
await dialog.TryShowAsync();
434+
});
435+
return GitOperationResult.GenericError;
421436
}
422437
catch (Exception ex)
423438
{
@@ -427,10 +442,13 @@ public static async Task FetchOrigin(string? repositoryPath, CancellationToken c
427442
}
428443
});
429444

430-
StatusCenterViewModel.RemoveItem(banner);
431-
StatusCenterHelper.AddCard_GitFetch(
432-
remoteName,
433-
result == GitOperationResult.Success ? ReturnResult.Success : ReturnResult.Failed);
445+
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
446+
{
447+
StatusCenterViewModel.RemoveItem(banner);
448+
StatusCenterHelper.AddCard_GitFetch(
449+
remoteName,
450+
result == GitOperationResult.Success ? ReturnResult.Success : ReturnResult.Failed);
451+
});
434452

435453
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
436454
{
@@ -452,36 +470,40 @@ public static async Task PullOriginAsync(string? repositoryPath)
452470
if (signature is null)
453471
return;
454472

455-
var branchName = repository.Head.FriendlyName;
456-
// We use 'origin' as generic remote name for display, though pull might use upstream
457-
var remoteName = "origin";
458-
459-
// Add Status Center Card
460-
var banner = StatusCenterHelper.AddCard_GitPull(remoteName, branchName, ReturnResult.InProgress);
461-
var fsProgress = new StatusCenterItemProgressModel(banner.ProgressEventSource, enumerationCompleted: true, FileSystemStatusCode.InProgress);
462-
var token = banner.CancellationToken;
463-
464-
var pullOptions = new PullOptions
465-
{
466-
FetchOptions = new FetchOptions
467-
{
468-
CredentialsProvider = CredentialsHandler,
469-
OnTransferProgress = (progress) =>
470-
{
471-
if (token.IsCancellationRequested)
472-
return false;
473-
474-
if (progress.TotalObjects > 0)
475-
{
476-
fsProgress.ItemsCount = progress.TotalObjects;
477-
fsProgress.SetProcessedSize(progress.ReceivedBytes);
478-
fsProgress.AddProcessedItemsCount(progress.ReceivedObjects);
479-
fsProgress.Report((int)((progress.ReceivedObjects / (double)progress.TotalObjects) * 100));
480-
}
481-
return true;
482-
}
483-
}
484-
};
473+
var branchName = repository.Head.FriendlyName;
474+
// We use 'origin' as generic remote name for display, though pull might use upstream
475+
var remoteName = "origin";
476+
477+
// Add Status Center Card
478+
var banner = StatusCenterHelper.AddCard_GitPull(remoteName, branchName, ReturnResult.InProgress);
479+
var fsProgress = new StatusCenterItemProgressModel(banner.ProgressEventSource, enumerationCompleted: true, FileSystemStatusCode.InProgress);
480+
var token = banner.CancellationToken;
481+
482+
var pullOptions = new PullOptions
483+
{
484+
FetchOptions = new FetchOptions
485+
{
486+
CredentialsProvider = CredentialsHandler,
487+
OnTransferProgress = (progress) =>
488+
{
489+
if (token.IsCancellationRequested)
490+
return false;
491+
492+
// Ensure StatusCenter updates are dispatched to the UI thread
493+
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
494+
{
495+
if (progress.TotalObjects > 0)
496+
{
497+
fsProgress.ItemsCount = progress.TotalObjects;
498+
fsProgress.SetProcessedSize(progress.ReceivedBytes);
499+
fsProgress.AddProcessedItemsCount(progress.ReceivedObjects);
500+
fsProgress.Report((int)((progress.ReceivedObjects / (double)progress.TotalObjects) * 100));
501+
}
502+
});
503+
return true;
504+
}
505+
}
506+
};
485507

486508
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
487509
{
@@ -492,12 +514,42 @@ public static async Task PullOriginAsync(string? repositoryPath)
492514
{
493515
try
494516
{
495-
// LibGit2Sharp Pull doesn't take CancellationToken explicitly in all overloads, rely on callbacks
517+
// LibGit2Sharp Pull doesn't take CancellationToken explicitly in all overloads, rely on callbacks
496518
LibGit2Sharp.Commands.Pull(
497519
repository,
498520
signature,
499521
pullOptions);
500522
}
523+
catch (CheckoutConflictException)
524+
{
525+
MainWindow.Instance.DispatcherQueue.TryEnqueue(async () =>
526+
{
527+
var dialog = new DynamicDialog(new DynamicDialogViewModel
528+
{
529+
TitleText = Strings.GitError.GetLocalizedResource(),
530+
SubtitleText = Strings.GitError_UncommittedChanges.GetLocalizedResource(),
531+
CloseButtonText = Strings.Close.GetLocalizedResource(),
532+
DynamicButtons = DynamicDialogButtons.Cancel
533+
});
534+
await dialog.TryShowAsync();
535+
});
536+
return GitOperationResult.GenericError;
537+
}
538+
catch (LibGit2SharpException e)
539+
{
540+
MainWindow.Instance.DispatcherQueue.TryEnqueue(async () =>
541+
{
542+
var dialog = new DynamicDialog(new DynamicDialogViewModel
543+
{
544+
TitleText = Strings.GitError.GetLocalizedResource(),
545+
SubtitleText = e.Message,
546+
CloseButtonText = Strings.Close.GetLocalizedResource(),
547+
DynamicButtons = DynamicDialogButtons.Cancel
548+
});
549+
await dialog.TryShowAsync();
550+
});
551+
return GitOperationResult.GenericError;
552+
}
501553
catch (Exception ex)
502554
{
503555
return IsAuthorizationException(ex)
@@ -508,8 +560,11 @@ public static async Task PullOriginAsync(string? repositoryPath)
508560
return GitOperationResult.Success;
509561
});
510562

511-
StatusCenterViewModel.RemoveItem(banner);
512-
StatusCenterHelper.AddCard_GitPull(remoteName, branchName, result == GitOperationResult.Success ? ReturnResult.Success : ReturnResult.Failed);
563+
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
564+
{
565+
StatusCenterViewModel.RemoveItem(banner);
566+
StatusCenterHelper.AddCard_GitPull(remoteName, branchName, result == GitOperationResult.Success ? ReturnResult.Success : ReturnResult.Failed);
567+
});
513568

514569
if (result is GitOperationResult.AuthorizationError)
515570
{
@@ -530,11 +585,11 @@ public static async Task PullOriginAsync(string? repositoryPath)
530585

531586
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
532587
{
533-
IsExecutingGitAction = false;
534-
if (result == GitOperationResult.Success)
535-
{
536-
GitFetchCompleted?.Invoke(null, EventArgs.Empty);
537-
}
588+
IsExecutingGitAction = false;
589+
if (result == GitOperationResult.Success)
590+
{
591+
GitFetchCompleted?.Invoke(null, EventArgs.Empty);
592+
}
538593
});
539594
}
540595

@@ -564,13 +619,13 @@ public static async Task PushToOriginAsync(string? repositoryPath, string? branc
564619
if (banner.CancellationToken.IsCancellationRequested)
565620
return false; // Cancel
566621

567-
if (total > 0)
568-
{
569-
fsProgress.ItemsCount = total;
570-
fsProgress.SetProcessedSize(bytes);
571-
fsProgress.AddProcessedItemsCount(1); // Not accurate but shows activity
572-
fsProgress.Report((int)((current / (double)total) * 100));
573-
}
622+
if (total > 0)
623+
{
624+
fsProgress.ItemsCount = total;
625+
fsProgress.SetProcessedSize(bytes);
626+
fsProgress.AddProcessedItemsCount(1); // Not accurate but shows activity
627+
fsProgress.Report((int)((current / (double)total) * 100));
628+
}
574629
return true;
575630
}
576631
};
@@ -598,7 +653,7 @@ public static async Task PushToOriginAsync(string? repositoryPath, string? branc
598653
try
599654
{
600655
if (banner.CancellationToken.IsCancellationRequested)
601-
return GitOperationResult.GenericError; // Or Cancelled
656+
return GitOperationResult.GenericError; // Or Cancelled
602657

603658
repository.Network.Push(branch, options);
604659
}
@@ -620,22 +675,22 @@ public static async Task PushToOriginAsync(string? repositoryPath, string? branc
620675
_logger.LogWarning(ex.Message);
621676
}
622677

623-
// Remove InProgress Card
678+
// Remove InProgress Card
624679
StatusCenterViewModel.RemoveItem(banner);
625680

626681
// Add Result Card
627-
StatusCenterHelper.AddCard_GitPush(
628-
"origin",
629-
branchName,
630-
result == GitOperationResult.Success ? ReturnResult.Success : ReturnResult.Failed);
682+
StatusCenterHelper.AddCard_GitPush(
683+
"origin",
684+
branchName,
685+
result == GitOperationResult.Success ? ReturnResult.Success : ReturnResult.Failed);
631686

632687
MainWindow.Instance.DispatcherQueue.TryEnqueue(() =>
633688
{
634689
IsExecutingGitAction = false;
635-
if (result == GitOperationResult.Success)
636-
{
637-
GitFetchCompleted?.Invoke(null, EventArgs.Empty);
638-
}
690+
if (result == GitOperationResult.Success)
691+
{
692+
GitFetchCompleted?.Invoke(null, EventArgs.Empty);
693+
}
639694
});
640695
}
641696

0 commit comments

Comments
 (0)