Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Cleanup error marshaling and generating code #183

Closed
wants to merge 6 commits into from

4 participants

@tclem
Owner

This is my second attempt to clean up error handling a bit. Here are the highlights.

  • I don't like Class or Type so I'm calling the libgit2 error class Category instead.
  • I'm using a custom marshaler for GitError, but we still have to manually do the UTF8 marshaling for the error message
  • Error Code and Category are now available on the exception as true properties so that consumers can use them instead of parsing the error message we generate.

This PR replaces #163

@tclem tclem referenced this pull request
Closed

Csharp friendly errors #163

@nulltoken nulltoken commented on the diff
LibGit2Sharp/Core/GitErrorMarshaler.cs
@@ -0,0 +1,44 @@
+using System;
+using System.Runtime.InteropServices;
+
+namespace LibGit2Sharp.Core
+{
+ internal class GitErrorMarshaler : ICustomMarshaler
@nulltoken Owner

Classy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
LibGit2Sharp/LibGit2SharpException.cs
((4 lines not shown))
namespace LibGit2Sharp
{
/// <summary>
/// The exception that is thrown when an error occurs during application execution.
/// </summary>
- [Obsolete("This type will be removed in the next release. Please use LibGit2SharpException instead.")]
- public class LibGit2Exception : LibGit2SharpException
@nulltoken Owner

Please do not remove the obsolete LibGit2Exception yet.This will be done after the next version (v0.10.0) is released. @dahlbyk has already pushed #181 to deal with this

@tclem Owner
tclem added a note

Cool. Added this back in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken
Owner

As discussed in #137, there are no so many error codes, and depending on the method being executed, the same error code may mean something different.

I'm not in favor of exposing those Categories and ErrorCodes as properties to the consumer as this would compel him to understand the correct meaning of "this error code in that context". I'd prefer exposing concrete Exceptions, deriving from LibGit2SharpException (RepositoryNotFoundException, ...).

My feeling is that if we're not able to infer correct exceptions from the (codes|categories|executionContext) there's zero chance that the consumer will be able to correctly handle those (codes|categories|executionContext) tuples by himself.

Moreover, by exposing our own exceptions, I think we might be more committed to maintain their "contract" (what they mean, when they're thrown) rather than if we just hand out to the consumer what libgit2 just returned.

However, there may be headaches down the road... For instance, when the Klass/Category is Invalid, instead of throwing a derived LibGit2SharpException, should we rather throw an plain ArgumentException?

Thoughts?

@dahlbyk
Collaborator

:+1: for ArgumentException and its derivatives for incorrect arguments, and descriptive subclasses of LibGit2SharpException instead of exposing the internal error code.

@tclem
Owner

Even if we do add specific exception types for the variety of class/code combinations, I personally would still like access to the real libgit2 error codes in the actual exception objects. When dealing with exceptional behavior, I've always found that the more you can pull away the abstractions and get to the real information, the better.

If we do go through and add new exception types that derive from LibGit2SharpException I would still want to know the libgit2 error codes/classes and not just in a pre-formatted string. I don't think these two approaches are mutually exclusive.

@nulltoken
Owner

I would still want to know the libgit2 error codes/classes and not just in a pre-formatted string

Fair enough :) Please add them to the Exception.Data dictionary.

@tclem
Owner

Great idea.

@nulltoken
Owner

https://github.com/nulltoken/libgit2sharp/tree/err fails on Mono.

Build log is available here

@tclem
Owner

https://github.com/tclem/libgit2sharp/tree/err should fix it. I don't have a mono setup to verify with right now unfortunately.

@nulltoken
Owner

Unfortunately, it doesn't. See the build log.
Previous build was targeting a squashed version of your changes. In order to ease the comparing, I've unsquashed everything and pushed it @ https://github.com/nulltoken/libgit2sharp/tree/err

I don't know if the same instance of Marshaler is used to call marshalNativeToManaged() and CleanUpNativeData()

LibGit2Sharp/LibGit2SharpException.cs
@@ -48,6 +54,16 @@ public LibGit2SharpException()
}
/// <summary>
+ /// Initializes a new instance of the <see cref = "LibGit2SharpException" /> class.
+ /// </summary>
+ public LibGit2SharpException(GitErrorCode code, GitErrorCategory category, string message) : base(message)
+ {
+ Data["libgit2.code"] = this.code = code;
+ Data["libgit2.class"] = this.category = category;
+ isLibraryError = true;
@Haacked Owner
Haacked added a note

Why did you stop exposing the GitErrorCode as a strongly typed property? We actually reference that in GHfW. Am I supposed to reach into Data["libgit2.code"] instead? That makes the code uglier and feels more fragile.

@nulltoken Owner

Why did you stop exposing the GitErrorCode as a strongly typed property?

I'm not sure to follow you. GitErrorCode has been turned Internal more than a year ago (cf 2bd0630)

@Haacked Owner
Haacked added a note

Ah, I think in our branch we exposed it as a property. Why would you make this internal and not give me anything else I can use to know exactly what the error is? Parsing exception messages is piss poor and likely to change. Reaching into the Data property is ugly too.

@dahlbyk Collaborator
dahlbyk added a note

The advantage of Data is that it's automatically serialized, whereas custom properties are not. What about public properties that encapsulate retrieving the code/class from Data?

@Haacked Owner
Haacked added a note

@dahlbyk Sure. As a consumer of the API, I sort of don't care what the backing storage of the properties are. As it stands, I have my own extension methods that do this, but it seems like a no-brainer to have it in the class as part of the API.

Another option is to implement the serialization constructors for the exception type per FX Design Guidelines: http://msdn.microsoft.com/en-us/library/ms229064.aspx

@dahlbyk Collaborator
dahlbyk added a note

Implementing deserialization isn't hard, it's just more effort than using Data.

@Haacked Owner
Haacked added a note

Sure, I really don't care how it's done. :)

But while we're on the topic, it does seem odd to me because Data is read/write, no? So that would allow any code along the stack to modify properties in there. Thus if you created GitErrorCode as a wrapper property, it wouldn't really be readonly, though semantically that's exactly what I'd expect.

All for a little less effort?

@nulltoken Owner

@Haacked

All for a little less effort?

It's not about avoiding implementing the deserialization. Although that's a nice side benefit :)

