[cip-dev] fixes and support for tags

[cip-dev] fixes and support for tags

[Daniel Sangorrin]

Hello Ben,

This series contains a few fixes and support for reporting on tags.

- Re-sent patches

[cip-kernel-sec 1/6] check_git_repo: add checks to the local

I decided to remove the repository creation code from the shared
function, because it is only used by prepared_remotes.
I have adjusted the exception raised, and I use .exists() instead
of .isdir() to account for git worktrees (which I didn't know 
about)

[cip-kernel-sec 2/6] prepare_remotes: helper script to prepare local

I have put the creation code here, and added remote update code
for the script to be useful.

[cip-kernel-sec 3/6] report_affected: fix code when branches are

This allows reporting on CIP branches. I have removed the
get_stable_branch function as you mentioned.

[cip-kernel-sec 4/6] report_affected: check user supplied branch

Small check to make sure the user supplied branch names actually
match the configured branches' names.

[cip-kernel-sec 5/6] report_affected: add support for reporting on

This is the new big patch, where I add support for reporting on
tags. Tags can be specified in different formats. For example:
- linux-4.4.y/v4.4.107
- v4.4.181-cip33 <-- linux-4.4.y is inferred from the tag's name
- linux-4.4.y/myproduct-v20190612 <-- product tags

[cip-kernel-sec 6/6] pep8: fix pep8-related errors such as too long

I fixed these ones because they were annoying.

Thanks,
Daniel

[cip-dev] [cip-kernel-sec 1/6] check_git_repo: add checks to the local repository

[Daniel Sangorrin]

Add checks to make sure that the local repository exists
and has the configured remotes in place.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 scripts/import_stable.py     |  1 +
 scripts/kernel_sec/branch.py | 21 +++++++++++++++++++++
 scripts/report_affected.py   |  1 +
 scripts/webview.py           |  1 +
 4 files changed, 24 insertions(+)

diff --git a/scripts/import_stable.py b/scripts/import_stable.py
index 2c88b07..26fcc2f 100755
--- a/scripts/import_stable.py
+++ b/scripts/import_stable.py
@@ -190,4 +190,5 @@ if __name__ == '__main__':
     remotes = kernel_sec.branch.get_remotes(args.remote_name,
                                             mainline=args.mainline_remote_name,
                                             stable=args.stable_remote_name)
+    kernel_sec.branch.check_git_repo(args.git_repo, remotes)
     main(args.git_repo, remotes, args.debug)
diff --git a/scripts/kernel_sec/branch.py b/scripts/kernel_sec/branch.py
index 10f3339..a138b96 100644
--- a/scripts/kernel_sec/branch.py
+++ b/scripts/kernel_sec/branch.py
@@ -4,10 +4,12 @@
 # Public License, Version 3 or later. See http://www.gnu.org/copyleft/gpl.html
 # for details.
 
+import argparse
 import io
 import os
 import re
 import subprocess
+import sys
 import time
 import urllib.error
 import urllib.request
@@ -219,3 +221,22 @@ def get_remotes(mappings, mainline=None, stable=None):
     if stable:
         remotes['stable']['git_name'] = stable
     return remotes
+
+
+def check_git_repo(git_repo, remotes):
+    if not os.path.isdir(git_repo):
+        msg = "directory %r not present" % git_repo
+        raise argparse.ArgumentError(None, msg)
+    # .git could be a regular file (worktrees) or a directory
+    if not os.path.exists(os.path.join(git_repo, '.git')):
+        msg = "directory %r is not a git repository" % git_repo
+        raise argparse.ArgumentError(None, msg)
+
+    current_remotes = subprocess.check_output(
+        ['git', 'remote', 'show'], cwd=git_repo).decode(
+            sys.stdout.encoding).strip().split('\n')
+    for key in remotes.keys():
+        remote = remotes[key]  # __getitem__ will add git_name
+        if remote['git_name'] not in current_remotes:
+            msg = "remote %r not in git repository" % remote['git_name']
+            raise argparse.ArgumentError(None, msg)
diff --git a/scripts/report_affected.py b/scripts/report_affected.py
index 879c021..d2f1f22 100755
--- a/scripts/report_affected.py
+++ b/scripts/report_affected.py
@@ -100,5 +100,6 @@ if __name__ == '__main__':
     remotes = kernel_sec.branch.get_remotes(args.remote_name,
                                             mainline=args.mainline_remote_name,
                                             stable=args.stable_remote_name)
+    kernel_sec.branch.check_git_repo(args.git_repo, remotes)
     main(args.git_repo, remotes,
          args.only_fixed_upstream, args.include_ignored, *args.branches)
diff --git a/scripts/webview.py b/scripts/webview.py
index a3a643b..52a7b15 100755
--- a/scripts/webview.py
+++ b/scripts/webview.py
@@ -219,6 +219,7 @@ if __name__ == '__main__':
     remotes = kernel_sec.branch.get_remotes(args.remote_name,
                                             mainline=args.mainline_remote_name,
                                             stable=args.stable_remote_name)
