A version of PlainOpen that detects the root of the repository

[mvdan]

If I have a repository at ~/foo, I can run git status in ~/foo/bar without a problem.

However, with any little tool that I write with this library, I use git.PlainOpen("."). This works fine if I'm at the root of the git repo, but quickly breaks down if I'm in any of its subdirectories.

I propose that a new Open variant be added that would iterate over the parent directories, stopping at the first containing a .git directory or file. This could be func PlainOpenDetect(from string) (*Repository, error), for example, to not have it depend on os.Getwd.

There's also the question of whether anyone would require the current PlainOpen versus this improved one. If the answer is not, perhaps the behavior of PlainOpen could simply be changed.

Happy to submit a patch with tests if this seems like a good idea.

/cc @mcuadros @smola

[mvdan]

Another option would be to expose an API for git rev-parse --show-toplevel, which prints the root directory of a git repository.

Then, one might do:

rootDir, err := git.TopLevelDir()
if err != nil { ... }
repo, err := git.PlainOpen(rootDir)
if err != nil { ... }

But I presume that 9/10 times, people just want the same behavior with Open that the command-line git tool has, so I would simply integrate this with one of the Open funcs.

[mcuadros]

Yep, the first approach looks more direct, maybe we can provide both, a PR is welcome.

[mvdan]

Would you change the existing PlainOpen, or add a new one?

[mcuadros]

Add a new function, since the new behaviour has a performance penalty

[mcuadros]

@mvdan what about:

type OpenOptions struct {
    DetectDotGit bool
}

func OpenWithOptions(s storage.Storer, worktree billy.Filesystem, o *OpenOptions) (*Repository, error)

And in v5 we can deprecate the old Open and replace it with this new one.
And and the detect function as the first flag f

[mvdan]

I think making this the default constructor isn't a great idea. Most people just want to open repos on disk, so they shouldn't concern themselves with Storer or Filesystem.

Also, I'd make options be functions: https://commandcenter.blogspot.co.uk/2014/01/self-referential-functions-and-design.html

So, ideally in a future version, we'd have:

Open() // just like the current PlainOpen
Open(DetectDotGit) // what I want
Open(WithStorer(s), WithWorktree(w)) // and any other optional stuff you'd want

[mcuadros]

We should have the Open with options in the same way that Clone has CloneOptions. So make sense having in the future Open and not OpenWithOptions

About the functions as options, yep maybe we can revisit the idea for v5.

[mvdan]

Gotcha; yes, that was a long-term suggestion. I'll add the clunkier API on the side for now.

[orirawlings]

I think this would be awesome and something I was looking for myself the other day.

I'd also like to request support for the standard GIT_DIR and GIT_WORK_TREE environment variables. I was trying to build a sub-command for git using this library the other day and found myself needing to implement this. I think extending git is a natural usage for this library and it would be really nice to support this.

[mvdan]

@orirawlings isn't GIT_DIR the directory you pass to PlainOpen already? I'm not familiar with worktrees at all, so that's probably best left for a separate proposal issue.

[mvdan]

@mcuadros if we only add OpenWithOptions, then I'd basically have to copy the func body for PlainOpen and replace its Open call - is that correct? That's not particularly great, as I'd like a one-liner like PlainOpen.

As said before, I think that opening a repository from disk in this parent-walking way is going to be the most common repository constructor for everyday users. I understand that PlainOpen may get changed for v5, but while we are in v4, perhaps we can add PlainOpenWithOptions or PlainOpenDetect.

[mcuadros]

PlainOpenWithOptions sounds good