Those aren't libgit2 names, but git names. libgit2 uses read-tree and write-tree because that's what the plumbing command is called which does precisely this.

@carlosmn I'm sorry, I didn't mean to complain about the libgit2 names. I just tried to choose different ones because I thought an Index.WriteTree that does not mutate the index and an Index.ReadTree that modifies the index would be confusing for C# developers: comparing with Stream, for example, where Stream.Read does not mutate the stream and Stream.Write does.

Stream.Read does mutate the stream, it just does it in a different way to Stream.Write.

My point was that these are the names of basic Git operations and exposing them with different names would add confusion for someone who is looking at it from the Git perspective.

I personally had to read the libgit2 methods description before I could tell which one does what; I could not tell at once based on their names. But I agree, they do what their names say and they are 1:1 mappings to the git core commands.

I think you've done a very good job with the libgit2Sharp API so far, and I don't have any attachment to the names, should I rename the two methods to Index.WriteTree and Index.ReadTree ?

Thank you!

I tried to stay away from libgit2 names, as they were confusing for me at first, so I thought to expose git_index_write_tree as Index.CreateTree and git_index_read_tree as Index.ReplaceWithTree

Actually, I'm not sure that those methods should be exposed by the Index.

For instance, rather than Index.CreateTree(), maybe a TreeDefinition.From(index) overload may make sense. This would allow the user to potentially manipulate the generated TreeDefinition. The Tree could then be created through ObjectDatabase.CreateTree(TreeDefinition).

I haven't a proposal yet regarding how to expose the replacement of the Index from a Tree in such a manner that the newbie user won't shoot himself in the foot by mistake.

In my project I need to be able to read/write the index from/to a tree

Getting a glimpse of the "end-user need" may help us design the API in a cleaner way (or identify existing features which could solve your requirements). Could you please share a little bit about the higher level scenarios that led you to implement such features?

Other than that, your PR looks a very nice piece of work ⚡. Thanks a lot for having taken the time to match the general code style ❤️

For instance, rather than Index.CreateTree(), maybe a TreeDefinition.From(index) overload may make sense. This would allow the user to potentially manipulate the generated TreeDefinition. The Tree could then be created through ObjectDatabase.CreateTree(TreeDefinition).

It's easy enough to create a TreeDefinition from a Tree... ObjectDatabase.CreateTree(Index) seems to more accurately reflect what's happening, i.e. a Tree would be created to populate the TreeDefinition even if the resulting definition was never passed to CreateTree().

ObjectDatabase.CreateTree(Index) seems to more accurately reflect what's happening

Sold! 👍

ObjectDatabase.CreateTree(Index) is brilliant! :) I can make the change once we settle also on the other method.

One scenario (as silly as it sounds :)) that I need to implement is a logical, incremental backup tool for git repositories. Let's say I've taken care of backing-up the objects database incrementally and now I need to backup the index incrementally. I can probably accomplish my goal by iterating through the index entries when creating the backup and build some tree data-structure from it. I can probably do the same when restoring a backup, by doing some sort of delta-logic in C#, by looking at the entries in the index, figuring out which ones are OK and which ones I should modify.

However, in reality, I need to quickly figure out if the index has changed since my last backup, then save the entire index and have the ability to restore it. Since I've got the backing up code for the objects database working, in my scenario it would work out great if the index's tree would be there too. I might be biased because I found them, but git_index_read_tree and git_index_write_tree seem to me, at least at this time, the best methods to get my scenario working. I might need in the future to get more fancy than just restoring the full index and allow for index tree diffing, etc. but not at this time.

I share your concern with the placement of the methods. For the other method, git_index_read_tree, since the index is a property on the Repository (which is pretty cool and makes a lot sense ;)), it seems that modifying the index should either be done on the Repository or on the Index. Thus, the following options come to my head:

• Repository.Index = tree (this might require some C# voo-doo, perhaps we'd be able to use implicit type conversions and a setter?)
• Repository.ReplaceIndexOrSomeOtherMethodName(tree)
• Index.ReplaceOrSomeOtherMethodName(tree)

Thanks again for your time & for libgit2Sharp, which I really think has a great API.

I'd hate to ruin it with my scenario, but at the same time I really want to use the nice API and not have to resort to exec git core or do some reflection hacks in my code.

However, in reality, I need to quickly figure out if the index has changed since my last backup, then save the entire index and have the ability to restore it

Reading a tree into the index will not restore the original index, but will replace the entries and all the auxiliary information will be lost. It will also not store entries in higher stages, which means you'd lose any chance of restoring an in-progress merge.

And that's assuming you have created a tree out of an in-progress merge, which means you've thrown away the merge information. This is why we don't let you perform write-tree on an index with higher-staged entries.

The index is a completely different data structure to a tree. You cannot pretend one is the other without losing information.

Here's a customer facing issue we have.

Whenever we make a commit with GHfW we need to explicitly unstage everything first and then stage the selected files. We were doing that using a mixed reset but unfortunately a mixed reset will clear MERGE_HEAD and related metadata files needed to create a merge commit.

We solved that by going one step lower and using read-tree to populate the index with the tree from HEAD.

I can't remember the exact reason why simply calling Unstage didn't work. @niik do you recall? I don't care if we implement a ReadTree method or not. But I'd like for this to work.

Also, we want to be able to unstage files added to a newly initialized repository that doesn't have any commits yet. The Reset method requires a SHA I believe. We can't do the equivalent of git reset

Whenever we make a commit with GHfW we need to explicitly unstage everything first and then stage the selected files.

Maybe this is obvious to GUI people, but... why? If it's in order to only commit a few files, it sounds like you're coming at it from the wrong side. While there is the index, you can still use other indices, even with git, to prepare commits.

Have you considered using the libgit2sharp equivalent to this (TreeDefinition) to create a new tree to commit? I'm not sure if it's possible to create a commit from an arbitrary tree with libgit2sharp, but it should be.

I can't remember the exact reason why simply calling Unstage didn't work

This is something that should definitely be fixed then. Though an issue with "unstage" and "reset" is that it's rather context-dependent and it can do different things depending on how and when you call it.

Have you considered using the libgit2sharp equivalent to this (TreeDefinition) to create a new tree to commit? I'm not sure if it's possible to create a commit from an arbitrary tree with libgit2sharp, but it should be.

That can be done very easily.

I can't remember the exact reason why simply calling Unstage didn't work

This is something that should definitely be fixed then. Though an issue with "unstage" and "reset" is that it's rather context-dependent and it can do different things depending on how and when you call it.

@haacked @niik Do you think you could produce a failing test? We'd be happy to fix that.

Maybe this is obvious to GUI people, but... why? If it's in order to only commit a few files

Yep, that's why. The user can check the set of files to commit.

Have you considered using the libgit2sharp equivalent to this (TreeDefinition) to create a new tree to commit?

I have not! Got a code sample of doing that?

I don't know if we have something turorialy, but the tests show some of the basic methods. You basically go

var td = TreeDefinition.From(someCommitOrTree);
td.Add("some/path", someBlob, theMode);
td.Remove("file/i/dont/want");

Forgive my ignorance, but how do I create a commit with that. Here's the signature of the Commit method: Commit Commit(ICollection<string> pathsToCommit, string commitMessage)

Maybe a better question to ask is what do I pass to someCommitOrTree if I'm trying to create a commit? Do I create an empty commit, then create the TreeDefinition passing in the commit sha, and then add files to it?

@haacked Take a look at the tests in ObjectDatabaseFixture.cs. Some of them demonstrate how to create a Commit from a TreeDefinition.

someCommitOrTree is whatever you want to start with, the baseline for your modifications. You would typically want to take whatever is in HEAD as a baseline and then perform the changes on top. I'm not sure if you can start with an empty baseline.

You use ObjectDatabase.CreateBlob() to create the blobs from the files on the filesystem. Once you have set the changes, you create the tree via ObjectDatabase.CreateTree() and use that tree for the commit.

As for creating the commit. The pretty Repository.Commit() is trying to pretend it's git. In order to do this, we must go deeper into the bowels of libgit2sharp and use ObjectDatabase.CreateCommit()[0]. It does require passing some stuff which Repository.Commit() assumes.

[0] https://github.com/libgit2/libgit2sharp/blob/vNext/LibGit2Sharp/ObjectDatabase.cs#L291

@haacked This test shows how to create a Commit from a TreeDefinition. Keep in mind that only a Commit is created in the odb. The branch references are not updated. You'll have to do this by hand.

Thanks fellas. I played around with this and it's a bit of a pain for my scenario.

As for creating the commit. The pretty Repository.Commit() is trying to pretend it's git.

And for the most part, this is what I want. Using CreateCommit means I have to do a lot of work I didn't have to do before. For example, I'm just given a ICollection<string> paths. From that, I need to create the tree definition, blobs, figure out if they're executable, non-executable files, etc.

What I really want is an overload of Commit like so:

public Commit Commit(string message, IEnumerable<string> paths);

And have it only commit the files specified in the paths regardless of the current status of the index.

I think one problem we ran into with unstaging all the paths is that it would wipe away the MERGE_HEAD. So we need to be able to unstage all the changes without wiping the MERGE_HEAD.

I think one problem we ran into with unstaging all the paths is that it would wipe away the MERGE_HEAD.

That was when we did a reset, yes. I think the main problem with unstaging is that we'd have to iterate/pass in every file including ones not in the previous commit (I could be mistaken on this) whereas with read-tree we can just overwrite the index with the tree of a previous commit.

What I really want is an overload of Commit like so:

I think an even more interesting overload for us would be

public Commit Commit(string message, TreeDefinition tree);

That way we could load up the tree from the previous commit, add in the files that has been selected and write the commit without worrying about what's in the index and still have Commit do all the heavy lifting around determining whether or not it should be a merge commit and such. I don't know how feasible something like this would be or what sort of edge-cases there might be though.

I can't remember the exact reason why simply calling Unstage didn't work. @niik do you recall?

A mixed reset didn't work for us because it blows away MERGE_HEAD. Unstaging explicitly isn't something we've explored due to the reasons I outlined above.

I think an even more interesting overload for us would be

public Commit Commit(string message, TreeDefinition tree);

TreeDefinition belongs to the ObjectDatabase API; however, make that public Commit Commit(string message, Tree tree) and I think we're in business.

That way we could load up the tree from the previous commit, add in the files that has been selected

The problem I ran into is all we have is a set of string paths. But the method to add to a tree definition looks like:

public virtual TreeDefinition Add(string targetTreeEntryPath, Blob blob, Mode mode)

Per @carlosmn's suggestion, I can use repositoryInstance.ObjectDatabase.CreateBlob to create the Blob instances, but it's not clear what I'm supposed to choose for Mode. I'd prefer to let Git choose that.

So to reiterate, I have a list of paths and I want libGit2Sharp to commit the files in those paths in the same way Git.exe would if I had individually added them to the index with no other changes in the index. I don't want to have to try and determine if the file is a binary, executable, non-executable, etc.

A Commit() which lets you do a partial commit would be fine. The work to be done is the same, it'd just be packaged inside the library. For that, I'd go with a list of filenames since you do want to work with the workdir. A TreeDefinition overload is more what you want when you're dealing with data and you know beforehand more information or can make more assumptions.

This method would have to work like git-commit's partial commits, by taking HEAD's tree and adding these changes. This is rather trivial in git or raw libgit2 where you can temporarily attach an index to a repository, but I'm not sure if that's possible in libgit2sharp, even internally.

The one edge case here is what to do with new files (git won't let you commit them unless they're in the index, presumably to avoid typos and such). I wouldn't mind lifting that (maybe optionally) since we're dealing with programs which do error checking at different levels.

The mentions of MERGE_BASE by @haacked and @niik make me worried you guys are allowing partial commits as a resolution to a merge, which you may not want to expose. git itself forbids it as it clashes with the conceptual model of the resolution, and it has a pretty high bar for letting you do stupid stuff. Or is the reset-command-like leaking into stuff?

@haacked You may be after this overload which will dynamically create the Blob for you.

As for the mode, in 99% of the cases, there are only one entry that make sense, on Windows, in the context of that method: NonExecutableFile. Indeed, on Windows, in order to mark a .sh file as executable, even Git require additional operations.

BTW, I think we may be derailing this thread off of its initial topic. Could we move the Commit(), Reset() and TreeDefinition discussions to dedicated issues?

Yeah, let's move the partial commit discussion into its own issue. It will likely depend on this one, but it's a different discussion. @nulltoken you can't just pass NonExecutableFile when you're updating existing files, you need to keep the one they had or you might convert symlinks or executable files into normal files.

As for the actual topic of this PR and the name for read-tree, I don't like ReplaceTree because an index and a tree aren't the same thing. You can't replace one with the other. You can replace the contents of the index with those of the (flattened) tree. The index remains the same (both the instance as well as the file) but you're setting the contents/entries with those that can be found in the given tree. Maybe something like SetEntries() would work, which would let us have MergeEntries() for the merging version of read-tree, and lets us have a sensibly-named prefix argument whenever we implement that.

@carlosmn @nulltoken

Thanks for the feedback! :) I've pushed another commit that exposes the two methods as ObjectDatabase.CreateTree(index) and Index.SetEntries(tree). Also adopted / moved the tests.

In my backup tool, I treat the case that the index has conflicts in the same way I treat the case that a file I want to backup is not readable (busy), so I do not back up a tree with conflicts. Once the tree becomes fully merged, I begin backing it up again (similar to a file that becomes readable). It is definitely not perfect and I want to improve this in the future, but for now it is ok to backup the index in the 99% of time that it doesn't contain unresolved conflicts.

Cheers,
Alex

@alexandrudima Neat! One thought though. I wonder if creating a new overload for repo.Reset() wouldn't be cleaner than introducing SetEntries().

The current Reset() method accepts a Commit. Creating a new one accepting a Tree should do the trick, shouldn't it?

repo.Reset() comes in two flavors. The one I'm referring to has the following documentation.

/// <summary>
/// Replaces entries in the <see cref="Repository.Index"/> with entries from the specified commit.
/// </summary>
/// <param name="commit">The target commit object.</param>
/// <param name="paths">The list of paths (either files or directories) that should be considered.</param>
/// <param name="explicitPathsOptions">
/// If set, the passed <paramref name="paths"/> will be treated as explicit paths.
/// Use these options to determine how unmatched explicit paths should be handled.
/// </param>

FWIW, there's even a full test suite dedicated to resetting the index (ie. https://github.com/libgit2/libgit2sharp/blob/vNext/LibGit2Sharp.Tests/ResetIndexFixture.cs).

