Add List Remote References without creating a Repository #1065

psawey · 2015-06-02T02:05:13Z

As mentioned #985, used git_repository_new() from the native side to implement this. Also introduced new public class RemoteHead as a container for the results.

nulltoken · 2015-06-02T08:33:44Z

@psawey 🆒 ‼️

Some quick nitpicks:

I'm not sure I understand the value of the new RemoteHead type. Unless there's a compelling reason not to, I'd rather stick with returning an IEnumerable<DirectReference>
Instead of creating a new RemoteRepository type, couldn't we rather make the Repository type expose a new static method?

public static IEnumerable<DirectReference> ListRemoteReferences(
         string url, CredentialsHandler credentialsProvider);

nulltoken · 2015-06-02T08:34:22Z

LibGit2Sharp/Core/NativeMethods.cs

@@ -1625,6 +1625,9 @@ IntPtr data

        [DllImport(libgit2)]
        internal static extern int git_cherrypick(RepositorySafeHandle repo, GitObjectSafeHandle commit, GitCherryPickOptions options);
+
+        [DllImport(libgit2)]
+        internal static extern int git_repository_new(out RepositorySafeHandle repo);


Could you please move this next to the other git_repository_* declarations?

psawey · 2015-06-02T22:52:59Z

Agreed, static method on the Repository type makes more sense, will update with the suggested method.
With DirectReference having a dependency on IRepository, didn't see a clean way to abstract it out.
- Possibly could use another implementation of the abstract Reference type, handling the IRepository dependency internally, RemoteReference?
- Maybe using some kind of implementation of IRepository without needing a backing repo
- Stick w/ RemoteHead, possibliy w/ better naming

Thoughts?

nulltoken · 2015-06-03T10:40:20Z

@psawey Duh. I forgot about the IRepository 😕

#806 brought IBelongToArepository which, in this case, gets in the middle and brings no value (The concrete repositoy being remote).

How about:

Modifying Reference so that it accepts a null IRepository
Decorate the Repository property with a comment explaining that null will be returned when the Reference has been generated through a git remote ls.

Another option may be to kill IBelongToArepository... 😈

/cc @SimonCropp @dahlbyk

dahlbyk · 2015-06-03T12:21:56Z

Could Reference.Repository throw if it's accessed while null?

psawey · 2015-06-04T22:25:03Z

The solution I'm looking at has Reference.Repository as null, but DirectReference.Target is also null as GitObject depends on IRepository. Acceptable for DirectReference.Target to be null when generated via git remote ls?

psawey · 2015-06-05T00:41:48Z

@dahlbyk It would, may be better to throw something useful in these cases.

nulltoken · 2015-06-05T05:44:38Z

Acceptable for DirectReference.Target to be null when generated via git remote ls?

👍

dahlbyk · 2015-06-05T06:00:43Z

I would prefer an exception for remote DirectReference.Target as well.

It occurs to me that we don't support remote symbolic refs (HEAD).

nulltoken · 2015-06-05T06:21:08Z

I would prefer an exception for remote DirectReference.Target as well.

Currently, If I hack a ref with the command-line making it point to a non existing Oid, then load the ref through LibGit2Sharp, it doesn't throw. It returns null. Why wouldn't we keep the same behavior?

It occurs to me that we don't support remote symbolic refs (HEAD).

Is that a thing git.git really supports? (/cc @carlosmn)

$ git ls-remote origin | grep HEAD
7202df7e971c1cbb32a7e26236064cde326a4e5a        HEAD

dahlbyk · 2015-06-05T08:34:38Z

Why wouldn't we keep the same behavior?

Because in the "anonymous" repo situation it's strictly unnecessary, even incorrect, to use that property. There's nothing local to target. No need to conflate "you need to fetch" with "you cannot fetch".

Is that a thing git.git really supports?

Yup, try cat .git/refs/remotes/origin/HEAD. See also git remote set-head and git_remote_head.

nulltoken · 2015-06-05T09:28:52Z

Is that a thing git.git really supports?

Yup, try cat .git/refs/remotes/origin/HEAD. See also git remote set-head and git_remote_head.

Sorry, I was unclear. Does git remote-ls support this? Meaning, is there a repo where a call to git remote-ls returns something that looks like a symbolic ref (ie. the HEAD target being something else that an oid?)

Why wouldn't we keep the same behavior?

Because in the "anonymous" repo situation it's strictly unnecessary, even incorrect, to use that property. There's nothing local to target. No need to conflate "you need to fetch" with "you cannot fetch".

Ok. Sounds reasonable. But you'll have to provide the exception message to be used 😉

dahlbyk · 2015-06-05T14:36:57Z

Does git remote-ls support this? Meaning, is there a repo where a call to git remote-ls returns something that looks like a symbolic ref (ie. the HEAD target being something else that an oid?)

git remote set-head implies that the information is available through the protocol:

With -a or --auto, the remote is queried to determine its HEAD, then the symbolic-ref refs/remotes/<name>/HEAD is set to the same branch…

InvalidOperationException: Target|Repository requires a local repository.

nulltoken · 2015-06-05T15:08:29Z

@psawey Could you please update your PR to reflect what has been discussed above?

Regarding symbolic references handling, I'd rather keep that as a potential enhancement which would be dealt with in a separate PR.

nulltoken · 2015-06-05T15:11:45Z

git remote set-head implies that the information is available through the protocol:

With -a or --auto, the remote is queried to determine its HEAD, then the symbolic-ref refs/remotes/<name>/HEAD is set to the same branch…

@carlosmn Does this rely on some magic guessing (finding a branch that points to the same oid than HEAD).

carlosmn · 2015-06-05T18:58:03Z

@carlosmn Does this rely on some magic guessing (finding a branch that points to the same oid than HEAD).

It does sometimes. The Git Smart Protocol lacked a way specify what the default branch actually was until fairly recently. What you'd do instead is get the id for HEAD and have to guess which one was the default branch. In case of tie, the first one wins, unless the master branch is in there, which always wins[0].

In order to fix this lack of control over which branch actually shows up when somebody clones, the protocol learnt a new extension, which lists the actual targets of the symrefs. As this is an extension, only newer gits understand this, so IIRC Ubuntu LTS ships with a version of git which does not understand this.

We do understand this extension, and fill in the information in git_remote_head. Often this will only be HEAD but any number of refs could be symrefs.

IMO trying to make this fit into a DirectReference is a false equivalence. We use this type to represent references in the repository, but what we're getting here is information about some other repository's references. There should be no expectation that any of the methods associated with references should work, it feels like shoving it to try to make it simpler, but it just ends up making it harder to map the concept to the type.

[0] This is in fact one of the very few places where 'master' is significantly relevant for git.

dahlbyk · 2015-06-06T02:54:45Z

Well really it should be shoving into a Reference, which is usually a DirectReference but may be a SymbolicReference. As @nulltoken suggests, I'm fine deferring a switch from DirectReference to Reference until later since IEnumerable<> is covariant.

The other option is a new RemoteReference hierarchy (supporting base, direct and symbolic), but it would only be used in the repository-less method call. Or I suppose it could also be returned by Network.ListReferences(Remote), since there are remote-specific concerns like "local oid" vs "remote oid" pre-fetch. Regardless, we still run into "belongs to a repository, so a target is resolvable" vs "no local objects to target" unless there's no relation between the two. I'm not sure two properties throwing is a great reason to increase our maintenance effort, as long as we document the (pretty obvious) "limitation".

psawey · 2015-06-06T18:23:20Z

I've updated the PR with all suggestions above.

Modifying Reference so that it accepts a null IRepository

Could Reference.Repository throw if it's accessed while null?

I would prefer an exception for remote DirectReference.Target as well.

Well really it should be shoving into a Reference

Returning a Reference with a DirectReference backing does prevent casual access to the invalid DirectReference.Target and IBelongToARepository.Repository properties without an explicit cast to either type, though they do throw if accessed. Seems like a good middle ground without introducing a new public RemoteReference type.

psawey · 2015-06-06T18:36:32Z

@nulltoken @dahlbyk

