@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!
Top comments (25)
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 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)
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
I think it is to late if you already pushed, so in this case, I guess you can only squash/rebase (I'm not a git ninja though).
I have to admit I use
--amendquite frequently (I even have an alias,git amendto `git commit --amend --no-edit) because I sometimes forget to stage one file or another in my commit.Maybe [act](github.com/nektos/act can help you, it allows to run something similar to github CI on your machine, so you can commit, run the CI and amend it if something is wrong.
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.
In the example you provided, it depends on how valuable it is to know the incorrect solutions.
If the answer is "it's very important, as I'm likely to do them again," then absolutely leave them. That is valuable history for anyone working on the project later.
If the answer is "they were silly typos, just took CI to find them for me" then I'd say they should be cleaned up. That way your git blame will be a very clear picture of what changed, rather than cluttered with a useless previous revision. This is incredibly helpful when using git blame in your tooling to show the commit that last changed a file/line.
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.
Life is imperfect. Why pretend otherwise?
The trick to success isn't hiding mistakes, it's learning from them.
In this specific case I don't see a downside of using
git commit --amend(unless you've already pushed), so why not use it?Having an unclean commit history is not too harmful, all things considered, so you don't need to be perfect with it. However, a clean commit history provides some benefits:
git bisector find the commit manually.It's quite easy to get used to using
git commit --amendandgit rebase -i master, it just takes some practice.Literally yesterday I was configuring a new CI pipeline for a new project. My console for changes and commits ended up being
up->enterso I could do the following:git add .gitlab-ci.yml && git commit -m "Changes for debugging" && git push origin 1-cicd-pipelineI resorted to that because I was tired of writing commit messages which could be easily explained by the couple of lines that were changed, and it was to create the pipeline rather than update it.
finally got that part working after 9 commits.
At least I have some confidence that my IaC will work
A neat workflow I've seen is developing on PR and using interactive rebase/force push. Once the feature is complete, it usually can be contained to a single commit.
Since you can freely force push to feature branch, this workflow will still be able to use CI/CD pipelines while keeping the main branch clean.
Amending and especially squash/fixup will make the history neater and more readable, which will, in turn, help with figuring out where to go to revert, especially if you don't want to go back to the last tag. So it doesn't matter looking forward, but I think it matters immensely looking backward.
Amending/rebasing helps a lot with making PRs easily reviewable. Reviewers can look at the code commit-by-commit. Yes, keeping PRs small is a good practice, but even for smallish PRs, this helps a lot.
Say, you add something like a new configuration that happens to touch multiple features, reviewing it commit-by-commit avoids the problem where you review file A for feature AA, followed by file B for feature BB, and then file C for feature AA again.
In my experience, it's just easier to wrap your head around the whole thing this way.
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! 😅