Merge pull request #27 from ccdc-opensource/no_jira_sanitise_filenames

This commit switches from using Shell=True in running commands inside the githooks to avoid problems with filenames containing certain characters that would need escaping otherwise.

Author: jasonccole

.github/workflows/quality_check.yml

@@ -7,7 +7,7 @@ jobs:
       - uses: actions/checkout@v3
       - uses: actions/setup-python@v4
         with:
-          python-version: "3.7"
+          python-version: "3.11"
       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip

.github/workflows/status_check.yml

@@ -13,7 +13,7 @@ jobs:
           fetch-depth: 0
       - uses: actions/setup-python@v4
         with:
-          python-version: "3.7"
+          python-version: "3.11"
       - name: Get the commit message
         run: |
           echo 'commit_message<<EOF' >> $GITHUB_ENV

main/githooks.py

@@ -56,9 +56,8 @@
 # File types that need a terminating newline
 TERMINATING_NEWLINE_EXTS = ['.c', '.cpp', '.h', '.inl']
 
-
-def _get_output(command, cwd='.'):
-    return subprocess.check_output(command, shell=True, cwd=cwd).decode(errors='replace')
+def _get_output(command_list, cwd='.'):
+    return subprocess.check_output(command_list, cwd=cwd).decode(errors='replace')
 
 
 def _is_github_event():
@@ -91,7 +90,7 @@ def get_user():
     if _is_github_event():
         return os.environ['GITHUB_ACTOR']
     else:
-        output = _get_output('git var GIT_AUTHOR_IDENT')
+        output = _get_output(['git', 'var', 'GIT_AUTHOR_IDENT'])
         match = re.match(r'^(.+) <', output)
         return match.group(1)
 
@@ -104,7 +103,7 @@ def get_branch():
         else:
             return os.environ['GITHUB_REF'].split('/')[-1]
     else:
-        return _get_output('git branch').split()[-1]
+        return _get_output(['git', 'branch']).split()[-1]
 
 
 def get_file_content_as_binary(filename):
@@ -121,7 +120,7 @@ def get_file_content_as_binary(filename):
             _skip(filename, 'File is not UTF-8 encoded')
             data = None
     else:
-        data = _get_output(f'git show :{filename}')
+        data = _get_output(['git','show', f':{filename}'])
     return data
 
 
@@ -134,7 +133,7 @@ def get_text_file_content(filename):
     if _is_github_event() or 'pytest' in sys.modules:
         data = Path(filename).read_text()
     else:
-        data = _get_output(f'git show :{filename}')
+        data = _get_output(['git', 'show', f':{filename}'])
     return data
 
 
@@ -147,7 +146,7 @@ def get_sha():
     GITHUB_SHA cannot be used because in a pull request it gives the sha of the
     fake merge commit.
     '''
-    return _get_output(f'git rev-parse {get_branch()}')
+    return _get_output(['git','rev-parse', get_branch()])
 
 
 def get_event():
@@ -160,13 +159,12 @@ def get_event():
 
 def get_branch_files():
     '''Get all files in branch'''
-    branch = get_branch()
-    return _get_output(f'git ls-tree -r {branch} --name-only').splitlines()
+    return _get_output(['git','ls-tree', '-r', get_branch(),'--name-only']).splitlines()
 
 
 def add_file_to_index(filename):
     '''Add file to current commit'''
-    return _get_output(f'git add {filename}')
+    return _get_output(['git','add',filename])
 
 
 def get_commit_files():
@@ -178,12 +176,15 @@ def get_commit_files():
 
     '''
     if _is_github_event():
+        commands = ['git','diff','--ignore-submodules','--name-status']
         if _is_pull_request():
-            output = _get_output(f'git diff --ignore-submodules --name-status remotes/origin/{os.environ["GITHUB_BASE_REF"]}..remotes/origin/{os.environ["GITHUB_HEAD_REF"]} --')
+            commands += [f'remotes/origin/{os.environ["GITHUB_BASE_REF"]}..remotes/origin/{os.environ["GITHUB_HEAD_REF"]}','--']
         else:
-            output = _get_output('git diff --ignore-submodules --name-status HEAD~.. --')
+            commands += ['HEAD~..', '--']
     else:
-        output = _get_output('git diff-index --ignore-submodules HEAD --cached')
+        commands = ['git', 'diff-index', '--ignore-submodules', 'HEAD', '--cached']
+        
+    output = _get_output(commands)
     result = defaultdict(list)
     for line in output.splitlines():
         parts = line.split()
@@ -242,15 +243,14 @@ def get_changed_lines(modified_file):
     :returns: A list of line number (integers and ranges) of changed lines
     '''
     if _is_github_event():
+        commands = ['git','diff','--unified=0']
         if _is_pull_request():
-            output = _get_output(
-                    f'git diff --unified=0 remotes/origin/{os.environ["GITHUB_BASE_REF"]}..remotes/origin/{os.environ["GITHUB_HEAD_REF"]} -- {modified_file}')
+            commands += [f'remotes/origin/{os.environ["GITHUB_BASE_REF"]}..remotes/origin/{os.environ["GITHUB_HEAD_REF"]}', '--',f'{modified_file}']
         else:
-            output = _get_output(
-                    f'git diff --unified=0 HEAD~ {modified_file}')
+            commands += ['HEAD~', f'{modified_file}']
     else:
-        output = _get_output(
-                f'git diff-index HEAD --unified=0 {modified_file}')
+        commands = [f'git', 'diff-index', 'HEAD', '--unified=0', f'{modified_file}']
+    output = _get_output(commands)
 
     lines = []
     for line in output.splitlines():
@@ -283,7 +283,7 @@ def _test(input, output):
 def get_config_setting(setting):
     '''Get the value of a config setting'''
     try:
-        return _get_output(f'git config --get {setting}').strip()
+        return _get_output(['git', 'config', '--get', setting]).strip()
     except subprocess.CalledProcessError:
         return None
 
@@ -460,20 +460,24 @@ class TestTrimTrailingWhitespace(unittest.TestCase):
     def test_trim_trailing_whitespace(self):
         content = 'first line\nsecond line \nthird line '
         trimmed_content = 'first line\nsecond line\nthird line'
-        with NamedTemporaryFile() as tmp:
-            Path(tmp.name).write_text(content)
 
+        name = NamedTemporaryFile().name        
+        try:
+            Path(name).write_text(content)
             # Trailing whitespace found
-            retval = trim_trailing_whitespace_in_file(tmp.name, True, True)
+            retval = trim_trailing_whitespace_in_file(name, True, True)
             self.assertEqual(retval, 1)
-            self.assertEqual(Path(tmp.name).read_text(), content)
+            self.assertEqual(Path(name).read_text(), content)
 
             # Now remove the trailing whitespace
-            trim_trailing_whitespace_in_file(tmp.name, True, False, False)
+            trim_trailing_whitespace_in_file(name, True, False, False)
             # Trailing whitespace no longer found
-            self.assertEqual(Path(tmp.name).read_text(), trimmed_content)
-            retval = trim_trailing_whitespace_in_file(tmp.name, True, True)
+            self.assertEqual(Path(name).read_text(), trimmed_content)
+            retval = trim_trailing_whitespace_in_file(name, True, True)
             self.assertEqual(retval, 0)
+        finally:
+            Path(name).unlink()
+
 
     def test_decodeerror(self):
         # A text file that is not utf-8 encoded - report and skip