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

counsel-git-grep: Breaks when filenames contain colons #153

Closed
hpdeifel opened this Issue Jun 18, 2015 · 9 comments

Comments

Projects
None yet
2 participants
@hpdeifel

hpdeifel commented Jun 18, 2015

The first colon seems to be taken as the separating character, but file names can contain colons. A current real life example for me were email files in an mbox.

M-x grep seems to handle the case correctory

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Jun 19, 2015

Owner

I don't understand what you mean here. I created a test repo, committed a file with a colon in it (weird, but fine). I'm not getting any bug when the input contains colons.

Owner

abo-abo commented Jun 19, 2015

I don't understand what you mean here. I created a test repo, committed a file with a colon in it (weird, but fine). I'm not getting any bug when the input contains colons.

@hpdeifel

This comment has been minimized.

Show comment
Hide comment
@hpdeifel

hpdeifel Jun 19, 2015

In a git repo with the file foo:bar, counsel-git-grep opens the file foo, when hitting RET on a matching line.

hpdeifel commented Jun 19, 2015

In a git repo with the file foo:bar, counsel-git-grep opens the file foo, when hitting RET on a matching line.

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Jun 19, 2015

Owner

I see it now, thanks. But how do you propose I fix this? git grep can't customize the separator.
I can put a regex there that tries to match a line number, but still it will be buggy if you manage to name a file to include a semicolon and a number.

Owner

abo-abo commented Jun 19, 2015

I see it now, thanks. But how do you propose I fix this? git grep can't customize the separator.
I can put a regex there that tries to match a line number, but still it will be buggy if you manage to name a file to include a semicolon and a number.

@hpdeifel

This comment has been minimized.

Show comment
Hide comment
@hpdeifel

hpdeifel Jun 19, 2015

It seems to have a --null option, just like grep. I don't know if that is usable from elisp, but it's the usual way to treat these ambiguities .

hpdeifel commented Jun 19, 2015

It seems to have a --null option, just like grep. I don't know if that is usable from elisp, but it's the usual way to treat these ambiguities .

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Jun 19, 2015

Owner

--null doesn't solve it:

[~/Downloads/test]$ git --no-pager grep --full-name -n --no-color -i -e "oo"
as:df:1:test:foo:bar
as:df:2:helloooo
[~/Downloads/test]$ git --no-pager grep --full-name -n --no-color -i -e "oo" --null
as:df1test:foo:bar
as:df2helloooo

But matching for a regex should work 99% of the time. I'll push the change shortly.

Owner

abo-abo commented Jun 19, 2015

--null doesn't solve it:

[~/Downloads/test]$ git --no-pager grep --full-name -n --no-color -i -e "oo"
as:df:1:test:foo:bar
as:df:2:helloooo
[~/Downloads/test]$ git --no-pager grep --full-name -n --no-color -i -e "oo" --null
as:df1test:foo:bar
as:df2helloooo

But matching for a regex should work 99% of the time. I'll push the change shortly.

@abo-abo abo-abo closed this in 5fcdfb4 Jun 19, 2015

@hpdeifel

This comment has been minimized.

Show comment
Hide comment
@hpdeifel

hpdeifel Jun 19, 2015

I think --null does solve it:

% git --no-pager grep --full-name -n --no-color -i -e "test" --null | xxd
00000000: 666f 6f3a 6261 7200 3100 7468 6973 2069  foo:bar.1.this i
00000010: 7320 6120 7465 7374 0a                   s a test.

as you can see, there is a 0x00 byte after the file name, which cannot appear as part of it.

I don't know how M-x grep does it, but it works in this case.

hpdeifel commented Jun 19, 2015

I think --null does solve it:

% git --no-pager grep --full-name -n --no-color -i -e "test" --null | xxd
00000000: 666f 6f3a 6261 7200 3100 7468 6973 2069  foo:bar.1.this i
00000010: 7320 6120 7465 7374 0a                   s a test.

as you can see, there is a 0x00 byte after the file name, which cannot appear as part of it.

I don't know how M-x grep does it, but it works in this case.

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Jun 19, 2015

Owner

0x00 byte after the file name

This doesn't work well: I have to use split-string on the output of start-process eventually. And it bugs out because of the null byte. Just try the current code. I'm sure it's sufficient.

Owner

abo-abo commented Jun 19, 2015

0x00 byte after the file name

This doesn't work well: I have to use split-string on the output of start-process eventually. And it bugs out because of the null byte. Just try the current code. I'm sure it's sufficient.

@hpdeifel

This comment has been minimized.

Show comment
Hide comment
@hpdeifel

hpdeifel Jun 19, 2015

Yep, the new code works very well. Thank you!

It still itches me that it's theoretically possible to be buggy (now with filenames that contain colons and numbers). I just tried

ELISP> (split-string (shell-command-to-string  "git --no-pager grep --full-name -n --no-color --null -i -e \"test\"") "\0")
("foo:bar" "1" "this is a test\n")

which seems to work very well.

But please don't bother. It's fine the way it's implemented, if I really need to scratch my itch, I'll create a pull request.

hpdeifel commented Jun 19, 2015

Yep, the new code works very well. Thank you!

It still itches me that it's theoretically possible to be buggy (now with filenames that contain colons and numbers). I just tried

ELISP> (split-string (shell-command-to-string  "git --no-pager grep --full-name -n --no-color --null -i -e \"test\"") "\0")
("foo:bar" "1" "this is a test\n")

which seems to work very well.

But please don't bother. It's fine the way it's implemented, if I really need to scratch my itch, I'll create a pull request.

@abo-abo

This comment has been minimized.

Show comment
Hide comment
@abo-abo

abo-abo Jun 19, 2015

Owner

which seems to work very well.

That isn't the problem. The problem I think is split-string ... "\n": the \0 chars interfere there.

But please don't bother. It's fine the way it's implemented, if I really need to scratch my itch, I'll create a pull request.

Fine, thanks.

Owner

abo-abo commented Jun 19, 2015

which seems to work very well.

That isn't the problem. The problem I think is split-string ... "\n": the \0 chars interfere there.

But please don't bother. It's fine the way it's implemented, if I really need to scratch my itch, I'll create a pull request.

Fine, thanks.

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