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: counsel--git-grep-count-func-default submodule support #1558

Conversation

mikecrowe
Copy link
Contributor

@mikecrowe mikecrowe commented May 5, 2018

(counsel--git-grep-count-func-default): Ask Git where the .git directory
is for the repository in the current directory.

Modern Git puts all the git directories for submodules underneath the top
level .git directory and then leaves a "git-file" in the submodule which
points back to the repository inside the top-level .git directory. This
means, that within a submodule, the .git file contains something like:

gitdir: ../../.git/modules/submodule-repo-name

counsel--git-grep-count-func-default runs "du -s .git 2>/dev/null" which is
bound to give a very small answer for just this small file.

So, let's ask Git to tell us where the real .git directory is and calculate
the size of that.

counsel.el Outdated
@@ -1265,7 +1265,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 .git 2>/dev/null"))))
(read (shell-command-to-string "du -s `git rev-parse --git-dir` 2>/dev/null"))))
Copy link
Collaborator

@basil-conto basil-conto May 6, 2018

Choose a reason for hiding this comment

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

$() syntax is preferred over backticks; see https://unix.stackexchange.com/a/126928/230197.

Copy link
Collaborator

@basil-conto basil-conto May 6, 2018

Choose a reason for hiding this comment

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

You should also wrap the command substitution in double quotes to handle filenames with spaces in them; see the two answers linked in https://stackoverflow.com/a/37932117/3084001 for elaboration.

Copy link
Collaborator

@basil-conto basil-conto May 6, 2018

Choose a reason for hiding this comment

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

Another flexible way of calling git from Emacs is via vc-git-command.

Pros:

  • Allows users to specify their preferred vc-git-program.

  • Calls vc-git-program directly.

  • Allows flexible error ignoring/handling via the OKSTATUS argument:

    if OKSTATUS is nil, that means to ignore error status, if it is
    `async', that means not to wait for termination of the
    subprocess; if it is t it means to ignore all execution errors
    

Cons:

  • Requires ex- or implicitly loading vc-git.
  • No other part of Counsel uses vc/vc-git.

Example:

(with-temp-buffer
  (vc-git-command t t () "rev-parse" "--git-dir")
  (buffer-substring (point-min) (1- (point-max))))

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented May 6, 2018

As always, I would prefer (and be more than willing to provide a PR) to call du and git as processes directly to avoid shell incompatibilities, thus freeing users to use whichever shell they like; see #1502 for precedent on this.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented May 6, 2018

be more than willing to provide a PR

Please do. I think I've come around more to your point of view. I'd like to see how it looks like in code, trying to make it look as simple as we can, easily traceable back to a shell command.

(counsel--git-grep-count-func-default): Ask Git where the .git directory
is for the repository in the current directory.

Modern Git puts all the git directories for submodules underneath the top
level `.git` directory and then leaves a "git-file" in the submodule which
points back to the repository inside the top-level .git directory. This
means, that within a submodule, the `.git` file contains something like:

 gitdir: ../../.git/modules/submodule-repo-name

counsel--git-grep-count-func-default runs "du -s .git 2>/dev/null" which is
bound to give a very small answer for just this small file.

So, let's ask Git to tell us where the real .git directory is and calculate
the size of that.
@mikecrowe mikecrowe force-pushed the fix-git-grep-size-check-in-submodules branch from f9872ba to 23cd702 Compare Jun 5, 2018
@mikecrowe
Copy link
Contributor Author

@mikecrowe mikecrowe commented Jun 5, 2018

Is there any chance of merging my fix for this whilst we wait for @basil-conto's improved version? It's a pain having to patch this every time I upgrade. I've updated the commit to quote the argument and use $() rather than backticks as suggested by @basil-conto.

Thanks.

Mike.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Jun 5, 2018

Is there any chance of merging my fix for this

I think @abo-abo has been busy / on holiday / AFK for the past week or two, otherwise I'm sure this would have been merged. Thanks for being patient.

whilst we wait for @basil-conto's improved version?

I wouldn't call it an "improved version" so much as a larger refactor of how Counsel invokes subprocesses, so don't hold your breath. :) It's on my todo list, but it'll take a bit of design first, so I don't expect to have anything ready before the end of the month.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Jun 14, 2018

Merged, thanks.

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

3 participants