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

lib/ignore: Consistent behaviour for nil *Matcher #4310

Closed
wants to merge 4 commits into from

Conversation

imsodin
Copy link
Member

@imsodin imsodin commented Aug 21, 2017

Some exported functions on *Matcher check whether it is nil, others don't. This PR adds such check consistently.
As the first match determines the result when ignoring, duplicate lines have no effect. Therefore these are now filtered out when parsing the file.

@@ -125,40 +127,42 @@ func New(fs fs.Filesystem, opts ...Option) *Matcher {
}

func (m *Matcher) Load(file string) error {
if m == nil {
return IsNil
}
Copy link
Member

Choose a reason for hiding this comment

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

I'd just return nil instead of IsNil everywhere, a nil matcher is a noop.

Copy link
Member

Choose a reason for hiding this comment

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

But Load() on a nil matcher is a clear error and should (imho) panic as it's always a bug.

Copy link
Member

Choose a reason for hiding this comment

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

well I'd panic anyway if we just removed a check with a nil pointer dereference.
So the fact that we are doing something implies we want to improve something.

@calmh
Copy link
Member

calmh commented Aug 21, 2017

Honestly, wouldn't it be simpler to just have it never check for nils and make sure that it is not nil when used from the Model? I bet that originally it was a neat shortcut to be able to pass a nil matcher when there were no ignores, and have Match() and Patterns() return something sane anyway. But apparently we don't do that any more (I guess?) since Lines() would explode when called on a nil matcher.

@imsodin
Copy link
Member Author

imsodin commented Aug 21, 2017

I should have referenced earlier discussions on this:
#4301
https://forum.syncthing.net/t/how-to-expose-ignore-package/10356

In a nut-shell: Audrius considers nil-Matcher a valid noop-Matcher, my opinion is/was to consider a nil Matcher as not valid everywhere. No other feedback (should have pinged) -> I was going with his option.

@imsodin
Copy link
Member Author

imsodin commented Aug 25, 2017

The last commit is the other variant where no nil checks are done, making it "illegal" to use Matcher nil pointers (panic will happen on locking). The only other adaption needed is to add a check for nil in scan.Walk as ignores aren't set in tests.

@imsodin
Copy link
Member Author

imsodin commented Sep 4, 2017

I split the unrelated part about .stignore line dedupliation into it's own PR #4350, such that this is now exclusively about the nil business.

@imsodin imsodin changed the title lib/ignore: Consistent nil checks and filter duplicate lines in .stignore lib/ignore: Consistent behaviour for nil *Matcher Sep 4, 2017
@AudriusButkevicius
Copy link
Member

I don't have an opinion on this, so I'll leave to to @calmh

@calmh
Copy link
Member

calmh commented Sep 6, 2017

@st-review merge it

lib/ignore: Consistent behaviour for nil *Matcher

@st-review
Copy link

👌 Merged as 9dbc509. Thanks, @imsodin!

@st-review st-review closed this Sep 6, 2017
st-review pushed a commit that referenced this pull request Sep 6, 2017
@imsodin imsodin deleted the ignore-stuff branch September 6, 2017 11:18
@st-review st-review added the pr-merged Legacy label used in the past for pull requests merged to the main tree label Jan 15, 2018
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Sep 10, 2018
@syncthing syncthing locked and limited conversation to collaborators Sep 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion pr-merged Legacy label used in the past for pull requests merged to the main tree
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants