Gitea: [Bug] Merged PRs have their commits and files changed metadata 'zeroed'... sometimes.
- Gitea version (or commit ref): v1.11.3
- Git version: 2.24.1
- Operating system: Ubuntu 18.04 (used as a Docker host for Gitea)
- Database (use
[x]):
- [ ] PostgreSQL
- [ ] MySQL
- [ ] MSSQL
- [x] SQLite
- Can you reproduce the bug at https://try.gitea.io:
- [ ] Yes (provide example URL)
- [x] No
- [ ] Not relevant
- Log gist: N/A
Description
As of recent (within the last month I'd estimate), many PRs have their commits and files changed counters zeroed out after merging. This happens most of the time _but weirdly not always_ on my instance now. Deleting the branch afterwards doesn't make seem to make a difference if that makes a difference.
I'm not sure how a lot of the internal Git plumbing works for Gitea so I haven't managed to take a look at the code directly. I know that #10618 seemed to be somewhat relevant - and the linked PR example in there does indeed show the behaviour below! I'm confused as to why this is shown for me as both branches definitely still exist.
Screenshots
jamesorlakin
All 31 comments
It should be related with #10618. @zeripath
lunny
on 19 Mar 2020
Nope. Merged PRs are displayed by a different set of code.
zeripath
on 19 Mar 2020
So the problem seems to be that the mergebase is incorrect - it's somehow ending up being the head!
zeripath
on 19 Mar 2020
This means that v128.go is incorrect.
The trouble is that:
pr.MergeBase, err = git.NewCommand("merge-base", "--", pr.MergedCommitID+"^", gitRefName).RunInDir(repoPath)
Appears to have selected the gitRefName if it's a parent of MergedCommitID instead of searching for the common root for the other parent.
zeripath
on 19 Mar 2020
Would you reckon this is a migration problem? This has been happening on new PRs straight after merging.
I tried running it in debug mode (to use breakpoints) with SQLite in VS Code on my Windows machine but hit issues around needing gcc setup properly. I may try and migrate the existing DB to MySQL before investigating further if I get time tomorrow 馃槂
jamesorlakin
on 19 Mar 2020
It's probably the same issue if something recalculates the mergebase.
zeripath
on 19 Mar 2020
We need to calculate the merge base from the other parents of the mergecommitid (if there are multiple parents)
git rev-list --parents -n 1 MERGE_COMMIT_ID
Will give the commit id followed by the parents. Can get the merge base from those.
Damn sorry for this bug the original code seemed to work in testing not sure why it didn't work in practice.
zeripath
on 20 Mar 2020
EDIT: Ignore me. It was the debugger... Printing it out to the console was fine.
jamesorlakin
on 20 Mar 2020
Aah I think I've gotcha, so this bug will appear if the merge commit created by a PR has a parent that's also a merge commit? That would explain it not always appearing!
jamesorlakin
on 20 Mar 2020
@jamesorlakin - ah I see how you're getting the problem of the randomly updating merge base.
We're back to the race between the checker and merger - which is mostly fixed in 1.12
--
Nope that can't happen because we check if it is an ancestor...
zeripath
on 21 Mar 2020
Are you sure you're definitely getting this after merging PRs? Could you check what the contenst of the version table is in your DB?
zeripath
on 21 Mar 2020
This instance (v1.11.3) is currently on DB version 118:
The MergeBase of the Pull model ends up being the commit that gitRefName points to:
It's very strange, and doesn't happen all the time. Some PRs are just fine after merging. I can't seem to figure out any consistent behaviour that would cause this - my theory about merge commits doesn't hold out, as we use a protected branch workflow so _every_ commit the branch points to is a merge commit from a previous PR...
jamesorlakin
on 23 Mar 2020
Have they been marked manually merged?
zeripath
on 23 Mar 2020
...Marked as manually merged? We just use the big green Merge Pull Request button - no rebasing or anything fancy... if that helps at all!
jamesorlakin
on 23 Mar 2020
In the database pull_request table the status column do the broken ones have status == 3.
There's a check goroutine that has almost zero locking in 1.11 - I've mostly fixed this in master (1.12) - that goes around checking if PRs have been merged. If the checker starts during the PR merging phase and runs at or around the same as the PR being merged - it can see the commit in the git repository and think that the commit has been merged manually. At which point I could imagine it might incorrectly update the MergeBase.
zeripath
on 23 Mar 2020
Aah right, indeed there is a status == 3 on a recent 'bad' one.
These two PRs were created and merged today. 556 contained one commit from a branch into dev, and 557 was another PR to merge that into aws which contained a commit and the merge commit from 556. No branches have been deleted at this stage.
jamesorlakin
on 23 Mar 2020
OK that means it is very likely to be the checker.
zeripath
on 23 Mar 2020
Aah okay, would you expect to see a lot of manually merged PRs? (what triggers it?)
For reference I have quite a few!
jamesorlakin
on 23 Mar 2020
OK so I think I've worked it out:
This is the checker from release/v1.11. As I've said before there is a race in this code - it's possible to run this as the PR is being merged or just after it is merged.
Line 203:
Calls TestPatch here:
Which updates the pr.MergeBase - but as I say above this can end up being after the PR is merged.
Finally the checker calls:
which will update the PR without checking if it is merged or not.
The situation is similar on 1.12 but happens less frequently because we're attempting to prevent some of the raceyness.
I think the answer is to change from pr.UpdateCols to:
// UpdateColsIfNotMerged updates specific fields of a pull request if it has not been merged
func (pr *PullRequest) UpdateColsIfNotMerged(cols ...string) error {
_, err := x.Where("id = ? AND has_merged = ?", pr.ID, false).Cols(cols...).Update(pr)
return err
}
would it be possible for any of you to test those PRs? I think that they should fix the problem.
zeripath
on 31 Mar 2020
I can try and test the backport by building a new Docker image. I'll try and report back tonight or tomorrow if it's worked. 馃憤
jamesorlakin
on 1 Apr 2020
7 PRs down and I've not seen a zeroed one yet. Thanks Zeripath!
jamesorlakin
on 1 Apr 2020
Still sorry that I broke it. The broken PRs will be fixed when the migration in #10786 runs - but you might be able to backport the migration manually or fix the broken PRs in the db.
In case you're interested. The correct git command for finding the merge base is: (With $MERGE_COMMIT_ID and $PR_ID as appropriate)
git merge-base $(git rev-list --parents -n 1 $MERGE_COMMIT_ID) refs/pull/$PR_ID/head
There's nothing to be sorry about, we all make small mistakes here and there. I'm very appreciative of all the work done in an open source project - voluntary work is very respectable and I can see it being a bit of a thankless job at times. Where our team doesn't pay for Gitea (and I'm unfortunately not somebody with financial power to sponsor/donate) I can't expect a support team dedicated to fix things for me 馃檪
That said I try and 'pay my debts' to the community that's helped me, so making PRs and generally improving Gitea is in my interest too. Win win!
Oh, and you've also taught me about the merge-base command and some of Gitea's pull logic. 鉂わ笍
jamesorlakin
on 2 Apr 2020
Hello friends, I found this problem seems like still happens in Gitea v1.11.4
My environment is:
- Gitea version: v1.11.4
- Git version: 2.23.0.windows.1
- Operating system: Windows Server 2008 R2 64bit
- Database: MySQL
_A recent pull request:_
_An old pull request:_
tanrui8765
on 17 Apr 2020
you have to manualy fix them - a gitea doctor task for this will be in 1.11.5
6543
on 17 Apr 2020
@tanrui8765 https://github.com/go-gitea/gitea/pull/10991
"Add non-default recalculate merge bases check/fixer to doctor"
6543
on 17 Apr 2020
ok, got it, thanks~~ @6543
tanrui8765
on 17 Apr 2020
@6543
Hello friend, I deployed the gitea version V1.11.5, and tried to use gitea doctor task to check this problem, the results was OK, but the problem still exist on our gitea repo.
Could you please tell me what to do to manually fix these problems? I'm so confused...
Many Thanks
tanrui8765
on 11 May 2020
./gitea doctor --all --fix @tanrui8765
6543
on 11 May 2020
Thank you very much @6543, it worked~
tanrui8765
on 12 May 2020
Related issues
BRMateus2
路
3Comments
lunny
路
3Comments
ghost
路
3Comments
thehowl
路
3Comments
Fastidious
路
3Comments
Most helpful comment
7 PRs down and I've not seen a zeroed one yet. Thanks Zeripath!