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
- SQLite
Can you reproduce the bug at https://try.gitea.io:
- Yes (provide example URL)
- 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

About this issue

Original URL
State: closed
Created 4 years ago
Reactions: 1
Comments: 31 (27 by maintainers)

Commits related to this issue

Only update merge_base if not already merged Fix #10766 Signed-off-by: Andrew Thornton <art27@cantab.net> — committed to zeripath/gitea by zeripath 4 years ago
Only update merge_base if not already merged Fix #10766 Signed-off-by: Andrew Thornton <art27@cantab.net> — committed to zeripath/gitea by zeripath 4 years ago
Only update merge_base in checker if not already merged (#10908) Fix #10766 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.... — committed to go-gitea/gitea by zeripath 4 years ago
Only update merge_base if not already merged (#10909) * Only update merge_base if not already merged Fix #10766 Signed-off-by: Andrew Thornton <art27@cantab.net> * Prevent race in transfer p... — committed to go-gitea/gitea by zeripath 4 years ago

Most upvoted comments

7 PRs down and I’ve not seen a zeroed one yet. Thanks Zeripath!

jamesorlakin on Apr 1, 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 Apr 1, 2020

./gitea doctor --all --fix @tanrui8765

6543 on May 11, 2020

@tanrui8765 https://github.com/go-gitea/gitea/pull/10991
“Add non-default recalculate merge bases check/fixer to doctor”

6543 on Apr 17, 2020

you have to manualy fix them - a gitea doctor task for this will be in 1.11.5

6543 on Apr 17, 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

zeripath on Apr 1, 2020

OK so I think I’ve worked it out:

https://github.com/go-gitea/gitea/blob/139fc7cfee2b872a99a1866ded8bc1f9a43c4c20/services/pull/check.go#L187-L218

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:

https://github.com/go-gitea/gitea/blob/139fc7cfee2b872a99a1866ded8bc1f9a43c4c20/services/pull/check.go#L203

Calls TestPatch here:

https://github.com/go-gitea/gitea/blob/139fc7cfee2b872a99a1866ded8bc1f9a43c4c20/services/pull/patch.go#L71-L215

Which updates the pr.MergeBase - but as I say above this can end up being after the PR is merged.

Finally the checker calls:

https://github.com/go-gitea/gitea/blob/139fc7cfee2b872a99a1866ded8bc1f9a43c4c20/services/pull/check.go#L211

https://github.com/go-gitea/gitea/blob/139fc7cfee2b872a99a1866ded8bc1f9a43c4c20/services/pull/check.go#L40-L54

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
}

zeripath on Mar 31, 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 Mar 19, 2020

It should be related with #10618. @zeripath

lunny on Mar 19, 2020