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

tree_entry: fix CommitsInfo messed up with entries have the same SHA1 #59

Merged
merged 1 commit into from
Aug 19, 2020

Conversation

manfer
Copy link
Contributor

@manfer manfer commented Aug 14, 2020

to fix gogs/gogs#6186
to fix gogs/gogs#6235
to fix gogs/gogs#6240

The problem is on git-module

git-module/tree_entry.go

Lines 261 to 270 in cf0a6d7

infos := make(map[[20]byte]*EntryCommitInfo, len(es))
for info := range results {
infos[info.Entry.id.bytes] = info
}
commitsInfo := make([]*EntryCommitInfo, len(es))
for i, e := range es {
commitsInfo[i] = infos[e.id.bytes]
}
return commitsInfo, nil

An entry id is not unique so it should not be used as map key. When on a tree there is a file or folder with same contents its SHA1 hash is equal. This can be tested in any repo with two files with same content executing git ls-tree HEAD

$ mkdir testrepo; cd testrepo
$ git init
$ echo "this is a test" > test1
$ git add .
$ git commit -m "adding test1 file"
$ echo "this is a test" > test2
$ git add .
$ git commit -m "adding test2 file"
$ git ls-tree HEAD
100644 blob 90bfcb510602aa11ae53a42dcec18ea39fbd8cec    test1
100644 blob 90bfcb510602aa11ae53a42dcec18ea39fbd8cec    test2

Maybe a solution is using name as the key as I bet entry name is unique in any tree.

	infos := make(map[string]*EntryCommitInfo, len(es))
	for info := range results {
		infos[info.Entry.name] = info
	}


	commitsInfo := make([]*EntryCommitInfo, len(es))
	for i, e := range es {
		commitsInfo[i] = infos[e.name]
	}
	return commitsInfo, nil

Or maybe just including entry index on info so later only one loop is needed to fill commitsInfo.

// EntryCommitInfo contains a tree entry with its commit information.
type EntryCommitInfo struct {
	Entry     *TreeEntry
        Index     int
	Commit    *Commit
	Submodule *Submodule
}

        ...
			go func(e *TreeEntry, i int) {
				defer func() {
					wg.Done()
        ...

				info := &EntryCommitInfo{
					Entry: e,
                                        Index: i,
				}

        ...

				results <- info
			}(e, i)
		}
	}()

        ...

	commitsInfo := make([]*EntryCommitInfo, len(es))
	for info := range results {
		commitsInfo[info.Index] = info
	}
	return commitsInfo, nil

@codecov
Copy link

codecov bot commented Aug 14, 2020

Codecov Report

Merging #59 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
- Coverage   83.58%   83.56%   -0.02%     
==========================================
  Files          24       24              
  Lines        1998     1996       -2     
==========================================
- Hits         1670     1668       -2     
  Misses        236      236              
  Partials       92       92              
Impacted Files Coverage Δ
tree_entry.go 75.38% <100.00%> (-0.38%) ⬇️

@unknwon
Copy link
Member

unknwon commented Aug 17, 2020

@manfer: Wow, thanks a lot for the great details! LGTM at first glance, I'll take a deeper look later this week.

@unknwon unknwon changed the title commitsInfo fixed tree_entry: fix CommitsInfo messed up with entries have the same SHA1 Aug 17, 2020
@unknwon
Copy link
Member

unknwon commented Aug 19, 2020

I'm merging it as-is, then add a test case later.

@unknwon unknwon merged commit c523924 into gogs:master Aug 19, 2020
@unknwon
Copy link
Member

unknwon commented Aug 19, 2020

https://github.com/gogs/git-module/releases/tag/v1.1.2 has tagged for this merge.

unknwon added a commit that referenced this pull request Aug 21, 2020
unknwon added a commit that referenced this pull request Aug 21, 2020
unknwon added a commit to gogs/gogs that referenced this pull request Aug 21, 2020
unknwon added a commit to gogs/gogs that referenced this pull request Aug 21, 2020
@xZero707
Copy link

@manfer Amazing work. Thank you!

mpsonntag pushed a commit to mpsonntag/gogs that referenced this pull request Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants