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

string match -n does not work with -r -v #3098

Closed
msteed opened this Issue May 31, 2016 · 5 comments

Comments

Projects
None yet
4 participants
@msteed
Contributor

msteed commented May 31, 2016

string match with -r (regex) and -v (invert) does not respect -n (index).

Reproduction Steps:

$ string match -r -v -n a b

Expected behavior:

fish should report the start position and length when -n is given, as it does without -v.

Observed behavior:

$ string match -r -v -n a b
b

Additional information:

I think this was introduced with the addition of the (-v | --invert) options to string. The problem appears to be here: https://github.com/fish-shell/fish-shell/blob/master/src/builtin_string.cpp#L378-L381 where the argument is reported without checking for opts.index as is done below.


Fish version: fish, version 2.3.0

Operating system: Arch linux, fish installed from the official package


For 2.3.1 release:

  • Update release notes

@msteed msteed changed the title from string match -r -v does not work with -n to string match -n does not work with -r -v May 31, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho May 31, 2016

Member

fish should report the start position and length when -n is given, as it does without -v.

"--invert" is maybe a bit inaccurate here. What it does is ignore all arguments/lines without a match (taken straight from grep's -v or --invert option). I can't see how you can say where a non-match starts - unless it's always "1", and the length is always the length of the entire string.

Member

faho commented May 31, 2016

fish should report the start position and length when -n is given, as it does without -v.

"--invert" is maybe a bit inaccurate here. What it does is ignore all arguments/lines without a match (taken straight from grep's -v or --invert option). I can't see how you can say where a non-match starts - unless it's always "1", and the length is always the length of the entire string.

@msteed

This comment has been minimized.

Show comment
Hide comment
@msteed

msteed May 31, 2016

Contributor

Right, and this is the current behavior without -r:

$ string match -n -v a bbb
1 3

Contributor

msteed commented May 31, 2016

Right, and this is the current behavior without -r:

$ string match -n -v a bbb
1 3

@faho faho closed this in 8d6735c May 31, 2016

@faho faho added the bug label May 31, 2016

@faho faho added this to the next-2.x milestone May 31, 2016

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho May 31, 2016

Member

Yeah, maybe this is useful for someone.

I've now pushed a commit which I believe fixes it.

Thanks!

Member

faho commented May 31, 2016

Yeah, maybe this is useful for someone.

I've now pushed a commit which I believe fixes it.

Thanks!

@ridiculousfish ridiculousfish modified the milestones: 2.3.1, next-2.x Jun 2, 2016

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Jun 2, 2016

Member

If we do a 2.3.1, we ought to take this too.

Member

ridiculousfish commented Jun 2, 2016

If we do a 2.3.1, we ought to take this too.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Jun 4, 2016

Member

I'm not sure I ever tried -n and -v together while I was adding -v -- never occurred to me that it should do anything. That it did anything at all without -r was a (lucky?) oversight. Are the exit codes as expected?

Member

floam commented Jun 4, 2016

I'm not sure I ever tried -n and -v together while I was adding -v -- never occurred to me that it should do anything. That it did anything at all without -r was a (lucky?) oversight. Are the exit codes as expected?

krader1961 added a commit that referenced this issue Jun 21, 2016

@floam floam self-assigned this Jul 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment