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

counsel.el: Call du process directly #1827

Closed
wants to merge 1 commit into from

Conversation

basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Nov 28, 2018

(counsel--git-grep-count-func-default): Call git and du directly to avoid shell incompatibilities in redirection.

Cc: @articuluxe, @icarus-sparry, @mikecrowe, @shalin24
Re: #1470, #1558
Fixes #1502

P.S. Sorry for not commenting on this at the time, but why do counsel--git-grep-count, counsel--git-grep-count-func, and counsel--git-grep-count-func-default have double hyphens in their names, if they're intended as a means for (advanced) user customisation?

Re: #1470 (comment)
Wouldn't it be cleaner to allow values of ivy-more-chars-alist to be functions, so that ivy-more-chars is no longer guarded ad-hoc by (> counsel--git-grep-count counsel--git-grep-count-threshold) in counsel-git-grep-function?

(counsel--git-grep-count-func-default): Call git and du directly to
avoid shell incompatibilities in redirection.

Re: abo-abo#1470, abo-abo#1558
Fixes abo-abo#1502
@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Nov 28, 2018

As was discussed in #1470, #1502, and #1558, it would be nice to create a uniform, flexible, and human-readable wrapper around process invocation, but I think it's more pressing to solve the current issue first and worry about refactoring later.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Nov 28, 2018

Thanks. Please also have a look at the new wrapper. I used call-process, but maybe we should use process-file.

basil-conto added a commit to basil-conto/swiper that referenced this issue Nov 28, 2018
Use process-file instead of call-process.  Add docstring.  Avoid
multi-line error message by returning only the first line of stderr.
Deconstruct error information as file-error data for easier access.
Guard stderr file access with file-{exists,readable}-p just in case.

Re: abo-abo#1827
@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Nov 28, 2018

Please also have a look at the new wrapper.

Thanks, I've submitted a few suggestions in #1828.

I used call-process, but maybe we should use process-file.

I think so too, given shell commands obey file handlers.

basil-conto added a commit to basil-conto/swiper that referenced this issue Nov 28, 2018
Use process-file instead of call-process.  Add docstring.  Avoid
multi-line error message by returning only the first line of stderr.
Deconstruct error information as file-error data for easier access.
Guard stderr file access with file-{exists,readable}-p just in case.

Re: abo-abo#1827
basil-conto added a commit to basil-conto/swiper that referenced this issue Nov 28, 2018
Use process-file instead of call-process.  Add docstring.  Avoid
multi-line error message by returning only the first line of stderr.
Deconstruct error information as file-error data for easier access.
Guard stderr file access with file-{exists,readable}-p just in case.

Re: abo-abo#1827
@basil-conto basil-conto deleted the blc/du branch Nov 30, 2018
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

2 participants