Skip to content

GitLink support in TreeDefinition #393

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.

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 5 commits into from
Apr 24, 2013
Merged

GitLink support in TreeDefinition #393

merged 5 commits into from
Apr 24, 2013

Conversation

dahlbyk
Copy link
Member

@dahlbyk dahlbyk commented Apr 14, 2013

Following on from this discussion:

  • Bit of somewhat-related cleanup
  • Add(string targetTreeEntryPath, GitLink gitLink)
  • Test TreeDefinition behavior with GitLinks
  • Add(string targetTreeEntryPath, ObjectId objectId) so that ObjectDatabaseFixture.CanCreateATreeContainingAGitLinkFromAnObjectId (seems equivalent to creating Blob from relative path)

Thoughts?

@dahlbyk dahlbyk mentioned this pull request Apr 14, 2013
13 tasks
@nulltoken
Copy link
Member

I'm not a huge fan of exposing the GitLink as a property of a Submodule.

You proposed earlier GitLink gl = TreeEntryDefinition.From(Submodule module) which seems to me more appropriate (at least from a semantic standpoint).

@nulltoken
Copy link
Member

I was thinking that we could introduce a new property on the TreeEntry and TreeEntryDefinition types: TargetType.

Instead of returning a GitObjectType, we could return a TreeEntryType (Tag, Tree, Blob, GitLink). The existing Target property would be marked as deprecated.

@nulltoken
Copy link
Member

I'm not a huge fan of exposing the GitLink as a property of a Submodule.

You proposed earlier GitLink gl = TreeEntryDefinition.From(Submodule module) which seems to me more appropriate (at least from a semantic standpoint).

Or maybe directly TreeEntryDefinition ted = TreeEntryDefinition.From(Submodule module).I think I may even prefer this last one.

@dahlbyk
Copy link
Member Author

dahlbyk commented Apr 14, 2013

I'm not a huge fan of exposing the GitLink as a property of a Submodule.

You proposed earlier GitLink gl = TreeEntryDefinition.From(Submodule module) which seems to me more appropriate (at least from a semantic standpoint).

Or maybe directly TreeEntryDefinition ted = TreeEntryDefinition.From(Submodule module).I think I may even prefer this last one.

The TreeEntryDefinition.From() overloads are all internal; so exposing this one to allow defining a Submodule doesn't feel right.

Just as many consumers of a Commit won't access its Tree, most users of a Submodule have no need for a GitLink. If we want the TreeDefinition API to align with Git internals then I think we should expose it somewhere. In that context, as we have Commit.Tree I propose Submodule.GitLink.

Or perhaps we abandon that method of constructing a GitLink altogether and only provide ObjectDatabase.CreateGitLink(ObjectId objectId) (which I just pushed, along with a test).

{
const string submodulePath = "sm_added_and_uncommited";

/* Unexpected behavior: test fails if this is uncommented... libgit2 caching?
Copy link
Member

Choose a reason for hiding this comment

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

libgit2 caching

Looks like so. See here and here

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we always proactively reload before doing anything else with the handle in SubmoduleCollection.Lookup().

Copy link
Member

Choose a reason for hiding this comment

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

Let's try this. If this leads to performance issues, we'll try to think of an alternative.

@nulltoken
Copy link
Member

Or perhaps we abandon that method of constructing a GitLink altogether and only provide ObjectDatabase.CreateGitLink(ObjectId objectId) (which I just pushed, along with a test).

Although this is a very tempting proposal, I'm a bit bothered by what it conveys. A GitLink is not a GitObject and is not persisted as such in the object database. The API would be a a bit misleading.

From what I understand, a GitLink is nothing more than a TreeEntry. How about adding a new Add() overload to TreeDefinition and making it accept a Submodule?

The path and the ObjectId would be valued behind the scene from the Submodule.Path and Submodule.HeadCommitId properties.

@dahlbyk
Copy link
Member Author

dahlbyk commented Apr 16, 2013

Although this is a very tempting proposal, I'm a bit bothered by what it conveys. A GitLink is not a GitObject and is not persisted as such in the object database. The API would be a a bit misleading.

Note that GitLink currently inherits from GitObject.

How about TreeDefinition.AddGitLink(string path, ObjectId objectId) and TreeDefinition.Add(Submodule submodule)?

@nulltoken
Copy link
Member

Note that GitLink currently inherits from GitObject

I know. But I can't find a better hack 😞

How about TreeDefinition.AddGitLink(string path, ObjectId objectId) and TreeDefinition.Add(Submodule submodule)?

👍 ‼️

@dahlbyk
Copy link
Member Author

dahlbyk commented Apr 24, 2013

Tough to test 7ff7168 explicitly since Submodule mutation is not yet implemented, but it does fix the unexpected behavior in ObjectDatabaseFixture.

Any other feedback before I rebase on latest?

@nulltoken
Copy link
Member

Any other feedback before I rebase on latest?

Yes: "You're awesome! Let's get this merged!"

BTW, although this should certainly be handled through another PR, have you had the time to think about this proposal?

@dahlbyk
Copy link
Member Author

dahlbyk commented Apr 24, 2013

Rebased!

BTW, although this should certainly be handled through another PR, have you had the time to think about this proposal?

I'm definitely on board with killing TreeEntryDefinition.Target. I'm not sure what we would really gain from introducing TreeEntryType.

@nulltoken
Copy link
Member

I'm definitely on board with killing TreeEntryDefinition.Target.

Grmpf. Typo. I was meaning to deprecate Type in favor of TargetType.

I'm not sure what we would really gain from introducing TreeEntryType.

Well, instead of checking against a GitObjectType.Commit, we could rather leverage TreeEntryType.GitLink, which may be more meaningful.

Beside this, it would avoid us exposing all the not-very-useful-for-a-user entries of GitObjectType.

@nulltoken nulltoken merged commit d5c4a44 into libgit2:vNext Apr 24, 2013
@nulltoken
Copy link
Member

Rebased!

Merged! 💎

@dahlbyk dahlbyk deleted the GitLink branch April 25, 2013 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants