From 46b1a657945e85360cf8ea667789e05531c0c31d Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 27 May 2021 17:28:12 -0700 Subject: [PATCH 1/5] Move tag checkout code to dedicated function The code for checking out tags was duplicated. Already fairly complex, it turns out to need even more code to work reliably. For this reason, it will be beneficial to move it to a function in the gitutils package to avoid code duplication and facilitate testing. --- internal/libraries/git_integration_test.go | 11 +--- internal/libraries/gitutils/gitutils.go | 30 ++++++++- internal/libraries/gitutils/gitutils_test.go | 66 +++++++++++++++++++- sync_libraries.go | 20 +----- 4 files changed, 92 insertions(+), 35 deletions(-) diff --git a/internal/libraries/git_integration_test.go b/internal/libraries/git_integration_test.go index 77a09ea8..30c3d0f5 100644 --- a/internal/libraries/git_integration_test.go +++ b/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) diff --git a/internal/libraries/gitutils/gitutils.go b/internal/libraries/gitutils/gitutils.go index 80dbee76..ed3ab02c 100644 --- a/internal/libraries/gitutils/gitutils.go +++ b/internal/libraries/gitutils/gitutils.go @@ -31,8 +31,8 @@ import ( "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 +117,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 +156,27 @@ 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 + } + + if err = repoTree.Clean(&git.CleanOptions{Dir: true}); err != nil { + return err + } + + return nil +} diff --git a/internal/libraries/gitutils/gitutils_test.go b/internal/libraries/gitutils/gitutils_test.go index 1c4423ea..53f8f411 100644 --- a/internal/libraries/gitutils/gitutils_test.go +++ b/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. diff --git a/sync_libraries.go b/sync_libraries.go index 21fcbd5d..0c38cda1 100644 --- a/sync_libraries.go +++ b/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 { From e7bbb3d184a4d4355dad333c62083827262d28ee Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 27 May 2021 18:13:33 -0700 Subject: [PATCH 2/5] Add integration test for clean checkout The engine's Git code is failing to get a clean checkout of releases under some circumstances. This resulted in the ssd1306@1.0.0 release being accepted even though the repository contains a .exe at that tag. However, the engine's checkout code fails to add the .exe when checking out this tag, leaving the repository in a dirty state that just so happens to allow it to pass validation. Although harmless in this particular situation, this could cause big problems if essential files went missing from releases, and so must be tested for. --- .prettierignore | 2 +- test/test_all.py | 51 ++++++++++++++++--- .../{ => test_all}/golden/library_index.json | 0 .../ArduinoCloudThing/index.html | 0 .../arduino-libraries/SpacebrewYun/index.html | 0 .../ArduinoCloudThing/index.html | 0 .../arduino-libraries/SpacebrewYun/index.html | 0 test/testdata/{ => test_all}/repos.txt | 0 test/testdata/test_clean_checkout/repos.txt | 1 + 9 files changed, 47 insertions(+), 7 deletions(-) rename test/testdata/{ => test_all}/golden/library_index.json (100%) rename test/testdata/{ => test_all}/golden/logs/generate/github.com/arduino-libraries/ArduinoCloudThing/index.html (100%) rename test/testdata/{ => test_all}/golden/logs/generate/github.com/arduino-libraries/SpacebrewYun/index.html (100%) rename test/testdata/{ => test_all}/golden/logs/update/github.com/arduino-libraries/ArduinoCloudThing/index.html (100%) rename test/testdata/{ => test_all}/golden/logs/update/github.com/arduino-libraries/SpacebrewYun/index.html (100%) rename test/testdata/{ => test_all}/repos.txt (100%) create mode 100644 test/testdata/test_clean_checkout/repos.txt diff --git a/.prettierignore b/.prettierignore index df8e1481..fdbfccbf 100644 --- a/.prettierignore +++ b/.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/ diff --git a/test/test_all.py b/test/test_all.py index 9752c4b7..430eee25 100644 --- a/test/test_all.py +++ b/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. diff --git a/test/testdata/golden/library_index.json b/test/testdata/test_all/golden/library_index.json similarity index 100% rename from test/testdata/golden/library_index.json rename to test/testdata/test_all/golden/library_index.json diff --git a/test/testdata/golden/logs/generate/github.com/arduino-libraries/ArduinoCloudThing/index.html b/test/testdata/test_all/golden/logs/generate/github.com/arduino-libraries/ArduinoCloudThing/index.html similarity index 100% rename from test/testdata/golden/logs/generate/github.com/arduino-libraries/ArduinoCloudThing/index.html rename to test/testdata/test_all/golden/logs/generate/github.com/arduino-libraries/ArduinoCloudThing/index.html diff --git a/test/testdata/golden/logs/generate/github.com/arduino-libraries/SpacebrewYun/index.html b/test/testdata/test_all/golden/logs/generate/github.com/arduino-libraries/SpacebrewYun/index.html similarity index 100% rename from test/testdata/golden/logs/generate/github.com/arduino-libraries/SpacebrewYun/index.html rename to test/testdata/test_all/golden/logs/generate/github.com/arduino-libraries/SpacebrewYun/index.html diff --git a/test/testdata/golden/logs/update/github.com/arduino-libraries/ArduinoCloudThing/index.html b/test/testdata/test_all/golden/logs/update/github.com/arduino-libraries/ArduinoCloudThing/index.html similarity index 100% rename from test/testdata/golden/logs/update/github.com/arduino-libraries/ArduinoCloudThing/index.html rename to test/testdata/test_all/golden/logs/update/github.com/arduino-libraries/ArduinoCloudThing/index.html diff --git a/test/testdata/golden/logs/update/github.com/arduino-libraries/SpacebrewYun/index.html b/test/testdata/test_all/golden/logs/update/github.com/arduino-libraries/SpacebrewYun/index.html similarity index 100% rename from test/testdata/golden/logs/update/github.com/arduino-libraries/SpacebrewYun/index.html rename to test/testdata/test_all/golden/logs/update/github.com/arduino-libraries/SpacebrewYun/index.html diff --git a/test/testdata/repos.txt b/test/testdata/test_all/repos.txt similarity index 100% rename from test/testdata/repos.txt rename to test/testdata/test_all/repos.txt diff --git a/test/testdata/test_clean_checkout/repos.txt b/test/testdata/test_clean_checkout/repos.txt new file mode 100644 index 00000000..722e1a06 --- /dev/null +++ b/test/testdata/test_clean_checkout/repos.txt @@ -0,0 +1 @@ +https://github.com/lexus2k/ssd1306.git|Contributed|ssd1306 From 356139bcba9e69cbcfee6d1db4b985e04a589943 Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 27 May 2021 18:33:15 -0700 Subject: [PATCH 3/5] Do hard reset after checkout 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. The resulting dirty repository state would cause Library Manager to serve a corrupted release. A hard reset after each checkout ensures that the repository is at its tagged state. --- internal/libraries/gitutils/gitutils.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/libraries/gitutils/gitutils.go b/internal/libraries/gitutils/gitutils.go index ed3ab02c..57984a77 100644 --- a/internal/libraries/gitutils/gitutils.go +++ b/internal/libraries/gitutils/gitutils.go @@ -174,9 +174,20 @@ func CheckoutTag(repository *git.Repository, tag *plumbing.Reference) error { return err } + // 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 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 err + } + return nil } From 34ca7031457b8fd3327b06202c1940e1608a4b3e Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 27 May 2021 19:28:12 -0700 Subject: [PATCH 4/5] Error checkout if repository is dirty Under certain circumstances, go-git can fail to complete checkout, while not even returning an error. While the clean and hard reset processes should ensure this is corrected, the fact that it is necessary casts doubt on the reliability of go-git. It's very important that the releases distributed by Library Manager match the repository, so it's better to reject any releases that can't be brought into a clean state. --- internal/libraries/gitutils/gitutils.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/internal/libraries/gitutils/gitutils.go b/internal/libraries/gitutils/gitutils.go index 57984a77..d31c575b 100644 --- a/internal/libraries/gitutils/gitutils.go +++ b/internal/libraries/gitutils/gitutils.go @@ -24,6 +24,7 @@ package gitutils import ( + "fmt" "sort" "github.com/go-git/go-git/v5" @@ -189,5 +190,13 @@ func CheckoutTag(repository *git.Repository, tag *plumbing.Reference) error { return err } + repoStatus, err := repoTree.Status() + if err != nil { + return err + } + if !repoStatus.IsClean() { + return fmt.Errorf("failed to get repository to clean state") + } + return nil } From d29bc2d54fc4edef7b085ac88e1f5c2ee611bb7c Mon Sep 17 00:00:00 2001 From: per1234 Date: Thu, 27 May 2021 20:11:01 -0700 Subject: [PATCH 5/5] Make two attempts to clean repository after checkout It's not certain that there is any point in another try after a first unsuccessful cleaning attempt. However, go-git's checkout code also does a hard reset, but still leaves the repository dirty under certain circumstances. Running an identical hard reset procedure again gets the repository to a clean state, so this indicates a possibility that a retry might be successful. --- internal/libraries/gitutils/gitutils.go | 38 +++++++++++++++++++------ 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/internal/libraries/gitutils/gitutils.go b/internal/libraries/gitutils/gitutils.go index d31c575b..e45bb71b 100644 --- a/internal/libraries/gitutils/gitutils.go +++ b/internal/libraries/gitutils/gitutils.go @@ -175,10 +175,26 @@ func CheckoutTag(repository *git.Repository, tag *plumbing.Reference) error { 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 err + if err := repoTree.Clean(&git.CleanOptions{Dir: true}); err != nil { + return false, err } // Remove untracked files and reset tracked files to clean state. @@ -186,17 +202,21 @@ func CheckoutTag(repository *git.Repository, tag *plumbing.Reference) error { // 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 err + 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 err - } - if !repoStatus.IsClean() { - return fmt.Errorf("failed to get repository to clean state") + return false, err } - return nil + // IsClean() detects: + // - Untracked files + // - Modified tracked files + // This does not detect: + // - Empty directories + // - Ignored files + return repoStatus.IsClean(), nil }