You can peek at the following comments for a better context

@Haacked Owner
Haacked added a note

@nulltoken ah, thanks for the context! I like the idea of exposing specific exception classes. But I didn't see such classes yet.

@nulltoken Owner

@Haacked You're right. They have to be created yet as discussed in #137.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken nulltoken commented on the diff
LibGit2Sharp/LibGit2SharpException.cs
((6 lines not shown))
/// </summary>
- /// <param name = "info">The <see cref="SerializationInfo "/> that holds the serialized object data about the exception being thrown.</param>
- /// <param name = "context">The <see cref="StreamingContext"/> that contains contextual information about the source or destination.</param>
- protected LibGit2SharpException(SerializationInfo info, StreamingContext context)
- : base(info, context)
+ public GitErrorCode Code
@nulltoken Owner

@tclem @Haacked As previously discussed, I'm really not willing to make the GitErrorCode and GitErrorCategory types publicly exposed, nor to expose such properties. Indeed, I don't feel the grain is fine enough (yet?) to safely build a stable/long lasting error handling strategy.

Therefore, for now, I'd prefer to only expose int typed values in the Data dictionary.
This makes the libgit2 native error codes available for logging purpose.

In order to reduce the pain, we'll indeed have to add proper exceptions to LibGit2Sharp. Which one would you see fit?

@nulltoken Owner

@tclem @Haacked I've created #189 to cope with the lack of proper exceptions issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nulltoken nulltoken referenced this pull request
Closed

Create proper exceptions #189

@nulltoken
Owner

@tclem I manually merged it in vNext. I focused on making the native error data accessible but dropped the Marshaler for now.

I don't have a mono setup to verify with right now unfortunately.

Now that @travisbot is watching our back with a Mono build for each PR, it should be easier ;-)

:heart:

@nulltoken nulltoken closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 15, 2012
  1. @tclem
Commits on Jun 18, 2012
  1. @tclem

    Add libgit2exception back in

    tclem authored
  2. @tclem
Commits on Jun 21, 2012
  1. @tclem

    Merge remote-tracking branch 'origin/vNext' into error-handling-refactor

    tclem authored
    Conflicts:
    	LibGit2Sharp/LibGit2SharpException.cs
  2. @tclem
  3. @tclem
This page is out of date. Refresh to see the latest.
View
27 LibGit2Sharp/Core/Ensure.cs
@@ -62,26 +62,19 @@ public static void Success(int result, bool allowPositiveResult = false)
return;
}
- string errorMessage;
- GitError error = NativeMethods.giterr_last().MarshalAsGitError();
-
-
+ var error = NativeMethods.giterr_last();
if (error == null)
{
- error = new GitError { Klass = GitErrorType.Unknown, Message = IntPtr.Zero };
- errorMessage = "No error message has been provided by the native library";
- }
- else
- {
- errorMessage = Utf8Marshaler.FromNative(error.Message);
+ throw new LibGit2SharpException(
+ (GitErrorCode)result,
+ GitErrorCategory.Unknown,
+ "No error message has been provided by the native library");
}
throw new LibGit2SharpException(
- String.Format(CultureInfo.InvariantCulture, "An error was raised by libgit2. Class = {0} ({1}).{2}{3}",
- error.Klass,
- result,
- Environment.NewLine,
- errorMessage));
+ (GitErrorCode)result,
+ error.Category,
+ Utf8Marshaler.FromNative(error.Message));
}
/// <summary>
@@ -108,8 +101,8 @@ public static void GitObjectIsNotNull(GitObject gitObject, string identifier)
}
throw new LibGit2SharpException(string.Format(CultureInfo.InvariantCulture,
- "No valid git object identified by '{0}' exists in the repository.",
- identifier));
+ "No valid git object identified by '{0}' exists in the repository.",
+ identifier));
}
}
}
View
2  LibGit2Sharp/Core/GitError.cs
@@ -7,6 +7,6 @@ namespace LibGit2Sharp.Core
internal class GitError
{
public IntPtr Message;
- public GitErrorType Klass;
+ public GitErrorCategory Category;
}
}
View
2  LibGit2Sharp/Core/GitErrorType.cs → LibGit2Sharp/Core/GitErrorCategory.cs
@@ -1,6 +1,6 @@
namespace LibGit2Sharp.Core
{
- internal enum GitErrorType
+ public enum GitErrorCategory
{
Unknown = -1,
NoMemory,
View
2  LibGit2Sharp/Core/GitErrorCode.cs
@@ -1,6 +1,6 @@
namespace LibGit2Sharp.Core
{
- internal enum GitErrorCode
+ public enum GitErrorCode
{
Ok = 0,
Error = -1,
View
47 LibGit2Sharp/Core/GitErrorMarshaler.cs
@@ -0,0 +1,47 @@
+using System;
+using System.Runtime.InteropServices;
+
+namespace LibGit2Sharp.Core
+{
+ internal class GitErrorMarshaler : ICustomMarshaler
@nulltoken Owner

Classy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ static readonly GitErrorMarshaler staticInstance = new GitErrorMarshaler();
+
+ public void CleanUpManagedData(object managedObj)
+ {
+ }
+
+ public void CleanUpNativeData(IntPtr pNativeData)
+ {
+ if (pNativeData != IntPtr.Zero)
+ {
+ Marshal.FreeHGlobal(pNativeData);
+ }
+ }
+
+ public int GetNativeDataSize()
+ {
+ return -1;
+ }
+
+ public IntPtr MarshalManagedToNative(object managedObj)
+ {
+ throw new NotImplementedException();
+ }
+
+ public object MarshalNativeToManaged(IntPtr pNativeData)
+ {
+ return NativeToGitError(pNativeData);
+ }
+
+ protected GitError NativeToGitError(IntPtr pNativeData)
+ {
+ return (GitError)Marshal.PtrToStructure(pNativeData, typeof(GitError));
+ }
+
+ public static ICustomMarshaler GetInstance(string cookie)
+ {
+ return staticInstance;
+ }
+ }
+}
View
12 LibGit2Sharp/Core/Handles/GitErrorSafeHandle.cs
@@ -1,12 +0,0 @@
-using System.Runtime.InteropServices;
-
-namespace LibGit2Sharp.Core.Handles
-{
- internal class GitErrorSafeHandle : NotOwnedSafeHandleBase
- {
- public GitError MarshalAsGitError()
- {
- return (GitError)Marshal.PtrToStructure(handle, typeof(GitError));
- }
- }
-}
View
3  LibGit2Sharp/Core/NativeMethods.cs
@@ -65,7 +65,8 @@ public static bool RepositoryStateChecker(RepositorySafeHandle repositoryPtr, Fu
}
[DllImport(libgit2)]
- public static extern GitErrorSafeHandle giterr_last();
+ [return: MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(GitErrorMarshaler))]
+ public static extern GitError giterr_last();
[DllImport(libgit2)]
public static extern int git_blob_create_fromdisk(
View
4 LibGit2Sharp/LibGit2Sharp.csproj
@@ -78,10 +78,10 @@
<Compile Include="Core\GitDiff.cs" />
<Compile Include="Core\GitDiffExtensions.cs" />
<Compile Include="Core\GitError.cs" />
- <Compile Include="Core\GitErrorType.cs" />
+ <Compile Include="Core\GitErrorCategory.cs" />
+ <Compile Include="Core\GitErrorMarshaler.cs" />
<Compile Include="Core\GitNoteData.cs" />
<Compile Include="Core\GitObjectExtensions.cs" />
- <Compile Include="Core\Handles\GitErrorSafeHandle.cs" />
<Compile Include="Core\Handles\NoteSafeHandle.cs" />
<Compile Include="Core\Handles\ObjectDatabaseSafeHandle.cs" />
<Compile Include="Core\Handles\DiffListSafeHandle.cs" />
View
45 LibGit2Sharp/LibGit2SharpException.cs
@@ -1,5 +1,6 @@
using System;
-using System.Runtime.Serialization;
+using System.Globalization;
+using LibGit2Sharp.Core;
namespace LibGit2Sharp
{
@@ -39,9 +40,12 @@ public LibGit2Exception(string message, Exception innerException)
/// <summary>
/// The exception that is thrown when an error occurs during application execution.
/// </summary>
- [Serializable]
public class LibGit2SharpException : Exception
{
+ readonly GitErrorCode code;
+ readonly GitErrorCategory category;
+ readonly bool isLibraryError;
+
/// <summary>
/// Initializes a new instance of the <see cref = "LibGit2SharpException" /> class.
/// </summary>
@@ -50,6 +54,16 @@ public LibGit2SharpException()
}
/// <summary>
+ /// Initializes a new instance of the <see cref = "LibGit2SharpException" /> class.
+ /// </summary>
+ public LibGit2SharpException(GitErrorCode code, GitErrorCategory category, string message) : base(message)
+ {
+ Data["libgit2.code"] = this.code = code;
+ Data["libgit2.class"] = this.category = category;
+ isLibraryError = true;
+ }
+
+ /// <summary>
/// Initializes a new instance of the <see cref = "LibGit2SharpException" /> class with a specified error message.
/// </summary>
/// <param name = "message">A message that describes the error. </param>
@@ -69,13 +83,30 @@ public LibGit2SharpException(string message, Exception innerException)
}
/// <summary>
- /// Initializes a new instance of the <see cref = "LibGit2SharpException" /> class with a serialized data.
+ /// The specific libgit2 error code.
/// </summary>
- /// <param name = "info">The <see cref="SerializationInfo "/> that holds the serialized object data about the exception being thrown.</param>
- /// <param name = "context">The <see cref="StreamingContext"/> that contains contextual information about the source or destination.</param>
- protected LibGit2SharpException(SerializationInfo info, StreamingContext context)
- : base(info, context)
+ public GitErrorCode Code
@nulltoken Owner

@tclem @Haacked As previously discussed, I'm really not willing to make the GitErrorCode and GitErrorCategory types publicly exposed, nor to expose such properties. Indeed, I don't feel the grain is fine enough (yet?) to safely build a stable/long lasting error handling strategy.

Therefore, for now, I'd prefer to only expose int typed values in the Data dictionary.
This makes the libgit2 native error codes available for logging purpose.

In order to reduce the pain, we'll indeed have to add proper exceptions to LibGit2Sharp. Which one would you see fit?

@nulltoken Owner

@tclem @Haacked I've created #189 to cope with the lack of proper exceptions issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ get { return Data.Contains("libgit2.code") ? (GitErrorCode)Data["libgit2.code"] : GitErrorCode.Error; }
+ }
+
+ /// <summary>
+ /// The specific libgit2 error class.
+ /// </summary>
+ public GitErrorCategory Category
+ {
+ get { return Data.Contains("libgit2.class") ? (GitErrorCategory)Data["libgit2.class"] : GitErrorCategory.Unknown; }
+ }
+
+ public override string ToString()
{
+ return isLibraryError
+ ? String.Format(CultureInfo.InvariantCulture, "An error was raised by libgit2. Class = {0} ({1}).{2}{3}",
+ category,
+ code,
+ Environment.NewLine,
+ Message)
+ : base.ToString();
}
}
}
Something went wrong with that request. Please try again.