Navigation Menu

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

Jump to bottom

gitoxide: init at 0.3.0 #95418

Merged
merged 2 commits into from Aug 16, 2020
Merged

gitoxide: init at 0.3.0 #95418

merged 2 commits into from Aug 16, 2020

Conversation

syberant
Copy link
Member

Motivation for this change

This PR is intended to fix #95317.

Things I'm not too sure about

The project is pretty young at the moment, I've only had 1 PR merged into nixpkgs so far so I'm not entirely sure what is supposed to go in here and what gets rejected. Some feedback would be appreciated.

I got the meta.description from the projects crates.io page but there might be a better one.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/), but only the help subcommand.
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@pstn
Copy link
Contributor

pstn commented Aug 14, 2020

If you want to, you can add a Fixes: line to your commit. That will to github closing that issue when the commit is merged.
It should look like this recent example.

TomSmeets
TomSmeets approved these changes Aug 14, 2020
View reviewed changes
Copy link
Contributor

@TomSmeets TomSmeets left a comment

Choose a reason for hiding this comment

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

Looks good.

But, as you said the project appears to be very young. As far as I know currently only gix init is implemented.

@syberant
Copy link
Member Author

If you want to, you can add a Fixes: line to your commit. That will to github closing that issue when the commit is merged.

@pstn, I think fix #95317 in this PR already does the job for GitHub, is it considered cleaner to do it in the commit message instead?

@alyssais
Copy link
Member

If you want to, you can add a Fixes: line to your commit. That will to github closing that issue when the commit is merged.

@pstn, I think fix #95317 in this PR already does the job for GitHub, is it considered cleaner to do it in the commit message instead?

It’s helpful for people going through the commit history. It’s also nice to include the full issue URL (which GitHub will still recognise and even abbreviate in the UI!) so it’s clickable and clearer if we ever move to a different system.

@syberant
Copy link
Member Author

Thanks, I included it into the commit message.

@marsam marsam changed the title Add gitoxide gitoxide: init at 0.3.0 Aug 15, 2020
sternenseemann
sternenseemann requested changes Aug 15, 2020
View reviewed changes
Copy link
Member

@sternenseemann sternenseemann left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, builds fine for me on NixOS, tested gix init seems good.

pkgs/applications/version-management/gitoxide/default.nix Outdated Show resolved Hide resolved
@sternenseemann
Copy link
Member

Probably good idea to squash the 4 commits into one or two, otherwise lgtm.

@syberant
Copy link
Member Author

I've squashed the three commits about gitoxide together but left the changes to maintainer-list as their own commit.

@alyssais
Copy link
Member

@GrahamcOfBorg build gitoxide

@syberant
Copy link
Member Author

It seems to be stuck at the darwin check, after inspecting https://monitoring.nix.ci/d/000000002/ofborg?orgId=1&refresh=10s I think that is because there is currently no builder for darwin.

@alyssais alyssais merged commit 8fb2885 into NixOS:master Aug 16, 2020
@turion
Copy link
Contributor

turion commented Aug 17, 2020

Awesome, thanks @syberant!

@syberant syberant deleted the gitoxide branch August 17, 2020 08:43
@syberant
Copy link
Member Author

No problem @turion, I saw it on reddit and thought it an interesting project. Your issue was the final push.

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

@zowoq zowoq zowoq left review comments

@sternenseemann sternenseemann sternenseemann requested changes

@TomSmeets TomSmeets TomSmeets approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gitoxide
7 participants
@syberant @pstn @alyssais @sternenseemann @turion @TomSmeets @zowoq