Skip to content
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

make excluding ignored files the default in status options (perf impr… #1331

Merged
merged 1 commit into from Jun 24, 2017

Conversation

paulgmiller
Copy link
Contributor

Switch the default as talked about. Only a few tests had to be changed.

@@ -652,7 +652,7 @@ public void ForceCheckoutRetainsIgnoredChanges()
"bin/some_ignored_file.txt",
"hello from this ignored file.");

Assert.Equal(1, repo.RetrieveStatus().Ignored.Count());
Assert.Equal(1, repo.RetrieveStatus(new StatusOptions { IncludeIgnored = true }).Ignored.Count());
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a similar test: Assert.Equal(0, repo.RetrieveStatus().Ignored.Count()) above this?

@ethomson
Copy link
Member

ethomson commented Jul 5, 2016

I'm 👍 on this. @carlosmn ?

I think that we should responsibly deprecate this, though. I think that we should hold on to this until after the next release, and immediately merge a note in the CHANGELOG and a deprecation warning to notify people of the change.

I'm not very familiar with all the c# attributes. Is it appropriate to use [Obsolete("...")] here to get people to notice...? It seems like it is not, but I'm not sure there's a superior alternative...

@carlosmn
Copy link
Member

carlosmn commented Jul 6, 2016

I don't know that there is a good way to do this. Yes we should switch the default, but I don't think there's a way to warn about this without it looking like we're going to remove the whole thing. A line in CHANGELOG is probably the best sensible way we have.

I'm also not a fan of billing this a performance improvement since you can already do this and the change in behaviour should be made to match git's defaults.

@ethomson ethomson merged commit 1a586e3 into libgit2:master Jun 24, 2017
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.

None yet

3 participants