Issue 5250 - Add copyright update script

update-copyright/update_copyright.py

 #!/usr/bin/env python3
 
 import os
 import subprocess
 import re
 import datetime
 import shutil
 import urllib.parse
 import urllib.request
 import html.parser
 import argparse
-from posixpath import dirname
 
 
 CURRENT_YEAR = datetime.datetime.now().year
 
 
 def process_repo(url, hg_upstream):

[Vasily Kuznetsov, 2017/06/27 16:18:54]

Is `hg_upstream` unused in this function? It should probably be passed to
`hg_commit` if I follow the logic correctly.

[rosie, 2017/07/03 15:33:46]

Done.

-    repo = os.path.basename(os.path.normpath(url))

[Vasily Kuznetsov, 2017/06/27 16:18:51]

Why are we applying `os.path.normpath` to a url here? This might work sometimes,
but in general that function is intended for filesystem paths, not urls. I
wonder if this won't break in a funny way some day.

[rosie, 2017/07/03 15:33:45]

Done.

+    repo = url.rstrip('/').split('/')[-1]
+
     if repo in {
         # headers are copied from libadblockplus, no need to update seperately
         'libadblockplus-binaries',
         # huge and only contains autogenerated builds
         'downloads',
     }:
         return
 
     try:
         subprocess.check_call(['hg', 'clone', url, repo])
         if repo == 'adblockbrowser':
             # adblockbrowser is a FF fork with its own changes in a
             #   seperate branch
             subprocess.check_call(['hg', 'up', '--rev', 'adblockbrowser',
                                    '--repository', repo])
         else:
             # switch to 'master' bookmark if it exists
             subprocess.call(['hg', 'up', '--rev', 'master',

[Vasily Kuznetsov, 2017/06/27 16:18:54]

Any idea why we're not doing `check_call` here just like in the other branch of
`if`? Seems like if `hg` fails, we should also break.

[rosie, 2017/07/03 15:33:45]

This line attempts to switch to the master branch.  If a branch called `master`
doesn't exist, then `hg up --rev master` returns a 255, but the script can keep
running whether or not there's a branch called master.

[Vasily Kuznetsov, 2017/07/03 19:25:45]

I see. Makes sense.

                              '--repository', repo])
         for dirpath, dirnames, filenames in os.walk(repo):
             if dirpath == repo:
                 dirnames.remove('.hg')
 
             for filename in filenames:
                 text_replace(dirpath, filename)
-        hg_commit(repo, url)
+        if hg_upstream is None:
+            hg_upstream = url
+        else:
+            hg_upstream += '/' + repo
+        hg_commit(repo, hg_upstream)
 
     finally:
         shutil.rmtree(repo, ignore_errors=True)
 
 
 def text_replace(dirpath, filename):
     with open(os.path.join(dirpath, filename), 'r+',
               encoding='utf-8', newline='') as file:
         try:
             text = file.read()
         except UnicodeDecodeError:
-            print("Error: Couldn't read {}{}".format(dirpath, filename))
             return
 
         text = re.sub(
             r'(copyright.*?\d{4})(?:-\d{4})?\s+eyeo gmbh',
             r'\1-{} eyeo GmbH'.format(CURRENT_YEAR), text, 0, re.I
         )
         file.seek(0)
         file.write(text)
         file.truncate()
 
 
 def hg_commit(repo, hg_upstream):
     try:
         subprocess.check_call(['hg', 'commit', '-m',
                                'Noissue - Updated copyright year',
                                '--repository', repo])
     except subprocess.CalledProcessError as e:
         if e.returncode == 1:  # no changes
             return
         raise
 
     # Push changes, or save patch if access denied
-    if 'ssh://hg@hg.adblockplus.org/' in hg_upstream:

[Vasily Kuznetsov, 2017/06/27 16:18:52]

`hg_commit` is called with the original url of the repository on line 45 of
`process_repo` as a second parameter, so it seems like `hg_upstream` should
already be a complete URL in it and we don't need to add `repo` to it. Or am I
missing something?

[rosie, 2017/07/03 15:33:45]

If we're pushing to ssh://hg@hg.adblockplus.org/ servers, then we need to add
the repo name on the end. Otherwise, it will push to the URL for each of the
repos it finds on the hg_page, and we don't need to add the repo names to the
end of those.

[rosie, 2017/07/04 13:38:24]

If we're pushing to ssh://hg@hg.adblockplus.org/ servers, then we need to add
the repo name on the end. Otherwise (not using SSH), it will push to the URL for
each of the repos it finds on the hg_page, and we don't need to add the repo
names to the end of those.

-        hg_upstream += repo
     if subprocess.call(['hg', 'push', '--repository', repo, hg_upstream]) != 0:
         with open(repo + '.patch', 'wb') as file:
             print('couldnt push, making patch instead')
             subprocess.check_call(['hg', 'export', '--repository', repo],
                                   stdout=file)
 
 
 class Parser(html.parser.HTMLParser):
     result = []
     recordtr = False
     cell = 0
     current_url = ''
-    dir_path = 'https://hg.adblockplus.org/'

[Vasily Kuznetsov, 2017/06/27 16:18:52]

Is this used anywhere?

[rosie, 2017/07/03 15:33:44]

Done.

 
     def handle_starttag(self, tag, attrs):
         if tag == 'tr':
             self.recordtr = True
         if tag == 'td':
             self.cell += 1
         if tag == 'a':
             attrs = dict(attrs)
             if 'list' in attrs.get('class', '').split():
                 self.current_url = attrs['href']
 
     def handle_endtag(self, tag):
         if tag == 'tr':
             self.recordtr = False
             self.cell = 0
 
     def handle_data(self, data):
-        if (self.cell == 2) and self.recordtr is True:

[Vasily Kuznetsov, 2017/06/27 16:18:51]

The parentheses around the first operand of `and` are redundant so it would be
more pythonic ;) to remove them.

[rosie, 2017/07/03 15:33:45]

Done.

+        if self.cell == 2 and self.recordtr is True:
             self.recordtr = False
             self.cell = 0
             # Only process the URL if the description is not Deprecated
-            depr1 = re.search(r'\*DEPRECATED\*', data)

[Vasily Kuznetsov, 2017/06/27 16:18:52]

I think it would make the code more readable and extensible if we do something
like this instead:

    deprecated = (re.search(....) or
                  re.search(....))
    if not deprecated and ...:

[rosie, 2017/07/03 15:33:44]

Done.

-            depr2 = re.search(r'(Deprecated)', data)
-            if not depr1 and not depr2 and len(self.current_url) > 2:
+            if ('*DEPRECATED*' not in data and
+                    '(Deprecated)' not in data and
+                    len(self.current_url) > 2):
                 self.result += [self.current_url]
                 return self.result
 
 
 def extract_urls(hg_page):
-    dir_path = dirname(hg_page) + '/'

[Vasily Kuznetsov, 2017/06/27 16:18:52]

Two points here:
- `hg_page` is an url, could we use something other than `dirname` from
`posixpath` (that's a bit of a misuse, I think)?
- `dir_path` is essentially a base url, perhaps that would be a more telling
name. What do you think?

[rosie, 2017/07/03 15:33:45]

Done.

+    base_url = os.path.dirname(hg_page) + '/'
     parser = Parser()
     with urllib.request.urlopen(hg_page) as response:
         parser.feed(response.read().decode('utf-8'))
     parser.close()
-    my_result = []

[Vasily Kuznetsov, 2017/06/27 16:18:52]

Maybe better call this `repo_urls`?

[rosie, 2017/07/03 15:33:46]

Done.

-    for i in range(len(parser.result)):

[Vasily Kuznetsov, 2017/06/27 16:18:53]

Why not `for url in parser.result:`?

[rosie, 2017/07/03 15:33:45]

Done.

-        my_result.append(urllib.parse.urljoin(dir_path, parser.result[i]))
-    return my_result
+    repo_urls = []
+    for url in parser.result:
+        repo_urls.append(urllib.parse.urljoin(base_url, url))
+    return repo_urls
 
 
 def main(hg_page, hg_upstream):
-    repo_list = extract_urls(hg_page)
-    for repo in repo_list:

[Vasily Kuznetsov, 2017/06/27 16:18:52]

Could probably just be `for repo in extract_urls(hg_page)`.

[rosie, 2017/07/03 15:33:45]

Done.

+    for repo in extract_urls(hg_page):
         process_repo(repo, hg_upstream)
 
 
 if __name__ == '__main__':
     arg_parser = argparse.ArgumentParser()
-    arg_parser.add_argument('-u', '--url',
-                            default='https://hg.adblockplus.org/',
+    arg_parser.add_argument('-u', '--hg-url',
                             help='specify which Mercurial URL site to scrape')
-    arg_parser.add_argument('-p', '--push',
-                            default='ssh://hg@hg.adblockplus.org/',

[Vasily Kuznetsov, 2017/06/27 16:18:52]

This should probably default to `None` and then if not specified, we should use
the value of `--url`. Otherwise if we specify `--url` but no `--push`, the
behavior will be rather unexpected.

It would probably also be better to name the option `--push-url` to make it more
clear.

[rosie, 2017/07/03 15:33:46]

Done.

+    arg_parser.add_argument('-p', '--push-url',
                             help='specify where to push the repository')
     args = arg_parser.parse_args()
-    hg_page = args.url
-    hg_upstream = args.push
-    main(hg_page, hg_upstream)
+    if args.hg_url and not args.push_url:
+        arg_parser.error('If -u is provided, -p is mandatory')
+    main(args.hg_url or 'https://hg.adblockplus.org/',
+         args.push_url or 'ssh://hg@hg.adblockplus.org/')