Merge lp:~allenap/bzr-update-copyright/force-ranges into lp:bzr-update-copyright

[Gavin Panella]

This branch adds a --force-ranges option (which I'm happy to rename) which always formats years as $min-$max or $year; it ignores gaps. This is how we do it on MAAS because it means we don't have to re-wrap copyright stanzas to appease the linter. It may not be "proper" but it's good enough.

I've also cleaned up all PEP8 warnings. I can revert these changes if they're unwanted.

[Richard Wilbur (richard-wilbur)]

Since I don't see a bug number that this is solving, a little background would be useful to help me understand. You mention "MAAS" as formatting years in the copyright statement either as a range or a single year. What is "MAAS"?

My second question regarding copyright dates is, "What is 'proper' and how hard would that be to implement?"

[Gavin Panella (allenap)]

> Since I don't see a bug number that this is solving, a little
> background would be useful to help me understand. You mention "MAAS"
> as formatting years in the copyright statement either as a range or a
> single year. What is "MAAS"?

MAAS is https://launchpad.net/maas. We have a standard copyright header
at the top of each file. Naturally it begins life with a single year,
but that can later be substituted with a date range, "2012-2015" or
"2012, 2013" for example, without causing the line to extend to more
than 80 characters.

When there are three years and they're not all contiguous with one
another, bzr-update-copyright will render it longer, as "2012, 2013,
2015" for example. For us it means that the line grows long enough that
our linter complains, meaning we would have to reflow the header.

It sounds really lazy to have an issue with that, and I kind of agree,
but then I also don't want to replace one manual job (changing the
header dates) with another (reflowing the paragraph), especially when
forgetting to do the latter would cause our landing bot to reject
submissions.

In any case, we've always used ${year-created}-${year-last-modified} in
MAAS, albeit manually. We would be happy to condense the non-contiguous
example from earlier into "2012-2015", even though no changes were made
in 2014.

So, this isn't in response to a bug, it's an itch that I scratched.

> My second question regarding copyright dates is, "What is 'proper' and
> how hard would that be to implement?"

"Proper" may have been an overstatement. I think of the current
behaviour of this plugin as "proper", and --force-range as somewhat
"improper", but I don't actually know which is more proper than the
other. I was motivated only to automate our current practice without
disrupting existing users of bzr-update-copyright.

[Richard Wilbur (richard-wilbur)]

Thanks for the background.

I have a question regarding line 32 of __init__.py: Why is this the only line of option description text delimited by double quotes(")? All the others seems to use single quotes(').

Other than that, it looks good to me.

[lp:~allenap/bzr-update-copyright/force-ranges]

19. By Gavin Panella on 2015-05-07

Make quoting consistent.

[Gavin Panella (allenap)]

Thanks Richard. I've fixed the quotes to be consistent.

[Richard Wilbur (richard-wilbur)]

Thank you for the PEP8 clean up and the new option and documentation.
+1

review: Approve

=== modified file '__init__.py'
--- __init__.py	2014-04-10 08:16:12 +0000
+++ __init__.py	2015-05-07 09:22:34 +0000
@@ -28,12 +28,10 @@
 want it active everywhere.
 """
 
-
 from bzrlib import (
     commands,
-    errors,
     _format_version_tuple,
-    mutabletree, # I wish we could make the hook registration fully lazy
+    mutabletree,  # I wish we could make the hook registration fully lazy
     option,
     trace,
     )
@@ -56,12 +54,16 @@
     """
 
     takes_args = ['path*']
-    takes_options = [option.Option('only-changed',
-                        help='Only update the copyright for files which are'
-                             ' marked as modified.'),
-                    ]
+    takes_options = [
+        option.Option(
+            'only-changed', help='Only update the copyright for files which '
+            'are marked as modified.'),
+        option.Option(
+            'force-range', help='Only update the copyright with a range of '
+            'years, i.e. ignore years in which no change was made.'),
+    ]
 
-    def run(self, path_list=None, only_changed=False):
+    def run(self, path_list=None, only_changed=False, force_range=False):
         from bzrlib import osutils, workingtree
         import update_copyright
 
@@ -72,8 +74,8 @@
             relpaths = None
         if relpaths is not None:
             relpaths = osutils.minimum_path_selection(relpaths)
-        result = update_copyright.update_tree_copyright(tree, relpaths,
-                only_changed=only_changed)
+        result = update_copyright.update_tree_copyright(
+            tree, relpaths, only_changed=only_changed, force_range=force_range)
         self.outf.write('Checked %(count)d files\n'
                         'Updated %(updated)d\n'
                         'Already correct %(copyright-correct)d\n'
@@ -105,8 +107,8 @@
                  % res)
 
 
-mutabletree.MutableTree.hooks.install_named_hook('start_commit',
-    _update_copyright_start_commit_hook, 'update-copyright')
+mutabletree.MutableTree.hooks.install_named_hook(
+    'start_commit', _update_copyright_start_commit_hook, 'update-copyright')
 
 
 def load_tests(standard_tests, module, loader):

=== modified file 'test_update_copyright.py'
--- test_update_copyright.py	2014-04-10 08:10:39 +0000
+++ test_update_copyright.py	2015-05-07 09:22:34 +0000
@@ -13,12 +13,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-# 
+#
 
 import datetime
 
 from bzrlib import (
-    errors,
     mutabletree,
     tests,
     )
@@ -80,6 +79,18 @@
         self.assertFormatYears('2005, 2007-2010',
                                [2005, 2007, 2008, 2009, 2010])
 
+    def assertFormatYearsAsRange(self, expected, years):
+        self.assertEqual(
+            expected, update_copyright.format_years_as_range(set(years)))
+
+    def test_format_years_as_range(self):
+        self.assertFormatYearsAsRange('2008-2009', [2008, 2009])
+        self.assertFormatYearsAsRange('2007-2009', [2007, 2009])
+        self.assertFormatYearsAsRange('2007-2009', [2007, 2008, 2009])
+        self.assertFormatYearsAsRange('2007-2010', [2007, 2008, 2009, 2010])
+        self.assertFormatYearsAsRange(
+            '2005-2010', [2005, 2007, 2008, 2009, 2010])
+
 
 class TestUpdateCopyrightDisk(tests.TestCaseWithTransport):
 
@@ -87,24 +98,26 @@
         t = self.make_branch_and_tree('.')
         t.lock_write()
         self.addCleanup(t.unlock)
-        self.build_tree_contents([('file',
-            'Copyright (c) 2008 Foo Bar\nsome content\nmore content\n')])
+        self.build_tree_contents([
+            ('file',
+             'Copyright (c) 2008 Foo Bar\nsome content\nmore content\n')])
         t.add(['file'], ['file-id'])
         rev_ids = []
         rev_ids.append(
-            t.commit('new file', timestamp=1199817864, timezone=0)) # 2008
-        self.build_tree_contents([('file',
-            'Copyright (c) 2008 Foo Bar\ndifferent content\n')])
+            t.commit('new file', timestamp=1199817864, timezone=0))  # 2008
+        self.build_tree_contents([
+            ('file', 'Copyright (c) 2008 Foo Bar\ndifferent content\n')])
         rev_ids.append(
-            t.commit('mod file', timestamp=1231353864, timezone=0)) # 2009
+            t.commit('mod file', timestamp=1231353864, timezone=0))  # 2009
         return t, rev_ids
 
     def make_old_modified_but_correct_tree(self):
         t, rev_ids = self.make_old_modified_tree()
-        self.build_tree_contents([('file',
-            'Copyright (c) 2008, 2009 Foo Bar\ndifferent content\n')])
-        rev_ids.append(
-            t.commit('fix copyright', timestamp=1231353865, timezone=0)) # 2009
+        self.build_tree_contents([
+            ('file', 'Copyright (c) 2008, 2009 Foo Bar\ndifferent content\n'),
+        ])
+        rev_ids.append(t.commit(
+            'fix copyright', timestamp=1231353865, timezone=0))  # 2009
         return t, rev_ids
 
     def make_tree_with_dirs(self):
@@ -112,15 +125,15 @@
         self.build_tree(['dir/'])
         self.build_tree_contents([('dir/file', 'Copyright 2008 Foo\r\n')])
         t.add(['dir', 'dir/file'], ['dir-id', 'dfile-id'])
-        rev_ids.append(
-            t.commit('dir and file', timestamp=1199817865, timezone=0)) # 2008
+        rev_ids.append(t.commit(
+            'dir and file', timestamp=1199817865, timezone=0))  # 2008
         self.build_tree(['dir/subdir/'])
         self.build_tree_contents([
             ('dir/file', 'Copyright 2008 Foo\r\nnew content\r\n'),
             ('dir/subdir/foo', 'Copyright 2008 Bar\ncontent\n')])
         t.add(['dir/subdir', 'dir/subdir/foo'], ['subdir-id', 'foo-id'])
         rev_ids.append(
-            t.commit('mods', timestamp=1231353866, timezone=0)) # 2009
+            t.commit('mods', timestamp=1231353866, timezone=0))  # 2009
         return t, rev_ids
 
     def make_tree_with_dirs_modified(self):
@@ -160,12 +173,24 @@
         self.assertFileEqual('Copyright (c) 2008, 2009, %d Foo Bar\n'
                              'different content\n' % (year,), 'file')
 
+    def test_update_copyright_with_ranges(self):
+        t, rev_ids = self.make_old_modified_tree()
+        bt = t.basis_tree()
+        bt.lock_read()
+        self.addCleanup(bt.unlock)
+        self.assertEqual(
+            'updated', update_copyright.update_copyright(
+                t, bt, 'file', False, update_copyright.format_years_as_range))
+        year = datetime.datetime.now().year
+        self.assertFileEqual('Copyright (c) 2008-%d Foo Bar\n'
+                             'different content\n' % (year,), 'file')
+
     def test_update_copyright_not_first_line(self):
         t, rev_ids = self.make_old_modified_tree()
-        self.build_tree_contents([('file',
-            '\n\n\nCopyright (c) 2008 Foo Bar\nultimate content\n')])
+        self.build_tree_contents([
+            ('file', '\n\n\nCopyright (c) 2008 Foo Bar\nultimate content\n')])
         rev_ids.append(
-            t.commit('remod file', timestamp=1231353864, timezone=0)) # 2010
+            t.commit('remod file', timestamp=1231353864, timezone=0))  # 2010
         bt = t.basis_tree()
         bt.lock_read()
         self.addCleanup(bt.unlock)
@@ -178,10 +203,11 @@
 
     def test_update_copyright_after_max_line(self):
         t, rev_ids = self.make_old_modified_tree()
-        self.build_tree_contents([('file',
-            '\n\n\n\n\nCopyright (c) 2008 Foo Bar\nultimate content\n')])
+        self.build_tree_contents([
+            ('file',
+             '\n\n\n\n\nCopyright (c) 2008 Foo Bar\nultimate content\n')])
         rev_ids.append(
-            t.commit('remod file', timestamp=1231353864, timezone=0)) # 2010
+            t.commit('remod file', timestamp=1231353864, timezone=0))  # 2010
         bt = t.basis_tree()
         bt.lock_read()
         self.addCleanup(bt.unlock)
@@ -280,9 +306,19 @@
         self.assertFileEqual('Copyright 2008 Bar\n'
                              'content\n', 'dir/subdir/foo')
 
+    def check_tree_with_dirs_modified_using_ranges(self):
+        year = datetime.datetime.now().year
+        self.assertFileEqual('Copyright (c) 2008 Foo Bar\n'
+                             'different content\n', 'file')
+        self.assertFileEqual('Copyright 2008-%d Foo\r\n'
+                             'modified content\r\n' % (year,), 'dir/file')
+        self.assertFileEqual('Copyright 2008 Bar\n'
+                             'content\n', 'dir/subdir/foo')
+
     def test_update_tree_only_changed(self):
         t, rev_ids = self.make_tree_with_dirs_modified()