I guess SetEntries() is really no different than git reset --soft <ref> -- ., where <ref>'s tip is our new Tree.

Er, scratch that - --soft mode is not valid for git-reset with paths. I meant git reset <ref> -- .

I meant git reset <ref> -- .

@dahlbyk Yep. For more background, see b93a9e8 and #165

Does this version of reset actually peform a reset of the repository state? That's something we most definitely want to avoid.

Does this version of reset actually peform a reset of the repository state?

@carlosmn The actual implementation updates the index entries that differs.

That's something we most definitely want to avoid.

What are you referring to?

git reset (and thus our implementation for the non-path mode of the command) clears the state of the repository. I though you were calling out to the libgit2 reset command.

Following the code is turning out to be confusing. I realise that git exposes this under the same name, but these are two completely different commands. One is about doing index operations from trees rather than files in the workdir, the other is about clearing the repository state and (possibly) moving elsewhere.

@carlosmn Ok. Proposal would be to:

• Create a new overload accepting a Tree (as described in Expose git_index_write_tree and git_index_read_tree with a nice API #799 (comment)) in order to make @alexandrudima PR merged quickly.
• Move those index only Reset() operations to Index.Reset() later

Deal?

Counterproposal:

• Implement Index.Reset(Tree) in this PR, in anticipation that in a separate PR we will...
• Move (modulo [Obsolete] release cycle) Repository.Reset(paths) to Index.Reset(paths).
• Provide Repository.ResetPaths(paths) to mimic CheckoutPaths, which would delegate to Index.Reset(paths).
• Leaving only Repository.Reset(mode, commit) overload for manipulating repo state.

👍 on implementing this already as Index.Reset(). It would be the first step towards moving the non-resetty bits of Reset() out of the repository.

👍 as well 😉

@dahlbyk Could you please log a new issue for the future refactoring to be made? 🙏

@alexandrudima Given these. I think it's now only a matter of renaming SetEntries() into Reset(). BTW, could you add something in the xml-doc explaining that this requires the Index to be fully merged?

Could you please log a new issue for the future refactoring to be made?

#805

Could you please log a new issue for the future refactoring to be made?

#805

❤️💣

@nulltoken I've pushed another commit that renames Index.SetEntries(tree) to Index.Reset(tree). I wasn't sure about your second suggestion about the xml-doc. Index.Reset(tree) works if the index is not fully merged (added another test case - CanResetIndexWithUnmergedEntriesFromTree - to show this) and ObjectDatabase.CreateTree(index) already mentioned that the index must not have any conflicts. I've changed the wording there to say "the index must be fully merged". I hope that's ok.

Thanks!

@ethomson @dahlbyk @carlosmn I've done a quick test against mergedrepo_wd which has conflicts. Indeed git.git doesn't seem to complain about those (which surprises me a bit...).

$ cd mergedrepo_wd/

$ git status
# On branch master
# You have unmerged paths.
#   (fix conflicts and run "git commit")
#
# Changes to be committed:
#
#       modified:   one.txt
#       modified:   two.txt
#
# Unmerged paths:
#   (use "git add/rm <file>..." as appropriate to mark resolution)
#
#       deleted by them:    ancestor-and-ours.txt
#       deleted by us:      ancestor-and-theirs.txt
#       both deleted:       ancestor-only.txt
#       both modified:      conflicts-one.txt
#       both modified:      conflicts-two.txt
#       both added:         ours-and-theirs.txt
#       added by us:        ours-only.txt
#       added by them:      theirs-only.txt
#

$ git reset HEAD^ --
Unstaged changes after reset:
M       ancestor-and-ours.txt
M       ancestor-and-theirs.txt
D       ancestor-only.txt
M       conflicts-one.txt
M       conflicts-two.txt
M       one.txt
M       two.txt

$ git status
# On branch master
# Changes not staged for commit:
#   (use "git add/rm <file>..." to update what will be committed)
#   (use "git checkout -- <file>..." to discard changes in working directory)
#
#       modified:   ancestor-and-ours.txt
#       modified:   ancestor-and-theirs.txt
#       deleted:    ancestor-only.txt
#       modified:   conflicts-one.txt
#       modified:   conflicts-two.txt
#       modified:   one.txt
#       modified:   two.txt
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       ours-and-theirs.txt
#       ours-only.txt
#       theirs-only.txt
no changes added to commit (use "git add" and/or "git commit -a")

$

Thoughts?

git-reset is a pretty plumby command, and it works with whatever entries you provide. There's no particular reason why it needs a merged index. And in fact, this would be how you can fix conflicts, a lower-level version of git checkout --ours|--theirs.

CreateTree()/write-tree needs a merged index as there is no possible representation of conflicts in trees, but for messing around with tree entries directly (or simply saying, "discard the data and read from this tree") there isn't any particular need for it. Doing reset on an entry with conflicts presumably clears them, so we should make sure we do the same in that case.

@carlosmn Thanks for the feedback.

Doing reset on an entry with conflicts presumably clears them, so we should make sure we do the same in that case.

So adding a Assert.Equal(true, repo.Index.IsFullyMerged); at the very end of CanResetIndexWithUnmergedEntriesFromTree() would be enough?

For the read-tree case, yes. The index needs to be fully merged, since we're setting the index to look like the tree.

So adding a Assert.Equal(true, repo.Index.IsFullyMerged); at the very end of CanResetIndexWithUnmergedEntriesFromTree() would be enough?

@alexandrudima Could you please add this little assertion?

Thinking about it a little more, I think we rather express this as Assert.True(repo.Index.IsFullyMerged). While you're at it, could you please update the PR so it rather relies on this kind of semantic?

@nulltoken

Done! :)

The tests pass on my machine, but the build failed, looks like it couldn't clone properly. Maybe you know how to trigger it again without another push from my side?

Cloning into 'libgit2/libgit2sharp'...
remote: Counting objects: 2101, done.
remote: Compressing objects:  39% (589/1510)   
No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

Thanks,
Alex

I restarted the build.

@alexandrudima Awesome!

Last request: Could you please squash those commits into one and rebase them onto vNext?

Spoiler: 😉

First, make sure to fetch the latest tips from libgit2/libgit2sharp. This should update your local vNext with the latest changes.

Then, given that your local checked out branch is alexdima/indexrw.

The following should do the trick

# reset the HEAD to ad12cca3ba989659 without modifying your index nor your working directory
$ git reset --soft ad12cca3ba989659 

# Create one single commit with all your changes
$ git commit -m "Introduce low level Index/Tree operations"

# Rebase this commit onto the latest vNext
$ git rebase vNext alexdima/indexrw
First, rewinding head to replay your work on top of it...
Applying: Teach RemoteUpdater to set the Url

$

Then a force push should dynamically update the PR.

@carlosmn Thank you!
@nulltoken Thanks for the detailed steps. Hope it's good now!

Cheers,
Alex

Hope it's good now!

It's in! Cool job, Sir! ✨