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

helix: fix UB in diff gutter #249163

Merged
merged 1 commit into from
Aug 15, 2023
Merged

helix: fix UB in diff gutter #249163

merged 1 commit into from
Aug 15, 2023

Conversation

lunagl
Copy link
Contributor

@lunagl lunagl commented Aug 14, 2023

Description of changes

Applies helix-editor/helix#7227 as a patch until the fix is included in the next release.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@atalii atalii self-requested a review August 14, 2023 17:43
@atalii
Copy link
Contributor

atalii commented Aug 14, 2023

should get a review from @danth @yusdacra or @zowoq. my words carry little weight, but i'm on board with the "just get UB out the tree now and fix it later" approach so long as the package maintainers are.

other than that, lgtm after a local build.

Copy link
Contributor

@atalii atalii left a comment

Choose a reason for hiding this comment

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

i'm approving for now barring any other opinions

@lunagl
Copy link
Contributor Author

lunagl commented Aug 14, 2023

Yeah I was hoping there would be a new helix release sooner, but as I'm not sure how long it will take until then (helix-editor/helix#7896 (comment) says it could be sometime this or next month), I thought It'd be best to temporarily patch it here ourselves.

It's not just that there's UB, the bug actively prevents me from using helix on any committed files in a git repo which is kinda annoying.

@yusdacra
Copy link
Member

Result of nixpkgs-review pr 249163 run on x86_64-linux 1

1 package built:
  • helix

Copy link
Member

@yusdacra yusdacra left a comment

Choose a reason for hiding this comment

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

Tested locally, seems to work as intended. LGTM

Applies helix-editor/helix#7227 as a patch
until the fix is included in the next release.
@beeb
Copy link
Contributor

beeb commented Aug 15, 2023

Builds and fixes the issue for me too, LGTM

@ofborg ofborg bot requested review from yusdacra and zowoq August 15, 2023 11:14
@zowoq zowoq merged commit 820d55b into NixOS:master Aug 15, 2023
@github-actions
Copy link
Contributor

Backport failed for release-23.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-23.05
git worktree add -d .worktree/backport-249163-to-release-23.05 origin/release-23.05
cd .worktree/backport-249163-to-release-23.05
git checkout -b backport-249163-to-release-23.05
ancref=$(git merge-base f1aa0b91207fc009eccde2167f7be066002c776f 2010902b385137f4ad6d8f4b8a4aaf6392064671)
git cherry-pick -x $ancref..2010902b385137f4ad6d8f4b8a4aaf6392064671

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.

5 participants