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

Monitor all git commands; move blame to git package and replace git as a variable #6864

Merged
merged 19 commits into from
Jun 26, 2019

Conversation

lunny
Copy link
Member

@lunny lunny commented May 6, 2019

This PR did some changes:

  • add to process manager when revoke command.Run in git package so that all git processes will be monitored from admin panel. All these git processes haven't been monitored before.
  • replaced all "git" as git.GitExecutable variable. will replace Support git.PATH entry in app.ini #6772.
  • create a method IsEmpty for git.Repository with tests.
  • move blame codes to package git.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label May 6, 2019
@lunny lunny added this to the 1.9.0 milestone May 6, 2019
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 6, 2019
@lunny lunny changed the title Monitor all git commands; move blame to git package and replace git as a variable WIP: Monitor all git commands; move blame to git package and replace git as a variable May 6, 2019
@codecov-io
Copy link

codecov-io commented May 7, 2019

Codecov Report

Merging #6864 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6864      +/-   ##
==========================================
+ Coverage   41.23%   41.24%   +<.01%     
==========================================
  Files         464      464              
  Lines       62832    62841       +9     
==========================================
+ Hits        25909    25916       +7     
- Misses      33531    33536       +5     
+ Partials     3392     3389       -3
Impacted Files Coverage Δ
routers/repo/blame.go 0% <0%> (ø) ⬆️
modules/git/blame.go 78.26% <0%> (ø)
models/release.go 52.94% <100%> (ø) ⬆️
modules/git/command.go 94.04% <100%> (+0.22%) ⬆️
models/git_diff.go 68.04% <12.5%> (ø) ⬆️
routers/repo/http.go 35.32% <16.66%> (-1.59%) ⬇️
models/update.go 46.66% <50%> (-0.28%) ⬇️
models/repo.go 48.71% <50%> (+0.08%) ⬆️
models/repo_mirror.go 22.91% <50%> (ø) ⬆️
modules/repofiles/temp_repo.go 70.5% <75%> (ø) ⬆️
... 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 161e12e...c24123d. Read the comment docs.

@lunny lunny changed the title WIP: Monitor all git commands; move blame to git package and replace git as a variable Monitor all git commands; move blame to git package and replace git as a variable May 7, 2019
@lunny lunny added the type/enhancement An improvement of existing functionality label May 7, 2019
@lafriks
Copy link
Member

lafriks commented May 7, 2019

Unit test for TestRepoIsEmpty fails in drone

if err != nil {
if strings.Contains(stderr, "fatal: bad default revision 'HEAD'") {
Copy link
Member

@sapk sapk May 7, 2019

Choose a reason for hiding this comment

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

This change seems to change how this function works. This will always error without setting the repo.IsEmpty previously to return. Is it intended because it fix an issue ? In place, of starting another cli command inside gitRepo.IsEmpty() couldn't we use a method IsEmptyRepoErr(err) parsing the error of OpenRepository ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I run make unit-test locally successfully but don't know why it failed on drone. I move these codes to git package so that external shouldn't know how it works.

@lunny lunny force-pushed the lunny/uniform_git branch 4 times, most recently from db5a963 to e73f303 Compare May 14, 2019 11:57
@zeripath
Copy link
Contributor

We should probably also set GIT_ASKPASS= to prevent git from ever hanging waiting for terminal input.

@lunny lunny force-pushed the lunny/uniform_git branch 2 times, most recently from 19a9117 to 08a22c2 Compare June 18, 2019 14:25
@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 Jun 18, 2019
@lunny
Copy link
Member Author

lunny commented Jun 26, 2019

@lafriks @sapk done.
@zeripath That should be another PR.

@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 26, 2019
@techknowlogick techknowlogick merged commit edc94c7 into go-gitea:master Jun 26, 2019
@lunny lunny deleted the lunny/uniform_git branch June 26, 2019 23:39
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
…s a variable (go-gitea#6864)

* monitor all git commands; move blame to git package and replace git as a variable

* use git command but not other commands

* fix build

* move exec.Command to git.NewCommand

* fix fmt

* remove unrelated changes

* remove unrelated changes

* refactor IsEmpty and add tests

* fix tests

* fix tests

* fix tests

* fix tests

* remove gitLogger

* fix fmt

* fix isEmpty

* fix lint

* fix tests
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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/enhancement An improvement of existing functionality type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants