Unable to merge ULT in some cases

[elsylambert]

When edits are made to the ult and then the ust - then saving and merging them at same time. On a repeated process, sometimes it only shows the ust as available to merge - not the ult.

• Testing the merge function in gWE, I found that on a fresh start, I edited UST and ULT on the same book, same chapter/verse and saved both the cards and did a merge to master.
• I continued editing another verse in ULT and UST and saved my changes. When I tried to merge again to master, ULT wont show in the merge resources checklist. That made me confused cause I edited and saved. The new changes can be found in my user branch, but it wont allow me to merge to master.

Testing Instructions

See: https://github.com/unfoldingWord/gateway-edit/pull/594#issue-2012429771

[PhotoNomad0]

@kintsoogi It looks like we are checking before the branch status has been updated. It seems we need to wait until later to check the merge status - checking after we get an updated file sha should be safe.

[birchamp]

@PhotoNomad0 @kintsoogi @elsylambert Does the merge update the branch after merge the way that tCC does?

[elsylambert]

Here, the merge updates the master branch and user branch automatically gets deleted, same like tCC.

On Tue, Sep 19, 2023 at 7:39 PM birchamp @.***> wrote:

@PhotoNomad0 https://github.com/PhotoNomad0 @kintsoogi https://github.com/kintsoogi @elsylambert https://github.com/elsylambert Does the merge update the branch after merge the way that tCC does?

— Reply to this email directly, view it on GitHub https://github.com/unfoldingWord/gateway-edit/issues/547#issuecomment-1726683609, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMLH7ZF3CJZOEKFSKIRRMQ3X3IULNANCNFSM6AAAAAA45H5EVM . You are receiving this because you were mentioned.Message ID: @.***>

[PhotoNomad0]

@PhotoNomad0 @kintsoogi @elsylambert Does the merge update the branch after merge the way that tCC does?

@birchamp It seems to be more of a timing issue - that GWE is checking merge status too soon. We don't continually poll the status, but we can delay when we check merge status after doing a save.

[PhotoNomad0]

@kintsoogi OK - I was having trouble reproducing it again, but when I did I saw that the mergeable state was false for the ULT, but not the UST. But when checked later it showed as mergable. Again it seems to be a timing issue. And I did not see the duplicate PR issue. @richmahn

[richmahn]

@PhotoNomad0 A branch becomes in "conflict" mode if it is being currently processed by Gitea at the time of your PR GET query. So yeah, a quick PR check will return "mergeable: false" but later will be true again.

[theNerd247]

@PhotoNomad0 @richmahn. @kintsoogi And I have spent some time digging into this issue. We've discovered the following:

1. we are making an API call to create a PR everytime we want to check the status of a particular PR. I'm not sure if this was done by design.
2. we are making duplicate API calls (to create a PR) everytime the content is saved - I believe only 1 call should be made.

[kintsoogi]

@theNerd247 @PhotoNomad0 @richmahn

We found a potential solution!

1. Only make the call to create a PR if one does not already exist.
2. Every other time we check the PR: a. GET the open pulls for the current user to get PR Number (Index) b. Use this index to perform the GET PR Based on Index c. Return this status

[theNerd247]

As we've been testing the above fix we have discovered that getting pre-existing PR statuses was not the solution. Running the following bash script yields the following results:

while true; do
  curl -s "https://qa.door43.org/api/v1/repos/unfoldingWord/en_tq/pulls/<pr-number>"\
  | jq '.mergeable'
done

null
null
null
null
null
null
null
null
null
null
true
true
true
true
true
true
false
false
true
true
true
true

where null implies the pr hasn't been created yet, and true means the PR is mergeable. NOTE how the PR is mergeable and then toggles to false and back. I've ran this test 3 time (each on a separate PR).

What's going on here is that we're repeatedly fetching the status for a PR (using a GET request) - and it appears that gitea is the culprit as it's changing the mereability flag from true to false back to true. I don't have enough knowledge of how gitea server works and hesitate to band-aid a solution (like waiting for the status to stabilize) until we at least find the cause of the problem.

@PhotoNomad0 @kintsoogi @birchamp any ideas?

[birchamp]

@richmahn we're really going to need your help with the above ^^

[richmahn]

@birchamp Thanks. Noah just alerted me to this on Discord in a DM. Still not sure how to remember to look in the Github inbox on here often. I should just do that.

[richmahn]

This is very interesting that it can't give you a reliable response.

[richmahn]

Mine keeps looping and says true, unless I save to my branch, which gives a correct false because a branch that is being processed for a commit to be added can't be merged, understandably (you'd lose the commit).

