Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix pull request update showing too many commits with multiple branches #22856

Merged
merged 8 commits into from Mar 9, 2023

Conversation

brechtvl
Copy link
Contributor

When the base repository contains multiple branches with the same commits as the base branch, pull requests can show a long list of commits already in the base branch as having been added.

What this is supposed to do is exclude commits already in the base branch. But the mechansim to do so assumed a commit only exists in a single branch. Now use git rev-list A B --not branchName instead of filtering commits afterwards.

The logic to detect if there was a force push also was wrong for multiple branches. If the old commit existed in any branch in the base repository it would assume there was no force push. Instead check if the old commit is an ancestor of the new commit.

When the base repository contains multiple branches with the same commits as
the base branch, pull requests can show a long list of commits already in the
base branch as having been added.

What this is supposed to do is exclude commits already in the base branch. But
the mechanism to do so assumed a commit only exists in a single branch. Now use
`git rev-list A B --not branchName` instead of filtering commits afterwards.

The logic to detect if there was a force push also was wrong for multiple
branches. If the old commit existed in any branch in the base repository it
would assume there was no force push. Instead check if the old commit is an
ancestor of the new commit.
@brechtvl
Copy link
Contributor Author

An example of this problem can be seen here:
https://projects.blender.org/blender/blender/pulls/104556

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 11, 2023
@codecov-commenter

This comment was marked as off-topic.

modules/git/repo_commit.go Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 11, 2023
@lunny lunny added the type/enhancement An improvement of existing functionality label Feb 12, 2023
@techknowlogick techknowlogick added this to the 1.20.0 milestone Feb 24, 2023
@lunny
Copy link
Member

lunny commented Mar 8, 2023

We have a function named IsForcePush in modules/repository/push.go. Looks like they are different git commands. But both work?

The IsBranch() test is no longer part of IsForcePush(), because the code path
this is called in already tests opts.IsBranch() and forced updates are not
just a thing in branches.
@brechtvl
Copy link
Contributor Author

brechtvl commented Mar 9, 2023

From what I can tell the commands are equivalent, with merge-base --is-ancestor being a more recent and convenient addition.

IsForcePush only affects trace logging currently, so not much risk breaking anything there. So I deduplicated and simplified the code.

modules/git/repo_commit.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 9, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 9, 2023
@jolheiser
Copy link
Member

🎺 🤖

@jolheiser jolheiser merged commit 8bdc0ac into go-gitea:main Mar 9, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 9, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 10, 2023
* giteaofficial/main:
  Improve squash merge commit author and co-author with private emails (go-gitea#22977)
  Fix broken Chroma CSS styles (go-gitea#23174)
  Add gradle samples in maven doc of packages (go-gitea#23374)
  Fix and move "Use this template" button (go-gitea#23398)
  [skip ci] Updated translations via Crowdin
  Add init file for Ubuntu (go-gitea#23362)
  Rename `canWriteUnit` to `canWriteProjects` (go-gitea#23386)
  Fix pull request update showing too many commits with multiple branches (go-gitea#22856)
  Fix incorrect NotFound conditions in org/projects.go (go-gitea#23384)
  Refactor merge/update git command calls (go-gitea#23366)
  Redirect to project again after editing it (go-gitea#23326)
  Add Gitea Community Code of Conduct (go-gitea#23188)
@brechtvl brechtvl deleted the fix-pr-too-many-commits branch March 11, 2023 11:29
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants