Merge pull request #42 from per1234/reset

Improve handling of erroneous dirty library repo state after release checkout

Author: per1234

.prettierignore

@@ -1,3 +1,3 @@
 # See: https://prettier.io/docs/en/ignore.html#ignoring-files-prettierignore
 
-/test/testdata/golden/logs/
+/test/testdata/test_all/golden/logs/

internal/libraries/git_integration_test.go

@@ -31,7 +31,6 @@ import (
 
 	"arduino.cc/repository/internal/libraries/db"
 	"arduino.cc/repository/internal/libraries/gitutils"
-	"github.com/go-git/go-git/v5"
 	"github.com/stretchr/testify/require"
 )
 
@@ -63,15 +62,7 @@ func TestUpdateLibraryJson(t *testing.T) {
 		tag, err := tags.Next()
 		require.NoError(t, err)
 
-		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 := gitutils.ResolveTag(tag, r.Repository)
-		require.NoError(t, err)
-		err = repoTree.Checkout(&git.CheckoutOptions{Hash: *resolvedTag, Force: true})
-		require.NoError(t, err)
-		err = repoTree.Clean(&git.CleanOptions{Dir: true})
-		require.NoError(t, err)
+		err = gitutils.CheckoutTag(r.Repository, tag)
 
 		library, err := GenerateLibraryFromRepo(r)
 		require.NoError(t, err)

internal/libraries/gitutils/gitutils.go

@@ -24,15 +24,16 @@
 package gitutils
 
 import (
+	"fmt"
 	"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.
-func ResolveTag(tag *plumbing.Reference, repository *git.Repository) (*plumbing.Hash, error) {
+// 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.
@@ -117,7 +118,7 @@ func SortedCommitTags(repository *git.Repository) ([]*plumbing.Reference, error)
 
 		// 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)
+		resolvedTag, err := resolveTag(tag, repository)
 		if err != nil {
 			// Non-commit object tags are not included in the sorted list.
 			continue
@@ -156,3 +157,66 @@ func SortedCommitTags(repository *git.Repository) ([]*plumbing.Reference, error)
 
 	return sortedTags, nil
 }
+
+// CheckoutTag checks out the repository to the given tag.
+func CheckoutTag(repository *git.Repository, tag *plumbing.Reference) error {
+	repoTree, err := repository.Worktree()
+	if err != nil {
+		return err
+	}
+
+	// Annotated tags have their own hash, different from the commit hash, so the tag must be resolved before checkout
+	resolvedTag, err := resolveTag(tag, repository)
+	if err != nil {
+		return err
+	}
+
+	if err = repoTree.Checkout(&git.CheckoutOptions{Hash: *resolvedTag, Force: true}); err != nil {
+		return err
+	}
+
+	// Ensure the repository is checked out to a clean state.
+	// Because it might not succeed on the first attempt, a retry is allowed.
+	for range [2]int{} {
+		clean, err := cleanRepository(repoTree)
+		if err != nil {
+			return err
+		}
+		if clean {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("failed to get repository to clean state")
+}
+
+func cleanRepository(repoTree *git.Worktree) (bool, error) {
+	// Remove now-empty folders which are left behind after checkout. These would not be removed by the reset action.
+	// Remove untracked files. These would also be removed by the reset action.
+	if err := repoTree.Clean(&git.CleanOptions{Dir: true}); err != nil {
+		return false, err
+	}
+
+	// Remove untracked files and reset tracked files to clean state.
+	// Even though in theory it shouldn't ever be necessary to do a hard reset in this application, under certain
+	// circumstances, go-git can fail to complete checkout, while not even returning an error. This results in an
+	// unexpected dirty repository state, which is corrected via a hard reset.
+	// See: https://github.com/go-git/go-git/issues/99
+	if err := repoTree.Reset(&git.ResetOptions{Mode: git.HardReset}); err != nil {
+		return false, err
+	}
+
+	// Get status to detect some forms of failed cleaning.
+	repoStatus, err := repoTree.Status()
+	if err != nil {
+		return false, err
+	}
+
+	// IsClean() detects:
+	// - Untracked files
+	// - Modified tracked files
+	// This does not detect:
+	// - Empty directories
+	// - Ignored files
+	return repoStatus.IsClean(), nil
+}

internal/libraries/gitutils/gitutils_test.go

@@ -84,7 +84,7 @@ func TestResolveTag(t *testing.T) {
 		} {
 			testName := fmt.Sprintf("%s, %s", testTable.objectTypeName, annotationConfig.descriptor)
 			tag := makeTag(t, repository, testName, testTable.objectHash, annotationConfig.annotated)
-			resolvedTag, err := ResolveTag(tag, repository)
+			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))
@@ -139,9 +139,69 @@ func TestSortedCommitTags(t *testing.T) {
 	assert.Equal(t, tags, sorted)
 }
 
+func TestCheckoutTag(t *testing.T) {
+	// Create a folder for the test repository.
+	repositoryPath, err := paths.TempDir().MkTempDir("gitutils-TestCheckoutTag-repo")
+	require.NoError(t, err)
+
+	// Create test repository.
+	repository, err := git.PlainInit(repositoryPath.String(), false)
+	require.NoError(t, err)
+
+	// Generate meaningless commit history, creating some tags along the way.
+	var tags []*plumbing.Reference
+	tags = append(tags, makeTag(t, repository, "1.0.0", makeCommit(t, repository, repositoryPath), true))
+	makeCommit(t, repository, repositoryPath)
+	makeCommit(t, repository, repositoryPath)
+	tags = append(tags, makeTag(t, repository, "1.0.1", makeCommit(t, repository, repositoryPath), true))
+	makeCommit(t, repository, repositoryPath)
+	makeTag(t, repository, "tree-tag", getTreeHash(t, repository), true)
+	tags = append(tags, makeTag(t, repository, "1.0.2", makeCommit(t, repository, repositoryPath), true))
+	makeTag(t, repository, "blob-tag", getBlobHash(t, repository), true)
+	trackedFilePath, _ := commitFile(t, repository, repositoryPath)
+
+	for _, tag := range tags {
+		// Put the repository into a dirty state.
+		// Add an untracked file.
+		_, err = paths.WriteToTempFile([]byte{}, repositoryPath, "gitutils-TestCheckoutTag-tempfile")
+		require.NoError(t, err)
+		// Modify a tracked file.
+		err = trackedFilePath.WriteFile([]byte{42})
+		require.NoError(t, err)
+		// Create empty folder.
+		emptyFolderPath, err := repositoryPath.MkTempDir("gitutils-TestCheckoutTag-emptyFolder")
+		require.NoError(t, err)
+
+		err = CheckoutTag(repository, tag)
+		assert.NoError(t, err, fmt.Sprintf("Checking out tag %s", tag))
+
+		expectedHash, err := resolveTag(tag, repository)
+		require.NoError(t, err)
+		headRef, err := repository.Head()
+		require.NoError(t, err)
+		assert.Equal(t, *expectedHash, headRef.Hash(), "HEAD is at tag")
+
+		// Check if cleanup was successful.
+		tree, err := repository.Worktree()
+		require.NoError(t, err)
+		status, err := tree.Status()
+		require.NoError(t, err)
+		assert.True(t, status.IsClean(), "Repository is clean")
+		emptyFolderExists, err := emptyFolderPath.ExistCheck()
+		require.NoError(t, err)
+		assert.False(t, emptyFolderExists, "Empty folder was removed")
+	}
+}
+
 // 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")
+	_, hash := commitFile(t, repository, repositoryPath)
+	return hash
+}
+
+// commitFile commits a file in the given repository and returns its path and the commit's plumbing.Hash object.
+func commitFile(t *testing.T, repository *git.Repository, repositoryPath *paths.Path) (*paths.Path, plumbing.Hash) {
+	filePath, err := paths.WriteToTempFile([]byte{}, repositoryPath, "gitutils-makeCommit-tempfile")
 	require.Nil(t, err)
 
 	worktree, err := repository.Worktree()
@@ -163,7 +223,7 @@ func makeCommit(t *testing.T, repository *git.Repository, repositoryPath *paths.
 	)
 	require.Nil(t, err)
 
-	return commit
+	return filePath, commit
 }
 
 // getTreeHash returns the plumbing.Hash object for an arbitrary Git tree object.

sync_libraries.go

@@ -37,7 +37,6 @@ import (
 	"arduino.cc/repository/internal/libraries/gitutils"
 	"arduino.cc/repository/internal/libraries/hash"
 	cc "github.com/arduino/golang-concurrent-workers"
-	"github.com/go-git/go-git/v5"
 	"github.com/go-git/go-git/v5/plumbing"
 )
 
@@ -249,27 +248,10 @@ func syncLibraryTaggedRelease(logger *log.Logger, repo *libraries.Repository, ta
 
 	// Checkout desired tag
 	logger.Printf("Checking out tag: %s", tag.Name().Short())
-
-	repoTree, err := repo.Repository.Worktree()
-	if err != nil {
-		return err
-	}
-
-	// Annotated tags have their own hash, different from the commit hash, so the tag must be resolved before checkout
-	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)
-	}
-
-	if err = repoTree.Checkout(&git.CheckoutOptions{Hash: *resolvedTag, Force: true}); err != nil {
+	if err := gitutils.CheckoutTag(repo.Repository, tag); err != nil {
 		return fmt.Errorf("Error checking out repo: %s", err)
 	}
 
-	if err = repoTree.Clean(&git.CleanOptions{Dir: true}); err != nil {
-		return fmt.Errorf("Error cleaning repo: %s", err)
-	}
-
 	// Create library metadata from library.properties
 	library, err := libraries.GenerateLibraryFromRepo(repo)
 	if err != nil {

test/test_all.py

@@ -58,7 +58,7 @@ def test_all(run_command, working_dir):
 
     libraries_repository_engine_command = [
         working_dir_path.joinpath("config.json"),
-        test_data_path.joinpath("repos.txt"),
+        test_data_path.joinpath("test_all", "repos.txt"),
     ]
 
     # Run the engine
@@ -69,12 +69,12 @@ def test_all(run_command, working_dir):
     check_libraries(configuration=configuration)
     check_logs(
         configuration=configuration,
-        golden_logs_parent_path=test_data_path.joinpath("golden", "logs", "generate"),
+        golden_logs_parent_path=test_data_path.joinpath("test_all", "golden", "logs", "generate"),
         logs_subpath=pathlib.Path("github.com", "arduino-libraries", "ArduinoCloudThing", "index.html"),
     )
     check_logs(
         configuration=configuration,
-        golden_logs_parent_path=test_data_path.joinpath("golden", "logs", "generate"),
+        golden_logs_parent_path=test_data_path.joinpath("test_all", "golden", "logs", "generate"),
         logs_subpath=pathlib.Path("github.com", "arduino-libraries", "SpacebrewYun", "index.html"),
     )
     check_index(configuration=configuration)
@@ -87,12 +87,12 @@ def test_all(run_command, working_dir):
     check_libraries(configuration=configuration)
     check_logs(
         configuration=configuration,
-        golden_logs_parent_path=test_data_path.joinpath("golden", "logs", "update"),
+        golden_logs_parent_path=test_data_path.joinpath("test_all", "golden", "logs", "update"),
         logs_subpath=pathlib.Path("github.com", "arduino-libraries", "ArduinoCloudThing", "index.html"),
     )
     check_logs(
         configuration=configuration,
-        golden_logs_parent_path=test_data_path.joinpath("golden", "logs", "update"),
+        golden_logs_parent_path=test_data_path.joinpath("test_all", "golden", "logs", "update"),
         logs_subpath=pathlib.Path("github.com", "arduino-libraries", "SpacebrewYun", "index.html"),
     )
     check_index(configuration=configuration)
@@ -160,7 +160,9 @@ def check_index(configuration):
         del release["checksum"]
 
     # Load golden index
-    golden_library_index_template = test_data_path.joinpath("golden", "library_index.json").read_text(encoding="utf-8")
+    golden_library_index_template = test_data_path.joinpath("test_all", "golden", "library_index.json").read_text(
+        encoding="utf-8"
+    )
     # Fill in mutable content
     golden_library_index_string = string.Template(template=golden_library_index_template).substitute(
         base_download_url=configuration["BaseDownloadUrl"]
@@ -173,6 +175,43 @@ def check_index(configuration):
         assert release in golden_library_index["libraries"]
 
 
+# The engine's Git code struggles to get a clean checkout of releases under some circumstances.
+def test_clean_checkout(run_command, working_dir):
+    working_dir_path = pathlib.Path(working_dir)
+    configuration = {
+        "BaseDownloadUrl": "http://www.example.com/libraries/",
+        "LibrariesFolder": working_dir_path.joinpath("libraries").as_posix(),
+        "LogsFolder": working_dir_path.joinpath("logs").as_posix(),
+        "LibrariesDB": working_dir_path.joinpath("libraries_db.json").as_posix(),
+        "LibrariesIndex": working_dir_path.joinpath("libraries", "library_index.json").as_posix(),
+        "GitClonesFolder": working_dir_path.joinpath("gitclones").as_posix(),
+        "DoNotRunClamav": True,
+        # Arduino Lint should be installed under PATH
+        "ArduinoLintPath": "",
+    }
+
+    # Generate configuration file
+    with working_dir_path.joinpath("config.json").open("w", encoding="utf-8") as configuration_file:
+        json.dump(obj=configuration, fp=configuration_file, indent=2)
+
+    libraries_repository_engine_command = [
+        working_dir_path.joinpath("config.json"),
+        test_data_path.joinpath("test_clean_checkout", "repos.txt"),
+    ]
+
+    # Run the engine
+    result = run_command(cmd=libraries_repository_engine_command)
+    assert result.ok
+
+    # Load generated index
+    with pathlib.Path(configuration["LibrariesIndex"]).open(mode="r", encoding="utf-8") as library_index_file:
+        library_index = json.load(fp=library_index_file)
+
+    for release in library_index["libraries"]:
+        # ssd1306@1.0.0 contains a .exe and so should fail validation.
+        assert not (release["name"] == "ssd1306" and release["version"] == "1.0.0")
+
+
 @pytest.fixture(scope="function")
 def run_command(pytestconfig, working_dir) -> typing.Callable[..., invoke.runners.Result]:
     """Provide a wrapper around invoke's `run` API so that every test will work in the same temporary folder.