sync: fix GetOutgoingRevisions to use draft phases

[marksvc]

• Overrides GetOutgoingRevisions from trying to use a remote
  repository url (which was empty), to looking at what commits are in
  draft phase.
• This enables our logging to start telling us what commits are being
  pushed.

---

This patch changes something that is called in the Sync process. But I'm comfortable enough with existing testing without marking it as Needs Testing.

---

This change is 

[devin-ai-integration]

Devin Review found 2 potential issues.

[devin-ai-integration]

🚩 Semantic shift: GetOutgoingRevisions now relies on hg phases rather than remote comparison

The new GetOutgoingRevisions override (JwtInternetSharedRepositorySource.cs:145-146) uses local Mercurial draft phases to determine outgoing revisions, rather than comparing with the remote repository. This is a fundamentally different approach from the base SharedRepositorySource.GetOutgoingRevisions. The comment at line 139-143 acknowledges this, explaining that the base implementation doesn't work because it needs remote repo access. The correctness of this approach depends on MarkSharedChangeSetsPublic (HgWrapper.cs:87) always being called after every pull and push to keep phases in sync. If phases ever get out of sync (e.g., manual hg operations, a failed push where MarkSharedChangeSetsPublic isn't reached), this could report incorrect outgoing revisions. The Push method at JwtInternetSharedRepositorySource.cs:105-132 marks tip public after pushing, but if the server accepts the push and then MarkSharedChangeSetsPublic fails, those changesets would remain draft and be re-reported as outgoing on next call.

---

Was this helpful? React with 👍 or 👎 to provide feedback.

[pmachapman]

I wonder whether the point this raises is valid? Should we perhaps be comparing draft revisions to outgoing revisions (by calling the base method), and then log this discrepancy if present? This might help use for future debugging. (I don't have an issue with your reimplementing this to use draft revisions - I agree with your rationale)

[marksvc]

One aspect that Devin may not be taking into account is that if we throw an exception, we roll back the local hg repository. And so

if the server accepts the push and then MarkSharedChangeSetsPublic fails, those changesets would remain draft and be re-reported as outgoing on next call.

is not a problem[1], for example (at least if MarkSharedChangeSetsPublic fails loudly, not quietly).

([1] Well, not a problem that the changes were not marked as public anyway :)

Another element that isn't obvious from the context is that the outgoing revisions information appears to be only informative, rather than influencing behaviour. It is mostly only called from ParatextData or SF in ParatextData VersionedText.ShareChanges() and its result written into a SendReceiveResult.RevisionsSent.

In ParatextData, RevisionsSent is read in a couple places, but none of which look influential. So it appears that whether our GetOutgoingRevisions does well or poorly only influences how good our ParatextService.ExplainSRResults() logging is. So it's not too high stakes.

IIRC, ParatextData takes into account multiple heads and compares large sets of commits, when comparing the local and remote to determine differences. A closer look at ParatextData InternetSharedRepositorySource.cs indicates that we might be successful using the REST API based GetOutgoingRevisions behaviour if we first call something that sets InternetSharedRepositorySource.baseRevisionsFinder, such as Push() or Pull() → GetBaseRevisions() → GetBaseRevisionsUsingNewAPI(). Hmm, I may be able to call Pull.

Should we perhaps be comparing draft revisions to outgoing revisions (by calling the base method), and then log this discrepancy if present? This might help use for future debugging.

Sure. I think I'd like to have a try at getting to the information by first calling Pull. But otherwise it may need some ParatextData modifications.

[codecov]

Codecov Report

❌ Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.32%. Comparing base (797889b) to head (0f9cf35).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/SIL.XForge.Scripture/Services/HgWrapper.cs	0.00%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3775      +/-   ##
==========================================
- Coverage   81.34%   81.32%   -0.02%     
==========================================
  Files         622      622              
  Lines       39202    39212      +10     
  Branches     6401     6377      -24     
==========================================
+ Hits        31889    31890       +1     
- Misses       6326     6348      +22     
+ Partials      987      974      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pmachapman]

