Introduce StatusOptions.IncludeUnaltered #863

Therzok · 2014-11-07T13:26:48Z

Test can be improved, what else needs done?

carlosmn · 2014-11-07T13:53:59Z

"unaltered" sounds like a weird way to talk about unmodified files; what's the reason behind the different name for the option than the libgit2 flag?

Therzok · 2014-11-07T14:31:40Z

This is the reason: https://github.com/libgit2/libgit2sharp/blob/vNext/LibGit2Sharp/FileStatus.cs#L19

Therzok · 2014-11-07T14:33:14Z

I know why this fails, I haven't rebased on top of master and it appears to merge cleanly. When this has a 👍 from everyone, I'll rebase.

nulltoken · 2014-11-10T09:43:41Z

Looks clean to me. 👍

Therzok · 2014-11-10T12:31:53Z

Rebased.

nulltoken · 2014-11-10T15:18:27Z

LibGit2Sharp.Tests/StatusFixture.cs

+            {
+                RepositoryStatus status = repo.RetrieveStatus(new StatusOptions() { IncludeUnaltered = true });
+
+                Assert.Equal(17, status.Unaltered.Count());


$ git ls-files 1.txt 1/branch_file.txt README branch_file.txt deleted_unstaged_file.txt modified_staged_file.txt modified_unstaged_file.txt new.txt new_tracked_file.txt $ git status -s D deleted_staged_file.txt D deleted_unstaged_file.txt M modified_staged_file.txt M modified_unstaged_file.txt A new_tracked_file.txt ?? new_untracked_file.txt

Considering this above, 17 looks like a very high number to me 😜

That includes the .git directory for some reason.

That includes the .git directory for some reason.

Huh? That's weird. Outputting to Console the content of Unaltered on my box renders the following

Unaltered: 1.txt Unaltered: 1\branch_file.txt Unaltered: branch_file.txt Unaltered: branch_file.txt Removed: deleted_staged_file.txt Missing: deleted_unstaged_file.txt Unaltered: deleted_unstaged_file.txt Unaltered: modified_staged_file.txt Staged: modified_staged_file.txt Modified: modified_unstaged_file.txt Unaltered: modified_unstaged_file.txt Unaltered: new.txt Unaltered: new.txt Unaltered: new_tracked_file.txt Added: new_tracked_file.txt Untracked: new_untracked_file.txt Unaltered: README

Oh, wut? All items that are staged are also marked as unaltered. This is a weird one.

Unaltered is defined as 0, which means it's always being matched by HasFlag().

Ooooooh. I see. Great, I'm going to modify it.

Also, I want to state that HasFlag is slow code compared to bitwise check and a status is performance critical.

Should I change it to a bitwise comparison?

http://www.dotnetbitsandpieces.com/?p=11

Which reminds me that the indexer operator on RepositoryStatus isn't tested, otherwise it would've been triggered in an unit test.

It's often premature optimization, but I agree that here it's probably worth it.

Therzok · 2014-11-10T20:55:16Z

Okay, new version up, I want to state that libgit2 is emitting two statuses for items that are changed in the index and unchanged in the workdir. So it ends up as Modified | Staged in the index and Unaltered in the workdir.

Therzok · 2014-11-10T21:00:18Z

In fact, let me rephrase, this is the output.
1.txt
1/branch_file.txt
branch_file.txt
branch_file.txt
deleted_unstaged_file.txt
modified_staged_file.txt
modified_unstaged_file.txt
new.txt
new.txt
new_tracked_file.txt
README

nulltoken · 2014-11-10T21:12:46Z

LibGit2Sharp.Tests/StatusFixture.cs

+            {
+                RepositoryStatus status = repo.Index.RetrieveStatus(new StatusOptions() { IncludeUnaltered = true });
+
+                Assert.Equal(11, status.Unaltered.Count());


What are we willing to retrieve here? The number of files that are identical in the workdir, the index and the HEAD? If that's the case, I've got the feeling that the number should be a bit lower.

Yep, that's why I'm confused over here. Probably a bug in libgit2?

Well, what are you trying to compare to? Is the file in the working directory the same as the index? The same as HEAD?

I'm trying to get all the files that are unchanged (both index and workdir) vs. HEAD.

It could've been solved easily if there were 2 entries of Unaltered, one for index, one for workdir, but README is the same as branch_file.txt in terms of status, yet the latter is appearing twice, while the former appears just once.

nulltoken · 2014-11-10T21:12:53Z

I want to state that libgit2 is emitting two statuses for items that are changed in the index and unchanged in the workdir. So it ends up as Modified | Staged in the index and Unaltered in the workdir.

@ethomson I think you're one of the latest that have worked in this area. Any thoughts?

dahlbyk · 2014-11-11T23:53:17Z

LibGit2Sharp/RepositoryStatus.cs

@@ -81,7 +82,7 @@ internal RepositoryStatus(Repository repo, StatusOptions options)
                    AddStatusEntryForDelta(entry.Status, deltaHeadToIndex, deltaIndexToWorkDir);
                }

-                isDirty = statusEntries.Any(entry => entry.State != FileStatus.Ignored);
+                isDirty = statusEntries.Any(entry => entry.State != FileStatus.Ignored || entry.State != FileStatus.Unaltered);


Shouldn't this be &&?

Argh, I'm a retard. Fixing it.

nulltoken · 2014-11-12T15:14:12Z

error CS1501: No overload for method 'RetrieveStatus' takes 1 arguments [C:\projects\libgit2sharp\LibGit2Sharp.Tests\LibGit2Sharp.Tests.csproj]

nulltoken · 2014-11-12T22:53:51Z

LibGit2Sharp.Tests/StatusFixture.cs

+            {
+                RepositoryStatus status = repo.RetrieveStatus(new StatusOptions() { IncludeUnaltered = true });
+
+                Assert.Equal(11, status.Unaltered.Count());


@Therzok Could you chnage the 11 so that it actually reflects the real number of files we'd like to see retrieved?

Yes, I can. And I will.

Or, even better maybe, directly assert the expected filenames?

Therzok · 2014-12-02T05:22:06Z

@ethomson anything else you'd like to add? I'm at a loss.

ethomson · 2014-12-05T17:21:49Z

Yeah, something fishy is going on in libgit2 land. I will try to dig in to this over the weekend.

Therzok · 2014-12-29T17:22:07Z

@ethomson am I actually doing something wrong regarding this? Or is it really a libgit2 bug?

ethomson · 2015-01-05T16:17:57Z

@Therzok There's a libgit2 bug in here around combining the two lists. I'll try to take a look at it further this week. Sorry about the delay with the holidays and the $DAYJOB and such.

Therzok · 2015-04-05T12:20:33Z

Okay, this seems fixed in latest libgit2. Merge at will. The appveyor failure seems unrelated.

nulltoken · 2015-04-05T17:27:08Z

LibGit2Sharp/RepositoryStatus.cs

@@ -81,7 +82,7 @@ internal RepositoryStatus(Repository repo, StatusOptions options)
                    AddStatusEntryForDelta(entry.Status, deltaHeadToIndex, deltaIndexToWorkDir);
                }

-                isDirty = statusEntries.Any(entry => entry.State != FileStatus.Ignored);
+                isDirty = statusEntries.Any(entry => entry.State != FileStatus.Ignored && entry.State != FileStatus.Unaltered);


@Therzok As we're modifying the behavior of the IsDirty property, could you please add some test coverage regarding this?

Actually, we don't quite modify the behaviour. We extend it here.

Without IncludeUnaltered, we never had Unaltered file status. I'll add coverage for it though.

That said, does having Untracked files mark the status as dirty? Thoughts?

I'd say no. What does git.git has to say about this?

I just did a:

test -n "$(git status --porcelain)" || echo "asdf" with untracked files and that makes it considered dirty. "asdf" isn't printed.

Okay, should be good to go now.

Ah, the option to take out Untracked files is something else.

You can query the git commandline in two ways for status changes:
git status --porcelain
git status --porcelain -uno (no untracked)

That's for later, yes.

Ah, the option to take out Untracked files is something else.

Dammit! I read it backwards.

That's for later, yes.

👍

FileStatus is a Flag enum - do you mean to use the != comparison, or should we use the Enum.HasFlag to check for these instead?

These are exclusive statuses. Unaltered and Ignored can only appear by themselves.

Also, x & 0 == 0 always. So we can't use bitwise and on Unaltered.

Therzok · 2015-04-05T18:08:25Z

LibGit2Sharp.Tests/StatusFixture.cs

+        [Theory]
+        [InlineData(false, 0)]
+        [InlineData(true, 5)]
+        public void CanRetrieveTheStatusOfTheWholeWorkingDirectory(bool includeUnaltered, int untrackedCount)


Testing both unaltered and altered for status dirtiness.

Shouldn't it be unalteredCount?

Yeah, I'm an idiot, fixing that now.

Therzok · 2015-04-10T22:16:13Z

@nulltoken can you slap travis around with a large trout?

nulltoken · 2015-04-11T08:41:24Z

@Therzok Travis seems to have recovered. Still needed?

Therzok · 2015-04-11T10:28:05Z

As long as it's green, I'm okay with it. Ready to merge. 🎱

jamill · 2015-04-11T11:19:26Z

LibGit2Sharp/RepositoryStatus.cs

@@ -164,14 +171,21 @@ private void AddStatusEntryForDelta(FileStatus gitStatus, GitDiffDelta deltaHead

            StatusEntry statusEntry = new StatusEntry(filePath, gitStatus, headToIndexRenameDetails, indexToWorkDirRenameDetails);

-            foreach (KeyValuePair<FileStatus, Action<RepositoryStatus, StatusEntry>> kvp in dispatcher)
+            if (gitStatus == FileStatus.Unaltered)


Could we handle the FileStatus.Unaltered value in the same way we handle the other values (in the dispatcher dictionary)?

Ah - probably not if FileStatus.Unaltered = 0.

nulltoken · 2015-04-14T18:09:58Z

@jamill Are you happy to see this in now?

jamill · 2015-04-15T17:51:42Z

Looks good, thanks!

Introduce StatusOptions.IncludeUnaltered

nulltoken · 2015-04-15T18:11:18Z

@Therzok 👍

Therzok changed the title ~~Introduce StatusOptions.IncludeUnaltered~~ Introduce StatusOptions switches Nov 7, 2014

Therzok changed the title ~~Introduce StatusOptions switches~~ Introduce StatusOptions.IncludeUnaltered Nov 7, 2014

Therzok force-pushed the therzok/statusUnmodified branch from 270a6b1 to 177e5f3 Compare November 10, 2014 12:31

nulltoken reviewed Nov 10, 2014
View reviewed changes

Therzok force-pushed the therzok/statusUnmodified branch from 177e5f3 to 7339b31 Compare November 10, 2014 20:54

nulltoken reviewed Nov 10, 2014
View reviewed changes

Therzok added libgit2 Bug and removed Bug libgit2 labels Nov 11, 2014

Therzok force-pushed the therzok/statusUnmodified branch from 7339b31 to 5fa6eef Compare November 11, 2014 11:42

dahlbyk reviewed Nov 11, 2014
View reviewed changes

Therzok force-pushed the therzok/statusUnmodified branch from 5fa6eef to a8493e3 Compare November 12, 2014 22:10

nulltoken reviewed Nov 12, 2014
View reviewed changes

Therzok force-pushed the therzok/statusUnmodified branch 2 times, most recently from fa21d54 to 77ab73a Compare November 13, 2014 09:02

Therzok mentioned this pull request Jan 22, 2015

Status adding unwanted duplicates with INCLUDE_UNMODIFIED libgit2/libgit2#2833

Closed

Therzok force-pushed the therzok/statusUnmodified branch from 77ab73a to d63c300 Compare April 5, 2015 12:10

Therzok force-pushed the therzok/statusUnmodified branch from d63c300 to c930e2f Compare April 5, 2015 14:30

nulltoken reviewed Apr 5, 2015
View reviewed changes

Therzok force-pushed the therzok/statusUnmodified branch 2 times, most recently from 1a1be46 to a3fcb1e Compare April 5, 2015 18:01

Therzok reviewed Apr 5, 2015
View reviewed changes

Introduce StatusOptions.IncludeUnaltered

11da2fa

Therzok force-pushed the therzok/statusUnmodified branch from a3fcb1e to 11da2fa Compare April 7, 2015 13:03

jamill reviewed Apr 11, 2015
View reviewed changes

nulltoken added this to the v0.22 milestone Apr 11, 2015

nulltoken added a commit that referenced this pull request Apr 15, 2015

Merge pull request #863 from libgit2/therzok/statusUnmodified

40de694

Introduce StatusOptions.IncludeUnaltered

nulltoken merged commit 40de694 into vNext Apr 15, 2015

nulltoken deleted the therzok/statusUnmodified branch April 15, 2015 18:11

Introduce StatusOptions.IncludeUnaltered #863

Introduce StatusOptions.IncludeUnaltered #863

Uh oh!

Conversation

Therzok commented Nov 7, 2014

Uh oh!

carlosmn commented Nov 7, 2014

Uh oh!

Therzok commented Nov 7, 2014

Uh oh!

Therzok commented Nov 7, 2014

Uh oh!

nulltoken commented Nov 10, 2014

Uh oh!

Therzok commented Nov 10, 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!

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!

Therzok commented Nov 10, 2014

Uh oh!

Therzok commented Nov 10, 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!

nulltoken commented Nov 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Nov 12, 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 Dec 2, 2014

Uh oh!

ethomson commented Dec 5, 2014

Uh oh!

Therzok commented Dec 29, 2014

Uh oh!

ethomson commented Jan 5, 2015

Uh oh!

Therzok commented Apr 5, 2015

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!