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.

Sign up for GitHub

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

Jump to bottom

Remove legacy git code (ver < 2.0), fine tune markup tests #19930

Merged
merged 21 commits into from
Jun 16, 2022
Merged

Remove legacy git code (ver < 2.0), fine tune markup tests #19930

merged 21 commits into from
Jun 16, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Jun 10, 2022 •
edited
Loading

Follows #19732 #19577

Close #19874

Major changes:

  1. Remove legacy code for git < 2.0
  2. Fix a bug in runHookPostReceive, now setup(including git.Init) is called before git.NewCommand
  3. Introduce strict mode for git.InitXxx
    • strict mode means that if the git.InitXxx isn't called, then the calls to git.NewCommand will result in error.
    • this mechanism will guarantee that there will be no mistake in the future.
  4. Fix the unit test code for markup module
  5. Add more comments for git.InitXxx
  6. Update document for commit signing, using Gitea's internal git config

Files changed: Diff with whitespace-ignored

@wxiaoguang wxiaoguang added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jun 10, 2022
@wxiaoguang wxiaoguang added this to the 1.17.0 milestone Jun 10, 2022
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jun 10, 2022
@wxiaoguang wxiaoguang added the pr/wip This PR is not ready for review label Jun 10, 2022
@wxiaoguang wxiaoguang marked this pull request as draft June 10, 2022 05:20
@wxiaoguang wxiaoguang force-pushed the fix-git-legacy branch 2 times, most recently from 8956d0b to d556a67 Compare June 10, 2022 10:41
delvh
delvh reviewed Jun 10, 2022
View reviewed changes
modules/git/repo_language_stats_gogit.go Show resolved Hide resolved
modules/git/repo_language_stats_nogogit.go Show resolved Hide resolved
modules/git/repo_tree.go Show resolved Hide resolved
services/gitdiff/gitdiff.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 10, 2022
@wxiaoguang wxiaoguang force-pushed the fix-git-legacy branch 2 times, most recently from 9404470 to a148f34 Compare June 11, 2022 05:45
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Jun 11, 2022 •
edited
Loading

This PR has done what it could do. There is still a remaining problem.(fixed)

To avoid blocking 1.17 release, the remaining problem is recorded in a new issue #19938

@wxiaoguang wxiaoguang marked this pull request as ready for review June 11, 2022 06:52
@wxiaoguang wxiaoguang removed the pr/wip This PR is not ready for review label Jun 11, 2022
@wxiaoguang wxiaoguang mentioned this pull request Jun 11, 2022
Closed
@zeripath

This comment was marked as outdated.

Sign in to view
@zeripath zeripath changed the title Fix legacy git code (ver < 2.0), fine tune markup tests Remove legacy git code (ver < 2.0), fine tune markup tests Jun 11, 2022
@zeripath
Copy link
Contributor

Hmm... I remember somewhere you commented about CheckLFSVersion causing problems.

I wonder if It could just be folded into the Init function?

zeripath
zeripath reviewed Jun 11, 2022
View reviewed changes
modules/git/git.go Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor Author

Hmm... I remember somewhere you commented about CheckLFSVersion causing problems.

I wonder if It could just be folded into the Init function?

I have tried but got no success at the moment. Since 1.17 is going to be released, I think the big refactoring could be postponed to next milestone, otherwise there will be a lot of code changed, which may affect the 1.17 milestone.

I think it might be reasonable to refactor this into a function: CheckAttributesFor

Agree it should be refactored. I was thinking about postponing it to next milestone (reason as above). How do you think? I am fine to either plan.

@zeripath
Copy link
Contributor

wxiaoguang#6

@wxiaoguang
Merge pull request #6 from zeripath/fix-git-legacy
a8f2999 
Refactor CheckAttributeReader to make a *git.Repository version
@wxiaoguang
Copy link
Contributor Author

wxiaoguang#6

Merged, thank you very much.

@wxiaoguang wxiaoguang mentioned this pull request Jun 14, 2022
Closed
@codecov-commenter
Copy link

Codecov Report

Merging #19930 (a8f2999) into main (0097fbc) will decrease coverage by 0.37%.
The diff coverage is 46.21%.

❗ Current head a8f2999 differs from pull request most recent head 8a2b4a8. Consider uploading reports for the commit 8a2b4a8 to get more accurate results

@@            Coverage Diff             @@
##             main   #19930      +/-   ##
==========================================
- Coverage   47.29%   46.91%   -0.38%     
==========================================
  Files         966      967       +1     
  Lines      134117   134087      -30     
==========================================
- Hits        63424    62909     -515     
- Misses      62977    63472     +495     
+ Partials     7716     7706      -10
Impacted Files Coverage Δ
cmd/cmd.go 40.74% <0.00%> (ø)
cmd/hook.go 7.11% <0.00%> (ø)
cmd/migrate_storage.go 0.00% <0.00%> (ø)
cmd/serv.go 2.22% <0.00%> (ø)
models/consistency.go 34.78% <ø> (+22.62%) ⬆️
models/db/consistency.go 0.00% <0.00%> (ø)
models/db/list_options.go 63.63% <ø> (ø)
models/error.go 38.66% <ø> (+2.79%) ⬆️
models/git/lfs_lock.go 41.96% <ø> (ø)
models/git/protected_tag.go 67.64% <ø> (ø)
... and 214 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 460c03c...8a2b4a8. Read the comment docs.

@wxiaoguang wxiaoguang linked an issue Jun 14, 2022 that may be closed by this pull request
GPG Commit Signatures by Gitea not working anymore in dev builds #19964
Closed
@silverwind
Copy link
Member

Do we document the min git version somewhere?

@delvh
Copy link
Member

delvh commented Jun 14, 2022

@silverwind https://github.com/go-gitea/gitea/pull/19577/files

@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 Jun 16, 2022
@lunny
Copy link
Member

lunny commented Jun 16, 2022

make L-G-T-M work.

@lunny lunny merged commit 157b405 into go-gitea:main Jun 16, 2022
@wxiaoguang wxiaoguang deleted the fix-git-legacy branch June 16, 2022 15:51
@spirkaa spirkaa mentioned this pull request Jun 18, 2022
Closed
@wxiaoguang wxiaoguang mentioned this pull request Jul 15, 2022
Merged
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
@wxiaoguang @zeripath @lunny
Remove legacy git code (ver < 2.0), fine tune markup tests (go-gitea#…
bb220ea 
…19930)

* clean git support for ver < 2.0

* fine tune tests for markup (which requires git module)

* remove unnecessary comments

* try to fix tests

* try test again

* use const for GitVersionRequired instead of var

* try to fix integration test

* Refactor CheckAttributeReader to make a *git.Repository version

* update document for commit signing with Gitea's internal gitconfig

* update document for commit signing with Gitea's internal gitconfig

Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@wxiaoguang @zeripath
@lunny @AbdulrhmnGhanem
Remove legacy git code (ver < 2.0), fine tune markup tests (go-gitea#…
6c1e3cc 
…19930)

* clean git support for ver < 2.0

* fine tune tests for markup (which requires git module)

* remove unnecessary comments

* try to fix tests

* try test again

* use const for GitVersionRequired instead of var

* try to fix integration test

* Refactor CheckAttributeReader to make a *git.Repository version

* update document for commit signing with Gitea's internal gitconfig

* update document for commit signing with Gitea's internal gitconfig

Co-authored-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@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.
Reviewers

@zeripath zeripath zeripath left review comments

@lunny lunny lunny approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
7 participants
@wxiaoguang @zeripath @codecov-commenter @silverwind @delvh @lunny @GiteaBot