Merge pull request #1329 from tcm-marcel/feature/clangformat_hunks

Run clang-format on staged hunks only instead of whole file

Author: tli2

script/formatting/formatter.py

@@ -20,7 +20,7 @@
     os.path.abspath(os.path.dirname(__file__)).replace('/formatting', '')
     )
 from helpers import CLANG_FORMAT, PELOTON_DIR, CLANG_FORMAT_FILE, LOG,\
-clang_format
+clang_format, hunks_from_staged_files, hunks_from_last_commits
 
 ## ==============================================
 ## CONFIGURATION
@@ -69,7 +69,7 @@
 ## ==============================================
 
 
-def format_file(file_path, update_header, clang_format_code):
+def format_file(file_path, file_hunks, update_header, clang_format_code):
     """Formats the file passed as argument."""
     file_name = os.path.basename(file_path)
     abs_path = os.path.abspath(file_path)
@@ -104,7 +104,7 @@ def format_file(file_path, update_header, clang_format_code):
             file.write(file_data)
 
         elif clang_format_code:
-            clang_format(file_path)
+            clang_format(file_path, file_hunks)
 
     #END WITH
 #END FORMAT__FILE(FILE_NAME)
@@ -118,7 +118,7 @@ def format_dir(dir_path, update_header, clang_format_code):
             file_path = subdir + os.path.sep + file
 
             if file_path.endswith(".h") or file_path.endswith(".cpp"):
-                format_file(file_path, update_header, clang_format_code)
+                format_file(file_path, None, update_header, clang_format_code)
             #END IF
         #END FOR [file]
     #END FOR [os.walk]
@@ -147,43 +147,58 @@ def format_dir(dir_path, update_header, clang_format_code):
         )
     PARSER.add_argument(
         "-f", "--staged-files",
-        help='Action: Apply the selected action(s) to all staged files (git)',
+        help='Action: Apply the selected action(s) to all staged files (git). ' + 
+        '(clang-format will only touch the staged lines)',
         action='store_true'
         )
+    PARSER.add_argument(
+        "-n", "--number-commits",
+        help='Action: Apply the selected action(s) to all changes of the last ' + 
+        '<n> commits (clang-format will only touch the changed lines)',
+        type=int, default=0
+        )
     PARSER.add_argument(
         'paths', metavar='PATH', type=str, nargs='*',
         help='Files or directories to (recursively) apply the actions to'
         )
 
     ARGS = PARSER.parse_args()
 
+    # TARGETS is a list of files with an optional list of hunks, represented as
+    # pair (start, end) of line numbers, 1 based. 
+    # element of TARGETS: (filename, None) or (filename, [(start,end)])
+
     if ARGS.staged_files:
-        PELOTON_DIR_bytes = bytes(PELOTON_DIR, 'utf-8')
-        TARGETS = [
-            str(os.path.abspath(os.path.join(PELOTON_DIR_bytes, f)), 'utf-8') \
-            for f in subprocess.check_output(
-                ["git", "diff", "--name-only", "HEAD", "--cached",
-                 "--diff-filter=d"
-                ]
-            ).split()]
+        TARGETS = hunks_from_staged_files()
 
         if not TARGETS:
             LOG.error(
                 "no staged files or not calling from a repository -- exiting"
                 )
             sys.exit("no staged files or not calling from a repository")
+            
+    elif ARGS.number_commits > 0:
+        TARGETS = hunks_from_last_commits(ARGS.number_commits)
+
+        if not TARGETS:
+            LOG.error(
+                "no changes could be extracted for formatting -- exiting"
+                )
+            sys.exit("no changes could be extracted for formatting")
+      
     elif not ARGS.paths:
         LOG.error("no files or directories given -- exiting")
         sys.exit("no files or directories given")
+        
     else:
-        TARGETS = ARGS.paths
-
-    for x in TARGETS:
-        if os.path.isfile(x):
-            LOG.info("Scanning file: %s", x)
-            format_file(x, ARGS.update_header, ARGS.clang_format_code)
-        elif os.path.isdir(x):
-            LOG.info("Scanning directory %s", x)
-            format_dir(x, ARGS.update_header, ARGS.clang_format_code)
+        TARGETS = [(f, None) for f in ARGS.paths]
+
+    for f, hunks in TARGETS:
+        if os.path.isfile(f):
+            LOG.info("Scanning file: %s", f)
+            format_file(f, hunks, ARGS.update_header, ARGS.clang_format_code)
+        elif os.path.isdir(f):
+            LOG.info("Scanning directory %s", f)
+            format_dir(f, ARGS.update_header, ARGS.clang_format_code)
     ## FOR
 ## IF

script/helpers.py

@@ -5,6 +5,7 @@
 import logging
 import os
 import subprocess
+import re
 
 from functools import reduce
 
@@ -16,6 +17,9 @@
     # Fill me
 ]
 
+DIFF_FILE_PATT = re.compile(r'^\+\+\+ b\/(.*)')
+DIFF_HUNK_PATT = re.compile(r'^@@ \-\d+(,\d+)? \+(\d+)(,)?(\d+)? @@.*')
+
 ## ==============================================
 ## LOGGING CONFIGURATION
 ## ==============================================
@@ -44,21 +48,20 @@ def find_clangformat():
 CLANG_FORMAT = find_clangformat()
 CLANG_COMMAND_PREFIX = [CLANG_FORMAT, "-style=file"]
 
-def clang_check(file_path):
+def clang_check(file_path, hunks=None):
     """Checks and reports bad code formatting."""
+    
+    assert not file_path is None and not file_path == ""
+    
     rel_path_from_peloton_dir = os.path.relpath(file_path, PELOTON_DIR)
 
     if rel_path_from_peloton_dir in FORMATTING_FILE_WHITELIST:
         return True
 
     file_status = True
 
-    # Run clang-format on the file
-    if CLANG_FORMAT is None:
-        LOG.error("clang-format seems not installed")
-        exit()
-    clang_format_cmd = CLANG_COMMAND_PREFIX + [file_path]
-    formatted_src = subprocess.check_output(clang_format_cmd).splitlines(True)
+    # Run clang-format on the file and get output (not inline!)
+    formatted_src = clang_format(file_path, None, inline=False)
 
     # For Python 3, the above command gives a list of binary sequences, each
     # of which has to be converted to string for diff to operate correctly.
@@ -86,12 +89,84 @@ def clang_check(file_path):
         return file_status
 
 
-def clang_format(file_path):
-    """Formats the file at file_path"""
+def clang_format(file_path, hunks=None, inline=True):
+    """Formats the file at file_path. 
+       'hunks' can be a list of pairs with (start,end) line numbers, 1 based. 
+    """
+    
+    assert not file_path is None and not file_path == ""
+    
     if CLANG_FORMAT is None:
         LOG.error("clang-format seems not installed")
         exit()
 
-    formatting_command = CLANG_COMMAND_PREFIX + ["-i", file_path]
+    formatting_command = CLANG_COMMAND_PREFIX + [file_path]
+    
+    if inline:
+        formatting_command.append("-i")
+    
+    if not hunks is None:
+        for start, end in hunks:
+            if start > 0 and end > 0:
+                formatting_command.append("-lines={}:{}".format(start, end))
+    
     LOG.info(' '.join(formatting_command))
-    subprocess.call(formatting_command)
+    output = subprocess.check_output(formatting_command).splitlines(True)
+    return output
+
+
+def hunks_from_last_commits(n):
+  """ Extract hunks of the last n commits. """
+  
+  assert n > 0
+  
+  diff_output = subprocess.check_output(["git", "diff", "HEAD~"+str(n) , "--diff-filter=d", "--unified=0"]
+  ).decode("utf-8").splitlines()
+  
+  return _hunks_from_diff(diff_output)
+
+
+def hunks_from_staged_files():
+  diff_output = subprocess.check_output(["git", "diff", "HEAD", 
+      "--cached", "--diff-filter=d", "--unified=0"]
+  ).decode("utf-8").splitlines()
+  
+  return _hunks_from_diff(diff_output)
+  
+
+def _hunks_from_diff(diff_output):
+  """ Parse a diff output and extract the hunks of changed files. 
+      The diff output must not have additional lines!
+      (use --unified=0) """
+      
+  # TARGETS is a list of files with an optional list of hunks, represented as
+  # pair (start, end) of line numbers, 1 based. 
+  # element of TARGETS: (filename, None) or (filename, [(start,end)])
+  target_files = []
+  
+  # hunks_current_list serves as a reference to the hunks list of the
+  # last added file
+  hunks_current_list = None
+
+  for line in diff_output:
+      file_match = DIFF_FILE_PATT.search(line)
+      hunk_match = DIFF_HUNK_PATT.search(line)
+      if file_match:
+          file_path = os.path.abspath(os.path.join(PELOTON_DIR, 
+              file_match.group(1)))
+          
+          hunks_current_list = []
+          if file_path.endswith(".h") or file_path.endswith(".cpp"):
+              target_files.append((file_path, hunks_current_list))
+          # If this file is not .cpp/.h the hunks_current_list reference
+          # will point to an empty list which will be discarded later
+      elif hunk_match:
+          # add entry in the hunk list of the last file
+          if hunk_match.group(4) is None:
+              hunk = (int(hunk_match.group(2)), int(hunk_match.group(2)))
+          else:
+              hunk = (int(hunk_match.group(2)), int(hunk_match.group(2)) + 
+                  int(hunk_match.group(4)))
+          hunks_current_list.append(hunk)
+  
+  return target_files