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-git-grep sometimes searches for any text without a prompt #1402

Closed
ryankask opened this issue Jan 10, 2018 · 26 comments
Closed

counsel-git-grep sometimes searches for any text without a prompt #1402

ryankask opened this issue Jan 10, 2018 · 26 comments

Comments

@ryankask
Copy link

@ryankask ryankask commented Jan 10, 2018

Sometimes when running counsel-git-grep, the command appears to search for any text without first prompting:

2018-01-10 at 15 12

I would normally expect to see this prompt:

2018-01-10 at 15 13

I see screenshot one when I run the command for the first time after creating a new buffer or switching to an existing one. Subsequent calls prompt as in the second screenshot. It's not always the case though but I haven't noticed a pattern.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 10, 2018

I can't do much without a reproduction scenario.

That said, the idea is:

  • if the repo line count is below 20k lines, pipe all lines into Emacs and use Elisp filtering, which is pretty fast in that case.
  • otherwise, let git-grep do the filtering for each input and send the results to Emacs.

It seems you're getting case 1, even though your repo line count is above 20k lines.

Try evaling (counsel--gg-count "" t) to see if the result ever gets messed up.

Loading

@ryankask
Copy link
Author

@ryankask ryankask commented Jan 10, 2018

Thanks for the tip. I used emacs -Q -l ... and saw it happening as well.

Running (counsel--gg-count "" t) after switching to the buffer returns 6269 at first. Then, after running consel-git-grep aain, (counsel--gg-count "" t) evaluates to 113081.

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 10, 2018

This is the shell command that runs in counsel--gg-count:

git --no-pager grep -c -n --no-color -i -e "" | sed 's/.*:\(.*\)/\1/g' | awk '{s+=$1} END {print s}'

Can you check what it returns for you?

Loading

@ryankask
Copy link
Author

@ryankask ryankask commented Jan 10, 2018

I see that the lower count is the result of the above command running in the buffer's default-directory. The higher count is the result of running the command from the root of the repository.

I forgot to mention that I recently upgraded to emacs 26.0.90 so this could be causing problems.

Loading

@abo-abo abo-abo closed this in 384ffff Jan 10, 2018
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 10, 2018

Thanks, please check if it's fixed for you.

Loading

@ryankask
Copy link
Author

@ryankask ryankask commented Jan 11, 2018

Thanks for your work on this. It didn't seem to solve the issue. counsel--gg-count assigns default-directory to (ivy-state-directory ivy-last) and it's this value that changes between the parent directory of the buffer's file and the git repository root path.

Loading

@ryankask
Copy link
Author

@ryankask ryankask commented Jan 12, 2018

@abo-abo Would it be possible to reopen this issue?

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 12, 2018

OK. Please provide a minimal repro if you can. I wasn't sure I fixed it last time.

Loading

@abo-abo abo-abo reopened this Jan 12, 2018
@ryankask
Copy link
Author

@ryankask ryankask commented Jan 12, 2018

Here is a way to reproduce the issue:

  1. Clone a larger git repository git clone https://github.com/facebook/react.git
  2. Run make plain
  3. Open $REPO_DIR/react/packages/react/index.js
  4. Run counsel-git-grep
  5. Screenshot shows the result for me

2018-01-12 at 10 11

Loading

ryankask added a commit to ryankask/dotfiles that referenced this issue Jan 19, 2018
@dfrib
Copy link

@dfrib dfrib commented Jan 29, 2018

+1 for also seeing this issue, sporadically, in a repo with <= 20 000 lines, using Emacs 25.3. Possible a regression as I've only ever run into it after my latest cask packages wipe/reinstall. Currently using swiper-20180101.1035.

Setting counsel-git-grep-skip-counting-lines to non-nil (as show by @ryankask in his linked commit) seems to fix it, though.

(use-package counsel
  :after ...
  :bind (...
         ("C-c j" . 'counsel-git-grep)
         ...)
  :init (setq counsel-git-grep-skip-counting-lines t)
  )

... if the repo line count is below 20k lines, pipe all lines into Emacs and use Elisp filtering, which is pretty fast in that case.

For this case (which should be present for my repo) it would seems as if (counsel-more-chars 3) in counsel.el does not take precedence in case counsel-git-grep-skip-counting-lines is nil, as directly upon invocation of counsel-git-grep a huge list is dumped in the minibuffer. Maybe (split-string (shell-command-to-string cmd) "\n" t) is a participating culprit; overwriting the light-weight fake candidates from counsel-more-chars with a (non-lazy?) large list of sub-strings?

Loading

@ideasman42
Copy link
Contributor

@ideasman42 ideasman42 commented Jan 31, 2018

Running into this problem recently too.

Why is (counsel--gg-count "" t) needed? - it's taking a long time on 4,090,614 lines of code in our project.
As well as showing part of the results in the output (which is the bug).

This fails quite often too with an error:

Debugger entered--Lisp error: (wrong-type-argument stringp nil)
  find-file-name-handler(nil process-file)
  process-file("/bin/zsh" nil t nil "-c" "git --no-pager grep -c -n --no-color -e \"\" -- 'CMakeLists.txt' '*.cmake' | sed 's/.*:\\(.*\\)/\\1/g' | awk '{s+=$1} END {print s}'")
  shell-command-to-string("git --no-pager grep -c -n --no-color -e \"\" -- 'CMakeLists.txt' '*.cmake' | sed 's/.*:\\(.*\\)/\\1/g' | awk '{s+=$1} END {print s}'")
  counsel--gg-count("" t)
  counsel-git-grep(nil)
  funcall-interactively(counsel-git-grep nil)
  call-interactively(counsel-git-grep nil nil)
  command-execute(counsel-git-grep)

Setting counsel-git-grep-skip-counting-lines resolves this, but I had to read the code before I knew it existed.

Loading

@abo-abo abo-abo closed this in e8f2de5 Jan 31, 2018
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jan 31, 2018

@ideasman42

Setting counsel-git-grep-skip-counting-lines resolves this, but I had to read the code before I knew it existed.

Thanks for the feedback, I removed counsel-git-grep-skip-counting-lines; now du is used to estimate the repo size - it's much faster.

@ryankask

clone a larger git repository git clone https://github.com/facebook/react.git

I just tried it and can no longer reproduce. Please test again.

Loading

@ryankask
Copy link
Author

@ryankask ryankask commented Feb 1, 2018

@abo-abo I tried this but it doesn't work on my machine. Please see the screenshots below.

I'd actually preferred just using counsel-git-grep-skip-counting-lines. The issue I was having wasn't related to speed but it was that the lines were being counted from the current directory rather than from the root of the repository. If the total number of lines counted for files in the buffer's directory was less than 20,000, it wouldn't use git-grep even though the whole repository had more than 20,000 lines. In other words, the context of the check wasn't correct. Sometimes it was though because (ivy-state-directory ivy-last) would return the root of the repo after the first run of counsel-git-grep.

2018-02-01 at 09 20

2018-02-01 at 09 21

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Feb 1, 2018

I tried this but it doesn't work on my machine. Please see the screenshots below.

I can't reproduce it on Ubuntu 16, Emacs 26.0.90. What are the software versions on your machine?

It looks like the problem is different this time: the async version is used (which is what you wanted), but it crashes for some reason? What are your reproduction steps for the last screenshots? Is it reliably reproduced?

Loading

@abo-abo abo-abo reopened this Feb 1, 2018
@ryankask
Copy link
Author

@ryankask ryankask commented Feb 2, 2018

In a previous comment I wrote a list of steps to reproduce. I followed that again.

My version: GNU Emacs 26.0.91 (build 1, x86_64-apple-darwin16.7.0, NS appkit-1504.83 Version 10.12.6 (Build 16G1114)) of 2018-01-22

Could it be related to the fact that I have an alias to use human readable sizes?

$ which du
du: aliased to du -kh

What I meant was that I was happy disabling the counting feature and always using git-grep. I can continue testing these issues, however, since that option was removed.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Feb 2, 2018

@ryankask

Could it be related to the fact that I have an alias to use human readable sizes?

Unless you are somehow making sure that Emacs defines the alias in its shell invocations as well, this shouldn't come into play. You can verify it yourself with M-:(shell-command-to-string "alias du")RET.

Loading

@ryankask
Copy link
Author

@ryankask ryankask commented Feb 2, 2018

This only seems to be happening when I start emacs using make plain in the swiper directory and the project has over 20,000 lines.

It doesn't happen with my normal configuration. It could be a bug in emacs? I'll try the previous 26 pre release later.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Feb 2, 2018

It doesn't happen with my normal configuration. It could be a bug in emacs? I'll try the previous 26 pre release later.

If the issue occurs with make plain but not when you load your configuration, all that means is there is something in your configuration which ensures your Emacs is in the right state/environment for this feature to work, not necessarily that there is a bug with Emacs (though there could very well be a subtle platform-specific bug in play).

It would help if you could recursively bisect your configuration and identify which settings make this feature work for you. As a wild guess, you may be setting some shell/process environment variables such as exec paths.

Loading

dfrib pushed a commit to dfrib/emacs_setup that referenced this issue Feb 13, 2018
Temporary fix to redeem regression in counsel-git-grep, where it would
start grepping at 0 characters (enumerating every line in the repo
into the mini-buffer).

See (currently) open ticket for details:
abo-abo/swiper#1402
@ssnnoo
Copy link

@ssnnoo ssnnoo commented Mar 19, 2019

I run into a problem that (with the git setup we have at work) in the worktree dir this command gives the correct amount of lines
git --no-pager grep -c -n --no-color -i -e "" | sed 's/.:(.)/\1/g' | awk '{s+=$1} END {print s}'
6945293

However,

git rev-parse --git-dir

points to:
/home/yyy/reps.git/worktrees/xxx which is small because this is not where the actual source code is, i.e. counsel-git-grep thinks the rep is small ($GIT_DIR is not set).

As a workaround I set

counsel--git-grep-count-threshold 1

would git rev-parse --show-toplevel be an option instead of --git-dir? Or re-introduce counsel-git-grep-skip-counting-lines (I think it has been removed now).

Loading

abo-abo added a commit that referenced this issue Mar 19, 2019
* counsel.el (counsel--git-grep-count-func):
(counsel--git-grep-count-func-default): Add obsolete aliases.

Re #1402
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 19, 2019

The new setting to skip counting lines is:

(setq counsel-git-grep-count-function (lambda () most-positive-fixnum))

counsel-git-grep-count-function is now defined as a defcustom with 3 possible choices for behavior.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Mar 19, 2019

Note that Emacs 27 comes with bignum support, so though unlikely, most-positive-fixnum is not guaranteed to be larger than any given threshold.

Loading

basil-conto added a commit to basil-conto/swiper that referenced this issue Mar 20, 2019
Fix quoting in custom :type and clarify documentation.
Declare obsolete varaliases before their referents.

Re: abo-abo#1402
abo-abo added a commit that referenced this issue Mar 21, 2019
Fix quoting in custom :type and clarify documentation.
Declare obsolete varaliases before their referents.

Re: #1402
Fixes #1975
@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 26, 2019

See also mainly #1912 (and maybe #1908). I keep counsel--git-grep-count-threshold as 0 for now because

  1. it ensures the use of ivy-more-chars (which may save your ass on a repository with extremely long lines);
  2. it ensures the use of counsel--async-command making minibuffer population with candidates non-blocking.

In general, I don't see how could heuristics of counsel-git-grep-count-function and counsel-git-grep-count-function-du be useful in any case as specialized external search tool such as git grep will always be faster at filtering and more responsive if invoked asynchronously from UI experience. These statements are easily extrapolated to other external search tools (hence #1908).

Loading

@abo-abo abo-abo closed this in adab07d Mar 27, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 27, 2019

Thanks all. counsel-git-grep is now always async.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 27, 2019

Awesome! And now if we also apply the same as a041fd6 from #1801, then we could potentially close #1812.

The only missing point would be what I mentioned about application of ranking/sorting algorithm on top of fuzzy search result candidates returned by external search tools in the last comment to #1812. But that part is a general feature applicable to all the external search tools (e.g. rg and ag too), where a041fd6 is either already applied or would apply, so we could have a separate ticket for that feature.

Loading

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 27, 2019

@Alexander-Shukaev Please PR or open a new issue with a summary of what you want to do. I don't want to dig through several threads to understand what needs to be done.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 27, 2019

@abo-abo, my paper assignment is still in progress. I do these updates mostly to document the development progress and dependencies between different issues, in order to myself follow what's going on and be able to take up the remaining open issues when I get the assignment (if not addressed by some volunteer before that happens).

Loading

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* counsel.el (counsel--git-grep-count-func):
(counsel--git-grep-count-func-default): Add obsolete aliases.

Re abo-abo#1402
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Fix quoting in custom :type and clarify documentation.
Declare obsolete varaliases before their referents.

Re: abo-abo#1402
Fixes abo-abo#1975
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
Now all filtering will be done by git-grep, so it will work exactly
like `counsel-ag'.

Fixes abo-abo#1402
Fixes abo-abo#1912
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants