[MERGE] New option --author for commit

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Lukáš Lalinský wrote:
> Here is a short patch that adds a new option --author to 'bzr commit'
> and stores the author name in the revision property 'author'.

!resubmit

I think that's a reasonable idea.  We may want "submitter" at some point
in the future.  (If I approve your patch, PQM will be the committer, I
will be the submitter, you will be the author.)

> If the
> property is set, it is also displayed in the log (only in the long
> format).

I think I'd rather see author than committer in most displays.  This
would include log --short, and annotate/gannotate output.  What do other
people think?

> === modified file 'bzrlib/builtins.py'
> --- bzrlib/builtins.py	2007-07-31 20:02:47 +0000
> +++ bzrlib/builtins.py	2007-08-04 12:27:04 +0000
> @@ -2107,6 +2107,10 @@
>      committed.  If a directory is specified then the directory and everything 
>      within it is committed.
>  
> +    If author of the change is not the same person as the committer, you
> +    can specify the author's name using the --author option. The name should
> +    be in format "John Doe <jdoe at example.com>".

^^^ We want this to be in exactly the same format as the committer-id,
right?  Let's say that, e.g. 'The name should be in the same format as a
committer-id, e.g. "John Doe <jdoe at example.com>"'

> @@ -2185,7 +2192,8 @@
>          return '\n'.join(properties)
>  
>      def run(self, message=None, file=None, verbose=True, selected_list=None,
> -            unchanged=False, strict=False, local=False, fixes=None):
> +            unchanged=False, strict=False, local=False, fixes=None,
> +            author=None):
>          from bzrlib.commit import (NullCommitReporter, ReportCommitToLog)
>          from bzrlib.errors import (PointlessCommit, ConflictsInTree,
>                  StrictCommitFailed)
> @@ -2211,6 +2219,14 @@
>          if bug_property:
>              properties['bugs'] = bug_property
>  
> +        if author:
> +            try:
> +                config.extract_email_address(author)
> +            except errors.NoEmailInUsername, e:
> +                warning('"%s" does not seem to contain an email address.  '
> +                        'This is allowed, but not recommended.', author)
> +            properties['author'] = author

I think handling this within builtins is not a good place, because it's
not reusable (the same applies for the bugs property).  It makes it hard
for other library clients to do their own commits.  I suggest doing it
in MutableTree.commit, like branch-nick.

Also, I think that this warning about author format could quickly become
irritating when the author doesn't want their email address publicized.
 I suggest removing this warning.

> === modified file 'bzrlib/log.py'
> --- bzrlib/log.py	2007-07-19 03:40:54 +0000
> +++ bzrlib/log.py	2007-08-04 12:27:04 +0000
> @@ -633,6 +633,11 @@
>              for parent_id in revision.rev.parent_ids:
>                  print >>to_file, indent+'parent:', parent_id
>          print >>to_file, indent+'committer:', revision.rev.committer
> +        try:
> +            print >>to_file, indent+'author: %s' % \
> +                revision.rev.properties['author']
> +        except KeyError:
> +            pass

I am not comfortable with sticking so much inside a try/except, when
you're catching a very generic exception.  Also, we need a space on
either side of the +.

> +    def test_author_sets_property(self):
> +        """commit --author='John Doe <jdoe at example.com>' sets the author revprop."""
> +        tree = self.make_branch_and_tree('tree')
> +        self.build_tree(['tree/hello.txt'])
> +        tree.add('hello.txt')
> +        self.run_bzr("commit -m hello --author='John Doe <jdoe at example.com>' tree/hello.txt")
> +        last_rev = tree.branch.repository.get_revision(tree.last_revision())
> +        properties = dict(last_rev.properties)
> +        del properties['branch-nick']
> +        self.assertEqual({'author': 'John Doe <jdoe at example.com>'},
> +                         properties)

I don't think it makes sense to delete the branch-nick property and then
make assertions about the whole set of revision properties.  That makes
it very brittle.  As soon as a new default revision property is created,
your test will fail.  I think you should just test properties['author],
not the whole properties dict.

> +
> +    def test_author_no_email(self):
> +        """multiple --author options are not allowed"""

^^^ docstring looks wrong.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGt/r+0F+nu1YWqI0RAvRnAJsHgzBVyW4WZJx53N0KDMIIT9LeNQCfbC28
lxy1r3qu5970Q3M3RzGPEA0=
=qovf
-----END PGP SIGNATURE-----