util: Refactor resolving git url

Split out a smaller function that receives a git repo URL and ref and
returns the commit id. The caching resolver class is modified to use
this second function if branch is given to it.

The new function checks if the ref received already looks like a commit
ID, and if so it does not attempt to do anything with it.

Signed-off-by: Lubomír Sedlář <lsedlar@redhat.com>

pungi/util.py

@@ -250,6 +250,32 @@ class GitUrlResolveError(RuntimeError):
     pass
 
 
+def resolve_git_ref(repourl, ref):
+    """Resolve a reference in a Git repo to a commit.
+
+    Raises RuntimeError if there was an error. Most likely cause is failure to
+    run git command.
+    """
+    if re.match(r"^[a-f0-9]{40}$", ref):
+        # This looks like a commit ID already.
+        return ref
+
+    _, output = git_ls_remote(repourl, ref)
+
+    lines = [line for line in output.split('\n') if line]
+    if len(lines) == 0:
+        # Branch does not exist in remote repo
+        raise GitUrlResolveError(
+            "Failed to resolve %s: ref does not exist in remote repo" % repourl
+        )
+    if len(lines) != 1:
+        # This should never happen. HEAD can not match multiple commits in a
+        # single repo, and there can not be a repo without a HEAD.
+        raise GitUrlResolveError("Failed to resolve %s", repourl)
+
+    return lines[0].split()[0]
+
+
 def resolve_git_url(url):
     """Given a url to a Git repo specifying HEAD or origin/<branch> as a ref,
     replace that specifier with actual SHA1 of the commit.
@@ -269,20 +295,8 @@ def resolve_git_url(url):
     scheme = r.scheme.replace('git+', '')
 
     baseurl = urllib.parse.urlunsplit((scheme, r.netloc, r.path, '', ''))
-    _, output = git_ls_remote(baseurl, ref)
+    fragment = resolve_git_ref(baseurl, ref)
 
-    lines = [line for line in output.split('\n') if line]
-    if len(lines) == 0:
-        # Branch does not exist in remote repo
-        raise GitUrlResolveError(
-            "Failed to resolve %s: ref does not exist in remote repo" % url
-        )
-    if len(lines) != 1:
-        # This should never happen. HEAD can not match multiple commits in a
-        # single repo, and there can not be a repo without a HEAD.
-        raise GitUrlResolveError("Failed to resolve %s", url)
-
-    fragment = lines[0].split()[0]
     result = urllib.parse.urlunsplit((r.scheme, r.netloc, r.path, r.query, fragment))
     if '?#' in url:
         # The urllib library drops empty query string. This hack puts it back in.
@@ -298,17 +312,19 @@ class GitUrlResolver(object):
         self.offline = offline
         self.cache = {}
 
-    def __call__(self, url):
+    def __call__(self, url, branch=None):
         if self.offline:
             return url
-        if url not in self.cache:
+        key = (url, branch)
+        if key not in self.cache:
             try:
-                self.cache[url] = resolve_git_url(url)
+                res = resolve_git_ref(url, branch) if branch else resolve_git_url(url)
+                self.cache[key] = res
             except GitUrlResolveError as exc:
-                self.cache[url] = exc
-        if isinstance(self.cache[url], GitUrlResolveError):
-            raise self.cache[url]
-        return self.cache[url]
+                self.cache[key] = exc
+        if isinstance(self.cache[key], GitUrlResolveError):
+            raise self.cache[key]
+        return self.cache[key]
 
 
 # fomat: {arch|*: [data]}

tests/test_util.py

@@ -43,6 +43,10 @@ class TestGitRefResolver(unittest.TestCase):
         run.assert_called_once_with(['git', 'ls-remote', 'https://git.example.com/repo.git', 'refs/heads/f24'],
                                     universal_newlines=True)
 
+    def test_resolve_ref_with_commit_id(self):
+        ref = util.resolve_git_ref("https://git.example.com/repo.git", "a" * 40)
+        self.assertEqual(ref, "a" * 40)
+
     @mock.patch('pungi.util.run')
     def test_resolve_missing_spec(self, run):
         url = util.resolve_git_url('https://git.example.com/repo.git')
@@ -121,18 +125,34 @@ class TestGitRefResolver(unittest.TestCase):
         )
         self.assertEqual(mock_resolve.call_args_list, [])
 
+    @mock.patch("pungi.util.resolve_git_ref")
     @mock.patch("pungi.util.resolve_git_url")
-    def test_resolver_caches_calls(self, mock_resolve):
+    def test_resolver_caches_calls(self, mock_resolve_url, mock_resolve_ref):
         url1 = "http://example.com/repo.git#HEAD"
         url2 = "http://example.com/repo.git#master"
-        mock_resolve.side_effect = ["1", "2"]
+        url3 = "http://example.com/repo.git"
+        ref1 = "foo"
+        ref2 = "bar"
+        mock_resolve_url.side_effect = ["1", "2"]
+        mock_resolve_ref.side_effect = ["cafe", "beef"]
         resolver = util.GitUrlResolver()
         self.assertEqual(resolver(url1), "1")
         self.assertEqual(resolver(url1), "1")
+        self.assertEqual(resolver(url3, ref1), "cafe")
+        self.assertEqual(resolver(url3, ref2), "beef")
         self.assertEqual(resolver(url2), "2")
+        self.assertEqual(resolver(url3, ref1), "cafe")
         self.assertEqual(resolver(url1), "1")
+        self.assertEqual(resolver(url3, ref2), "beef")
         self.assertEqual(resolver(url2), "2")
-        self.assertEqual(mock_resolve.call_args_list, [mock.call(url1), mock.call(url2)])
+        self.assertEqual(resolver(url3, ref2), "beef")
+        self.assertEqual(
+            mock_resolve_url.call_args_list, [mock.call(url1), mock.call(url2)]
+        )
+        self.assertEqual(
+            mock_resolve_ref.call_args_list,
+            [mock.call(url3, ref1), mock.call(url3, ref2)],
+        )
 
     @mock.patch("pungi.util.resolve_git_url")
     def test_resolver_caches_failure(self, mock_resolve):