Upgrade binaries to current LibGit2 HEAD. (5d91bea) #722

Therzok · 2014-05-28T14:11:21Z

Okay so this is the bump itself. I split it into binary update then API usage update.

A few things which I will handle later on:

There are now callbacks for ssh_interactive
There is new remote API and diff stats API.
There is a config snapshot API.
There is cherry-pick API.

carlosmn · 2014-05-28T14:13:10Z

The default config getter needs to grab the snapshot version.

Therzok · 2014-05-28T14:16:42Z

Perfect, I'll do that, thanks.

Therzok · 2014-05-28T14:21:39Z

@carlosmn So, instead of getter being 'config.Value', you mean adding a config_snapshot call before-hand, right?

Therzok · 2014-05-28T14:29:07Z

Oh, I understood what you meant now.

Therzok · 2014-05-28T16:05:25Z

@nulltoken Any insight on the unit tests that are failing? Also on the MergePreference approach.

Therzok · 2014-05-28T16:44:28Z

LibGit2Sharp/Configuration.cs

@@ -88,7 +88,7 @@ public Configuration(string globalConfigurationFileLocation = null, string xdgCo
        /// </summary>
        public virtual bool HasConfig(ConfigurationLevel level)


I'm unsure whether this would require a snapshot or not.

We should favour using a snapshot unless we need to write into it.

Therzok · 2014-05-28T20:15:07Z

Update on failing tests: The RetrieveStatus function apparently has lots of +0/-0, which either means lineendings changed or filemodes, but I don't see them changed either. I'm currently at a loss and I'd like someone to give some insight over this.

jamill · 2014-05-28T20:17:04Z

LibGit2Sharp/Core/GitMergeOpts.cs

@@ -72,6 +72,28 @@ internal enum GitMergeAnalysis
    }

    [Flags]


I don't think we need the Flags attribute - I think these are all exclusive values, even if the C representation treats these as combinable values...

/cc @ethomson ?

That's correct.

Other than not being able to safely use them in a switch, what could be the issue here?

I feel marking an enum as flag when the enum is not a flag will be confusing to people who work with this. We can do direct comparisons on the value, instead of seeing using the HasFlag(..) method to see if a value contains a flag.

(off-topic: HasFlag is slower than (x & flag) != 0)

jamill · 2014-05-28T20:38:40Z

This new MergePreference thing should be exposed, right? So, how do I handle it? Move the result to a ref parameter or make the return value a Tuple/KeyValuePair?

I think there are a couple of ways this could be done. My initial thoughts would be to move GitMergePreference and the GitMergeAnalysis to ref parameters. Then we could add some logic that would take these two parameters as input and decide how to handle the merge...

Either way, I would suggest doing the changes for the MergePreference handling in a separate commit.

Therzok · 2014-05-28T20:40:42Z

@jamill Obviously, the same way I handled Configuration changes. I'm only rebasing where necessary so I keep commits atomic.

Therzok · 2014-05-29T08:38:46Z

Removed the Flags attribute as per @jamill 's request. Also added a commit in which we keep track of MergePreference (it's still unused, all I know about it is that it's the value of the user's config for merge.ff and I don't know how that changes a call to Merge.)

carlosmn · 2014-05-29T08:45:41Z

It doesn't change the call to Merge, just like the analysis of the possibilities doesn't. It lets the tool writer know what the user preference is so it can decide whether to call Merge or not.

nulltoken · 2014-05-30T09:56:59Z

@Therzok I've started to look at the failing tests.

CanCleanWorkingDirectory() is actually a false positive. It looks like it was wrongly passing before.

Below the output of git status for the test repo on my box:

$ git status
warning: LF will be replaced by CRLF in modified_staged_file.txt.
The file will have its original line endings in your working directory.
warning: LF will be replaced by CRLF in new_tracked_file.txt.
The file will have its original line endings in your working directory.
# On branch master
# Your branch and 'origin/master' have diverged,
# and have 2 and 2 different commits each, respectively.
#   (use "git pull" to merge the remote branch into yours)
#
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#       deleted:    deleted_staged_file.txt
#       modified:   modified_staged_file.txt
#       new file:   new_tracked_file.txt
#
# 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:   1/branch_file.txt
#       modified:   README
#       modified:   branch_file.txt
#       deleted:    deleted_unstaged_file.txt
#       modified:   modified_unstaged_file.txt
#       modified:   new.txt
#
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       new_untracked_file.txt

Following patch makes it pass:

diff --git a/LibGit2Sharp.Tests/CleanFixture.cs b/LibGit2Sharp.Tests/CleanFixture.cs
index 9613a2c..8ca9972 100644
--- a/LibGit2Sharp.Tests/CleanFixture.cs
+++ b/LibGit2Sharp.Tests/CleanFixture.cs
@@ -13,14 +13,16 @@ public void CanCleanWorkingDirectory()
             using (var repo = new Repository(path))
             {
                 // Verify that there are the expected number of entries and untracked files
-                Assert.Equal(6, repo.Index.RetrieveStatus().Count());
-                Assert.Equal(1, repo.Index.RetrieveStatus().Untracked.Count());
+                var status = repo.Index.RetrieveStatus();
+                Assert.Equal(10, status.Count());
+                Assert.Equal(1, status.Untracked.Count());

                 repo.RemoveUntrackedFiles();

                 // Verify that there are the expected number of entries and 0 untracked files
-                Assert.Equal(5, repo.Index.RetrieveStatus().Count());
-                Assert.Equal(0, repo.Index.RetrieveStatus().Untracked.Count());
+                var newStatus = repo.Index.RetrieveStatus();
+                Assert.Equal(9, newStatus.Count());
+                Assert.Equal(0, newStatus.Untracked.Count());
             }
         }

