Fix retrieve file last commit branchName #98
Conversation
|
LGTM |
commit_info.go
Outdated
| treePath: treePath, | ||
| headCommit: headCommit, | ||
| } | ||
| } | ||
|
|
||
| // GetCommitsInfo gets information of all commits that are corresponding to these entries | ||
| func (tes Entries) GetCommitsInfo(commit *Commit, treePath string) ([][]interface{}, error) { | ||
| state := initGetCommitInfoState(tes, commit, treePath) | ||
| func (tes Entries) GetCommitsInfo(commit *Commit, branchName, treePath string) ([][]interface{}, error) { |
There was a problem hiding this comment.
branch name seems redundant here, a commit is enough to identify an entity in a repository. Also changing this signature needlessly changes API of the module
There was a problem hiding this comment.
Also, sometimes when calling this function there won't be a branch (e.g. viewing the source tree at a particular commit)
There was a problem hiding this comment.
I'd call it revision like git does https://git-scm.com/docs/gitrevisions
There was a problem hiding this comment.
@strk maybe right. HEAD -> commit.ID.String(). I will fix this.
|
@strk @ethantkoenig also should fix go-gitea/gitea#3202 |
commit_info.go
Outdated
| @@ -90,7 +90,9 @@ func targetedSearch(state *getCommitsInfoState, done chan error) { | |||
| done <- err | |||
| return | |||
| } | |||
| state.lock.Lock() | |||
There was a problem hiding this comment.
Why do we need to lock here? I don't see why it would be a problem if two threads run a read-only git command in parallel.
|
Please include a test for the fix |
|
@strk will include a test on gitea's integration test. |
|
@ethantkoenig I think you are right. Done. |
|
LGTM |
|
@strk I will add tests (which is probably fair, since I introduced the bug) |
|
@ethantkoenig maybe add an integration test and compare github's home page tree list and gitea's home page tree list. |
This will be a part of PR to fix go-gitea/gitea#3181