#7727 - feat: improved performances when checking for conflicts on pull requests - forgejo/forgejo - Codeberg.org

#7727 - feat: improved performances when checking for conflicts on pull requests - forgejo/forgejo - Codeberg.org
forgejo
forgejo
Fork
You've already forked forgejo
737
Code
Issues
1.3k
Pull requests
156
Projects
Releases
100
Packages
Activity
Actions
88
feat: improved performances when checking for conflicts on pull requests
#7727
Merged
Gusted
merged 1 commit from
gusted/forgejo-test-pr
into
forgejo
2025-05-16 14:40:43 +02:00
AGit
Conversation
18
Commits
Files changed
13
+630
-141
Gusted
commented
2025-04-29 23:15:21 +02:00
Owner
Copy link
testPatch
is a function that is called to test a pull request and determine the state of the pull request. Checking for merge conflicts, check if the diff is empty and if the pull request modifies any protected files.
The checking for merge conflict and if the diff is empty used git commands that relied on a working tree to correctly functions. Forgejo store repositories in a bare format which do not contain a working tree. This means that a temporary copy was created every time a pull request had to be re-checked and for large repositories involving quite some I/O interaction.
This patch adjusts those codepaths to instead use newer Git plumbing commands that work without requiring a work tree and can thus be used directly on the bare repository. The merge conflict is now done via
git-merge-tree(1)
and checking if the diff is empty is done via
git-diff-tree(1)
If the function is called to test a patch where the head and base repository are not the same, then
Git alternate
is used to make the head commit available in the base repository, this done on a per git command basis via the
GIT_ALTERNATE_OBJECT_DIRECTORIES
environment.
As far as I can understand the documentation and the existing code, there's no edge case that the new code cannot handle. It also results in a cleaner codepath, as the existing code did a lot of checking and merging in a more traditional approach that required a lot of (parsing) code, while the new code offloads this to git and has trivial parser of the output.
Resolves
forgejo/forgejo#7701
Added exhaustive integration testing.
Release notes
Features
PR
: improved performances when checking for conflicts on pull requests
- `testPatch` is a function that is called to test a pull request and determine the state of the pull request. Checking for merge conflicts, check if the diff is empty and if the pull request modifies any protected files.
- The checking for merge conflict and if the diff is empty used git commands that relied on a working tree to correctly functions. Forgejo store repositories in a bare format which do not contain a working tree. This means that a temporary copy was created every time a pull request had to be re-checked and for large repositories involving quite some I/O interaction.
- This patch adjusts those codepaths to instead use newer Git plumbing commands that work without requiring a work tree and can thus be used directly on the bare repository. The merge conflict is now done via [`git-merge-tree(1)`](https://git-scm.com/docs/git-merge-tree/) and checking if the diff is empty is done via [`git-diff-tree(1)`](https://git-scm.com/docs/git-diff-tree).
- If the function is called to test a patch where the head and base repository are not the same, then [Git alternate](https://git-scm.com/docs/gitglossary#Documentation/gitglossary.txt-aiddefalternateobjectdatabaseaalternateobjectdatabase) is used to make the head commit available in the base repository, this done on a per git command basis via the `GIT_ALTERNATE_OBJECT_DIRECTORIES` environment.
- As far as I can understand the documentation and the existing code, there's no edge case that the new code cannot handle. It also results in a cleaner codepath, as the existing code did a lot of checking and merging in a more traditional approach that required a lot of (parsing) code, while the new code offloads this to git and has trivial parser of the output.
- Resolves forgejo/forgejo#7701
- Added exhaustive integration testing.

## Release notes

- Features
- [PR](https://codeberg.org/forgejo/forgejo/pulls/7727): improved performances when checking for conflicts on pull requests

❤️
Gusted
added the
enhancement/feature
test
needed
forgejo/scaling
labels
2025-04-29 23:15:44 +02:00
Gusted
referenced this pull request
2025-04-29 23:21:25 +02:00
feat: run actions against merge result
#4734
Gusted
referenced this pull request
2025-04-30 12:48:50 +02:00
bug: conflict checking when pushing is proportional to the number of open pull requests
#7701
Gusted
referenced this pull request
2025-04-30 13:07:34 +02:00
bug: conflict checking when pushing is proportional to the number of open pull requests
#7701
Gusted
force-pushed gusted/forgejo-test-pr from
044bbe1e36
Some checks failed
testing / frontend-checks (pull_request)
Successful in 55s
Details
issue-labels / backporting (pull_request_target)
Has been skipped
Details
issue-labels / cascade (pull_request_target)
Has been skipped
Details
testing / backend-checks (pull_request)
Successful in 2m58s
Details
testing / test-e2e (pull_request)
Successful in 2m41s
Details
testing / test-unit (pull_request)
Successful in 5m6s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 1m47s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 1m48s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 1m48s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 1m48s
Details
testing / test-mysql (pull_request)
Successful in 18m29s
Details
testing / test-sqlite (pull_request)
Successful in 20m32s
Details
testing / test-pgsql (pull_request)
Successful in 23m38s
Details
testing / security-check (pull_request)
Successful in 1m5s
Details
requirements / merge-conditions (pull_request)
Failing after 2s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
to
752e85c7bb
Some checks failed
testing / frontend-checks (pull_request)
Successful in 46s
Details
testing / backend-checks (pull_request)
Failing after 1m35s
Details
testing / test-unit (pull_request)
Has been skipped
Details
testing / test-e2e (pull_request)
Has been skipped
Details
testing / test-mysql (pull_request)
Has been skipped
Details
testing / test-pgsql (pull_request)
Has been skipped
Details
testing / test-sqlite (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redis) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (valkey) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (garnet) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redict) (pull_request)
Has been skipped
Details
testing / security-check (pull_request)
Has been skipped
Details
issue-labels / backporting (pull_request_target)
Has been skipped
Details
issue-labels / cascade (pull_request_target)
Has been skipped
Details
requirements / merge-conditions (pull_request)
Successful in 1s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
2025-05-02 01:58:20 +02:00
Compare
Gusted
added
test
present
and removed
test
needed
labels
2025-05-02 02:01:56 +02:00
Gusted
changed title from
WIP: feat: PR testing in bare repository
to
feat: Patch testing on bare repository
2025-05-02 02:02:15 +02:00
Gusted
force-pushed gusted/forgejo-test-pr from
752e85c7bb
Some checks failed
testing / frontend-checks (pull_request)
Successful in 46s
Details
testing / backend-checks (pull_request)
Failing after 1m35s
Details
testing / test-unit (pull_request)
Has been skipped
Details
testing / test-e2e (pull_request)
Has been skipped
Details
testing / test-mysql (pull_request)
Has been skipped
Details
testing / test-pgsql (pull_request)
Has been skipped
Details
testing / test-sqlite (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redis) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (valkey) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (garnet) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redict) (pull_request)
Has been skipped
Details
testing / security-check (pull_request)
Has been skipped
Details
issue-labels / backporting (pull_request_target)
Has been skipped
Details
issue-labels / cascade (pull_request_target)
Has been skipped
Details
requirements / merge-conditions (pull_request)
Successful in 1s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
to
51d3a7670d
Some checks failed
requirements / merge-conditions (pull_request)
Successful in 1s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
testing / frontend-checks (pull_request)
Successful in 56s
Details
testing / backend-checks (pull_request)
Failing after 2m33s
Details
testing / test-unit (pull_request)
Has been skipped
Details
testing / test-e2e (pull_request)
Has been skipped
Details
testing / test-mysql (pull_request)
Has been skipped
Details
testing / test-pgsql (pull_request)
Has been skipped
Details
testing / test-sqlite (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redis) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (valkey) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (garnet) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redict) (pull_request)
Has been skipped
Details
testing / security-check (pull_request)
Has been skipped
Details
2025-05-02 02:02:33 +02:00
Compare
Gusted
force-pushed gusted/forgejo-test-pr from
51d3a7670d
Some checks failed
requirements / merge-conditions (pull_request)
Successful in 1s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
testing / frontend-checks (pull_request)
Successful in 56s
Details
testing / backend-checks (pull_request)
Failing after 2m33s
Details
testing / test-unit (pull_request)
Has been skipped
Details
testing / test-e2e (pull_request)
Has been skipped
Details
testing / test-mysql (pull_request)
Has been skipped
Details
testing / test-pgsql (pull_request)
Has been skipped
Details
testing / test-sqlite (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redis) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (valkey) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (garnet) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redict) (pull_request)
Has been skipped
Details
testing / security-check (pull_request)
Has been skipped
Details
to
f049eab73f
All checks were successful
testing / frontend-checks (pull_request)
Successful in 1m34s
Details
testing / backend-checks (pull_request)
Successful in 3m31s
Details
testing / test-e2e (pull_request)
Successful in 4m30s
Details
testing / test-unit (pull_request)
Successful in 7m17s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 2m31s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 2m34s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 2m30s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m30s
Details
testing / test-mysql (pull_request)
Successful in 20m28s
Details
testing / test-sqlite (pull_request)
Successful in 23m53s
Details
testing / test-pgsql (pull_request)
Successful in 26m19s
Details
testing / security-check (pull_request)
Successful in 1m12s
Details
requirements / merge-conditions (pull_request)
Successful in 1s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
2025-05-02 02:09:02 +02:00
Compare
Gusted
changed title from
feat: Patch testing on bare repository
to
WIP: feat: Patch testing on bare repository
2025-05-02 02:09:09 +02:00
Gusted
force-pushed gusted/forgejo-test-pr from
f049eab73f
All checks were successful
testing / frontend-checks (pull_request)
Successful in 1m34s
Details
testing / backend-checks (pull_request)
Successful in 3m31s
Details
testing / test-e2e (pull_request)
Successful in 4m30s
Details
testing / test-unit (pull_request)
Successful in 7m17s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 2m31s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 2m34s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 2m30s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m30s
Details
testing / test-mysql (pull_request)
Successful in 20m28s
Details
testing / test-sqlite (pull_request)
Successful in 23m53s
Details
testing / test-pgsql (pull_request)
Successful in 26m19s
Details
testing / security-check (pull_request)
Successful in 1m12s
Details
requirements / merge-conditions (pull_request)
Successful in 1s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
to
c95317df8c
All checks were successful
testing / frontend-checks (pull_request)
Successful in 58s
Details
testing / backend-checks (pull_request)
Successful in 3m24s
Details
testing / test-e2e (pull_request)
Successful in 3m3s
Details
testing / test-unit (pull_request)
Successful in 5m57s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 2m13s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m13s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 2m17s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 2m16s
Details
testing / test-mysql (pull_request)
Successful in 20m56s
Details
testing / test-sqlite (pull_request)
Successful in 25m44s
Details
testing / test-pgsql (pull_request)
Successful in 28m45s
Details
testing / security-check (pull_request)
Successful in 1m31s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
requirements / merge-conditions (pull_request)
Successful in 2s
Details
2025-05-02 16:17:38 +02:00
Compare
n0toose
reviewed
2025-05-02 16:31:09 +02:00
tests/integration/patch_status_test.go
@ -0,0 +39,4 @@
Files
optional
Some
files_service
ChangeRepoFile
Operation
"create"
TreePath
".spokeperson"
n0toose
commented
2025-05-02 16:31:09 +02:00
Member
Copy link
please keep this typo <3
please keep this typo <3
Gusted
marked this conversation as resolved
n0toose
reviewed
2025-05-02 16:32:41 +02:00
tests/integration/patch_status_test.go
@ -0,0 +66,4 @@
// Normal pull request, should be mergeable.
Run
"Normal"
func
testing
require
NoError
git
NewCommand
Context
"switch"
"-c"
"normal"
AddDynamicArguments
repo
DefaultBranch
Run
git
RunOpts
Dir
dstPath
require
NoError
os
WriteFile
filepath
Join
dstPath
"CONTACT"
byte
"n0toose@example.com"
o600
n0toose
commented
2025-05-02 16:32:41 +02:00
Member
Copy link
10/10 no notes
10/10 no notes
Gusted
marked this conversation as resolved
Gusted
changed title from
WIP: feat: Patch testing on bare repository
to
feat: Patch testing on bare repository
2025-05-02 19:14:25 +02:00
Gusted
requested review from
earl-warren
2025-05-02 19:14:31 +02:00
Gusted
commented
2025-05-02 19:18:34 +02:00
Author
Owner
Copy link
@earl-warren
its quite a daunting pull request, it might be easier to review this with split view enabled in the diff. The old codepath retains its code comments and the new codepath should have sufficient code comments to explain its intended behavior. The testing is exhaustive but does not really tell a lot about the code, at best it explains the control flow of
GetHeadRefForTestPatch
@earl-warren its quite a daunting pull request, it might be easier to review this with split view enabled in the diff. The old codepath retains its code comments and the new codepath should have sufficient code comments to explain its intended behavior. The testing is exhaustive but does not really tell a lot about the code, at best it explains the control flow of `GetHeadRefForTestPatch`.
earl-warren
requested changes
2025-05-03 09:11:51 +02:00
Dismissed
earl-warren
left a comment
Copy link
I'm not done but I found one obvious and simple change that is required. Plus I have a question
I'm not done but I found one obvious and simple change that is required. Plus I have a question 😁
services/pull/patch.go
@ -291,0 +390,4 @@
// can run in a base repository. It returns the object ID of the merge tree and
// any conflicted files.
func
MergeTree
ctx
context
Context
gitRepo
git
Repository
base
ours
theirs
string
env
string
string
string
error
cmd
:=
git
NewCommand
ctx
"merge-tree"
"--write-tree"
"-z"
"--name-only"
"--no-messages"
earl-warren
commented
2025-05-03 09:05:15 +02:00
Contributor
Copy link
Note to self
Note to self https://git-scm.com/docs/git-merge-tree/2.38.0
earl-warren
marked this conversation as resolved
services/pull/patch.go
@ -291,0 +391,4 @@
// any conflicted files.
func
MergeTree
ctx
context
Context
gitRepo
git
Repository
base
ours
theirs
string
env
string
string
string
error
cmd
:=
git
NewCommand
ctx
"merge-tree"
"--write-tree"
"-z"
"--name-only"
"--no-messages"
if
git
CheckGitVersionAtLeast
"2.40"
==
nil
earl-warren
commented
2025-05-03 08:56:44 +02:00
Contributor
Copy link
why not raise the min version to 2.40 to get rid of this if?
why not raise the min version to 2.40 to get rid of this if?

