Doubt: why squashing commits?
vcapretz opened this issue · 8 comments
I saw that you ask for developers to squash the commits into only one in each PR.
Why is that? Don't you lose commit's history? The commits being to big and having a lot of files changed is not bad?
I am used to commit very small changes each time so I can track record of a task progress, so it's just for understanding your point and maybe changing mine
@vcapretz Good call! I also disagree that we should squash all commits into one.
However, we should not have commits that fix errors introduced on the same PR.
@vcapretz IMO if the PR introduce only 1 feature change in the codebase then it should be squashed into 1 commit even if it has many cosmetic changes.
We don't have a rule about squashing every PR into 1 commit, but when I do commits here I try to keep 1 commit per feature/improvement since all the cosmetic changes you introduced is part of that feature and that won't mess up the history.
Let's say you have to add a feature in a product but you also want to fix some linting changes in the same place and refactor some old code. If you separated every change in a commit that would give you something like this:
* transactions: fix linting
* transactions: simplify old code logic
* transactions: add new feature
Fixing the lint and simplifying the old code does not reflect a complete logical change in the codebase but rather a cosmetic one. If every PR you merge don't squash the cosmetical changes your history will quickly be a mess and finding the commits that introduce bugs will be harder because for every logical change in the codebase you will have more than 1 commit. Did your feature addition introduce a bug in the identation fix or in the logical change? I don't know so i'll have to revert 3 commits instead of 1 to be able to roll back to the last non-buggy state and then debug 3 commits to find the culprit.
If you squash and merge this as transactions: add new feature you can simply go in the commit and the 3 descriptions will be in the body so you know that commit brings some refactoring besides the feature addition. You do lose some granular tracking but the trade-off is a cleaner history of logical changes.
I think this all comes down to what makes sense to your code history and bug findability. I don't think it's valuable to have more than 1 commit per logical change, but this doesn't mean 1 PR = 1 commit, if a PR needs to change many places to achieve a feature than you should separate the commits by the places you changes.
An example of a PR with more then 1 commit would be something like this: We need to make a feature but you found out that the webpack file could use some refactoring. So your PR has something like this:
* transactions: add super cool new feature
* webpack: refactor something
Note that the webpack change is not related to the feature, it's another change but you want to fix it as well so it can totally go on the same PR. Once merged (without squash) the 2 separate commits are in the history. If your team starts debating about the webpack change in your PR and that's blocking the other change to get merged then it's wise to open up another PR to move the webpack commit to it
I don't think granular commits are bad, I don't even have enough time of git usage to say which one of the alternatives is better, but it makes more sense for me today in large projects to have logical changes squashed into 1 commit to keep the git log sane to read and navigate :)
Sidenote: This is totally open for change and debate! Thanks for opening the discussion, let's gather more arguments to see what makes more sense :D
Nice @MarcoWorms! I totally understand your point!
You mentioned about the webpack case when you open a PR with something that is not related to the feature you are developing. I often catch myself doing small optimizations when developing something, like while developing a feature and using some helper function, I realize that some variable can be a constant if I just map some array and use concat over push.
Do you guys at Pagar.me avoid it or is it totally cool? Allowing small improvements while developing a feature is often bad because instead of having just one updated file you can have 10 files altered (I already did this but I can't avoid hahah)
If you separate the commits correctly this is awesome! IMO you should totally improve every bit of code you want to, and if you separate the commits correctly than the PR review shouldn't become polluted because when reviewing a PR every commit should be able to be reviewed independently. In the example I've mentioned above, the webpack changes can easily be reviewed by clicking on that commit, and the feature is also clean on it's own commit :)
Cool! It's something I will keep doing because one of my favorite kind of task is to improve code, keeps me motivated to keep learning
Thank you all for making it clear about squashing commits and other stuff here.
Closing the issue
There is no need to close the issue :D, @mccraveiro probably has a different opinion than mine
@MarcoWorms I totally agree with you.
However I like cosmetic changes to be on its own commit. I hate when I use git blame to check the history of a line and the message doesn't make any sense because of a style refactoring that was commited with a new feature.
I only find rebasing and squashing useful when a PR pushes changes on a commit that are remove or changed on a following commit. Noises like these shouldn't be merged on master as it will only pollute the project history.