From 0925bb5efa1dd4e03f361fda01bb934a0312573f Mon Sep 17 00:00:00 2001 From: Robin Munn Date: Thu, 26 Feb 2026 11:44:34 +0700 Subject: [PATCH 1/5] Exceptions during hg push should be errors The IProgress implementations have an ErrorEncountered flag that is set when their WriteException or WriteError methods are called. But PushToTarget was only calling WriteMessageWithColor, so if the only exception during a Send/Receive was happening during the hg push step, then ErrorEncountered was incorrectly still false. --- src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs b/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs index d69f1b36..85f75539 100644 --- a/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs +++ b/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs @@ -302,7 +302,8 @@ public void PushToTarget(string targetLabel, string targetUri) catch (Exception err) { _progress.WriteMessageWithColor("OrangeRed", - $"Could not send to {ServerSettingsModel.RemovePasswordForLog(targetUri)}{Environment.NewLine}{err.Message}"); + $"Could not send to {ServerSettingsModel.RemovePasswordForLog(targetUri)}"); + _progress.WriteException(err); } if (GetIsLocalUri(targetUri)) @@ -314,7 +315,8 @@ public void PushToTarget(string targetLabel, string targetUri) catch (Exception err) { _progress.WriteMessageWithColor("OrangeRed", - $"Could not update the actual files after a pushing to {ServerSettingsModel.RemovePasswordForLog(targetUri)}{Environment.NewLine}{err.Message}"); + $"Could not update the actual files after a pushing to {ServerSettingsModel.RemovePasswordForLog(targetUri)}"); + _progress.WriteException(err); } } } From b33648aca20456aa4387236390f22b2de61fff64 Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 28 Apr 2026 15:29:48 +0200 Subject: [PATCH 2/5] Gitignore new vs code c# extension generated files --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index d5a665fd..e85f7850 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ .idea/ .vs/ *.files +*.lscache *.mdb *.nupkg *.orig From 9d867f6ecc394a24bac6ae7d7e0adc47eac44fbf Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 28 Apr 2026 15:30:25 +0200 Subject: [PATCH 3/5] Add failed push test --- .../sync/Synchronizer.BadSituationTests.cs | 48 ++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs b/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs index c40af5ec..46a49fe2 100644 --- a/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs +++ b/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs @@ -4,6 +4,7 @@ using Chorus.merge; using Chorus.sync; using Chorus.Utilities; +using Chorus.VcsDrivers; using Chorus.VcsDrivers.Mercurial; using LibChorus.TestUtilities; using NUnit.Framework; @@ -55,6 +56,29 @@ public void Sync_HgrcInUseByOther_FailsGracefully() } } + [Test] + public void Sync_PushFails_ErrorEncounteredIsSet() + { + using (var sender = new RepositorySetup("sender")) + { + sender.AddAndCheckinFile("one.txt", "sender's content"); + + var options = new SyncOptions + { + DoMergeWithOthers = false, + DoPullFromOthers = false, + DoSendToOthers = true, + }; + options.RepositorySourcesToTry.Add(new PretendConnectableHttpRepositoryAddress()); + + var result = sender.SyncWithOptions(options); + Assert.That(result.Succeeded, Is.False); + Assert.That(result.ErrorEncountered, Is.Not.Null); + Assert.That(result.ErrorEncountered.Message, Does.Contain("Failed to send to")); + Assert.That(sender.GetProgressString(), Does.Contain("Failed to send to")); + } + } + [Test] public void Sync_ExceptionInMergeCode_LeftWith2HeadsAndErrorOutputToProgress() { @@ -518,5 +542,27 @@ public void Sync_NewFileWithNonAsciCharacters_FileAdded() } } + /// + /// Test double that bypasses HttpRepositoryPath's remote validation. + /// HttpRepositoryPath.CanConnect() validates the endpoint and aborts if invalid, preventing + /// sync operations from executing. This allows CanConnect() to return true while sync operations + /// fail, enabling tests of sync failure scenarios (push/pull/merge failures). + /// + private class PretendConnectableHttpRepositoryAddress : RepositoryAddress + { + public PretendConnectableHttpRepositoryAddress() : base("unknownname", "http://127.0.0.1:1/test-project", false) + { + } + + public override bool CanConnect(HgRepository localRepository, string projectName, IProgress progress) + { + return true; + } + + public override string GetPotentialRepoUri(string repoIdentifier, string projectName, IProgress progress) + { + return URI; + } + } } -} \ No newline at end of file +} From 4b6df3d93f7a714e169d5b6242e456bd27f757ce Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 28 Apr 2026 15:33:18 +0200 Subject: [PATCH 4/5] Fail sync on push exceptions --- .../VcsDrivers/Mercurial/HgRepository.cs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs b/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs index 85f75539..0f5eb695 100644 --- a/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs +++ b/src/LibChorus/VcsDrivers/Mercurial/HgRepository.cs @@ -294,17 +294,9 @@ public bool Pull(RepositoryAddress source, string targetUri) public void PushToTarget(string targetLabel, string targetUri) { - try - { - CheckAndUpdateHgrc(); - Execute(false, _mercurialTwoCompatible,SecondsBeforeTimeoutOnRemoteOperation, "push --new-branch " + GetProxyConfigParameterString(targetUri), SurroundWithQuotes(targetUri)); - } - catch (Exception err) - { - _progress.WriteMessageWithColor("OrangeRed", - $"Could not send to {ServerSettingsModel.RemovePasswordForLog(targetUri)}"); - _progress.WriteException(err); - } + CheckAndUpdateHgrc(); + Execute(false, _mercurialTwoCompatible, SecondsBeforeTimeoutOnRemoteOperation, + "push --new-branch " + GetProxyConfigParameterString(targetUri), SurroundWithQuotes(targetUri)); if (GetIsLocalUri(targetUri)) { @@ -314,9 +306,9 @@ public void PushToTarget(string targetLabel, string targetUri) } catch (Exception err) { - _progress.WriteMessageWithColor("OrangeRed", - $"Could not update the actual files after a pushing to {ServerSettingsModel.RemovePasswordForLog(targetUri)}"); - _progress.WriteException(err); + throw new ApplicationException( + $"Could not update the actual files after pushing to {ServerSettingsModel.RemovePasswordForLog(targetUri)}", + err); } } } From 9dfcc8f8440cc7412653f1ca27b8cc269f37203b Mon Sep 17 00:00:00 2001 From: Tim Haasdyk Date: Tue, 5 May 2026 11:34:30 +0200 Subject: [PATCH 5/5] Push to all targets even if some fail --- src/LibChorus/sync/Synchronizer.cs | 25 +++++++- .../sync/Synchronizer.BadSituationTests.cs | 62 +++++++++++++++++-- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/src/LibChorus/sync/Synchronizer.cs b/src/LibChorus/sync/Synchronizer.cs index c2173e30..31a00f05 100644 --- a/src/LibChorus/sync/Synchronizer.cs +++ b/src/LibChorus/sync/Synchronizer.cs @@ -251,16 +251,39 @@ public void SetIsOneOfDefaultSyncAddresses(RepositoryAddress address, bool enabl private void SendToOthers(HgRepository repo, List sourcesToTry, Dictionary connectionAttempt) { using var activity = LibChorusActivitySource.Value.StartActivity(); + List errors = new List(); + bool atLeastOneSucceeded = false; foreach (RepositoryAddress address in sourcesToTry) { ThrowIfCancelPending(); if (!address.IsReadOnly) { - SendToOneOther(address, connectionAttempt, repo); + try + { + SendToOneOther(address, connectionAttempt, repo); + atLeastOneSucceeded = true; + } + catch (UserCancelledException) + { + throw; + } + catch (Exception e) + { + _progress.WriteException(e); + _progress.WriteError(e.Message); + errors.Add(e); + } } } ThrowIfCancelPending(); + + // Only throw and fail the sync if every writable target failed; partial success still counts as success. + if (errors.Any() && !atLeastOneSucceeded) + { + var error = errors.Count == 1 ? errors.Single() : new AggregateException(errors); + throw new SynchronizationException(error, WhatToDo.CheckAddressAndConnection, "Failed to send changes"); + } } private void ThrowIfCancelPending() diff --git a/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs b/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs index 46a49fe2..41137d71 100644 --- a/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs +++ b/src/LibChorusTests/sync/Synchronizer.BadSituationTests.cs @@ -69,13 +69,65 @@ public void Sync_PushFails_ErrorEncounteredIsSet() DoPullFromOthers = false, DoSendToOthers = true, }; - options.RepositorySourcesToTry.Add(new PretendConnectableHttpRepositoryAddress()); + options.RepositorySourcesToTry.Add(new FailingConnectableHttpRepositoryAddress("test-repo")); var result = sender.SyncWithOptions(options); Assert.That(result.Succeeded, Is.False); Assert.That(result.ErrorEncountered, Is.Not.Null); - Assert.That(result.ErrorEncountered.Message, Does.Contain("Failed to send to")); - Assert.That(sender.GetProgressString(), Does.Contain("Failed to send to")); + Assert.That(result.ErrorEncountered.Message, Does.Contain("Failed to send changes")); + Assert.That(sender.GetProgressString(), Does.Contain("Failed to send to test-repo")); + } + } + + [Test] + public void Sync_FirstPushFails_OtherSourcesStillTried() + { + using (var sender = new RepositorySetup("sender")) + { + sender.AddAndCheckinFile("one.txt", "sender's content"); + + var options = new SyncOptions + { + DoMergeWithOthers = false, + DoPullFromOthers = false, + DoSendToOthers = true, + }; + options.RepositorySourcesToTry.Add(new FailingConnectableHttpRepositoryAddress("first-test-repo")); + options.RepositorySourcesToTry.Add(new FailingConnectableHttpRepositoryAddress("second-test-repo")); + + var result = sender.SyncWithOptions(options); + Assert.That(result.Succeeded, Is.False); + Assert.That(result.ErrorEncountered, Is.Not.Null); + // Both targets should have been attempted even though the first failed. + var progress = sender.GetProgressString(); + Assert.That(progress, Does.Contain("Failed to send to first-test-repo")); + Assert.That(progress, Does.Contain("Failed to send to second-test-repo")); + } + } + + [Test] + public void Sync_OneOfMultiplePushTargetsFails_SyncStillSucceeds() + { + using (var sender = new RepositorySetup("sender")) + { + sender.AddAndCheckinFile("one.txt", "sender's content"); + + using (var receiver = new RepositorySetup("receiver", sender)) + { + var options = new SyncOptions + { + DoMergeWithOthers = false, + DoPullFromOthers = false, + DoSendToOthers = true, + }; + options.RepositorySourcesToTry.Add(new FailingConnectableHttpRepositoryAddress("test-repo")); + options.RepositorySourcesToTry.Add(receiver.GetRepositoryAddress()); + + var result = sender.SyncWithOptions(options); + Assert.That(result.Succeeded, Is.True); + Assert.That(result.ErrorEncountered, Is.Null); + Assert.That(sender.GetProgressString(), Does.Contain("Failed to send to test-repo")); + } } } @@ -548,9 +600,9 @@ public void Sync_NewFileWithNonAsciCharacters_FileAdded() /// sync operations from executing. This allows CanConnect() to return true while sync operations /// fail, enabling tests of sync failure scenarios (push/pull/merge failures). /// - private class PretendConnectableHttpRepositoryAddress : RepositoryAddress + private class FailingConnectableHttpRepositoryAddress : RepositoryAddress { - public PretendConnectableHttpRepositoryAddress() : base("unknownname", "http://127.0.0.1:1/test-project", false) + public FailingConnectableHttpRepositoryAddress(string name) : base(name, "http://127.0.0.1:1/test-project", false) { }