Gusted
commented
2025-05-04 14:26:06 +02:00
Author
Owner
Copy link
I am looking at Codeberg's git version, which is 2.39.5 - I'd prefer to get something in that also benefits Codeberg. Also, as far as I can see this is only a small optimization to not recalculate the merge base, we already did that a few moments before.
I am looking at Codeberg's git version, which is 2.39.5 - I'd prefer to get something in that also benefits Codeberg. Also, as far as I can see this is only a small optimization to not recalculate the merge base, we already did that a few moments before.
Gusted
marked this conversation as resolved
services/pull/patch.go
@ -291,0 +406,4 @@
// 1. Clean merge and the output is NUL
// 2. Merge conflict and the output is NULNUL
treeOID
conflictedFileInfo
:=
strings
Cut
stdout
String
"\x00"
if
len
conflictedFileInfo
==
earl-warren
commented
2025-05-03 09:11:10 +02:00
Contributor
Copy link
Do NOT interpret an empty Conflicted file info list as a clean merge; check the exit status. A merge can have conflicts without having individual files conflict (there are a few types of directory rename conflicts that fall into this category, and others might also be added in the future).

> Do NOT interpret an empty Conflicted file info list as a clean merge; check the exit status. A merge can have conflicts without having individual files conflict (there are a few types of directory rename conflicts that fall into this category, and others might also be added in the future).
Gusted
commented
2025-05-04 14:32:21 +02:00
Author
Owner
Copy link
I was aware of this and I have zero clue how to convey such a merge conflict ('Git knows there is a merge conflict but we have no clue where, goodluck!' is not user-friendly). I was not able to reproduce such a scenario, therefore adding tests for it makes it not trivial. The way how the current code handles such conflicts is also unclear to me. I will look into the Git test suite if they have a specific scenario.
I was aware of this and I have zero clue how to convey such a merge conflict ('Git knows there is a merge conflict but we have no clue where, goodluck!' is not user-friendly). I was not able to reproduce such a scenario, therefore adding tests for it makes it not trivial. The way how the current code handles such conflicts is also unclear to me. I will look into the Git test suite if they have a specific scenario.
Gusted
commented
2025-05-04 16:49:40 +02:00
Author
Owner
Copy link
does not suggest that Git themselves has a testcase for this specific case.
earl-warren
commented
2025-05-04 19:21:09 +02:00
Contributor
Copy link
I think the recommended way would be to test if the exit status is 1?
I think the recommended way would be to test if the exit status is 1?