nulltoken · 2014-05-30T10:16:18Z

@Therzok Hmmm. Hold on to the previous comment. I need to dig a little deeper.

Therzok · 2014-05-30T10:31:16Z

@nulltoken there are 44 tests failing, and I don't know what changed. It's the index behaving differently.

nulltoken · 2014-05-30T11:28:51Z

@nulltoken there are 44 tests failing, and I don't know what changed. It's the index behaving differently.

Actually, I think this is because libgit2 has been tuned to behave more like git.git. (Kudos to @arrbee!).

#727 updates the test repo so that they're no longer expose a dirty status. Before I merge it, could you please give a try and rebase your PR on top of it. My local tests show that it fixes most of the issues.

Therzok · 2014-05-30T11:43:19Z

Sure thing! Will do that in a jiffy.

Therzok · 2014-05-30T11:47:22Z

Yes, all tests pass now, thanks @nulltoken . Merge your PR, then this build can be redone.

Therzok · 2014-05-30T11:48:07Z

Wait, one still failed. LibGit2Sharp.Tests.StatusFixture.RetrievingTheStatusOfTheRepositoryHonorsTheGitIgnoreDirectivesThroughoutDirectories.

nulltoken · 2014-05-30T11:49:22Z

RetrievingTheStatusOfTheRepositoryHonorsTheGitIgnoreDirectivesThroughoutDirectories

I've seen this as well. @carlosmn is trying to reproduce this through a libgit2 unit test.

nulltoken · 2014-05-30T11:52:02Z

Yes, all tests pass now, thanks @nulltoken . Merge your PR, then this build can be redone.

It's currently being run through TeamCity CI server (http://teamcity.codebetter.com/project.html?projectId=LibGit2Sharp). Will be merged shortly after.

nulltoken · 2014-05-30T12:08:45Z

Yes, all tests pass now, thanks @nulltoken . Merge your PR, then this build can be redone.

It's currently being run through TeamCity CI server (http://teamcity.codebetter.com/project.html?projectId=LibGit2Sharp). Will be merged shortly after.

@Therzok #727 has been merged.

Therzok · 2014-05-30T12:09:00Z

I'll rebase on top of master and repush.

Therzok · 2014-05-30T12:10:31Z

Done.

Therzok · 2014-05-30T12:34:04Z

So, do I wait for a fix in libgit2 and amend e869a8e?

carlosmn · 2014-05-30T12:34:45Z

The fix is in two parts, I'm going to send PRs for them.

Therzok · 2014-05-30T14:05:38Z

Updated to new tip of libgit2 and cherry-picked @carlosmn 's fix. This should be good to go.

nulltoken · 2014-05-30T14:12:31Z

@Therzok When the build passes, could you please squash all the commits together into 542e6f0 and add a line to the commit message with the link to the libgit2 github compare (As it's done in fb3c4f0 for instance)?

Therzok · 2014-05-30T14:41:51Z

All the outstanding fixes have been applied and the commits have been squashed. When it goes green, I think this can be merged.

nulltoken · 2014-05-30T14:56:00Z

@Therzok Amazing work! Thanks: ✨ ✨ ✨ ✨ ✨

Therzok reviewed May 28, 2014
View reviewed changes

jamill reviewed May 28, 2014
View reviewed changes

Therzok mentioned this pull request May 29, 2014

Introduce merge.ff configuration handling #709

Merged

nulltoken mentioned this pull request May 30, 2014

Fix indices of test repos so that they're not dirty #727

Merged

nulltoken added this to the v0.18.0 milestone May 30, 2014

nulltoken merged commit 40ac869 into libgit2:vNext May 30, 2014

Therzok deleted the upgradeBinaries branch May 30, 2014 14:57

		@@ -88,7 +88,7 @@ public Configuration(string globalConfigurationFileLocation = null, string xdgCo
		/// </summary>
		public virtual bool HasConfig(ConfigurationLevel level)

Upgrade binaries to current LibGit2 HEAD. (5d91bea) #722

Upgrade binaries to current LibGit2 HEAD. (5d91bea) #722

Uh oh!

Conversation

Therzok commented May 28, 2014

Uh oh!

carlosmn commented May 28, 2014

Uh oh!

Therzok commented May 28, 2014

Uh oh!

Therzok commented May 28, 2014

Uh oh!

Therzok commented May 28, 2014

Uh oh!

Therzok commented May 28, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Therzok commented May 28, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented May 28, 2014

Uh oh!

Therzok commented May 28, 2014

Uh oh!

Therzok commented May 29, 2014

Uh oh!

carlosmn commented May 29, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

carlosmn commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

Therzok commented May 30, 2014

Uh oh!

nulltoken commented May 30, 2014

Uh oh!

Uh oh!