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

fixing bad regex on file tracking in two VCS (git and hg) #509

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

jaesivsm
Copy link
Contributor

@jaesivsm jaesivsm commented Mar 3, 2017

untracked flag is showing as soon as modification has been made (as soon as git status --porcelain show something)

This fix should solve this for both git and mercurial

@Rycieos
Copy link
Collaborator

Rycieos commented Mar 3, 2017

This appears to undo a previous commit?
e21a4c2

Firstly, grep -E is not portable: systems without GNU grep do not have support for the -E flag. Possibly try to solve this without that option?

Second, what problem is this solving? Your explanation seems confusing, the flag should show as soon as there is something that is not tracked.

@jaesivsm
Copy link
Contributor Author

jaesivsm commented Mar 5, 2017

Sorry for not having been clearer, I'll show you what I get on my machine (ubuntu 16.10):

Both, with -E and without it works for checking if untracked file are present:

$ touch testfile
$ git status --porcelain
?? testfile
$ if LC_ALL=C \git status --porcelain 2>/dev/null | \grep -Eq '^\?\?'; then echo untracked; fi> 
untracked
$ if LC_ALL=C \git status --porcelain 2>/dev/null | \grep -q '^\?\?'; then echo untracked; fi
untracked

But if I modify a file and have no untracked file:

$ echo >> README.md 
$ git status --porcelain
 M README.md
$ if LC_ALL=C \git status --porcelain 2>/dev/null | \grep -q '^\?\?'; then echo untracked; fi
untracked

I still get the untracked mark which is false.

I'll look into a better solution though, since -E is not portable.

replacing it by Basic Regular Expression, Extended ones are not supported on non GNU grep
@j-degreef
Copy link

j-degreef commented Mar 6, 2017

From the grep man page (Linux Mint 18.1):

Basic vs Extended Regular Expressions
> In basic regular expressions the meta-characters ?, +, {, |, (, and ) lose
> their special meaning; instead use the backslashed versions \?, \+, \{,
> \|, \(, and \).

I assume that without -E we are using BRE so we should use grep -q '^??'. Escaping the "?" does exactly the opposite of what we want.

if LC_ALL=C \git status --porcelain 2>/dev/null | \grep -q '^??'; then echo untracked; fi
is working fine here.

@jaesivsm
Copy link
Contributor Author

jaesivsm commented Mar 6, 2017

@j-degreef yeah, sorry about not writing anything about it but I came to the exact same conclusion yesterday; I updated my PR and that's what it now does.

Afaik (and could test) it's now working fine and is ready to merge

@Rycieos
Copy link
Collaborator

Rycieos commented Mar 6, 2017

Interestingly, I was unable to reproduce the original problem on machines without GNU grep. It is only on machines with GNU grep that have this issue. So much for grep portability...

Anyway, this patch now doesn't break anything on my machines, and it does fix the problem on the machines that had the problem, so this gets my approval.

@j-degreef
Copy link

j-degreef commented Mar 6, 2017

@Rycieos
Beware that on machines without GNU grep this solution may not work.
It all depends on the fact that grep interprets "?" as a normal character or a control one.
I would suggest you try this solution on your machine without GNU grep to see if it work.

@Rycieos
Copy link
Collaborator

Rycieos commented Mar 6, 2017

I'm not sure what version of grep my machine has (SunOS 11.3, no man pages, grep won't return a version), but I get this:

$ grep -E
grep: illegal option -- E
Usage: grep [-c|-l|-q] -bhinsvw pattern file . . .

As I said above, I have tested this patch on my systems with this lesser grep, and it works as intended.

@j-degreef
Copy link

Ok, my edit crossed your reply ;)

@dolmen dolmen merged commit feafb03 into liquidprompt:master Apr 4, 2017
dolmen added a commit that referenced this pull request Apr 4, 2017
@dolmen
Copy link
Collaborator

dolmen commented Apr 4, 2017

@Rycieos Do you have /bin/egrep or /usr/bin/egrep on that machine?

