Please support discovering repositories using the standard git environment variables

[joshtriplett]

git::Repository::discover passes specific default arguments to the underlying git_repository_discover function, which prevents respecting $GIT_DISCOVERY_ACROSS_FILESYSTEM or $GIT_CEILING_DIRECTORIES. Please consider providing a binding that allows passing these values explicitly (as a bool and a &str), and please consider making the default binding respect the default values.

In addition, please also consider providing a function discover_default or similar, which doesn't take a path parameter, and which defaults to $GIT_DIR (or "." if that doesn't exist).

Also see libgit2/libgit2#3711 in libgit2, which requests a new underlying C API for that case. However, it should be easy for git2-rs to support this with or without the underlying function.

[alexcrichton]

Sounds like a good idea to me! I'm not actually sure how one detects when you cross filesystems, but respecting ceiling directories seems reasonable

[joshtriplett]

@alexcrichton You can detect if you've crossed filesystems by calling stat on each directory as you ascend and noticing when the st_dev field changes. libgit2 already does that, if you pass the appropriate parameter to git_repository_discover or git_repository_open_ext.

git2-rs doesn't need any special handling here; it just needs to provide a way to access that functionality in libgit2, and (when libgit2/libgit2#3711 goes in) provide git::Repository::discover_default or similar mapping to git_repository_discover_default.

[alexcrichton]

Oh sorry I may have misunderstood, is there an API we're not binding that exists today?

[joshtriplett]

@alexcrichton Yes: git::Repository::discover currently hardcodes the additional arguments to git_repository_discover, so you can't specify ceiling directories or whether to cross filesystems. There's also no binding to git_repository_open_ext, which would allow similar functionality without having to call git_repository_discover followed by git_repository_open.

Also, as noted in git::Repository::discover, "TODO: this diverges significantly from the libgit2 API". The current git::Repository::discover actually acts like a less flexible version of git_repository_open_ext right now.

I'd suggest adding a new API git::Repository::open_ext that binds to git_repository_open_ext, along with the GIT_REPOSITORY_OPEN_* flags. As far as I can tell, that provides a superset of the functionality of the additional arguments to git_repository_discover, so you wouldn't need to support both.

Separate from that, after libgit2/libgit2#3711 gets merged into libgit2, I'd suggest adding a git::Repository::discover_default that calls that (followed by open, to match git::Repository::discover).

(I'm not bothered by discover calling open rather than just returning the path; there isn't much use for obtaining the path without opening it.)

[alexcrichton]

Gah I knew I shouldn't have defaulted those APIs at the time... But yeah if open_ext is a superset of all others then it seems totally plausible that we could leverage that!

[joshtriplett]

I'll send a pull request for both items, as soon as I finish with the libgit2 changes.

[alexcrichton]

Thanks!

[joshtriplett]

A question on the binding for git_repository_open_env (the replacement for git_repository_discover_default): the latest version ended up taking a const char * parameter, but the common case will be to pass NULL. If I made it open_env<P: AsRef<Path>>(p: Option<P>), then the common case of passing None would require a typecast to specify a specific type implementing the trait AsRef<Path>. I'd really like to avoid that.

Any suggestions for a binding that won't require a typecast for the most common kind of call?

At the moment, the only idea I've come up with involves having two separate functions, one taking no argument and one taking an AsRef<Path> without the Option. That seems unfortunate, though.

[alexcrichton]

Hm... How often would the API be called with None? If that's pretty rare maybe it's not so bad?

[joshtriplett]

@alexcrichton Far more often than it'll be called with Some. Calling the underlying function with NULL will be the common case.

[alexcrichton]

Hm did you mean to say that it'd be more often called with None? I'd actually expect that if this is a "power user" API you'd probably only reach for it if you had the extra info to pass? Or maybe this is a more ergonomic API for other situations?

[joshtriplett]

Hm did you mean to say that it'd be more often called with None?

Yes. You asked how often the API would be called with None, and I said that would happen far more often than it'll be called with a Some.

I'd actually expect that if this is a "power user" API you'd probably only reach for it if you had the extra info to pass?

Not in this case. This is a function to autodetect all the right parameters from environment variables, and the parameter is for if you want to explicitly specify a repository path (overriding GIT_DIR or a search from the current working directory) but still get other information from environment variables. The common case would be passing NULL for the repository path; the parameter is there for the case where you need to override the default behavior.

[alexcrichton]

Oh oops I didn't see the word "than" so I misinterpreted, sorry about that!

Hm so alternatively, if it's possible, we could perhaps switch this all to being a builder anyway. I'm not sure if all the various open-a-repository methods could be consolidated into one builder, but that'd at least solve this problem perhaps?

[joshtriplett]

Might be workable, though it'd take some careful design to make sure it always had a call it could map onto; the calls in libgit2 are somewhat orthogonal, and don't go strictly from less general to more general. I like the idea though.

See https://internals.rust-lang.org/t/default-types-for-generics-to-improve-type-inference/3336 for another alternative: feature(default_type_parameter_fallback). Using that would make life easier at least on unstable compilers.

[alexcrichton]

Hm seems odd that there's no "truly general" API to call here, I figured everything would be a shim around one another... Maybe something could be exposed or added to upstream libgit2 though?

[joshtriplett]

After some iteration, libgit2 now supports a GIT_REPOSITORY_OPEN_FROM_ENV flag to respect the git environment variables.

To use this implementation, the caller will normally want to call git_repository_open_ext(&repo, NULL, GIT_REPOSITORY_OPEN_FROM_ENV, NULL);. However, libgit2's open_ext binding doesn't support passing NULL for the path, and any non-NULL value will override $GIT_DIR. On the other hand, if you don't pass GIT_REPOSITORY_OPEN_FROM_ENV, you shouldn't pass a NULL path, so I don't think it makes sense for open_ext to take an Option for path. (And changing open_ext to take an Option would also break compatibility, though that doesn't matter too much with libgit2 still using a 0.* version.)

Given that, I'm inclined to add the two new flags, leave open_ext as it is, and add a new open_from_env function that doesn't take any parameters. In addition to fixing the problem, this also makes the common case much more convenient.

[joshtriplett]

I just turned this into a pull request, adding the two new flags and the new Repository::open_from_env function.

[joshtriplett]

It looks like updating libgit2-sys to use current libgit2 from Git breaks several tests in the test suite, unrelated to this change. That'll need fixing before this can go in.

[alexcrichton]

Could this elaborate a bit on what "respecting git environment variables" means in the documentation as well? It's not entirely obvious to me at first glance.

[joshtriplett]

Sure, I can elaborate a bit. I'd rather not duplicate the entire libgit2 documentation for this, though.

(Not least of which because git2-rs uses a different license than libgit2, so I can't just copy/paste from libgit2. Though I suppose I could in this case, since I wrote the documentation in question. :) But in the general case, that doesn't work.)

[alexcrichton]

Note that quite a few docs are copy/pasted from libgit2 (I think it's good to be exhaustive here as not everyone will jump to libgit2 docs), but if there's license shenanigans that should likely be mentioned in the README or something like that.

[alexcrichton]

Looks good to me, thanks!

This may have to do with the fact that some of the FFI definitions have broken as evidenced by the systest suite I believe (bottom of the travis logs). If it gets too hard trying to fix I can also help out as well to get this landed.

[joshtriplett]

Updated for current libgit2.

[alexcrichton]

Oh dear sorry I forgot about this!

[joshtriplett]

Not a problem; this PR needed updating before merging anyway. Thanks for doing the work to update the version of libgit2!