Submodules! #312

Merged
merged 1 commit into from Apr 13, 2013

Conversation

Projects
None yet
8 participants
Member

dahlbyk commented Feb 4, 2013

SubmoduleCollection

  • this[name]
  • Hackish GetEnumerator()
  • Correct GetEnumerator() - need marshaling help
  • Add() - I'd suggest we tackle this with a follow-up PR

Submodule

  • Name, Path, Url
  • FetchRecurseSubmodules, Ignore, Update rules
  • Init(), Reload(), Save(), Sync()
  • RetrieveStatus()
  • Stage()

Index

  • af9c7974b52afe2969276d5670df6ff022a5ca0e
  • Stage(submodule) (see #220)

Tests

  • Works on my machine
  • Real tests

While I'm working on setting up proper tests, I thought I'd get this out for feedback...

@nulltoken nulltoken and 1 other commented on an outdated diff Feb 4, 2013

LibGit2Sharp.Tests/SubmoduleFixture.cs
+ [Theory]
+ [InlineData("libgit2")]
+ [InlineData("libgit2/")]
+ public void CanStageChangeInSubmoduleViaIndexStage(string submodulePath)
+ {
+ using (var repo = new Repository(libgit2SubmodulePath))
+ {
+ repo.Checkout("a8182d495d3cf9f29b3339db7d6320a680a84690");
+ }
+
+ using (var repo = new Repository(libgit2SharpPath))
+ {
+ var submodule = repo.Submodules[submodulePath];
+
+ var statusBefore = submodule.RetrieveStatus();
+ Assert.Equal(SubmoduleStatus.WorkDirModified, statusBefore & SubmoduleStatus.WorkDirModified);
@nulltoken

nulltoken Feb 4, 2013

Member

Wouldn't HasFlag() work here?

@dahlbyk

dahlbyk Feb 4, 2013

Member

Yes, but it's internal.

@dahlbyk

dahlbyk Feb 7, 2013

Member

There are other tests where we could use HasFlag() (or HasAny<>()) too. From a purity standpoint, they really don't belong as part of the public surface area of a Git library, but is that a good enough reason? Or there's the slippery slope of [InternalsVisibleTo], which would allow Core.Epoch and the public classes in Core.Compat to be hidden.

@nulltoken nulltoken commented on an outdated diff Feb 4, 2013

LibGit2Sharp/Core/Handles/SubmoduleSafeHandle.cs
@@ -0,0 +1,6 @@
+namespace LibGit2Sharp.Core.Handles
+{
+ internal class SubmoduleSafeHandle : NotOwnedSafeHandleBase
+ {
+ }
+}

@nulltoken nulltoken commented on an outdated diff Feb 4, 2013

LibGit2Sharp/Core/NativeMethods.cs
+ RepositorySafeHandle repo,
+ [MarshalAs(UnmanagedType.CustomMarshaler, MarshalTypeRef = typeof(Utf8Marshaler))] string name);
+
+ internal delegate int submodule_callback(
+ IntPtr sm,
+ IntPtr name,
+ IntPtr payload);
+
+ [DllImport(libgit2)]
+ internal static extern int git_submodule_foreach(
+ RepositorySafeHandle repo,
+ submodule_callback callback,
+ IntPtr payload);
+
+ [DllImport(libgit2)]
+ internal static extern int git_submodule_add_to_index(SubmoduleSafeHandle submodule, bool write_index);
@nulltoken

nulltoken Feb 4, 2013

Member

In order ot maintain style consistency, please left align the parameters

@nulltoken nulltoken and 2 others commented on an outdated diff Feb 4, 2013

LibGit2Sharp/Submodule.cs
+ /// The name of the submodule.
+ /// </summary>
+ public virtual string Name { get { return name; } }
+
+ /// <summary>
+ /// The name of the submodule.
+ /// </summary>
+ public virtual string Path { get { return lazyPath.Value; } }
+
+ /// <summary>
+ /// The URL of the submodule.
+ /// </summary>
+ public virtual string Url
+ {
+ get { return Proxy.git_submodule_url(handle); }
+ set { Proxy.git_submodule_set_url(handle, value); }
@nulltoken

nulltoken Feb 4, 2013

Member

I'd rather not expose a mutable type. Have you considered something similar to @yorah's proposal?

@dahlbyk

dahlbyk Feb 4, 2013

Member

I'll see what I can come up with.

@yorah

yorah Mar 25, 2013

Contributor

I think my other comment addresses the same concern.

@nulltoken nulltoken commented on an outdated diff Feb 4, 2013

LibGit2Sharp/SubmoduleCollection.cs
+
+ IEnumerator IEnumerable.GetEnumerator()
+ {
+ return GetEnumerator();
+ }
+
+ private string DebuggerDisplay
+ {
+ get
+ {
+ return string.Format(CultureInfo.InvariantCulture,
+ "Count = {0}", this.Count());
+ }
+ }
+ }
+}

@nulltoken nulltoken and 1 other commented on an outdated diff Feb 4, 2013

LibGit2Sharp/SubmoduleExistsException.cs
+ public SubmoduleExistsException(string message, Exception innerException)
+ : base(message, innerException)
+ {
+ }
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref = "SubmoduleExistsException" /> class with a serialized data.
+ /// </summary>
+ /// <param name = "info">The <see cref="SerializationInfo "/> that holds the serialized object data about the exception being thrown.</param>
+ /// <param name = "context">The <see cref="StreamingContext"/> that contains contextual information about the source or destination.</param>
+ protected SubmoduleExistsException(SerializationInfo info, StreamingContext context)
+ : base(info, context)
+ {
+ }
+ }
+}
@nulltoken

nulltoken Feb 4, 2013

Member

You don't like trailing newlines, don't you? ;-)

@dahlbyk

dahlbyk Feb 4, 2013

Member

I blame R#, will fix.

@nulltoken nulltoken and 1 other commented on an outdated diff Feb 4, 2013

LibGit2Sharp/Core/Proxy.cs
@@ -1663,6 +1663,144 @@ public static ICollection<TResult> git_status_foreach<TResult>(RepositorySafeHan
#endregion
+ #region git_submodule_
+
+ public static SubmoduleSafeHandle git_submodule_lookup(RepositorySafeHandle repo, string name)
+ {
+ using (ThreadAffinity())
+ {
+ SubmoduleSafeHandle reference;
+ var res = NativeMethods.git_submodule_lookup(out reference, repo, name);
+
+ switch (res)
+ {
+ case (int)GitErrorCode.NotFound:
+ case (int)GitErrorCode.OrphanedHead:
+ return new SubmoduleSafeHandle();
@nulltoken

nulltoken Feb 4, 2013

Member

That seems a bit strange...

@dahlbyk

dahlbyk Feb 4, 2013

Member

Yeah, probably makes more sense to just return null?

@dahlbyk

dahlbyk Feb 7, 2013

Member

On closer inspection, null is even worse, and it wouldn't be consistent with anywhere else in the project where we return handles.

It turns out git_submodule_lookup already sets reference to an invalid handle for an error-returning call - any objection to just returning that regardless of success or failure?

@nulltoken nulltoken and 2 others commented on an outdated diff Feb 4, 2013

LibGit2Sharp/Core/Proxy.cs
+ public static SubmoduleSafeHandle git_submodule_lookup(RepositorySafeHandle repo, string name)
+ {
+ using (ThreadAffinity())
+ {
+ SubmoduleSafeHandle reference;
+ var res = NativeMethods.git_submodule_lookup(out reference, repo, name);
+
+ switch (res)
+ {
+ case (int)GitErrorCode.NotFound:
+ case (int)GitErrorCode.OrphanedHead:
+ return new SubmoduleSafeHandle();
+
+ case (int)GitErrorCode.Exists:
+ // TODO: Add suggested resolution to message
+ throw new SubmoduleExistsException("A valid submodule '{0}' exists, but Git doesn't know about it.");
@nulltoken

nulltoken Feb 4, 2013

Member

SubmoduleExistsException sounds like the kind of exception one would throw when blindly overwriting an existing submodule.

How about Partial or IncompleteSubmoduleConfiguration?

@dahlbyk

dahlbyk Feb 4, 2013

Member

You're right, this is a terrible name... @arrbee what would you call it? I'm really not sure how a repo could even get in this state.

@arrbee

arrbee Feb 4, 2013

Member

So, this error means that you have a git repository inside your parent git repository, but there is no submodule information configured for it - i.e. you looked up a submodule (say "foo") and there is no configuration information for that submodule, but the submodule directory exists and contains a .git subdirectory (i.e. "parent/foo/.git" exists). It's a bit of an odd case but it would happen if you cloned into a parent and then tried to treat that clone as if it were a submodule without actually adding it.

It seems like this might be a misfeature and you could probably just treat this case the same as NOTFOUND (i.e. you tried to look up "xyz" as a submodule and it is not a submodule).

@dahlbyk

dahlbyk Feb 4, 2013

Member

It seems like this might be a misfeature and you could probably just treat this case the same as NOTFOUND (i.e. you tried to look up "xyz" as a submodule and it is not a submodule).

I'll run with this until we discover a compelling reason to do otherwise. Thanks!

@nulltoken nulltoken and 2 others commented on an outdated diff Feb 4, 2013

LibGit2Sharp/Submodule.cs
+ Proxy.git_submodule_init(handle, overwrite);
+ }
+
+ /// <summary>
+ /// Retrieves the state of this submodule in the working directory compared to the staging area and the latest commmit.
+ /// </summary>
+ /// <returns></returns>
+ public virtual SubmoduleStatus RetrieveStatus()
+ {
+ return Proxy.git_submodule_status(handle);
+ }
+
+ /// <summary>
+ /// Reread submodule info from config, index, and HEAD.
+ /// </summary>
+ public virtual void Reload()
@nulltoken

nulltoken Feb 4, 2013

Member

I'm not sure about exposing Reload(), Save() and Sync()

@dahlbyk

dahlbyk Feb 7, 2013

Member

I'm trying to bake these operations into options for the the "updater" mechanism.

@yorah

yorah Mar 25, 2013

Contributor

Shouldn't these methods be part of SubModuleCollection?

My feeling is that:

  • everything that modifies the superproject should be done through Repository.Submodules
  • a submodule instance should be immutable, as is a Reference

@nulltoken nulltoken commented on an outdated diff Feb 4, 2013

LibGit2Sharp.Tests/IndexFixture.cs
@@ -311,7 +311,7 @@ public void StageFileWithBadParamsThrows()
{
Assert.Throws<ArgumentException>(() => repo.Index.Stage(string.Empty));
Assert.Throws<ArgumentNullException>(() => repo.Index.Stage((string)null));
- Assert.Throws<ArgumentException>(() => repo.Index.Stage(new string[] { }));
+ Assert.DoesNotThrow(() => repo.Index.Stage(new string[] { }));
@nulltoken

nulltoken Feb 4, 2013

Member

I'm not sure I agree with this change. What makes this compelling?

@nulltoken nulltoken and 1 other commented on an outdated diff Feb 4, 2013

LibGit2Sharp.Tests/IndexFixture.cs
@@ -583,7 +583,7 @@ public void RemovingFileWithBadParamsThrows()
{
Assert.Throws<ArgumentException>(() => repo.Index.Remove(string.Empty));
Assert.Throws<ArgumentNullException>(() => repo.Index.Remove((string)null));
- Assert.Throws<ArgumentException>(() => repo.Index.Remove(new string[] { }));
+ Assert.DoesNotThrow(() => repo.Index.Remove(new string[] { }));
@nulltoken

nulltoken Feb 4, 2013

Member

I'm not sure I agree with this change. What makes this compelling?

@dahlbyk

dahlbyk Feb 4, 2013

Member

Honestly, it makes my submodule support in Index.Stage() easier. Without it, staging only a submodule doesn't work without more complex changes to PrepareBatch() to override the Count == 0 check that throws. Not difficult, just makes the flow of control less obvious.

From an API standpoint, it seems reasonable to me that passing empty would be a no-op - we'll just stage all 0 of the paths passed in. From a consumer standpoint, this also allows passing in a lazy enumeration without checking its length first, e.g. Stage(myPaths.Where(p => GetCheckBox(p).IsChecked)).

@nulltoken nulltoken and 1 other commented on an outdated diff Feb 4, 2013

LibGit2Sharp.Tests/SubmoduleFixture.cs
+using System;
+using System.IO;
+using System.Linq;
+using LibGit2Sharp.Tests.TestHelpers;
+using Xunit;
+using Xunit.Extensions;
+
+namespace LibGit2Sharp.Tests
+{
+ public class SubmoduleFixture : BaseFixture
+ {
+ private static readonly string libgit2SharpPath = @"C:\Dev\GitHub\libgit2sharp";
+ private static readonly string libgit2SubmodulePath = Path.Combine(libgit2SharpPath, @"libgit2");
+ private readonly string libgit2Head = File.ReadAllLines(Path.Combine(libgit2SharpPath, ".git/modules/libgit2/HEAD"))[0];
+
+ public override void Dispose()
@nulltoken

nulltoken Feb 4, 2013

Member

I think I'd prefer having the test repository BuildTemporaryCloneOfTestRepo()'d when it'll be modified by the test, rather than reverting the changes afterwards.

Moreover, this may prevent the tests from being run in parallel when xUnit 2.0 is released.

@dahlbyk

dahlbyk Feb 4, 2013

Member

Yeah, this setup/teardown is specifically so I could use my libgit2sharp clone as a sanity check. Real tests will work against temporarily clones, likely of a repo designed specifically for testing submodules like libgit2 uses.

@jamill jamill commented on an outdated diff Feb 7, 2013

LibGit2Sharp/SubmoduleCollection.cs
+ /// Gets the <see cref = "LibGit2Sharp.Branch" /> with the specified name.
+ /// </summary>
+ public virtual Submodule this[string name]
+ {
+ get
+ {
+ Ensure.ArgumentNotNullOrEmptyString(name, "name");
+
+ return Submodule.BuildFromPtr(repo.RegisterForCleanup(Proxy.git_submodule_lookup(repo.Handle, name)), name);
+ }
+ }
+
+ public IEnumerator<Submodule> GetEnumerator()
+ {
+ return Proxy.git_submodule_foreach(repo.Handle,
+ // TODO: h is a Submodule handle, but I can't figure out how to use it...
@jamill

jamill Feb 7, 2013

Member

I don't think you will be able to cast h to a SafeHandle derived type here in the callback - I have had problems with this before as well. I think you either need to do what you are doing here, or treat the submodule handle as an IntPtr. Maybe we should consider having NotOwnedSafeHandleBase not derive from SafeHandle (with updated naming), as we are not using any of the safe bits. Just a thought, I have not fully thought it through.

Member

dahlbyk commented Feb 18, 2013

New set of commits based on latest, with proper tests. First three commits could be merged early if they look good - nothing submodule specific.

Next up: immutability.

I'd appreciate some feedback on other scenarios everyone would like to see tests for.

Contributor

Yogu commented Feb 26, 2013

I'm using this fork in an evaluation project and it works quite well. I would appreciate it being merged :)

Contributor

Yogu commented Feb 26, 2013

However, I've got a question: is cloning repositories recursively already supported? When I clone a repository, and then call Init() for all the submodules, the submodule directories are created, but empty. How do I set up the .git files and the modules directory? What's the stategy to clone recursively?

Member

dahlbyk commented Feb 26, 2013

I would appreciate it being merged :)

@nulltoken How about I strip out the parts related to mutation for now and hit that as a separate PR? Enumeration, lookup and staging seem reasonably solid...

However, I've got a question: is cloning repositories recursively already supported? When I clone a repository, and then call Init() for all the submodules, the submodule directories are created, but empty. How do I set up the .git files and the modules directory? What's the stategy to clone recursively?

I've not tried using Init() yet - @arrbee?

@jamill jamill and 1 other commented on an outdated diff Feb 27, 2013

...dule_wd/not_submodule/dot_git/hooks/pre-rebase.sample
@@ -0,0 +1,169 @@
+#!/bin/sh
@jamill

jamill Feb 27, 2013

Member

Please make sure we have the proper permission to accept 3rd party code here (or remove the hooks if they are not necessary?).

@dahlbyk

dahlbyk Feb 27, 2013

Member

I'll kill them.

Related: libgit2/libgit2@fc6c5b5

@dahlbyk

dahlbyk Feb 27, 2013

Member

See 49ce35b (which can be cherry-picked standalone) and updated 69da28c

Member

dahlbyk commented Feb 27, 2013

I've broken the existing mutation members out into their own commit, though I've left all the bindings. If everything else looks satisfactory, I propose we close out this PR and continue with mutation and add/init/update in separate PRs.

Member

dahlbyk commented Mar 3, 2013

Another update on latest, with a new set of properties: HeadCommitId, IndexCommitId and WorkDirCommitId (test = dahlbyk/libgit2sharp@c81f423#L1R48).

Works on my machine, but Travis build is failing for the two tests that expect a null ObjectId. Just pushed a possible fix.

I believe this exposes everything one would need to manually clone/update a Submodule, though that seems like something we should include eventually.

Another question I had while learning the libgit2 APIs a bit better... Is there any way to tell if git_submodule_init needs to be called, other than manually inspecting config? git_submodule_status doesn't differentiate between config in .gitmodules or .git/config, returning GIT_SUBMODULE_STATUS_IN_CONFIG for both. /cc @arrbee

Member

dahlbyk commented Mar 3, 2013

Just pushed a possible fix.

Looks like d082a81 worked; rebased accordingly.

Contributor

KindDragon commented Mar 5, 2013

I think you should also support FileStatus Submodule, so we can detect submodules in Index.RetrieveStatus() like does git status

Contributor

KindDragon commented Mar 5, 2013

I found that git_status_foreach doesn't support that :-(

Member

arrbee commented Mar 5, 2013

Another question I had while learning the libgit2 APIs a bit better... Is there any way to tell if git_submodule_init needs to be called, other than manually inspecting config? git_submodule_status doesn't differentiate between config in .gitmodules or .git/config, returning GIT_SUBMODULE_STATUS_IN_CONFIG for both. /cc @arrbee

@dahlbyk Sounds like we should change that part of submodule status to make it clear. I know of an issue with that IN_CONFIG status for some corner cases of git_checkout already, so I'd like to change it so that I can fix those checkout cases. As you probably already know, the submodule APIs have not seen that much usage, so we'll probably have to evolve them...

I found that git_status_foreach doesn't support that :-(

@KindDragon I'm not quite sure what you're saying is not supported? If status finds a submodule, it should compare the SHA of the current HEAD of the submodule to the SHA in the HEAD of the current branch of the parent repository. That is the extent of what's supported in the current status implementation.

We can add flags to enable other behaviors if you can help me pin down what is needed. I can imagine a flag to check if any files in the submodule working directory are dirty (which is useful, but unfortunately somewhat expensive). Or a flag to include the submodule in the status results even if the quick is-HEAD-moved check appears unmodified, so that you can call the deeper git_submodule_status() API to check further on demand. I'm not sure exactly what you are looking for, but as I mentioned above, this facet of the library hasn't been exercised that much, so I'm very interested in feedback (and undoubtably, bug reports).

Contributor

KindDragon commented Mar 5, 2013

Repository.Index.RetrieveStatus() return all items including submodules, but there is no easy way to determine is current StatusEntry submodule.

Contributor

KindDragon commented Mar 14, 2013

@dahlbyk
How implement git submodule status --cached <submodule local path>? I just want to get commit hash.
Repository.Submodules[<path>].HeadCommitId?

Member

dahlbyk commented Mar 14, 2013

git submodule status --cached <path> would be Submodules[<path>].IndexCommitId
git submodule status <path> is Submodules[<path>].HeadCommitId

Contributor

KindDragon commented Mar 16, 2013

@nulltoken when you plan merge this pull request? We want submodules API for our project

Member

nulltoken commented Mar 16, 2013

@KindDragon It's next on my list. Will be done by end of next week.

Hi,

I would like to know what the status is of submodule add. Is this already being worked on? And would it be possible to add (and update/stage) a submodule without cloning it? I'm building some code which automatically generates files and stores them in a repository. This also needs to have some submodules included, but there is no need to have those submodules available at the location where the files are getting generated (as the repo will always be cloned and after the clone you of course need to run the submodule update --init). I guess that would be possible by only setting the submodules HEAD, right? And is this feature available in libgit2 and will this also make it into libgit2sharp?

Anyway, thanks for the great work on the library in general. It's just so easy to create and update a repository with it 😄

Contributor

KindDragon commented Mar 19, 2013

@nulltoken cool 👍

Member

arrbee commented Mar 19, 2013

@RobertMe writes:

I would like to know what the status is of submodule add.
...
And is this feature available in libgit2

At least at the libgit2 level, this is possible, but awkward. The git_submodule_add_to_index() API requires the submodule to be checked out because it looks at the current HEAD commit in the submodule to know what to add to the parent repository index. Behind the scenes, however, this API is just filling in a git_index_entry struct to describe the submodule HEAD and calling git_index_add() to update the parent repo, and you can do that yourself provided you know the commit id to which you want the parent to refer.

However, I don't think this functionality is available through libgit2sharp currently. You have to be able to call git_index_add() with a manually constructed git_index_entry and from what I can tell, there is no way to build the struct and call the underlying API at that low a level. Hopefully, one of the libgit2sharp gurus will correct me if I'm wrong about this. 😄

Member

dahlbyk commented Mar 19, 2013

Hopefully, one of the libgit2sharp gurus will correct me if I'm wrong about this. 😄

@RobertMe Ignoring a WIP commit that I'm just keeping around for my reference (not meant to be merged), this PR is focused on reading submodule status. Adding and modifying submodules will be added later.

The general approach in libgit2sharp (with a few exceptions, like ObjectDatabase) is to leave internals as internals and expose higher-level APIs. We would be more inclined to include repo.Submodules.Add(name, url, headCommitId) than expose the index entry manipulation capabilities so you can do it yourself.

@dahlbyk (and @arrbee) thank you for your answers :).

I've secretly already read a bit about the underlying libgit2 code and read it may be possible (by changing or "overloading" git_submodule_add_to_index). This was just a disguised feature request 😄
I also understand this PR is only meant for the reading the submodules, but I was just interested in whether work is already being done on the manipulation bit. And Submodules.Add(name, url, headCommitId) seems to be a nice solution indeed. Probably in combination with a submodule.Stage(headCommitId) to update it. Or maybe even just a setter to Submodule.HeadCommitId. As my guess would be libgit2 just reads the .git/HEAD file to set the commit id in the tree. So writing the specified commit to HEAD could already be enough to keep the normal (clone based) and this one the same API wise. From a user perspective you can than change the submodule (by commiting or pulling or, ... in the submodule, or setting HEAD manually) and stage it afterwards.

Member

dahlbyk commented Mar 19, 2013

I also understand this PR is only meant for the reading the submodules, but I was just interested in whether work is already being done on the manipulation bit.

Not by me (beyond the WIP commit). As I don't particularly need to add or update submodules, if someone that does would like to own the next step (or at least give API input) that would be encouraged.

Probably in combination with a submodule.Stage(headCommitId) to update it. Or maybe even just a setter to Submodule.HeadCommitId.

I expect we'll follow the BranchUpdater pattern, in which case I'd suggest a CommitId setter on the updater which will be written into the index if specified. Ideally (in my mind) that setter would effect a change that needs to be staged separately, but I assume WorkDirCommitId depends on having the submodule cloned?

Contributor

KindDragon commented Mar 20, 2013

Please also update libgit2 binaries to commit libgit2/libgit2@7dbf403, that fix some problems with submodules

dahlbyk closed this Mar 21, 2013

dahlbyk reopened this Mar 21, 2013

@yorah yorah and 1 other commented on an outdated diff Mar 25, 2013

LibGit2Sharp/Core/Proxy.cs
+ /// Returns a handle to the corresponding submodule,
+ /// or an invalid handle if a submodule is not found.
+ /// </summary>
+ public static SubmoduleSafeHandle git_submodule_lookup(RepositorySafeHandle repo, string name)
+ {
+ using (ThreadAffinity())
+ {
+ SubmoduleSafeHandle reference;
+ var res = NativeMethods.git_submodule_lookup(out reference, repo, name);
+
+ switch (res)
+ {
+ case (int)GitErrorCode.NotFound:
+ case (int)GitErrorCode.Exists:
+ case (int)GitErrorCode.OrphanedHead:
+ break;
@yorah

yorah Mar 25, 2013

Contributor

Would it be more explicit to return null here?

@dahlbyk

dahlbyk Mar 26, 2013

Member

Returning a null handle felt weird, but I guess we do it other places.

@yorah yorah and 1 other commented on an outdated diff Mar 25, 2013

...t2Sharp.Tests/TestHelpers/TemporaryCloneOfTestRepo.cs
var source = new DirectoryInfo(sourceDirectoryPath);
- if (Directory.Exists(Path.Combine(sourceDirectoryPath, ".git")))
- {
- // If there is a .git subfolder, we're dealing with a non-bare repo and we have to
- // copy the working folder as well
+ RepositoryPath = Path.Combine(scd.DirectoryPath, source.Name);
@yorah

yorah Mar 25, 2013

Contributor

[Comment related to the naming of the RepositoryPath property]

With this modification, we don't always point to a Repository path (i.e.: when cloning a standard repository, this property will point to the working directory, whereas when cloning a bare repository, this property will point to the repository).

What about a more generic name like Path?

@dahlbyk

dahlbyk Mar 26, 2013

Member

Good point - will switch to Path.

@dahlbyk

dahlbyk Mar 26, 2013

Member

On second thought, what if TemporaryCloneOfTestRepo just went away as an explicitly referenced class? If Path is the only useful property of a TemporaryCloneOfTestRepo, why not just return a string from BaseFixture.BuildTemporaryCloneOfTestRepo() (which could really be renamed to simply CloneForTest or something?

@dahlbyk

dahlbyk Mar 26, 2013

Member

(In which case I'm inclined to leave this patch as-is for now, and submit a follow-up PR with the larger cleanup effort.)

@yorah

yorah Mar 26, 2013

Contributor

CloneForTest

👍 !!!

@yorah yorah and 2 others commented on an outdated diff Mar 25, 2013

LibGit2Sharp/Core/Handles/OidSafeHandle.cs
@@ -6,7 +6,7 @@ internal class OidSafeHandle : NotOwnedSafeHandleBase
{
private GitOid? MarshalAsGitOid()
{
- return (GitOid?)Marshal.PtrToStructure(handle, typeof(GitOid));
+ return IsInvalid ? null : (GitOid?)Marshal.PtrToStructure(handle, typeof(GitOid));
@yorah

yorah Mar 25, 2013

Contributor

When would libgit2 return a zeroed oid?

Was this change triggered by a bug you encountered?

@dahlbyk

dahlbyk Mar 26, 2013

Member

Was this change triggered by a bug you encountered?

Yes, but I can't seem to reproduce it now... Discarding unless I can figure out why I thought it was necessary in the first place.

@yorah

yorah Mar 26, 2013

Contributor

(Following up on the comment you left on the commit).

You're right, it seems Mono has a different implementation. I opened a bug regarding this on the mono bug tracker.

Maybe we can just add a comment here to reference it, so we know this is a temporary (?) workaround.

/cc @joncham

@dahlbyk

dahlbyk Mar 26, 2013

Member

Open source: it works! 💖

@dahlbyk

dahlbyk Mar 26, 2013

Member

Including the comment since that commit will be GC'd at some point:

Now it all comes back... without this the Mono build fails:

LibGit2Sharp.Tests.SubmoduleFixture.CanRetrieveTheCommitIdsOfASubmodule(name: "sm_gitmodules_only", headId: null, indexId: null, workDirId: null): System.ArgumentNullException : Argument cannot be null.
Parameter name: src
  at (wrapper managed-to-native) System.Runtime.InteropServices.Marshal:PtrToStructure (intptr,System.Type)
  at LibGit2Sharp.Core.Handles.OidSafeHandle.MarshalAsGitOid ()
  at LibGit2Sharp.Core.Handles.OidSafeHandle.MarshalAsObjectId ()
  at LibGit2Sharp.Core.Proxy.git_submodule_head_id (LibGit2Sharp.Core.Handles.SubmoduleSafeHandle submodule)
  at LibGit2Sharp.Submodule.get_HeadCommitId ()
  at LibGit2Sharp.Tests.SubmoduleFixture.CanRetrieveTheCommitIdsOfASubmodule (System.String name, System.String headId, System.String indexId, System.String workDirId)

@yorah yorah and 1 other commented on an outdated diff Mar 25, 2013

LibGit2Sharp/Submodule.cs
+ {
+ return equalityHelper.GetHashCode(this);
+ }
+
+ /// <summary>
+ /// Returns the <see cref = "Name" />, a <see cref = "String" /> representation of the current <see cref = "Submodule" />.
+ /// </summary>
+ /// <returns>The <see cref = "Name" /> that represents the current <see cref = "Submodule" />.</returns>
+ public override string ToString()
+ {
+ return Name;
+ }
+
+ internal static Submodule BuildFromPtr(SubmoduleSafeHandle handle, string name)
+ {
+ return handle.IsInvalid ? null : new Submodule(handle, name);
@yorah

yorah Mar 25, 2013

Contributor

Maybe it would be better if we don't pass/store the handle to the Submodule. We could do a git_submodule_lookup with the name each time we want to access one of the properties, as we do in the Commit object (through the GitObjectLazyGroup)?

This would also allow to not keep a reference to the handle in the repository cleanup list.

@dahlbyk

dahlbyk Mar 26, 2013

Member

I guess I'm not sure what the advantage would be to not associating the handle with a Submodule instance?

@yorah

yorah Mar 26, 2013

Contributor

Accessing Repository.Submodules[string] returns a new instance of Submodule each time it is accessed. Most of the time, the client is only interested in the latest instance. However, in the current implementation, we are keeping a reference to the handle until the repository is disposed.
If we don't pass the handle, we don't have to keep a reference to it

Another reason is that I think Submodule should be immutable, like the other objects returned by LibGit2Sharp. The way it is currently done in the rest of the codebase is through the use of Lazy/GitObjectLazyGroup utility classes.
I make a parallel with Commit: we keep an instance of ObjectId, and rebuild a new instance of ObjectSafeWrapper (which in turns calls Proxy.git_object_lookup()) when we first access each property.
I think we can do the same with Submodule: instead of keeping an instance of ObjectId we can keep an instance of the name, and rebuild an xxxxWrapper (which will call Proxy.git_submodule_lookup()).

Also, slightly related: I wouldn't keep the Stage() method in the Submodule class. As this method acts on the superproject, I think what you did in Repository.Index is actually enough (although we would have to move the call to Proxy.git_submodule_add_to_index() there) => this is one less reason to keep the handle in Submodule I guess.

@nulltoken nulltoken and 1 other commented on an outdated diff Mar 25, 2013

LibGit2Sharp/Submodule.cs
+
+ private Submodule(SubmoduleSafeHandle handle, string name)
+ {
+ this.handle = handle;
+ this.name = name;
+
+ lazyPath = new Lazy<string>(() => Proxy.git_submodule_path(handle));
+ }
+
+ /// <summary>
+ /// The name of the submodule.
+ /// </summary>
+ public virtual string Name { get { return name; } }
+
+ /// <summary>
+ /// The name of the submodule.
@nulltoken

nulltoken Mar 25, 2013

Member

name or path?

@dahlbyk

dahlbyk Mar 26, 2013

Member

Definitely name. Shaking things up.

Member

nulltoken commented Mar 25, 2013

I concur with @yorah.

Moreover, going down the "snapshot" road, I think I'd prefer to not expose HeadCommitId, IndexId and HeadId as dynamic properties.

@nulltoken nulltoken and 1 other commented on an outdated diff Mar 25, 2013

LibGit2Sharp/Submodule.cs
+using System.Diagnostics;
+using System.Globalization;
+using LibGit2Sharp.Core;
+using LibGit2Sharp.Core.Compat;
+using LibGit2Sharp.Core.Handles;
+
+namespace LibGit2Sharp
+{
+ /// <summary>
+ /// A Submodule.
+ /// </summary>
+ [DebuggerDisplay("{DebuggerDisplay,nq}")]
+ public class Submodule : IEquatable<Submodule>
+ {
+ private static readonly LambdaEqualityHelper<Submodule> equalityHelper =
+ new LambdaEqualityHelper<Submodule>(x => x.Name);
@nulltoken

nulltoken Mar 25, 2013

Member

Nitpick: Two submodules from two different repos, bearing the same name, shouldn't be considered as equal, should they?

If a Submodule turns into a snapshot, maybe should we add the IndexCommitId, HeadCommitId, WorkdirCommitId as contributors to the equality definition.

@dahlbyk

dahlbyk Mar 26, 2013

Member

Nitpick: Two submodules from two different repos, bearing the same name, shouldn't be considered as equal, should they?

I suppose not, but I'm not entirely convinced it matters. Can we just document somewhere that "interaction between objects created from different Repository instances could yield unexpected results"?

Another "identity crisis" repro:

using (var repo = Repository.Init(BuildSelfCleaningDirectory().DirectoryPath, true))
using (var repo2 = Repository.Init(BuildSelfCleaningDirectory().DirectoryPath, true))
{
    Assert.NotNull(repo.Head);
    Assert.Equal(repo.Head, repo2.Head);
}
@dahlbyk

dahlbyk Mar 26, 2013

Member

That said, HeadCommitId can absolutely contribute to equality. Something feels wrong to me about IndexCommitId and WorkdirCommitId contributing to equality, but I can't put a finger on exactly what it is.

Contributor

yorah commented Mar 26, 2013

@dahlbyk 0662c27 is a spike related to the removal of handle from Submodule. I thought there would have been more changes, but the LazyGroup<T> was actually made to allow this kind of things!

If we don't like the added complexity of having lazy properties, another solution would be to evaluate the properties when we build the Submodule instance.

Another nitpick while I was at it: I added the Rule suffix to Ignore/Update/FetchRecurse properties (I was confused at first, as Update/Ignore looked like names related to actions that we want to perform).

Member

dahlbyk commented Mar 26, 2013

I like the direction you're going, though I don't like the change in semantics for SubmoduleCollection that the index getter will return an invalid Submodule instance given an incorrect name - I think we should still do an initial lookup (perhaps also fetching Path, Url, other properties?) before constructing the Submodule so that we can return null as appropriate.

I'll try to put together a rebound after work.

I added the Rule suffix...

👍

Member

dahlbyk commented Apr 8, 2013

@nulltoken Updated 3cffa1f with mention of the Mono issue, so it can probably be cherry-picked standalone.

@yorah I'm not a huge fan of how uses of git_submodule_lookup are scattered and don't handle null handles consistently... I'm open to ideas. Otherwise, I think this strikes a reasonable balance between reusing handles and avoiding extra work.

Any other feedback?

@yorah yorah commented on an outdated diff Apr 8, 2013

LibGit2Sharp/Core/SubmoduleLazyGroup.cs
+ protected override void EvaluateInternal(Action<SubmoduleSafeHandle> evaluator)
+ {
+ using (var handle = Proxy.git_submodule_lookup(repo.Handle, name))
+ {
+ if (handle == null)
+ {
+ throw new LibGit2SharpException(string.Format(
+ CultureInfo.InvariantCulture,
+ "Submodule lookup failed for '{0}'.", name));
+ }
+
+ evaluator(handle);
+ }
+ }
+ }
+}
@yorah

yorah Apr 8, 2013

Contributor

Oops, I think this missing newline originates from my spike 😊

@yorah yorah and 1 other commented on an outdated diff Apr 8, 2013

LibGit2Sharp/SubmoduleCollection.cs
+ return Proxy.git_submodule_foreach(repo.Handle, (h, n) => Utf8Marshaler.FromNative(n))
+ .Select(n => this[n])
+ .GetEnumerator();
+ }
+
+ /// <summary>
+ /// Returns an enumerator that iterates through the collection.
+ /// </summary>
+ /// <returns>An <see cref = "IEnumerator" /> object that can be used to iterate through the collection.</returns>
+ IEnumerator IEnumerable.GetEnumerator()
+ {
+ return GetEnumerator();
+ }
+
+ internal bool TryStage(string relativePath, bool writeIndex)
+ {
@yorah

yorah Apr 8, 2013

Contributor

Even if we are internal, maybe we could pass a Submodule here (so that we have one less direct call to Proxy.git_submodule_lookup)?
Your call.

@dahlbyk

dahlbyk Apr 8, 2013

Member

Passing a Submodule doesn't help because it doesn't have a handle.

Contributor

yorah commented Apr 8, 2013

Impressive PR, it's great to see Submodules coming to libgit2sharp!! 👏 👏

Member

dahlbyk commented Apr 8, 2013

I'm not a huge fan of how uses of git_submodule_lookup are scattered and don't handle null handles consistently...

How about cf574fd?

Member

dahlbyk commented Apr 9, 2013

Updated on latest. I left 5de5182 on its own for review, but it can be squashed into ac4a5f6 if you like the change.

Contributor

yorah commented Apr 9, 2013

Introduce SubmoduleCollection.Lookup()

Elegant solution!

Member

dahlbyk commented Apr 10, 2013

Updated to use new Stage() implementation from #343: TryStage() is now called from Index.AddToIndex().

Also new is 9b7252b, used in a new test that exercises staging a submodule along with other paths.

Member

dahlbyk commented Apr 10, 2013

Introduce SubmoduleCollection.Lookup()

Elegant solution!

Thanks! Squashed!

Member

nulltoken commented Apr 11, 2013

Wow! You did a fantastic job! 👍

I cherry-picked all of them but the last one.

@nulltoken nulltoken and 1 other commented on an outdated diff Apr 11, 2013

LibGit2Sharp/TreeEntry.cs
@@ -72,7 +72,7 @@ internal ObjectId TargetId
private GitObject RetrieveTreeEntryTarget()
{
- if (!Type.HasAny(new[]{GitObjectType.Tree, GitObjectType.Blob}))
+ if (!Type.HasAny(new[] { GitObjectType.Tree, GitObjectType.Blob, GitObjectType.Commit }))
@nulltoken

nulltoken Apr 11, 2013

Member

I agree that we should not throw in this case. However something bothers me.

Technically, this will allow the creation (few lines below) of a concrete Commit which won't mean anything in the super-project as it technically doesn't exist in this context.

How about adding a new GitLink type. This type would derive from GitObject and would be returned as such.

Provided we go down this road,I can think of some additional topics to cope with

  • Inspect any public method accepting a GitObject as a parameter and determine how they should behave when being passed a GitLink.
  • Decide if the GitLink type should be internal-only or public

Thoughts?

@dahlbyk

dahlbyk Apr 11, 2013

Member

I agree that returning a Commit that's not actually a Commit feels a bit strange. We could certainly handle GitObjectType.Commit as a special case here to return a GitLink, but I wonder if it would make more sense to return a new type from libgit2 (e.g. add GIT_OBJ_LINK to git_otype)?

@nulltoken

nulltoken Apr 12, 2013

Member

I wonder if it would make more sense to return a new type from libgit2 (e.g. add GIT_OBJ_LINK to git_otype)?

I have the feeling that this proposal may lead to some debate.

How about tweaking this now at the LibGit2Sharp level? When/if this leads to a new entry to git_otype in libgit2, this would later bubble up through failing tests.

/cc @carlosmn @arrbee

@dahlbyk

dahlbyk Apr 13, 2013

Member

I have the feeling that this proposal may lead to some debate.

You're probably right. I see that git ls-tree returns commit; diverging probably doesn't make sense.

@dahlbyk

dahlbyk Apr 13, 2013

Member

How about adding a new GitLink type. This type would derive from GitObject and would be returned as such.

21696b8

Provided we go down this road,I can think of some additional topics to cope with

  • Inspect any public method accepting a GitObject as a parameter and determine how they should behave when being passed a GitLink.
  • Decide if the GitLink type should be internal-only or public

Please have a look for yourself, but at a glance I didn't see anywhere that we publicly accept GitObject where we have special cases that depend on the type provided, presumably allowing libgit2 to determine which object types are acceptable.

It would be good to support TreeDefinition.Add(string targetTreeEntryPath, GitLink link) eventually.

@nulltoken

nulltoken Apr 13, 2013

Member

Please have a look for yourself, but at a glance I didn't see anywhere that we publicly accept GitObject where we have special cases that depend on the type provided, presumably allowing libgit2 to determine which object types are acceptable.

You're right. Thanks!

It would be good to support TreeDefinition.Add(string targetTreeEntryPath, GitLink link) eventually.

I strongly agree with you. However; I'm not sure about the API. How would the user create the GitLink in the first place? Beside this, I'm not sure it would be safe to let pick the path by himself.

I wonder if TreeDefinition.Add(Submodule module) would work? Though, I can still think of a corner case where the TreeDefinition would be used to not build a root Tree, but a subTree (which would result in having the path in the .gitmodules file and the path from the root Tree being different)

@dahlbyk

dahlbyk Apr 13, 2013

Member

How would the user create the GitLink in the first place?

Maybe ObjectDatabase.CreateGitLink(ObjectId objectId), similar to existing CreateBlob() and CreateTree()? It would just call the ctor, but the public doesn't need to know that.

I wonder if TreeDefinition.Add(Submodule module) would work?

That wouldn't really align with the other TreeDefinition APIs, IMO. Perhaps TreeEntryDefinition.From(Submodule module), but that's about as far as I think we'd want to let a high-level concept creep into this low-level API.

@dahlbyk

dahlbyk Apr 14, 2013

Member

To be continued: #393

nulltoken merged commit 21696b8 into libgit2:vNext Apr 13, 2013

1 check passed

default The Travis build passed
Details
Member

nulltoken commented Apr 13, 2013

@dahlbyk Merged!

Member

nulltoken commented Apr 13, 2013

🎱

dahlbyk deleted the dahlbyk:Submodule branch Apr 14, 2013

dahlbyk referenced this pull request Apr 14, 2013

Merged

GitLink support in TreeDefinition #393

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