Skip to content

Conversation

@krobelus
Copy link
Contributor

@krobelus krobelus commented Sep 5, 2021

Description

Fixes issue #8264

TODOs:

  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@krobelus krobelus added this to the fish 3.4.0 milestone Sep 5, 2021
}
ssize_t bufsize = buf.st_size + 1;
char target_buf[bufsize];
const std::string tmp = wcs2string(file_name);
Copy link
Member

Choose a reason for hiding this comment

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

(note: We really really do way too much of this wcs2string and back - this will do wcs2string for the lwstat above, and then wcs2string again for the readlink. In some benchmarks ~50% of the time is taken in widening and narrowing strings)

(but since this is just for the error, it's okay for now - we should just probably start doing all the path stuff in narrow exclusively, so you can keep one narrow string around if you need to do more than one thing)

src/wutil.cpp Outdated
file_id_t file_id_for_fd(const autoclose_fd_t &fd) {
return file_id_for_fd(fd.fd());
}
file_id_t file_id_for_fd(const autoclose_fd_t &fd) { return file_id_for_fd(fd.fd()); }
Copy link
Member

Choose a reason for hiding this comment

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

This line wasn't touched, at all. That's the issue with running clang-format on save - you touch a lot of unrelated code.

We should probably do another formatting run (but changes like this are the worst - clang-format is super anal about having lines exactly the correct way, which is just useless churn and should be disabled, but I've so far not found a way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but changes like this are the worst - clang-format is super anal about having lines exactly the correct way, which is just useless churn and should be disabled, but I've so far not found a way

I'm not sure what you mean exactly - it's the formatter's job to be precise, so we don't have to.
This style can be configured with AllowShortFunctionsOnASingleLine, see also https://zed0.co.uk/clang-format-configurator/. I somehow have lots of energy for evangelizing clang-format.. it makes refactorings much easier and avoids thinking or communicating about style.

Assuming we want this change, I'd probably still do it because it's small and I'm already touching the file / invalidating builds. Though that's really not a strong reason.

Copy link
Member

@faho faho Sep 14, 2021

Choose a reason for hiding this comment

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

The problem is that it's annoyingly precise.

We've had cases where it flip-flopped. E.g. rename this variable to "file_desc" and it might just decide that it has to be split. Rename it again to "file_des" and it might be forced on one line. That's reformatting that basically helps nobody. Line length really only matters if lines constantly go over by a lot which requires you to scroll to view the lines you care about.

And because this particular kind of reformatting isn't whitespace-only, git blame -w won't ignore it and will show you the reformatting commit first, so you then need to run it again with somehash^. There's a way to give git blame commits to ignore, but that also only really works if the commits only reformat (or you could be ignoring a functional change!).

So I'm really quite annoyed by clang-format's super precise insistence on line length and would much prefer if it had some sort of slack where it keeps whatever's there, because as it stands it either forces joined style or forces separated style. (that's what I meant by "disabling" it - currently you can only say "always separate" or "separate exactly when clang-format decides to")

And if we can't have that, we could at least restyle the entire codebase every so often in separate commits and add their hashes to some file so I can run git blame -w --ignore-revs-file :/restylecommits.txt (or set "blame.ignoreRevsFile") to ignore them, and stop restyling unrelated stuff in regular commits.

Copy link
Member

Choose a reason for hiding this comment

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

Note: I don't particularly care about this one occurence, it's more that I don't want commits to have unrelated formatting changes so git blame is nicer to use, and that I consider clang-format to not be a great autoformatter because it is annoyingly precise, which causes it to add a bunch of formatting churn.

Copy link
Contributor Author

@krobelus krobelus Sep 21, 2021

Choose a reason for hiding this comment

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

Thanks for clarifying.

Unfortunately, clang-format doesn't have a soft column limit.
gofmt avoids this problem by never breaking lines (except for some trivial cases).

There are some larger formatting commits in our history, we don't want to add another one.

I think that not formatting (and making formatting commits) is a workaround.
The fix is to format eagerly, that will produce a cleaner history overall.

It could help to adapt the formatter config to match our tastes.
For example disabling AllowShortFunctionsOnASingleLine or removing ColumnLimit to allow arbitrarily long lines. I wouldn't do the second since I'm still using tools that don't work well with long lines.

In any case, we should probably do things like placing each list item on a separate line, like we do for path_subcommands but not for string_subcommands.

@faho faho added the bug Something that's not working as intended label Sep 7, 2021
When cd is passed a broken symlink, this changes the error message from
"no such directory" to "broken symbolic link".  This scenario probably
won't happen very often since completion won't suggest broken symlinks
but it can't hurt to give a good error.

Fish used to do this until 7ac5932.  This logic used to be in
path_get_cdpath, however, that is only used for highlighting, so we
don't need error messages there. Changing cd is enough.

Reword from "rotten" to "broken" since that's what file(1) uses.
Clean-up leftovers from old "rotten" code (nomen est omen).

See fish-shell#8264
Now that we removed EROTTEN which had the same error code as EPERM,
we can give a less confusing error in case a user has not allowed
their terminal access to a directory.

See fish-shell#8264
@krobelus krobelus merged commit 7f71df0 into fish-shell:master Sep 16, 2021
krobelus added a commit to krobelus/fish-shell that referenced this pull request Dec 12, 2021
I used the command from fish-shell#8092 to list issues/PRs with missing changelog entries,
and went through most of them.

Each issue gets three lines
- subject
- URL
- verdict

If the verdict ends with ", ignoring", I added it the the ignored list in the changelog.

The issues are grouped by verdict, with the interesting/leftover ones
on top (obviously no need to double check *everything*).

The "gh" script is already a quantum leap we can still find better
ways to share the changelog burden.
I noticed that there are many minor updates that can probably be
ignored.  Filtering them out doesn't take much time but it adds up,
especially if it's a single person doing it.

---

Issue/PR title: Make --no-config mode more comfortable
fish-shell#8493
in-flight

Issue/PR title: Debian fish binary package difficult to install
fish-shell#7845
TODO

Issue/PR title: Work around `setpgid` error on older Apple platforms
fish-shell#8153
this extends 7474, which wasn't listed either, we should probably mention it?

Issue/PR title: Abbr -q return status inconsistent
fish-shell#8431
fixes return status of "abbr -q", should we mention this?

Issue/PR title: math: (n n): incorrect error
fish-shell#8511
improved error output, which is very nice but too minor?

Issue/PR title: Fish autocomplete error on iOS procursus
fish-shell#8205
niche fix, ignoring

Issue/PR title: Fix `fish_key_reader` wrapper check
fish-shell#8271
minor update to not create a harmless alias for fish_key_reader, ignoring

Issue/PR title: funced dosn't like backslash escapes in function names
fish-shell#8289
minor escaping fix, ignoring

Issue/PR title: Hide whatis database building from the user
fish-shell#8310
not something many users would notice in the first place, ignoring

Issue/PR title: Duplicated "Type 'help argparse' for related documentation" for argparse
fish-shell#8368
minor update to error message, ignoring

Issue/PR title: Variable highlight color does not span lines
fish-shell#8444
very obscure fix, ignoring

---

Issue/PR title: Add --function to `read`
fish-shell#8295
added to existing entry (565)

Issue/PR title: Added completions for ethtool
fish-shell#8283
added to existing entry

Issue/PR title: Add dart completion
fish-shell#8315
added to existing entry

Issue/PR title: Add common lisp completions(sbcl/roswell)
fish-shell#8330
added to existing entry

Issue/PR title: Fix st issue with shift+tab
fish-shell#8354
added to existing entry (8352)

Issue/PR title: Support vi-mode cursors in Foot Terminal 
fish-shell#8391
added to existing entry (8167)

Issue/PR title: Completions pager should redraw if the subbed completion wraps/unwraps the line
fish-shell#8405
added to existing entry (8509)

---

Issue/PR title: Binding escape as user binding breaks escape sequence bindings (arrows, etc)
fish-shell#8428
added new entry

Issue/PR title: Windows "color" command completion
fish-shell#8483
added new entry

Issue/PR title: Doesn't build when using netbsd curses on Linux
fish-shell#8087
added new entry

Issue/PR title: Don't override linker
fish-shell#8152
added new entry

Issue/PR title: Add completions for `git-sizer`
fish-shell#8156
added new entry

Issue/PR title: `d3ceba107e88b6c6e1a0358ebcb30366aeef653f` causes issues with repainting multi-line prompt
fish-shell#8163
added new entry

Issue/PR title: Builtin math ncr can be extremely slow
fish-shell#8170
added new entry

Issue/PR title: Completion sometimes missing the last token
fish-shell#8175
added new entry

Issue/PR title: `set -S` should mark read-only variables
fish-shell#8179
added new entry

Issue/PR title: Errors when trying to autocomplete (invalid) UTF-8 escapes
fish-shell#8195
added new entry

Issue/PR title: Always use LC_NUMERIC=C internally
fish-shell#8204
added new entry

Issue/PR title: Slow interaction between backgrounding, universal variables, and repainting
fish-shell#8209
added new entry

Issue/PR title: Unsetting `$fish_emoji_width` doesn't clear the cached width
fish-shell#8274
added new entry

Issue/PR title: If prompt ends in an empty line, the commandline is inserted at the width of the line before
fish-shell#8298
added new entry

Issue/PR title: assertion normal_exited() failed related to paged builtin help
fish-shell#8308
added new entry

Issue/PR title: colors don't kick in for ls on macOS Big Sur, Monterey (and maybe FreeBSD)
fish-shell#8309
added new entry

Issue/PR title: Adds sub-command clear-session to history command. Issue fish-shell#5791
fish-shell#8337
added new entry (as 5791)

Issue/PR title: Display local branches before unique remote branches in git completion
fish-shell#8338
added new entry

Issue/PR title: Fix delete-key in st
fish-shell#8352
added new entry

Issue/PR title: sigsegv on set --show variable (when LANG is set to fr_FR.utf8)
fish-shell#8358
added new entry

Issue/PR title: Add clasp completion
fish-shell#8373
added new entry

Issue/PR title: `cargo run --example` completions break with nested example directories
fish-shell#8429
added new entry

Issue/PR title: argparse completions
fish-shell#8434
added new entry

Issue/PR title: Stop linking to StackOverflow
fish-shell#8495
added new entry

Issue/PR title: fish_key_reader ^C warning isn't right
fish-shell#8510
added new entry

Issue/PR title: Use `--almost-all` in `la` function
fish-shell#8519
added new entry

---

Issue/PR title: improve the experience of using fish over mosh
fish-shell#1363
listed as 8376, ignoring

Issue/PR title: incomplete man page completions
fish-shell#8305
listed as 8309, ignoring

Issue/PR title: Support "$(cmd)" command substitution without line splitting
fish-shell#8059
listed as 159, ignoring

Issue/PR title: fish_config: Read colorschemes from .theme files
fish-shell#8127
listed as 8132, ignoring

Issue/PR title: funced: edit the whole file, not just the function definition
fish-shell#8130
listed as 391, ignoring

Issue/PR title: builtin cd: print error about broken symlink
fish-shell#8270
listed as 8264, ignoring

Issue/PR title: fix man completion for BSD's mandoc
fish-shell#8306
listed as 8305, ignoring

Issue/PR title: Don't escape tildes that come from custom completions
fish-shell#8441
listed as 8441, ignoring

Issue/PR title: Use `cargo run --example` to get list of examples
fish-shell#8446
listed as 8429, ignoring

---

Issue/PR title: Node completion: add v8 sparkplug option
fish-shell#8118
update to existing completions, ignoring

Issue/PR title: Add zypper subcommands completion
fish-shell#8183
update to existing completions, ignoring

Issue/PR title: completion nmap: suppress warning when local scripts folder exists
fish-shell#8184
update to existing completions, ignoring

Issue/PR title: add missing `git commit` completions
fish-shell#8191
update to existing completions, ignoring

Issue/PR title: Updated ping completions
fish-shell#8192
update to existing completions, ignoring

Issue/PR title: Add `--function` to `set` completion
fish-shell#8202
update to existing completions, ignoring

Issue/PR title: completion: support `--no` prefixes for mpv flag options
fish-shell#8219
update to existing completions, ignoring

Issue/PR title: complete "mpc load"
fish-shell#8241
update to existing completions, ignoring

Issue/PR title: Add and fix completions for new options
fish-shell#8243
update to existing completions, ignoring

Issue/PR title: Fix completions/ls.fish
fish-shell#8249
update to existing completions, ignoring

Issue/PR title: Fix completions/coredumpctl.fish and add new complete
fish-shell#8256
update to existing completions, ignoring

Issue/PR title: completions/git: Handle "1 .T" & "1 AT" files
fish-shell#8311
update to existing completions, ignoring

Issue/PR title: completions/xbps-query: add missing `-p` completions
fish-shell#8323
update to existing completions, ignoring

Issue/PR title: Update ldapsearch.fish
fish-shell#8326
update to existing completions, ignoring

Issue/PR title: small fix completions/duply.fish
fish-shell#8327
update to existing completions, ignoring

Issue/PR title: Update ip.fish
fish-shell#8334
update to existing completions, ignoring

Issue/PR title: Fix ant completion
fish-shell#8344
update to existing completions, ignoring

Issue/PR title: Update dmesg completions
fish-shell#8365
update to existing completions, ignoring

Issue/PR title: No hints for -g|--global and -U|--universal flags for abbr command
fish-shell#8367
update to existing completions, ignoring

Issue/PR title: Updated systemd-analyze completions
fish-shell#8381
update to existing completions, ignoring

Issue/PR title: vmctl completion function call needs to be quoted
fish-shell#8406
update to existing completions, ignoring

Issue/PR title: pabcnetcclear command completion update
fish-shell#8480
update to existing completions, ignoring

---

Issue/PR title: document `--no-config`
fish-shell#8176
doc update, ignoring

Issue/PR title: Theme demo needs to be adjusted so that only unmatched quote is an error
fish-shell#8260
doc update, ignoring

Issue/PR title: no error about wrong >>? redirection operator
fish-shell#8380
doc update, ignoring

Issue/PR title: set -l works outside of command block
fish-shell#8385
doc update, ignoring

Issue/PR title: Some enhancements to "for" and "while" loop pages
fish-shell#8409
doc update, ignoring

Issue/PR title: Html docs: Remove link underlines again?
fish-shell#8439
doc update, ignoring

Issue/PR title: Old-style options support "=" assignment operator in complete builtin
fish-shell#8457
doc update, ignoring

Issue/PR title: Document prompt_hostname
fish-shell#8522
doc update, ignoring

---

Issue/PR title: edit_command_buffer: use "command" to ignore any functions with the same name
fish-shell#8221
only helps broken systems, ignoring

Issue/PR title: Prepend command to cat
fish-shell#8287
only helps broken systems, ignoring

Issue/PR title: Make less version check compatible with older Fish
fish-shell#8299
only helps broken systems, ignoring

Issue/PR title: Skip leading `command` in `__fish_man_page`
fish-shell#8447
only helps broken systems, ignoring

Issue/PR title: fish_config doesn't work without curses module
fish-shell#8487
only helps broken systems, ignoring

---

Issue/PR title: fix 'socket file name too long' error
fish-shell#8128
test fix with long tempdirs (macOS), not really user-visible, ignoring

Issue/PR title: Give tests a more generic name
fish-shell#8449
not user-visible, ignoring

Issue/PR title: string tests sometimes failing on macOS (Github Actions)
fish-shell#8353
not user-visible, ignoring

Issue/PR title: history merge test fails on OpenBSD
fish-shell#6477
not user-visible, ignoring

---

Issue/PR title: Obtain Deno completions from itself
fish-shell#8471
update to an unreleased feature (7138), ignoring

Issue/PR title: `string length --visible` performance
fish-shell#8253
update to an unreleased feature, ignoring

Issue/PR title: Backspace character is ignored when calculating string widths
fish-shell#8277
update to an unreleased feature, ignoring

Issue/PR title: `fish_config choose` leaves previous right prompt in place
fish-shell#8314
update to an unreleased feature, ignoring

Issue/PR title: parenthesis characters outer of $(command substitution) in string cause error
fish-shell#8394
update to an unreleased feature, ignoring

Issue/PR title: Parser bug with command substitutions in strings inside parenthesis
fish-shell#8500
update to an unreleased feature, ignoring

Issue/PR title: fish_config: silently doesn't set color schemes.
fish-shell#8419
regression, not in any release, ignoring

Issue/PR title: :program: in sphinx doesn't link
fish-shell#8438
regression, not in any release, ignoring

Issue/PR title: __fish_seen_argument.fish throws exception when autocompleting
fish-shell#8478
regression, not in any release, ignoring

---

Issue/PR title: Fix typo in abbr docs
fish-shell#8280
typofix, ignoring

Issue/PR title: Fix typo in `set_colors` command documentation
fish-shell#8321
typofix, ignoring

Issue/PR title: Typo funcions -> functions
fish-shell#8257
typofix, ignoring

---

Issue/PR title: remove make_pair
fish-shell#8206
no behavior change, ignoring

Issue/PR title: replace push_back with emplate_back
fish-shell#8222
no behavior change, ignoring

Issue/PR title: clang-tidy: remove pointless virtual
fish-shell#8224
no behavior change, ignoring

Issue/PR title: change value to rvalue reference
fish-shell#8227
no behavior change, ignoring

Issue/PR title: convert const ref to rvalue ref
fish-shell#8228
no behavior change, ignoring

Issue/PR title: clang-tidy: use for range loops
fish-shell#8229
no behavior change, ignoring

Issue/PR title: fix deleted constructors
fish-shell#8230
nno behavior change, ignoring

Issue/PR title: clang-tidy: const reference conversions
fish-shell#8231
no behavior change, ignoring

Issue/PR title: clang-tidy: simplify two bool returns
fish-shell#8235
no behavior change, ignoring

Issue/PR title: clang-tidy: replace size comparisons with empty
fish-shell#8237
no behavior change, ignoring

Issue/PR title: clang-tidy: replace NULL with nullptr
fish-shell#8239
no behavior change, ignoring

Issue/PR title: add constexpr
fish-shell#8252
no behavior change, ignoring

Issue/PR title: __fish_seen_subcommand_from and __fish_seen_argument update
fish-shell#8430
no behavior change (apart from a regression that's fixed), ignoring

Issue/PR title: Run fish_indent on all non-test .fish files
fish-shell#8476
no behavior change, ignoring

Issue/PR title: Use test command instead of bracket command
fish-shell#8477
no behavior change, ignoring

Issue/PR title: Fix code scanning alert - Wrong type of arguments to formatting function
fish-shell#8521
no behavior change, ignoring

Issue/PR title: clang-tidy: replace push_back with emplace_back
fish-shell#8236
no behavior change, ignoring

---
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Something that's not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants