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--call): Touch-up #1828

Merged
merged 3 commits into from Nov 29, 2018

Conversation

@basil-conto
Copy link
Collaborator

commented 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: #1827

@basil-conto basil-conto force-pushed the basil-conto:blc/counsel-call branch from 4b54a4d to 653f0b2 Nov 28, 2018

counsel.el Outdated
(with-temp-buffer
(insert-file-contents stderr)
(buffer-substring-no-properties
(point) (line-end-position))))))))))

This comment has been minimized.

Copy link
@Alexander-Shukaev

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 28, 2018

Author Collaborator

point-min and point are equivalent here, point is simpler, and point-min conveys no more intention than point.

line-beginning-position conveys more intention in the context of a subsequent line-end-position, but that would be overkill.

I call the colour of this bikeshed point.

counsel.el (counsel--call): Touch-up
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: #1827

@basil-conto basil-conto force-pushed the basil-conto:blc/counsel-call branch from 653f0b2 to 40f7d58 Nov 28, 2018

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 28, 2018

What's the reason for using only the first line of the error message? I think if an error happens, we should get all the info we can get to debug it.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 28, 2018

What's the reason for using only the first line of the error message?

There's a subtle difference between error messages and error data. Error messages are inherently user-visible, and by convention comprise a single line that doesn't hog too much real-estate in the minibuffer and *Messages* buffer. Error data, on the other hand, can be more internal/mechanical.

My initial thought was to try to fit the error message on a single line, hence only including the first line of stderr, which ideally would provide enough context in the majority of cases. Only after this did I think of deconstructing the error message into error data that can be analysed more easily and mechanically, and I simply didn't think of bringing back the whole stderr message after that.

I think if an error happens, we should get all the info we can get to debug it.

Sure. It's just my knee-jerk reaction was that including verbose, multiline file contents in an Elisp error felt a bit awkward/non-idiomatic. So I propose three compromises:

  1. Bring back the entire stderr contents as the final data field of the file-error (a bit unusual, but it works)
  2. Store the entire stderr contents in a global variable which the caller can inspect/print as needed (kind of the same as (1))
  3. Strip down the file-error to its simplest parts, namely the command and its exit status (as per process-lines), and print the entire stderr contents to the *Warnings* buffer via lwarn or display-warning, using either the :debug or :warning severity levels (less unusual, more customisable, possibly overkill)

I wouldn't mind (1) or (2), but I kind of prefer (3) for its flexibility and losslessness (I don't think warnings buffers are automatically truncated like *Messages* is).

@Alexander-Shukaev

This comment has been minimized.

Copy link

commented Nov 28, 2018

I think you can simply offer ivy-debug kind of customization, in which case this error buffer will be available for inspection (but buried by default), while that logic with single line error message in minibuffer should stay because it's rather a conventional way of first-hand error reporting.

@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 28, 2018

I think you can simply offer ivy-debug kind of customization, in which case this error buffer will be available for inspection (but buried by default),

That's exactly how the built-in warning system works, which is why I'd like to reuse it; see (elisp) Warnings.

Simplify counsel--git-grep-count-func-default
counsel.el (counsel--git-grep-count-func-default): Simplify.
basil-conto added a commit to basil-conto/swiper that referenced this pull request Nov 29, 2018
counsel.el: (counsel--call): Warn about stderr
Bring back the previous behaviour[1] of returning the entire
contents of stderr, but this time put them in a warning, rather than
in the error data itself.

[1]: counsel.el (counsel--call): Add wrapper around call-process
  2018-11-28 11:07:54 +0100 9862640

Re: abo-abo#1828
@basil-conto

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 29, 2018

I have pushed a couple more commits to help illustrate my point, but will revert them if you disagree with the warning approach.

(let ((git-dir (counsel--call "git" "rev-parse" "--git-dir")))
(read (counsel--call "du" "-s" git-dir)))
(error 0))))
(or (unless (eq system-type 'windows-nt)

This comment has been minimized.

Copy link
@Alexander-Shukaev

Alexander-Shukaev Nov 29, 2018

Why Windows is excluded? MSYS2 has coreutils package and also available as standalone from gnuwin32. I think you better let it fail simply which would be consistent on either Unix or Windows if du is not available (what makes you think that there could not be a situation on Unix when du is not available?).

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 29, 2018

Author Collaborator

Why Windows is excluded?

Perhaps @abo-abo can explain; see 8f87f74.

I don't use Windows so I'm not sure what the best approach is.

This comment has been minimized.

Copy link
@abo-abo

abo-abo Nov 29, 2018

Owner

On Windows it's possible for the user to use https://git-scm.com/download/win, with no du available on the system. Plus process calls are a lot more expensive on Windows than on Linux. So I decided to go with piping the whole git tree into Emacs and do elisp filtering, rather than start a new process on each key press. Of course it will work poorly with a repo that has over 20k lines; it's a trade-off.

This comment has been minimized.

Copy link
@Alexander-Shukaev

Alexander-Shukaev Nov 29, 2018

Did you profile your statement about slow process calling on Windows? I believe that decision is not worth the hassle that it creates, namely platform support asymmetry and, as a result, convoluted implementation and additional maintenance. Most of those who use Emacs under Windows, will have either MSYS2 or Cygwin, which both provide du. If du is not there, then the call will simply fail and you can still fallback to your filtering algorithm. Otherwise, I see no reason to cut off Windows power users like this. As you admitted, your custom filtering will not perform well on large repos anyways and 20K is nothing by the way.

This comment has been minimized.

Copy link
@Alexander-Shukaev

Alexander-Shukaev Nov 29, 2018

Furthermore, may I ask why do you have to call du on each key press?

(let ((git-dir (counsel--call "git" "rev-parse" "--git-dir")))
(read (counsel--call "du" "-s" git-dir)))
(error 0))))
(or (unless (eq system-type 'windows-nt)

This comment has been minimized.

Copy link
@Alexander-Shukaev

Alexander-Shukaev Nov 29, 2018

swiper/counsel.el

Line 1214 in 9862640

"Store the line count in current repository.")

It's size in bytes, not line count isn't it? Perhaps change the documentation and rename it to ...-size instead to convey the intent in more standard way?

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 29, 2018

Author Collaborator

See my unanswered questions in #1827. I find the naming, documentation, and implementation of these features confusing, but I'm not sure how to proceed in changing them. Perhaps @abo-abo can advise.

Don't forget, though, that all of this counting is merely intended as a proxy, not as a hard and fast metric.

counsel.el Outdated
(setq res (list 'file-error
(mapconcat #'identity `(,@command "failed") " ")
res))
(lwarn 'ivy :warning "%s"

This comment has been minimized.

Copy link
@Alexander-Shukaev

Alexander-Shukaev Nov 29, 2018

I think it's an :error (it already failed and we raise).

This comment has been minimized.

Copy link
@basil-conto

basil-conto Nov 29, 2018

Author Collaborator

The process indeed encountered an error, but that doesn' necessarily translate as an error with Ivy. I'm not disagreeing with you, I'm just playing devil's advocate. I'd be happy with any one of :debug, :warning, or :error. The only thing I'm concerned about is that, the higher the severity, the higher the chance of annoying users, e.g. with false positives.

Here's the description of the different severity levels, for posterity:

‘:emergency’
     A problem that will seriously impair Emacs operation soon if you do
     not attend to it promptly.
‘:error’
     A report of data or circumstances that are inherently wrong.
‘:warning’
     A report of data or circumstances that are not inherently wrong,
     but raise suspicion of a possible problem.
‘:debug’
     A report of information that may be useful if you are debugging.

I'll let someone else decide.

counsel.el Outdated Show resolved Hide resolved
counsel.el Outdated Show resolved Hide resolved
basil-conto added a commit to basil-conto/swiper that referenced this pull request Nov 29, 2018
counsel.el (counsel--call): Remove stderr guards
delete-file does not signal an error when given a non-existent file,
and guarding insert-file-contents is at best unnecessary and at
worst a race condition.

Re: abo-abo#1828

@basil-conto basil-conto force-pushed the basil-conto:blc/counsel-call branch from 5dd66d5 to b67b3be Nov 29, 2018

basil-conto added a commit to basil-conto/swiper that referenced this pull request Nov 29, 2018
counsel.el: (counsel--call): Warn about stderr
Bring back the previous behaviour[1] of returning the entire
contents of stderr, but this time put them in a warning, rather than
in the error data itself.

Also remove racey file-{readable,exists}-p guards for the stderr
file.  delete-file is able to handle non-existent files, and any
errors that insert-file-contents encounters can be used in place of
the stderr contents.

[1]: counsel.el (counsel--call): Add wrapper around call-process
  2018-11-28 11:07:54 +0100 9862640

Re: abo-abo#1828

@basil-conto basil-conto force-pushed the basil-conto:blc/counsel-call branch from b67b3be to fb126fb Nov 29, 2018

basil-conto added a commit to basil-conto/swiper that referenced this pull request Nov 29, 2018
counsel.el: (counsel--call): Warn about stderr
Bring back the previous behaviour[1] of returning the entire
contents of stderr, but this time put them in a warning, rather than
in the error data itself.

Also remove racey file-{readable,exists}-p guards for the stderr
file.  delete-file is able to handle non-existent files, and any
errors that insert-file-contents encounters can be used in place of
the stderr contents.

[1]: counsel.el (counsel--call): Add wrapper around call-process
  2018-11-28 11:07:54 +0100 9862640

Re: abo-abo#1828
counsel.el: (counsel--call): Warn about stderr
Bring back the previous behaviour[1] of returning the entire
contents of stderr, but this time put them in a warning, rather than
in the error data itself.

Also remove racey file-{readable,exists}-p guards for the stderr
file.  delete-file is able to handle non-existent files, and any
errors that insert-file-contents encounters can be used in place of
the stderr contents.

Also fix off-by-one error when returning empty output from a
successful command.

[1]: counsel.el (counsel--call): Add wrapper around call-process
  2018-11-28 11:07:54 +0100 9862640

Re: #1828

@basil-conto basil-conto force-pushed the basil-conto:blc/counsel-call branch from fb126fb to 33d8e96 Nov 29, 2018

@abo-abo abo-abo merged commit 33d8e96 into abo-abo:master Nov 29, 2018

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Nov 29, 2018

Thanks.

@Alexander-Shukaev

This comment has been minimized.

Copy link

commented Nov 29, 2018

GJ, looks solid.

@basil-conto basil-conto deleted the basil-conto:blc/counsel-call branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.