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

Fix obscure bug with sorting zk-index buffer by size #64

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

boyechko
Copy link
Contributor

The bug is rare, but if zk-id-regexp happens to match a portion of zk-index buffer that is not a zk-id, then zk-index--current-id-list will include it in its returned list, which in turn is used in zk-index--current-file-list to return a list of files. But since it's not a zk-id, zk--parse-id will return nil, resulting in zk-index--current-file-list returning a list with at least one nil element. This becomes a problem in zk-index--sort-size, since it uses > (greater) function to compare file sizes, which signals an error if an argument is nil. However, file-attributes (and file-attribute-size) can return nil if one is passed to them or if the file does not exist (see Emacs Lisp manual about "File Attributes").

Additionally, zk-index--format-candidates does not expect nils in the list of the files it works with, which again causes an error when string-match ends up being passed a nil rather than a string.

The fix involves having 1) zk-index--sort-size ensure that it's always passing numbers to >, but since that just propagates the nil inside the list of files returned by zk-index--sort, we also make sure 2) zk-index--format-candidates does not pass nil to string-match.

Another possibility would be to have zk-index--current-file-list remove nils from the file list it returns (replacing files with (delq nil files) as the final line), but that results in a small performance loss because delq needs to traverse the list looking for nils. I think my two-pronged fix is more robust, anyway, since it deals with the errors at the level where they occur (in the calls to > and string-match). Of course, we can also wrap those calls in ignore-errors, but that might make debugging more difficult in the future.

The bug is rare, but if `zk-id-regexp` happens to match a portion of zk-index
buffer that is *not* a zk-id, then `zk-index--current-id-list` will include it
in its returned list, which in turn is used in `zk-index--current-file-list` to
return a list of files. But since it's not a zk-id, `zk--parse-id` will return
nil, resulting in `zk-index--current-file-list` returning a list with at least
one nil element. This becomes a problem in `zk-index--sort-size`, since it uses
`>` (greater) function to compare file sizes, which signals an error if an
argument is nil. However, `file-attributes` (and `file-attribute-size`) can
return `nil` if one is passed to them or if the file does not exist (see [Emacs
Lisp manual about "File Attributes"][1]).

Additionally, `zk-index--format-candidates` does not expect nils in the list of
the files it works with, which again causes an error when `string-match` ends up
being passed a nil rather than a string.

The fix involves having 1) `zk-index--sort-size` ensure that it's always passing
numbers to `>`, but since that just propagates the nil inside the list of files
returned by `zk-index--sort`, we also make sure 2) `zk-index--format-candidates`
does not pass nil to `string-match`.

Another possibility would be to have `zk-index--current-file-list` remove nils
from the file list it returns (replacing `files` with `(delq nil files)` as the
final line), but that results in a small performance loss because `delq` needs
to traverse the list looking for nils. I think my two-pronged fix is more
robust, anyway, since it deals with the errors at the level where they
occur (in the calls to `>` and `string-match`). Of course, we can also wrap
those calls in `ignore-errors`, but that would make debugging more difficult in
the future.

[1]: https://www.gnu.org/software/emacs/manual/html_node/elisp/File-Attributes.html
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.

1 participant