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

Use git attributes to determine generated and vendored status for language stats and diffs #16773

Merged
merged 18 commits into from
Sep 9, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Aug 22, 2021

Replaces #16262
Replaces #16250
Replaces #14833

This PR first implements a git check-attr pipe reader - using git check-attr --stdin -z --cached - taking account of the change in the output format in git 1.8.5 and creates a helper function to read a tree into a temporary index file for that pipe reader.

It then wires this in to the language stats helper and into the git diff generation.

Files which are marked generated will be folded by default.

Fixes #14786
Fixes #12653

@zeripath zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Aug 22, 2021
@zeripath zeripath added this to the 1.16.0 milestone Aug 22, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 22, 2021
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
Signed-off-by: Andrew Thornton <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2021

Codecov Report

Merging #16773 (7ae4ab9) into main (b83b4fb) will increase coverage by 0.04%.
The diff coverage is 61.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16773      +/-   ##
==========================================
+ Coverage   45.15%   45.19%   +0.04%     
==========================================
  Files         765      766       +1     
  Lines       86300    86574     +274     
==========================================
+ Hits        38968    39131     +163     
- Misses      41023    41113      +90     
- Partials     6309     6330      +21     
Impacted Files Coverage Δ
modules/analyze/generated.go 50.00% <50.00%> (ø)
modules/git/repo_language_stats_nogogit.go 50.00% <54.76%> (+2.43%) ⬆️
services/gitdiff/gitdiff.go 73.10% <59.61%> (-0.97%) ⬇️
modules/git/repo_attribute.go 66.32% <64.37%> (-3.13%) ⬇️
modules/git/repo_index.go 22.53% <66.66%> (+22.53%) ⬆️
models/repo_mirror.go 58.69% <0.00%> (-10.87%) ⬇️
services/mirror/mirror.go 8.19% <0.00%> (-6.56%) ⬇️
modules/avatar/avatar.go 47.72% <0.00%> (-4.55%) ⬇️
modules/charset/charset.go 71.71% <0.00%> (-4.05%) ⬇️
modules/cron/tasks_basic.go 85.43% <0.00%> (-2.92%) ⬇️
... and 10 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 b83b4fb...7ae4ab9. Read the comment docs.

@zeripath zeripath mentioned this pull request Aug 25, 2021
@silverwind
Copy link
Member

silverwind commented Aug 26, 2021

Gave it a quick try:

  • .gitignore was detected as vendored which I think is wrong
  • yarn.lock was not detected as generated (but still suppressed because to large)

Also, I think we need a button to load such hidden diffs, is that in #16829?

@zeripath
Copy link
Contributor Author

zeripath commented Aug 27, 2021

.gitignore is being detected as vendored by enry.

https://github.com/go-enry/go-enry/blob/7c24e3d5d222dff34a4eb2f2fd7f79dab3d66eb6/data/vendor.go#L124

did you add yarn.lock to the .gitattributes or just expect that enry would detect it? If so that's another enry issue.

https://github.com/go-enry/go-enry/blob/7c24e3d5d222dff34a4eb2f2fd7f79dab3d66eb6/data/generated.go#L74-L77

Doesn't list yarn.lock as a generated type.


For files which are not too big but are vendored just click the chevron at the left and the file will be displayed.

Files which are too big need another PR - that's an orthogonal issue.

@silverwind
Copy link
Member

.gitignore was detected as vendored which I think is wrong

Hmm, I think it matches linguist in that regard, but I think we are missing something because I'm generally able to view .gitignore changes in GitHub diffs.

Doesn't list yarn.lock as a generated type.

Ah, I remember this is because linguist does not consider yarn.lock to be generated but does so for package-lock.json. Here is some discussion about it, I generally disagree with them, both files serve the same purpose and are definitely generated, it's just that yarn.lock is slightly more readable. I guess I'd need to bring it up on enry.

For files which are not too big but are vendored just click the chevron at the left and the file will be displayed.

Ah, I guess I was expecting a button like in the GitHub UI, but I guess we can still add that.

@zeripath
Copy link
Contributor Author

zeripath commented Aug 27, 2021

Screenshot from 2021-08-27 09-20-38

If you click the > the diff will be shown.


I added the Enry IsVendor() determination because it felt like it would be a helpful addition and extension. (BTW adding .gitignore to .gitattributes as linguist-vendored false would prevent it from being listed as vendored.)

Looking more closely at github's behaviour it appears that it only collapses generated by default.

I can stop collapsing the things marked vendored if that would be better. <- DONE

@zeripath
Copy link
Contributor Author

The CI Failure is not due to this PR but due to a problem related to #1441

@zeripath
Copy link
Contributor Author

@silverwind would you like to re-review?

modules/git/repo_language_stats_gogit.go Outdated Show resolved Hide resolved
modules/git/repo_language_stats_nogogit.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 6, 2021
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 6, 2021
@delvh
Copy link
Member

delvh commented Sep 9, 2021

Screenshot from 2021-08-27 09-20-38

If you click the > the diff will be shown.

Does that mean that the diff is already precomputed even if the file is most likely never seen?
I would rather suggest lazy-loading the diff the first time it is being requested (i.e. when expanding it, or explicitly using a "Load Diff" button inside it) because especially for repos that have lots of generated (and vendored, if you decide to collapse it again by default) files can then be handled almost instantly, resulting in a way better initial performance. As a result, Gitea would be even more responsive under normal circumstances.

By the way, apart from the .gitignore, I would also say that vendored files should be lazy-loaded by default.

But, to be fair, I am not exactly sure whether that fits inside the scope of this branch.
I don't know how many fundamentals for lazy-loading are already present, so this might potentially be a whole separate issue.

@zeripath
Copy link
Contributor Author

zeripath commented Sep 9, 2021

But, to be fair, I am not exactly sure whether that fits inside the scope of this branch.
I don't know how many fundamentals for lazy-loading are already present, so this might potentially be a whole separate issue.

It doesn't fit within the scope of this PR, no. That would seriously and unnecessarily complicate

I need the ideas in #16829 and some the extension work blocked by that for lazy loading to become possible - but I'm blocked til both of these PRs are merged.

@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 Sep 9, 2021
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/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
8 participants