From 93772535c57ef33ae863ba5a34008c9e67a1c6cb Mon Sep 17 00:00:00 2001 From: Keith Dahlby Date: Tue, 2 Sep 2014 01:37:48 -0500 Subject: [PATCH 1/6] Test for invalid signature config --- LibGit2Sharp.Tests/CommitFixture.cs | 22 +++++++++++++++++++ LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs | 16 ++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/LibGit2Sharp.Tests/CommitFixture.cs b/LibGit2Sharp.Tests/CommitFixture.cs index 6ac64c31f..a4d7eca44 100644 --- a/LibGit2Sharp.Tests/CommitFixture.cs +++ b/LibGit2Sharp.Tests/CommitFixture.cs @@ -5,6 +5,7 @@ using LibGit2Sharp.Core; using LibGit2Sharp.Tests.TestHelpers; using Xunit; +using Xunit.Extensions; namespace LibGit2Sharp.Tests { @@ -519,6 +520,27 @@ public void DirectlyAccessingAnUnknownTreeEntryOfTheCommitReturnsNull() } } + [Theory] + [InlineData(null, "x@example.com")] + [InlineData("", "x@example.com")] + [InlineData("X", null)] + [InlineData("X", "")] + public void CommitWithInvalidSignatureConfigThrows(string name, string email) + { + string repoPath = InitNewRepository(); + string configPath = CreateConfigurationWithDummyUser(name, email); + var options = new RepositoryOptions { GlobalConfigurationLocation = configPath }; + + using (var repo = new Repository(repoPath, options)) + { + Assert.Equal(name, repo.Config.GetValueOrDefault("user.name")); + Assert.Equal(email, repo.Config.GetValueOrDefault("user.email")); + + Assert.Throws( + () => repo.Commit("Initial egotistic commit", new CommitOptions { AllowEmptyCommit = true })); + } + } + [Fact] public void CanCommitWithSignatureFromConfig() { diff --git a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs index 2cab50d5a..dd2ab2c37 100644 --- a/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs +++ b/LibGit2Sharp.Tests/TestHelpers/BaseFixture.cs @@ -298,6 +298,11 @@ private static RepositoryOptions BuildFakeRepositoryOptions(SelfCleaningDirector /// The signature to use for user.name and user.email /// The path to the configuration file protected string CreateConfigurationWithDummyUser(Signature signature) + { + return CreateConfigurationWithDummyUser(signature.Name, signature.Email); + } + + protected string CreateConfigurationWithDummyUser(string name, string email) { SelfCleaningDirectory scd = BuildSelfCleaningDirectory(); Directory.CreateDirectory(scd.DirectoryPath); @@ -305,8 +310,15 @@ protected string CreateConfigurationWithDummyUser(Signature signature) using (Configuration config = new Configuration(configFilePath)) { - config.Set("user.name", signature.Name, ConfigurationLevel.Global); - config.Set("user.email", signature.Email, ConfigurationLevel.Global); + if (name != null) + { + config.Set("user.name", name, ConfigurationLevel.Global); + } + + if (email != null) + { + config.Set("user.email", email, ConfigurationLevel.Global); + } } return configFilePath; From 5913654ebcb664ffbcd577752783bc7baa707599 Mon Sep 17 00:00:00 2001 From: Keith Dahlby Date: Tue, 2 Sep 2014 01:38:04 -0500 Subject: [PATCH 2/6] Add preemptive config null check libgit2 will throw if you set a NULL value, so we might as well. --- LibGit2Sharp/Configuration.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs index 9e0a2633d..4df937b9d 100644 --- a/LibGit2Sharp/Configuration.cs +++ b/LibGit2Sharp/Configuration.cs @@ -227,6 +227,7 @@ public virtual ConfigurationEntry Get(string key, ConfigurationLevel level /// The configuration file which should be considered as the target of this operation public virtual void Set(string key, T value, ConfigurationLevel level = ConfigurationLevel.Local) { + Ensure.ArgumentNotNull(value, "value"); Ensure.ArgumentNotNullOrEmptyString(key, "key"); using (ConfigurationSafeHandle h = RetrieveConfigurationHandle(level, true, configHandle)) From 665f107343fa9528793beb3f0947275fca85f8d9 Mon Sep 17 00:00:00 2001 From: Ian Clanton-Thuon Date: Mon, 1 Sep 2014 16:17:35 -0700 Subject: [PATCH 3/6] Safer email and name validation in Configuration.BuildSignature --- LibGit2Sharp/Configuration.cs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs index 4df937b9d..93bdd84fa 100644 --- a/LibGit2Sharp/Configuration.cs +++ b/LibGit2Sharp/Configuration.cs @@ -342,30 +342,29 @@ public virtual Signature BuildSignature(DateTimeOffset now) internal Signature BuildSignature(DateTimeOffset now, bool shouldThrowIfNotFound) { - var name = Get("user.name"); - var email = Get("user.email"); + var name = this.GetValueOrDefault("user.name"); + var email = this.GetValueOrDefault("user.email"); - if (shouldThrowIfNotFound) + if (shouldThrowIfNotFound && (string.IsNullOrEmpty(name) || string.IsNullOrEmpty(email))) { - if (name == null || string.IsNullOrEmpty(name.Value)) + if (string.IsNullOrEmpty(name)) { throw new LibGit2SharpException( - "Can not find Name setting of the current user in Git configuration."); + "Cannot find Name setting of the current user in Git configuration."); } - if (email == null || string.IsNullOrEmpty(email.Value)) + if (string.IsNullOrEmpty(email)) { throw new LibGit2SharpException( - "Can not find Email setting of the current user in Git configuration."); + "Cannot find Email setting of the current user in Git configuration."); } } - var nameForSignature = name == null || string.IsNullOrEmpty(name.Value) ? "unknown" : name.Value; - var emailForSignature = email == null || string.IsNullOrEmpty(email.Value) - ? string.Format("{0}@{1}", Environment.UserName, Environment.UserDomainName) - : email.Value; - - return new Signature(nameForSignature, emailForSignature, now); + return new Signature( + !string.IsNullOrEmpty(name) ? name : "unknown", + !string.IsNullOrEmpty(email) ? email : string.Format( + CultureInfo.InvariantCulture, "{0}@{1}", Environment.UserName, Environment.UserDomainName), + now); } private ConfigurationSafeHandle Snapshot() From 381428ea2e6da7df4378f234323f372f555716a5 Mon Sep 17 00:00:00 2001 From: nulltoken Date: Mon, 3 Nov 2014 18:10:47 -0800 Subject: [PATCH 4/6] Prevent creation of a Signature with an empty email --- LibGit2Sharp.Tests/SignatureFixture.cs | 8 +------- LibGit2Sharp/Signature.cs | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/LibGit2Sharp.Tests/SignatureFixture.cs b/LibGit2Sharp.Tests/SignatureFixture.cs index 978141837..e40cabd6c 100644 --- a/LibGit2Sharp.Tests/SignatureFixture.cs +++ b/LibGit2Sharp.Tests/SignatureFixture.cs @@ -35,13 +35,7 @@ public void CreatingASignatureWithBadParamsThrows() Assert.Throws(() => new Signature(null, "me@there.com", DateTimeOffset.Now)); Assert.Throws(() => new Signature(string.Empty, "me@there.com", DateTimeOffset.Now)); Assert.Throws(() => new Signature("Me", null, DateTimeOffset.Now)); - } - - [Fact] - public void CanCreateASignatureWithAnEmptyEmail() - { - var sig = new Signature("Me", string.Empty, DateTimeOffset.Now); - Assert.Equal(string.Empty, sig.Email); + Assert.Throws(() => new Signature("Me", string.Empty, DateTimeOffset.Now)); } } } diff --git a/LibGit2Sharp/Signature.cs b/LibGit2Sharp/Signature.cs index 74be26448..7dbb437d7 100644 --- a/LibGit2Sharp/Signature.cs +++ b/LibGit2Sharp/Signature.cs @@ -36,7 +36,7 @@ internal Signature(IntPtr signaturePtr) public Signature(string name, string email, DateTimeOffset when) { Ensure.ArgumentNotNullOrEmptyString(name, "name"); - Ensure.ArgumentNotNull(email, "email"); + Ensure.ArgumentNotNullOrEmptyString(email, "email"); Ensure.ArgumentDoesNotContainZeroByte(name, "name"); Ensure.ArgumentDoesNotContainZeroByte(email, "email"); From 8a71afe31064f23dd9e50a3395b0a4a5c238a764 Mon Sep 17 00:00:00 2001 From: nulltoken Date: Mon, 3 Nov 2014 18:40:29 -0800 Subject: [PATCH 5/6] Refactor Signature creation from configuration --- LibGit2Sharp/Configuration.cs | 42 ++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs index 93bdd84fa..901797ec8 100644 --- a/LibGit2Sharp/Configuration.cs +++ b/LibGit2Sharp/Configuration.cs @@ -342,29 +342,35 @@ public virtual Signature BuildSignature(DateTimeOffset now) internal Signature BuildSignature(DateTimeOffset now, bool shouldThrowIfNotFound) { - var name = this.GetValueOrDefault("user.name"); - var email = this.GetValueOrDefault("user.email"); + const string userNameKey = "user.name"; + var name = this.GetValueOrDefault(userNameKey); + var normalizedName = NormalizeUserSetting(shouldThrowIfNotFound, userNameKey, name, + () => "unknown"); + + const string userEmailKey = "user.email"; + var email = this.GetValueOrDefault(userEmailKey); + var normalizedEmail = NormalizeUserSetting(shouldThrowIfNotFound, userEmailKey, email, + () => string.Format( + CultureInfo.InvariantCulture, "{0}@{1}", Environment.UserName, Environment.UserDomainName)); + + return new Signature(normalizedName, normalizedEmail, now); + } - if (shouldThrowIfNotFound && (string.IsNullOrEmpty(name) || string.IsNullOrEmpty(email))) + private string NormalizeUserSetting(bool shouldThrowIfNotFound, string entryName, string currentValue, Func defaultValue) + { + if (!string.IsNullOrEmpty(currentValue)) { - if (string.IsNullOrEmpty(name)) - { - throw new LibGit2SharpException( - "Cannot find Name setting of the current user in Git configuration."); - } + return currentValue; + } - if (string.IsNullOrEmpty(email)) - { - throw new LibGit2SharpException( - "Cannot find Email setting of the current user in Git configuration."); - } + string message = string.Format("Configuration value '{0}' is missing or invalid.", entryName); + + if (shouldThrowIfNotFound) + { + throw new LibGit2SharpException(message); } - return new Signature( - !string.IsNullOrEmpty(name) ? name : "unknown", - !string.IsNullOrEmpty(email) ? email : string.Format( - CultureInfo.InvariantCulture, "{0}@{1}", Environment.UserName, Environment.UserDomainName), - now); + return defaultValue(); } private ConfigurationSafeHandle Snapshot() From cf3b8062fc5bda68ccb1c72b75662ced021f6c6c Mon Sep 17 00:00:00 2001 From: nulltoken Date: Mon, 3 Nov 2014 21:00:26 -0800 Subject: [PATCH 6/6] Warn when signature cannot be properly inferred from configuration --- LibGit2Sharp/Configuration.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs index 901797ec8..89059b63e 100644 --- a/LibGit2Sharp/Configuration.cs +++ b/LibGit2Sharp/Configuration.cs @@ -370,6 +370,8 @@ private string NormalizeUserSetting(bool shouldThrowIfNotFound, string entryName throw new LibGit2SharpException(message); } + Log.Write(LogLevel.Warning, message); + return defaultValue(); }