-        res = update_copyright.update_tree_copyright(t, None, only_changed=True)
+        res = update_copyright.update_tree_copyright(
+            t, None, only_changed=True)
         self.check_tree_with_dirs_modified()
         self.assertEqual({'count': 1, 'updated': 1, 'no-copyright': 0,
                           'copyright-correct': 0, 'not-versioned': 0},
@@ -292,7 +328,8 @@
         t, rev_ids = self.make_tree_with_dirs_modified()
         self.build_tree_contents([('file2', 'Copyright 1700 Fooz\ncontent\n')])
         t.add(['file2'])
-        res = update_copyright.update_tree_copyright(t, None, only_changed=True)
+        res = update_copyright.update_tree_copyright(
+            t, None, only_changed=True)
         year = datetime.datetime.now().year
         self.check_tree_with_dirs_modified()
         self.assertEqual({'count': 2, 'updated': 2, 'no-copyright': 0,
@@ -307,7 +344,8 @@
         # modified versus the committed content
         self.build_tree_contents([
             ('dir/file', 'Copyright 2008, 2009 Foo\r\nmodified content\r\n')])
-        res = update_copyright.update_tree_copyright(t, None, only_changed=True)
+        res = update_copyright.update_tree_copyright(
+            t, None, only_changed=True)
         self.check_tree_with_dirs_modified()
         self.assertEqual({'count': 1, 'updated': 1, 'no-copyright': 0,
                           'copyright-correct': 0, 'not-versioned': 0},
@@ -317,8 +355,9 @@
         t, rev_ids = self.make_tree_with_dirs_modified()
         # We have to install it here, because the test suite clears out hooks
         # as part of its isolation mechanism.
-        mutabletree.MutableTree.hooks.install_named_hook('start_commit',
-            _update_copyright_start_commit_hook, 'update-copyright')
+        mutabletree.MutableTree.hooks.install_named_hook(
+            'start_commit', _update_copyright_start_commit_hook,
+            'update-copyright')
         # Without any config, nothing should be modified.
         t.commit('Modified content')
         self.assertFileEqual('Copyright (c) 2008 Foo Bar\n'
@@ -332,8 +371,9 @@
         t, rev_ids = self.make_tree_with_dirs_modified()
         # We have to install it here, because the test suite clears out hooks
         # as part of its isolation mechanism.
-        mutabletree.MutableTree.hooks.install_named_hook('start_commit',
-            _update_copyright_start_commit_hook, 'update-copyright')
+        mutabletree.MutableTree.hooks.install_named_hook(
+            'start_commit', _update_copyright_start_commit_hook,
+            'update-copyright')
         t.branch.get_config().set_user_option('auto_update_copyright', 'True')
         t.commit('Modified content')
         # With the hook enabled, commit should update the copyright in the
@@ -353,6 +393,19 @@
         self.assertFileEqual('Copyright (c) 2008, 2009, %d Foo Bar\n'
                              'different content\n' % (year,), 'file')
 
+    def test_blackbox_force_range(self):
+        t, rev_ids = self.make_old_modified_tree()
+        # We need to unlock so that run_bzr can grab a lock
+        t.unlock()
+        try:
+            self.run_bzr('update-copyright --force-range file')
+        finally:
+            # leave the tree locked, because we have pending cleanup
+            t.lock_read()
+        year = datetime.datetime.now().year
+        self.assertFileEqual('Copyright (c) 2008-%d Foo Bar\n'
+                             'different content\n' % (year,), 'file')
+
     def test_blackbox_only_changed(self):
         t, rev_ids = self.make_tree_with_dirs_modified()
         t.unlock()
@@ -363,10 +416,21 @@
             t.lock_read()
         self.check_tree_with_dirs_modified()
 
+    def test_blackbox_only_changed_force_range(self):
+        t, rev_ids = self.make_tree_with_dirs_modified()
+        t.unlock()
+        try:
+            self.run_bzr('update-copyright --only-changed --force-range')
+        finally:
+            # leave the tree locked, because we have pending cleanup
+            t.lock_read()
+        self.check_tree_with_dirs_modified_using_ranges()
+
     def test_blackbox_with_commit_hook(self):
         t, rev_ids = self.make_tree_with_dirs_modified()
-        mutabletree.MutableTree.hooks.install_named_hook('start_commit',
-            _update_copyright_start_commit_hook, 'update-copyright')
+        mutabletree.MutableTree.hooks.install_named_hook(
+            'start_commit', _update_copyright_start_commit_hook,
+            'update-copyright')
         t.branch.get_config().set_user_option('auto_update_copyright', 'True')
         t.unlock()
         try:

=== modified file 'update_copyright.py'
--- update_copyright.py	2014-04-10 08:10:39 +0000
+++ update_copyright.py	2015-05-07 09:22:34 +0000
@@ -13,12 +13,11 @@
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
-# 
+#
 
 """Tools for updating the copyright of given files."""
 
-import cStringIO
-import datetime
+from datetime import datetime
 import re
 
 from bzrlib import (
@@ -39,13 +38,12 @@
     """Determine the dates a given file was modified."""
     tip = (file_id, last_rev_id)
     ancestry = graph.Graph(repository.texts).iter_ancestry([tip])
-    revision_ids = set([k[1] for k,p in ancestry if p is not None])
+    revision_ids = set(k[1] for k, p in ancestry if p is not None)
     # Oddly enough, caching the Revision object *slows* it down. My guess is
     # that it interacts poorly with gc
     revisions = repository.get_revisions(revision_ids)
     # TODO: Should we use utcfromtimestamp?
-    years = set([datetime.datetime.fromtimestamp(r.timestamp).year
-                 for r in revisions])
+    years = set(datetime.fromtimestamp(r.timestamp).year for r in revisions)
     return years
 
 
@@ -76,6 +74,14 @@
     return ', '.join(out_years)
 
 
+def format_years_as_range(years):
+    """Format a bunch of years back into a date range string."""
+    if len(years) == 1:
+        return '%d' % years.pop()
+    else:
+        return '%d-%d' % (min(years), max(years))
+
+
 def _iter_all_files(tree, relpaths, basis_tree):
     file_ids = tree.paths2ids(relpaths, trees=[basis_tree])
     for path, ie in tree.iter_entries_by_dir(file_ids):
@@ -90,18 +96,25 @@
             yield path[1]
 
 
-def update_tree_copyright(tree, relpaths, only_changed=False):
+def update_tree_copyright(
+        tree, relpaths, only_changed=False, force_range=False):
     """Update all of the paths specified in the tree."""
-    result = {'not-versioned': 0,
-              'no-copyright': 0,
-              'copyright-correct': 0,
-              'updated': 0,
-              'count': 0,
-             }
+    if force_range:
+        fmt = format_years_as_range
+    else:
+        fmt = format_years
+
+    result = {
+        'not-versioned': 0,
+        'no-copyright': 0,
+        'copyright-correct': 0,
+        'updated': 0,
+        'count': 0,
+    }
     # We only *need* lock_tree_write, however the
     # WorkingTree.put_file_bytes_non_atomic api uses a @needs_lock_write
     # decorator, which means it tries to write lock the branch...
-    #tree.lock_tree_write()
+    # tree.lock_tree_write()
     tree.lock_write()
     try:
         basis_tree = tree.basis_tree()
@@ -117,8 +130,9 @@
                     result['count'] += 1
                     # If we are iterating changed files only, then we know this
                     # file is changed
-                    action = update_copyright(tree, basis_tree, path,
-                                              is_changed=only_changed)
+                    action = update_copyright(
+                        tree, basis_tree, path, is_changed=only_changed,
+                        format_years=fmt)
                     result[action] += 1
                     pb.update('%s %s' % (action, path), result[action],
                               result['count'])
@@ -131,17 +145,21 @@
     return result
 
 
-def update_copyright(tree, basis_tree, filename, is_changed=False):
+def update_copyright(
+        tree, basis_tree, filename, is_changed=False,
+        format_years=format_years):
     """Update the copyright line for a given file.
-    
+
     :param is_changed: If we know the file is changed, then we know it must
         have this years date in the Copyright header. If it is unknown if the
         file has changed, just pass False.
+    :param format_years: The function to use to format a set of years into a
+        string for inclusion in the copyright header.
     """
     file_id = tree.path2id(filename)
     if file_id is None:
         return 'not-versioned'
-    this_year = datetime.datetime.now().year
+    this_year = datetime.now().year
     f = tree.get_file(file_id, path=filename)
     max_lines = 5
     cur_line = 0