Migration: make squashing history an optional task

[rousseldenis]

Recently, some migration PRs were blocked by some to require squashes of administrative commits as defined here:

https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-18.0#tasks-to-do-in-the-migration

See: OCA/sale-workflow#2555 (review)

IMHO, this kind of tasks are nice-to-have, but should not block a PR. Personnally, I see a possible security hole in this kind of request. As soon as our migration PR's can contain now thousands of commits, it become more and more difficult for reviewers to detect a malicious one.

To be focus on this request, I suggest to split todo tasks in two categories : must and nice-to.

@OCA/core-maintainers @OCA/board

Thanks!

[ivs-cetmix]

Personnally, I see a possible security hole in this kind of request. As soon as our migration PR's can contain now thousands of commits, it become more and more difficult for reviewers to detect a malicious one.

So let's leave even more commits so we get totally lost in the end 😄

To be focus on this request, I suggest to split todo tasks in two categories : must and nice-to.

I would say that as soon as some rules becomes a "nice-to" it is almost never followed. With this said we either need to define clean and clear definition of when to squish those commits and when not to. Or just keep this requirement mandatory as it is now.
I would prefer the second option because a) it's not a time consuming task b) it makes the commit history more readable c) it enforces discipline.

[rousseldenis]

I would prefer the second option because a) it's not a time consuming task b) it makes the commit history more readable c) it enforces discipline.

Keep in mind that such change rewrite something passed (and exist in previous main branches). We should not rewrite history...

So let's leave even more commits so we get totally lost in the end 😄

So, you'll like the discussion about resetting or not new main branches from previous one :-)

[StefanRijnhart]

Squashing the administrative commit history is a risky task in the hands of less experienced contributors. I find it daunting myself at times. Commits can easily be dropped accidentally, and sometimes the situation is ambiguous if a single merged PR contained more than one commit. Also, the correctness of the squashing is difficult to review. The reviewer request to squash commits might come after several approvals have been granted already. As a result, errors might go unnoticed. All in all, I think it should be left to those who feel comfortable with it so I support this suggestion.

[rvalyi]

I have no strong opinion on this. But I think we should do something automatic, possibly in the migration script to squash translation commits when possible. If I take OCA/l10n-brazil, it has 14k commits (that's 10% of Odoo CE and about the same size in lines of code, and this ratio is in line with other repos). But may be 20% of the commits (at least in branch 14.0) are migration commits. It sucks...

[ivs-cetmix]

So, you'll like the discussion about resetting or not new main branches from previous one :-)

I'm not discussing it because I'm not such a technical person. And I'm not discussing if this is risky or not. I'm following a very simple principle: if this is written in the todo list then it must be done. So either we need to remove it from the todo list, or add some conditions that could be checked and matched. That is.