+    kernel_sec.branch.check_git_repo(args.git_repo, remotes)
 
     conf = {
         '/static/style.css': {
-- 
2.17.1

[cip-dev] [cip-kernel-sec 2/6] prepare_remotes: helper script to prepare local repo

[Daniel Sangorrin]

Helper script that prepares the local git repository
with the configured remote branches. Expert developers
and kernel maintainers will probably have their own
worktrees but for new users or a quickstart, this
script should be helpful.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 README.md                    |  5 +++
 conf/remotes.yml             |  3 ++
 scripts/import_stable.py     |  8 ++---
 scripts/kernel_sec/branch.py | 10 ++++++
 scripts/prepare_remotes.py   | 62 ++++++++++++++++++++++++++++++++++++
 5 files changed, 82 insertions(+), 6 deletions(-)
 create mode 100755 scripts/prepare_remotes.py

diff --git a/README.md b/README.md
index 4c5808f..576cc75 100644
--- a/README.md
+++ b/README.md
@@ -23,6 +23,10 @@ this is assumed to be in `../kernel`, with remotes configured in
 stable and cip repositories. These can be overridden by command-line options
 or configuration (`~/.config/kernel-sec/remotes.yml`).
 
+* `scripts/prepare_remotes.py` - creates the local git repository
+and adds all configured remotes. You may prefer to skip this script
+and configure the repository by hand.
+
 * `scripts/import_debian.py` - import information from Debian's
 `kernel_sec` project.  It includes all issues that Debian considers
 active or that are already tracked here.
@@ -81,6 +85,7 @@ with the keys:
   branch from this remote.
 * `git_name`: (optional) The name actually used for this git
   remote, if it's different from the default.
+* `git_repo_url`: URL of the remote git repository.
 
 ## Contributions
 
diff --git a/conf/remotes.yml b/conf/remotes.yml
index 51c523d..cfaa35f 100644
--- a/conf/remotes.yml
+++ b/conf/remotes.yml
@@ -1,6 +1,9 @@
 torvalds:
   commit_url_prefix: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=
+  git_repo_url: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
 stable:
   commit_url_prefix: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit?id=
+  git_repo_url: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
 cip:
   commit_url_prefix: https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git/commit?id=
+  git_repo_url: https://git.kernel.org/pub/scm/linux/kernel/git/cip/linux-cip.git
diff --git a/scripts/import_stable.py b/scripts/import_stable.py
index 26fcc2f..194219d 100755
--- a/scripts/import_stable.py
+++ b/scripts/import_stable.py
@@ -31,11 +31,6 @@ BACKPORT_COMMIT_BOTTOM_RE = re.compile(
     .format(**RE_USE))
 
 
-def update(git_repo, remote_name):
-    subprocess.check_call(['git', 'remote', 'update', remote_name],
-                          cwd=git_repo)
-
-
 def get_backports(git_repo, remotes, branches, debug=False):
     backports = {}
 
@@ -140,7 +135,8 @@ def main(git_repo, remotes, debug=False):
     remote_names = set(branch['git_remote'] for branch in branches)
 
     for remote_name in remote_names:
-        update(git_repo, remotes[remote_name]['git_name'])
+        kernel_sec.branch.remote_update(
+            git_repo, remotes[remote_name]['git_name'])
     backports = get_backports(git_repo, remotes, branches, debug)
     c_b_map = kernel_sec.branch.CommitBranchMap(git_repo, remotes, branches)
 
diff --git a/scripts/kernel_sec/branch.py b/scripts/kernel_sec/branch.py
index a138b96..0023497 100644
--- a/scripts/kernel_sec/branch.py
+++ b/scripts/kernel_sec/branch.py
@@ -223,6 +223,16 @@ def get_remotes(mappings, mainline=None, stable=None):
     return remotes
 
 
+def remote_update(git_repo, remote_name):
+    subprocess.check_call(['git', 'remote', 'update', remote_name],
+                          cwd=git_repo)
+
+
+def remote_add(git_repo, remote_name, remote_url):
+    subprocess.check_call(['git', 'remote', 'add', remote_name, remote_url],
+                          cwd=git_repo)
+
+
 def check_git_repo(git_repo, remotes):
     if not os.path.isdir(git_repo):
         msg = "directory %r not present" % git_repo
diff --git a/scripts/prepare_remotes.py b/scripts/prepare_remotes.py
new file mode 100755
index 0000000..59310fc
--- /dev/null
+++ b/scripts/prepare_remotes.py
@@ -0,0 +1,62 @@
+#!/usr/bin/python3
+
+# Copyright 2019 Toshiba corp.
+# Based on import_stable.py by Codethink Ltd.
+#
+# This script is distributed under the terms and conditions of the GNU General
+# Public License, Version 3 or later. See http://www.gnu.org/copyleft/gpl.html
+# for details.
+
+# Helper script that prepares the local git repository with the configured
+# remote branches
+
+import argparse
+import os
+import subprocess
+
+import kernel_sec.branch
+
+
+def main(git_repo, remotes):
+    if os.path.isdir(git_repo):
+        msg = "directory %r already exists" % git_repo
+        raise argparse.ArgumentError(None, msg)
+    else:
+        os.mkdir(git_repo)
+        subprocess.check_call(['git', 'init', '.'], cwd=git_repo)
+
+    for key in remotes.keys():
+        remote = remotes[key]  # __getitem__ will add git_name
+        kernel_sec.branch.remote_add(
+            git_repo, remote['git_name'], remote['git_repo_url'])
+        kernel_sec.branch.remote_update(git_repo, remote['git_name'])
+
+    # self-check
+    kernel_sec.branch.check_git_repo(git_repo, remotes)
+
+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser(
+        description=('Prepare local git repository with configured remotes.'))
+    parser.add_argument('--git-repo',
+                        dest='git_repo', default='../kernel',
+                        help=('local git repository location '
+                              '(default: ../kernel)'),
+                        metavar='DIRECTORY')
+    parser.add_argument('--remote-name',
+                        dest='remote_name', action='append', default=[],
+                        help='git remote name mappings, e.g. stable:korg-stable',
+                        metavar='NAME:OTHER-NAME')
+    parser.add_argument('--mainline-remote',
+                        dest='mainline_remote_name',
+                        help="git remote name to use instead of 'torvalds'",
+                        metavar='OTHER-NAME')
+    parser.add_argument('--stable-remote',
+                        dest='stable_remote_name',
+                        help="git remote name to use instead of 'stable'",
+                        metavar='OTHER-NAME')
+    args = parser.parse_args()
+    remotes = kernel_sec.branch.get_remotes(args.remote_name,
+                                            mainline=args.mainline_remote_name,
+                                            stable=args.stable_remote_name)
+    main(args.git_repo, remotes)
-- 
2.17.1

[cip-dev] [cip-kernel-sec 3/6] report_affected: fix code when branches are specified

[Daniel Sangorrin]

The previous code could not handle branches with names
other than stable branch names. For example, passing
"linux-4.4.y-cip" as a branch would return an error.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 scripts/kernel_sec/branch.py |  8 --------
 scripts/report_affected.py   | 15 +++++++++------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/scripts/kernel_sec/branch.py b/scripts/kernel_sec/branch.py
index 0023497..ef88b54 100644
--- a/scripts/kernel_sec/branch.py
+++ b/scripts/kernel_sec/branch.py
@@ -21,9 +21,6 @@ import yaml
 from . import version
 
 
-_STABLE_BRANCH_RE = re.compile(r'^linux-([\d.]+)\.y$')
-
-
 def get_base_ver_stable_branch(base_ver):
     branch_name = 'linux-%s.y' % base_ver
     return {
@@ -34,11 +31,6 @@ def get_base_ver_stable_branch(base_ver):
         }
 
 
-def get_stable_branch(branch_name):
-    match = _STABLE_BRANCH_RE.match(branch_name)
-    return match and get_base_ver_stable_branch(match.group(1))
-
-
 def _extract_live_stable_branches(doc):
     xhtml_ns = 'http://www.w3.org/1999/xhtml'
     ns = {'html': xhtml_ns}
diff --git a/scripts/report_affected.py b/scripts/report_affected.py
index d2f1f22..bd22e29 100755
--- a/scripts/report_affected.py
+++ b/scripts/report_affected.py
@@ -18,14 +18,17 @@ import kernel_sec.version
 
 def main(git_repo, remotes,
          only_fixed_upstream, include_ignored, *branch_names):
+    live_branches = kernel_sec.branch.get_live_branches()
     if branch_names:
-        # Support stable release strings as shorthand for stable branches
-        branches = [kernel_sec.branch.get_base_ver_stable_branch(name)
-                    if name[0].isdigit()
-                    else kernel_sec.branch.get_stable_branch(name)
-                    for name in branch_names]
+        branches = []
+        for branch in live_branches:
+            for name in branch_names:
+                if name[0].isdigit():
+                    name = 'linux-%s.y' % name
+                if branch['short_name'] == name:
+                    branches.append(branch)
     else:
-        branches = kernel_sec.branch.get_live_branches()
+        branches = live_branches
         if only_fixed_upstream:
             branches = [branch for branch in branches
                         if branch['short_name'] != 'mainline']
-- 
2.17.1

[cip-dev] [cip-kernel-sec 4/6] report_affected: check user supplied branch names

[Daniel Sangorrin]

This makes sure that we return when the user supplied
branch names are not within the configured branches.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 scripts/report_affected.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/report_affected.py b/scripts/report_affected.py
index bd22e29..7557dc8 100755
--- a/scripts/report_affected.py
+++ b/scripts/report_affected.py
@@ -27,6 +27,9 @@ def main(git_repo, remotes,
                     name = 'linux-%s.y' % name
                 if branch['short_name'] == name:
                     branches.append(branch)
+        if not branches:
+            msg = "supplied branches didn't match any known branch"
+            raise argparse.ArgumentError(None, msg)
     else:
         branches = live_branches
         if only_fixed_upstream:
-- 
2.17.1

[cip-dev] [cip-kernel-sec 5/6] report_affected: add support for reporting on tags

[Daniel Sangorrin]

Reporting on tags is useful for product engineers that
have shipped a kernel with a specific tag and need to know
which issues affect their product after some time.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 scripts/report_affected.py | 60 ++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/scripts/report_affected.py b/scripts/report_affected.py
index 7557dc8..32e9345 100755
--- a/scripts/report_affected.py
+++ b/scripts/report_affected.py
@@ -9,7 +9,9 @@
 # Report issues affecting each stable branch.
 
 import argparse
+import copy
 import subprocess
+import re
 
 import kernel_sec.branch
 import kernel_sec.issue
@@ -23,10 +25,26 @@ def main(git_repo, remotes,
         branches = []
         for branch in live_branches:
             for name in branch_names:
+                # there could be multiple tags for the same branch
+                branch_copy = copy.deepcopy(branch)
+                if name[0] == 'v':
+                    # a stable tag, e.g. v4.4.92-cip11
+                    branch_copy['tag'] = name
+                    match = re.match(r'^v(\d+\.\d+).*', name)
+                    if not match:
+                        raise ValueError('failed to parse tag %r' % name)
+                    if 'cip' in name:
+                        name = 'linux-%s.y-cip' % match.group(1)
+                    else:
+                        name = 'linux-%s.y' % match.group(1)
+                if '/' in name:
+                    # a possibly custom tag, e.g. product-v1
+                    branch_copy['tag'] = name.split('/')[1]
+                    name = name.split('/')[0]
                 if name[0].isdigit():
                     name = 'linux-%s.y' % name
-                if branch['short_name'] == name:
-                    branches.append(branch)
+                if branch_copy['short_name'] == name:
+                    branches.append(branch_copy)
         if not branches:
             msg = "supplied branches didn't match any known branch"
             raise argparse.ArgumentError(None, msg)
@@ -40,6 +58,18 @@ def main(git_repo, remotes,
 
     c_b_map = kernel_sec.branch.CommitBranchMap(git_repo, remotes, branches)
 
+    # cache tag commits and set full_name to show the tag
+    tag_commits = {}
+    for branch in branches:
+        if 'tag' in branch:
+            start = 'v' + branch['base_ver']
+            end = branch['tag']
+            for commit in kernel_sec.branch._get_commits(git_repo, end, start):
+                tag_commits.setdefault(end, []).append(commit)
+            branch['full_name'] = '/'.join([branch['short_name'], end])
+        else:
+            branch['full_name'] = branch['short_name']
+
     branch_issues = {}
     issues = set(kernel_sec.issue.get_list())
 
@@ -60,14 +90,24 @@ def main(git_repo, remotes,
             if not include_ignored and ignore.get(branch_name):
                 continue
 
+            # Check if the branch is affected. If not and the issue was fixed
+            # on that branch, then make sure the tag contains that fix
             if kernel_sec.issue.affects_branch(
                     issue, branch, c_b_map.is_commit_in_branch):
-                branch_issues.setdefault(branch_name, []).append(cve_id)
+                branch_issues.setdefault(
+                    branch['full_name'], []).append(cve_id)
+            elif 'tag' in branch and fixed:
+                if fixed.get(branch_name, 'never') == 'never':
+                    continue
+                for commit in fixed[branch_name]:
+                    if commit not in tag_commits[branch['tag']]:
+                        branch_issues.setdefault(
+                            branch['full_name'], []).append(cve_id)
+                        break
 
     for branch in branches:
-        branch_name = branch['short_name']
-        print('%s:' % branch_name,
-              *sorted(branch_issues.get(branch_name, []),
+        print('%s:' % branch['full_name'],
+              *sorted(branch_issues.get(branch['full_name'], []),
                       key=kernel_sec.issue.get_id_sort_key))
 
 
@@ -99,9 +139,11 @@ if __name__ == '__main__':
                         help='include issues that have been marked as ignored')
     parser.add_argument('branches',
                         nargs='*',
-                        help=('specific branch to report on '
-                              '(default: all active branches)'),
-                        metavar='BRANCH')
+                        help=('specific branch[/tag] or stable tag to '
+                              'report on (default: all active branches). '
+                              'e.g. linux-4.14.y linux-4.4.y/v4.4.107 '
+                              'v4.4.181-cip33 linux-4.19.y-cip/myproduct-v33'),
+                        metavar='[BRANCH[/TAG]|TAG]')
     args = parser.parse_args()
     remotes = kernel_sec.branch.get_remotes(args.remote_name,
                                             mainline=args.mainline_remote_name,
-- 
2.17.1

[cip-dev] [cip-kernel-sec 6/6] pep8: fix pep8-related errors such as too long lines

[Daniel Sangorrin]

These were distracting when checking new code.

Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
---
 scripts/import_stable.py     | 8 +++++---
 scripts/kernel_sec/branch.py | 4 ++--
 scripts/prepare_remotes.py   | 2 +-
 scripts/report_affected.py   | 2 +-
 scripts/webview.py           | 2 +-
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/scripts/import_stable.py b/scripts/import_stable.py
index 194219d..b4d8e0a 100755
--- a/scripts/import_stable.py
+++ b/scripts/import_stable.py
@@ -64,7 +64,8 @@ def get_backports(git_repo, remotes, branches, debug=False):
                     backports.setdefault(mainline_commit, {})[branch_name] \
                         = stable_commit
                 if line.strip() != '':
-                    commit_re = BACKPORT_COMMIT_BOTTOM_RE  # next line is not top
+                    # next line is not top
+                    commit_re = BACKPORT_COMMIT_BOTTOM_RE
 
     return backports
 
@@ -120,7 +121,8 @@ def add_backports(branches, c_b_map, issue_commits, all_backports,
                 if debug_context:
                     print('%s/%s: recording commits' %
                           (debug_context, branch_name))
-                issue_commits.setdefault(branch_name, []).extend(branch_commits)
+                issue_commits.setdefault(
+                    branch_name, []).extend(branch_commits)
                 changed = True
             else:
                 if debug_context:
@@ -169,7 +171,7 @@ if __name__ == '__main__':
                         metavar='DIRECTORY')
     parser.add_argument('--remote-name',
                         dest='remote_name', action='append', default=[],
-                        help='git remote name mappings, e.g. stable:korg-stable',
+                        help='git remote name mappings, e.g. stable:mystable',
                         metavar='NAME:OTHER-NAME')
     parser.add_argument('--mainline-remote',
                         dest='mainline_remote_name',
diff --git a/scripts/kernel_sec/branch.py b/scripts/kernel_sec/branch.py
index ef88b54..9a7bc3a 100644
--- a/scripts/kernel_sec/branch.py
+++ b/scripts/kernel_sec/branch.py
@@ -62,8 +62,8 @@ def _extract_live_stable_branches(doc):
             if match:
                 version = match.group(1)
                 eol = match.group(2) is not None
-        if branch_type not in ['mainline', 'stable', 'longterm', 'linux-next'] \
-           or version is None:
+        if branch_type not in ['mainline', 'stable', 'longterm',
+                               'linux-next'] or version is None:
             raise ValueError('failed to parse releases row text %r' % row_text)
 
         # Filter out irrelevant branches
diff --git a/scripts/prepare_remotes.py b/scripts/prepare_remotes.py
index 59310fc..ab0db24 100755
--- a/scripts/prepare_remotes.py
+++ b/scripts/prepare_remotes.py
@@ -45,7 +45,7 @@ if __name__ == '__main__':
                         metavar='DIRECTORY')
     parser.add_argument('--remote-name',
                         dest='remote_name', action='append', default=[],
-                        help='git remote name mappings, e.g. stable:korg-stable',
+                        help='git remote name mappings, e.g. stable:mystable',
                         metavar='NAME:OTHER-NAME')
     parser.add_argument('--mainline-remote',
                         dest='mainline_remote_name',
diff --git a/scripts/report_affected.py b/scripts/report_affected.py
index 32e9345..0296649 100755
--- a/scripts/report_affected.py
+++ b/scripts/report_affected.py
@@ -121,7 +121,7 @@ if __name__ == '__main__':
                         metavar='DIRECTORY')
     parser.add_argument('--remote-name',
                         dest='remote_name', action='append', default=[],
-                        help='git remote name mappings, e.g. stable:korg-stable',
+                        help='git remote name mappings, e.g. stable:mystable',
                         metavar='NAME:OTHER-NAME')
     parser.add_argument('--mainline-remote',
                         dest='mainline_remote_name',
diff --git a/scripts/webview.py b/scripts/webview.py
index 52a7b15..cf54948 100755
--- a/scripts/webview.py
+++ b/scripts/webview.py
@@ -205,7 +205,7 @@ if __name__ == '__main__':
                         metavar='DIRECTORY')
     parser.add_argument('--remote-name',
                         dest='remote_name', action='append', default=[],
-                        help='git remote name mappings, e.g. stable:korg-stable',
+                        help='git remote name mappings, e.g. stable:mystable',
                         metavar='NAME:OTHER-NAME')
     parser.add_argument('--mainline-remote',
                         dest='mainline_remote_name',
-- 
2.17.1

[cip-dev] [cip-kernel-sec 3/6] report_affected: fix code when branches are specified

[Ben Hutchings]

On Tue, 2019-06-25 at 12:26 +0900, Daniel Sangorrin wrote:
> The previous code could not handle branches with names
> other than stable branch names. For example, passing
> "linux-4.4.y-cip" as a branch would return an error.
[...]
> --- a/scripts/report_affected.py
> +++ b/scripts/report_affected.py
> @@ -18,14 +18,17 @@ import kernel_sec.version
> ?
> ?def main(git_repo, remotes,
> ??????????only_fixed_upstream, include_ignored, *branch_names):
> +????live_branches = kernel_sec.branch.get_live_branches()
> ?????if branch_names:
> -????????# Support stable release strings as shorthand for stable branches
> -????????branches = [kernel_sec.branch.get_base_ver_stable_branch(name)
> -????????????????????if name[0].isdigit()
> -????????????????????else kernel_sec.branch.get_stable_branch(name)
> -????????????????????for name in branch_names]
> +????????branches = []
> +????????for branch in live_branches:
> +????????????for name in branch_names:
> +????????????????if name[0].isdigit():
> +????????????????????name = 'linux-%s.y' % name
> +????????????????if branch['short_name'] == name:
> +????????????????????branches.append(branch)
[...]

This results in quietly skipping arguments that don't match any known
branch.  The current behaviour (failing with a TypeError) is not good
but I think failing quietly is worse.

Ben.

-- 
Ben Hutchings, Software Developer                ?        Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom

[cip-dev] [cip-kernel-sec 4/6] report_affected: check user supplied branch names

[Ben Hutchings]

On Tue, 2019-06-25 at 12:26 +0900, Daniel Sangorrin wrote:
> This makes sure that we return when the user supplied
> branch names are not within the configured branches.

This partly addresses my comment on patch 3, but it's still only
checking that at least one name matched a known branch.

I think the inner and outer loops should be swapped, so we can do:

????????branches = []
????????for name in branch_names:
????????????if name[0].isdigit():
????????????????name = 'linux-%s.y' % name
    ????????for branch in live_branches:
????????????????if branch['short_name'] == name:
????????????????????branches.append(branch)
                    break
            else:
                # not found; raise error

Ben.

> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
> ?scripts/report_affected.py | 3 +++
> ?1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/report_affected.py b/scripts/report_affected.py
> index bd22e29..7557dc8 100755
> --- a/scripts/report_affected.py
> +++ b/scripts/report_affected.py
> @@ -27,6 +27,9 @@ def main(git_repo, remotes,
> ?????????????????????name = 'linux-%s.y' % name
> ?????????????????if branch['short_name'] == name:
> ?????????????????????branches.append(branch)
> +????????if not branches:
> +????????????msg = "supplied branches didn't match any known branch"
> +????????????raise argparse.ArgumentError(None, msg)
> ?????else:
> ?????????branches = live_branches
> ?????????if only_fixed_upstream:
-- 
Ben Hutchings, Software Developer                ?        Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom

[cip-dev] [cip-kernel-sec 5/6] report_affected: add support for reporting on tags

[Ben Hutchings]

On Tue, 2019-06-25 at 12:26 +0900, Daniel Sangorrin wrote:
> Reporting on tags is useful for product engineers that
> have shipped a kernel with a specific tag and need to know
> which issues affect their product after some time.

I think this is a really important feature, but I'm not convinced about
this implementation.

> Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> ---
> ?scripts/report_affected.py | 60 ++++++++++++++++++++++++++++++++------
> ?1 file changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/scripts/report_affected.py b/scripts/report_affected.py
> index 7557dc8..32e9345 100755
> --- a/scripts/report_affected.py
> +++ b/scripts/report_affected.py
> @@ -9,7 +9,9 @@
> ?# Report issues affecting each stable branch.
> ?
> ?import argparse
> +import copy
> ?import subprocess
> +import re
> ?
> ?import kernel_sec.branch
> ?import kernel_sec.issue
> @@ -23,10 +25,26 @@ def main(git_repo, remotes,
> ?????????branches = []
> ?????????for branch in live_branches:
> ?????????????for name in branch_names:
> +????????????????# there could be multiple tags for the same branch
> +????????????????branch_copy = copy.deepcopy(branch)

This makes a lot of unnecessary copies!

> +????????????????if name[0] == 'v':
> +????????????????????# a stable tag, e.g. v4.4.92-cip11
> +????????????????????branch_copy['tag'] = name
> +????????????????????match = re.match(r'^v(\d+\.\d+).*', name)
> +????????????????????if not match:
> +????????????????????????raise ValueError('failed to parse tag %r' % name)
> +????????????????????if 'cip' in name:
> +????????????????????????name = 'linux-%s.y-cip' % match.group(1)
> +????????????????????else:
> +????????????????????????name = 'linux-%s.y' % match.group(1)

How about adding regexps for tags to the branch configuration?  Then
here we could match tags to branches with
re.matchall(branch['tag_regexp'], name).

> +????????????????if '/' in name:

'/' is valid in a branch or tag name, so how about using ':' which is
not?

> +????????????????????# a possibly custom tag, e.g. product-v1
> +????????????????????branch_copy['tag'] = name.split('/')[1]
> +????????????????????name = name.split('/')[0]

It would be more Pythonic to write this as a tuple assginment.

> ?????????????????if name[0].isdigit():
> ?????????????????????name = 'linux-%s.y' % name
> -????????????????if branch['short_name'] == name:
> -????????????????????branches.append(branch)
> +????????????????if branch_copy['short_name'] == name:
> +????????????????????branches.append(branch_copy)
> ?????????if not branches:
> ?????????????msg = "supplied branches didn't match any known branch"
> ?????????????raise argparse.ArgumentError(None, msg)
> @@ -40,6 +58,18 @@ def main(git_repo, remotes,
> ?
> ?????c_b_map = kernel_sec.branch.CommitBranchMap(git_repo, remotes, branches)
> ?
> +????# cache tag commits and set full_name to show the tag
> +????tag_commits = {}
> +????for branch in branches:
> +????????if 'tag' in branch:
> +????????????start = 'v' + branch['base_ver']
> +????????????end = branch['tag']
> +????????????for commit in kernel_sec.branch._get_commits(git_repo, end, start):

The leading '_' means private, so it shouldn't be called from here.  I
think it could be made public but it needs a better name, maybe
'iter_rev_list' matching the underlying command.

> +????????????????tag_commits.setdefault(end, []).append(commit)
[...]

Suppose I tried to query 'v4.4'; then I think we get start == end ==
'v4.4' and this loop doesn't execute at all.  tag_commits['v4.4'] won't
be defined at all, and the reporting loop will crash.

So this could be simply:

               tag_commits[end] = list(
                   kernel_sec.branch.iter_rev_list(git_repo, end, start))

But I think the collection type should also be changed from list to
set, so that tests for membership will be fast.

Ben.

-- 
Ben Hutchings, Software Developer                ?        Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom

[cip-dev] [cip-kernel-sec 3/6] report_affected: fix code when branches are specified

[daniel.sangorrin at toshiba.co.jp]

> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
> On Tue, 2019-06-25 at 12:26 +0900, Daniel Sangorrin wrote:
> > The previous code could not handle branches with names
> > other than stable branch names. For example, passing
> > "linux-4.4.y-cip" as a branch would return an error.
> [...]
> > --- a/scripts/report_affected.py
> > +++ b/scripts/report_affected.py
> > @@ -18,14 +18,17 @@ import kernel_sec.version
> >
> > ?def main(git_repo, remotes,
> > ??????????only_fixed_upstream, include_ignored, *branch_names):
> > +????live_branches = kernel_sec.branch.get_live_branches()
> > ?????if branch_names:
> > -????????# Support stable release strings as shorthand for stable branches
> > -????????branches = [kernel_sec.branch.get_base_ver_stable_branch(name)
> > -????????????????????if name[0].isdigit()
> > -????????????????????else kernel_sec.branch.get_stable_branch(name)
> > -????????????????????for name in branch_names]
> > +????????branches = []
> > +????????for branch in live_branches:
> > +????????????for name in branch_names:
> > +????????????????if name[0].isdigit():
> > +????????????????????name = 'linux-%s.y' % name
> > +????????????????if branch['short_name'] == name:
> > +????????????????????branches.append(branch)
> [...]
> 
> This results in quietly skipping arguments that don't match any known
> branch.  The current behaviour (failing with a TypeError) is not good
> but I think failing quietly is worse.

I was taking the approach of "doing as much as possible". Something like what a recovery tool does on a disk with broken sectors: if a broken sector is found skip it.

In the new version, the script will raise an argument error if it cannot find the corresponding stable branch.

Thanks,
Daniel


> 
> Ben.
> 
> --
> Ben Hutchings, Software Developer                ?        Codethink Ltd
> https://www.codethink.co.uk/                 Dale House, 35 Dale Street
>                                      Manchester, M1 2HF, United Kingdom

[cip-dev] [cip-kernel-sec 4/6] report_affected: check user supplied branch names

[daniel.sangorrin at toshiba.co.jp]

> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
> On Tue, 2019-06-25 at 12:26 +0900, Daniel Sangorrin wrote:
> > This makes sure that we return when the user supplied
> > branch names are not within the configured branches.
> 
> This partly addresses my comment on patch 3, but it's still only
> checking that at least one name matched a known branch.
> 
> I think the inner and outer loops should be swapped, so we can do:
> 
> ????????branches = []
> ????????for name in branch_names:
> ????????????if name[0].isdigit():
> ????????????????name = 'linux-%s.y' % name
>     ????????for branch in live_branches:
> ????????????????if branch['short_name'] == name:
> ????????????????????branches.append(branch)
>                     break
>             else:
>                 # not found; raise error
> 
> Ben.

Yes, you are right. I came up with exactly the same solution.

Thanks,
Daniel


> 
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> > ?scripts/report_affected.py | 3 +++
> > ?1 file changed, 3 insertions(+)
> >
> > diff --git a/scripts/report_affected.py b/scripts/report_affected.py
> > index bd22e29..7557dc8 100755
> > --- a/scripts/report_affected.py
> > +++ b/scripts/report_affected.py
> > @@ -27,6 +27,9 @@ def main(git_repo, remotes,
> > ?????????????????????name = 'linux-%s.y' % name
> > ?????????????????if branch['short_name'] == name:
> > ?????????????????????branches.append(branch)
> > +????????if not branches:
> > +????????????msg = "supplied branches didn't match any known branch"
> > +????????????raise argparse.ArgumentError(None, msg)
> > ?????else:
> > ?????????branches = live_branches
> > ?????????if only_fixed_upstream:
> --
> Ben Hutchings, Software Developer                ?        Codethink Ltd
> https://www.codethink.co.uk/                 Dale House, 35 Dale Street
>                                      Manchester, M1 2HF, United Kingdom

[cip-dev] [cip-kernel-sec 5/6] report_affected: add support for reporting on tags

[daniel.sangorrin at toshiba.co.jp]

> From: Ben Hutchings <ben.hutchings@codethink.co.uk>
> 
> On Tue, 2019-06-25 at 12:26 +0900, Daniel Sangorrin wrote:
> > Reporting on tags is useful for product engineers that
> > have shipped a kernel with a specific tag and need to know
> > which issues affect their product after some time.
> 
> I think this is a really important feature, but I'm not convinced about
> this implementation.

Glad to hear about the importance. Other members also expressed their interest.
 
> > Signed-off-by: Daniel Sangorrin <daniel.sangorrin@toshiba.co.jp>
> > ---
> > ?scripts/report_affected.py | 60 ++++++++++++++++++++++++++++++++------
> > ?1 file changed, 51 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/report_affected.py b/scripts/report_affected.py
> > index 7557dc8..32e9345 100755
> > --- a/scripts/report_affected.py
> > +++ b/scripts/report_affected.py
> > @@ -9,7 +9,9 @@
> > ?# Report issues affecting each stable branch.
> >
> > ?import argparse
> > +import copy
> > ?import subprocess
> > +import re
> >
> > ?import kernel_sec.branch
> > ?import kernel_sec.issue
> > @@ -23,10 +25,26 @@ def main(git_repo, remotes,
> > ?????????branches = []
> > ?????????for branch in live_branches:
> > ?????????????for name in branch_names:
> > +????????????????# there could be multiple tags for the same branch
> > +????????????????branch_copy = copy.deepcopy(branch)
> 
> This makes a lot of unnecessary copies!

Sorry about that. The new version only makes copies when necessary (one copy per branch name supplied by the user).


> 
> > +????????????????if name[0] == 'v':
> > +????????????????????# a stable tag, e.g. v4.4.92-cip11
> > +????????????????????branch_copy['tag'] = name
> > +????????????????????match = re.match(r'^v(\d+\.\d+).*', name)
> > +????????????????????if not match:
> > +????????????????????????raise ValueError('failed to parse tag %r' % name)
> > +????????????????????if 'cip' in name:
> > +????????????????????????name = 'linux-%s.y-cip' % match.group(1)
> > +????????????????????else:
> > +????????????????????????name = 'linux-%s.y' % match.group(1)
> 
> How about adding regexps for tags to the branch configuration?  Then
> here we could match tags to branches with
> re.matchall(branch['tag_regexp'], name).

OK, I have tried. Let me know if my implementation is what you had in mind.
I used re.match, not re.matchall (maybe you meant re.findall?). Let me know if there is a problem with that.
Tags that are not on a stable branch (e.g. 5.2 is in mainline but not in a stable branch) are ignored at the moment.

> 
> > +????????????????if '/' in name:
> 
> '/' is valid in a branch or tag name, so how about using ':' which is
> not?

Thanks, I had chosen '/' just for aesthetics (the final report uses : before displaying the issues).
Now fixed.

> 
> > +????????????????????# a possibly custom tag, e.g. product-v1
> > +????????????????????branch_copy['tag'] = name.split('/')[1]
> > +????????????????????name = name.split('/')[0]
> 
> It would be more Pythonic to write this as a tuple assginment.

I tried using tuples, but I'm not sure it is pythonic enough.
There is also a "tag = None" that may not be pythonic either.

> 
> > ?????????????????if name[0].isdigit():
> > ?????????????????????name = 'linux-%s.y' % name
> > -????????????????if branch['short_name'] == name:
> > -????????????????????branches.append(branch)
> > +????????????????if branch_copy['short_name'] == name:
> > +????????????????????branches.append(branch_copy)
> > ?????????if not branches:
> > ?????????????msg = "supplied branches didn't match any known branch"
> > ?????????????raise argparse.ArgumentError(None, msg)
> > @@ -40,6 +58,18 @@ def main(git_repo, remotes,
> >
> > ?????c_b_map = kernel_sec.branch.CommitBranchMap(git_repo, remotes, branches)
> >
> > +????# cache tag commits and set full_name to show the tag
> > +????tag_commits = {}
> > +????for branch in branches:
> > +????????if 'tag' in branch:
> > +????????????start = 'v' + branch['base_ver']
> > +????????????end = branch['tag']
> > +????????????for commit in kernel_sec.branch._get_commits(git_repo, end, start):
> 
> The leading '_' means private, so it shouldn't be called from here.  I
> think it could be made public but it needs a better name, maybe
> 'iter_rev_list' matching the underlying command.

Fixed.

> > +????????????????tag_commits.setdefault(end, []).append(commit)
> [...]
> 
> Suppose I tried to query 'v4.4'; then I think we get start == end ==
> 'v4.4' and this loop doesn't execute at all.  tag_commits['v4.4'] won't
> be defined at all, and the reporting loop will crash.

Good catch, I should have tested the edge cases sorry.

> So this could be simply:
> 
>                tag_commits[end] = list(
>                    kernel_sec.branch.iter_rev_list(git_repo, end, start))
> 
> But I think the collection type should also be changed from list to
> set, so that tests for membership will be fast.

Thanks, I finally used "set" although I didn't see big performance changes.

Thanks,
Daniel

> 
> Ben.
> 
> --
> Ben Hutchings, Software Developer                ?        Codethink Ltd
> https://www.codethink.co.uk/                 Dale House, 35 Dale Street
>                                      Manchester, M1 2HF, United Kingdom