Skip to content

Commit

Permalink
fix(oca-gen-addon-readme): reduce unneeded merge conflicts
Browse files Browse the repository at this point in the history
As seen in OCA#190 (comment), when somebody tried to merge 2 PRs for the same module, chances are that both imply different readme digests and the merge fails.

As seen in OCA#220 (comment), this was already happening under some circumstances with `requirements.txt` files.

Here I added a `.gitattributes` file which specifies a `merge=union` driver for those files. [Git docs][1] explain that this driver will just keep the changes from all files, marking the merge as succeeded and not including merge markers. This is not ideal, but is OK for our current situation because:

1. Duplicate/conflicting lines in those files should not really harm a lot.
2. The bot will re-render the README upon merge anyways, fixing that conflict automatically.
3. We could define a custom merge driver, but `union` is built into Git, meaning less configurations required downstream.

Still, it doesn't seem to work in octopus merges. At least, if you do consecutive merges instead, all will work as expected.

At the same time, I moved the hook to be the last. This is because previous hooks can reformat the files used to obtain the digest.

Closes OCA#220.

[1]: https://git-scm.com/docs/gitattributes.html#Documentation/gitattributes.txt-union
  • Loading branch information
yajo committed Nov 8, 2023
1 parent 967f1f4 commit fb3a5ce
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 8 deletions.
3 changes: 3 additions & 0 deletions src/.gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
*/README.rst merge=union
*/static/description/index.html merge=union
requirements.txt merge=union
19 changes: 11 additions & 8 deletions src/.pre-commit-config.yaml.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -101,20 +101,14 @@ repos:
entry: found a en.po file
language: fail
files: '[a-zA-Z0-9_]*/i18n/en\.po$'
- repo: https://github.com/oca/maintainer-tools
- &maintainer_tools
repo: https://github.com/oca/maintainer-tools
rev: {{ repo_rev.maintainer_tools }}
hooks:
# update the NOT INSTALLABLE ADDONS section above
- id: oca-update-pre-commit-excluded-addons
- id: oca-fix-manifest-website
args: ["{{ repo_website }}"]
- id: oca-gen-addon-readme
args:
- --addons-dir=.
- --branch={{ "%.01f" | format(odoo_version) }}
- --org-name={{ org_slug }}
- --repo-name={{ repo_slug }}
- --if-source-changed
- repo: https://github.com/OCA/odoo-pre-commit-hooks
rev: {{ repo_rev.odoo_pre_commit_hooks }}
hooks:
Expand Down Expand Up @@ -234,4 +228,13 @@ repos:
- id: pylint_odoo
args:
- --rcfile=.pylintrc-mandatory
- <<: *maintainer_tools
hooks:
- id: oca-gen-addon-readme
args:
- --addons-dir=.
- --branch={{ "%.01f" | format(odoo_version) }}
- --org-name={{ org_slug }}
- --repo-name={{ repo_slug }}
- --if-source-changed
{%- endif %}
98 changes: 98 additions & 0 deletions tests/test_pre_commit.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import re
from pathlib import Path

import pytest
from copier import run_copy
from plumbum import local
from plumbum.cmd import git, pre_commit
Expand All @@ -17,3 +19,99 @@ def test_hooks_installable(tmp_path: Path, odoo_version: float, cloned_template:
with local.cwd(tmp_path):
git("init")
pre_commit("install-hooks")


def test_no_readme_generator_conflicts(
tmp_path: Path, cloned_template: Path, odoo_version: Path
):
"""Test that you can merge more than one PR targeting the same module."""
if odoo_version < 14:
pytest.skip("Older hooks don't generate README.rst files")
# Generate a repo
data = {
"odoo_version": odoo_version,
"repo_slug": "oca-addons-repo-template",
"repo_name": "Test repo",
"repo_description": "Test repo description",
}
run_copy(str(cloned_template), tmp_path, data=data, defaults=True, unsafe=True)
with local.cwd(tmp_path):
git("init")
git("checkout", "-b", odoo_version)
pre_commit("install")
git("add", "-A")
git("commit", "-m", "[BUILD] hello world", retcode=1)
git("commit", "-am", "[BUILD] hello world")
# Create a module
Path("useless_module").mkdir()
with local.cwd("useless_module"):
Path("__init__.py").touch()
Path("README.rst").touch()
Path("__manifest__.py").write_text(
repr(
{
"name": "Useless module",
"version": f"{odoo_version}.0.1.0",
"author": "Odoo Community Association (OCA)",
"summary": "A module that does nothing.",
"license": "LGPL-3",
"website": "https://github.com/OCA/oca-addons-repo-template",
}
)
)
Path("readme").mkdir()
Path("readme", "DESCRIPTION.md").write_text(
"This module is absurdly useless.\n"
)
# Commit it and let pre-commit reformat it
git("add", "-A")
git("commit", "-m", "[ADD] useless_module", retcode=1)
git("add", "-A")
git("commit", "-m", "[ADD] useless_module")
# At this point, the README contains the dir hash
orig_digest = re.search(
r"source digest: (.*)", Path("README.rst").read_text()
).group(1)
assert orig_digest
assert Path("static", "description", "index.html").is_file()
# Change the module, and thus the digest in two different branches
git("checkout", "-b", "change1")
Path("readme", "USAGE.md").write_text("You cannot use this.\n")
git("add", "-A")
git("commit", "-m", "[IMP] useless_module: Usage", retcode=1)
git("commit", "-am", "[IMP] useless_module: Usage")
chg1_digest = re.search(
r"source digest: (.*)", Path("README.rst").read_text()
).group(1)
assert chg1_digest
git("checkout", odoo_version)
git("checkout", "-b", "change2")
Path("readme", "CONFIGURE.md").write_text("You cannot configure this.\n")
git("add", "-A")
git("commit", "-m", "[IMP] useless_module: Configuration", retcode=1)
git("commit", "-am", "[IMP] useless_module: Configuration")
chg2_digest = re.search(
r"source digest: (.*)", Path("README.rst").read_text()
).group(1)
assert chg2_digest
assert orig_digest != chg1_digest != chg2_digest
git("checkout", odoo_version)
# Merge the two branches and check there are no conflicts
git("merge", "change1")
assert f"source digest: {orig_digest}" not in Path("README.rst").read_text()
assert f"source digest: {chg1_digest}" in Path("README.rst").read_text()
assert f"source digest: {chg2_digest}" not in Path("README.rst").read_text()
git("merge", "change2")
assert f"source digest: {orig_digest}" not in Path("README.rst").read_text()
assert f"source digest: {chg1_digest}" in Path("README.rst").read_text()
assert f"source digest: {chg2_digest}" in Path("README.rst").read_text()
# Pre-commit can still fix the README
assert not git("status", "--porcelain=v1")
pre_commit("run", "-a", retcode=1)
assert git("status", "--porcelain=v1") == (
" M useless_module/README.rst\n"
" M useless_module/static/description/index.html\n"
)
assert f"source digest: {orig_digest}" not in Path("README.rst").read_text()
assert f"source digest: {chg1_digest}" not in Path("README.rst").read_text()
assert f"source digest: {chg2_digest}" not in Path("README.rst").read_text()

0 comments on commit fb3a5ce

Please sign in to comment.