On 2013-09-23 12:18:01 -0400 (-0400), Monty Taylor wrote: > I'm copying our infra list, because I don't remember the details. > > BUT - short version is, the one in the gerrit repo doesn't work. :) > > Fungi, Clark, do you remember what didn't work about it?
It was merely an example script in the contrib directory, not under active development based on what I saw in their Git history. I've attached my original patches for reference, with detailed commit messages (though there have been small incremental changes over time in the openstack-infra/jeepyb project, which should be detailed sufficiently in that project's commit history). -- Jeremy Stanley
From 0f279d21d950e00917e8d7b91de1df31055f9c4a Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 14:31:35 +0000 Subject: [PATCH 01/13] Swallow Unused Gerrit Hook Options Ensures all hook options can be passed directly. * contrib/trivial_rebase.py(OptionParser): It would be nice to be able to pass Gerrit hook options directly to the script, but optparse chokes on unexpected change-url, branch and uploader. This patch allows it to swallow any unknown option quietly and move on. --- contrib/trivial_rebase.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index a514b4c..353ede9 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -35,10 +35,20 @@ # Documentation is available here: https://www.codeaurora.org/xwiki/bin/QAEP/Gerrit import json -from optparse import OptionParser import subprocess from sys import exit +from optparse import OptionParser as _realOptionParser, AmbiguousOptionError, \ + BadOptionError +class OptionParser(_realOptionParser): + """Make OptionParser silently swallow unrecognized options.""" + def _process_args(self, largs, rargs, values): + while rargs: + try: + _realOptionParser._process_args(self, largs, rargs, values) + except (AmbiguousOptionError, BadOptionError), e: + largs.append(e.opt_str) + class CheckCallError(OSError): """CheckCall() returned non-0.""" def __init__(self, command, cwd, retcode, stdout, stderr=None): -- 1.7.10.4
From 0007a10cbf94c09bcc41ded6c02ead50ff476c02 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 14:43:26 +0000 Subject: [PATCH 02/13] Ignore APRV Approval Category Stop OpenStack's APRV category fouling the script. * contrib/trivial_rebase.py(Main): This patch treats an APRV category, if found, like VRFY... it simply gets ignored rather than stopping execution. --- contrib/trivial_rebase.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index 353ede9..fcc57aa 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -214,6 +214,9 @@ def Main(): elif approval["category_id"] == "SUBM": # We don't care about previous submit attempts continue + elif approval["category_id"] == "APRV": + # Similarly squash old approvals + continue else: print "Unsupported category: %s" % approval exit(0) -- 1.7.10.4
From dd3622e4e826368252a900504d0e6e1f4d5048b8 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 16:56:23 +0000 Subject: [PATCH 03/13] Add Server Option The --server option can now be specified. * contrib/trivial_rebase.py(Main): Changed server from a hard-coded string variable to one which can be specified with a --server option on the command line, but still defaulting to the same "localhost" value used previously. This is in preparation for eliminating many of the common variables passed to function calls. --- contrib/trivial_rebase.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index fcc57aa..beb7aae 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -148,7 +148,6 @@ def DiffCommitMessages(commit1, commit2): return False def Main(): - server = 'localhost' usage = "usage: %prog <required options> [--server-port=PORT]" parser = OptionParser(usage=usage) parser.add_option("--change", dest="changeId", help="Change identifier") @@ -160,6 +159,9 @@ def Main(): parser.add_option("--server-port", dest="port", default='29418', help="Port to connect to Gerrit's SSH daemon " "[default: %default]") + parser.add_option("--server", dest="server", default="localhost", + help="Server name/address for Gerrit's SSH daemon " + "[default: %default]") (options, args) = parser.parse_args() @@ -171,8 +173,8 @@ def Main(): # Nothing to detect on first patchset exit(0) prev_revision = None - prev_revision = FindPrevRev(options.changeId, options.patchset, server, - options.port) + prev_revision = FindPrevRev(options.changeId, options.patchset, + options.server, options.port) if not prev_revision: # Couldn't find a previous revision exit(0) @@ -190,14 +192,15 @@ def Main(): # commit message changed comment_msg = ("\'--message=New patchset patch-id matches previous patchset" ", but commit message has changed.'") - comment_cmd = ['ssh', '-p', options.port, server, 'gerrit', 'approve', - '--project', options.project, comment_msg, options.commit] + comment_cmd = ['ssh', '-p', options.port, options.server, 'gerrit', + 'approve', '--project', options.project, comment_msg, + options.commit] CheckCall(comment_cmd) exit(0) # Need to get all approvals on prior patch set, then suexec them onto # this patchset. - approvals = GetApprovals(options.changeId, options.patchset, server, + approvals = GetApprovals(options.changeId, options.patchset, options.server, options.port) gerrit_approve_msg = ("\'Automatically re-added by Gerrit trivial rebase " "detection script.\'") @@ -225,9 +228,9 @@ def Main(): gerrit_approve_cmd = ['gerrit', 'approve', '--project', options.project, '--message', gerrit_approve_msg, approve_category, score, options.commit] - email_addr = GetEmailFromAcctId(approval["account_id"], server, + email_addr = GetEmailFromAcctId(approval["account_id"], options.server, options.port) - SuExec(server, options.port, options.private_key_path, email_addr, + SuExec(options.server, options.port, options.private_key_path, email_addr, ' '.join(gerrit_approve_cmd)) exit(0) -- 1.7.10.4
From 300d651c0640e39353451dcd8cf1079cecafc416 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 17:14:57 +0000 Subject: [PATCH 04/13] Stop Passing Options In Variables CLI options are now passed around together. * contrib/trivial_rebase.py(GsqlQuery, FindPrevRev, GetApprovals) (GetEmailFromAcctId, SuExec, Main): Instead of passing options around in individual variables, just pass the options object to any function which needs to know one or more of the command-line options. --- contrib/trivial_rebase.py | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index beb7aae..4e2212a 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -73,10 +73,10 @@ def CheckCall(command, cwd=None): raise CheckCallError(command, cwd, process.returncode, std_out, std_err) return std_out, std_err -def GsqlQuery(sql_query, server, port): +def GsqlQuery(sql_query, options): """Runs a gerrit gsql query and returns the result""" - gsql_cmd = ['ssh', '-p', port, server, 'gerrit', 'gsql', '--format', - 'JSON', '-c', sql_query] + gsql_cmd = ['ssh', '-p', options.port, options.server, 'gerrit', 'gsql', + '--format', 'JSON', '-c', sql_query] try: (gsql_out, gsql_stderr) = CheckCall(gsql_cmd) except CheckCallError, e: @@ -87,26 +87,27 @@ def GsqlQuery(sql_query, server, port): new_out = gsql_out.replace('}}\n', '}}\nsplit here\n') return new_out.split('split here\n') -def FindPrevRev(changeId, patchset, server, port): +def FindPrevRev(options): """Finds the revision of the previous patch set on the change""" sql_query = ("\"SELECT revision FROM patch_sets,changes WHERE " "patch_sets.change_id = changes.change_id AND " "patch_sets.patch_set_id = %s AND " - "changes.change_key = \'%s\'\"" % ((patchset - 1), changeId)) - revisions = GsqlQuery(sql_query, server, port) + "changes.change_key = \'%s\'\"" % ((options.patchset - 1), + options.changeId)) + revisions = GsqlQuery(sql_query, options) json_dict = json.loads(revisions[0], strict=False) return json_dict["columns"]["revision"] -def GetApprovals(changeId, patchset, server, port): +def GetApprovals(options): """Get all the approvals on a specific patch set Returns a list of approval dicts""" sql_query = ("\"SELECT value,account_id,category_id FROM patch_set_approvals " "WHERE patch_set_id = %s AND change_id = (SELECT change_id FROM " "changes WHERE change_key = \'%s\') AND value <> 0\"" - % ((patchset - 1), changeId)) - gsql_out = GsqlQuery(sql_query, server, port) + % ((options.patchset - 1), options.changeId)) + gsql_out = GsqlQuery(sql_query, options) approvals = [] for json_str in gsql_out: dict = json.loads(json_str, strict=False) @@ -114,11 +115,11 @@ def GetApprovals(changeId, patchset, server, port): approvals.append(dict["columns"]) return approvals -def GetEmailFromAcctId(account_id, server, port): +def GetEmailFromAcctId(account_id, options): """Returns the preferred email address associated with the account_id""" sql_query = ("\"SELECT preferred_email FROM accounts WHERE account_id = %s\"" % account_id) - email_addr = GsqlQuery(sql_query, server, port) + email_addr = GsqlQuery(sql_query, options) json_dict = json.loads(email_addr[0], strict=False) return json_dict["columns"]["preferred_email"] @@ -131,9 +132,10 @@ def GetPatchId(revision): git_show_process = subprocess.Popen(git_show_cmd, stdout=subprocess.PIPE) return patch_id_process.communicate(git_show_process.communicate()[0])[0] -def SuExec(server, port, private_key, as_user, cmd): - suexec_cmd = ['ssh', '-l', "Gerrit Code Review", '-p', port, server, '-i', - private_key, 'suexec', '--as', as_user, '--', cmd] +def SuExec(options, as_user, cmd): + suexec_cmd = ['ssh', '-l', "Gerrit Code Review", '-p', options.port, + options.server, '-i', options.private_key_path, 'suexec', + '--as', as_user, '--', cmd] CheckCall(suexec_cmd) def DiffCommitMessages(commit1, commit2): @@ -173,8 +175,7 @@ def Main(): # Nothing to detect on first patchset exit(0) prev_revision = None - prev_revision = FindPrevRev(options.changeId, options.patchset, - options.server, options.port) + prev_revision = FindPrevRev(options) if not prev_revision: # Couldn't find a previous revision exit(0) @@ -200,8 +201,7 @@ def Main(): # Need to get all approvals on prior patch set, then suexec them onto # this patchset. - approvals = GetApprovals(options.changeId, options.patchset, options.server, - options.port) + approvals = GetApprovals(options) gerrit_approve_msg = ("\'Automatically re-added by Gerrit trivial rebase " "detection script.\'") for approval in approvals: @@ -228,10 +228,8 @@ def Main(): gerrit_approve_cmd = ['gerrit', 'approve', '--project', options.project, '--message', gerrit_approve_msg, approve_category, score, options.commit] - email_addr = GetEmailFromAcctId(approval["account_id"], options.server, - options.port) - SuExec(options.server, options.port, options.private_key_path, email_addr, - ' '.join(gerrit_approve_cmd)) + email_addr = GetEmailFromAcctId(approval["account_id"], options) + SuExec(options, email_addr, ' '.join(gerrit_approve_cmd)) exit(0) if __name__ == "__main__": -- 1.7.10.4
From 368ad9cd74437b8372954a60c2bd027e6069c58c Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 20:44:37 +0000 Subject: [PATCH 05/13] Generalize Usage Message There are multiple CLI options with defaults. * contrib/trivial_rebase.py(Main): With the prior addition of --server and other upcoming CLI options with defaults, it makes no sense to list the --server-port option as optional on the usage line. It's already clear from the remainder of the help output which options have defaults and what they are. --- contrib/trivial_rebase.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index 4e2212a..65522e3 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -150,7 +150,7 @@ def DiffCommitMessages(commit1, commit2): return False def Main(): - usage = "usage: %prog <required options> [--server-port=PORT]" + usage = "usage: %prog <required options> [optional options]" parser = OptionParser(usage=usage) parser.add_option("--change", dest="changeId", help="Change identifier") parser.add_option("--project", help="Project path in Gerrit") -- 1.7.10.4
From 6c70d7ba20134e631ee286cafc274eb4c7e8a370 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 21:12:01 +0000 Subject: [PATCH 06/13] Add Role User Option An option to specify the --role-user. * contrib/trivial_rebase.py(Main): This is a stub adding a new unused --role-user option which will eventually identify the Gerrit role account to which "altered commit message" detection comments are attributed, rather than relying on separate SSH authentication. --- contrib/trivial_rebase.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index 65522e3..e4e0b0e 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -156,6 +156,8 @@ def Main(): parser.add_option("--project", help="Project path in Gerrit") parser.add_option("--commit", help="Git commit-ish for this patchset") parser.add_option("--patchset", type="int", help="The patchset number") + parser.add_option("--role-user", dest="role_user", + help="E-mail/ID of user commenting on commit messages") parser.add_option("--private-key-path", dest="private_key_path", help="Full path to Gerrit SSH daemon's private host key") parser.add_option("--server-port", dest="port", default='29418', -- 1.7.10.4
From a1a133790931b80f136d80138f41c2d6e8429efe Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 21:35:12 +0000 Subject: [PATCH 07/13] SuExec Role User Comments The role user's comments now come in via suexec. * contrib/trivial_rebase.py(Main): The "altered commit message" detection comments posted by the role user, identified by the E-mail address passed via --role-user on the command line, are now forged by the magic "Gerrit Code Review" SSH user with suexec. --- contrib/trivial_rebase.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index e4e0b0e..c034d0a 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -193,12 +193,11 @@ def Main(): if changed: # Insert a comment into the change letting the approvers know only the # commit message changed - comment_msg = ("\'--message=New patchset patch-id matches previous patchset" - ", but commit message has changed.'") - comment_cmd = ['ssh', '-p', options.port, options.server, 'gerrit', - 'approve', '--project', options.project, comment_msg, - options.commit] - CheckCall(comment_cmd) + comment_msg = "\"New patchset patch-id matches previous patchset, " \ + "but commit message has changed.\"" + comment_cmd = ['gerrit', 'approve', '--project', options.project, + '--message', comment_msg, options.commit] + SuExec(options, options.role_user, ' '.join(comment_cmd)) exit(0) # Need to get all approvals on prior patch set, then suexec them onto -- 1.7.10.4
From 7e397b2ef4cd192699a00bc673e46b441aa4d675 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 21:48:56 +0000 Subject: [PATCH 08/13] Use Gerrit Code Review Account Now gsql calls don't use the role account. * contrib/trivial_rebase.py(GsqlQuery): Hopefully the last of the role account related changes, this switches to using the magic Gerrit Code Review account for all gsql queries. The role account no longer requires membership in the Administrators group in Gerrit as a result. --- contrib/trivial_rebase.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index c034d0a..8fec54c 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -75,8 +75,9 @@ def CheckCall(command, cwd=None): def GsqlQuery(sql_query, options): """Runs a gerrit gsql query and returns the result""" - gsql_cmd = ['ssh', '-p', options.port, options.server, 'gerrit', 'gsql', - '--format', 'JSON', '-c', sql_query] + gsql_cmd = ['ssh', '-l', "Gerrit Code Review", '-i', + options.private_key_path, '-p', options.port, options.server, + 'gerrit', 'gsql', '--format', 'JSON', '-c', sql_query] try: (gsql_out, gsql_stderr) = CheckCall(gsql_cmd) except CheckCallError, e: -- 1.7.10.4
From cec25de9ebd34b3f63c386d323c934ee31353119 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 21:57:00 +0000 Subject: [PATCH 09/13] New Gssh Function A new Gssh() function for consistent SSH calls. * contrib/trivial_rebase.py(Gssh): This new function is intended to be used whenever connecting to the Gerrit API via SSH. Future commits will employ it to replace the individual SSH CLI invocations found throughout the module. --- contrib/trivial_rebase.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index 8fec54c..9622b02 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -73,6 +73,22 @@ def CheckCall(command, cwd=None): raise CheckCallError(command, cwd, process.returncode, std_out, std_err) return std_out, std_err +def Gssh(options, api_command): + """Makes a Gerrit API call via SSH and returns the stdout results.""" + ssh_cmd = ['ssh', + '-l', 'Gerrit Code Review', + '-p', options.port, + '-i', options.private_key_path, + options.server, + api_command] + try: + return CheckCall(ssh_cmd)[0] + except CheckCallError, e: + import sys + err_template = "call: %s\nreturn code: %s\nstdout: %s\nstderr: %s\n" + sys.stderr.write(err_template%(ssh_cmd, e.retcode, e.stdout, e.stderr)) + raise + def GsqlQuery(sql_query, options): """Runs a gerrit gsql query and returns the result""" gsql_cmd = ['ssh', '-l', "Gerrit Code Review", '-i', -- 1.7.10.4
From 52fdc81fcd0a48588a059d76c5da46e844ce666a Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 22:05:23 +0000 Subject: [PATCH 10/13] Use Gssh in GsqlQuery The GsqlQuery() function now uses Gssh(). * contrib/trivial_rebase.py(GsqlQuery): Gets rid of the embedded SSH call and just passes the API command line through to Gssh. --- contrib/trivial_rebase.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index 9622b02..f4dc9ed 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -91,16 +91,8 @@ def Gssh(options, api_command): def GsqlQuery(sql_query, options): """Runs a gerrit gsql query and returns the result""" - gsql_cmd = ['ssh', '-l', "Gerrit Code Review", '-i', - options.private_key_path, '-p', options.port, options.server, - 'gerrit', 'gsql', '--format', 'JSON', '-c', sql_query] - try: - (gsql_out, gsql_stderr) = CheckCall(gsql_cmd) - except CheckCallError, e: - print "return code is %s" % e.retcode - print "stdout and stderr is\n%s%s" % (e.stdout, e.stderr) - raise - + gsql_cmd = "gerrit gsql --format JSON -c %s"%sql_query + gsql_out = Gssh(options, gsql_cmd) new_out = gsql_out.replace('}}\n', '}}\nsplit here\n') return new_out.split('split here\n') -- 1.7.10.4
From e691c3c5ebf7e3492fdb9a68ea0a88994c6e9a5e Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Fri, 31 Aug 2012 22:19:01 +0000 Subject: [PATCH 11/13] Use Gssh in SuExec The SuExec() function now uses Gssh(). * contrib/trivial_rebase.py(SuExec): Gets rid of the embedded SSH call and just passes the API command line through to Gssh. --- contrib/trivial_rebase.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index f4dc9ed..df06267 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -142,10 +142,8 @@ def GetPatchId(revision): return patch_id_process.communicate(git_show_process.communicate()[0])[0] def SuExec(options, as_user, cmd): - suexec_cmd = ['ssh', '-l', "Gerrit Code Review", '-p', options.port, - options.server, '-i', options.private_key_path, 'suexec', - '--as', as_user, '--', cmd] - CheckCall(suexec_cmd) + suexec_cmd = "suexec --as %s -- %s"%(as_user, cmd) + Gssh(options, suexec_cmd) def DiffCommitMessages(commit1, commit2): log_cmd1 = ['git', 'log', '--pretty=format:"%an %ae%n%s%n%b"', -- 1.7.10.4
From cc5f67d2b4e00e38545cf3b2359b31ee0535d6ce Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Tue, 4 Sep 2012 14:01:24 +0000 Subject: [PATCH 12/13] Removed Unnecessary E-Mail Lookups Turns out suexec will take an account_id directly. * contrib/trivial_rebase.py(GetEmailFromAcctId, Main): Adjusted the suexec calls to use a straight account_id number, which obviated the need for the GetEmailFromAcctId function entirely. This in fact fixes a bug whereby the script would fail attempting to re-add approvals previously made by accounts with no listed E-mail address, but has the added benefit of reducing the number of gsql queries made in the process. --- contrib/trivial_rebase.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index df06267..4aca289 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -124,15 +124,6 @@ def GetApprovals(options): approvals.append(dict["columns"]) return approvals -def GetEmailFromAcctId(account_id, options): - """Returns the preferred email address associated with the account_id""" - sql_query = ("\"SELECT preferred_email FROM accounts WHERE account_id = %s\"" - % account_id) - email_addr = GsqlQuery(sql_query, options) - - json_dict = json.loads(email_addr[0], strict=False) - return json_dict["columns"]["preferred_email"] - def GetPatchId(revision): git_show_cmd = ['git', 'show', revision] patch_id_cmd = ['git', 'patch-id'] @@ -236,8 +227,7 @@ def Main(): gerrit_approve_cmd = ['gerrit', 'approve', '--project', options.project, '--message', gerrit_approve_msg, approve_category, score, options.commit] - email_addr = GetEmailFromAcctId(approval["account_id"], options) - SuExec(options, email_addr, ' '.join(gerrit_approve_cmd)) + SuExec(options, approval["account_id"], ' '.join(gerrit_approve_cmd)) exit(0) if __name__ == "__main__": -- 1.7.10.4
From 2559b3b3bd909f292cc253c0cb4c1e006eaf8b75 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley <[email protected]> Date: Thu, 27 Sep 2012 18:15:49 +0000 Subject: [PATCH 13/13] 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. * contrib/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. --- contrib/trivial_rebase.py | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/contrib/trivial_rebase.py b/contrib/trivial_rebase.py index 4aca289..7575935 100755 --- a/contrib/trivial_rebase.py +++ b/contrib/trivial_rebase.py @@ -124,13 +124,25 @@ 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) - return patch_id_process.communicate(git_show_process.communicate()[0])[0] + 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): suexec_cmd = "suexec --as %s -- %s"%(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 -- 1.7.10.4
signature.asc
Description: Digital signature
_______________________________________________ OpenStack-Infra mailing list [email protected] http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-infra
