Respect core.logAllRefUpdates #499
Conversation
I don't think so.
Why not. Although I'm not sure this makes the code that easier to read. For instance, |
Well that's not a ringing endorsement... A few alternatives:
If we stick with 1, 2 or 3, I wonder if we should accept a default value lambda instead (in addition?) since evaluating the default might be expensive. |
I like this one.
👍
I think that would help. Thanks for bearing with me ;-) |
|
@dahlbyk Shouldn't it rather be It looks like this is set at repo (re)initialization time (see in git.git), not dynamically evaluated. Or am I missing something obvious? |
I suppose I should have checked on that... yeah, since we can expect repos to have this set, then defaulting to |
Isn't it the opposite? The config entry |
|
Er yeah, the opposite. What I meant to say is, libgit2 does the right thing. |
|
Updated per feedback. Need to write tests for |
|
@dahlbyk It looks like Travis choked on the following |
Achievement unlocked! Guessing it's an issue with generic type inference..pushing a workaround and will see if I can come up with a minimal repro. |
Beside covering with tests the reflog changes when creating and moving branches... Perfect! |
Boom goes the msc stack trace: |
Spot on! See this SO question : How to detect what commit a branch has been created from in LibGit2Sharp |
|
That should do it... Any other feedback before commit cleanup? |
|
On second thought, this is probably easier to review after cleanup. What say you? |
|
@dahlbyk That's a very nice job! Just one nitpick. I'm not a huge fan of allowing a null Assert.Equal(false, repo.Config.GetValueOrDefault<bool>("missing.key", null));How about the following? ---
LibGit2Sharp.Tests/ConfigurationFixture.cs | 11 ++++-------
LibGit2Sharp/ConfigurationExtensions.cs | 13 ++++++++++---
2 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/LibGit2Sharp.Tests/ConfigurationFixture.cs b/LibGit2Sharp.Tests/ConfigurationFixture.cs
index 8dfb2f7..ad8ca8b 100644
--- a/LibGit2Sharp.Tests/ConfigurationFixture.cs
+++ b/LibGit2Sharp.Tests/ConfigurationFixture.cs
@@ -113,7 +113,6 @@ public void CanReadBooleanValue()
Assert.True(repo.Config.GetValueOrDefault<bool>("core.ignorecase"));
Assert.Equal(false, repo.Config.GetValueOrDefault<bool>("missing.key"));
- Assert.Equal(false, repo.Config.GetValueOrDefault<bool>("missing.key", null));
Assert.Equal(true, repo.Config.GetValueOrDefault<bool>("missing.key", true));
Assert.Equal(true, repo.Config.GetValueOrDefault<bool>("missing.key", () => true));
}
@@ -129,7 +128,6 @@ public void CanReadIntValue()
Assert.Equal(2, repo.Config.GetValueOrDefault<int>("unittests.intsetting", ConfigurationLevel.Local));
Assert.Equal(0, repo.Config.GetValueOrDefault<int>("missing.key"));
- Assert.Equal(0, repo.Config.GetValueOrDefault<int>("missing.key", null));
Assert.Equal(4, repo.Config.GetValueOrDefault<int>("missing.key", 4));
Assert.Equal(4, repo.Config.GetValueOrDefault<int>("missing.key", () => 4));
}
@@ -144,7 +142,6 @@ public void CanReadLongValue()
Assert.Equal(15234, repo.Config.GetValueOrDefault<long>("unittests.longsetting"));
Assert.Equal(0, repo.Config.GetValueOrDefault<long>("missing.key"));
- Assert.Equal(0, repo.Config.GetValueOrDefault<long>("missing.key", null));
Assert.Equal(4, repo.Config.GetValueOrDefault<long>("missing.key", 4));
Assert.Equal(4, repo.Config.GetValueOrDefault<long>("missing.key", () => 4));
}
@@ -164,22 +161,22 @@ public void CanReadStringValue()
Assert.Equal("+refs/heads/*:refs/remotes/origin/*", repo.Config.GetValueOrDefault<string>(new[] { "remote", "origin", "fetch" }));
Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing.key"));
- Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing.key", default(Func<string>)));
+ Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing.key", default(string)));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>("missing.key", "value"));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>("missing.key", () => "value"));
Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing.key", ConfigurationLevel.Local));
- Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing.key", ConfigurationLevel.Local, default(Func<string>)));
+ Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing.key", ConfigurationLevel.Local, default(string)));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>("missing.key", ConfigurationLevel.Local, "value"));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>("missing.key", ConfigurationLevel.Local, () => "value"));
Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing", "config", "key"));
- Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing", "config", "key", default(Func<string>)));
+ Assert.Equal(null, repo.Config.GetValueOrDefault<string>("missing", "config", "key", default(string)));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>("missing", "config", "key", "value"));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>("missing", "config", "key", () => "value"));
Assert.Equal(null, repo.Config.GetValueOrDefault<string>(new[] { "missing", "key" }));
- Assert.Equal(null, repo.Config.GetValueOrDefault<string>(new[] { "missing", "key" }, default(Func<string>)));
+ Assert.Equal(null, repo.Config.GetValueOrDefault<string>(new[] { "missing", "key" }, default(string)));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>(new[] { "missing", "key" }, "value"));
Assert.Equal("value", repo.Config.GetValueOrDefault<string>(new[] { "missing", "key" }, () => "value"));
}
diff --git a/LibGit2Sharp/ConfigurationExtensions.cs b/LibGit2Sharp/ConfigurationExtensions.cs
index efbe948..14fdff3 100644
--- a/LibGit2Sharp/ConfigurationExtensions.cs
+++ b/LibGit2Sharp/ConfigurationExtensions.cs
@@ -1,4 +1,5 @@
using System;
+using System.Diagnostics;
using LibGit2Sharp.Core;
namespace LibGit2Sharp
@@ -139,6 +140,8 @@ public static T GetValueOrDefault<T>(this Configuration config, string firstKeyP
/// <returns>The configuration value, or a generated default.</returns>
public static T GetValueOrDefault<T>(this Configuration config, string key, Func<T> defaultValueSelector)
{
+ Ensure.ArgumentNotNull(defaultValueSelector, "defaultValueSelector");
+
return ValueOrDefault(config.Get<T>(key), defaultValueSelector);
}
@@ -155,6 +158,8 @@ public static T GetValueOrDefault<T>(this Configuration config, string key, Func
/// <returns>The configuration value, or a generated default.</returns>
public static T GetValueOrDefault<T>(this Configuration config, string key, ConfigurationLevel level, Func<T> defaultValueSelector)
{
+ Ensure.ArgumentNotNull(defaultValueSelector, "defaultValueSelector");
+
return ValueOrDefault(config.Get<T>(key, level), defaultValueSelector);
}
@@ -170,6 +175,8 @@ public static T GetValueOrDefault<T>(this Configuration config, string key, Conf
/// <returns>The configuration value, or a generated default.</returns>
public static T GetValueOrDefault<T>(this Configuration config, string[] keyParts, Func<T> defaultValueSelector)
{
+ Ensure.ArgumentNotNull(defaultValueSelector, "defaultValueSelector");
+
return ValueOrDefault(config.Get<T>(keyParts), defaultValueSelector);
}
@@ -187,6 +194,8 @@ public static T GetValueOrDefault<T>(this Configuration config, string[] keyPart
/// <returns>The configuration value, or a generated default.</returns>
public static T GetValueOrDefault<T>(this Configuration config, string firstKeyPart, string secondKeyPart, string thirdKeyPart, Func<T> defaultValueSelector)
{
+ Ensure.ArgumentNotNull(defaultValueSelector, "defaultValueSelector");
+
return ValueOrDefault(config.Get<T>(firstKeyPart, secondKeyPart, thirdKeyPart), defaultValueSelector);
}
@@ -198,9 +207,7 @@ private static T ValueOrDefault<T>(ConfigurationEntry<T> value, T defaultValue)
private static T ValueOrDefault<T>(ConfigurationEntry<T> value, Func<T> defaultValueSelector)
{
return value == null
- ? defaultValueSelector == null
- ? default(T)
- : defaultValueSelector()
+ ? defaultValueSelector()
: value.Value;
}
}
-- |
|
How does @fefa364 look? |
|
@dahlbyk Awesome! Squashing time! 🔨 🕙 |
|
Squashed |
|
Merged 💖 |
Stumbled on this setting tonight...
This isn't ready to merge yet, but I wanted to get it out for comment...
Using
ObjectDatabaseto create a commit to target with a branch was the first thing I thought of to create a reflog entry in a bare repo...did I overlook something obviously simpler?CreateBranch()does not write to reflog (or I can open a separate issue... easy enough to fix here and I'd like my test to use it instead of the more obtuseRefs.Add(...)).repo.Config.GetValueOrDefault(key, defaultValue)? Seems a common enough pattern (even in our tests) to make simple, but didn't want to waste time on overloads/docs if you disagree.