[richmahn]

Remember, mergeable flag does NOT necessarily mean it is not conflicting, but just if that user at that time can merge it. Unable to merge due to a conflict is only ONE reason mergeable would be false.

I confirmed this. I sent a simple PUT to https://qa.door43.org/api/v1/repos/unfoldingWord/en_tq/contents/README.md

With this payload (correct SHA):

{
  "branch": "richmahn-tc-create-1",
  "content": "SGVsbG8gV29ybGQh",
  "message": "test from postman",
  "author": {},
  "sha": "87cacddaa0a172ea47c2f25eca1b1ceecdf5bb72"
}

It properly set mergeable to false during the translation.

[richmahn]

But yes, I have to admit, while I realize this is how it works, that split second or two that it is false is really worthless. I'm going to dig into how the UI figures out a branch/PR is in conflict and if it would also give a false negative to the branch being in conflict. I ASUME if you were to actually go through with the merge, and at that very moment "mergeable" was false, it would just wait until that transaction is done to handle yours.

[richmahn]

Interestingly in the UI it says this instead when I hit the PR page at the same time I use the API to commit content:

On the next reload it is fine and nothing is broken. But in that PR page it only shows an old commit, not the new one. Then I reload and it shows everything.

[richmahn]

Ok, looking at the code, here are all the things that must be true for mergeable to be true:

pr.Status != PullRequestStatusChecking && pr.Status != PullRequestStatusConflict && pr.Status != PullRequestStatusError && !pr.IsWorkInProgress()

[richmahn]

I was wondering why the API GET response of a PR doesn't show files conflicting, I realized this was due this:

Given we don't expose which files are conflicting with base branch(which is gitea's internal check to determine conflicting PR's) they don't have any other way to determine if a PR is conflicting with base branch.

Ugh. Yet you can know if conflicting at least.

[richmahn]

@kintsoogi @theNerd247 @birchamp I find the only solution is if I expose the same info the UI had in the API, if there are conflicts and which files. Since we deal with public repos only anyway, best we do that. The properties will be:

has_conflicts: true|false conflicted_files: []string|null

Example:

[theNerd247]

Rich what would has_conflicts: true, files_conflicting: [] mean? Wouldn't we only need files_conflicting : []string(no null) field to encode the same information? (empty list is no conflicts, non-empty list indicates conflicts)

[richmahn]

Sure, could have just one, but figured it was also better to have a boolean there. We may find listing files is not the most reliable, for example, when a file is renamed or deleted. The PR model has these two properties.

[theNerd247]

Hmmm...my only concern is how we handle non-valid states. in the case I presented would the lack of conflicting files indicate a bug on gitea or would would the true be the bug because no files were presented?

[richmahn]

@theNerd247 So what do you suggest I add?

[richmahn]

Basically you would check the boolean has_conflicts property. If true, which would be valid state since this solely means there are conflicts. Then you tell the user it can't be merged. Optionally, if you want to tell the user which files had conflicts, you can list those from the conflicted_files property, but not necessary.

[theNerd247]

I'm making a note here regarding our research into the problem:

mergeable has an ambiguous meaning. The gitea API. Particularly mergeable's false means one of the following:

• conflict check in progress
• Has a conflict
• Received an error while being conflict checked.
• Is a work-in-progress pull request.

This is what is actually causing the "bug" as we are currently using mergeable to determine if a user's branch does/does not have conflicts with the master branch. A possible solution here is to update the gitea API to fetch the statuses directly (instead of evaluating them into a boolean)

[theNerd247]

Linking with #551 as both point to the same underlying issue.

[richmahn]

@theNerd247 @kintsoogi @PhotoNomad0

Here's what I have now with the latest fields added to the PR Response object:

https://qa.door43.org/api/v1/repos/unfoldingWord/en_tq/pulls/184

I now have "Status" and "ConflictedFiles" in the PR response object. If I run

while true; do curl -s "https://qa.door43.org/api/v1/repos/unfoldingWord/en_tq/pulls/184" | jq -r '{"mergeable": .mergeable, "status": .status, "conflicted_files": .conflicted_files}'; done

in my terminal, and then make a change to my tq branch, I get this:

...
{
  "mergeable": true,
  "status": "MERGEABLE",
  "conflicted_files": null
}
{
  "mergeable": false,
  "status": "CHECKING",
  "conflicted_files": null
}
{
  "mergeable": true,
  "status": "MERGEABLE",
  "conflicted_files": null
}
...

If I make a conflicting change to master at the same place:

