Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] introduce Squash Bot #66

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

trisdoan
Copy link
Contributor

@trisdoan trisdoan commented Dec 25, 2024

Context

  • During migration process, we sometimes forget (or leave out intentionally 😋) to clean commit history
  • Purpose of this PR is to ease the process with interactive SquashCommit Bot

This change

  • Introduce SquashCommitBot, automatically run after pre-commit hook (except non_interactive or dry_run mode)
  • Replace editor during interactive rebase with custom bash script, with GIT_SEQUENCE_EDITOR and GIT_EDITOR

Result

oca_port_squashbot.webm

@giarve
Copy link

giarve commented Dec 26, 2024

I like the careful approach to ask the user which commits to rebase or not, however, is there any case where the user would not want to fixup to the previous commit a commit made by any of the SQUASHABLE_AUTHOR_EMAIL?
i.e. by default an automatic fixup is done unless specified by a flag that changes the default (no need to remove already programmed logic, if you don't want)

the temp file to do the rebase looks like a quick hack as it's assuming bash will exist in the user environment which probably will in 99% of cases in development environments) + uses subprocess. Nonetheless, I have not found an easy rebase option for libgit in python and it would need to be implemented by its primitive commands. It think it will need a little bit of testing (not done yet from my side)

@giarve
Copy link

giarve commented Jan 5, 2025

try running oca-port on this one:
https://github.com/oca/payroll/commits/15.0/hr_payroll_period

Then try squashing the rest of the commits. It loops on the same commit but does nothing.

If you cannot reproduce this issue let me know (as it will be environment related stuff and I can help)

Logs:

🚀 Starting reducing number of commits...
Squashing 28e310201 Update translation files
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 0
Skip this commit? [y/N]: y

Skipped 28e31020 Update translation files



Squashing 45400d2fe [BOT] post-merge updates
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 1

Commits to Squash:
        45400d2fe [BOT] post-merge updates
        f351f844a [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
 [y/N]: y
✨ Done! Successfully squashed.


Squashing 45400d2fe [BOT] post-merge updates
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 1

Commits to Squash:
        45400d2fe [BOT] post-merge updates
        f351f844a [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
 [y/N]: y
✨ Done! Successfully squashed.


Squashing 45400d2fe [BOT] post-merge updates
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 1

Commits to Squash:
        45400d2fe [BOT] post-merge updates
        f351f844a [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
 [y/N]: y
✨ Done! Successfully squashed.


Squashing 45400d2fe [BOT] post-merge updates
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 1

Commits to Squash:
        45400d2fe [BOT] post-merge updates
        f351f844a [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
 [y/N]: y
✨ Done! Successfully squashed.


Squashing 45400d2fe [BOT] post-merge updates
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 1

Commits to Squash:
        45400d2fe [BOT] post-merge updates
        f351f844a [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
 [y/N]: y
✨ Done! Successfully squashed.


Squashing 45400d2fe [BOT] post-merge updates
0) Skip this commit
1) f351f844 [IMP] hr_payroll_period: Add a cron to create next fiscal year automatically
Select a commit to squash into: [0]: 

@trisdoan trisdoan force-pushed the identify-squashable-commits branch from 3cbf201 to 83cc505 Compare January 8, 2025 10:06
@trisdoan
Copy link
Contributor Author

trisdoan commented Jan 8, 2025

Hello @giarve, I provided a fix

Why it happened

  • there were 7 characters in file git-rebase-todo
  • previous algorithm scanned 8-character commit and failed

Solution

  • Adjust the bash script to impact the correct one
printf "%s\\n" "/^pick {commit.hexsha[:7]}/ m1" "wq" | ed -s "$tmp_file"
printf "%s\\n" "/^pick {commit.hexsha[:7]} /s//squash {commit.hexsha[:7]} /" "wq" | ed -s "$tmp_file"
"""

I tested it on hr_payroll_period, and it worked

Copy link
Collaborator

@sebalix sebalix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice feature, still didn't test it but I will, thank you!

Comment on lines +16 to +30
TRANSLATION_SUMMARY = [
"Added translation using Weblate",
"Translated using Weblate",
]
SQUASHABLE_SUMMARY = [
"Update translation files",
]
SQUASHABLE_AUTHOR_EMAIL = [
"transbot@odoo-community.org",
"noreply@weblate.org",
"oca-git-bot@odoo-community.org",
"oca+oca-travis@odoo-community.org",
"oca-ci@odoo-community.org",
"shopinvader-git-bot@shopinvader.com",
]
Copy link
Collaborator

@sebalix sebalix Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These values are also declared in port_addon_pr.py module.
I would rename SQUASHABLE_AUTHOR_EMAIL to AUTHOR_EMAILS, then use them in your code below and in port_addon_pr.py with g.AUTHOR_EMAILS and g.TRANSLATION_SUMMARIES in replacement of AUTHOR_EMAILS_TO_SKIP and SUMMARY_TERMS_TO_SKIP

Copy link
Collaborator

@sebalix sebalix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it (quickly), some feedback:

  • like @giarve said, asking the user if he wants to squash each commit doesn't help the workflow (I tested on a module with 30+ translations commits), so the added value compared to a git rebase after the migration done by oca-port is still far easier (but as you said, nobody is doing it!)
  • the bot should detect the list of commits to squash, and propose to the user to squash all of them in a go

@rvalyi
Copy link
Member

rvalyi commented Jan 15, 2025

Thank you for this. However I have been thinking about another complementary approach also. If you look in OCA/l10n-brazil for instance you will find out that may be up to 20% of the commits are weblate commits generating a lot of pollution in the commit history!

What about using a tool like git filter-branch or git filter-repo to filter the i18n module directory entirely and only do one commit with the latest i18n version? This is also equivalent in squashing all commits in i18n even when they are not consequitive.

We are generally not interested in preserving all weblate iterations from the previous module versions...
What do you think?

cc @pedrobaeza @antoniospneto @sbidoul

@pedrobaeza
Copy link
Member

I think that preserving translator attribution is important.

@rvalyi
Copy link
Member

rvalyi commented Jan 15, 2025

I think that preserving translator attribution is important.

Then may be squash the weblate commits together by author? Also this approach can probably be done only during migrations and may be not as effectively during normal ports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants