From a0109cb093a2a2607d4114f1fc0ac3fda9c386de Mon Sep 17 00:00:00 2001 From: per1234 Date: Fri, 21 May 2021 00:35:46 -0700 Subject: [PATCH 1/2] Create package for Git utility functions The initial porting from `github.com/arduino/arduino-modules/git` to `github.com/go-git/go-git` was done with the intention to follow the pre-existing code structure as closely as possible. This resulted in some code duplication. Although minimal, the duplicated code was fairly complex and related to somewhat obscure details of how Git tags work. With work in progress to add even more such complex Git repository handling code, it makes sense to functionalize this previously duplicated code, which also facilitates testing. --- go.mod | 1 + go.sum | 3 + libraries/git_integration_test.go | 4 +- libraries/gitutils/gitutils.go | 38 ++++++ libraries/gitutils/gitutils_test.go | 166 +++++++++++++++++++++++++++ libraries/testdata/git_test_repo.txt | 1 + 6 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 libraries/gitutils/gitutils.go create mode 100644 libraries/gitutils/gitutils_test.go diff --git a/go.mod b/go.mod index 22bacee5..c94ef087 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ module arduino.cc/repository go 1.14 require ( + github.com/arduino/go-paths-helper v1.5.0 github.com/arduino/golang-concurrent-workers v0.0.0-20170202182617-6710cdc954bc github.com/blang/semver v3.5.1+incompatible github.com/go-git/go-git/v5 v5.3.0 diff --git a/go.sum b/go.sum index e44431f5..39c71200 100644 --- a/go.sum +++ b/go.sum @@ -5,6 +5,8 @@ github.com/alcortesm/tgz v0.0.0-20161220082320-9c5fe88206d7 h1:uSoVVbwJiQipAclBb github.com/alcortesm/tgz v0.0.0-20161220082320-9c5fe88206d7/go.mod h1:6zEj6s6u/ghQa61ZWa/C2Aw3RkjiTBOix7dkqa1VLIs= github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 h1:kFOfPq6dUM1hTo4JG6LR5AXSUEsOjtdm0kw0FtQtMJA= github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239/go.mod h1:2FmKhYUyUczH0OGQWaF5ceTx0UBShxjsH6f8oGKYe2c= +github.com/arduino/go-paths-helper v1.5.0 h1:RVo189hD+GhUS1rQ3gixwK1nSbvVR8MGIGa7Gxv2bdM= +github.com/arduino/go-paths-helper v1.5.0/go.mod h1:V82BWgAAp4IbmlybxQdk9Bpkz8M4Qyx+RAFKaG9NuvU= github.com/arduino/golang-concurrent-workers v0.0.0-20170202182617-6710cdc954bc h1:PzGY1Ppud/Ng+LFHU16oOrWhYsnSLYurwiHlbVc/FJ0= github.com/arduino/golang-concurrent-workers v0.0.0-20170202182617-6710cdc954bc/go.mod h1:E+WBbLkFBdPp+N+yijgbdDI33mr5pm6j42RYLN5K4do= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= @@ -67,6 +69,7 @@ github.com/sirupsen/logrus v1.4.1/go.mod h1:ni0Sbl8bgC9z8RoU9G6nDWqqs/fq4eDPysMB github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= +github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/stretchr/testify v1.7.0 h1:nwc3DEeHmmLAfoZucVR881uASk0Mfjw8xYJ99tb5CcY= github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= diff --git a/libraries/git_integration_test.go b/libraries/git_integration_test.go index 31b997e2..01462c54 100644 --- a/libraries/git_integration_test.go +++ b/libraries/git_integration_test.go @@ -30,8 +30,8 @@ import ( "testing" "arduino.cc/repository/libraries/db" + "arduino.cc/repository/libraries/gitutils" "github.com/go-git/go-git/v5" - "github.com/go-git/go-git/v5/plumbing" "github.com/stretchr/testify/require" ) @@ -66,7 +66,7 @@ func TestUpdateLibraryJson(t *testing.T) { repoTree, err := r.Repository.Worktree() require.NoError(t, err) // Annotated tags have their own hash, different from the commit hash, so the tag must be resolved before checkout - resolvedTag, err := r.Repository.ResolveRevision(plumbing.Revision(tag.Hash().String())) + resolvedTag, err := gitutils.ResolveTag(tag, r.Repository) require.NoError(t, err) err = repoTree.Checkout(&git.CheckoutOptions{Hash: *resolvedTag, Force: true}) require.NoError(t, err) diff --git a/libraries/gitutils/gitutils.go b/libraries/gitutils/gitutils.go new file mode 100644 index 00000000..d4131a42 --- /dev/null +++ b/libraries/gitutils/gitutils.go @@ -0,0 +1,38 @@ +// This file is part of libraries-repository-engine. +// +// Copyright 2021 ARDUINO SA (http://www.arduino.cc/) +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published +// by the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// +// You can be released from the requirements of the above licenses by purchasing +// a commercial license. Buying such a license is mandatory if you want to +// modify or otherwise use the software for commercial activities involving the +// Arduino software without disclosing the source code of your own applications. +// To purchase a commercial license, send an email to license@arduino.cc. + +package gitutils + +import ( + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" +) + +// ResolveTag returns the commit hash associated with a tag. +func ResolveTag(tag *plumbing.Reference, repository *git.Repository) (*plumbing.Hash, error) { + // Annotated tags have their own hash, different from the commit hash, so the tag must be resolved to get the has for + // the associated commit. + // Tags may point to any Git object. Although not common, this can include tree and blob objects in addition to commits. + // Resolving non-commit objects results in an error. + return repository.ResolveRevision(plumbing.Revision(tag.Hash().String())) +} diff --git a/libraries/gitutils/gitutils_test.go b/libraries/gitutils/gitutils_test.go new file mode 100644 index 00000000..2bedb86e --- /dev/null +++ b/libraries/gitutils/gitutils_test.go @@ -0,0 +1,166 @@ +// This file is part of libraries-repository-engine. +// +// Copyright 2021 ARDUINO SA (http://www.arduino.cc/) +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published +// by the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . +// +// You can be released from the requirements of the above licenses by purchasing +// a commercial license. Buying such a license is mandatory if you want to +// modify or otherwise use the software for commercial activities involving the +// Arduino software without disclosing the source code of your own applications. +// To purchase a commercial license, send an email to license@arduino.cc. + +package gitutils + +import ( + "fmt" + "testing" + "time" + + "github.com/arduino/go-paths-helper" + "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveTag(t *testing.T) { + // Prepare the file system for the test repository. + repositoryPath, err := paths.TempDir().MkTempDir("gitutils-TestResolveTag-repo") + require.Nil(t, err) + + // Create test repository. + repository, err := git.PlainInit(repositoryPath.String(), false) + require.Nil(t, err) + + testTables := []struct { + objectTypeName string + objectHash plumbing.Hash + annotated bool + errorAssertion assert.ErrorAssertionFunc + }{ + { + objectTypeName: "Commit", + objectHash: makeCommit(t, repository, repositoryPath), + errorAssertion: assert.NoError, + }, + { + objectTypeName: "Tree", + objectHash: getTreeHash(t, repository), + errorAssertion: assert.Error, + }, + { + objectTypeName: "Blob", + objectHash: getBlobHash(t, repository), + errorAssertion: assert.Error, + }, + } + + for _, testTable := range testTables { + for _, annotationConfig := range []struct { + annotated bool + descriptor string + }{ + { + annotated: true, + descriptor: "Annotated", + }, + { + annotated: false, + descriptor: "Lightweight", + }, + } { + testName := fmt.Sprintf("%s, %s", testTable.objectTypeName, annotationConfig.descriptor) + tag := makeTag(t, repository, testName, testTable.objectHash, annotationConfig.annotated) + resolvedTag, err := ResolveTag(tag, repository) + testTable.errorAssertion(t, err, fmt.Sprintf("%s tag resolution error", testName)) + if err == nil { + assert.Equal(t, testTable.objectHash, *resolvedTag, fmt.Sprintf("%s tag resolution", testName)) + } + } + } +} + +// makeCommit creates a test commit in the given repository and returns its plumbing.Hash object. +func makeCommit(t *testing.T, repository *git.Repository, repositoryPath *paths.Path) plumbing.Hash { + _, err := paths.WriteToTempFile([]byte{}, repositoryPath, "gitutils-makeCommit-tempfile") + require.Nil(t, err) + + worktree, err := repository.Worktree() + require.Nil(t, err) + _, err = worktree.Add(".") + require.Nil(t, err) + + signature := &object.Signature{ + Name: "Jane Developer", + Email: "janedeveloper@example.com", + When: time.Now(), + } + + commit, err := worktree.Commit( + "Test commit message", + &git.CommitOptions{ + Author: signature, + }, + ) + require.Nil(t, err) + + return commit +} + +// getTreeHash returns the plumbing.Hash object for an arbitrary Git tree object. +func getTreeHash(t *testing.T, repository *git.Repository) plumbing.Hash { + trees, err := repository.TreeObjects() + require.Nil(t, err) + tree, err := trees.Next() + require.Nil(t, err) + return tree.ID() +} + +// getTreeHash returns the plumbing.Hash object for an arbitrary Git blob object. +func getBlobHash(t *testing.T, repository *git.Repository) plumbing.Hash { + blobs, err := repository.BlobObjects() + require.Nil(t, err) + blob, err := blobs.Next() + require.Nil(t, err) + return blob.ID() +} + +// makeTag creates a Git tag in the given repository and returns its *plumbing.Reference object. +func makeTag(t *testing.T, repository *git.Repository, name string, hash plumbing.Hash, annotated bool) *plumbing.Reference { + var tag *plumbing.Reference + var err error + if annotated { + signature := &object.Signature{ + Name: "Jane Developer", + Email: "janedeveloper@example.com", + When: time.Now(), + } + + tag, err = repository.CreateTag( + name, + hash, + &git.CreateTagOptions{ + Tagger: signature, + Message: name, + }, + ) + } else { + tag, err = repository.CreateTag(name, hash, nil) + } + require.Nil(t, err) + + return tag +} diff --git a/libraries/testdata/git_test_repo.txt b/libraries/testdata/git_test_repo.txt index 220b7198..518b76c2 100644 --- a/libraries/testdata/git_test_repo.txt +++ b/libraries/testdata/git_test_repo.txt @@ -1,3 +1,4 @@ # Must not contain any non-compliant releases (so use an archived repo to avoid breakage). # Should contain both lightweight and annotated tags. +# Must not contain any tags for non-commit objects. https://github.com/arduino-libraries/ArduinoCloud.git|Arduino|ArduinoCloud From d825cc25e49474a4648b9969617819fe33a89527 Mon Sep 17 00:00:00 2001 From: per1234 Date: Fri, 21 May 2021 00:39:00 -0700 Subject: [PATCH 2/2] Improve order of release processing Previously, library releases were processed in a lexicographical order. This meant that, for example a 1.0.10 tag was processed before a 1.0.2 tag. In addition to making the logs difficult to read, it's possible this could result in unexpected results in the event multiple library releases had the same library.properties value. The most intuitive sorting order is to follow the order of the commits the tags are associated with in the Git history. --- libraries/gitutils/gitutils.go | 120 ++++++++++++++++++++++++++++ libraries/gitutils/gitutils_test.go | 46 +++++++++++ sync_libraries.go | 14 ++-- 3 files changed, 171 insertions(+), 9 deletions(-) diff --git a/libraries/gitutils/gitutils.go b/libraries/gitutils/gitutils.go index d4131a42..80dbee76 100644 --- a/libraries/gitutils/gitutils.go +++ b/libraries/gitutils/gitutils.go @@ -24,8 +24,11 @@ package gitutils import ( + "sort" + "github.com/go-git/go-git/v5" "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" ) // ResolveTag returns the commit hash associated with a tag. @@ -36,3 +39,120 @@ func ResolveTag(tag *plumbing.Reference, repository *git.Repository) (*plumbing. // Resolving non-commit objects results in an error. return repository.ResolveRevision(plumbing.Revision(tag.Hash().String())) } + +// SortedCommitTags returns the repository's commit object tags sorted by their chronological order in the current branch's history. +// Tags for commits not in the branch's history are returned in lexicographical order relative to their adjacent tags. +func SortedCommitTags(repository *git.Repository) ([]*plumbing.Reference, error) { + /* + Given a repository tag structure like so (I've omitted 1.0.3-1.0.9 as irrelevant): + + * HEAD -> main, tag: 1.0.11 + * tag: 1.0.10 + * tag: 1.0.2 + | * tag: 1.0.2-rc2, development-branch + | * tag: 1.0.2-rc1 + |/ + * tag: 1.0.1 + * tag: 1.0.0 + + The raw tags order is lexicographical: + 1.0.0 + 1.0.1 + 1.0.10 + 1.0.2 + 1.0.2-rc1 + 1.0.2-rc2 + + This order is not meaningful. More meaningful would be to order the tags according to the chronology of the + branch's commit history: + 1.0.0 + 1.0.1 + 1.0.2 + 1.0.10 + + This leaves the question of how to handle tags from other branches, which is likely why a sensible sorting + capability was not provided. However, even if the sorting of those tags is not optimal, a meaningful sort of the + current branch's tags will be a significant improvement over the default behavior. + */ + + headRef, err := repository.Head() + if err != nil { + return nil, err + } + + headCommit, err := repository.CommitObject(headRef.Hash()) + if err != nil { + return nil, err + } + + commits := object.NewCommitIterCTime(headCommit, nil, nil) // Iterator for the head commit and parents in reverse chronological commit time order. + commitMap := make(map[plumbing.Hash]int) // commitMap associates each hash with its chronological position in the branch history. + var commitIndex int + for { // Iterate over all commits. + commit, err := commits.Next() + if err != nil { + // Reached end of commits + break + } + commitMap[commit.Hash] = commitIndex + + commitIndex-- // Decrement to reflect reverse chronological order. + } + + tags, err := repository.Tags() // Get an iterator of the refs of the repository's tags. These are returned in a useless lexicographical order (e.g, 1.0.10 < 1.0.2), so it's necessary to cross-reference them against the commits, which are in a meaningful order. + + type tagDataType struct { + tag *plumbing.Reference + position int + } + var tagData []tagDataType + associatedCommitIndex := commitIndex // Initialize to index of oldest commit in case the first tags aren't in the branch. + var tagIndex int + for { // Iterate over all tag refs. + tag, err := tags.Next() + if err != nil { + // Reached end of tags + break + } + + // Annotated tags have their own hash, different from the commit hash, so tags must be resolved before + // cross-referencing against the commit hashes. + resolvedTag, err := ResolveTag(tag, repository) + if err != nil { + // Non-commit object tags are not included in the sorted list. + continue + } + + commitIndex, ok := commitMap[*resolvedTag] + if ok { + // There is a commit in the branch associated with the tag. + associatedCommitIndex = commitIndex + } + + tagData = append( + tagData, + tagDataType{ + tag: tag, + position: associatedCommitIndex*10000 + tagIndex, // Leave intervals between positions to allow the insertion of unassociated tags in the existing lexicographical order relative to the last associated tag. + }, + ) + + tagIndex++ + } + + // Sort the tags according to the branch's history where possible. + sort.SliceStable( + tagData, + // "less" function + func(thisIndex, otherIndex int) bool { + return tagData[thisIndex].position < tagData[otherIndex].position + }, + ) + + var sortedTags []*plumbing.Reference + for _, tagDatum := range tagData { + sortedTags = append(sortedTags, tagDatum.tag) + } + + return sortedTags, nil +} diff --git a/libraries/gitutils/gitutils_test.go b/libraries/gitutils/gitutils_test.go index 2bedb86e..1c4423ea 100644 --- a/libraries/gitutils/gitutils_test.go +++ b/libraries/gitutils/gitutils_test.go @@ -93,6 +93,52 @@ func TestResolveTag(t *testing.T) { } } +func TestSortedCommitTags(t *testing.T) { + // Create a folder for the test repository. + repositoryPath, err := paths.TempDir().MkTempDir("gitutils-TestSortedTags-repo") + require.Nil(t, err) + + // Create test repository. + repository, err := git.PlainInit(repositoryPath.String(), false) + require.Nil(t, err) + + var tags []*plumbing.Reference + tags = append(tags, makeTag(t, repository, "1.0.0", makeCommit(t, repository, repositoryPath), true)) + // Throw a tree tag into the mix. This should not have any effect. + makeTag(t, repository, "tree-tag", getTreeHash(t, repository), true) + tags = append(tags, makeTag(t, repository, "1.0.1", makeCommit(t, repository, repositoryPath), false)) + + worktree, err := repository.Worktree() + require.Nil(t, err) + worktree.Checkout( + &git.CheckoutOptions{ + Branch: "development-branch", + Create: true, + }, + ) + var branchTags []*plumbing.Reference + branchTags = append(branchTags, makeTag(t, repository, "1.0.2-rc1", makeCommit(t, repository, repositoryPath), true)) + branchTags = append(branchTags, makeTag(t, repository, "1.0.2-rc2", makeCommit(t, repository, repositoryPath), true)) + config, err := repository.Config() + require.Nil(t, err) + worktree.Checkout( + &git.CheckoutOptions{ + Branch: plumbing.ReferenceName(config.Init.DefaultBranch), + Create: false, + }, + ) + + tags = append(tags, makeTag(t, repository, "1.0.2", makeCommit(t, repository, repositoryPath), true)) + // Throw a blob tag into the mix. This should not have any effect. + makeTag(t, repository, "blob-tag", getBlobHash(t, repository), true) + tags = append(tags, branchTags...) + tags = append(tags, makeTag(t, repository, "1.0.10", makeCommit(t, repository, repositoryPath), true)) + + sorted, err := SortedCommitTags(repository) + require.Nil(t, err) + assert.Equal(t, tags, sorted) +} + // makeCommit creates a test commit in the given repository and returns its plumbing.Hash object. func makeCommit(t *testing.T, repository *git.Repository, repositoryPath *paths.Path) plumbing.Hash { _, err := paths.WriteToTempFile([]byte{}, repositoryPath, "gitutils-makeCommit-tempfile") diff --git a/sync_libraries.go b/sync_libraries.go index 729cbba5..c6ad521c 100644 --- a/sync_libraries.go +++ b/sync_libraries.go @@ -34,6 +34,7 @@ import ( "arduino.cc/repository/libraries" "arduino.cc/repository/libraries/db" + "arduino.cc/repository/libraries/gitutils" "arduino.cc/repository/libraries/hash" cc "github.com/arduino/golang-concurrent-workers" "github.com/go-git/go-git/v5" @@ -227,19 +228,13 @@ func syncLibrary(logger *log.Logger, repoMetadata *libraries.Repo, libraryDb *db } // Retrieve the list of git-tags - tags, err := repo.Repository.Tags() + tags, err := gitutils.SortedCommitTags(repo.Repository) if err != nil { logger.Printf("Error retrieving git-tags: %s", err) return } - for { - tag, err := tags.Next() - if err != nil { - // Reached end of tags - break - } - + for _, tag := range tags { // Sync the library release for each git-tag err = syncLibraryTaggedRelease(logger, repo, tag, repoMetadata, libraryDb) if err != nil { @@ -260,8 +255,9 @@ func syncLibraryTaggedRelease(logger *log.Logger, repo *libraries.Repository, ta } // Annotated tags have their own hash, different from the commit hash, so the tag must be resolved before checkout - resolvedTag, err := repo.Repository.ResolveRevision(plumbing.Revision(tag.Hash().String())) + resolvedTag, err := gitutils.ResolveTag(tag, repo.Repository) if err != nil { + // All unresolvable tags were already removed by gitutils.SortedCommitTags(), so there will never be an error under normal conditions. panic(err) }