[RFC] Contribution: Merger Commit Squashing

[oca]

[RFC] Contribution: Merger Commit Squashing

[lmignon]

I'm personally against the squash of commits of others and have a lot of times expressed my opinion on this topics when reviewers asked me to do this during module migrations. IMO we must keep the commit history when merging prs AND when we migrate modules

[pedrobaeza]

That's a nonsense to keep a lot of "PEP8 fix", "Fix onchange" and so on commits. They are useless and make the size of the repository unmanageable when we reach 5 or 6 version branches (it's already passing in l10n-spain, that has branches from 4.2 - the size has gone to 150 MB for that repository). We don't have to blindly squash, but also we don't have to keep all the history as the development/review has come to the module.

[rafaelbn]

Usually we must ask contributors to squash their selves

[JonathanNEMRY]

@pedrobaeza I fully agree with the fact that "pep8 fix" is not clean into a commit history.

But I'am not in favor by forcing users to squash existing commits into migration or existing source history.
Also I suggest to do it when you are into a review and with the implied commits of the PR only: "pep8 fix" should't be committed at all by the developer that is applying the changes for its review into the linked pull request. (as @lasley said, there are many tools into Git to avoid committing that kind of stuff) So IMO that's exactly the place where we have to change/allow to ask something more.

But we have to be careful with existing commits that appears as beeing "not-clean" (implying some size addition too...) because of a "pep8 fix" label as they possibly contain more than a simple "pep8 fix" and then it became sensitive to ask some change about these commits (added time and authors issue)

[pedrobaeza]

The problem is that in the past we haven't applied that rule in the past, and now we are transferring the dirty commit history across branches. Take into account that this doubles the size of the repository, as the commits are not cherry-pick, but extracted by folder, so although the diff can be identical, the commit has another hash and counts as another one.

We have also the "OCA Transbot translations" commits, that can accumulate a lot of commits during the lifecycle of the version.

Those 2 are the reasons for me to ask a squash of the commits when migrating a module. You can agree with me about the second one, isn't it? And about the first, if we do it in this way and properly treat the v9 commit history, for v10 we won't have this problem again (except for the point 1, that will always happen).

[moylop260]

I like to merge&squash from github.

I don't like squash commits before merge:

• If I'm have a active revision I like to see the new commits to know what exactly changed. (Avoid review all changes again)
• If other reviewer is agree with a old version we can use the first commit instead of request revert it
  • Example: odoo/odoo#12769

However, I understand the good point of @lasley

Then we should have a open door for these cases.
What about the following rules table?

Commits	PR comment "please, don't squash"	How to merge
>1	Yes	Developer squash after approvals --by author--
>1	No	Merger Squash&merge
1	Yes	Merger Merge original commit
1	No	Merger Squash&merge

[lmignon]

@moylop260 You must also consider when the PR has more than 1 author. In this case you must preserve the credit of all the authors.

[moylop260]

@lmignon You are right!
Thank you

Point 1 changed

- Developer squash after approvals
+ Developer squash after approvals --by author-- 

[lasley]

Given @lmignon's point, we also cannot use the Github Squash options if there are multiple authors in the PR. This was the cause of OCA/server-tools#619

[pedrobaeza]

We can only use it when there's only one contributor in the PR, which happens a lot with new modules or patches that are done in several steps, so the button worths the while.

[lmignon]

@pedrobaeza I've a little bit analyzed the l10n-spain repository. IMO It appears that the problem of size that you encounter with this repository is not related to the number of commits. The problem is that you've stored in your repository a lot of large files.... (jar, csv, )
Takes a look at http://naleid.com/blog/2012/01/17/finding-and-purging-big-files-from-git-history to analyze your repository first...

[pedrobaeza]

I know that big files, but we could save a lot because of some diff files on these large files that could be squashed across versions, so another more reason for doing it.

[lmignon]

So the rule is not global and this problem must be addressed on a per repo/pr basis.

[pedrobaeza]

No, the rule can be global: squash what you can, and you'll save some bytes to be transferred, some trees to be cut, and why not, some kitties 😝

[yvaucher]

+1 @lmignon

To come back on this I think it should be up to the commiter to decide. There are some case where a commit squashing makes sense. Mostly when the PR is kind of messy and we want to merge it. Asking to a new developer to solve it leads to a dead-end as there could be a gab in knownledge from creating a commit and mastering amending and rebase tools.
I agree it's better to teach to fish instead of giving the fishes but for important fixes and for most wanted features the commiter can (is not forced to) ease a part of the job.

[dreispt]

I also agree that it really depend and must decided per PR.
Having said that, blindly squashing commits should not be recommended.

[pedrobaeza]

GitHub offers now the possibility to the repo writers of modifying PR branch, so we can do the work instead the newbies. A clean commit history is very important in my opinion for avoiding unmanageable repos in a couple of versions more.

[lasley]

I feel like when someone squashes my commits, the message is significantly less meaningful than if I would have squashed it myself though. The same Fix my dumbness comments still make their way into the history, just as one comment instead of multiple.

I do agree that the need to squash probably outweighs this though. Particularly with newbs - git is a pain point for sure.

[lasley]

I'm going to go ahead and close this - I feel that the new Wiki entry is a great start & any additional changes should be made direct there. Thanks everyone!