@molly_struve caught this one in a recent pull request of mine.
The obvious question here is.... Should I be using git commit --amend or another history rewriting technique to clean this up?
I know I can, and maybe I should, but I've never developed the habit of using this technique and generally never thought of it as a big problem in most cases, but I'm wondering if other folks feel differently bout the harm of not cleaning up extra commits like this.
All around I could have been clearer with the commit messages, but also, contextually it was somewhat clear. It was me trying a few things and committing the code to see if it worked with our continuous integration.
Feedback is definitely welcome here!
Oldest comments (25)
Life is imperfect. Why pretend otherwise?
The trick to success isn't hiding mistakes, it's learning from them.
I like to keep history somewhat clean, so what I usually do is create "fix" commits to later
fixupthem in relevant commits with agit rebaseonce I'm done.This. Fixup is fantastic for the case of "Add thing" "fix thing" "stupid fdescribe" "fix thing for real" "Add other thing" "fix that now wtf". And then it magically moves to the main branch as "Add thing" "Add other thing" (or just Add thing once it's out of review if that makes more sense for the feature)
I think it's definitely fine in the context of CI trial and error. I usually increment a counter on each commit. (Trying to get that HIGHSCORE)
That said, I clean it up using rebase and fixup so it makes sense chronologically when submitting a PR where context is important.
Glad to see that this is not so uncommon in CI. Recently added gitlabci to our projects and last whole week of commits looks like this:
Fix it.
fix it again
Not yet fixed
Maybe now
Nope
This is it
Yet another fix
Finally
You wished
For godsake, please fix it
😅
Dat high score tho ☝️
I like it as long as it's in a feature branch. If directly on the trunk, no way, as that would require a force push! 😅
My current workplace has a pretty strong and widespread practice of amending (or fixups). It helps immensely for reviews of slightly more complex units of work to present it as a story; an anecdote that I often find myself doing:
I want these in one PR, because the refactor makes little sense without the context of the new unicorns, but the new unicorns would be horrifically difficult to implement without the refactor. Grouping the relevant changes (even if I had to go back and forth a few times to get each group right; nobody's perfect) makes it much easier to review each change (because the intention can be clearly seen) than going through a messy history or looking at everything at once.
I think my dream feature would be if history could be preserved but PRs could be constructed to present the history into a set of meaningful diffs for review... a dev can dream right?
If I'm understanding you correctly, this seems to be pretty much what Phabricator does with Differential.
By default Phabricator squash merges accepted PRs as single commits into the mainline development branch, so "one idea" = one ticket = one commit, which makes the Git history really easy to understand. No funny "Fix spec?" in between commits there (sorry Ben).
But the commit history that led to that accepted change is preserved in Differential revisions, so it's possible to see the whole discussion and in between changes that led to the final commit
It's similar; that appears to be a fancy version of github's squash and merge (if I'm reading correctly).
What I want is an even beefier version where I could have two distinct commits on the mainline branch (or more) based on some magical input from me on how to squish the original history together.
Consider building an AB test for new functionality. I know one of these code paths will be deleted after the test (either A or B). It would be useful if I could do that with only
git revertinstead of re-digging through the code...If I create a first commit that implements the B feature as though it simply replaces the A feature then add a second commit that adds the A feature back in according to whatever testing switch is needed. Then after the test concludes I can simply
git revert <second-commit>to choose the B variant or follow that with agit revert <first-commit>to choose the original A variant.The only way to get commits like that is by doing it perfectly from the beginning or editing history with amends, squashes, and fixups...
Got it, that's an interesting scenario. Especially when A-B testing is done in production.
I guess that problem with reverting a throwaway feature commit could be prevented by maintaining two feature branches for each tested feature, and build them so they maybe run in separate production environments. And a load balancer or some other kind of front end would decide where a user ends up (test A or B)
That would make sure that those distinct features don't have to be added to the same mainline dev branch, and in the end you can simply abandon one branch and merge in the other
Meh. We all have bad history. Why rewrite it?
The commit ids are unique anyway so you can still go back to any revision you want. It’s just not pretty.
I don't always
git commit --amendbut when I do, it's
git commit -a --amend --no-edit --no-verify && git push -fI don't know if this correct or not but I usually use git commit --amend if the commit contains a change that is really part of the previous one, to deploy some services I have to change their configuration files but I forget one so I amend my commit.
When I'm developing I prefer to make more commits, one per change and then, squash merge the resulting PR (if I have write access to the repo) or squash-merge before opening the PR upstream.
I believe that commit history should tell a clean story, and as such I feel Multiple fixit type commits are redundant.
If I look at these after few months or years, Iwill have no clue whats going on here.
Did you have any trouble in running specs locally?