Allow libgit2# to redirect the global/system/xdg config search path #1123
Conversation
rcorre
force-pushed
the
search-path
branch
3 times, most recently
from
555c275
to
3cd756e
Compare
| internal static extern int git_libgit2_opts(int option, uint level, | ||
| [MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))]string name); | ||
|
|
||
| // Not yet implemented libgit2_opts signatures: |
There was a problem hiding this comment.
We could definitely change the functionality in libgit2 and this comment would get out of date. This might be frustrating to a future implementer, so I would recommend dropping these comments. Maybe we should have something alluding to the fact that there are other possible signatures that the above do not cover.
There was a problem hiding this comment.
Good point, I guess someone wanting to implement another one of these would have checked the libgit2 docs anyways
|
I have a branch here where I use this for the libgit2# tests. With that in place, I should be able to strip out all uses of As a bonus, it prevents tests from accidentally writing to a user's config or taking a dependency on the test environment (e.g. failing if you don't have the right |
|
@rcorre 🆒 We would also have to cleanup all the tests that actually rely on those RepositoryOptions properties as well. However, before diving into this, I'd like to make sure that it's ok to set this globally and that there's no need for a temporary override of those ones. |
Given the desire to be able to, eventually, run unit-tests in parallel this would be a bummer. It would be nice if the |
I've pondered this as well, but I think we're already doomed (filters, subtransports, ...). Besides, if the only use of those |
touché |
|
@nulltoken I'm interested in this proposal. |
| // set a value in the first config | ||
| using (var config = Configuration.BuildFrom(null)) | ||
| { | ||
| config.Set("luggage.code", 9876, ConfigurationLevel.Global); |
There was a problem hiding this comment.
ZOMG! You're disclosing secrets, here!!1! 😜
There was a problem hiding this comment.
Isn't that supposed to be "1-2-3-4-5?"?
I mean "That's the stupidest combination I've ever heard of in my life! That's the kinda thing an idiot would have on his luggage!"
😛
There was a problem hiding this comment.
Shout out to obscure Mel Brooks references...
There was a problem hiding this comment.
Literally that first thing that came to mind when I read that line.
|
Removed |
|
What's the expected/actual behavior for |
|
It should restore the default search path. Having trouble getting libgit2# to build right now, but I'll verify that when I can. |
|
Confirmed that |
Add tests to prove the behavior?
I'd be fine with an overloaded |
Maybe in the plural form? |
|
|
|
|
|
|
|
Overloading for I noticed that libgit2 documents that the special string "$PATH" refers to the current path, so I documented and tested that too (as well as the empty string behavior). As tempting as it was to shave another character off of the function name, I decided to keep the |
👍
👍
👍
👎 |
I kicked it, but Travis is again having Mac Issues. |
|
Thanks! Incidentally, should LibGit2Sharp build with |
|
|
This should make it build? #1034 |
|
Apologies if I mess up something here, I've not kept up with this PR as it's evolved over time...
I guess I'm not clear on the scenario where a "per repository" override would be needed. I'm trying to think of a scenario in the past where I've come across this, but coming up empty. 👍 with the direction this is heading in - will definitely help with testing on my end... |
|
Another thing I've been looking into is libgit2's Setting it up might be a bit more involved than using |
I think this is the interesting use case - a setup where multiple components depend on the same LG2S, and the individual components do not want to affect each other. |
Nobody should be using the Visual Studio bundled in Visual Studio except people internal to Visual Studio. (I'm including the GitHub VS extension in this, since they're shipping in our box but note that as soon as 2015 was RTM'd, they are now using their own LibGit2Sharp.) I realize that you may be using this as an example of a plug-in model type system. I don't think that LibGit2Sharp can predict this sort of behavior and that if Visual Studio (to continue this example) were to offer consumers the ability to work with its LibGit2Sharp instance then there needs to be a contract between consumers and the provider about what can be called. I don't think LibGit2Sharp can really be good at predicting what this/these things should look like. |
|
Anyway. I'm 👍 on this change and on pluggable configuration backends. That's something various people have wanted for a while but it has not bubbled up very high on my priority list. |
| Assert.Equal(xdgPath, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Xdg).Single()); | ||
|
|
||
| // reset the search paths to their defaults | ||
| GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null); |
There was a problem hiding this comment.
Do we want to try and always run the code that reset's the search paths to their defaults (via try / finally), to try and prevent possibly polluting other tests? Although, if there is a bug in logic for setting the global search paths, then maybe the rest logic would also not work anways...
There was a problem hiding this comment.
Yeah, probably a good idea. As you point out, this test is a bit weird -- if the logic is flawed, the whole setup/teardown process is bugged.
There was a problem hiding this comment.
I'm generally opposed to asserts in setup/teardown, but this might be a case where we want to capture global config state at the fixture level, and then restore (and assert restoration) on teardown. If any of that fails, we're probably not going to trust these config tests anyway.
There was a problem hiding this comment.
Right now its only being used in the tests I added, the other ConfigurationFixture tests, still rely on the old redirection method (RepositoryOptions set up through a BuildFakeConfigs test method).
I could use this redirection for all the config tests (and potentially all tests in the library), but I think that's a whole different PR :)
|
Thanks @rcorre - I think this looks good! |
|
This fell pretty far behind, so I've rebased back up to vNext (and added a comment about |
|
Anything preventing this from going through at this point? As far as whether to deprecate
|
|
@rcorre Let's do this. Could you please squash this PR into a single commit so we can merge it? |
Allow getting/setting the global/system/XDG config search path using Configuration.GetSearchPath Configuration.SetSearchPath. This is useful for libgit2sharp consumers who are writing unit tests that want to 'fake' a global config. While the config search path can be changed using the RepositoryOptions, this requires that every function you test would need to accept a config path argument. Changing the search path at the library level ensures that all config access is redirected. This is how the libgit2 config tests work; see the initialization of tests/config/global.c for an example.
|
@nulltoken: squashed and ready to go |
|
I am sad Cofigs are no longer an option. :( |
@rcorre ❤️ Let's merge it. Awesome job!
@Therzok I'm going to open an issue discussing the potential deprecation of those |
Allow libgit2# to redirect the global/system/xdg config search path
|
@nulltoken you do realise I actually meant to type |
|
of course 😉
|
It is one letter shorter... |
Dammit! You're right. There are indeed a finite number of keystrokes left in developers'hands before they die.
Help us @shanselman you're our only hope! |
This provides an interface for libgit2# to redirect config file lookups via libgit2's
git_libgit2_opt.This is useful for unit-testing methods that hit the global, system, or xdg config files.
It is currently possible to redirect config access using
RepositoryOptions, but this only works if the functions you are testing take aRepositoryas an argument. This isn't much of a problem for libgit2#, but other frameworks may not want to pass aRepositoryall the way through their call stack to support testing with a faked config. This provides a more foolproof way to sandbox your tests as it guarantees that all config access is redirected (libgit2 uses this for testing).The first commit adds the actual functionality.
git_libgit2_optshas a number of other options -- I only added bindings for search path redirection, but this sets the stage for binding other libgit2 settings if they are needed.The second commit shows how this could be used within libgit2# tests. The current redirection scheme can be flaky -- I noticed some tests failing because they only redirected system config access, and my personal global config was interfering.
The test could be fixed without this, of course, but I think this will help prevent mistakes in the long run.
[EDIT]
I've removed the second commit for now -- my partial fixup of the test fixtures was causing problems in as many tests as it fixed.
This PR just allows libgit2# consumers to redirect config access, I can work on a second PR to use this in the lg2# test fixtures if it seems like a good idea (started here).