Add `list` and `repair` commands #38

vdye · 2023-03-27T22:27:44Z

Part of #35

Summary

This pull request introduces two new commands to the bundle server CLI: list and repair. The list command lists the route and remote URL pairs for each route active within the bundle server. It is implemented by the first three commits:

Commit 1 adds a missing mock method for GitHelper
Commit 2 adds a new method to GitHelper to retrieve the origin remote URL
Commit 3 implements the list command, including documentation

The repair command is meant to be a general-purpose tool for keeping the bundle server internally consistent. The first subcommand (implemented here) is the routes option, which checks the repo content in the reporoot directory and compares it to the routes file. The remaining commits implement it:

Commit 4 removes an unused function mock
Commit 5 adds a FileSystem helper function for recursively listing directory contents
Commit 6 replaces any remaining manual path concatenations associated with RepoProvider with filepath.Joins
Commit 7 adds a RepoProvider function for getting the repo content that exists at reporoot (independent of the routes file)
Commit 8 makes the method to write the routes file accessible to external packages
Commit 9 implements the repair routes command, including documentation and two options (--dry-run and --start-all)

Future work

There's lots of room for future improvement on these commands/things related to them, including:

adding an option to have list include the "inactive" repos not in the routes file
adding an --all option to start or stop to enable or disable all repos in the server at once
adding a --cleanup flag to repair routes to delete files that aren't part of a valid repository in the reporoot
adding more subcommands to repair, such as bundle (to correct the bundle list(s))
store all repos tracked by the bundle server in the routes file, each having an Active boolean flag to indicate active status

The 'UpdateBareRepo()' method was added to the 'GitHelper' interface in 62f35ce (bundles: fetch changes before building incremental bundle, 2023-03-14), but the corresponding mock method was not added to 'MockGitHelper'. Add it so that 'MockGitHelper' can be used as a 'GitHelper' mock in later commits. Signed-off-by: Victoria Dye <[email protected]>

Add a 'GetRemoteUrl()' function to 'git.GitHelper', which calls 'git remote get-url origin' on a given repository path and returns the whitespace- trimmed remote URL. Neither stdout nor stderr are printed to the console, but the stderr text is included in the error message if Git exits with an error status. Although it is not used in this patch, 'GetRemoteUrl()' will be used in later commits to display information about the configuration of the bundle server. Signed-off-by: Victoria Dye <[email protected]>

Add a new 'git-bundle-server' command to list the routes that are currently configured and their remote Git URLs. Update manpage and 'README.md' with description and usage information. Due to how routes are currently stored in the bundle server, only "active" routes are listed (that is, routes that have not been stopped with 'git-bundle-server stop'). This is intended to be only a temporary issue, with future updates to how routes are stored allowing better tracking of "stopped" routes. Signed-off-by: Victoria Dye <[email protected]>

derrickstolee

Good changes. Looking forward to integration tests that let us check these CLI changes on a real filesystem.

derrickstolee · 2023-03-28T13:12:08Z

internal/core/repo_test.go

 	for _, tt := range getRepositoriesTests {
 		t.Run(tt.title, func(t *testing.T) {
-			testFileSystem.On("UserHomeDir").Return("/my/test/dir", nil)
 			testFileSystem.On("ReadFileLines",


I suppose this means we are not checking that each mock is called at least once?

In that test, we weren't. There is a function for doing that (mock.AssertExpectationsForObjects()), so I've also added that to TestRepos_GetRepositories().

internal/core/repo_test.go

cmd/git-bundle-server/repair.go

derrickstolee · 2023-03-28T13:38:56Z

internal/utils/maps.go

+		}
+	}
+
+	return


Interesting that since you named the return values in the function prototype, you don't need to list them here.

Remove mock of 'UserHomeDir()' for the 'FileSystem' interface leftover from an earlier iteration of de58f24 (repo: add more GetRepositories() tests, fix bug, 2023-01-31). The function was removed by the time the commit was merged, so the mock can safely be deleted. Add a call to 'mock.AssertExpectationsForObjects()' to ensure all mocked functions are called. This function should be called in all mocked unit tests to ensure similar issues do not occur in the future. Signed-off-by: Victoria Dye <[email protected]>

Add the 'ReadDirRecursive()' function to 'FileSystem' to facilitate easy lookup of filesystem entries at a fixed depth from the root. In later commits, this will be used to look up routes in the bundle server under the 'reporoot' on the filesystem. 'ReadDirRecursive()' takes three arguments: * 'path': the root directory into which we want to recurse * 'depth': the number of times to recurse into 'path'. A depth of 0 will return nothing, a depth of 1 will return all entries inside 'path', a depth of 2 will return all entries inside the subdirectories of 'path', etc. * 'strictDepth': a flag indicating whether to return only entries that are *exactly 'depth' levels beneath 'path'. If 'false', 'ReadDirRecursive()' will also include files and empty directories at depths less than 'depth'. For example, consider the following directory tree: . ├── another-dir/ │ └── testfile ├── myfile ├── org/ │ └── repo/ │ ├── deeper/ │ └── repofile └── too-shallow/ 'ReadDirRecursive(".", 2, true)' will return: - ./another-dir/testfile - ./org/repo/ Whereas 'ReadDirRecursive(".", 2, false)' will return: - ./another-dir/testfile - ./myfile - ./org/repo/ - ./too-shallow/ Signed-off-by: Victoria Dye <[email protected]>

Replace manual path joining via string concatenation with use of 'filepath.Join()'. The path normalization provided by the function helps avoid issues caused by missing (or extra) path separators and aligns paths to the to the appropriate filesystem's conventions. Signed-off-by: Victoria Dye <[email protected]>

Add a function to the 'core.RepositoryProvider' that reads the on-disk repository storage for the bundle server and returns the valid repos in a route -> 'core.Repository' map. 'ReadRepositoryStorage()' identifies repositories by finding the directories at a depth of 2 from the repo storage root, then checking that the repository has a remote 'origin'. The result includes the "active" routes in the bundle server (stored in the 'routes' file) as well as those that have been removed from the routes file with 'git-bundle-server stop'. In later commits, this information will be used to provide more complete information about the state of the bundle server and configure "repairs" to a corrupted server. Signed-off-by: Victoria Dye <[email protected]>

Rename the 'core.RepositoryProvider' function 'writeRouteFile()' to 'WriteAllRoutes()' to make it accessible from other packages. The ability to make "bulk" changes to the repository registry and commit all at once will be useful in future patches, such as rebuilding the route list based on the repositories on disk. Signed-off-by: Victoria Dye <[email protected]>

Create a new command 'repair' to fix common issues with the bundle server's internal storage. Start by implementing the 'routes' subcommand, which (by default) removes routes from the 'routes' file if they do not appear (or are not Git repositories) on disk. Include two additional options: - '--dry-run', which gathers repository information and constructs the list of repairs needed, but does not update anything - '--start-all', which adds any unregistered, valid repositories on disk to the 'routes' file (effectively running 'git-bundle-server start' on all) Signed-off-by: Victoria Dye <[email protected]>

vdye added 3 commits March 27, 2023 14:44

vdye added this to the v1.0 milestone Mar 27, 2023

vdye self-assigned this Mar 27, 2023

vdye requested review from derrickstolee and ldennington as code owners March 27, 2023 22:27

derrickstolee approved these changes Mar 28, 2023

View reviewed changes

vdye added 6 commits March 28, 2023 10:59

vdye force-pushed the vdye/list-repair branch from 5c01ff6 to 2694cb1 Compare March 28, 2023 18:02

vdye merged commit 4ba9ca2 into main Mar 30, 2023

vdye deleted the vdye/list-repair branch March 30, 2023 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add `list` and `repair` commands #38

Add `list` and `repair` commands #38

Uh oh!

vdye commented Mar 27, 2023

Uh oh!

derrickstolee left a comment

Uh oh!

derrickstolee Mar 28, 2023

Uh oh!

vdye Mar 28, 2023

Uh oh!

Uh oh!

Uh oh!

derrickstolee Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

+              		}
+              	}
+              	return

Add list and repair commands #38

Add list and repair commands #38

Uh oh!

Conversation

vdye commented Mar 27, 2023

Summary

Future work

Uh oh!

derrickstolee left a comment

Choose a reason for hiding this comment

Uh oh!

derrickstolee Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

vdye Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

derrickstolee Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add `list` and `repair` commands #38

Add `list` and `repair` commands #38