DEV Community

Is git commit --amend truly *important*?

Ben Halpern on September 18, 2020

@molly_struve caught this one in a recent pull request of mine. // Detect dark theme var iframe = document.getElementById('tweet-1306940922...
Collapse
 
reobin profile image
Robin Gagnon • Edited

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.

Collapse
 
dvddpl profile image
Davide de Paolis

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

😅

Collapse
 
sirseanofloxley profile image
Sean Allin Newell

Dat high score tho ☝️

Collapse
 
nirebu profile image
Nicolò Rebughini

I like to keep history somewhat clean, so what I usually do is create "fix" commits to later fixup them in relevant commits with a git rebase once I'm done.

Collapse
 
itsasine profile image
ItsASine (Kayla)

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)

Collapse
 
kallmanation profile image
Nathan Kallman

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:

  1. Refactored animals to handle mythological creatures without edge-cases.
  2. Added my unicorns to the code-base

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?

Collapse
 
tiguchi profile image
Thomas Werner

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

Collapse
 
kallmanation profile image
Nathan Kallman

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 revert instead 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 a git 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...

Thread Thread
 
tiguchi profile image
Thomas Werner

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

Collapse
 
fennecdjay profile image
Jérémie Astor

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 --amend quite frequently (I even have an alias, git amend to `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.

Collapse
 
juliang profile image
Julian Garamendy

I don't always git commit --amend
but when I do, it's
git commit -a --amend --no-edit --no-verify && git push -f

Collapse
 
scanepa profile image
Stefano Canepa

I 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.

Collapse
 
jwkicklighter profile image
Jordan Kicklighter

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.

Collapse
 
amritanshupandey profile image
Amritanshu Pandey • Edited

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.

Collapse
 
alainvanhout profile image
Alain Van Hout

Life is imperfect. Why pretend otherwise?

The trick to success isn't hiding mistakes, it's learning from them.

Collapse
 
sargalias profile image
Spyros Argalias

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:

  • Can revert to any previous commit and it will be stable, so there is less risk when reverting.
  • Can help you debug by narrowing down the bad code by finding the commit where the bug originated. You can use git bisect or find the commit manually.
  • Smaller commits can help even more with reverting, as you won't lose as many changes when you revert, and with debugging, as the bad code will be narrowed down further.
  • Easy to read back and understand the history of the codebase and features.
  • Easier to code review. E.g. when a reviewer requests changes, they can review just the relevant commits after you push the changes, rather than the whole PR from scratch.

It's quite easy to get used to using git commit --amend and git rebase -i master, it just takes some practice.

Collapse
 
_garybell profile image
Gary Bell

Literally yesterday I was configuring a new CI pipeline for a new project. My console for changes and commits ended up being up -> enter so I could do the following: git add .gitlab-ci.yml && git commit -m "Changes for debugging" && git push origin 1-cicd-pipeline

I 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.
Lazy commits finally got that part working after 9 commits.

At least I have some confidence that my IaC will work

Collapse
 
cipharius profile image
Valts Liepiņš

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.

Collapse
 
camilo profile image
Camilo Payan

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.

Collapse
 
michi profile image
Michael Z • Edited

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.

Collapse
 
simme profile image
Simme

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! 😅

Collapse
 
gwutama profile image
Galuh Utama

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.

Collapse
 
elmuerte profile image
Michiel Hendriks

If only you could test locally before pushing.

Collapse
 
cacilhas profile image
Montegasppα Cacilhας

Did you have any trouble in running specs locally?

Collapse
 
luscala profile image
Luca Scala

It depends on the PR. If you’re playing with a pipeline, finding the way your app builds without errors this works. In other case devs should commit only effective change with a a precise scope.