...
{
  "mergeable": true,
  "status": "MERGEABLE",
  "conflicted_files": null
}
{
  "mergeable": false,
  "status": "CHECKING",
  "conflicted_files": null
}
{
  "mergeable": false,
  "status": "CHECKING",
  "conflicted_files": null
}
{
  "mergeable": false,
  "status": "CONFLICT",
  "conflicted_files": [
    "tq_MAT.tsv"
  ]
}
...

If I then make another edit in my TQ branch:

...
{
  "mergeable": false,
  "status": "CONFLICT",
  "conflicted_files": [
    "tq_MAT.tsv"
  ]
}
{
  "mergeable": false,
  "status": "CHECKING",
  "conflicted_files": [
    "tq_MAT.tsv"
  ]
}
{
  "mergeable": false,
  "status": "CONFLICT",
  "conflicted_files": [
    "tq_MAT.tsv"
  ]
}
...

So you can see status flips back to "CHECKING" and returns to "CONFLICT"

[richmahn]

@theNerd247 More thoughts on our conversation yesterday:

We need to keep in mind that Gitea, when it gets the actual merge request, it will still use its own logic to determine if it can be merged when the POST request is made, before doing the git command, and then report any error for the latter. So despite what we determine is mergeable or not, Gitea still has the final say and action....thus its "interpretation" trumps all others.

So going by what Gitea determines is mergeable in the end is something our apps need to know. The problem is that mergeable == false doesn't mean there is a failure, so we do have to also look to see if status == "CHECKING". (Github does this by setting mergeable to null! Not sure if better nor why Gitea, a Github clone, doesn't do that). And Gitea will wait to do an actual merge for status to go from "CHECKING" to any other status before performing hte merge, if we set the "merge_when_checks_succeed" when making the POST. https://git.door43.org/api/swagger#/repository/repoMergePullRequest

So remember when editing dcs-branch-merger that we are setting that to true.

[richmahn]

Also, it looks like when a merge request is sent and the status is "CHECKING" then it schedules an auto merge to be done in the background on the server when all checks are done and pass. I'm not sure what this will look like to the app / dcs-branch-merger! We may think it succeeded but didn't?

https://github.com/unfoldingWord/dcs/blob/50d6ddbda638dedc377ca2f829ae067099e571c2/routers/api/v1/repo/pull.go#L838

In this case of scheduling, it looks like it returns a status code of Created (201) as a response, instead of a 200. We may need to check for that and handle it accordingly?

[richmahn]

@theNerd247 Here is Gitea's ULTIMATE check for mergeable, which is called when a POST request is made to pulls/{index}/merge API endpoint.

https://github.com/unfoldingWord/dcs/blob/50d6ddbda638dedc377ca2f829ae067099e571c2/services/pull/check.go#L69

[richmahn]

@theNerd247 I know you're trying to make a robust solution, which is good, but I want to suggest to not make this too complicated. We are just trying to warn the user ahead of time if there are conflicts before hand so they don't think they can merge, and if there are, to tell them to contact their admin. It's better to error on them thinking it is mergeable (i.e. if status is checking, just assume there are no conflicts). We can do one last check when they are clicking on the "Merge my work" button to get the form, but ultimately the response from the server when the POST is made (the actual merge request) is where the magic/logic needs to happen if it was mergeable or not.

[theNerd247]

Thanks for your update Rich! Thank you for your suggestion I have been keeping this in mind.

Also, thank you for pointing out the code points where Gitea defines merge ability this helps a lot!

On Nov 15, 2023, at 10:46 AM, Richard Mahn @.***> wrote:

@theNerd247 https://github.com/theNerd247 I know you're trying to make a robust solution, which is good, but I want to suggest to not make this too complicated. We are just trying to warn the user if there are conflicts before hand so they don't think they can merge, and if there are, to tell them to contact their admin. It's better to error on them thinking it is mergeable (i.e. if status is checking, just assume there are no conflicts). We can do one last check when they are clicking on the "Merge my work" button to get the form, but ultimately the response from the server when the POST is made (the actual merge request) is where the magic/logic needs to happen if it was mergeable or not.

— Reply to this email directly, view it on GitHub https://github.com/unfoldingWord/gateway-edit/issues/547#issuecomment-1812781635, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG6ND7PC57SI6GB4RXV2NTYETPVVAVCNFSM6AAAAAA45H5EVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJSG44DCNRTGU. You are receiving this because you were mentioned.

[elsylambert]

Looks good in v2.2.0 build 7018555 QA. Merge my work shows the scripture resources in the list whenever there are changes that can be merged.

[danielklapp]

Agree, looks good to me. Tested with v2.2.0 build 7018555 QA.