Skip to content

Comments

git-instafix: init at 0.2.1#275402

Merged
pbsds merged 1 commit intoNixOS:masterfrom
mightyiam:git-fixup
Mar 14, 2024
Merged

git-instafix: init at 0.2.1#275402
pbsds merged 1 commit intoNixOS:masterfrom
mightyiam:git-fixup

Conversation

@mightyiam
Copy link

Description of changes

This app seems nice. Here's a package for everyone!

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@mightyiam mightyiam marked this pull request as ready for review December 19, 2023 12:56
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3125

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Dec 19, 2023
Copy link
Contributor

@jfly jfly left a comment

Choose a reason for hiding this comment

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

:shipit:

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Dec 19, 2023
@rjpcasalino
Copy link
Contributor

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

1 package built:
  • git-fixup
[nix-shell:~/.cache/nixpkgs-review/pr-275402/results/git-fixup/bin]$ ./git-fixup --version
git-fixup 0.1.8

@delroth delroth added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Dec 19, 2023
@kuruczgy
Copy link
Contributor

kuruczgy commented Dec 21, 2023

Could you talk a bit about this particular git-fixup project (link)? It is made mostly by a single author, with the last commit from more than a year ago. Contrast this with https://github.com/keis/git-fixup, which seems to be receiving semi-regular updates, with multiple contributors.

I would propose either

@mightyiam
Copy link
Author

Author considering renaming their project. I'd give them some more time.

@mightyiam mightyiam marked this pull request as draft January 3, 2024 23:26
@kuruczgy
Copy link
Contributor

kuruczgy commented Feb 23, 2024

It seems that the author renamed the project, did a new release with the new name, and even provided a description of what differentiates it from the shell-based version I linked, so from my side this can go ahead 👍 (As long as you adjust the PR with the new name and the new version of course.)

@mightyiam mightyiam force-pushed the git-fixup branch 3 times, most recently from 371eed0 to a5a288e Compare February 24, 2024 13:51
@mightyiam mightyiam marked this pull request as ready for review February 24, 2024 13:51
@mightyiam
Copy link
Author

Made all the updates.

@mightyiam mightyiam changed the title git-fixup: init at 0.1.8 git-instafix: init at 0.2.0 Feb 24, 2024
Copy link
Contributor

@kuruczgy kuruczgy left a comment

Choose a reason for hiding this comment

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

LGTM, just minor formatting nitpicks.

Comment on lines 45 to 38
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should ask the author to make the git binary configurable. But for the first version this should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: unnecessary trailing comma.

Comment on lines 31 to 41
Copy link
Contributor

Choose a reason for hiding this comment

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

meta should be at the very bottom, at least that's what I see in most packages.

@mightyiam mightyiam force-pushed the git-fixup branch 2 times, most recently from 02206a4 to 395d4b1 Compare February 26, 2024 10:20
@mightyiam
Copy link
Author

Applied feedback.

@pbsds
Copy link
Member

pbsds commented Feb 26, 2024

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

1 package built:
  • git-instafix

Ofborg darwin seem to be failing due to a linker error, consider marking it meta.broken = stdenv.isDarwin

@mightyiam mightyiam changed the title git-instafix: init at 0.2.0 git-instafix: init at 0.2.1 Mar 4, 2024
Copy link
Member

@pbsds pbsds left a comment

Choose a reason for hiding this comment

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

still broken on darwin

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = "Quickly fix up an old commit using your currently-staged changes.";
description = "Quickly fix up an old commit using your currently-staged changes";

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label Mar 8, 2024
@pbsds pbsds merged commit 4ea4e37 into NixOS:master Mar 14, 2024
@mightyiam mightyiam deleted the git-fixup branch March 15, 2024 00:57
@mightyiam
Copy link
Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants