String format ctor for exceptions #1202

odedw · 2015-10-17T17:17:45Z

Added a c'tor to all exceptions that accepts (IFormatProvider provider, string format, params object[] args) and calls the corresponding LibGit2SharpException c'tor
changed the LibGit2SharpException c'tor (and all the rest) to accept IFormatProvider instead of CultureInfo.
Changed all usages to use the new c'tors (where applicable)

nulltoken · 2015-10-18T16:14:44Z

Thanks!

Looks nicely done to me.

@whoisj Any comment?

whoisj · 2015-10-19T13:33:07Z

👍 @odedw thanks 😄

ethomson · 2015-10-19T14:01:44Z

From a brief glance, it looks like we construct all exceptions with CultureInfo.InvariantCulture - which makes me wonder why we don't have the base class use that as the default.

odedw · 2015-10-19T16:14:11Z

@ethomson I thought about that but wanted to keep options open (same reason I switched to IFormatProvider instead of CultureInfo).
I'd be happy to change it if you think it'll be more convenient.

whoisj · 2015-10-19T17:05:32Z

From a brief glance, it looks like we construct all exceptions with CultureInfo.InvariantCulture - which makes me wonder why we don't have the base class use that as the default.

👍 agreed. I think that would be a valuable change.

Something like:
ctor(IFormatProvider provider, format, args) : base(IFormatProvider provider, format, args)
ctor(string format, param object[] args) : this(CultureInfo.InvariantCulture, format, args)
could help reduce redundant code.

odedw · 2015-10-20T05:39:02Z

@whoisj @ethomson Sounds good! Removed the IFormatProvider parameter and used InvariantCulture in LibGit2SharpException's ctor.

nulltoken · 2015-10-20T07:34:44Z

Hmmm. Weird unrelated error in the build (Cannot cast from source type to destination type).

First time I see it. That may be caused by this line (or one a few lines above) whenever result can't find a suitable enum entry.

Strangely, only entry we're missing is GIT_EDIRECTORY.

@carlosmn @ethomson @jamill Any idea?

Errors:
/home/travis/build/libgit2/libgit2sharp/CI/build.msbuild (Deploy) ->
(Test target) ->
    : error : LibGit2Sharp.Tests.SmartSubtransportFixture.CustomSmartSubtransportTest(scheme: "http", url: "http://github.com/libgit2/TestGitRepository"):
