[Bug] Merged PRs have their commits and files changed metadata &#39;zeroed&#39;... sometimes. #5079

@zeripath commented on GitHub (Mar 19, 2020):

So the problem seems to be that the mergebase is incorrect - it's somehow ending up being the head!

@zeripath commented on GitHub (Mar 19, 2020): So the problem seems to be that the mergebase is incorrect - it's somehow ending up being the head!

GiteaMirror commented

@zeripath commented on GitHub (Mar 19, 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 commented on GitHub (Mar 19, 2020): This means that v128.go is incorrect. The trouble is that: ```go 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.

GiteaMirror commented

@jamesorlakin commented on GitHub (Mar 19, 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 commented on GitHub (Mar 19, 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 😃

GiteaMirror commented

@zeripath commented on GitHub (Mar 19, 2020):

It's probably the same issue if something recalculates the mergebase.

@zeripath commented on GitHub (Mar 19, 2020): It's probably the same issue if something recalculates the mergebase.

GiteaMirror commented

2025-11-02 06:13:40 -06:00

@zeripath commented on GitHub (Mar 19, 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 commented on GitHub (Mar 19, 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.

GiteaMirror commented

@jamesorlakin commented on GitHub (Mar 20, 2020):

EDIT: Ignore me. It was the debugger... Printing it out to the console was fine.

@jamesorlakin commented on GitHub (Mar 20, 2020): EDIT: Ignore me. It was the debugger... Printing it out to the console was fine.

GiteaMirror commented

2025-11-02 06:13:40 -06:00

@jamesorlakin commented on GitHub (Mar 20, 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 commented on GitHub (Mar 20, 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!

GiteaMirror commented

2025-11-02 06:13:40 -06:00

@zeripath commented on GitHub (Mar 20, 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 commented on GitHub (Mar 20, 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...

GiteaMirror commented

@zeripath commented on GitHub (Mar 20, 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 commented on GitHub (Mar 20, 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?

GiteaMirror commented

@jamesorlakin commented on GitHub (Mar 23, 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 commented on GitHub (Mar 23, 2020): This instance (v1.11.3) is currently on DB version 118: ![image](https://user-images.githubusercontent.com/7294642/77299549-8badfc00-6ce4-11ea-8fa0-9cbfde929c82.png) The `MergeBase` of the Pull model ends up being the commit that `gitRefName` points to: ![image](https://user-images.githubusercontent.com/7294642/77299685-bef08b00-6ce4-11ea-903a-1140057dfe51.png) ![image](https://user-images.githubusercontent.com/7294642/77299808-f2cbb080-6ce4-11ea-856b-65082ed4c07c.png) 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...

GiteaMirror commented

@zeripath commented on GitHub (Mar 23, 2020):

Have they been marked manually merged?

@zeripath commented on GitHub (Mar 23, 2020): Have they been marked manually merged?

GiteaMirror commented

@jamesorlakin commented on GitHub (Mar 23, 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 commented on GitHub (Mar 23, 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!

GiteaMirror commented

@zeripath commented on GitHub (Mar 23, 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 commented on GitHub (Mar 23, 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.

GiteaMirror commented

@jamesorlakin commented on GitHub (Mar 23, 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 commented on GitHub (Mar 23, 2020): Aah right, indeed there is a `status` == 3 on a recent 'bad' one. ![image](https://user-images.githubusercontent.com/7294642/77315282-316e6480-6cff-11ea-9173-fa2f1a79b6f3.png) 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.

GiteaMirror commented

@zeripath commented on GitHub (Mar 23, 2020):

OK that means it is very likely to be the checker.

@zeripath commented on GitHub (Mar 23, 2020): OK that means it is very likely to be the checker.

GiteaMirror commented

@jamesorlakin commented on GitHub (Mar 23, 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 commented on GitHub (Mar 23, 2020): Aah okay, would you expect to see a lot of manually merged PRs? (what triggers it?) For reference I have quite a few! ![image](https://user-images.githubusercontent.com/7294642/77317380-01c15b80-6d03-11ea-9416-804cb0fb2111.png)

GiteaMirror commented

139fc7cfee/services/pull/check.go (L187-L218)

@zeripath commented on GitHub (Mar 31, 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:

139fc7cfee/services/pull/check.go (L203)

Calls TestPatch here:

139fc7cfee/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:

139fc7cfee/services/pull/check.go (L211)

139fc7cfee/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 commented on GitHub (Mar 31, 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 } ```

GiteaMirror commented

@zeripath commented on GitHub (Mar 31, 2020):

would it be possible for any of you to test those PRs? I think that they should fix the problem.

@zeripath commented on GitHub (Mar 31, 2020): would it be possible for any of you to test those PRs? I think that they should fix the problem.

GiteaMirror commented

@jamesorlakin commented on GitHub (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 commented on GitHub (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. 👍

GiteaMirror commented

@jamesorlakin commented on GitHub (Apr 1, 2020):

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

@jamesorlakin commented on GitHub (Apr 1, 2020): 7 PRs down and I've not seen a zeroed one yet. Thanks Zeripath!

GiteaMirror commented

@zeripath commented on GitHub (Apr 1, 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 commented on GitHub (Apr 1, 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 ```

GiteaMirror commented

2025-11-02 06:13:44 -06:00

@jamesorlakin commented on GitHub (Apr 2, 2020):

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 commented on GitHub (Apr 2, 2020): 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. ❤️

GiteaMirror commented

@tanrui8765 commented on GitHub (Apr 17, 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 commented on GitHub (Apr 17, 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:_** ![image](https://user-images.githubusercontent.com/3466957/79574094-3d78f680-80f2-11ea-8383-f6ec570824c3.png) **_An old pull request:_** ![image](https://user-images.githubusercontent.com/3466957/79574154-54b7e400-80f2-11ea-9d2c-0c1a1fc2647d.png)

GiteaMirror commented

2025-11-02 06:13:44 -06:00

@6543 commented on GitHub (Apr 17, 2020):

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

@6543 commented on GitHub (Apr 17, 2020): you have to manualy fix them - a `gitea doctor task` for this will be in 1.11.5

GiteaMirror commented

2025-11-02 06:13:44 -06:00

@6543 commented on GitHub (Apr 17, 2020):

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

@6543 commented on GitHub (Apr 17, 2020): @tanrui8765 https://github.com/go-gitea/gitea/pull/10991 "Add non-default recalculate merge bases check/fixer to doctor"

GiteaMirror commented

@tanrui8765 commented on GitHub (Apr 17, 2020):

ok, got it, thanks~~ @6543

@tanrui8765 commented on GitHub (Apr 17, 2020): ok, got it, thanks~~ @6543

GiteaMirror commented

@tanrui8765 commented on GitHub (May 11, 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 commented on GitHub (May 11, 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 > ![image](https://user-images.githubusercontent.com/3466957/81569633-1d2b1780-93d2-11ea-94b6-07bb4217cda0.png) > > ![image](https://user-images.githubusercontent.com/3466957/81569793-52376a00-93d2-11ea-8909-7d3649e2349b.png) > > ![image](https://user-images.githubusercontent.com/3466957/81569741-4186f400-93d2-11ea-9b4a-26e32854d90e.png)

GiteaMirror commented

@6543 commented on GitHub (May 11, 2020):

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

@6543 commented on GitHub (May 11, 2020): `./gitea doctor --all --fix` @tanrui8765

GiteaMirror commented