[MERGE] New option --author for commit

[Lukáš Lalinský]

On Ut, 2007-08-07 at 00:54 -0400, Aaron Bentley wrote:
> 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.)

I was thinking about a different usage of this. In case of PQM, I think
PQM should be the author, because it's just a merge of another revisions
which are already properly attributed to their authors.

> > 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?

I'd like this as well (and do it this way in QBzr), but I thought I'd
wait for more comments before changing things like the short log or
annotate in bzrlib.

> > === 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>"'

Yes, it should be in the same format as the committer-id, because it
might be useful to be able to to something like this:

  author = rev.properties.get('author', rev.committer)

> > +        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.

Moved.

> 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.

Removed.

> > === 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 +.

I've removed the try/except, hopefully it's better now.

> > +    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.

Yep, I've blindly followed the tests for 'commit --fixes'. Changed.

If this patch will be approved, I'll change also the other tests that
use this.

On Ut, 2007-08-07 at 15:43 +1000, Martin Pool wrote:
> In addition to Aaron's comments, I think we should add some developer
> documentation about well-known or standardized property names. Maybe
> we should also add this to the user documentation.
> 
I've added the developer docs about revision properties to
doc/developer/revision-properties.txt, please review my Slovenglish! :)

But I'm not sure which user documentation do you mean. It is already in
'bzr help commit', which, if I'm not mistaked, is also used to generate
the HTML docs.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr-commit-author.diff
Type: text/x-patch
Size: 17663 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070807/90ee90fb/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Toto je =?ISO-8859-1?Q?digit=E1lne?=
	=?ISO-8859-1?Q?_podp=EDsan=E1?= =?UTF-8?Q?_=C4=8Das=C5=A5?=
	=?ISO-8859-1?Q?_spr=E1vy?=
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070807/90ee90fb/attachment-0001.pgp