Skip to content

Conversation

Loutro
Copy link
Contributor

@Loutro Loutro commented Apr 25, 2021

The check for image on each file is really heavy, and i don't think it's mandatory for non binary files.
So i just propose to skip this IsImage check for non binary file.

@codecov-commenter
Copy link

Codecov Report

Merging #15618 (85e7f56) into master (6ea6e2b) will decrease coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15618      +/-   ##
==========================================
- Coverage   43.93%   43.87%   -0.06%     
==========================================
  Files         678      678              
  Lines       81742    81742              
==========================================
- Hits        35911    35867      -44     
- Misses      39980    40022      +42     
- Partials     5851     5853       +2     
Impacted Files Coverage Δ
modules/notification/ui/ui.go 58.97% <0.00%> (-6.84%) ⬇️
modules/notification/mail/mail.go 33.67% <0.00%> (-5.11%) ⬇️
services/pull/temp_repo.go 26.59% <0.00%> (-3.20%) ⬇️
services/pull/check.go 26.02% <0.00%> (-2.74%) ⬇️
modules/notification/base/null.go 76.31% <0.00%> (-2.64%) ⬇️
modules/notification/notification.go 85.12% <0.00%> (-2.48%) ⬇️
models/issue_comment.go 50.71% <0.00%> (-1.87%) ⬇️
services/pull/patch.go 54.23% <0.00%> (-1.70%) ⬇️
services/pull/pull.go 43.02% <0.00%> (-0.92%) ⬇️
models/notification.go 65.18% <0.00%> (-0.89%) ⬇️
... and 6 more

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 6ea6e2b...85e7f56. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 25, 2021
@Loutro Loutro changed the title disbale image check for non binary files disable image check for non binary files Apr 25, 2021
@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 25, 2021

SVG files are not binary but image files.

@Loutro
Copy link
Contributor Author

Loutro commented Apr 26, 2021

SVG files are not binary but image files.

I tested for this, but in my case it was not detected as image file.
So it's not binary and the image check returned false.

Is there a test case with a svg file detected a image?

@Loutro
Copy link
Contributor Author

Loutro commented Apr 26, 2021

to be clear, what is the good option:

  • should the svg be detected as an image, so there is a bug here?
  • the svg doesn't need to be detected as an image, so i can bypass it with the non binary test i introduced.

@KN4CK3R
Copy link
Member

KN4CK3R commented Apr 26, 2021

Have a look at #14867

@Loutro
Copy link
Contributor Author

Loutro commented Apr 26, 2021

ok perfect, i'm going to close this PR and will see after your PR is merged.

@Loutro Loutro closed this Apr 26, 2021
@Loutro Loutro deleted the disable-isimage-for-non-binary branch April 26, 2021 10:25
@go-gitea go-gitea locked and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants