Merged
Conversation
299123c to
7c5ab08
Compare
fbf3aef to
306228b
Compare
a35d01e to
9e2ed48
Compare
mislav
suggested changes
Sep 29, 2022
Contributor
mislav
left a comment
There was a problem hiding this comment.
Awesome start; thank you!
It's potentially a bit confusing that old-style git helpers like git.GitCommand() and git.CurrentBranch() are in the same package as the new git.Client, but I suspect you chose that to ease the gradual migration.
The main changes that I'd like to suggest for the new git Client is that it's concurrency-safe and that any I/O-bound operations take in a Context as the first argument.
Contributor
Author
|
@mislav I addressed the changes you request if you wouldn't mind taking another look 🙇 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce the concept of a git client to be used for interacting with local git executable. I decided to keep the old interface for interacting with git in order to not break all the tests. I also kept in calls to
run.PrepareCmdfor this same reason. I was able to remove most of the calls torun.PrepareCmdas they are now consolidated in the new client. This PR maintains all current functionality and should result in no functional changes. None of the existing test assertions were changed.In a future PR I will start inlining calls to the old interface with the equivalent client methods. I will also convert calls to git commands that need authentication to use the new
client.AuthenticatedCommandmethod. These changes should be straight forward but would have ballooned this PR and resulted in test changes.This PR also adds the a
GitClientto the factory struct for convenience going forward.