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

provide hooks to customize counsel-git-grep behavior on large repos #1470

Closed

Conversation

articuluxe
Copy link

@articuluxe articuluxe commented Feb 20, 2018

I hope these (non-breaking) changes are OK. The current solution (du -s) can take from 7 - 24 sec. on a medium-size repo, which already takes so long that it doesn't matter that we limit the minimum input to 3 characters. These changes just provide a customization point for users to change this behavior.

counsel.el Outdated
"Default defun to calculate `counsel--git-grep-count'."
(if (eq system-type 'windows-nt)
0
(read (shell-command-to-string "du -s"))))
Copy link
Collaborator

@basil-conto basil-conto Feb 20, 2018

Choose a reason for hiding this comment

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

It's about twice as fast calling

(read (car (process-lines "du" "-s")))

plus you avoid the shell, unless that was intentional for some reason.

Copy link
Author

@articuluxe articuluxe Feb 21, 2018

Choose a reason for hiding this comment

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

Thanks, seems to work. I've pushed this change.

counsel.el Outdated
@@ -1256,7 +1256,7 @@ files in a project.")
"Default defun to calculate `counsel--git-grep-count'."
(if (eq system-type 'windows-nt)
0
(read (shell-command-to-string "du -s"))))
Copy link
Owner

@abo-abo abo-abo Feb 21, 2018

Choose a reason for hiding this comment

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

What makes this more efficient?

Copy link
Collaborator

@basil-conto basil-conto Feb 21, 2018

Choose a reason for hiding this comment

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

@abo-abo Calling du directly instead of via the shell cuts out the middle man (the shell).

I just noticed that shell-command-to-string calls process-file (which inspects file handlers) instead of call-process.

It is possible to keep this behaviour without calling the shell, thus still halving subprocess overhead:

(read (with-output-to-string
        (process-file "du" nil standard-output nil "-s")))

Obviously this overhead reduction is completely negligible compared to the execution time of du. My comment was more about avoiding the shell where it's not needed than making any noticeable optimisation.

Copy link
Owner

@abo-abo abo-abo Feb 22, 2018

Choose a reason for hiding this comment

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

I agree that not piping it all through the shell is a bit more efficient.

On the other hand, using the shell is a lot more flexible:

  • You can copy paste the command into an actual shell to test it
  • You can add/remove flags more easily, since it's just in the shell format

Copy link
Collaborator

@basil-conto basil-conto Feb 22, 2018

Choose a reason for hiding this comment

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

@abo-abo Those aren't the arguments in favour of the shell's flexibility that I would have made (think piping, logical operators...), but agreed, the shell is of course more flexible (and thus problematic when users use anything other than compliant bourne/bash shells with corresponding Unix utils).

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 22, 2018

Thanks, I merged the first commit. I think not merging the second one keeps things more simple.

@articuluxe
Copy link
Author

@articuluxe articuluxe commented Feb 22, 2018

Thanks for merging.

In case anyone is curious, my local change to require 3 chars for counsel-git-grep, regardless of repo size, is to execute the following after 'counsel is loaded (e.g. in use-package's :config stanza):

   (setq counsel--git-grep-count-func
         (lambda() (1+ counsel--git-grep-count-threshold)))

@icarus-sparry
Copy link

@icarus-sparry icarus-sparry commented Mar 24, 2018

Please see issue #1502 where a suggested fix uses the flexibility of the shell to at least stop counsel-git-grep from failing.

basil-conto added a commit to basil-conto/swiper that referenced this issue Nov 28, 2018
(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
abo-abo pushed a commit that referenced this issue Nov 28, 2018
(counsel--git-grep-count-func-default): Call git and du directly to
avoid shell incompatibilities in redirection.

Re: #1470, #1558
Fixes #1502
Fixes #1827
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

4 participants