Gusted
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 10:50:16 +02:00
tests/integration/patch_status_test.go
@ -0,0 +84,4 @@
defer
tests
PrintCurrentTest
require
NoError
git
NewCommand
Context
"push"
"fork"
"HEAD:normal"
Run
git
RunOpts
Dir
dstPath
testPullCreateDirectly
session
repo
OwnerName
repo
Name
repo
DefaultBranch
forkRepo
OwnerName
forkRepo
Name
"normal"
"across repo normal"
earl-warren
commented
2025-05-03 10:50:16 +02:00
Contributor
Copy link
This is a degraded case as there is no commit in the PR. Or do I read this incorrectly?
This is a degraded case as there is no commit in the PR. Or do I read this incorrectly?
Gusted
commented
2025-05-04 14:45:26 +02:00
Author
Owner
Copy link
There is, see L69-L71
There is, see L69-L71
Gusted
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 10:51:59 +02:00
tests/integration/patch_status_test.go
@ -0,0 +28,4 @@
func
TestPatchStatus
testing
onGiteaRun
func
testing
url
URL
earl-warren
commented
2025-05-03 10:51:59 +02:00
Contributor
Copy link
If you loop over mocking
SupportGitMergeTree
to be false (it will be true as the CI ensure git >= 2.42), it should cover the matching codepath as well. Or is there a reason why it would behave differently?
If you loop over mocking `SupportGitMergeTree` to be false (it will be true as the CI ensure git >= 2.42), it should cover the matching codepath as well. Or is there a reason why it would behave differently?
Gusted
commented
2025-05-04 14:34:21 +02:00
Author
Owner
Copy link
I am looking at solutions on how to do Git version mocking for
forgejo/discussions#324
, I want to avoid individual tests only doing such mocking as it might miss cases where other codepaths are also affected by the changed code.
I am looking at solutions on how to do Git version mocking for https://codeberg.org/forgejo/discussions/issues/324, I want to avoid individual tests only doing such mocking as it might miss cases where other codepaths are also affected by the changed code.
earl-warren
commented
2025-05-04 20:16:05 +02:00
Contributor
Copy link
I agree it would be better. The CI could install multiple Git versions so the test can pick them depending.
I agree it would be better. The CI could install multiple Git versions so the test can pick them depending.

Gusted
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 11:10:28 +02:00
services/pull/patch.go
@ -72,0 +151,4 @@
cancel
gitRepo
Close
earl-warren
commented
2025-05-03 11:10:28 +02:00
Contributor
Copy link
Could you split the lines before this one out of this function into its own and add unit testing for it as well as
GetHeadRefForTestPatch
? To help maintenance and border case testings in the future.
Could you split the lines before this one out of this function into its own and add unit testing for it as well as `GetHeadRefForTestPatch`? To help maintenance and border case testings in the future.
Gusted
commented
2025-05-04 16:52:08 +02:00
Author
Owner
Copy link
I slightly object to unit tests for these functions, there are no border cases that aren't already handled by the integration tests that were added.
I slightly object to unit tests for these functions, there are no border cases that aren't already handled by the integration tests that were added.
earl-warren
commented
2025-05-04 19:24:34 +02:00
Contributor
Copy link
I slightly prefer unit tests for just the code added. They are bound to receive bug fixes and improvements that may benefit from them.
I slightly prefer unit tests for just the code added. They are bound to receive bug fixes and improvements that may benefit from them.
Gusted
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 11:23:41 +02:00
services/pull/patch.go
@ -72,0 +124,4 @@
if
len
envs
testPatchCtx
env
append
os
Environ
envs
...
earl-warren
commented
2025-05-03 11:23:41 +02:00
Contributor
Copy link
Note to self: the default for git is os.Environ() and since it is completely overridden when set, it has to be included.
Note to self: the default for git is os.Environ() and since it is completely overridden when set, it has to be included.
earl-warren
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 11:28:42 +02:00
services/pull/patch.go
@ -67,0 +63,4 @@
// If AGit, then use HeadCommitID if set (AGit flow creates pull request),
// otherwise use the pull request reference.
if
pr
Flow
==
issues_model
PullRequestFlowAGit
if
len
pr
HeadCommitID
earl-warren
commented
2025-05-03 11:28:42 +02:00
Contributor
Copy link
in which case will the
HeadCommitID
not be set?
in which case will the `HeadCommitID` not be set?
Gusted
commented
2025-05-04 14:34:46 +02:00
Author
Owner
Copy link
Existing AGit pull request being updated.
Existing AGit pull request being updated.
Gusted
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 11:34:02 +02:00
services/pull/patch.go
@ -67,0 +59,4 @@
// GetHeadRefForTestPatch returns a git reference for the head branch, it only
// should be called when it will be used for [testPatch]. `env` should be given
// to a git command if it's going to use headRef.
func
GetHeadRefForTestPatch
ctx
context
Context
pr
issues_model
PullRequest
headRef
string
env
string
err
error
earl-warren
commented
2025-05-03 11:34:02 +02:00
Contributor
Copy link
s/GetHeadRefForTestPatch/GetHeadRevisionForTestPatch/
since it can either return a ref or a SHA?
`s/GetHeadRefForTestPatch/GetHeadRevisionForTestPatch/` since it can either return a ref or a SHA?
Gusted
marked this conversation as resolved
earl-warren
reviewed
2025-05-03 11:39:16 +02:00
services/pull/patch.go
@ -83,1 +167,3 @@
return
fmt
Errorf
"GetBranchCommitID: can't find commit ID for head: %w"
err
// If envs was set, it means `headRef` is already resolved to a commit ID and
// trying to resolve it would result in a error.
earl-warren
commented
2025-05-03 11:39:16 +02:00
Contributor
Copy link
This would benefit from being more consistent. Either by having
headCommitID
always or an explicit
testPatchCtx.headIsARef = true
set to not infer it from the length of env.
This would benefit from being more consistent. Either by having `headCommitID` always or an explicit `testPatchCtx.headIsARef = true` set to not infer it from the length of env.
Gusted
marked this conversation as resolved
Gusted
force-pushed gusted/forgejo-test-pr from
c95317df8c
All checks were successful
testing / frontend-checks (pull_request)
Successful in 58s
Details
testing / backend-checks (pull_request)
Successful in 3m24s
Details
testing / test-e2e (pull_request)
Successful in 3m3s
Details
testing / test-unit (pull_request)
Successful in 5m57s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 2m13s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m13s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 2m17s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 2m16s
Details
testing / test-mysql (pull_request)
Successful in 20m56s
Details
testing / test-sqlite (pull_request)
Successful in 25m44s
Details
testing / test-pgsql (pull_request)
Successful in 28m45s
Details
testing / security-check (pull_request)
Successful in 1m31s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
requirements / merge-conditions (pull_request)
Successful in 2s
Details
to
b448a1dda6
All checks were successful
requirements / merge-conditions (pull_request)
Successful in 2s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
testing / frontend-checks (pull_request)
Successful in 49s
Details
testing / backend-checks (pull_request)
Successful in 3m11s
Details
testing / test-e2e (pull_request)
Successful in 3m7s
Details
testing / test-unit (pull_request)
Successful in 5m18s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 2m3s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 2m2s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 2m5s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m4s
Details
testing / test-mysql (pull_request)
Successful in 16m57s
Details
testing / test-sqlite (pull_request)
Successful in 21m10s
Details
testing / test-pgsql (pull_request)
Successful in 24m4s
Details
testing / security-check (pull_request)
Successful in 1m7s
Details
2025-05-04 16:53:55 +02:00
Compare
earl-warren
approved these changes
2025-05-04 20:32:07 +02:00
Dismissed
Gusted
force-pushed gusted/forgejo-test-pr from
b448a1dda6
All checks were successful
requirements / merge-conditions (pull_request)
Successful in 2s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
testing / frontend-checks (pull_request)
Successful in 49s
Details
testing / backend-checks (pull_request)
Successful in 3m11s
Details
testing / test-e2e (pull_request)
Successful in 3m7s
Details
testing / test-unit (pull_request)
Successful in 5m18s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 2m3s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 2m2s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 2m5s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m4s
Details
testing / test-mysql (pull_request)
Successful in 16m57s
Details
testing / test-sqlite (pull_request)
Successful in 21m10s
Details
testing / test-pgsql (pull_request)
Successful in 24m4s
Details
testing / security-check (pull_request)
Successful in 1m7s
Details
to
89fb466239
Some checks failed
requirements / merge-conditions (pull_request)
Successful in 2s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
testing / frontend-checks (pull_request)
Successful in 1m31s
Details
testing / backend-checks (pull_request)
Failing after 4m33s
Details
testing / test-unit (pull_request)
Has been skipped
Details
testing / test-e2e (pull_request)
Has been skipped
Details
testing / test-mysql (pull_request)
Has been skipped
Details
testing / test-pgsql (pull_request)
Has been skipped
Details
testing / test-sqlite (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redis) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (valkey) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (garnet) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redict) (pull_request)
Has been skipped
Details
testing / security-check (pull_request)
Has been skipped
Details
2025-05-04 21:01:03 +02:00
Compare
Gusted
force-pushed gusted/forgejo-test-pr from
89fb466239
Some checks failed
requirements / merge-conditions (pull_request)
Successful in 2s
Details
issue-labels / release-notes (pull_request_target)
Has been skipped
Details
testing / frontend-checks (pull_request)
Successful in 1m31s
Details
testing / backend-checks (pull_request)
Failing after 4m33s
Details
testing / test-unit (pull_request)
Has been skipped
Details
testing / test-e2e (pull_request)
Has been skipped
Details
testing / test-mysql (pull_request)
Has been skipped
Details
testing / test-pgsql (pull_request)
Has been skipped
Details
testing / test-sqlite (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redis) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (valkey) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (garnet) (pull_request)
Has been skipped
Details
testing / test-remote-cacher (redict) (pull_request)
Has been skipped
Details
testing / security-check (pull_request)
Has been skipped
Details
to
2a163f78f9
All checks were successful
testing / frontend-checks (pull_request)
Successful in 1m55s
Details
testing / backend-checks (pull_request)
Successful in 5m11s
Details
testing / test-e2e (pull_request)
Successful in 3m46s
Details
testing / test-unit (pull_request)
Successful in 7m18s
Details
testing / test-remote-cacher (redis) (pull_request)
Successful in 1m52s
Details
testing / test-remote-cacher (valkey) (pull_request)
Successful in 1m55s
Details
testing / test-remote-cacher (garnet) (pull_request)
Successful in 1m53s
Details
testing / test-remote-cacher (redict) (pull_request)
Successful in 2m1s
Details
testing / test-mysql (pull_request)
Successful in 21m51s
Details
testing / test-sqlite (pull_request)
Successful in 24m32s
Details
testing / test-pgsql (pull_request)
Successful in 28m19s
Details
testing / security-check (pull_request)
Successful in 1m17s
Details
issue-labels / cascade (pull_request_target)
Has been skipped
Details
requirements / merge-conditions (pull_request)
Successful in 2s
Details
issue-labels / release-notes (pull_request_target)
Successful in 37s
Details
issue-labels / backporting (pull_request_target)
Has been skipped
Details
milestone / set (pull_request_target)
Successful in 5s
Details
2025-05-04 21:02:38 +02:00
Compare
Gusted
commented
2025-05-04 21:03:30 +02:00
Author
Owner
Copy link
@earl-warren
All of your reviews has been addressed.
@earl-warren All of your reviews has been addressed.
❤️
earl-warren
approved these changes
2025-05-05 07:12:43 +02:00
earl-warren
left a comment
Copy link
The extra effort on unit testing is much appreciated
❤️
The extra effort on unit testing is much appreciated ❤️
earl-warren
commented
2025-05-05 07:18:21 +02:00
Contributor
Copy link
I suggest a PR title similar to this for the release notes:
feat: improved performances when checking for conflicts on pull requests
I think it deserves a release note line despite the fact that most instances users or admins won't notice a difference.
I suggest a PR title similar to this for the release notes:

> feat: improved performances when checking for conflicts on pull requests

I think it deserves a release note line despite the fact that most instances users or admins won't notice a difference.
earl-warren
added the
worth a release-note
label
2025-05-05 07:18:36 +02:00
forgejo-release-notes-assistant
commented
2025-05-05 07:19:25 +02:00
Member
Copy link
Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/7727.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.
This message and the release notes originate from a call to the
release-notes-assistant
@@ -5,3 +5,11 @@
- As far as I can understand the documentation and the existing code, there's no edge case that the new code cannot handle. It also results in a cleaner codepath, as the existing code did a lot of checking and merging in a more traditional approach that required a lot of (parsing) code, while the new code offloads this to git and has trivial parser of the output.
- Resolves forgejo/forgejo#7701
-- Added exhaustive integration testing.
\ No newline at end of file
+- Added exhaustive integration testing.
+
+## Release notes
+
+- Features
+ - [PR](https://codeberg.org/forgejo/forgejo/pulls/7727): Patch testing on bare repository
+
Release notes
Features
PR
: Patch testing on bare repository

## Release notes

- Features
- [PR](https://codeberg.org/forgejo/forgejo/pulls/7727): Patch testing on bare repository

Gusted
changed title from
feat: Patch testing on bare repository
to
feat: improved performances when checking for conflicts on pull requests
2025-05-05 14:43:06 +02:00
forgejo-release-notes-assistant
commented
2025-05-05 14:43:47 +02:00
Member
Copy link
Where does that come from?
The following is a preview of the release notes for this pull request, as they will appear in the upcoming release. They are derived from the content of the `release-notes/7727.md` file, if it exists, or the title of the pull request. They were also added at the bottom of the description of this pull request for easier reference.
This message and the release notes originate from a call to the
release-notes-assistant
@@ -12,4 +12,4 @@

- Features
- - [PR](https://codeberg.org/forgejo/forgejo/pulls/7727): Patch testing on bare repository
+ - [PR](https://codeberg.org/forgejo/forgejo/pulls/7727): improved performances when checking for conflicts on pull requests

Release notes
Features
PR
: improved performances when checking for conflicts on pull requests

## Release notes

- Features
- [PR](https://codeberg.org/forgejo/forgejo/pulls/7727): improved performances when checking for conflicts on pull requests

fnetX
approved these changes
2025-05-15 01:23:18 +02:00
fnetX
left a comment
Copy link
Looks good to me.
Looks good to me.
Gusted
referenced this pull request from a commit
2025-05-16 14:40:40 +02:00
feat: improved performances when checking for conflicts on pull requests (#7727)
Gusted
merged commit
7c150be23d
into
forgejo
2025-05-16 14:40:45 +02:00
forgejo-release-notes-assistant
added this to the
Forgejo v12.0.0
milestone
2025-05-16 14:40:54 +02:00
famfo
referenced this pull request from a commit
2025-05-19 15:15:08 +02:00
feat: improved performances when checking for conflicts on pull requests (#7727)
famfo
referenced this pull request from a commit
2025-06-01 16:49:13 +02:00
feat: improved performances when checking for conflicts on pull requests (#7727)
davrot
referenced this pull request from a commit
2025-06-02 14:13:57 +02:00
feat: improved performances when checking for conflicts on pull requests (#7727)
Gusted
referenced this pull request
2025-07-01 01:51:11 +02:00
feat: run actions against merge result
#4734
Gusted
referenced this pull request from forgejo/website
2025-07-07 02:48:22 +02:00
Forgejo v12.0.0 blog post snippets
#608
Gusted
referenced this pull request
2025-07-09 02:00:22 +02:00
feat: improve checking if diffs differ
#8451
Gusted
referenced this pull request from a commit
2025-07-16 18:19:39 +02:00
feat: improve checking if diffs differ (#8451)
dmowitz
referenced this pull request from a commit
2025-08-08 01:41:45 +02:00
feat: improved performances when checking for conflicts on pull requests (#7727)
earl-warren
referenced this pull request from a commit
2025-08-26 10:42:00 +02:00
feat: improve checking if diffs differ (#8451)
forgejo-backport-action
referenced this pull request
2025-08-26 10:43:10 +02:00
[v12.0/forgejo] feat: improve checking if diffs differ
#9034
forgejo-backport-action
referenced this pull request from a commit
2025-08-26 10:43:15 +02:00
feat: improve checking if diffs differ (#8451)
forgejo-actions
referenced this pull request from forgejo/website
2025-11-27 18:11:42 +01:00
Dead links report
#529
forgejo-actions
referenced this pull request from forgejo/website
2025-12-08 18:03:53 +01:00
Dead links report
#529
forgejo-actions
referenced this pull request from forgejo/website
2025-12-18 18:03:26 +01:00
Dead links report
#529
forgejo-actions
referenced this pull request from forgejo/website
2026-01-08 18:02:12 +01:00
Dead links report
#529
alberic89
referenced this pull request from a commit
2026-02-23 18:42:29 +01:00
feat: improved performances when checking for conflicts on pull requests (#7727)
alberic89
referenced this pull request from a commit
2026-02-23 18:42:40 +01:00
feat: improve checking if diffs differ (#8451)
to join this conversation.
Reviewers
No reviewers
earl-warren
fnetX
Labels
Clear labels
arch
riscv64
Archived
backport/v1.19
Scheduled for backport to Forgejo v1.19
Archived
backport/v1.20
Scheduled for backport to Forgejo v1.20
Archived
backport/v1.21/forgejo
Scheduled for backport to Forgejo v1.21
Archived
backport/v10.0/forgejo
Automated backport to v10.0
Archived
backport/v11.0/forgejo
Automated backport to v11.0
backport/v12.0/forgejo
Automated backport to v12.0
Archived
backport/v13.0/forgejo
Automated backport to v13.0
Archived
backport/v14.0/forgejo
Automated backport to v14.0
backport/v15.0/forgejo
Automated backport to v15.0
backport/v7.0/forgejo
Scheduled for backport to Forgejo v7.0
Archived
backport/v8.0/forgejo
Scheduled for backport to Forgejo v8.0
Archived
backport/v9.0/forgejo
Scheduled for backport to Forgejo v9.0
Archived
breaking
The release containing this change is not backward compatible
bug
Something is not working
Archived
bug
confirmed
it can be reproduced
bug
duplicate
bug has already been reported in the Forgejo tracker
bug
needs-more-info
the information provided does not contain enough details
bug
new-report
bug has just been reported and need triage (default label on issue creation)
bug
reported-upstream
bug cannot be fixed within Forgejo easily, it has been reported upstream
code/actions
Forgejo Actions feature
code/api
API
code/auth
Forgejo Authentication
code/auth/faidp
Forgejo as Identity Provider (in OAuth/OIDC flow)
code/auth/farp
Forgejo as Relying Party / Client (in OAuth/OIDC flow)
code/email
Everything related to email in Forgejo
code/federation
Federation
code/git
Related to the Git backend in Forgejo
code/migrations
Migration between Git forges (i.e. for GitHub, GitLab, Gitea, Forgejo, etc.). NOT for database migrations.
code/packages
Forgejo package and container registry
code/wiki
database
MySQL
database
PostgreSQL
database
SQLite
dependency-upgrade
dependency
Chi
Archived
dependency
Chroma
Archived
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Gitea
Archived
dependency
Golang
Discussion
duplicate
This issue or pull request already exists
enhancement/feature
New feature
forgejo/accessibility
Accessibility (a11y)
forgejo/branding
Branding (logo, name, tagline etc.)
forgejo/ci
Forgejo Actions CI configuration
forgejo/commit-graph
The commit graph feature and page.
forgejo/documentation
forgejo/furnace cleanup
Keeping Forgejo in sync with its dependencies and contributing back to them
Archived
forgejo/i18n
t9n/translation, l10n/localization, and i18n/internationalization of Forgejo
forgejo/interop
Interoperability with other services: Webhooks, bridges, integrations
forgejo/moderation
Moderation
forgejo/privacy
Privacy first
forgejo/release
Release management
forgejo/scaling
Performance and scaling
forgejo/security
Security (please disclose responsibly)
forgejo/ui
User interface
Gain
High
User research provides indicators that this would be good to have, interested contributors are encouraged to pick this.
Gain
Nice to have
This is likely worth having, but the assumption is not backed by user research data (it might benefit a small amount of users only.) Unlikely to receive much attention, but feel free to pick.
Gain
Undefined
Not enough information to assess the request's benefits. This issue may be closed if no gain is established: You can help by giving us more input.
Gain
Very High
User research indicates that this is an important improvement for Forgejo users. Contributions very welcome!
good first issue
Optimal for first-timers! Make sure to look for further explanations and ask for help if needed. If you want, you can consider the person who added this label as a point of contact.
i18n/backport-stable
This PR needs to be backported to stable branch of Forgejo safely and manually, using a migration script.
impact
large
Large impact: Potential data loss, many users affected, major degradation in UX.
impact
medium
Medium impact: Several users affected, degradation in UX, workarounds might be available but inconvenient.
impact
small
Small impact: No data loss, workarounds might be available, affects few users.
impact
unknown
Report was not yet triaged to assess impact.
Incompatible license
This pull request contains changes that are not (yet) compatible with the current Forgejo license
issue
closed
The issue was resolved in the repository of the dependency
issue
do-not-exist-yet
An issue should be created in the respository of the dependency
issue
open
An open issue exists in the upstream repository of the dependency
manual test
Pull requests that have been merged with a manual test
Archived
Manually tested during feature freeze
The manual test instructions were followed
OS
FreeBSD
Specific to the FreeBSD Operating System
OS
Linux
Specific to (GNU/)Linux Operating Systems
OS
macOS
Specific to the MacOS Operating System
OS
Windows
Specific to the Windows Operating System
problem
A user report about a problem. Needs to be triaged to find potential solutions.
QA
regression
found in the version of the milestone and not before
release blocker
Issues that must be fixed before the release can be published
Release Cycle
Feature Freeze
Only bug fixes with automated tests (except for CSS/JavaScript)
release-blocker
v7.0
Issues that must be fixed before Forgejo v7.0 can be released 17 April 2024
Archived
release-blocker
v7.0.1
Issues that must be fixed before Forgejo v7.0.1 can be released
Archived
release-blocker
v7.0.2
Issues that must be fixed before Forgejo v7.0.2 can be released
Archived
release-blocker
v7.0.3
Issues that must be fixed before Forgejo v7.0.3 can be released
Archived
release-blocker
v7.0.4
Issues that must be fixed before Forgejo v7.0.4 can be released
Archived
release-blocker
v8.0.0
Issues that must be fixed before Forgejo v8.0.0 can be released
Archived
release-blocker/v9.0.0
Issues that must be fixed before Forgejo v9.0.0 can be released
Archived
run-all-playwright-tests
Add this label to a PR to run all playwright tests manually.
run-end-to-end-tests
Trigger additional tests on the PR when it is ready to be merged
test
manual
manual testing has been documented
test
needed
test should be added
test
needs-help
help needed to add a test
test
not-needed
no additional test is needed
test
present
test has been added
untested
Pull requests that have been merged with no test and submitted as is to the dependency where they belong
Archived
User research - time-tracker
Time tracking feature for issues and the JS stopwatch.
valuable code
This PR was closed because the implementation is incomplete
worth a release-note
Add this PR to the release notes
User research - Accessibility
Requires input about accessibility features, likely involves user testing.
User research - Blocked
Do not pick as-is! We are happy if you can help, but please coordinate with ongoing redesign in this area.
User research - Community
Community features, such as discovering other people's work or otherwise feeling welcome on a Forgejo instance.
User research - Config (instance)
Instance-wide configuration, authentication and other admin-only needs.
User research - Errors
How to deal with errors in the application and write helpful error messages.
User research - Filters
How filter and search is being worked with.
User research - Future backlog
The issue might be inspiring for future design work.
User research - Git workflow
AGit, fork-based and new Git workflow, PR creation etc
User research - Labels
Active research about Labels
User research - Moderation
Moderation Featuers for Admins are undergoing active User Research
User research - Needs input
Use this label to let the User Research team know their input is requested.
User research - Notifications/Dashboard
Research on how users should know what to do next.
User research - Rendering
Text rendering, markup languages etc
User research - Repo creation
Active research about the New Repo dialog.
User research - Repo units
The repo sections, disabling them and the "Add more" button.
User research - Security
User research - Settings (in-app)
How to structure in-app settings in the future?
No labels
arch
riscv64
backport/v1.19
backport/v1.20
backport/v1.21/forgejo
backport/v10.0/forgejo
backport/v11.0/forgejo
backport/v12.0/forgejo
backport/v13.0/forgejo
backport/v14.0/forgejo
backport/v15.0/forgejo
backport/v7.0/forgejo
backport/v8.0/forgejo
backport/v9.0/forgejo
breaking
bug
bug
confirmed
bug
duplicate
bug
needs-more-info
bug
new-report
bug
reported-upstream
code/actions
code/api
code/auth
code/auth/faidp
code/auth/farp
code/email
code/federation
code/git
code/migrations
code/packages
code/wiki
database
MySQL
database
PostgreSQL
database
SQLite
dependency-upgrade
dependency
Chi
dependency
Chroma
dependency
F3
dependency
ForgeFed
dependency
garage
dependency
Gitea
dependency
Golang
Discussion
duplicate
enhancement/feature
forgejo/accessibility
forgejo/branding
forgejo/ci
forgejo/commit-graph
forgejo/documentation
forgejo/furnace cleanup
forgejo/i18n
forgejo/interop
forgejo/moderation
forgejo/privacy
forgejo/release
forgejo/scaling
forgejo/security
forgejo/ui
Gain
High
Gain
Nice to have
Gain
Undefined
Gain
Very High
good first issue
i18n/backport-stable
impact
large
impact
medium
impact
small
impact
unknown
Incompatible license
issue
closed
issue
do-not-exist-yet
issue
open
manual test
Manually tested during feature freeze
OS
FreeBSD
OS
Linux
OS
macOS
OS
Windows
problem
QA
regression
release blocker
Release Cycle
Feature Freeze
release-blocker
v7.0
release-blocker
v7.0.1
release-blocker
v7.0.2
release-blocker
v7.0.3
release-blocker
v7.0.4
release-blocker
v8.0.0
release-blocker/v9.0.0
run-all-playwright-tests
run-end-to-end-tests
test
manual
test
needed
test
needs-help
test
not-needed
test
present
untested
User research - time-tracker
valuable code
worth a release-note
User research - Accessibility
User research - Blocked
User research - Community
User research - Config (instance)
User research - Errors
User research - Filters
User research - Future backlog
User research - Git workflow
User research - Labels
User research - Moderation
User research - Needs input
User research - Notifications/Dashboard
User research - Rendering
User research - Repo creation
User research - Repo units
User research - Security
User research - Settings (in-app)
Milestone
Clear milestone
No items
No milestone
Forgejo v12.0.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".
No due date set.
Dependencies
No dependencies set.
Reference
forgejo/forgejo!7727
Reference in a new issue
No description provided.
Delete branch "gusted/forgejo-test-pr"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?