The Great Libgit2 Renaming of 2012 #260
Conversation
|
The following seems to fix the issue. diff --git a/LibGit2Sharp/Core/NativeMethods.cs b/LibGit2Sharp/Core/NativeMethods.cs
index 4b014c4..0cf2a7a 100644
--- a/LibGit2Sharp/Core/NativeMethods.cs
+++ b/LibGit2Sharp/Core/NativeMethods.cs
@@ -484,8 +484,8 @@ private static bool IsRunningOnLinux()
RepositorySafeHandle repo);
internal delegate int git_note_foreach_cb(
- OidSafeHandle blob_id,
- OidSafeHandle annotated_object_id,
+ ref GitOid blob_id,
+ ref GitOid annotated_object_id,
IntPtr payload);
[DllImport(libgit2)]
diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs
index da877b9..721da1e 100644
--- a/LibGit2Sharp/Core/Proxy.cs
+++ b/LibGit2Sharp/Core/Proxy.cs
@@ -793,9 +793,12 @@ public static string git_note_default_ref(RepositorySafeHandle repo)
}
}
- public static ICollection<TResult> git_note_foreach<TResult>(RepositorySafeHandle repo, string notes_ref, Func<OidSafeHandle, OidSafeHandle, TResult> resultSelector)
+ public static ICollection<TResult> git_note_foreach<TResult>(RepositorySafeHandle repo, string notes_ref, Func<GitOid, GitOid, TResult> resultSelector)
{
- return git_foreach(resultSelector, c => NativeMethods.git_note_foreach(repo, notes_ref, (x, y, p) => c(x, y, p), IntPtr.Zero));
+ return git_foreach(resultSelector,
+ c => NativeMethods.git_note_foreach(repo, notes_ref,
+ (ref GitOid x, ref GitOid y, IntPtr p)
+ => c(x, y, p), IntPtr.Zero));
}
public static void git_note_free(IntPtr note)
diff --git a/LibGit2Sharp/NoteCollection.cs b/LibGit2Sharp/NoteCollection.cs
index 504175b..49944bd 100644
--- a/LibGit2Sharp/NoteCollection.cs
+++ b/LibGit2Sharp/NoteCollection.cs
@@ -113,7 +113,7 @@ where refCanonical.StartsWith(refsNotesPrefix, StringComparison.Ordinal) && refC
string canonicalNamespace = NormalizeToCanonicalName(@namespace);
return Proxy.git_note_foreach(repo.Handle, canonicalNamespace,
- (blobId,annotatedObjId) => RetrieveNote(annotatedObjId.MarshalAsObjectId(), canonicalNamespace));
+ (blobId,annotatedObjId) => RetrieveNote(new ObjectId(annotatedObjId), canonicalNamespace));
}
} |
| internal delegate int skipped_notify_cb( | ||
| IntPtr skipped_file, | ||
| internal delegate int conflict_cb( | ||
| IntPtr conflicting_path, |
There was a problem hiding this comment.
nit - I think these are \t chars above (and should instead be ' ')
|
Here is a patch with the diff --git a/LibGit2Sharp/Core/NativeMethods.cs b/LibGit2Sharp/Core/NativeMethods.cs
index 9f74e55..caf77b9 100644
--- a/LibGit2Sharp/Core/NativeMethods.cs
+++ b/LibGit2Sharp/Core/NativeMethods.cs
@@ -484,8 +484,8 @@ private static bool IsRunningOnLinux()
RepositorySafeHandle repo);
internal delegate int git_note_foreach_cb(
- GitOid blob_id,
- GitOid annotated_object_id,
+ IntPtr blob_id,
+ IntPtr annotated_object_id,
IntPtr payload);
[DllImport(libgit2)]
diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs
index 08c4a75..ae749d2 100644
--- a/LibGit2Sharp/Core/Proxy.cs
+++ b/LibGit2Sharp/Core/Proxy.cs
@@ -795,7 +795,9 @@ public static string git_note_default_ref(RepositorySafeHandle repo)
public static ICollection<TResult> git_note_foreach<TResult>(RepositorySafeHandle repo, string notes_ref, Func<GitOid, GitOid, TResult> resultSelector)
{
- return git_foreach(resultSelector, c => NativeMethods.git_note_foreach(repo, notes_ref, (x, y, p) => c(x, y, p), IntPtr.Zero));
+ return git_foreach(resultSelector, c => NativeMethods.git_note_foreach(repo, notes_ref,
+ (x, y, p) => c((GitOid)Marshal.PtrToStructure(x, typeof(GitOid)), (GitOid)Marshal.PtrToStructure(y, typeof(GitOid)), p),
+ IntPtr.Zero));
}
public static void git_note_free(IntPtr note) |
|
Should we also take this opportunity to fix the type the interop uses for git_off_t - which I beleive is now always a 64-bit value. For instance, git_blob_rawsize now returns git_off_t, which we are currently handling as in int in the interop (but should be a long?) |
@jamill 👍 |
|
Like this? 🎐 |
|
👍 |
|
the return type on |
|
I've rebased this on top of @jamill's latest, and included a couple of commits that adjust some interop types. |
|
I am not sure about 9a1b7f1. Maybe we should avoid this change (which affects the public API) until we are more sure how we want to handle these to avoid potential churn. For instance, it seems akward to me to treat |
|
@jamill Rebased again! As for using longs in the API; it's pretty unlikely that this will happen, but it's possible that you'll get a number larger than 2^32 from the native code. Since the CLR doesn't have an actual integer type that's the same bitness as your CPU ( Or at least that's how I read it. I'd be happy to hear a convincing argument or a better solution. 😃 |
|
@ben I'd tend to agree with @jamill here. I'd say that a repo more than 2,147,483,647 files is quite unlikely to happen. However, before casting it, as it's ok to be paranoid, let's add a test which throws an Exception if the index_count exceeds When that exception is reported as a bug, then we'll decide if returning a long is wise. 😉 |
|
How's that? |
| throw new LibGit2SharpException("Data size exceeds size of int. You probably don't want all this in memory anyway."); | ||
| } | ||
|
|
||
| Marshal.Copy(NativeMethods.git_blob_rawcontent(obj.ObjectPtr), arr, 0, (int)size); | ||
| return arr; |
There was a problem hiding this comment.
@jamill Is this what you had in mind?
There was a problem hiding this comment.
Almost - I would suggest one of the following tweaks:
- Put the size check before we allocate the byte array. Looking into this more, I suspect that attempting to allocate an array larger than int.MaxValue will throw anyways (at least with default settings).
- Keep the int on the method signature (as it the method does not handle anything larger than an int) and force callers to provide an int. I think there is only one caller - the Content property in Blob.cs
There was a problem hiding this comment.
@ben I'm still uneasy with this... I can't help but wonder, "what would the user do with a 2Gb blob, in memory"? Given that this is copied, this would mean that 4Gb of RAM are required to handle that blob. That doesn't scale well.
I'm in favor of killing this method and let the user handle the retrieval of the Blob content from the streamed version below. This would mean that the Blob type wouldn't expose a Contentproperty anymore.
@dahlbyk, @xpaulbettsx, @tclem, @phkelley, @jamill , @spraints, @gorbach Thoughts?
There was a problem hiding this comment.
I don't know that it hurts to provide Content, since it should be clear to the consumer that to make a byte array of content we would need to allocate a byte array for the content. It may be worth adding a comment in the API that for cases where that memory pressure is not acceptable we encourage the use of ContentStream instead.
There was a problem hiding this comment.
I agree with @dahlbyk - for the common case where the amount of content is going to be small, does it hurt to leave the Content property? I like the suggestion to add a comment to the API regarding using ContentStream when the memory pressure is not acceptable.
|
Thanks! One last point, which would force our decision on this point anyways: According to msdn (http://msdn.microsoft.com/en-us/library/czz5hkty.aspx), the default maximum array size in the CLR is 2 GB. With .NET 4.5 it is possible to enable larger arrays on 64-bit platforms, but this requires extra configuration and is still limited to 4 billion elements. So, even if we wanted to support Really Large Arrays, there are other limitations we would hit first. We should keep these limitations in mind and double check our other logic around potential 64-bit values. |
|
@nulltoken, @ben - I would advocate that we publish as is (modulo outstanding feedback). The current libgit2 binary in vNext is almost a month up and has changed quite a bit (and more changes have gone in since the rename work). Also, we have some changes that are waiting for LibGit2Sharp to be updated with a libgit2 with the rename changes, so merging this in sooner will definitely help in bringing those changes up to date. In fact, I would be interesting in updating libgit2 binaries to ones containing push (libgit2/libgit2@f1e5c50) and possibly libgit2/libgit2#1111 when it is ready. It would be easier to do this if LibGit2Sharp was updated with the rename work. (/cc @ethomson). Then, I could open a PR with initial push implementation in LibGit2Sharp. |
|
@nulltoken, here's a question for you. Introducing What would you like to do about this? I can see several ways of handling it, but there's also the question of whether this is a valid config in the first place; here's the relevant config section from that test repo: |
|
Wow. Looks like libgit2 is doing the wrong thing here. |
|
Although I think that failing tests are greatest thing since sliced bread, I have the feeling that now wouldn't be the most appropriate moment for this to happen ;-) @ben @dahlbyk I think libgit2 does the correct thing here. How about the following? diff --git a/LibGit2Sharp/Branch.cs b/LibGit2Sharp/Branch.cs
index d5b4c90..caa4c47 100644
--- a/LibGit2Sharp/Branch.cs
+++ b/LibGit2Sharp/Branch.cs
@@ -169,15 +169,15 @@ public virtual Remote Remote
{
get
{
- string remoteName = repo.Config.Get<string>("branch", Name, "remote", null);
- Remote remote = null;
+ var remoteName = repo.Config.Get<string>("branch", Name, "remote", null);
- if (!string.IsNullOrEmpty(remoteName))
+ if (string.IsNullOrEmpty(remoteName) ||
+ string.Equals(remoteName, ".", StringComparison.Ordinal))
{
- remote = repo.Remotes[remoteName];
+ return null;
}
- return remote;
+ return repo.Remotes[remoteName];
}
} /cc @carlosmn |
|
I guess it depends on what the purpose of |
I don't remember all the contexts where git defaults to From my point of view, the |
|
That question leaves "if this branch is the current one" implicit, which may not be clear to the consumer. There is only one default remote to fetch/push from/to at each point in time, which seems like it should make this more part of the repo, rather than the branch. What the config really says is in which remote the branch's upstream lives, and the current branch's upstream repo is the one chosen by git (or |
Wouldn't the
So, from what I understand, the default FWIW, the following test (not part of the current codebase) pass: [Fact]
public void QueryingTheRemoteForADetachedHeadBranchReturnsNull()
{
TemporaryCloneOfTestRepo path = BuildTemporaryCloneOfTestRepo(StandardTestRepoWorkingDirPath);
using (var repo = new Repository(path.DirectoryPath))
{
repo.Checkout(repo.Head.Tip.Sha, CheckoutOptions.Force, null);
Branch trackLocal = repo.Head;
Assert.Null(trackLocal.Remote);
}
} |
|
So, it seems that the fetch and push code might need a bit more work to fully support:
In the meantime, I like @nulltoken 's patch to return to the pre-update behavior (where the local repository was not supported and returned |
Yes, but this condition wasn't stated in the question and it looks like reading the doc for this attribute would cause confusion about what information is giving you. I don't think that this should be handled by fetch/push, IMO that'd be mixing what the I'm not familiar with the checkout part of the lg2# code. It looks like the test is detaching HEAD and making sure there's default remote, which is correct, because you are relying on calling |
How would you reword the documentation in order to reduce the confusion?
Huh. Actually, I think this is the opposite. It asserts there's no defined remote. But I think this is ok as one cannot push/fetch from a |
👍 |
|
Yeah, I finally came to the realization that libgit2 is probably doing the right thing; a branch that tracks another local branch doesn't really have a remote, so the name And libgit2sharp is now doing the right thing too; in the case of a local-tracking branch, there isn't a remote, so returning null is the right thing. I've integrated @nulltoken's patch; it was very close to what I had locally. That should make Travis and TeamCity happy. |
|
One more force-push; I added @nulltoken's |
This implements git clean and exposes the ability to remove untracked files from the working directory. If / when git clean functionality is exposed directly by libgit2, we can update this to use that entry point. Currently, we call git_checkout_index with a flags indicating that untracked entries should be removed. This does not support other flags exposed by core Git, such as removing ignored files.
Checkout tests were originally written under the assumption that git reset would remove untracked files (reset, as implemented in LibGit2Sharp, originally mistakenly removed untracked files). This updates the tests to reset and clean the working directory.
|
Updated to include libgit2/libgit2#1119, and |
The Great Libgit2 Renaming of 2012
|
@nulltoken I'd change the /// <summary>
/// Gets the <see cref="Remote"/> of the branch's upstream
/// </summary>
public virtual Remote Remote |
Adjust to all the renamed/reorganized APIs as of libgit2/libgit2@64c5112.
This is based from #259, so that should be merged before this. There is one failing test;
ResetHeadFixture.HardResetInABareRepositoryThrows, which is a known issue, and needs correcting in libgit2.