Update url validity check with better messages and tag priority (#20287)

* Update url validity check with better messages and tag priority

This is capturing the requirements from #20286 more clearly.

* Allow source entries to be tags in some cases.

If test_pull_requests and test_commits are both explicitly false, this
will allow the source entry to contain tags.

* Add support for using commit ids as version.

Co-authored-by: Kei Okada <[email protected]>

* Always clean up tempdir.

* Only check commit id if the version matches a 40 char id string.

* fixing check priority

test/test_url_validity.py

@@ -5,8 +5,11 @@
 
 from io import StringIO
 import os
+import re
+import shutil
 import subprocess
 import sys
+import tempfile
 import unittest
 try:
     from urllib.parse import urlparse
@@ -73,50 +76,89 @@ def detect_lines(diffstr):
     return resultant_lines
 
 
-def check_git_remote_exists(url, version, tags_valid=False):
+def check_git_remote_exists(url, version, tags_valid=False, commits_valid=False):
     """ Check if the remote exists and has the branch version.
     If tags_valid is True query tags as well as branches """
-    cmd = ('git ls-remote %s refs/heads/*' % url).split()
 
-    try:
-        output = subprocess.check_output(cmd).decode('utf-8')
-    except:
-        return False
-    if not version:
-        # If the above passed assume the default exists
-        return True
+    # Check for tags first as they take priority.
+    # From Cloudbees Support:
+    #  >the way git plugin handles this conflict, a tag/sha1 is always preferred to branch as this is the way most user use an existing job to trigger a release build.
+    #  Catching the corner case to #20286
 
-    if 'refs/heads/%s' % version in output:
-        return True
-
-    # If tags are valid. query for all tags and test for version
-    if not tags_valid:
-        return False
+    tag_match = False
     cmd = ('git ls-remote %s refs/tags/*' % url).split()
 
     try:
-        output = subprocess.check_output(cmd).decode('utf-8')
-    except:
-        return False
+        tag_list = subprocess.check_output(cmd).decode('utf-8')
+    except subprocess.CalledProcessError as ex:
+        return (False, 'subprocess call %s failed: %s' % (cmd, ex))
+
+    if 'refs/tags/%s' % version in tag_list:
+        tag_match = True
+
+    if tag_match:
+        if tags_valid:
+            return (True, '')
+        else:
+            error_str = 'Tags are not valid, but a tag %s was found. ' % version
+            error_str += 'Re: https://github.com/ros/rosdistro/pull/20286'
+            return (False, error_str)
 
-    if 'refs/tags/%s' % version in output:
-        return True
-    return False
+    branch_match = False
+    # check for branch name
+    cmd = ('git ls-remote %s refs/heads/*' % url).split()
+
+    commit_match = False
+    # Only try to match a full length git commit id as this is an expensive operation
+    if re.match('[0-9a-f]{40}', version):
+        try:
+            tmpdir = tempfile.mkdtemp()
+            subprocess.check_call('git clone %s %s/git-repo' % (url, tmpdir), shell=True)
+            # When a commit id is not found it results in a non-zero exit and the message
+            # 'error: malformed object name...'.
+            subprocess.check_call('git -C %s/git-repo branch -r --contains %s' % (tmpdir, version), shell=True)
+            commit_match = True
+        except:
+            pass #return (False, 'No commit found matching %s' % version)
+        finally:
+            shutil.rmtree(tmpdir)
+
+    if commit_match:
+        if commits_valid:
+            return (True, '')
+        else:
+            error_str = 'Commits are not valid, but a commit %s was found. ' % version
+            error_str += 'Re: https://github.com/ros/rosdistro/pull/20286'
+            return (False, error_str)
+
+    # Commits take priority only check for the branch after checking for tags and commits first
+    try:
+        branch_list = subprocess.check_output(cmd).decode('utf-8')
+    except subprocess.CalledProcessError as ex:
+        return (False, 'subprocess call %s failed: %s' % (cmd, ex))
+    if not version:
+        # If the above passed assume the default exists
+        return (True, '')
 
+    if 'refs/heads/%s' % version in branch_list:
+        return (True, '')
+    return (False, 'No branch found matching %s' % version)
+
 
-def check_source_repo_entry_for_errors(source, tags_valid=False):
+def check_source_repo_entry_for_errors(source, tags_valid=False, commits_valid=False):
     errors = []
     if source['type'] != 'git':
         print('Cannot verify remote of type[%s] from line [%s] skipping.'
               % (source['type'], source['__line__']))
         return None
 
     version = source['version'] if source['version'] else None
-    if not check_git_remote_exists(source['url'], version, tags_valid):
+    (remote_exists, error_reason) = check_git_remote_exists(source['url'], version, tags_valid, commits_valid)
+    if not remote_exists:
         errors.append(
             'Could not validate repository with url %s and version %s from'
-            ' entry at line %s'
-            % (source['url'], version, source['__line__']))
+            ' entry at line %s. Error reason: %s'
+            % (source['url'], version, source['__line__'], error_reason))
     test_pr = source['test_pull_requests'] if 'test_pull_requests' in source else None
     if test_pr:
         parsedurl = urlparse(source['url'])
@@ -141,12 +183,17 @@ def check_source_repo_entry_for_errors(source, tags_valid=False):
 def check_repo_for_errors(repo):
     errors = []
     if 'source' in repo:
-        source_errors = check_source_repo_entry_for_errors(repo['source'])
+        source = repo['source']
+        test_prs = source['test_pull_requests'] if 'test_pull_requests' in source else None
+        test_commits = source['test_commits'] if 'test_commits' in source else None
+        # Allow tags in source entries if test_commits and test_pull_requests are both explicitly false.
+        tags_and_commits_valid = True if test_prs is False and test_commits is False else False
+        source_errors = check_source_repo_entry_for_errors(repo['source'], tags_and_commits_valid, tags_and_commits_valid)
         if source_errors:
             errors.append('Could not validate source entry for repo %s with error [[[%s]]]' %
                           (repo['repo'], source_errors))
     if 'doc' in repo:
-        source_errors = check_source_repo_entry_for_errors(repo['doc'], tags_valid=True)
+        source_errors = check_source_repo_entry_for_errors(repo['doc'], tags_valid=True, commits_valid=True)
         if source_errors:
             errors.append('Could not validate doc entry for repo %s with error [[[%s]]]' %
                           (repo['repo'], source_errors))