Remote LS able to handle Symbolic References #1132

psawey · 2015-07-01T23:11:12Z

Initial thoughts on #1085, still work in progress.

psawey · 2015-07-01T23:13:56Z

LibGit2Sharp/Core/Proxy.cs

                {
                    throw new InvalidOperationException("Not expecting null value for reference name.");
                }

-                string name = LaxUtf8Marshaler.FromNative(remoteHead.NamePtr);
-                refs.Add(new DirectReference(name, repository, remoteHead.Oid));
+                string symRefTarget = LaxUtf8Marshaler.FromNative(remoteHead.SymRefTargetPtr);


Looking for best way to resolve this SymbolicReference to it's target

nulltoken · 2015-07-04T14:15:48Z

@psawey That's a pretty good start!

I've played a bit with it.

Tweaking the Proxy as follows does most of the heavy-lifting. There are some other smaller things to iron out, but that should give you a head start.

diff --git a/LibGit2Sharp/Core/Proxy.cs b/LibGit2Sharp/Core/Proxy.cs
index 09ad8a1..b4586d8 100644
--- a/LibGit2Sharp/Core/Proxy.cs
+++ b/LibGit2Sharp/Core/Proxy.cs
@@ -2142,26 +2142,57 @@ public static IEnumerable<DirectReference> git_remote_ls(Repository repository,
                 throw new OverflowException();
             }

-            var refs = new List<Reference>();
+            var dirRefs = new Dictionary<string, Reference>();
+            var symRefs = new Dictionary<string, string>();
+
             IntPtr currentHead = heads;

             for (int i = 0; i < intCount; i++)
             {
                 var remoteHead = Marshal.ReadIntPtr(currentHead).MarshalAs<GitRemoteHead>();
+
                 string name = LaxUtf8Marshaler.FromNative(remoteHead.NamePtr);
+                string target = LaxUtf8Marshaler.FromNative(remoteHead.SymRefTargetPtr);

                 // The name pointer should never be null - if it is,
                 // this indicates a bug somewhere (libgit2, server, etc).
-                if (string.IsNullOrEmpty(name))
+                Debug.Assert(!string.IsNullOrEmpty(name));
+
+                if (!string.IsNullOrEmpty(target))
                 {
-                    throw new InvalidOperationException("Not expecting null value for reference name.");
+                    Trace.TraceInformation("{0} -> {1}", name, target);
+
+                    symRefs.Add(name, target);
                 }
+                else
+                {
+                    Trace.TraceInformation("{0} -> {1}", name, new ObjectId(remoteHead.Oid).Sha);

-                string symRefTarget = LaxUtf8Marshaler.FromNative(remoteHead.SymRefTargetPtr);
+                    var directReference = new DirectReference(name, repository, remoteHead.Oid);
+                    dirRefs.Add(name, directReference);
+                }

-                if (!string.IsNullOrEmpty(symRefTarget) && Reference.IsValidName(name))
-                {
-                    //ReferenceSafeHandle handle;
-                    //NativeMethods.git_reference_lookup(out handle, repository.Handle, symRefTarget);
-                    using (var handle = Proxy.git_reference_lookup(repository.Handle, symRefTarget, false))
-                    {
-                        var reference = NativeMethods.git_reference_symbolic_target(handle);
-                    }
+                currentHead = IntPtr.Add(currentHead, IntPtr.Size);
+            }
+
+            while (symRefs.Count > 0)
+            {
+                var kvp = symRefs.First();

-                    //var symbolicReference = new SymbolicReference(repository, name, symRefTarget, reference);
-                    //refs.Add(symbolicReference);
+                if (dirRefs.ContainsKey(kvp.Value))
+                {
+                    dirRefs.Add(kvp.Key, new SymbolicReference(repository, kvp.Key, kvp.Value, dirRefs[kvp.Value]));
                 }
                 else
                 {
-                    refs.Add(new DirectReference(name, repository, remoteHead.Oid));
+                    throw new InvalidOperationException("Hmpf... What should we do in this case?");
                 }

-                currentHead = IntPtr.Add(currentHead, IntPtr.Size);
+                symRefs.Remove(kvp.Key);
             }

+            var refs = dirRefs.Values.ToList();
+            refs.Sort((r1, r2) => String.CompareOrdinal(r1.CanonicalName, r2.CanonicalName));
             return refs;
         }

nulltoken · 2015-07-04T14:16:48Z

BTW, when you're back working on this, would you please rebase your work on top of vNext?

psawey · 2015-07-07T22:44:37Z

@nulltoken 💥 Should have known to get the SymbolicReference target from the git_remote_ls results, similar implementation to ReferenceCollection. Was under the impression that there would be cases when a SymbolicReference would point to a target not returned by that method, requiring a way to resolve it somehow.

psawey · 2015-07-07T23:41:08Z

👍 Rebased!

nulltoken · 2015-07-08T07:52:05Z

LibGit2Sharp/Core/Proxy.cs

+            {
+                var symRef = symRefs.First();
+
+                if (!directRefs.ContainsKey(symRef.Value))


May chained symbolic references be returned by git ls-remote. Eg. Can it return something like?

HEAD -> CHAINED CHAINED -> refs/heads/master refs/heads/master -> deadbeefcafe.....

/cc @dahlbyk @carlosmn

Only direct references and HEAD (which may be symbolic or direct) are supported by the current protocol.

That's true of the base protocol, but there is an extension named symref which looks like symref=HEAD:refs/heads/notmaster symref=refs/heads/somebranch:refs/heads/targetbranch which does let the server specify which references (which look like direct refs in the listing) are actually symbolic references and what they point to.

This extension is mostly intended to let the client know which is the default branch in cases when there are multiple branches which point to the same commit as HEAD, but any of the refs advertised could be symbolic.

As to the chain depth, there's no particular limit other than what the implementation is willing to accept before it decides to trigger its loop short-circuit code (which IIRC is 7 deep).

Upon further investigation, it looks like git won't actually add the symref list for anything other than HEAD, and it will resolve to the final reference, so you won't end up with chains.

psawey · 2015-07-10T01:23:17Z

LibGit2Sharp/Core/Proxy.cs

+                }
+                else
+                {
+                    directRefs.Add(name, new DirectReference(name, repository, remoteHead.Oid));


One additional thought on representing tags, as originally mentioned in #1085

c070ad8c08840c8116da865b2d65593a6bb9cd2a refs/tags/annotated_tag^{}

should these dereferenced tags be represented in the final output, possibly

if (!string.IsNullOrEmpty(symRefTarget)) { symRefs.Add(name, symRefTarget); } else if (Reference.IsValidName(name)) {

@nulltoken Thoughts on this?

I'd be 👍 to not extend the scope of this PR too much.

How about logging a new issue to keep track of this and discuss about pros/cons there?

psawey · 2015-07-10T01:44:07Z

LibGit2Sharp.Tests/NetworkFixture.cs


                Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs.Count, actualRefs.Count);
+                Assert.True(references.Single(reference => reference.CanonicalName == "HEAD") is SymbolicReference);


Amended the PR to verify HEAD References as SymbolicReferences

Note that HEAD is not always a symbolic reference.

Out it goes then!

@psawey I think the Assert should stay.

Indeed, in this* case, HEAD should actually be a SymboilcReference.

I think @carlosmn was hinting at the fact that would the HEAD be detached server-side, we should make sure the code still works.

Do you think you could add some test coverage to assert this?

IIRC there's a test in PushFixture that leverages two repository through the local transport. Maybe could we do the same (setup a fake server, detach the HEAD, then invoke RetrieveRemoteReferences() against it?)

@nulltoken Spoke too soon, will take a look at adding test coverge. Have been trying to brainstorm some additional tests, hence the Assert

jamill · 2015-07-10T20:29:01Z

LibGit2Sharp/Core/Proxy.cs


                currentHead = IntPtr.Add(currentHead, IntPtr.Size);
            }

+            while (symRefs.Count > 0)


Is there a particular reason to remove the symRefs as we process them? could we just use a plain loop here?

@jamill I think this comes from my initial spike. IIRC when I wrote this I wasn't sure whether or not chained symrefs could be reported, thus the while reprocessing.

Considering that @carlosmn cleared that out, your proposal makes indeed a lot of sense and should lead to a easier to read code.

psawey · 2015-07-13T22:16:33Z

LibGit2Sharp.Tests/NetworkFixture.cs

+        {
+            var scd = BuildSelfCleaningDirectory();
+
+            string originalRepoPath = SandboxStandardTestRepo();


@nulltoken Included test coverage on detached remote head

🆒

I've played a bit with your idea. What do you think of the following rebound which actually leverages your code from #1065 😉?

[Fact] public void CanListRemoteReferencesWithDetachedRemoteHead() { string originalRepoPath = SandboxStandardTestRepo(); string detachedHeadSha; using (var originalRepo = new Repository(originalRepoPath)) { detachedHeadSha = originalRepo.Head.Tip.Sha; originalRepo.Checkout(detachedHeadSha); Assert.True(originalRepo.Info.IsHeadDetached); } IEnumerable<Reference> references = Repository.ListRemoteReferences(originalRepoPath); Reference head = references.SingleOrDefault(reference => reference.CanonicalName == "HEAD"); Assert.NotNull(head); Assert.True(head is DirectReference); Assert.Equal(detachedHeadSha, head.TargetIdentifier); }

@nulltoken 👍 Makes for a much cleaner test! Was thinking the additional Clone was needed when calling ListRemoteReferences

nulltoken · 2015-07-15T20:36:24Z

Fix #1085

nulltoken · 2015-07-15T20:36:56Z

@dahlbyk This was your idea to being with. Any comment on the proposal?

returned by remote ls

Remote LS able to handle Symbolic References

nulltoken · 2015-07-17T06:08:12Z

💣

dahlbyk · 2015-07-21T03:52:49Z

Late to the party, but this is pretty much exactly what I had in mind. 💯

nulltoken · 2015-07-21T09:56:57Z

🎉

psawey reviewed Jul 1, 2015
View reviewed changes

psawey force-pushed the remotels-symref branch from 2bb95e7 to fe881ac Compare July 7, 2015 23:40

psawey force-pushed the remotels-symref branch from fe881ac to b46587b Compare July 7, 2015 23:43

nulltoken reviewed Jul 8, 2015
View reviewed changes

psawey force-pushed the remotels-symref branch from b46587b to 018d4c5 Compare July 8, 2015 14:34

psawey reviewed Jul 10, 2015
View reviewed changes

psawey force-pushed the remotels-symref branch from 792c692 to a6ef2ac Compare July 10, 2015 01:41

psawey reviewed Jul 10, 2015
View reviewed changes

psawey force-pushed the remotels-symref branch from a6ef2ac to 6a1d0c5 Compare July 10, 2015 14:20

jamill reviewed Jul 10, 2015
View reviewed changes

psawey force-pushed the remotels-symref branch from 6a1d0c5 to 58f36f9 Compare July 13, 2015 22:09

psawey reviewed Jul 13, 2015
View reviewed changes

Functionality to correctly handle Symbolic References

f68697b

returned by remote ls

psawey force-pushed the remotels-symref branch from 58f36f9 to f68697b Compare July 17, 2015 01:47

nulltoken added a commit that referenced this pull request Jul 17, 2015

Merge pull request #1132 from psawey/remotels-symref

f4a6001

Remote LS able to handle Symbolic References

nulltoken merged commit f4a6001 into libgit2:vNext Jul 17, 2015

carlosmn mentioned this pull request Mar 11, 2016

Teach Network.ListReferences() and Repository.ListRemoteReferences to cope with symbolic references #1085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote LS able to handle Symbolic References #1132

Remote LS able to handle Symbolic References #1132

psawey commented Jul 1, 2015

psawey Jul 1, 2015

nulltoken commented Jul 4, 2015

nulltoken commented Jul 4, 2015

psawey commented Jul 7, 2015

psawey commented Jul 7, 2015

nulltoken Jul 8, 2015

ethomson Jul 8, 2015

carlosmn Jul 8, 2015

carlosmn Jul 8, 2015

carlosmn Jul 9, 2015

psawey Jul 10, 2015

psawey Jul 10, 2015

nulltoken Jul 15, 2015

psawey Jul 10, 2015

carlosmn Jul 10, 2015

psawey Jul 10, 2015

nulltoken Jul 10, 2015

psawey Jul 10, 2015

jamill Jul 10, 2015

nulltoken Jul 10, 2015

psawey Jul 13, 2015

nulltoken Jul 15, 2015

psawey Jul 17, 2015

nulltoken commented Jul 15, 2015

nulltoken commented Jul 15, 2015

nulltoken commented Jul 17, 2015

dahlbyk commented Jul 21, 2015

nulltoken commented Jul 21, 2015


		Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs.Count, actualRefs.Count);
		Assert.True(references.Single(reference => reference.CanonicalName == "HEAD") is SymbolicReference);

Remote LS able to handle Symbolic References #1132

Remote LS able to handle Symbolic References #1132

Conversation

psawey commented Jul 1, 2015

Choose a reason for hiding this comment

nulltoken commented Jul 4, 2015

nulltoken commented Jul 4, 2015

psawey commented Jul 7, 2015

psawey commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nulltoken commented Jul 15, 2015

nulltoken commented Jul 15, 2015

nulltoken commented Jul 17, 2015

dahlbyk commented Jul 21, 2015

nulltoken commented Jul 21, 2015