News for hackers. Stuff that matters.
Re:Making a PR to Nixpkgs by gigatexal on Tuesday August 02, 2022 @01:41AM (#32315499)
I’d do a git commit —amend instead of a rebase to keep the number of commits to 1
Re:Making a PR to Nixpkgs by e3bc54b2 on Tuesday August 02, 2022 @01:49AM (#32315533)
That is what I dislike about the Pull Request workflow. It encourages piling on commits, polluting commit log (not to mention useless merge commits). To this day I haven't found anything better than Gerrit for reviewing changes across iterations. I wish it makes a comeback on broader scale.
Re:Making a PR to Nixpkgs by saagarjha on Tuesday August 02, 2022 @02:58AM (#32315885)
Pull requests are great, you just need to be mindful of what commits you're sending people. Every commit I put in a PR is a commit I made with intent. Just squashing them all does them a disservice.
Re:Making a PR to Nixpkgs by alexchamberlain on Tuesday August 02, 2022 @04:07AM (#32316219)
I don't know how many times I've said that reviewing the structure of the commits is part of a PR, just like reviewing the code. It seems to be a revelation.
Re:Making a PR to Nixpkgs by johnny22 on Tuesday August 02, 2022 @06:54AM (#32317132)
luckily github can just squash it for you if you choose. You don't have to have a useless merge commit. The option is right there in the dropdown. I assume gitlab as well.
Re:Making a PR to Nixpkgs by jrmurray on Tuesday August 02, 2022 @07:27AM (#32317300)
Not sure why I always forget amend exists. That is much cleaner
Re:Making a PR to Nixpkgs by meibo on Tuesday August 02, 2022 @02:43AM (#32315813)
Ideally your commits are squashed and then merged anyway, GitHub just still uses merge as a default since I assume it's the most "logical" to git newbies.
Re:Making a PR to Nixpkgs by Avamander on Tuesday August 02, 2022 @07:25AM (#32317288)
Only a polluted commit history should be squashed - bad commit messages, incorrect segmentation, fixes to earlier commits etc.

Think of it this way, if the commit works on its own it should remain as-is. If it's a hodgepodge of ten commits that inter-depend then they probably should be squashed.