Skip to content

Simple function added that check is valid git repository path#394

Merged
nulltoken merged 1 commit into
libgit2:vNextfrom
KindDragon:checkIsValidRepositoryPath
Apr 17, 2013
Merged

Simple function added that check is valid git repository path#394
nulltoken merged 1 commit into
libgit2:vNextfrom
KindDragon:checkIsValidRepositoryPath

Conversation

@KindDragon

Copy link
Copy Markdown
Contributor

without initializing Repository function

@arrbee

arrbee commented Apr 14, 2013

Copy link
Copy Markdown
Member

May I suggest you use git_repository_open_ext() with a NULL out pointer to test if there is an openable repository at the path without actually allocating the repository object. The behavior is designed for this use case without the overhead of allocating the repository data structures.

@KindDragon

Copy link
Copy Markdown
Contributor Author

Cool! I rewrite it

@dahlbyk

dahlbyk commented Apr 15, 2013

Copy link
Copy Markdown
Member
  1. Can you define an enum for flags?
  2. Tests!

Comment thread LibGit2Sharp/Core/NativeMethods.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please drop this overload. It's safer to rely on SafeHandles.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You'll certainly have to create a NullRepositorySafeHandle type to cope with this constraint.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I cannot pass null pointer to SafeHandle parameter (null pointer to pointer)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I cannot pass null pointer to SafeHandle parameter (null pointer to pointer)

See above. "You'll certainly have to create a NullRepositorySafeHandle type to cope with this constraint."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work. git_repository_open_ext throw System.ArgumentNullException

[DllImport(libgit2)]
internal static extern int git_repository_open_ext(
    ref NullRepositorySafeHandle repository,
    [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(FilePathMarshaler))] FilePath path,
    int flags,
    [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(FilePathMarshaler))] FilePath ceilingDirs);

NullRepositorySafeHandle safeHandle = null;
Proxy.git_repository_open_ext(ref safeHandle, path, 1, null);
    mscorlib.dll!System.StubHelpers.StubHelpers.SafeHandleAddRef(System.Runtime.InteropServices.SafeHandle pHandle, ref bool success) + 0x7c bytes  
>   LibGit2Sharp.dll!LibGit2Sharp.Core.Proxy.git_repository_open_ext(ref LibGit2Sharp.Core.Handles.NullRepositorySafeHandle repo, string path, int flags, string ceilingDirs) Line 1661 + 0x49 bytes    C#
    LibGit2Sharp.dll!LibGit2Sharp.Repository.IsValid(string path) Line 148 + 0x16 bytes C#
    GitCommands.dll!GitCommands.GitModule.IsValidGitWorkingDir(string dir) Line 352 + 0xa bytes C#

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@KindDragon You're completely correct! Thanks a lot for having pointed at this.

I fixed as well the definition of the NullRepositorySafeHandle type. My proposal was indeed a bit hasty. 😊

Applying the following patch should now make the code behave as expected

diff --git a/LibGit2Sharp/Core/Handles/NullRepositorySafeHandle.cs b/LibGit2Sharp/Core/Handles/NullRepositorySafeHandle.cs
index fa00b0b..cf57b96 100644
--- a/LibGit2Sharp/Core/Handles/NullRepositorySafeHandle.cs
+++ b/LibGit2Sharp/Core/Handles/NullRepositorySafeHandle.cs
@@ -2,7 +2,7 @@

 namespace LibGit2Sharp.Core.Handles
 {
-    internal class NullRepositorySafeHandle : GitObjectSafeHandle
+    internal class NullRepositorySafeHandle : SafeHandleBase
     {
         public NullRepositorySafeHandle()
         {
diff --git a/LibGit2Sharp/Core/NativeMethods.cs b/LibGit2Sharp/Core/NativeMethods.cs
index 10b9332..022f824 100644
--- a/LibGit2Sharp/Core/NativeMethods.cs
+++ b/LibGit2Sharp/Core/NativeMethods.cs
@@ -907,7 +907,7 @@ private static bool IsRunningOnLinux()

         [DllImport(libgit2)]
         internal static extern int git_repository_open_ext(
-            out NullRepositorySafeHandle repository,
+            NullRepositorySafeHandle repository,
             [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(FilePathMarshaler))] FilePath path,
             RepositoryOpenFlags flags,
             [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(FilePathMarshaler))] FilePath ceilingDirs);
diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs
index 5df546c..b6529b4 100644
--- a/LibGit2Sharp/Core/Proxy.cs
+++ b/LibGit2Sharp/Core/Proxy.cs
@@ -1658,10 +1658,15 @@ public static void git_repository_open_ext(string path, RepositoryOpenFlags flag
         {
             using (ThreadAffinity())
             {
-                NullRepositorySafeHandle repo;
+                int res;
+
+                using (var repo = new NullRepositorySafeHandle())
+                {
+                    res = NativeMethods.git_repository_open_ext(repo, path, flags, ceilingDirs);

-                int res = NativeMethods.git_repository_open_ext(out repo, path, flags, ceilingDirs);
-                repo.Dispose();
+                    // To be removed before final merge
+                    System.Diagnostics.Debug.Assert(repo.DangerousGetHandle() == IntPtr.Zero);
+                }

                 if (res == (int)GitErrorCode.NotFound)
                 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This git_repository_open_ext declaration will crash app if we pass not null handle, because of that I create two git_repository_open_ext declarations one that use out parameter (this method should be used in future instead of non ext method) and declaration with IntPtr that should be used only with IntPtr.Zero.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This git_repository_open_ext declaration will crash app if we pass not null handle

Would you please elaborate on this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not crash. It just can't return SafeHandle.
git_repository_open_ext with current declaration cannot return handle. It's by design?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's by design?

Yes it is. A different overload will have to be created when this is required to be used.

@nulltoken

Copy link
Copy Markdown
Member

This is a very neat addition!

Could you please squash the commits altogether?

@nulltoken

Copy link
Copy Markdown
Member

@KindDragon Could you please squash my proposal into you commit as well? 🙏

… initializing it

NullRepositorySafeHandle based proposal
@KindDragon

Copy link
Copy Markdown
Contributor Author

@nulltoken ok, sorry

@nulltoken
nulltoken merged commit 98a708f into libgit2:vNext Apr 17, 2013
@nulltoken

Copy link
Copy Markdown
Member

@KindDragon Merged!

✨✨✨✨✨✨✨✨✨✨

👍 🐉

@KindDragon
KindDragon deleted the checkIsValidRepositoryPath branch April 17, 2013 20:56
@KindDragon
KindDragon restored the checkIsValidRepositoryPath branch April 17, 2013 21:06
@KindDragon
KindDragon deleted the checkIsValidRepositoryPath branch April 17, 2013 21:07
@KindDragon

Copy link
Copy Markdown
Contributor Author

👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants