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

[maguro]

To me

r, err := git.Open(s, worktree, WithGlobalConfig(), DetectDotGit())

seems more elegant. If you don't want to use options you get the backwards compatible

r, err := git.Open(s, worktree)

[mvdan]

I agree, but that breaks backwards compatibility slightly. And the v4 API would be inconsistent; some funcs would use option funcs, others option structs.

[maguro]

How would backward compatibility be broken?

[mvdan]

Changing a func signature is not a big backwards compatibility break, but it can break programs nevertheless. Imagine if PlainOpen was passed around as a func literal, for example.

[maguro]

Ouch, that seems like an obtuse example.

[maguro]

So, what's the consensus?

[mvdan]

PlainOpenWithOptions for the current v4, as you can see above.

[maguro]

Uh, it's not checked in so I thought it was open to discussion. I guess not. I'll work with *WithOptions() constructors.

[maguro]

I'm sorry, but I just have to point out the confusing cornucopia of constructors and then I'll get back to work:

Clone() // you can pass options
CloneContext()  // you can pass options
Init() // no options
Open() // no options
PlainClone()  // you can pass options
PlainCloneContext() // you can pass options
PlainInit()  // no options
PlainOpen() // no options
PlainOpenWithOptions() // you can pass options

as opposed to

Clone(url string, opts ...CloneOption)
Init(s storage.Storer, wt billy.Filesystem, opts ...InitOption)
Open(s storage.Storer, wt billy.Filesystem, opts ...OpenOption)

Why not cut a v5? (just throwing out an idea)

[maguro]

So to add #760 working, I'm going to have to create additional constructors:

InitWithOptions()
OpenWithOptions()

If we wanted init, open, plain open with options, would we want PlainInitWithOptions() as well?

[mvdan]

v4 was made stable just over two months ago, so I doubt they're going to deprecate it anytime soon. But yes, once v5 comes, all of this can be cleaned up in favor of a more consistent and flexible API design.

[mcuadros]

yup, we should clean up this for v5