Skip to content

Remote LS able to handle Symbolic References #1132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 17, 2015
Merged

Remote LS able to handle Symbolic References #1132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions LibGit2Sharp.Tests/NetworkFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,22 @@ public void CanListRemoteReferences(string url)
using (var repo = new Repository(repoPath))
{
Remote remote = repo.Network.Remotes.Add(remoteName, url);
IList<DirectReference> references = repo.Network.ListReferences(remote).ToList();
IList<Reference> references = repo.Network.ListReferences(remote).ToList();

foreach (var directReference in references)

foreach (var reference in references)
{
// None of those references point to an existing
// object in this brand new repository
Assert.Null(directReference.Target);
Assert.Null(reference.ResolveToDirectReference().Target);
}

List<Tuple<string, string>> actualRefs = references.
Select(directRef => new Tuple<string, string>(directRef.CanonicalName, directRef.TargetIdentifier)).ToList();
Select(directRef => new Tuple<string, string>(directRef.CanonicalName, directRef.ResolveToDirectReference()
.TargetIdentifier)).ToList();

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

Choose a reason for hiding this comment

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

Amended the PR to verify HEAD References as SymbolicReferences

Copy link
Member

Choose a reason for hiding this comment

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

Note that HEAD is not always a symbolic reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out it goes then!

Copy link
Member

Choose a reason for hiding this comment

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

@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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

for (int i = 0; i < TestRemoteRefs.ExpectedRemoteRefs.Count; i++)
{
Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs[i].Item2, actualRefs[i].Item2);
Expand All @@ -53,19 +56,21 @@ public void CanListRemoteReferencesFromUrl(string url)

using (var repo = new Repository(repoPath))
{
IList<DirectReference> references = repo.Network.ListReferences(url).ToList();
IList<Reference> references = repo.Network.ListReferences(url).ToList();

foreach (var directReference in references)
foreach (var reference in references)
{
// None of those references point to an existing
// object in this brand new repository
Assert.Null(directReference.Target);
Assert.Null(reference.ResolveToDirectReference().Target);
}

List<Tuple<string, string>> actualRefs = references.
Select(directRef => new Tuple<string, string>(directRef.CanonicalName, directRef.TargetIdentifier)).ToList();
Select(directRef => new Tuple<string, string>(directRef.CanonicalName, directRef.ResolveToDirectReference()
.TargetIdentifier)).ToList();

Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs.Count, actualRefs.Count);
Assert.True(references.Single(reference => reference.CanonicalName == "HEAD") is SymbolicReference);
for (int i = 0; i < TestRemoteRefs.ExpectedRemoteRefs.Count; i++)
{
Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs[i].Item2, actualRefs[i].Item2);
Expand All @@ -87,18 +92,22 @@ public void CanListRemoteReferenceObjects()
using (var repo = new Repository(clonedRepoPath))
{
Remote remote = repo.Network.Remotes[remoteName];
IEnumerable<DirectReference> references = repo.Network.ListReferences(remote);
IEnumerable<Reference> references = repo.Network.ListReferences(remote).ToList();

var actualRefs = new List<Tuple<string,string>>();

foreach(DirectReference reference in references)
foreach(Reference reference in references)
{
Assert.NotNull(reference.CanonicalName);
Assert.NotNull(reference.Target);
actualRefs.Add(new Tuple<string, string>(reference.CanonicalName, reference.Target.Id.Sha));

var directReference = reference.ResolveToDirectReference();

Assert.NotNull(directReference.Target);
actualRefs.Add(new Tuple<string, string>(reference.CanonicalName, directReference.Target.Id.Sha));
}

Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs.Count, actualRefs.Count);
Assert.True(references.Single(reference => reference.CanonicalName == "HEAD") is SymbolicReference);
for (int i = 0; i < TestRemoteRefs.ExpectedRemoteRefs.Count; i++)
{
Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs[i].Item1, actualRefs[i].Item1);
Expand All @@ -123,9 +132,9 @@ public void CanListRemoteReferencesWithCredentials()

var references = repo.Network.ListReferences(remote, Constants.PrivateRepoCredentials);

foreach (var directReference in references)
foreach (var reference in references)
{
Assert.NotNull(directReference);
Assert.NotNull(reference);
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions LibGit2Sharp.Tests/PushFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,9 @@ public void CanForcePush()
private static void AssertRemoteHeadTipEquals(IRepository localRepo, string sha)
{
var remoteReferences = localRepo.Network.ListReferences(localRepo.Network.Remotes.Single());
DirectReference remoteHead = remoteReferences.Single(r => r.CanonicalName == "HEAD");
Reference remoteHead = remoteReferences.Single(r => r.CanonicalName == "HEAD");

Assert.Equal(sha, remoteHead.TargetIdentifier);
Assert.Equal(sha, remoteHead.ResolveToDirectReference().TargetIdentifier);
}

private void UpdateTheRemoteRepositoryWithANewCommit(string remoteRepoPath)
Expand Down
40 changes: 34 additions & 6 deletions LibGit2Sharp.Tests/RepositoryFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,7 @@ public void CanListRemoteReferencesWithCredentials()
InconclusiveIf(() => string.IsNullOrEmpty(Constants.PrivateRepoUrl),
"Populate Constants.PrivateRepo* to run this test");

IEnumerable<DirectReference> references = Repository.ListRemoteReferences(Constants.PrivateRepoUrl,
IEnumerable<Reference> references = Repository.ListRemoteReferences(Constants.PrivateRepoUrl,
Constants.PrivateRepoCredentials);

foreach (var reference in references)
Expand All @@ -694,24 +694,49 @@ public void CanListRemoteReferencesWithCredentials()
[InlineData("git://github.com/libgit2/TestGitRepository.git")]
public void CanListRemoteReferences(string url)
{
IEnumerable<DirectReference> references = Repository.ListRemoteReferences(url);
IEnumerable<Reference> references = Repository.ListRemoteReferences(url).ToList();

List<Tuple<string, string>> actualRefs = references.
Select(directRef => new Tuple<string, string>(directRef.CanonicalName, directRef.TargetIdentifier)).ToList();
Select(reference => new Tuple<string, string>(reference.CanonicalName, reference.ResolveToDirectReference().TargetIdentifier)).ToList();

Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs.Count, actualRefs.Count);
Assert.True(references.Single(reference => reference.CanonicalName == "HEAD") is SymbolicReference);
for (int i = 0; i < TestRemoteRefs.ExpectedRemoteRefs.Count; i++)
{
Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs[i].Item2, actualRefs[i].Item2);
Assert.Equal(TestRemoteRefs.ExpectedRemoteRefs[i].Item1, actualRefs[i].Item1);
}
}

[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);
}

[Theory]
[InlineData("http://github.com/libgit2/TestGitRepository")]
public void ReadingReferenceRepositoryThroughListRemoteReferencesThrows(string url)
{
IEnumerable<DirectReference> references = Repository.ListRemoteReferences(url);
IEnumerable<Reference> references = Repository.ListRemoteReferences(url);

foreach (var reference in references)
{
Expand All @@ -724,11 +749,14 @@ public void ReadingReferenceRepositoryThroughListRemoteReferencesThrows(string u
[InlineData("http://github.com/libgit2/TestGitRepository")]
public void ReadingReferenceTargetFromListRemoteReferencesThrows(string url)
{
IEnumerable<DirectReference> references = Repository.ListRemoteReferences(url);
IEnumerable<Reference> references = Repository.ListRemoteReferences(url);

foreach (var reference in references)
{
Assert.Throws<InvalidOperationException>(() => reference.Target);
Assert.Throws<InvalidOperationException>(() =>
{
var target = reference.ResolveToDirectReference().Target;
});
}
}
}
Expand Down
36 changes: 31 additions & 5 deletions LibGit2Sharp/Core/Proxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2147,7 +2147,7 @@ public static IList<string> git_remote_list(RepositorySafeHandle repo)
}
}

public static IEnumerable<DirectReference> git_remote_ls(Repository repository, RemoteSafeHandle remote)
public static IEnumerable<Reference> git_remote_ls(Repository repository, RemoteSafeHandle remote)
{
IntPtr heads;
UIntPtr count;
Expand All @@ -2162,26 +2162,52 @@ public static IEnumerable<DirectReference> git_remote_ls(Repository repository,
throw new OverflowException();
}

var refs = new List<DirectReference>();
var directRefs = 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 symRefTarget = LaxUtf8Marshaler.FromNative(remoteHead.SymRefTargetPtr);

// The name pointer should never be null - if it is,
// this indicates a bug somewhere (libgit2, server, etc).
if (remoteHead.NamePtr == IntPtr.Zero)
if (string.IsNullOrEmpty(name))
{
throw new InvalidOperationException("Not expecting null value for reference name.");
}

string name = LaxUtf8Marshaler.FromNative(remoteHead.NamePtr);
refs.Add(new DirectReference(name, repository, remoteHead.Oid));
if (!string.IsNullOrEmpty(symRefTarget))
{
symRefs.Add(name, symRefTarget);
}
else
{
directRefs.Add(name, new DirectReference(name, repository, remoteHead.Oid));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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))
{

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nulltoken Thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

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?

}

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