@pmachapman reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 94 at r2 (raw file):

    public string[] GetDraftRevisions(string repositoryPath)
    {
        string ids = RunCommand(repositoryPath, "log --rev \"draft()\" --template \"{node}\n\"");

Is this supposed to be an escaped "\n", i.e. \\n?

On Windows I had to run this from the command line:

C:\My Paratext 9 Projects\TDT>hg log --rev "draft()" --template "{node}\n"

Interestingly, running this command for projects in "C:\My Paratext 9 Projects" returned all commits in the project as draft, while on SF it ran correctly. Update: After having written this, I now see your comment below "Distinguishing between public and draft commit phase is not something ParatextData uses."

Code quote:

\n

---

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 97 at r2 (raw file):

        return
        [
            .. ids.Split(["\n"], StringSplitOptions.RemoveEmptyEntries)

You could simplify this to:

.. ids.Split('\n', StringSplitOptions.RemoveEmptyEntries)

Code quote:

.. ids.Split(["\n"], StringSplitOptions.RemoveEmptyEntries)

[pmachapman]

I wonder whether the point this raises is valid? Should we perhaps be comparing draft revisions to outgoing revisions (by calling the base method), and then log this discrepancy if present? This might help use for future debugging. (I don't have an issue with your reimplementing this to use draft revisions - I agree with your rationale)

[pmachapman]

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

[marksvc]

I'm noticing some interesting behaviour that I want to look closer at before merging this.

[marksvc]

Oh, I see it did not publish my messages. Trying again.

@marksvc made 4 comments.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on marksvc).

---

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 94 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Is this supposed to be an escaped "\n", i.e. \\n?

On Windows I had to run this from the command line:

C:\My Paratext 9 Projects\TDT>hg log --rev "draft()" --template "{node}\n"

Interestingly, running this command for projects in "C:\My Paratext 9 Projects" returned all commits in the project as draft, while on SF it ran correctly. Update: After having written this, I now see your comment below "Distinguishing between public and draft commit phase is not something ParatextData uses."

Yes, it should have been passing \n as part of the argument for hg to process, rather than passing a newline character. Good catch.

Interesting. I hadn't really thought about whether the commits would all be draft or public in repos that PT is managing (i.e. not that SF is managing).

I changed to use a raw string literal. And another place in the same file that was doing the same thing.

In bash, if I pass a newline character rather than \n to hg in the template, I see that it is working the same way as \n.

I don't like ending the string with a space (to separate the in-string double quote from the end-of-string terminator). But the way the command is processed by ParatextData, it works out okay.

---

src/SIL.XForge.Scripture/Services/HgWrapper.cs line 97 at r2 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

You could simplify this to:

.. ids.Split('\n', StringSplitOptions.RemoveEmptyEntries)

Done.

[marksvc]

One aspect that Devin may not be taking into account is that if we throw an exception, we roll back the local hg repository. And so

if the server accepts the push and then MarkSharedChangeSetsPublic fails, those changesets would remain draft and be re-reported as outgoing on next call.

is not a problem[1], for example (at least if MarkSharedChangeSetsPublic fails loudly, not quietly).

([1] Well, not a problem that the changes were not marked as public anyway :)

Another element that isn't obvious from the context is that the outgoing revisions information appears to be only informative, rather than influencing behaviour. It is mostly only called from ParatextData or SF in ParatextData VersionedText.ShareChanges() and its result written into a SendReceiveResult.RevisionsSent.

In ParatextData, RevisionsSent is read in a couple places, but none of which look influential. So it appears that whether our GetOutgoingRevisions does well or poorly only influences how good our ParatextService.ExplainSRResults() logging is. So it's not too high stakes.

IIRC, ParatextData takes into account multiple heads and compares large sets of commits, when comparing the local and remote to determine differences. A closer look at ParatextData InternetSharedRepositorySource.cs indicates that we might be successful using the REST API based GetOutgoingRevisions behaviour if we first call something that sets InternetSharedRepositorySource.baseRevisionsFinder, such as Push() or Pull() → GetBaseRevisions() → GetBaseRevisionsUsingNewAPI(). Hmm, I may be able to call Pull.

Should we perhaps be comparing draft revisions to outgoing revisions (by calling the base method), and then log this discrepancy if present? This might help use for future debugging.

Sure. I think I'd like to have a try at getting to the information by first calling Pull. But otherwise it may need some ParatextData modifications.