Gerrit Whitespace Change Detection

Consider whitespace changes significant enough not to reapply code
reviews, but still comment if that's all which changed between
patchsets. This addresses bug 1057506.

* modules/openstack_project/files/gerrit/scripts/trivial_rebase.py
(GetPatchId): Add a flag called consider_whitespace, but defaulting to
False so as to preserve default behavior of the module. Add conditional
behavior to replace all spaces and tabs with percent signs before
calculating the patch-id hash.
(Main): Add a --whitespace command-line option to turn on whitespace
change checking. If enabled and if normal GetPatchId calls return a
match, re-run with consider_whitespace set to True and apply a comment
to the new patchset in Gerrit if the result is non-matching.

* modules/openstack_project/templates/gerrit_patchset-created.erb: Add
--whitespace to the trivial_rebase.py invocation, enabling whitespace
checking.

Change-Id: I89c479614a637717cf515a5d3f6d03b5f7830581
Reviewed-on: https://review.openstack.org/13775
Reviewed-by: James E. Blair <corvus@inaugust.com>
Reviewed-by: Clark Boylan <clark.boylan@gmail.com>
Reviewed-by: Paul Belanger <paul.belanger@polybeacon.com>
Approved: Monty Taylor <mordred@inaugust.com>
Reviewed-by: Monty Taylor <mordred@inaugust.com>
Tested-by: Jenkins

modules/openstack_project/files/gerrit/scripts/trivial_rebase.py

@@ -124,12 +124,24 @@ def GetApprovals(options):
       approvals.append(dict["columns"])
   return approvals
 
-def GetPatchId(revision):
+def GetPatchId(revision, consider_whitespace=False):
   git_show_cmd = ['git', 'show', revision]
   patch_id_cmd = ['git', 'patch-id']
   patch_id_process = subprocess.Popen(patch_id_cmd, stdout=subprocess.PIPE,
                                       stdin=subprocess.PIPE)
   git_show_process = subprocess.Popen(git_show_cmd, stdout=subprocess.PIPE)
+  if consider_whitespace:
+    # This matches on change lines in the patch (those starting with "+"
+    # or "-" but not followed by another of the same), then replaces any
+    # space or tab characters with "%" before calculating a patch-id.
+    replace_ws_cmd = ['sed', r'/^\(+[^+]\|-[^-]\)/y/ \t/%%/']
+    replace_ws_process = subprocess.Popen(replace_ws_cmd,
+                                          stdout=subprocess.PIPE,
+                                          stdin=subprocess.PIPE)
+    return patch_id_process.communicate(
+        replace_ws_process.communicate(git_show_process.communicate()[0])[0]
+        )[0]
+  else:
     return patch_id_process.communicate(git_show_process.communicate()[0])[0]
 
 def SuExec(options, as_user, cmd):
@@ -164,6 +176,8 @@ def Main():
   parser.add_option("--server", dest="server", default="localhost",
                     help="Server name/address for Gerrit's SSH daemon "
                          "[default: %default]")
+  parser.add_option("--whitespace", action="store_true",
+                    help="Treat whitespace as significant")
 
   (options, args) = parser.parse_args()
 
@@ -185,8 +199,25 @@ def Main():
     # patch-ids don't match
     exit(0)
   # Patch ids match. This is a trivial rebase.
-  # In addition to patch-id we should check if the commit message changed. Most
-  # approvers would want to re-review changes when the commit message changes.
+  # In addition to patch-id we should check if whitespace content changed. Some
+  # languages are more sensitive to whitespace than others, and some changes
+  # may either introduce or be intended to fix style problems specifically
+  # involving whitespace as well.
+  if options.whitespace:
+    prev_patch_ws = GetPatchId(prev_revision, consider_whitespace=True)
+    cur_patch_ws = GetPatchId(options.commit, consider_whitespace=True)
+    if cur_patch_ws.split()[0] != prev_patch_ws.split()[0]:
+      # Insert a comment into the change letting the approvers know only the
+      # whitespace changed
+      comment_msg = "\"New patchset patch-id matches previous patchset, " \
+                     "but whitespace content has changed.\""
+      comment_cmd = ['gerrit', 'approve', '--project', options.project,
+                     '--message', comment_msg, options.commit]
+      SuExec(options, options.role_user, ' '.join(comment_cmd))
+      exit(0)
+
+  # We should also check if the commit message changed. Most approvers would
+  # want to re-review changes when the commit message changes.
   changed = DiffCommitMessages(prev_revision, options.commit)
   if changed:
     # Insert a comment into the change letting the approvers know only the

modules/openstack_project/templates/gerrit_patchset-created.erb

@@ -6,5 +6,6 @@ timeout -k 2m 10m python /usr/local/gerrit/scripts/update_bug.py patchset-create
 timeout -k 2m 10m python /usr/local/gerrit/scripts/notify_doc_impact.py patchset-created "$@"
 timeout -k 2m 10m python /usr/local/gerrit/scripts/trivial_rebase.py \
     patchset-created \
+    --whitespace \
     --private-key-path=<%= ssh_host_key %> \
     --role-user=<%= trivial_rebase_role_id %> "$@"