for (int i = 0; i < symRefs.Count; i++)
{
var symRef = symRefs.ElementAt(i);

if (!directRefs.ContainsKey(symRef.Value))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
throw new InvalidOperationException("Symbolic reference target not found in direct reference results.");
}

directRefs.Add(symRef.Key, new SymbolicReference(repository, symRef.Key, symRef.Value, directRefs[symRef.Value]));
}

var refs = directRefs.Values.ToList();
refs.Sort((r1, r2) => String.CompareOrdinal(r1.CanonicalName, r2.CanonicalName));

return refs;
}

Expand Down
10 changes: 5 additions & 5 deletions LibGit2Sharp/Network.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public virtual RemoteCollection Remotes
/// </summary>
/// <param name="remote">The <see cref="Remote"/> to list from.</param>
/// <returns>The references in the <see cref="Remote"/> repository.</returns>
public virtual IEnumerable<DirectReference> ListReferences(Remote remote)
public virtual IEnumerable<Reference> ListReferences(Remote remote)
{
Ensure.ArgumentNotNull(remote, "remote");

Expand All @@ -67,7 +67,7 @@ public virtual IEnumerable<DirectReference> ListReferences(Remote remote)
/// <param name="remote">The <see cref="Remote"/> to list from.</param>
/// <param name="credentialsProvider">The <see cref="Func{Credentials}"/> used to connect to remote repository.</param>
/// <returns>The references in the <see cref="Remote"/> repository.</returns>
public virtual IEnumerable<DirectReference> ListReferences(Remote remote, CredentialsHandler credentialsProvider)
public virtual IEnumerable<Reference> ListReferences(Remote remote, CredentialsHandler credentialsProvider)
{
Ensure.ArgumentNotNull(remote, "remote");
Ensure.ArgumentNotNull(credentialsProvider, "credentialsProvider");
Expand All @@ -86,7 +86,7 @@ public virtual IEnumerable<DirectReference> ListReferences(Remote remote, Creden
/// </summary>
/// <param name="url">The url to list from.</param>
/// <returns>The references in the remote repository.</returns>
public virtual IEnumerable<DirectReference> ListReferences(string url)
public virtual IEnumerable<Reference> ListReferences(string url)
{
Ensure.ArgumentNotNull(url, "url");

Expand All @@ -105,15 +105,15 @@ public virtual IEnumerable<DirectReference> ListReferences(string url)
/// <param name="url">The url to list from.</param>
/// <param name="credentialsProvider">The <see cref="Func{Credentials}"/> used to connect to remote repository.</param>
/// <returns>The references in the remote repository.</returns>
public virtual IEnumerable<DirectReference> ListReferences(string url, CredentialsHandler credentialsProvider)
public virtual IEnumerable<Reference> ListReferences(string url, CredentialsHandler credentialsProvider)
{
Ensure.ArgumentNotNull(url, "url");
Ensure.ArgumentNotNull(credentialsProvider, "credentialsProvider");

return ListReferencesInternal(url, credentialsProvider);
}

private IEnumerable<DirectReference> ListReferencesInternal(string url, CredentialsHandler credentialsProvider)
private IEnumerable<Reference> ListReferencesInternal(string url, CredentialsHandler credentialsProvider)
{
using (RemoteSafeHandle remoteHandle = BuildRemoteSafeHandle(repository.Handle, url))
{
Expand Down
4 changes: 2 additions & 2 deletions LibGit2Sharp/Repository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ internal Commit LookupCommit(string committish)
/// </para>
/// <param name="url">The url to list from.</param>
/// <returns>The references in the remote repository.</returns>
public static IEnumerable<DirectReference> ListRemoteReferences(string url)
public static IEnumerable<Reference> ListRemoteReferences(string url)
{
return ListRemoteReferences(url, null);
}
Expand All @@ -579,7 +579,7 @@ public static IEnumerable<DirectReference> ListRemoteReferences(string url)
/// <param name="url">The url to list from.</param>
/// <param name="credentialsProvider">The <see cref="Func{Credentials}"/> used to connect to remote repository.</param>
/// <returns>The references in the remote repository.</returns>
public static IEnumerable<DirectReference> ListRemoteReferences(string url, CredentialsHandler credentialsProvider)
public static IEnumerable<Reference> ListRemoteReferences(string url, CredentialsHandler credentialsProvider)
{
Ensure.ArgumentNotNull(url, "url");

Expand Down