LibGit2Sharp.LibGit2SharpException : Cannot cast from source type to destination type.
    : error :   at LibGit2Sharp.Core.Ensure.HandleError (Int32 result) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Core.Ensure.ZeroResult (Int32 result) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Core.Proxy.git_remote_fetch (LibGit2Sharp.Core.Handles.RemoteSafeHandle remote, IEnumerable`1 refSpecs, LibGit2Sharp.Core.GitFetchOptions fetchOptions, System.String logMessage) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Network.DoFetch (LibGit2Sharp.FetchOptions options, LibGit2Sharp.Core.Handles.RemoteSafeHandle remoteHandle, System.String logMessage, IEnumerable`1 refspecs) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Network.DoFetch (LibGit2Sharp.Core.Handles.RepositorySafeHandle repoHandle, LibGit2Sharp.Remote remote, LibGit2Sharp.FetchOptions options, System.String logMessage, IEnumerable`1 refspecs) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Network.Fetch (LibGit2Sharp.Remote remote, LibGit2Sharp.FetchOptions options, System.String logMessage) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Network.Fetch (LibGit2Sharp.Remote remote, LibGit2Sharp.FetchOptions options) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Tests.SmartSubtransportFixture.CustomSmartSubtransportTest (System.String scheme, System.String url) [0x00000] in <filename unknown>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0

nulltoken · 2015-10-20T07:35:13Z

Restarted the build as this seems unrelated to this PR.

nulltoken · 2015-10-20T07:52:25Z

@odedw Neat job!

How would you feel about rewriting the history and only come up with two commits:

One that would introduce the breaking change to LibGit2sharpException and updates its dependencies (callers and derived types)
Another one that would add the missing constructors to the the remaining exceptions

odedw · 2015-10-20T07:57:05Z

@nulltoken Thanks!

Working on it.

nulltoken · 2015-10-20T08:14:03Z

@odedw Easy way:

Create a branch backup pointing at the latest commit.
Checkout your string_format_ctor_for_exceptions branch
Reset its content in mixed mode to the parent of your first commit. This will reset your index without touching your workdir

From here on, stage the required files for the first commit, commit. Stage the remaining files and perform the second commit. Force push your changes to GitHub, this PR will be updated accordingly.

Would anything go sideways, your local backup branch will be there holding on to your previous version.

odedw · 2015-10-20T08:33:27Z

@nulltoken Got it. could you describe a bit more what you think each commit should include?

nulltoken · 2015-10-20T10:04:52Z

@nulltoken Got it. could you describe a bit more what you think each commit should include?

Sorry, let me try to reword my train of thoughts.

Changes that do no impact any existing code (eg. UnmergedIndexEntriesException) should go in the second commit.

Everything else should go in the first commit.

Did that help?

odedw · 2015-10-20T10:23:01Z

@nulltoken Yes, LibGit2sharpException related changes in the first one, other exception related changes in the second one. Working on it.

odedw · 2015-10-20T10:54:46Z

@nulltoken Done, let me know if that's what you had in mind.

nulltoken · 2015-10-20T12:11:08Z

@odedw I've pulled this locally. It looks like your first commit doesn't seem to build by itself.

Applying the following changes to it should fix this. This should take care of fixing the "updates its dependencies (callers and derived types)" I was referring to earlier

 LibGit2Sharp/AmbiguousSpecificationException.cs |  5 ++---
 LibGit2Sharp/Core/Proxy.cs                      |  6 ++----
 LibGit2Sharp/RemoveFromIndexException.cs        |  5 ++---
 LibGit2Sharp/Repository.cs                      | 12 ++++--------
 LibGit2Sharp/UnbornBranchException.cs           |  2 +-
 5 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/LibGit2Sharp/AmbiguousSpecificationException.cs b/LibGit2Sharp/AmbiguousSpecificationException.cs
index 577b84f..bad7af1 100644
--- a/LibGit2Sharp/AmbiguousSpecificationException.cs
+++ b/LibGit2Sharp/AmbiguousSpecificationException.cs
@@ -27,11 +27,10 @@ public AmbiguousSpecificationException(string message)
         /// <summary>
         /// Initializes a new instance of the <see cref="AmbiguousSpecificationException"/> class with a specified error message.
         /// </summary>
-        /// <param name="cultureInfo">An object that supplies culture-specific formatting information.</param>
         /// <param name="format">A composite format string for use in <see cref="String.Format(IFormatProvider, string, object[])"/>.</param>
         /// <param name="args">An object array that contains zero or more objects to format.</param>
-        public AmbiguousSpecificationException(CultureInfo cultureInfo, string format, params object[] args)
-            : base(String.Format(cultureInfo, format, args))
+        public AmbiguousSpecificationException(string format, params object[] args)
+            : base(String.Format(format, args))
         {
         }

diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs
index 71c75db..da2bd00 100644
--- a/LibGit2Sharp/Core/Proxy.cs
+++ b/LibGit2Sharp/Core/Proxy.cs
@@ -2555,8 +2555,7 @@ public static void git_repository_set_head(RepositorySafeHandle repo, string ref
                     return null;

                 case (int)GitErrorCode.Ambiguous:
-                    throw new AmbiguousSpecificationException(CultureInfo.InvariantCulture,
-                                                              "Provided abbreviated ObjectId '{0}' is too short.",
+                    throw new AmbiguousSpecificationException("Provided abbreviated ObjectId '{0}' is too short.",
                                                               objectish);

                 default:
@@ -2777,8 +2776,7 @@ public static FileStatus git_status_file(RepositorySafeHandle repo, FilePath pat
                     return FileStatus.Nonexistent;

                 case (int)GitErrorCode.Ambiguous:
-                    throw new AmbiguousSpecificationException(CultureInfo.InvariantCulture,
-                                                              "More than one file matches the pathspec '{0}'. " +
+                    throw new AmbiguousSpecificationException("More than one file matches the pathspec '{0}'. " +
                                                               "You can either force a literal path evaluation " +
                                                               "(GIT_STATUS_OPT_DISABLE_PATHSPEC_MATCH), or use git_status_foreach().",
                                                               path);
diff --git a/LibGit2Sharp/RemoveFromIndexException.cs b/LibGit2Sharp/RemoveFromIndexException.cs
index a1cea4b..c31b86c 100644
--- a/LibGit2Sharp/RemoveFromIndexException.cs
+++ b/LibGit2Sharp/RemoveFromIndexException.cs
@@ -27,11 +27,10 @@ public RemoveFromIndexException(string message)
         /// <summary>
         /// Initializes a new instance of the <see cref="LibGit2SharpException"/> class with a specified error message and a reference to the inner exception that is the cause of this exception.
         /// </summary>
-        /// <param name="cultureInfo">An object that supplies culture-specific formatting information.</param>
         /// <param name="format">A composite format string for use in <see cref="String.Format(IFormatProvider, string, object[])"/>.</param>
         /// <param name="args">An object array that contains zero or more objects to format.</param>
-        public RemoveFromIndexException(CultureInfo cultureInfo, string format, params object[] args)
-            : base(cultureInfo, format, args)
+        public RemoveFromIndexException(string format, params object[] args)
+            : base(format, args)
         {
         }

diff --git a/LibGit2Sharp/Repository.cs b/LibGit2Sharp/Repository.cs
index 3759743..1b5cf22 100644
--- a/LibGit2Sharp/Repository.cs
+++ b/LibGit2Sharp/Repository.cs
@@ -2104,8 +2104,7 @@ private IEnumerable<string> RemoveStagedItems(IEnumerable<string> paths, bool re
                             status.HasFlag(FileStatus.ModifiedInIndex) ||
                             status.HasFlag(FileStatus.NewInIndex)))
                         {
-                            throw new RemoveFromIndexException(CultureInfo.InvariantCulture,
-                                                               "Unable to remove file '{0}', as it has changes staged in the index. You can call the Remove() method with removeFromWorkingDirectory=false if you want to remove it from the index only.",
+                            throw new RemoveFromIndexException("Unable to remove file '{0}', as it has changes staged in the index. You can call the Remove() method with removeFromWorkingDirectory=false if you want to remove it from the index only.",
                                                                treeEntryChanges.Path);
                         }
                         removed.Add(RemoveFromIndex(treeEntryChanges.Path));
@@ -2114,22 +2113,19 @@ private IEnumerable<string> RemoveStagedItems(IEnumerable<string> paths, bool re
                     case ChangeKind.Modified:
                         if (status.HasFlag(FileStatus.ModifiedInWorkdir) && status.HasFlag(FileStatus.ModifiedInIndex))
                         {
-                            throw new RemoveFromIndexException(CultureInfo.InvariantCulture,
-                                                               "Unable to remove file '{0}', as it has staged content different from both the working directory and the HEAD.",
+                            throw new RemoveFromIndexException("Unable to remove file '{0}', as it has staged content different from both the working directory and the HEAD.",
                                                                treeEntryChanges.Path);
                         }
                         if (removeFromWorkingDirectory)
                         {
-                            throw new RemoveFromIndexException(CultureInfo.InvariantCulture,
-                                                               "Unable to remove file '{0}', as it has local modifications. You can call the Remove() method with removeFromWorkingDirectory=false if you want to remove it from the index only.",
+                            throw new RemoveFromIndexException("Unable to remove file '{0}', as it has local modifications. You can call the Remove() method with removeFromWorkingDirectory=false if you want to remove it from the index only.",
                                                                treeEntryChanges.Path);
                         }
                         removed.Add(RemoveFromIndex(treeEntryChanges.Path));
                         continue;

                     default:
-                        throw new RemoveFromIndexException(CultureInfo.InvariantCulture,
-                                                           "Unable to remove file '{0}'. Its current status is '{1}'.",
+                        throw new RemoveFromIndexException("Unable to remove file '{0}'. Its current status is '{1}'.",
                                                            treeEntryChanges.Path,
                                                            treeEntryChanges.Status);
                 }
diff --git a/LibGit2Sharp/UnbornBranchException.cs b/LibGit2Sharp/UnbornBranchException.cs
index 5efa884..1f1feb4 100644
--- a/LibGit2Sharp/UnbornBranchException.cs
+++ b/LibGit2Sharp/UnbornBranchException.cs
@@ -32,7 +32,7 @@ public UnbornBranchException(string message)
         /// <param name="format">A composite format string for use in <see cref="String.Format(IFormatProvider, string, object[])"/>.</param>
         /// <param name="args">An object array that contains zero or more objects to format.</param>
         public UnbornBranchException(CultureInfo cultureInfo, string format, params object[] args)
-            : base(cultureInfo, format, args)
+            : base(format, args)
         { }

         /// <summary>

Of course, you'll have to remove those changes from your second commit.

…always InvariantCulture

odedw · 2015-10-20T13:29:46Z

@nulltoken Done, thanks for the help.

String format ctor for exceptions

nulltoken · 2015-10-20T13:45:12Z

Merged 💥 ‼️

@odedw Very neat first PR! ✨ ✨ ✨

odedw · 2015-10-20T13:52:08Z

Thanks guys! Here's to many more 👍

nulltoken · 2015-10-20T13:56:15Z

@odedw If you want an easy one, #1203 (discovered thanks to a build issue in this one) is yours 😉

odedw mentioned this pull request Oct 18, 2015

Ensure all LibGit2SharpException derived types exposes a string.Format friendly ctor #1199

Closed

nulltoken mentioned this pull request Oct 20, 2015

Create missing Directory entry in GitErrorCode #1203

Closed

odedw force-pushed the string_format_ctor_for_exceptions branch from 4877b28 to 51d560a Compare October 20, 2015 10:44

Oded Welgreen added 2 commits October 20, 2015 16:14

Remove CultureInfo parameter from LibGit2sharpException ctor and use …

7dd95b8

…always InvariantCulture

Add string format ctor to all exceptions and change existing usages

0625b7f

odedw force-pushed the string_format_ctor_for_exceptions branch from 51d560a to 0625b7f Compare October 20, 2015 13:21

nulltoken added a commit that referenced this pull request Oct 20, 2015

Merge pull request #1202 from odedw/string_format_ctor_for_exceptions

9f334aa

String format ctor for exceptions

nulltoken merged commit 9f334aa into libgit2:vNext Oct 20, 2015

nulltoken added this to the v0.22 milestone Oct 20, 2015

nulltoken mentioned this pull request Oct 20, 2015

Strange error in Mac OS X build #1204

Closed

String format ctor for exceptions #1202

String format ctor for exceptions #1202

Uh oh!

Conversation

odedw commented Oct 17, 2015

Uh oh!

nulltoken commented Oct 18, 2015

Uh oh!

whoisj commented Oct 19, 2015

Uh oh!

ethomson commented Oct 19, 2015

Uh oh!

odedw commented Oct 19, 2015

Uh oh!

whoisj commented Oct 19, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

odedw commented Oct 20, 2015

Uh oh!

nulltoken commented Oct 20, 2015

Uh oh!

Uh oh!