[U-Boot] [PATCH 0/8] Various patman fixes
Since checkpatch has changed in U-Boot, patman no longer detects errors properly. It seems like a good oppotunity to fix some of the bugs and annoying habits of patman that people have mentioned on the list.
I believe the first five patches should be considered bug fixes. The ones after that are new features.
If I have missed any bugs / problems in this sweep, please let me know.
Simon Glass (8): patman: Fix up checkpatch parsing to deal with 'CHECK' lines patman: Don't look for tags inside quotes patman: Minor help message/README fixes patman: Fix the comment in CheckTags to mention multiple tags patman: Provide option to ignore bad aliases patman: Add -a option to refrain from test-applying the patches patman: Add Cover-letter-cc tag to Cc cover letter to people patman: Add Series-process-log tag to sort/uniq change logs
tools/patman/README | 22 +++++++++++-- tools/patman/checkpatch.py | 75 +++++++++++++++++++++++++++++---------------- tools/patman/commit.py | 7 +++-- tools/patman/gitutil.py | 62 ++++++++++++++++++++++++++----------- tools/patman/patchstream.py | 10 +++++- tools/patman/patman.py | 24 ++++++++++----- tools/patman/series.py | 26 +++++++++++----- tools/patman/test.py | 9 ++++-- 8 files changed, 165 insertions(+), 70 deletions(-)
checkpatch has a new type of warning, a 'CHECK'. At present patman fails with these, which makes it less than useful.
Add support for checks, making it backwards compatible with the old checkpatch.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/checkpatch.py | 75 +++++++++++++++++++++++++++++----------------- tools/patman/test.py | 9 ++++-- 2 files changed, 54 insertions(+), 30 deletions(-)
diff --git a/tools/patman/checkpatch.py b/tools/patman/checkpatch.py index d3a0477..d1743ae 100644 --- a/tools/patman/checkpatch.py +++ b/tools/patman/checkpatch.py @@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False): 'msg': text message 'file' : filename 'line': line number + error_count: Number of errors + warning_count: Number of warnings + check_count: Number of checks lines: Number of lines + stdout: Full output of checkpatch """ result = False - error_count, warning_count, lines = 0, 0, 0 + error_count, warning_count, check_count, lines = 0, 0, 0, 0 problems = [] chk = FindCheckPatch() item = {} @@ -76,11 +80,16 @@ def CheckPatch(fname, verbose=False): #stdout, stderr = pipe.communicate()
# total: 0 errors, 0 warnings, 159 lines checked + # or: + # total: 0 errors, 2 warnings, 7 checks, 473 lines checked re_stats = re.compile('total: (\d+) errors, (\d+) warnings, (\d+)') + re_stats_full = re.compile('total: (\d+) errors, (\d+) warnings, (\d+)' + ' checks, (\d+)') re_ok = re.compile('.*has no obvious style problems') re_bad = re.compile('.*has style problems, please review') re_error = re.compile('ERROR: (.*)') re_warning = re.compile('WARNING: (.*)') + re_check = re.compile('CHECK: (.*)') re_file = re.compile('#\d+: FILE: ([^:]*):(\d+):')
for line in stdout.splitlines(): @@ -91,29 +100,39 @@ def CheckPatch(fname, verbose=False): if not line and item: problems.append(item) item = {} - match = re_stats.match(line) + match = re_stats_full.match(line) + if not match: + match = re_stats.match(line) if match: error_count = int(match.group(1)) warning_count = int(match.group(2)) - lines = int(match.group(3)) + match_count = int(match.group(3)) + if len(match.groups()) == 4: + check_count = match_count + lines = int(match.group(4)) elif re_ok.match(line): result = True elif re_bad.match(line): result = False - match = re_error.match(line) - if match: - item['msg'] = match.group(1) + err_match = re_error.match(line) + warn_match = re_warning.match(line) + file_match = re_file.match(line) + check_match = re_check.match(line) + if err_match: + item['msg'] = err_match.group(1) item['type'] = 'error' - match = re_warning.match(line) - if match: - item['msg'] = match.group(1) + elif warn_match: + item['msg'] = warn_match.group(1) item['type'] = 'warning' - match = re_file.match(line) - if match: - item['file'] = match.group(1) - item['line'] = int(match.group(2)) + elif check_match: + item['msg'] = check_match.group(1) + item['type'] = 'check' + elif file_match: + item['file'] = file_match.group(1) + item['line'] = int(file_match.group(2))
- return result, problems, error_count, warning_count, lines, stdout + return (result, problems, error_count, warning_count, check_count, + lines, stdout)
def GetWarningMsg(col, msg_type, fname, line, msg): '''Create a message for a given file/line @@ -128,37 +147,39 @@ def GetWarningMsg(col, msg_type, fname, line, msg): msg_type = col.Color(col.YELLOW, msg_type) elif msg_type == 'error': msg_type = col.Color(col.RED, msg_type) + elif msg_type == 'check': + msg_type = col.Color(col.MAGENTA, msg_type) return '%s: %s,%d: %s' % (msg_type, fname, line, msg)
def CheckPatches(verbose, args): '''Run the checkpatch.pl script on each patch''' - error_count = 0 - warning_count = 0 + error_count, warning_count, check_count = 0, 0, 0 col = terminal.Color()
for fname in args: - ok, problems, errors, warnings, lines, stdout = CheckPatch(fname, - verbose) + ok, problems, errors, warnings, checks, lines, stdout = CheckPatch( + fname, verbose) if not ok: error_count += errors warning_count += warnings - print '%d errors, %d warnings for %s:' % (errors, - warnings, fname) - if len(problems) != error_count + warning_count: + check_count += checks + print '%d errors, %d warnings, %d checks for %s:' % (errors, + warnings, checks, col.Color(col.BLUE, fname)) + if len(problems) != errors + warnings + checks: print "Internal error: some problems lost" for item in problems: - print GetWarningMsg(col, item['type'], + print GetWarningMsg(col, item.get('type', '<unknown>'), item.get('file', '<unknown>'), - item.get('line', 0), item['msg']) + item.get('line', 0), item.get('msg', 'message')) + print #print stdout - if error_count != 0 or warning_count != 0: - str = 'checkpatch.pl found %d error(s), %d warning(s)' % ( - error_count, warning_count) + if error_count or warning_count or check_count: + str = 'checkpatch.pl found %d error(s), %d warning(s), %d checks(s)' color = col.GREEN if warning_count: color = col.YELLOW if error_count: color = col.RED - print col.Color(color, str) + print col.Color(color, str % (error_count, warning_count, check_count)) return False return True diff --git a/tools/patman/test.py b/tools/patman/test.py index f801ced..5a41ef5 100644 --- a/tools/patman/test.py +++ b/tools/patman/test.py @@ -218,7 +218,8 @@ index 0000000..2234c87 def testCheckpatch(self): """Test checkpatch operation""" inf = self.SetupData('good') - result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf) + result, problems, err, warn, check, lines, stdout = ( + checkpatch.CheckPatch(inf)) self.assertEqual(result, True) self.assertEqual(problems, []) self.assertEqual(err, 0) @@ -227,7 +228,8 @@ index 0000000..2234c87 os.remove(inf)
inf = self.SetupData('no-signoff') - result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf) + result, problems, err, warn, check, lines, stdout = ( + checkpatch.CheckPatch(inf)) self.assertEqual(result, False) self.assertEqual(len(problems), 1) self.assertEqual(err, 1) @@ -236,7 +238,8 @@ index 0000000..2234c87 os.remove(inf)
inf = self.SetupData('spaces') - result, problems, err, warn, lines, stdout = checkpatch.CheckPatch(inf) + result, problems, err, warn, check, lines, stdout = ( + checkpatch.CheckPatch(inf)) self.assertEqual(result, False) self.assertEqual(len(problems), 2) self.assertEqual(err, 0)
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
checkpatch has a new type of warning, a 'CHECK'. At present patman fails with these, which makes it less than useful.
Add support for checks, making it backwards compatible with the old checkpatch.
There are also a few other minor fixups in this: * Cleanup formatting of the CheckPatches() output. * Fix erroneous "internal error" if multiple patches have warnings. * Be more robust to new types of problems.
@@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False): 'msg': text message 'file' : filename 'line': line number
error_count: Number of errorswarning_count: Number of warningscheck_count: Number of checks lines: Number of linesstdout: Full output of checkpatch
nit: Right above this it says you're returning a 4-tuple. That's no longer true. Could just remove the "4-".
optional: I've found that returning big tuples like this is problematic. Whenever you change the number of things returned you've got to modify any callers that call like:
a, b, c, d = CheckPatch(...)
to now be:
a, b, c, d, e = CheckPatch(...)
...or never use the above syntax and use:
result = CheckPatch(...) blah = result[0]
Maybe use a namedtuple so that callers can use the result more cleanly?
if match: error_count = int(match.group(1)) warning_count = int(match.group(2))
lines = int(match.group(3))
match_count = int(match.group(3))if len(match.groups()) == 4:check_count = match_countlines = int(match.group(4))
I'm confused about match_count here. What is it supposed to contain? I can't tell from the name of it. It looks like a temporary variable holding either check_count or lines. ...but you forget to assign "lines = match_count" in an "else" case so things are broken with old versions of checkpatch, right?
-Doug
Hi Doug,
On Thu, Mar 21, 2013 at 9:35 AM, Doug Anderson dianders@chromium.org wrote:
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
checkpatch has a new type of warning, a 'CHECK'. At present patman fails with these, which makes it less than useful.
Add support for checks, making it backwards compatible with the old checkpatch.
There are also a few other minor fixups in this:
- Cleanup formatting of the CheckPatches() output.
- Fix erroneous "internal error" if multiple patches have warnings.
- Be more robust to new types of problems.
Yes - I'll add them to the commit message.
@@ -64,10 +64,14 @@ def CheckPatch(fname, verbose=False): 'msg': text message 'file' : filename 'line': line number
error_count: Number of errorswarning_count: Number of warningscheck_count: Number of checks lines: Number of linesstdout: Full output of checkpatchnit: Right above this it says you're returning a 4-tuple. That's no longer true. Could just remove the "4-".
optional: I've found that returning big tuples like this is problematic. Whenever you change the number of things returned you've got to modify any callers that call like:
a, b, c, d = CheckPatch(...)
to now be:
a, b, c, d, e = CheckPatch(...)
...or never use the above syntax and use:
result = CheckPatch(...) blah = result[0]
Maybe use a namedtuple so that callers can use the result more cleanly?
Yes, we are well past the point of needing something like that, so will add it.
if match: error_count = int(match.group(1)) warning_count = int(match.group(2))
lines = int(match.group(3))
match_count = int(match.group(3))if len(match.groups()) == 4:check_count = match_countlines = int(match.group(4))I'm confused about match_count here. What is it supposed to contain? I can't tell from the name of it. It looks like a temporary variable holding either check_count or lines. ...but you forget to assign "lines = match_count" in an "else" case so things are broken with old versions of checkpatch, right?
Yes - we don't currently use the return value. But I will fix it.
Regards, Simon
-Doug
At present something like:
Revert "arm: Add cache operations"
will try to use
Revert "arm
as a tag. Clearly this is wrong, so fix it.
If the revert is intended to be tagged, then the tag can come before the revert, perhaps. Alternatively the 'Cc' tag can be used in the commit messages.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/commit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/patman/commit.py b/tools/patman/commit.py index 7144e54..64baf52 100644 --- a/tools/patman/commit.py +++ b/tools/patman/commit.py @@ -22,7 +22,7 @@ import re
# Separates a tag: at the beginning of the subject from the rest of it -re_subject_tag = re.compile('([^:]*):\s*(.*)') +re_subject_tag = re.compile('([^:"]*):\s*(.*)')
class Commit: """Holds information about a single commit/patch in the series.
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
# Separates a tag: at the beginning of the subject from the rest of it -re_subject_tag = re.compile('([^:]*):\s*(.*)') +re_subject_tag = re.compile('([^:"]*):\s*(.*)')
I'd go further and prevent all spaces. re_subject_tag = re.compile('([^:\s]*):\s*(.*)')
I ran into a similar problem with my patch I sent up earlier:
patman: Add Cover-letter-cc: tag to Cc cover letter to people
I "fixed" that by removing the extra colon, but that was sorta lame. We shouldn't have tags with spaces in them anyway...
patman: Add Cover-letter-cc tag to Cc cover letter to people
-Doug
Hi Doug,
On Thu, Mar 21, 2013 at 9:50 AM, Doug Anderson dianders@chromium.org wrote:
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
# Separates a tag: at the beginning of the subject from the rest of it -re_subject_tag = re.compile('([^:]*):\s*(.*)') +re_subject_tag = re.compile('([^:"]*):\s*(.*)')
I'd go further and prevent all spaces. re_subject_tag = re.compile('([^:\s]*):\s*(.*)')
I ran into a similar problem with my patch I sent up earlier:
patman: Add Cover-letter-cc: tag to Cc cover letter to people
I "fixed" that by removing the extra colon, but that was sorta lame. We shouldn't have tags with spaces in them anyway...
patman: Add Cover-letter-cc tag to Cc cover letter to people
Yes - will update this, thanks.
Regards, Simon
A few of the help messages are not quite right, and there is a typo in the README. Fix these.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/README | 2 +- tools/patman/patman.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README index 86d366f..c0657bc 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -68,7 +68,7 @@ will get a consistent result each time. How to configure it ===================
-For most cases of using patman for U-Boot developement patman will +For most cases of using patman for U-Boot development, patman will locate and use the file 'doc/git-mailrc' in your U-Boot directory. This contains most of the aliases you will need.
diff --git a/tools/patman/patman.py b/tools/patman/patman.py index 377408d..5000b7d 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -49,7 +49,7 @@ parser.add_option('-i', '--ignore-errors', action='store_true', dest='ignore_errors', default=False, help='Send patches email even if patch errors are found') parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run', - default=False, help="Do a try run (create but don't email patches)") + default=False, help="Do a dry run (create but don't email patches)") parser.add_option('-p', '--project', default=project.DetectProject(), help="Project name; affects default option values and " "aliases [default: %default]") @@ -72,7 +72,7 @@ parser.add_option('--no-tags', action='store_false', dest='process_tags', parser.usage = """patman [options]
Create patches from commits in a branch, check them and email them as -specified by tags you place in the commits. Use -n to """ +specified by tags you place in the commits. Use -n to do a dry run first."""
# Parse options twice: first to get the project and second to handle
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
A few of the help messages are not quite right, and there is a typo in the README. Fix these.
Signed-off-by: Simon Glass sjg@chromium.org
tools/patman/README | 2 +- tools/patman/patman.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Doug Anderson dianders@chromium.org
This comment is less than helpful. Since multiple tags are supported, add an example of how multiple tags work.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/commit.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/patman/commit.py b/tools/patman/commit.py index 64baf52..eb3c023 100644 --- a/tools/patman/commit.py +++ b/tools/patman/commit.py @@ -61,9 +61,10 @@ class Commit:
Subject tags look like this:
- propounder: Change the widget to propound correctly + propounder: fort: Change the widget to propound correctly
- Multiple tags are supported. The list is updated in self.tag + Here the tags are propounder and fort. Multiple tags are supported. + The list is updated in self.tag.
Returns: None if ok, else the name of a tag with no email alias
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
This comment is less than helpful. Since multiple tags are supported, add an example of how multiple tags work.
Signed-off-by: Simon Glass sjg@chromium.org
tools/patman/commit.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Doug Anderson dianders@chromium.org
Often it happens that patches include tags which don't have aliases. It is annoying that patman fails in this case, and provides no option to continue other than adding empty tags to the .patman file.
Correct this by adding a '-t' option to ignore tags that don't exist. Print a warning instead.
Since running the tests is not a common operation, move this to --test instead, to reserve -t for this new option.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/gitutil.py | 62 +++++++++++++++++++++++++++++++++++-------------- tools/patman/patman.py | 10 +++++--- tools/patman/series.py | 9 ++++--- 3 files changed, 57 insertions(+), 24 deletions(-)
diff --git a/tools/patman/gitutil.py b/tools/patman/gitutil.py index c35d209..5c860c4 100644 --- a/tools/patman/gitutil.py +++ b/tools/patman/gitutil.py @@ -159,7 +159,7 @@ def ApplyPatches(verbose, args, start_point): print stdout, stderr return error_count == 0
-def BuildEmailList(in_list, tag=None, alias=None): +def BuildEmailList(in_list, tag=None, alias=None, ignore_errors=False): """Build a list of email addresses based on an input list.
Takes a list of email addresses and aliases, and turns this into a list @@ -172,6 +172,8 @@ def BuildEmailList(in_list, tag=None, alias=None): Args: in_list: List of aliases/email addresses tag: Text to put before each address + alias: Alias dictionary + ignore_errors: Don't raise on alias errors
Returns: List of email addresses @@ -193,7 +195,7 @@ def BuildEmailList(in_list, tag=None, alias=None): quote = '"' if tag and tag[0] == '-' else '' raw = [] for item in in_list: - raw += LookupEmail(item, alias) + raw += LookupEmail(item, alias, raise_on_error=not ignore_errors) result = [] for item in raw: if not item in result: @@ -202,7 +204,7 @@ def BuildEmailList(in_list, tag=None, alias=None): return ['%s %s%s%s' % (tag, quote, email, quote) for email in result] return result
-def EmailPatches(series, cover_fname, args, dry_run, cc_fname, +def EmailPatches(series, cover_fname, args, dry_run, ignore_errors, cc_fname, self_only=False, alias=None, in_reply_to=None): """Email a patch series.
@@ -211,6 +213,7 @@ def EmailPatches(series, cover_fname, args, dry_run, cc_fname, cover_fname: filename of cover letter args: list of filenames of patch files dry_run: Just return the command that would be run + ignore_errors: Don't raise on alias errors cc_fname: Filename of Cc file for per-commit Cc self_only: True to just email to yourself as a test in_reply_to: If set we'll pass this to git as --in-reply-to. @@ -233,20 +236,21 @@ def EmailPatches(series, cover_fname, args, dry_run, cc_fname, >>> series = series.Series() >>> series.to = ['fred'] >>> series.cc = ['mary'] - >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, 'cc-fname', False, \ - alias) + >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \ + False, alias) 'git send-email --annotate --to "f.bloggs@napier.co.nz" --cc \ "m.poppins@cloud.net" --cc-cmd "./patman --cc-cmd cc-fname" cover p1 p2' - >>> EmailPatches(series, None, ['p1'], True, 'cc-fname', False, alias) + >>> EmailPatches(series, None, ['p1'], True, False, 'cc-fname', False, \ + alias) 'git send-email --annotate --to "f.bloggs@napier.co.nz" --cc \ "m.poppins@cloud.net" --cc-cmd "./patman --cc-cmd cc-fname" p1' >>> series.cc = ['all'] - >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, 'cc-fname', True, \ - alias) + >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \ + True, alias) 'git send-email --annotate --to "this-is-me@me.com" --cc-cmd "./patman \ --cc-cmd cc-fname" cover p1 p2' - >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, 'cc-fname', False, \ - alias) + >>> EmailPatches(series, 'cover', ['p1', 'p2'], True, False, 'cc-fname', \ + False, alias) 'git send-email --annotate --to "f.bloggs@napier.co.nz" --cc \ "f.bloggs@napier.co.nz" --cc "j.bloggs@napier.co.nz" --cc \ "m.poppins@cloud.net" --cc-cmd "./patman --cc-cmd cc-fname" cover p1 p2' @@ -254,14 +258,14 @@ def EmailPatches(series, cover_fname, args, dry_run, cc_fname, # Restore argv[0] since we clobbered it. >>> sys.argv[0] = _old_argv0 """ - to = BuildEmailList(series.get('to'), '--to', alias) + to = BuildEmailList(series.get('to'), '--to', alias, ignore_errors) if not to: print ("No recipient, please add something like this to a commit\n" "Series-to: Fred Bloggs f.blogs@napier.co.nz") return - cc = BuildEmailList(series.get('cc'), '--cc', alias) + cc = BuildEmailList(series.get('cc'), '--cc', alias, ignore_errors) if self_only: - to = BuildEmailList([os.getenv('USER')], '--to', alias) + to = BuildEmailList([os.getenv('USER')], '--to', alias, ignore_errors) cc = [] cmd = ['git', 'send-email', '--annotate'] if in_reply_to: @@ -279,13 +283,15 @@ def EmailPatches(series, cover_fname, args, dry_run, cc_fname, return str
-def LookupEmail(lookup_name, alias=None, level=0): +def LookupEmail(lookup_name, alias=None, raise_on_error=True, level=0): """If an email address is an alias, look it up and return the full name
TODO: Why not just use git's own alias feature?
Args: lookup_name: Alias or email address to look up + alias: Dictionary containing aliases (None to use settings default) + raise_on_error: True to raise an error when an alias fails to match
Returns: tuple: @@ -319,6 +325,15 @@ def LookupEmail(lookup_name, alias=None, level=0): Traceback (most recent call last): ... OSError: Recursive email alias at 'other' + >>> LookupEmail('odd', alias, False) + \033[1;31mAlias 'odd' not found\033[0m + [] + >>> # In this case the loop part will effectively be ignored. + >>> LookupEmail('loop', alias, False) + \033[1;31mRecursive email alias at 'other'\033[0m + \033[1;31mRecursive email alias at 'john'\033[0m + \033[1;31mRecursive email alias at 'mary'\033[0m + ['j.bloggs@napier.co.nz', 'm.poppins@cloud.net'] """ if not alias: alias = settings.alias @@ -327,16 +342,27 @@ def LookupEmail(lookup_name, alias=None, level=0): return [lookup_name]
lookup_name = lookup_name.lower() + col = terminal.Color()
+ out_list = [] if level > 10: - raise OSError, "Recursive email alias at '%s'" % lookup_name + msg = "Recursive email alias at '%s'" % lookup_name + if raise_on_error: + raise OSError, msg + else: + print col.Color(col.RED, msg) + return out_list
- out_list = [] if lookup_name: if not lookup_name in alias: - raise ValueError, "Alias '%s' not found" % lookup_name + msg = "Alias '%s' not found" % lookup_name + if raise_on_error: + raise ValueError, msg + else: + print col.Color(col.RED, msg) + return out_list for item in alias[lookup_name]: - todo = LookupEmail(item, alias, level + 1) + todo = LookupEmail(item, alias, raise_on_error, level + 1) for new_item in todo: if not new_item in out_list: out_list.append(new_item) diff --git a/tools/patman/patman.py b/tools/patman/patman.py index 5000b7d..5768f56 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -57,7 +57,9 @@ parser.add_option('-r', '--in-reply-to', type='string', action='store', help="Message ID that this series is in reply to") parser.add_option('-s', '--start', dest='start', type='int', default=0, help='Commit to start creating patches from (0 = HEAD)') -parser.add_option('-t', '--test', action='store_true', dest='test', +parser.add_option('-t', '--ignore-bad-tags', action='store_true', + default=False, help='Ignore bad tags / aliases') +parser.add_option('--test', action='store_true', dest='test', default=False, help='run tests') parser.add_option('-v', '--verbose', action='store_true', dest='verbose', default=False, help='Verbose output of errors and warnings') @@ -159,13 +161,15 @@ else: options.count + options.start): ok = False
- cc_file = series.MakeCcFile(options.process_tags, cover_fname) + cc_file = series.MakeCcFile(options.process_tags, cover_fname, + options.ignore_bad_tags)
# Email the patches out (giving the user time to check / cancel) cmd = '' if ok or options.ignore_errors: cmd = gitutil.EmailPatches(series, cover_fname, args, - options.dry_run, cc_file, in_reply_to=options.in_reply_to) + options.dry_run, options.ignore_bad_tags, cc_file, + in_reply_to=options.in_reply_to)
# For a dry run, just show our actions as a sanity check if options.dry_run: diff --git a/tools/patman/series.py b/tools/patman/series.py index 6c5c570..fab17c9 100644 --- a/tools/patman/series.py +++ b/tools/patman/series.py @@ -206,7 +206,7 @@ class Series(dict): str = 'Change log exists, but no version is set' print col.Color(col.RED, str)
- def MakeCcFile(self, process_tags, cover_fname): + def MakeCcFile(self, process_tags, cover_fname, ignore_bad_tags): """Make a cc file for us to use for per-commit Cc automation
Also stores in self._generated_cc to make ShowActions() faster. @@ -214,6 +214,7 @@ class Series(dict): Args: process_tags: Process tags as if they were aliases cover_fname: If non-None the name of the cover letter. + ignore_bad_tags: Ignore bad tags / aliases Return: Filename of temp file created """ @@ -224,8 +225,10 @@ class Series(dict): for commit in self.commits: list = [] if process_tags: - list += gitutil.BuildEmailList(commit.tags) - list += gitutil.BuildEmailList(commit.cc_list) + list += gitutil.BuildEmailList(commit.tags, + ignore_errors=ignore_bad_tags) + list += gitutil.BuildEmailList(commit.cc_list, + ignore_errors=ignore_bad_tags) list += get_maintainer.GetMaintainer(commit.patch) all_ccs += list print >>fd, commit.patch, ', '.join(list)
Simon,
Nothing critical and this could go in as-is, but a few nits below.
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
raw += LookupEmail(item, alias)
raw += LookupEmail(item, alias, raise_on_error=not ignore_errors)
optional: Change it so functions are consistent about whether it's "raise_on_error" or "ignore_errors"
False, alias)
Doctest that actually tests the raise_on_error? See doctests in gitutil.py for example syntax.
-def LookupEmail(lookup_name, alias=None, level=0): +def LookupEmail(lookup_name, alias=None, raise_on_error=True, level=0): """If an email address is an alias, look it up and return the full name
TODO: Why not just use git's own alias feature? Args: lookup_name: Alias or email address to look up
alias: Dictionary containing aliases (None to use settings default)raise_on_error: True to raise an error when an alias fails to match
...and what happens if you pass False?
- \033[1;31mAlias 'odd' not found\033[0m
- []
- \033[1;31mRecursive email alias at 'other'\033[0m
- \033[1;31mRecursive email alias at 'john'\033[0m
- \033[1;31mRecursive email alias at 'mary'\033[0m
- ['j.bloggs@napier.co.nz', 'm.poppins@cloud.net']
optional nit: for optional args it's nice to specify them with keywords, like
LookupEmail('loop', alias=alias, raise_on_error=False)
Please test the raise_on_error=True case.
-Doug
Hi Doug,
On Thu, Mar 21, 2013 at 10:09 AM, Doug Anderson dianders@chromium.org wrote:
Simon,
Nothing critical and this could go in as-is, but a few nits below.
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
raw += LookupEmail(item, alias)
raw += LookupEmail(item, alias, raise_on_error=not ignore_errors)optional: Change it so functions are consistent about whether it's "raise_on_error" or "ignore_errors"
Will do.
False, alias)Doctest that actually tests the raise_on_error? See doctests in gitutil.py for example syntax.
It is tested by LookupEmail() below.
-def LookupEmail(lookup_name, alias=None, level=0): +def LookupEmail(lookup_name, alias=None, raise_on_error=True, level=0): """If an email address is an alias, look it up and return the full name
TODO: Why not just use git's own alias feature? Args: lookup_name: Alias or email address to look up
alias: Dictionary containing aliases (None to use settings default)raise_on_error: True to raise an error when an alias fails to match...and what happens if you pass False?
Will add comment.
- \033[1;31mAlias 'odd' not found\033[0m
- []
- \033[1;31mRecursive email alias at 'other'\033[0m
- \033[1;31mRecursive email alias at 'john'\033[0m
- \033[1;31mRecursive email alias at 'mary'\033[0m
- ['j.bloggs@napier.co.nz', 'm.poppins@cloud.net']
optional nit: for optional args it's nice to specify them with keywords, like
LookupEmail('loop', alias=alias, raise_on_error=False)
Please test the raise_on_error=True case.
Yes, that's the current ignore_errors=False I think, so I will keep this.
Regards, Simon
Especially with the Linux kernel, it takes a long time (a minute or more) to test-apply the patches, so patman becomes significantly less useful. The only real problem that is found with this apply step is trailing spaces. Provide a -a option to skip this step, for those working with clean patches.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/patman.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/patman/patman.py b/tools/patman/patman.py index 5768f56..b92a393 100755 --- a/tools/patman/patman.py +++ b/tools/patman/patman.py @@ -41,6 +41,9 @@ import test
parser = OptionParser() +parser.add_option('-a', '--no-apply', action='store_false', + dest='apply_patches', default=True, + help="Don't test-apply patches with git am") parser.add_option('-H', '--full-help', action='store_true', dest='full_help', default=False, help='Display the README file') parser.add_option('-c', '--count', dest='count', type='int', @@ -157,9 +160,10 @@ else: ok = checkpatch.CheckPatches(options.verbose, args) else: ok = True - if not gitutil.ApplyPatches(options.verbose, args, - options.count + options.start): - ok = False + if options.apply_patches: + if not gitutil.ApplyPatches(options.verbose, args, + options.count + options.start): + ok = False
cc_file = series.MakeCcFile(options.process_tags, cover_fname, options.ignore_bad_tags)
Simon,
On Wed, Mar 20, 2013 at 7:42 PM, Simon Glass sjg@chromium.org wrote:
Especially with the Linux kernel, it takes a long time (a minute or more) to test-apply the patches, so patman becomes significantly less useful. The only real problem that is found with this apply step is trailing spaces. Provide a -a option to skip this step, for those working with clean patches.
Signed-off-by: Simon Glass sjg@chromium.org
tools/patman/patman.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
Reviewed-by: Doug Anderson dianders@chromium.org
The cover letter is sent to everyone who is on the Cc list for any of the patches in the series. Sometimes it is useful to send just the cover letter to additional people, so that they are aware of the series, but don't need to wade through all the individual patches.
Add a new Cover-letter-cc tag for this purpose.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/README | 12 +++++++++++- tools/patman/patchstream.py | 8 ++++++++ tools/patman/series.py | 11 ++++++++--- 3 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README index c0657bc..d4e7f36 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -182,6 +182,10 @@ END Sets the cover letter contents for the series. The first line will become the subject of the cover letter
+Cover-letter-cc: email / alias + Additional email addresses / aliases to send cover letter to (you + can add this multiple times) + Series-notes: blah blah blah blah @@ -263,7 +267,13 @@ will create a patch which is copied to x86, arm, sandbox, mikef, ag and afleming.
If you have a cover letter it will get sent to the union of the CC lists of -all of the other patches. +all of the other patches. If you want to sent it to additional people you +can add a tag: + +Cover-letter-cc: <list of addresses> + +These people will get the cover letter even if they are not on the To/Cc +list for any of the patches.
Example Work Flow diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 76f815b..a4f2f31 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -42,6 +42,9 @@ re_signoff = re.compile('^Signed-off-by:') # The start of the cover letter re_cover = re.compile('^Cover-letter:')
+# A cover letter Cc +re_cover_cc = re.compile('^Cover-letter-cc: *(.*)') + # Patch series tag re_series = re.compile('^Series-(\w*): *(.*)')
@@ -153,6 +156,7 @@ class PatchStream: # Handle state transition and skipping blank lines series_match = re_series.match(line) commit_match = re_commit.match(line) if self.is_log else None + cover_cc_match = re_cover_cc.match(line) tag_match = None if self.state == STATE_PATCH_HEADER: tag_match = re_tag.match(line) @@ -205,6 +209,10 @@ class PatchStream: self.in_section = 'cover' self.skip_blank = False
+ elif cover_cc_match: + value = cover_cc_match.group(1) + self.AddToSeries(line, 'cover-cc', value) + # If we are in a change list, key collected lines until a blank one elif self.in_change: if is_blank: diff --git a/tools/patman/series.py b/tools/patman/series.py index fab17c9..fe919be 100644 --- a/tools/patman/series.py +++ b/tools/patman/series.py @@ -27,7 +27,8 @@ import gitutil import terminal
# Series-xxx tags that we understand -valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name']; +valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name', + 'cover-cc']
class Series(dict): """Holds information about a patch series, including all tags. @@ -43,6 +44,7 @@ class Series(dict): def __init__(self): self.cc = [] self.to = [] + self.cover_cc = [] self.commits = [] self.cover = None self.notes = [] @@ -69,6 +71,7 @@ class Series(dict): value: Tag value (part after 'Series-xxx: ') """ # If we already have it, then add to our list + name = name.replace('-', '_') if name in self: values = value.split(',') values = [str.strip() for str in values] @@ -140,7 +143,8 @@ class Series(dict): print 'Prefix:\t ', self.get('prefix') if self.cover: print 'Cover: %d lines' % len(self.cover) - all_ccs = itertools.chain(*self._generated_cc.values()) + cover_cc = gitutil.BuildEmailList(self.get('cover_cc', '')) + all_ccs = itertools.chain(cover_cc, *self._generated_cc.values()) for email in set(all_ccs): print ' Cc: ',email if cmd: @@ -235,7 +239,8 @@ class Series(dict): self._generated_cc[commit.patch] = list
if cover_fname: - print >>fd, cover_fname, ', '.join(set(all_ccs)) + cover_cc = gitutil.BuildEmailList(self.get('cover_cc', '')) + print >>fd, cover_fname, ', '.join(set(cover_cc + all_ccs))
fd.close() return fname
Simon,
On Wed, Mar 20, 2013 at 7:43 PM, Simon Glass sjg@chromium.org wrote:
The cover letter is sent to everyone who is on the Cc list for any of the patches in the series. Sometimes it is useful to send just the cover letter to additional people, so that they are aware of the series, but don't need to wade through all the individual patches.
Add a new Cover-letter-cc tag for this purpose.
Signed-off-by: Simon Glass sjg@chromium.org
tools/patman/README | 12 +++++++++++- tools/patman/patchstream.py | 8 ++++++++ tools/patman/series.py | 11 ++++++++--- 3 files changed, 27 insertions(+), 4 deletions(-)
Identical to the already posted http://patchwork.ozlabs.org/patch/222755/ so I'll repeat my existing review. ;)
Reviewed-by: Doug Anderson dianders@chromium.org
For some series with lots of changes it is annoying that duplicate change log items are not caught. It is also helpful sometimes to sort the change logs.
Add a Series-process-log tag to enable this, which can be placed in a commit to control this.
The change to the Cc: line is to fix a checkpatch warning.
Signed-off-by: Simon Glass sjg@chromium.org --- tools/patman/README | 8 +++++++- tools/patman/patchstream.py | 2 +- tools/patman/series.py | 8 ++++++-- 3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README index d4e7f36..566f54d 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -225,9 +225,15 @@ Series-changes: n to update the log there and then, knowing that the script will do the rest.
-Cc: Their Name <email> + Cc: Their Name <email> This copies a single patch to another email address.
+Series-process-log: sort, uniq + This tells patman to sort and/or uniq the change logs. It is + assumed that each change log entry is only a single line long. + Use 'sort' to sort the entries, and 'uniq' to include only + unique entries. If omitted, no change log processing is done. + Various other tags are silently removed, like these Chrome OS and Gerrit tags:
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index a4f2f31..9d8a918 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -46,7 +46,7 @@ re_cover = re.compile('^Cover-letter:') re_cover_cc = re.compile('^Cover-letter-cc: *(.*)')
# Patch series tag -re_series = re.compile('^Series-(\w*): *(.*)') +re_series = re.compile('^Series-([a-z-]*): *(.*)')
# Commit tags that we want to collect and keep re_tag = re.compile('^(Tested-by|Acked-by|Reviewed-by|Cc): (.*)') diff --git a/tools/patman/series.py b/tools/patman/series.py index fe919be..7080a55 100644 --- a/tools/patman/series.py +++ b/tools/patman/series.py @@ -28,7 +28,7 @@ import terminal
# Series-xxx tags that we understand valid_series = ['to', 'cc', 'version', 'changes', 'prefix', 'notes', 'name', - 'cover-cc'] + 'cover-cc', 'process_log']
class Series(dict): """Holds information about a patch series, including all tags. @@ -167,15 +167,19 @@ class Series(dict): etc. """ final = [] + process_it = self.get('process_log', '') need_blank = False for change in sorted(self.changes, reverse=True): out = [] for this_commit, text in self.changes[change]: if commit and this_commit != commit: continue - out.append(text) + if not ('uniq' in process_it and text in out): + out.append(text) line = 'Changes in v%d:' % change have_changes = len(out) > 0 + if 'sort' in process_it: + out = sorted(out) if have_changes: out.insert(0, line) else:
Simon,
On Wed, Mar 20, 2013 at 7:43 PM, Simon Glass sjg@chromium.org wrote:
For some series with lots of changes it is annoying that duplicate change log items are not caught. It is also helpful sometimes to sort the change logs.
Add a Series-process-log tag to enable this, which can be placed in a commit to control this.
The change to the Cc: line is to fix a checkpatch warning.
Signed-off-by: Simon Glass sjg@chromium.org
tools/patman/README | 8 +++++++- tools/patman/patchstream.py | 2 +- tools/patman/series.py | 8 ++++++-- 3 files changed, 14 insertions(+), 4 deletions(-)
Not sure I'd find this terribly useful myself, but I don't see anything wrong with it. I think my change log items tend to be more than one line long for one...
if not ('uniq' in process_it and text in out):
optional: My brain had a hard time processing this. I did the logic transformation myself:
if 'uniq' not in process_it or text not in out:
Also: Do you really want the "process_it" to be so free-form? That seems like it might be asking for disaster. Why not specify that it's comma-separated and be done.
-Doug
Hi Doug,
On Thu, Mar 21, 2013 at 10:51 AM, Doug Anderson dianders@chromium.org wrote:
Simon,
On Wed, Mar 20, 2013 at 7:43 PM, Simon Glass sjg@chromium.org wrote:
For some series with lots of changes it is annoying that duplicate change log items are not caught. It is also helpful sometimes to sort the change logs.
Add a Series-process-log tag to enable this, which can be placed in a commit to control this.
The change to the Cc: line is to fix a checkpatch warning.
Signed-off-by: Simon Glass sjg@chromium.org
tools/patman/README | 8 +++++++- tools/patman/patchstream.py | 2 +- tools/patman/series.py | 8 ++++++-- 3 files changed, 14 insertions(+), 4 deletions(-)
Not sure I'd find this terribly useful myself, but I don't see anything wrong with it. I think my change log items tend to be more than one line long for one...
I use it a lot. Brevity is a virtue :-)
if not ('uniq' in process_it and text in out):optional: My brain had a hard time processing this. I did the logic transformation myself:
if 'uniq' not in process_it or text not in out:
Will update.
Also: Do you really want the "process_it" to be so free-form? That seems like it might be asking for disaster. Why not specify that it's comma-separated and be done.
OK I'll add processing to check that.
Regards, Simon
participants (2)
-
Doug Anderson -
Simon Glass