Would it make sense to use the same null functionality from Network ListReferences in case of a git remote ls?

        /// When the remote tips are ahead of the local ones, the retrieved
        /// <see cref="DirectReference"/>s may point to non existing
        /// <see cref="GitObject"/>s in the local repository. In that
        /// case, <see cref="DirectReference.Target"/> will return <c>null</c>.
        /// </para>
        /// </summary>
        /// <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)
        {

dahlbyk · 2015-06-07T00:18:53Z

LibGit2Sharp/RemoteHead.cs

+    /// <summary>
+    /// Reference to a Remote Head
+    /// </summary>
+    public class RemoteHead


dahlbyk · 2015-06-07T00:23:37Z

Would it make sense to use the same null functionality from Network ListReferences in case of a git remote ls?

I think they're both fine as currently implemented. There's a useful distinction to me between unfetched (null) and couldn't-have-been fetched (missing repo).

Revised API and tests look great to me.

psawey · 2015-06-07T01:20:54Z

Whoops, yes, has been removed!

nulltoken · 2015-06-09T08:40:23Z

LibGit2Sharp/Reference.cs

+            get
+            {
+                if (repo == null)
+                    throw new InvalidOperationException("Repository requires a local repository");


Could you please wrap this in braces?

psawey · 2015-06-09T22:28:55Z

@nulltoken 👌

nulltoken · 2015-06-10T06:11:30Z

@psawey I'm very eager to see this merged! 👍

Last request: We tend to not merge review commits. Now that the review is done, could you please simplify the history and squash all those into one cohesive commit?

…tory

psawey · 2015-06-10T14:41:07Z

@nulltoken Sure thing, will know for next time!

Add List Remote References without creating a Repository

nulltoken · 2015-06-10T16:48:48Z

@psawey Amazing contribution! A thousand thanks for it! ✨ ✨ ✨

nulltoken reviewed Jun 2, 2015
View reviewed changes

dahlbyk reviewed Jun 7, 2015
View reviewed changes

LibGit2Sharp/RemoteHead.cs

/// <summary>

/// Reference to a Remote Head

/// </summary>

public class RemoteHead

Copy link

Member

dahlbyk Jun 7, 2015

Choose a reason for hiding this comment

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

Drop this?

nulltoken reviewed Jun 9, 2015
View reviewed changes

nulltoken mentioned this pull request Jun 9, 2015

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

Closed

Add Functionality to List Remote References without creating a Reposi…

12c4f9b

…tory

psawey force-pushed the remotels branch from 29d861f to 12c4f9b Compare June 10, 2015 14:39

nulltoken added a commit that referenced this pull request Jun 10, 2015

Merge pull request #1065 from psawey/remotels

62c0763

Add List Remote References without creating a Repository

nulltoken merged commit 62c0763 into libgit2:vNext Jun 10, 2015

nulltoken added this to the v0.22 milestone Jun 10, 2015

This was referenced Jul 12, 2015

Expose a way to list remote references without having to go through a repository instance #985

Closed

Remote LS able to handle Symbolic References #1132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add List Remote References without creating a Repository #1065

Add List Remote References without creating a Repository #1065

psawey commented Jun 2, 2015

nulltoken commented Jun 2, 2015

nulltoken Jun 2, 2015

psawey commented Jun 2, 2015

nulltoken commented Jun 3, 2015

dahlbyk commented Jun 3, 2015

psawey commented Jun 4, 2015

psawey commented Jun 5, 2015

nulltoken commented Jun 5, 2015

dahlbyk commented Jun 5, 2015

nulltoken commented Jun 5, 2015

dahlbyk commented Jun 5, 2015

nulltoken commented Jun 5, 2015

dahlbyk commented Jun 5, 2015

nulltoken commented Jun 5, 2015

nulltoken commented Jun 5, 2015

carlosmn commented Jun 5, 2015

dahlbyk commented Jun 6, 2015

psawey commented Jun 6, 2015

psawey commented Jun 6, 2015

dahlbyk Jun 7, 2015

dahlbyk commented Jun 7, 2015

psawey commented Jun 7, 2015

nulltoken Jun 9, 2015

psawey commented Jun 9, 2015

nulltoken commented Jun 10, 2015

psawey commented Jun 10, 2015

nulltoken commented Jun 10, 2015

Add List Remote References without creating a Repository #1065

Add List Remote References without creating a Repository #1065

Conversation

psawey commented Jun 2, 2015

nulltoken commented Jun 2, 2015

nulltoken Jun 2, 2015

Choose a reason for hiding this comment

psawey commented Jun 2, 2015

nulltoken commented Jun 3, 2015

dahlbyk commented Jun 3, 2015

psawey commented Jun 4, 2015

psawey commented Jun 5, 2015

nulltoken commented Jun 5, 2015

dahlbyk commented Jun 5, 2015

nulltoken commented Jun 5, 2015

dahlbyk commented Jun 5, 2015

nulltoken commented Jun 5, 2015

dahlbyk commented Jun 5, 2015

nulltoken commented Jun 5, 2015

nulltoken commented Jun 5, 2015

carlosmn commented Jun 5, 2015

dahlbyk commented Jun 6, 2015

psawey commented Jun 6, 2015

psawey commented Jun 6, 2015

dahlbyk Jun 7, 2015

Choose a reason for hiding this comment

dahlbyk commented Jun 7, 2015

psawey commented Jun 7, 2015

nulltoken Jun 9, 2015

Choose a reason for hiding this comment

psawey commented Jun 9, 2015

nulltoken commented Jun 10, 2015

psawey commented Jun 10, 2015

nulltoken commented Jun 10, 2015