@Rycieos
Copy link
Collaborator

Rycieos commented Apr 4, 2017

Yes I do. For reference:

$ uname -a
SunOS hostname 5.11 11.3 sun4v sparc sun4v
$ grep --version
grep: illegal option -- version
Usage: grep [-c|-l|-q] -bhinsvw pattern file . . .
$ grep -E
grep: illegal option -- E
Usage: grep [-c|-l|-q] -bhinsvw pattern file . . .
$ grep -e
grep: illegal option -- e
Usage: grep [-c|-l|-q] -bhinsvw pattern file . . .
$ egrep --version
egrep: illegal option -- version
usage: egrep [ -bchilnsv ] [ -e exp ] [ -f file ] [ strings ] [ file ] ...
$ ls -l /usr/bin/*grep*
lrwxrwxrwx   1 root     root           8 Dec 22  2015 /usr/bin/bzegrep -> ./bzgrep
lrwxrwxrwx   1 root     root           8 Dec 22  2015 /usr/bin/bzfgrep -> ./bzgrep
-r-xr-xr-x   1 root     bin         1.6K Dec 22  2015 /usr/bin/bzgrep
-r-xr-xr-x   1 root     bin          39K Aug 31  2016 /usr/bin/egrep
-r-xr-xr-x   1 root     bin          21K Aug 31  2016 /usr/bin/fgrep
-r-xr-xr-x   1 root     bin          21K Aug 31  2016 /usr/bin/grep
-r-xr-xr-x   1 root     bin          126 Dec 22  2015 /usr/bin/gzegrep
-r-xr-xr-x   1 root     bin          126 Dec 22  2015 /usr/bin/gzfgrep
-r-xr-xr-x   1 root     bin         5.8K Dec 22  2015 /usr/bin/gzgrep
lrwxrwxrwx   1 root     root           6 Dec 22  2015 /usr/bin/lzegrep -> xzgrep
lrwxrwxrwx   1 root     root           6 Dec 22  2015 /usr/bin/lzfgrep -> xzgrep
lrwxrwxrwx   1 root     root           6 Dec 22  2015 /usr/bin/lzgrep -> xzgrep
-r-xr-xr-x   1 root     bin         426K Aug 31  2016 /usr/bin/msggrep
-r-xr-xr-x   1 root     bin          72K Aug 31  2016 /usr/bin/pcregrep
-r-xr-xr-x   2 root     bin          24K Aug 31  2016 /usr/bin/pgrep
-rwxr-xr-x   1 root     bin          69K Aug 31  2016 /usr/bin/pkgrepo
lrwxrwxrwx   1 root     root           6 Dec 22  2015 /usr/bin/xzegrep -> xzgrep
lrwxrwxrwx   1 root     root           6 Dec 22  2015 /usr/bin/xzfgrep -> xzgrep
-r-xr-xr-x   1 root     bin         5.1K Dec 22  2015 /usr/bin/xzgrep
-r-xr-xr-x   1 root     bin         2.9K Dec 22  2015 /usr/bin/zipgrep

Not sure how it could be used though in place of grep -E.

@dolmen
Copy link
Collaborator

dolmen commented Apr 4, 2017

Not sure how it could be used though in place of grep -E.

We could use egrep everywhere instead of grep -E (a single place actually). But the GNU grep man page says about egrep/fgrep/rgrep:

These variants are deprecated, but are provided for backward compatibility

@j-degreef
Copy link

Well, on Linux egrep is actually a wrapper around grep -E

#!/bin/sh
exec grep -E "$@"

@Rycieos
Copy link
Collaborator

Rycieos commented Apr 5, 2017

I don't think this is too pressing of a problem, since almost* anything that could be written in extended grep could also be written in normal grep.

* The | 'or' syntax is not a part of true normal grep.

The only remaining usage of grep -E is in a rarely used function. It looks like it couldn't be rewritten to use normal grep, since it uses the | symbol for or-ing. However, since that grep check can be disabled by disabling the sudo check in the config, it shouldn't be a problem.

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.

4 participants