-
Notifications
You must be signed in to change notification settings - Fork 899
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
Conversation
I'm not a huge fan of exposing the You proposed earlier |
I was thinking that we could introduce a new property on the Instead of returning a |
Or maybe directly |
The Just as many consumers of a Or perhaps we abandon that method of constructing a |
{ | ||
const string submodulePath = "sm_added_and_uncommited"; | ||
|
||
/* Unexpected behavior: test fails if this is uncommented... libgit2 caching? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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()
.
There was a problem hiding this comment.
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.
Although this is a very tempting proposal, I'm a bit bothered by what it conveys. A From what I understand, a The path and the ObjectId would be valued behind the scene from the |
Note that How about |
I know. But I can't find a better hack 😞
👍 |
Tough to test 7ff7168 explicitly since 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? |
Rebased!
I'm definitely on board with killing |
Grmpf. Typo. I was meaning to deprecate
Well, instead of checking against a Beside this, it would avoid us exposing all the not-very-useful-for-a-user entries of |
Merged! 💎 |
Following on from this discussion:
Add(string targetTreeEntryPath, GitLink gitLink)
TreeDefinition
behavior withGitLink
sAdd(string targetTreeEntryPath, ObjectId objectId)
so thatObjectDatabaseFixture.CanCreateATreeContainingAGitLinkFromAnObjectId
(seems equivalent to creatingBlob
from relative path)Thoughts?