Code review comment for lp:~asac/bzr/lp459276

This looks good to me. I've implemented Ian's review suggestions (added tests, added NEWS entry, and added '.' to the end of the option help). I also fixed it to preserve the timezone specified in the --commit-time.

I'm submitting it to PQM now.

Here's the relevant diff of my changes:

=== modified file 'NEWS'
--- NEWS 2009-11-18 22:03:00 +0000
+++ NEWS 2009-11-19 06:30:25 +0000
@@ -23,6 +23,9 @@
 New Features
 ************

+* ``bzr commit`` now has a ``--commit-time`` option.
+ (Alexander Sack, #459276)
+
 Bug Fixes
 *********

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2009-10-23 18:00:44 +0000
+++ bzrlib/builtins.py 2009-11-19 06:27:16 +0000
@@ -2943,8 +2943,8 @@
                     help="Refuse to commit if there are unknown "
                     "files in the working tree."),
              Option('commit-time', type=str,
- help="Manually set a commit time using commit date format, e.g."
- "'2009-10-10 08:00:00 +0100'"),
+ help="Manually set a commit time using commit date "
+ "format, e.g. '2009-10-10 08:00:00 +0100'."),
              ListOption('fixes', type=str,
                     help="Mark a bug as being fixed by this revision "
                          "(see \"bzr help bugs\")."),
@@ -2996,9 +2996,13 @@
             make_commit_message_template_encoded
         )

- commit_stamp = None
+ commit_stamp = offset = None
         if commit_time is not None:
+ try:
                 commit_stamp, offset = timestamp.parse_patch_date(commit_time)
+ except ValueError, e:
+ raise errors.BzrCommandError(
+ "Could not parse --commit-time: " + str(e))

         # TODO: Need a blackbox test for invoking the external editor; may be
         # slightly problematic to run this cross-platform.
@@ -3058,6 +3062,7 @@
                         allow_pointless=unchanged, strict=strict, local=local,
                         reporter=None, verbose=verbose, revprops=properties,
                         authors=author, timestamp=commit_stamp,
+ timezone=offset,
                         exclude=safe_relpath_files(tree, exclude))
         except PointlessCommit:
             # FIXME: This should really happen before the file is read in;

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py 2009-08-28 05:00:33 +0000
+++ bzrlib/tests/blackbox/test_commit.py 2009-11-19 06:27:16 +0000
@@ -590,6 +590,26 @@
         properties = last_rev.properties
         self.assertEqual('John Doe\nJane Rey', properties['authors'])

+ def test_commit_time(self):
+ tree = self.make_branch_and_tree('tree')
+ self.build_tree(['tree/hello.txt'])
+ tree.add('hello.txt')
+ out, err = self.run_bzr("commit -m hello "
+ "--commit-time='2009-10-10 08:00:00 +0100' tree/hello.txt")
+ last_rev = tree.branch.repository.get_revision(tree.last_revision())
+ self.assertEqual(
+ 'Sat 2009-10-10 08:00:00 +0100',
+ osutils.format_date(last_rev.timestamp, last_rev.timezone))
+
+ def test_commit_time_bad_time(self):
+ tree = self.make_branch_and_tree('tree')
+ self.build_tree(['tree/hello.txt'])
+ tree.add('hello.txt')
+ out, err = self.run_bzr("commit -m hello "
+ "--commit-time='NOT A TIME' tree/hello.txt", retcode=3)
+ self.assertStartsWith(
+ err, "bzr: ERROR: Could not parse --commit-time:")
+
     def test_partial_commit_with_renames_in_tree(self):
         # this test illustrates bug #140419
         t = self.make_branch_and_tree('.')

Thanks very much for the code!