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

Fix for bad commitID to show up in error #148

merged 3 commits into from Mar 8, 2019


5 participants
Copy link

richmahn commented Mar 6, 2019

This error message for a bad commitID makes no sense because when the Error object is created, commitID is always empty since NewCommand() call replaced the given commitID. This makes sure we retain the old commitID until after this block.


This comment has been minimized.

Copy link

codecov-io commented Mar 6, 2019

Codecov Report

Merging #148 into master will increase coverage by 0.41%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #148      +/-   ##
+ Coverage   36.07%   36.48%   +0.41%     
  Files          28       28              
  Lines        1838     1839       +1     
+ Hits          663      671       +8     
+ Misses       1085     1079       -6     
+ Partials       90       89       -1
Impacted Files Coverage Δ
repo_commit.go 28.67% <100%> (+2.84%) ⬆️

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 8983773...0c7c947. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 label Mar 6, 2019

@@ -153,13 +153,14 @@ func (repo *Repository) getCommit(id SHA1) (*Commit, error) {
func (repo *Repository) GetCommit(commitID string) (*Commit, error) {
if len(commitID) != 40 {
var err error
commitID, err = NewCommand("rev-parse", commitID).RunInDir(repo.Path)
actualCommitID, err := NewCommand("rev-parse", commitID).RunInDir(repo.Path)
if err != nil {
if strings.Contains(err.Error(), "unknown revision or path") {
return nil, ErrNotExist{commitID, ""}

This comment has been minimized.


richmahn Mar 6, 2019


Here is where commitID was always empty because NewCommand() returned nil for it, replacing the commitID passed to the function.

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Mar 8, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Mar 8, 2019

@techknowlogick techknowlogick merged commit 6cba05a into go-gitea:master Mar 8, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful

This comment has been minimized.

Copy link

techknowlogick commented Mar 8, 2019

Thanks for another PR @richmahn 😄 Now that this has been merged would you be able to update the deps in the main repo? Please let me know if you have any questions.


This comment has been minimized.

Copy link

richmahn commented Mar 8, 2019

@techknowlogick Yes, will do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.