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

Improve TestPatch to make it much quicker when there are no conflicts and large files #15314

Closed
wants to merge 15 commits into from

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Apr 6, 2021

Prior to this PR essentially the test patch code was git diff -p --binary mergebase base head | git apply --check --cached

This is extremely slow if there are large binary files and the implementation dumped large temporary files on to the system.

We therefore propose a different option here:

  • git read-tree -m to attempt the basic merge
  • git status --porcelain can then be used to find the conflicts here.
  • we can then limit the diff patch to only the conflicts - and we will drop the binary diff as the whitespace diffs matching is unlikely to be of help.

Signed-off-by: Andrew Thornton art27@cantab.net

… and large files

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the performance/speed performance issues with slow downs label Apr 6, 2021
@zeripath zeripath added this to the 1.15.0 milestone Apr 6, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Apr 6, 2021

This PR should save at least 12s in our tests.

Was:
+++ TestGit/HTTP/MergeFork/CreatePRAndMerge is a slow test (took 40.292540645s)

Is:
+++ TestGit/SSH/MergeFork/CreatePRAndMerge is a slow test (took 34.214094827s)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 6, 2021
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath changed the title Improve TestPatch to make it much quicker when there are no conflicts and large files WIP Improve TestPatch to make it much quicker when there are no conflicts and large files Apr 6, 2021
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the pr/wip This PR is not ready for review label Apr 7, 2021
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath changed the title WIP Improve TestPatch to make it much quicker when there are no conflicts and large files Improve TestPatch to make it much quicker when there are no conflicts and large files Apr 7, 2021
@zeripath zeripath removed the pr/wip This PR is not ready for review label Apr 7, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Apr 7, 2021

Ready

@a1012112796

This comment has been minimized.

@zeripath
Copy link
Contributor Author

zeripath commented Apr 8, 2021

@a1012112796 All of your changes are covered in

git read-tree base
git read-tree -m --aggressive merge-base base tracking
git status --porcelain

and are much faster and better covered in that.

git read-tree -m --aggressive handles renames correctly amongst other things - your changes do not.

Copy link
Member

@a1012112796 a1012112796 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some logic simplify about else :)

services/pull/patch.go Outdated Show resolved Hide resolved
services/pull/patch.go Outdated Show resolved Hide resolved
services/pull/patch.go Outdated Show resolved Hide resolved
services/pull/patch.go Outdated Show resolved Hide resolved
services/pull/patch.go Outdated Show resolved Hide resolved
@a1012112796
Copy link
Member

a1012112796 commented Apr 9, 2021

Now this function is a little long(near 190 lines) ..........
Maybe it's better to split it...

@zeripath
Copy link
Contributor Author

zeripath commented Apr 9, 2021

Have split it up and added a few more comments

@codecov-commenter
Copy link

codecov-commenter commented May 6, 2021

Codecov Report

Merging #15314 (a2dbf1f) into main (17be645) will increase coverage by 0.00%.
The diff coverage is 68.42%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #15314   +/-   ##
=======================================
  Coverage   44.01%   44.01%           
=======================================
  Files         681      681           
  Lines       82351    82393   +42     
=======================================
+ Hits        36249    36269   +20     
- Misses      40194    40213   +19     
- Partials     5908     5911    +3     
Impacted Files Coverage Δ
services/pull/patch.go 53.88% <68.42%> (-0.36%) ⬇️
models/unit.go 46.57% <0.00%> (-2.74%) ⬇️
modules/log/event.go 59.90% <0.00%> (-1.89%) ⬇️
routers/repo/view.go 40.66% <0.00%> (-0.59%) ⬇️
modules/queue/workerpool.go 53.81% <0.00%> (+0.76%) ⬆️
modules/process/manager.go 75.30% <0.00%> (+2.46%) ⬆️
modules/queue/queue_channel.go 96.66% <0.00%> (+5.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17be645...a2dbf1f. Read the comment docs.

@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 May 6, 2021
@zeripath zeripath mentioned this pull request May 9, 2021
12 tasks
@6543
Copy link
Member

6543 commented May 16, 2021

this did not recognize a custom created conflict:

to reproduce:

  • create two branches from master (A & B)
  • add file CONFL with content aaa\n to branch A
  • add file CONFL with content bbb\n to branch B
  • create pull from A to master
  • create pull from B to master
  • merge B
  • look at A: still show "merge ..." button!

@zeripath
Copy link
Contributor Author

Damn - I'll take a look. In the meantime let me mark this as WIP

@zeripath zeripath added the pr/wip This PR is not ready for review label May 16, 2021
@zeripath zeripath marked this pull request as draft May 16, 2021 09:47
@zeripath zeripath modified the milestones: 1.15.0, 1.16.0 Jun 23, 2021
@lunny lunny removed this from the 1.16.0 milestone Nov 9, 2021
@zeripath
Copy link
Contributor Author

Replaced by #18004

@zeripath zeripath closed this Dec 21, 2021
@zeripath zeripath deleted the speed-up-testpatch branch December 21, 2021 20:39
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. performance/speed performance issues with slow downs pr/wip This PR is not ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants