modules: Restructure git source checks

- Drop module-{module_name}-source-git-local-path, url is already
  mandatory
- Add module-{module_name}-source-git-no-tag-commit-branch when url is
  present but not branch, commit or tag. Slight rename of
  `module-{module_name}-source-git-no-commit-or-tag`
- Add module-{module_name}-source-git-commit-invalid when git commit
  hash is an invalid commit

flatpak_builder_lint/checks/modules.py

@@ -1,6 +1,12 @@
+import re
+
 from . import Check
 
 
+def _is_git_commit_hash(s: str) -> bool:
+    return re.match(r"[a-f0-9]{4,40}", s) is not None
+
+
 class ModuleCheck(Check):
     def check_source(self, module_name: str, source: dict) -> None:
         source_type = source.get("type")
@@ -19,26 +25,22 @@ def check_source(self, module_name: str, source: dict) -> None:
             commit = source.get("commit")
             branch = source.get("branch")
             tag = source.get("tag")
-            branch_committish = False
-
-            if branch:
-                if len(branch) != 40:
-                    self.errors.add(f"module-{module_name}-source-git-branch")
-                else:
-                    branch_committish = True
-
-            if not branch_committish and not commit and not tag:
-                self.errors.add(f"module-{module_name}-source-git-no-commit-or-tag")
-
-            if source.get("path"):
-                self.errors.add(f"module-{module_name}-source-git-local-path")
-
             url = source.get("url")
+
             if not url:
                 self.errors.add(f"module-{module_name}-source-git-no-url")
-            elif not url.startswith("https:") and not url.startswith("http:"):
+                return
+
+            if url and not url.startswith(("http://", "https://")):
                 self.errors.add(f"module-{module_name}-source-git-url-not-http")
 
+            if not any([commit, branch, tag]):
+                self.errors.add(f"module-{module_name}-source-git-no-tag-commit-branch")
+                return
+
+            if branch and not _is_git_commit_hash(branch):
+                self.errors.add(f"module-{module_name}-source-git-branch")
+
     def check_module(self, module: dict) -> None:
         name = module.get("name")
         buildsystem = module.get("buildsystem", "autotools")

flatpak_builder_lint/staticfiles/exceptions.json

@@ -1007,7 +1007,7 @@
         "finish-args-flatpak-spawn-access": "application requires ptyxis-agent for PTY setup on host"
     },
     "org.gnome.Totem.Devel": {
-        "module-totem-source-git-no-commit-or-tag": "This is the development version of Totem, tracking upstream",
+        "module-totem-source-git-no-tag-commit-branch": "This is the development version of Totem, tracking upstream",
         "appstream-id-mismatch-flatpak-id": "appstream-cid predates this linter rule"
     },
     "org.gimp.GIMP": {

tests/manifests/modules.json

@@ -8,27 +8,6 @@
                 "-DCMAKE_BUILD_TYPE=Debug"
             ],
             "sources": [
-                {
-                    "type": "git",
-                    "path": "."
-                },
-                {
-                    "type": "git",
-                    "url": "ssh://foobar.git",
-                    "tag": "foo",
-                    "dest-filename": "foo/bar"
-                },
-                {
-                    "type": "git",
-                    "url": "ssh://foobar2.git",
-                    "branch": "master",
-                    "commit": "1234567890abcdef1234567890abcdef12345678"
-                },
-                {
-                    "type": "git",
-                    "url": "ssh://foobar3.git",
-                    "branch": "1234567890abcdef1234567890abcdef12345678"
-                },
                 {
                     "type": "archive",
                     "url": "https://example.com/foo.tar.gz",

tests/manifests/modules_git_disallowed.json

@@ -5,8 +5,7 @@
             "name": "module1",
             "sources": [
                 {
-                    "type": "git",
-                    "url": "https://foobar2.git"
+                    "type": "git"
                 }
             ]
         },
@@ -23,7 +22,9 @@
             "name": "module3",
             "sources": [
                 {
-                    "type": "git"
+                    "type": "git",
+                    "url": "ssh://foobar2.git",
+                    "tag": "foo"
                 }
             ]
         },
@@ -32,8 +33,7 @@
             "sources": [
                 {
                     "type": "git",
-                    "url": "ssh://foobar2.git",
-                    "commit": "8bdc272f381e2633b96d1846994c7cb0f90f079f"
+                    "url": "https://example.org/foobar2.git"
                 }
             ]
         },
@@ -42,8 +42,8 @@
             "sources": [
                 {
                     "type": "git",
-                    "url": "https://foobar2.git",
-                    "branch": "branch_name"
+                    "url": "https://example.org/foobar2.git",
+                    "branch": "foobarfoo" 
                 }
             ]
         }

tests/test_manifest.py

@@ -224,24 +224,15 @@ def test_manifest_finish_args_empty() -> None:
 
 
 def test_manifest_modules() -> None:
-    errors = {
-        "module-module1-source-git-no-commit-or-tag",
-        "module-module1-source-git-local-path",
-        "module-module1-source-git-no-url",
-        "module-module1-source-git-url-not-http",
-    }
-
     warnings = {
         "module-module1-buildsystem-is-plain-cmake",
         "module-module1-cmake-non-release-build",
         "module-module1-source-sha1-deprecated",
     }
 
     ret = run_checks("tests/manifests/modules.json")
-    found_errors = set(ret["errors"])
     found_warnings = set(ret["warnings"])
 
-    assert errors.issubset(found_errors)
     assert warnings.issubset(found_warnings)
 
 
@@ -253,8 +244,16 @@ def test_manifest_modules_git_allowed() -> None:
 
 def test_manifest_modules_git_disallowed() -> None:
     ret = run_checks("tests/manifests/modules_git_disallowed.json")
+    errors = {
+        "module-module1-source-git-no-url",
+        "module-module2-source-git-no-url",
+        "module-module3-source-git-url-not-http",
+        "module-module4-source-git-no-tag-commit-branch",
+        "module-module5-source-git-branch",
+    }
     found_errors = set(ret["errors"])
-    assert [x for x in found_errors if x.startswith("module-")]
+    for e in errors:
+        assert e in found_errors
 
 
 def test_manifest_exceptions() -> None: