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

editline: fix crash with term narrower than completions #60279

Merged
merged 1 commit into from May 21, 2019

Conversation

dtzWill
Copy link
Member

@dtzWill dtzWill commented Apr 26, 2019

Motivation for this change

Fixes issue mentioned on #nixos earlier today by @edef1c.

Repro instructions:

  • Open a terminal and resize it to very narrow (~20 should work,
    haven't calculated the breaking point)
  • Run nix repl
  • Type :b and hit TAB for completions
  • Sadface

Compare this with and without this change :).

Submitted upstream, FWIW: troglobit/editline#24

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Fixes issue mentioned on #nixos earlier today by @edef1c.
@teto
Copy link
Member

teto commented Apr 27, 2019

I confirm the bug. can't we wait for the next update of editline instead this the patch was merged and conditions to trigger are quite specific (if you open the repl in a wide terminal and narrow it afterwards, the bug is not triggered).

@dtzWill
Copy link
Member Author

dtzWill commented Apr 28, 2019

I confirm the bug. can't we wait for the next update of editline instead this the patch was merged and conditions to trigger are quite specific

Oh, hooray I somehow missed it being merged. :3. We can wait if that sounds better 👍.

(if you open the repl in a wide terminal and narrow it afterwards, the bug is not triggered).

I don't believe this is entirely true-- I think you need to hit enter or something before the new size is respected? Anyway it's not likely something many folks encounter, which I think is the point :).

@teto
Copy link
Member

teto commented Apr 28, 2019

I don't believe this is entirely true-- I think you need to hit enter or something before the new size is respected? Anyway it's not likely something many folks encounter, which I think is the point :).

Exactly :D so feel free to merge but I don't think it's worth the trouble.

@samueldr
Copy link
Member

samueldr commented May 1, 2019

Do we know what's the ETA on an upstream release with the patch? Couple days, few weeks probably this can be left for an upstream update. Longer may be annoying.

In any ways, if the update is a breaking one, the patch might need to be backported to 19.03, as a QOL patch.

@edef1c
Copy link
Member

edef1c commented May 1, 2019

Gonna propose we merge in a week or two (2019-05-15) if we don't have an ETA by then, and otherwise re-evaluate. But upstream seems to be fairly responsive, so I suspect we'll hit the latter case.

@edef1c
Copy link
Member

edef1c commented May 21, 2019

Merging this, as promised.

@edef1c edef1c merged commit 2ec33d1 into NixOS:master May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants