Skip to content

Allow libgit2# to redirect the global/system/xdg config search path #1123

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

Merged
merged 1 commit into from
Sep 9, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
122 changes: 122 additions & 0 deletions LibGit2Sharp.Tests/ConfigurationFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -431,5 +431,127 @@ public void CannotBuildAProperSignatureFromConfigWhenFullIdentityCannotBeFoundIn
Assert.Null(signature);
}
}

[Fact]
public void CanSetAndGetSearchPath()
{
string globalPath = Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName());
string systemPath = Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName());
string xdgPath = Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName());

GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, globalPath);
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.System, systemPath);
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Xdg, xdgPath);

Assert.Equal(globalPath, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global).Single());
Assert.Equal(systemPath, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.System).Single());
Assert.Equal(xdgPath, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Xdg).Single());

// reset the search paths to their defaults
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null);
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.System, null);
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Xdg, null);
}

[Fact]
public void CanSetAndGetMultipleSearchPaths()
{
string[] paths =
{
Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName()),
Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName()),
Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName()),
};

GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, paths);

Assert.Equal(paths, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global));

// set back to the defaults
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null);
}

[Fact]
public void CanResetSearchPaths()
{
// all of these calls should reset the config path to the default
Action[] resetActions =
{
() => GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global),
() => GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null),
() => GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, string.Empty),
() => GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, new string[] { }),
};

// record the default search path
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null);
var oldPaths = GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global);
Assert.NotNull(oldPaths);

// generate a non-default path to set
var newPaths = new string[] { Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName()) };

foreach (var tryToReset in resetActions)
{
// change to the non-default path
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, newPaths);
Assert.Equal(newPaths, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global));

// set it back to the default
tryToReset();
Assert.Equal(oldPaths, GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global));
}

// make sure the config paths are reset after the test ends
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null);
}

[Fact]
public void CanAppendToSearchPaths()
{
string appendMe = Path.Combine(Constants.TemporaryReposPath, Path.GetRandomFileName());
var prevPaths = GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global);

// append using the special name $PATH
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, "$PATH", appendMe);

var currentPaths = GlobalSettings.GetConfigSearchPaths(ConfigurationLevel.Global);
Assert.Equal(currentPaths, prevPaths.Concat(new[] { appendMe }));

// set it back to the default
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null);
}

[Fact]
public void CanRedirectConfigAccess()
{
var scd1 = BuildSelfCleaningDirectory();
var scd2 = BuildSelfCleaningDirectory();

Touch(scd1.RootedDirectoryPath, ".gitconfig");
Touch(scd2.RootedDirectoryPath, ".gitconfig");

// redirect global access to the first path
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, scd1.RootedDirectoryPath);

// set a value in the first config
using (var config = Configuration.BuildFrom(null))
{
config.Set("luggage.code", 9876, ConfigurationLevel.Global);
Copy link
Member

Choose a reason for hiding this comment

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

ZOMG! You're disclosing secrets, here!!1! 😜

Copy link

Choose a reason for hiding this comment

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

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!"

😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Shout out to obscure Mel Brooks references...

Copy link
Member

Choose a reason for hiding this comment

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

Literally that first thing that came to mind when I read that line.

Assert.Equal(9876, config.Get<int>("luggage.code", ConfigurationLevel.Global).Value);
}

// redirect global config access to path2
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, scd2.RootedDirectoryPath);

// if the redirect succeeds, the value set in the prior config should not be visible
using (var config = Configuration.BuildFrom(null))
{
Assert.Equal(-1, config.GetValueOrDefault<int>("luggage.code", ConfigurationLevel.Global, -1));
}

// reset the search path to the default
GlobalSettings.SetConfigSearchPaths(ConfigurationLevel.Global, null);
}
}
}
19 changes: 19 additions & 0 deletions LibGit2Sharp/Core/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,25 @@ internal static extern int git_filter_unregister(
[DllImport(libgit2)]
internal static extern int git_libgit2_features();

#region git_libgit2_opts

// Bindings for git_libgit2_opts(int option, ...):
// Currently only GIT_OPT_GET_SEARCH_PATH and GIT_OPT_SET_SEARCH_PATH are supported,
// but other overloads could be added using a similar pattern.
// CallingConvention.Cdecl is used to allow binding the the C varargs signature, and each possible call signature must be enumerated.
// __argslist was an option, but is an undocumented feature that should likely not be used here.

// git_libgit2_opts(GIT_OPT_GET_SEARCH_PATH, int level, git_buf *buf)
[DllImport(libgit2, CallingConvention = CallingConvention.Cdecl)]
Copy link
Member

Choose a reason for hiding this comment

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

Do you think a note on why you are using cdecl calling convention here would be useful (or being more explicit in the above comment)? (because of the varargs in the native library?)

internal static extern int git_libgit2_opts(int option, uint level, GitBuf buf);

// git_libgit2_opts(GIT_OPT_SET_SEARCH_PATH, int level, const char *path)
[DllImport(libgit2, CallingConvention = CallingConvention.Cdecl)]
Copy link
Member

Choose a reason for hiding this comment

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

Why CallingConvention.Cdecl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as @jamill mentioned, this was (seemingly) necessary to bind to a varargs function. I'll include a note in the comments about that.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! This is (I think) the first time we've changed the decoration like this, so I think it would be worthwhile to document why.

internal static extern int git_libgit2_opts(int option, uint level,
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))]string path);

#endregion

[DllImport(libgit2)]
internal static extern int git_graph_ahead_behind(out UIntPtr ahead, out UIntPtr behind, RepositorySafeHandle repo, ref GitOid one, ref GitOid two);

Expand Down
54 changes: 54 additions & 0 deletions LibGit2Sharp/Core/Proxy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3193,6 +3193,60 @@ public static BuiltInFeatures git_libgit2_features()
return (BuiltInFeatures)NativeMethods.git_libgit2_features();
}

// C# equivalent of libgit2's git_libgit2_opt_t
private enum LibGitOption
{
GetMWindowSize, // GIT_OPT_GET_MWINDOW_SIZE
Copy link
Member

Choose a reason for hiding this comment

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

Did you want to explicitly set the values on the enum to match what is in lg2, just to remove any possibly ambiguity? I think this is how most of our other enums are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lg2 doesn't explicitly set any values, it just uses the default enum numbering, so I figured it was best to just match lg2 (I'm guessing its highly unlikely that either language is going to change the default numbering anytime soon).

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

SetMWindowSize, // GIT_OPT_SET_MWINDOW_SIZE
GetMWindowMappedLimit, // GIT_OPT_GET_MWINDOW_MAPPED_LIMIT
SetMWindowMappedLimit, // GIT_OPT_SET_MWINDOW_MAPPED_LIMIT
GetSearchPath, // GIT_OPT_GET_SEARCH_PATH
SetSearchPath, // GIT_OPT_SET_SEARCH_PATH
SetCacheObjectLimit, // GIT_OPT_SET_CACHE_OBJECT_LIMIT
SetCacheMaxSize, // GIT_OPT_SET_CACHE_MAX_SIZE
EnableCaching, // GIT_OPT_ENABLE_CACHING
GetCachedMemory, // GIT_OPT_GET_CACHED_MEMORY
GetTemplatePath, // GIT_OPT_GET_TEMPLATE_PATH
SetTemplatePath, // GIT_OPT_SET_TEMPLATE_PATH
SetSslCertLocations, // GIT_OPT_SET_SSL_CERT_LOCATIONS
}

/// <summary>
/// Get the paths under which libgit2 searches for the configuration file of a given level.
/// </summary>
/// <param name="level">The level (global/system/XDG) of the config.</param>
/// <returns>
/// The paths delimited by 'GIT_PATH_LIST_SEPARATOR' (<see cref="GlobalSettings.PathListSeparator"/>).
/// </returns>
public static string git_libgit2_opts_get_search_path(ConfigurationLevel level)
{
string path;

using (var buf = new GitBuf())
{
var res = NativeMethods.git_libgit2_opts((int)LibGitOption.GetSearchPath, (uint)level, buf);
Ensure.ZeroResult(res);

path = LaxUtf8Marshaler.FromNative(buf.ptr) ?? string.Empty;
}

return path;
}

/// <summary>
/// Set the path(s) under which libgit2 searches for the configuration file of a given level.
/// </summary>
/// <param name="level">The level (global/system/XDG) of the config.</param>
/// <param name="path">
/// A string of paths delimited by 'GIT_PATH_LIST_SEPARATOR' (<see cref="GlobalSettings.PathListSeparator"/>).
/// Pass null to reset the search path to the default.
/// </param>
public static void git_libgit2_opts_set_search_path(ConfigurationLevel level, string path)
{
var res = NativeMethods.git_libgit2_opts((int)LibGitOption.SetSearchPath, (uint)level, path);
Ensure.ZeroResult(res);
}

#endregion

private static ICollection<TResult> git_foreach<T, TResult>(
Expand Down
28 changes: 28 additions & 0 deletions LibGit2Sharp/GlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Collections.Generic;
using System.IO;
using System.Reflection;
using System.Collections.Generic;
using LibGit2Sharp.Core;

namespace LibGit2Sharp
Expand Down Expand Up @@ -272,5 +273,32 @@ internal static void DeregisterFilter(Filter filter)
DeregisterFilter(registration);
}
}

/// <summary>
/// Get the paths under which libgit2 searches for the configuration file of a given level.
/// </summary>
/// <param name="level">The level (global/system/XDG) of the config.</param>
/// <returns>The paths that are searched</returns>
public static IEnumerable<string> GetConfigSearchPaths(ConfigurationLevel level)
Copy link
Member

Choose a reason for hiding this comment

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

Do you think returning the array here might be more useful for consumers? We are returning a copy of the data, so I don't think we need the "read only" property of the IEnumerable... just a thought...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, forgot that Split returns an [] already, so I might as well be explicit about it.Good catch!

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we return that many arrays API wide. Would there be any trouble sticking with an IEnumerable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On one hand, array is more flexible: I can assign a string[] to an IEnumerable<string> but not visa-versa. This makes it clear on what you are getting.
On the other, IEnumerable<string> makes it easier to avoid breaking API changes by providing a more loose contract.
In that light, I guess I'd prefer sticking with IEnumerable.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Member

Choose a reason for hiding this comment

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

👍 for immutable IEnumerable

Copy link
Member

Choose a reason for hiding this comment

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

IEnumerable, you have my sword.

{
return Proxy.git_libgit2_opts_get_search_path(level).Split(Path.PathSeparator);
}

/// <summary>
/// Set the paths under which libgit2 searches for the configuration file of a given level.
///
/// <seealso cref="RepositoryOptions"/>.
/// </summary>
/// <param name="level">The level (global/system/XDG) of the config.</param>
/// <param name="paths">
/// The new search paths to set.
/// Pass null to reset to the default.
/// The special string "$PATH" will be substituted with the current search path.
/// </param>
public static void SetConfigSearchPaths(ConfigurationLevel level, params string[] paths)
{
var pathString = (paths == null) ? null : string.Join(Path.PathSeparator.ToString(), paths);
Proxy.git_libgit2_opts_set_search_path(level, pathString);
}
}
}
3 changes: 3 additions & 0 deletions LibGit2Sharp/RepositoryOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public sealed class RepositoryOptions
/// The path has either to lead to an existing valid configuration file,
/// or to a non existent configuration file which will be eventually created.
/// </para>
/// <seealso cref="GlobalSettings.SetConfigSearchPaths"/>.
/// </summary>
public string GlobalConfigurationLocation { get; set; }

Expand All @@ -41,6 +42,7 @@ public sealed class RepositoryOptions
/// The path has either to lead to an existing valid configuration file,
/// or to a non existent configuration file which will be eventually created.
/// </para>
/// <seealso cref="GlobalSettings.SetConfigSearchPaths"/>.
/// </summary>
public string XdgConfigurationLocation { get; set; }

Expand All @@ -50,6 +52,7 @@ public sealed class RepositoryOptions
/// The path has to lead to an existing valid configuration file,
/// or to a non existent configuration file which will be eventually created.
/// </para>
/// <seealso cref="GlobalSettings.SetConfigSearchPaths"/>.
/// </summary>
public string SystemConfigurationLocation { get; set; }

Expand Down