Exceptions during hg push should be errors

[rmunn]

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.

Fixes #366.

---

This change is 

[rmunn]

Note that this might change the output of any code that uses SIL.Windows.Forms.Progress.LogBox as the progress dialog, because LogBox writes a full exception stacktrace in its WriteException implementation, rather than just the exception message as the existing code was doing. If this matters, a possible fix would be WriteError(err.Message) instead of WriteException, but that would potentially lose useful debugging info that users might copy and paste into an email. So I chose to use WriteException here, but I'll change it if requested.

[github-actions]

Test Results

       8 files  ±0     333 suites  ±0   2h 27m 58s ⏱️ + 4m 57s
   989 tests +1     933 ✔️ +1    56 💤 ±0  0 ❌ ±0 
3 148 runs  +3  3 025 ✔️ +3  123 💤 ±0  0 ❌ ±0 

Results for commit 4b6df3d. ± Comparison against base commit 1ab24b9.

♻️ This comment has been updated with latest results.

[myieye]

@rmunn I refactored a tad:
You recorded the exception, but the SyncResults object still had Succeeded == true and didn't know about the exception, which didn't feel quite right.
So, I mimicked exception handling in PullFromOthers more closely. Essentially, I just let exceptions bubble all the way up to Synchronizer.SyncNow. Then all 3 "error things" happens:

• Succeeded is marked as false
• The result object gets the error
• The progress object gets the error

I also added a test to demonstrate that.

I kept the one fairly specific error message "Could not update the actual files after pushing to..." by wrapping it in an ApplicationException, which is pretty generic, but used heavily in the Execute method, which the try is wrapping. So, it seemed like a fine choice.

[jasonleenaylor]

So there is one thing this PR does not take into account that I think does matter.
Previously if a client tried to sync to both USB and a resumeable server a failure in the remote server sync would not stop the usb sync. Now a failed sync on one source would block the other attempts.

@jasonleenaylor reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on rmunn).

[myieye]

@jasonleenaylor

You're right, I didn't think about that.
But, it's not quite that simple. 🤔

It is already the case that a failure during the pull step will fail the entire sync.
All I've done is apply that same behavior to the push step.

So, the only difference now is this quite precise case:

• A sync is happening to multiple sources
• One of those sources is a server and it's listed BEFORE other sources
• Chorus CAN connect to the server
• The PULL from the server is successful
• The PUSH to the server is not successful
• => Then pushes will not be done/attempted to sources listed AFTER it.

That seems reasonable to me.
But, if you think that would negatively affect users we can revert to @rmunn's initial approach.

In summary, the options I see are:

1. Robin's initial implementation - Leave pull and push inconsistent:

• Failed pulls fail the whole sync and result in SyncResults.Succeeded == false
• Failed pushes do neither, but DO log exceptions to _progress. (this logging would be the only change to current behavior. The exception just really needs to be exposed to the caller in a clean way somehow)

(This would work fine for us and be the smallest change. It just seems semantically odd.)

2. My current implementation - Make pull and push consistent:

• Both failed pulls and pushes fail the whole sync
• They record SyncResults.Succeeded == false and "log" exceptions to both _progress and SyncResults.

[papeh]

I misinterpreted this name before reading the comment. Would FalseConnectableHttpRepositoryAddress or ConnectableButUnsyncableRepositoryAddress be clearer? But it's already a long name and is commented, so also fin as is.

[jasonleenaylor]

Send/Receive is both a collaboration and a backup feature for some users.
I agree it is a fairly specific edge case, but the impact of a user encountering that edge case and the type of user who might encounter it are worth considering.
Consider a user in a location with unreliable internet (the main reason that USB is still an option) trying to do Send/Receive with both resumable and usb options. They may find themselves having to disable the resumable sync in order to get it onto a USB stick as a backup or as a sneaker net collaboration. While that is an available workaround and seems fine for technical users it is a bigger hurdle for translators in the field who may not have as many computer skills.