Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ability to specify the git work directory #173

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mcinj
Copy link

@mcinj mcinj commented Aug 26, 2024

  • can specify via cli or with the library, work-tree and git directory values allowing you to specify a repository outside of the current working directory
  • created a repository object that stores work-tree and git directory values which are then used on any 'run' call to the git binary

 - can specify via cli or with the library, work-tree and git directory values
 - created a repository object that stores work-tree and git directory values which are then used on any 'run' call to the git binary
internal/git/git.go Outdated Show resolved Hide resolved
internal/git/git_test.go Outdated Show resolved Hide resolved
Comment on lines +145 to +147
tempDir := tempdir(t, true)
localDir := dir(tempDir, "a-folder", t)
defer func() { os.RemoveAll(localDir) }()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using homemade tempdir instead of t.TempDir that comes with automatic cleanup?

Copy link
Author

@mcinj mcinj Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That function existed prior to this PR, but I believe the reason for it was that it also CDs into the temporary directory.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. It should be addressed outside the scope of this PR

cc @caarlos0

internal/git/git_test.go Outdated Show resolved Hide resolved
main.go Outdated
@@ -53,6 +56,8 @@ func main() {
Build: *build,
Directory: *directory,
TagMode: *tagMode,
WorkTree: *workTree,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this would be better

Suggested change
WorkTree: *workTree,
GitWorkTree: *workTree,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to GitWorkTree

@mcinj mcinj requested a review from ccoVeille August 26, 2024 20:43
Comment on lines +21 to 24
type Repository struct {
GitWorkTree string
GitDirectory string
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I didn't notice all this was in the git package

Maybe using git in git.Repository wasn't the best idea. Maybe @caarlos0 will provide some feedbacks here

func getAllTags(args ...string) ([]string, error) {
tags, err := run(append([]string{"-c", "versionsort.suffix=-", "tag", "--sort=-version:refname"}, args...)...)
// NewRepository creates a Repository. gitworktree and gitdirectory will default to 'pwd' and 'pwd/.git'
func NewRepository(gitworktree, gitdirectory string) (*Repository, error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be overkill when there is only two argument, but an Option pattern here may help a lot for readability.

Suggested change
func NewRepository(gitworktree, gitdirectory string) (*Repository, error) {
func NewRepository(opts...RepositoryOption) (*Repository, error) {

So we can do

git.NewRepository(git.FromDirectory("whatever"))

git.NewRepository(git.FromWorkspace("whatever"))

I let @caarlos0 share his thoughts

Comment on lines +145 to +147
tempDir := tempdir(t, true)
localDir := dir(tempDir, "a-folder", t)
defer func() { os.RemoveAll(localDir) }()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. It should be addressed outside the scope of this PR

cc @caarlos0

@mcinj
Copy link
Author

mcinj commented Aug 31, 2024

@caarlos0, looking forward to any feedback you have. Although the PR is labeled as large due to the number of line changes, most of those are in unit tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants