From 0eebd0000be4f5f01dd5c83d0c87cc5eb278ae7b Mon Sep 17 00:00:00 2001 From: Jairo Llopis Date: Tue, 31 Oct 2023 11:03:42 +0000 Subject: [PATCH] fix(oca-gen-addon-readme): reduce unneeded merge conflicts As seen in https://github.com/OCA/oca-addons-repo-template/pull/190#issuecomment-1784076360, 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 https://github.com/OCA/oca-addons-repo-template/pull/220#issuecomment-1786814130, 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 #220. [1]: https://git-scm.com/docs/gitattributes.html#Documentation/gitattributes.txt-union --- src/.gitattributes | 3 + src/.pre-commit-config.yaml.jinja | 19 ++++--- tests/test_pre_commit.py | 94 +++++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+), 8 deletions(-) create mode 100644 src/.gitattributes diff --git a/src/.gitattributes b/src/.gitattributes new file mode 100644 index 0000000..cd4c8cf --- /dev/null +++ b/src/.gitattributes @@ -0,0 +1,3 @@ +*/README.rst merge=union +*/static/description/index.html merge=union +requirements.txt merge=union diff --git a/src/.pre-commit-config.yaml.jinja b/src/.pre-commit-config.yaml.jinja index 63f4764..e2993b9 100644 --- a/src/.pre-commit-config.yaml.jinja +++ b/src/.pre-commit-config.yaml.jinja @@ -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: @@ -235,3 +229,12 @@ repos: args: - --rcfile=.pylintrc-mandatory {%- endif %} + - <<: *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 diff --git a/tests/test_pre_commit.py b/tests/test_pre_commit.py index 2a8f338..062cca1 100644 --- a/tests/test_pre_commit.py +++ b/tests/test_pre_commit.py @@ -1,3 +1,4 @@ +import re from pathlib import Path from copier import run_copy @@ -17,3 +18,96 @@ 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.""" + # 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") + 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", "main") + 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", "main") + # 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()