by dnilasor on 1/19/22, 6:50 PM with 114 comments
by dljsjr on 1/19/22, 8:30 PM
Much like the thesis of this article, the goal is to have a set of well organized commits so that when it comes time to do a PR review, you have 1-3 logical units of change and it also improves the ability to do reverts.
But you can very easily do this just by rebasing instead of resetting and re-committing.
In particular, `git commit --fixup <ref>` and `git commit --squash <ref>` are extremely useful for this during the "WIP" stage as well as when handling PR comments, and these are things that I only learned about in the last 6 months. I would recommend doing a google search on "fixup commits" to learn more about them. I enjoyed this article: https://www.mikulskibartosz.name/git-fixup-explained/
Yes, rebasing is scary if you're new to git, but it does everything in this article in a way that's much cleaner and once you learn how to use it effectively you'll feel like you have super powers. It's worth taking the time to learn.
by _hao on 1/19/22, 8:05 PM
by ninkendo on 1/19/22, 9:02 PM
But this part set off alarm bells:
> Once you’ve finished making your changes, it’s time to prepare your work for some “git clean up.” To do this, we’ll run the following command:
> git reset origin/main
Be very careful here! If you've run `git fetch origin` since you've started your work, you may be resetting to a commit that's newer than what you based your work on, and thus you'll wind up creating a commit which effectively reverts anything that's happened on `main` since then.
The more technically correct command would be:
> git reset $(git merge-base origin/main HEAD)
Since that resets to the commit you started your branch from.
by alkonaut on 1/19/22, 8:55 PM
Then the article describes a very elaborate way of… not addressing the problem?
Reset-Cleanup is a decent idea and helps keep a tidy repo which is a good goal. But the revert reason seems contrived.
The problem is going to be later code (from other pull requests) being dependent on the code in my pull request.
If my pull request is reverted then any later change can break. If my PR consists of 5 well separated commits or 3 messsy ones doesn’t matter in that scenario.
If someone against all odds wants to revert one individual commit from a merged branch but not revert the whole PR by reverting the merge commit then not only can the feature in the PR stop working (if the feature/bug fix in the PR didn’t need that commit to work then why was it there in the first place!?), any later code can break just as it can when PR is reverted as a whole.
This is why I recommend squashing for almost all cases. For really complex features with dozens of commits you can always do a rebase + FF (but in that case all commits should build + pass tests, which is an unrealistic goal in all code bases where a build + test takes hours).
by sandofsky on 1/19/22, 8:30 PM
As far as I could tell, this was the recommended workflow from the earliest users of git. Look at the Linux kernel's public history.
When git gained widespread adoption, it was sold as "Subversion, but with cheap branching." People don't realize that this power requires discipline, otherwise you end up with complicated history that makes change management a nightmare.
In code review systems like Gerrit, you have to approve every single commit, and you can also enforce rules like "fast-forward merges only." It sure makes Github's pull-request model feel like an anti pattern.
by leifg on 1/19/22, 8:10 PM
I use GitHub and always squash commits before merging a PR. This keeps the commit log clean and also has the side effect that you have the PR number in the merge commit.
Having said that I also suggest keeping PRs small. If you are going to reformat a code base, make that a separate commit. Updating a library, separate commit. Adding a library you need for a new feature: create a PR for the library update, base your implementation off that branch and rebase against main when the first PR is merged.
by mattwad on 1/19/22, 7:44 PM
by danhab99 on 1/19/22, 9:05 PM
by joelmbell on 1/19/22, 8:04 PM
When developing my priority is to easily get back to a last known working state. This allows me to try out risky changes, and throw them all away if it doesn't work out.
When pushing my changes for PR, those working states I saved before may not be the best logically. I usually re-write my commits to break it into more logical chunks, that are easy to revert and easier for other teammates to digest.
by bronzecarnage on 1/19/22, 8:10 PM
Previously I avoided creating messy commits simply because it was "tedious" to reorganize commits. And making overly atomic commits and typing out git commands more frequently didn't appeal to me either.
Once I got used to the above tools, life got so much easier. Lazygit makes it super easy to amend, reword, and even re-position commits in a TUI environment. Real life-changer for me. I can stage hunks super easily too, though that's even easier in neovim.
The only issue I face is my C-j/C-k keys are already bound to tmux, but are needed by lazygit to reposition commits.
[0]: https://github.com/jesseduffield/lazygit [1]: https://github.com/tpope/vim-fugitive
by agd on 1/19/22, 7:45 PM
I'm not sure this gives the right impression. Yes rename/squash/interactive rebase if necessary to tidy, however I still believe you should strive to create clear, separate commits as you go.
If you have to regularly make major changes to history before review, I could be a sign that your process/approach is disorganised.
by krainboltgreene on 1/19/22, 10:05 PM
"Added new styles to navigation"
No duh, that's what the diff shows, but that doesn't tell us why you made the change, and that's the part we're going to struggle to remember in 6 months.
by lamontcg on 1/20/22, 4:37 PM
As far as the proposal goes, this is a much better way to do it than trying to preserve every mistake you made along the way. I'd prefer to see either this or just the one squashed commit. I'd just argue that maybe if its important to break it up into multiple commits that you might consider that it should also be separate PRs. If it is necessarily so interlinked that you can't produce separate passing PRs then I probably need to wrap my brain around the whole thing at once as well.
Usually my objection is that multiple different concerns need to be done totally separately. I can't think of the last time I really wanted just separate commits and not entirely different PRs.
by timmit on 1/19/22, 8:08 PM
For example,
Most of one core change is in multiple files, it will be very bad to commit by files group
by zwieback on 1/19/22, 7:52 PM
Either way, in the end it's up to developer discipline to make clean commits, git doesn't really help all that much although in my (long) source control history before git I never thought of committing hunks.
by travisd on 1/19/22, 9:05 PM
> Be mindful of not leaving your codebase in a broken state during this step
You’re still relying on very fallible human intervention here. Even worse, often times when grouping commits post-facto you’ve forgotten some of the context of what depends on what.
Ostensibly the approach presented could be better than other strategies in this regard, but that’s now how the article presents itself and it loses credibility for that.
by okl on 1/19/22, 8:32 PM
[tig "bind"]
main = = !git commit --fixup=%(commit)by musicmatze on 1/20/22, 11:57 AM
This is what the project I was just hired to work on looks like. Result is a linear history, with multi-thousand-line changes in a commit with message "Implement fixes" and a list of "fix bug", "fix more", "format", "change implementation" in the long-form commit message.
People out there do not even know how to work properly with git, I think it is way too much for them when we start telling them how to "workflow" with git. It is sad, but that's what I see.
by mb20281 on 1/19/22, 8:06 PM
by ozim on 1/19/22, 8:45 PM
Example from the start ... revert.
Don't revert stuff, fix stuff forward (make new commits with new changes) you won't introduce bugs in silly way.
by ltbarcly3 on 1/19/22, 8:17 PM
If you want to follow the pattern described here, create a new branch and do `git checkout the_feature_branch -- $filename` and pull the changes from the branch you did the work in into the new branch, making commits as described in the article. You can even do `git diff --numstat $feature_branch $new_branch` to see what has changed.
by alunchbox on 1/19/22, 9:24 PM
> When the feature is complete, make a pull request.
gotta love 100 file commits that take a day to review.
by jayd16 on 1/19/22, 9:32 PM
Git should introduce stages and allow you to have any number of stages. Git should also have some porcelain around stashing all but one stage and restoring all etc etc.
by whoomp12342 on 1/19/22, 8:28 PM
the problem with your approach is that sometimes, changes are logically grouped but cross file.
by endigma on 1/20/22, 12:57 AM
by hill613 on 1/19/22, 8:37 PM