[MERGE][#215334] Show author information on commit

[Aaron Bentley]

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

Martin Albisetti wrote:
> This partially addresses the bug I opened
> (https://bugs.launchpad.net/bzr/+bug/215334), and I'm splitting the
> work into to two as there might be some discussion on each item
> separately.

Hi Martin,

Showing a message like this is fine in principle, but we need to work a
bit on the implementation.

I would suggest that:
1. we determine the committer in the body of Commit.commit
2. we supply that committer to CommitBuilder instead of letting it
   choose.
3. we implement a new set_committer method on NullCommitReporter
4. we change ReportCommitToLog.started to use the committer specified by
   set_committer (if any).
5. we make Commit.commit() invoke self.report.set_committer before
   self.started().

There also seems to be some confusion here about what's being displayed.
 The committer is not the same thing as the author.  Both committer and
author may be specified.  The author is the person who wrote the code.
The committer is the person who committed it to Bazaar.  They are
frequently the same.  But the --author flag allows the commiter to
specify the real author.

Also, your NEWS entry should include the bug number.

There are some problems with your approach.  You're duplicating code by
trying to determine the committer in ReportCommitToLog.started.  Code
duplication can lead to unintentional behavior differences.  The
CommitBuilder has determined what the committer will be, and if you want
to show what the committer will be, you need to use the same data.

For example, Commit.commit allows the committer to be supplied.  If it
is, that would cause your function to give incorrect information.

As another example, Commit.commit requires a configuration to be
supplied, and if the committer is not specified, CommitBuilder uses that
configuration to determine the committer.  Since your function doesn't
use that configuration, this may also cause it to be wrong.

We don't really want to dig information out of the CommitBuilder,
especially since it may be provided by bzr-svn or some other
ForeignBranch implementation.  But I don't see any need for
CommitBuilder to determine the committer at all, which is why I suggest
doing it in Commit.commit.

If we were to use your function, we would use Branch.open, not
Branch.open_containing.  The location supplied is supposed to be the
exact location of the branch, as determined by Bazaar.  If no branch is
available there, this is a critical error.  Using open_containing could
cause us to open a containing branch, and that would mask the problem,
possibly providing the wrong information.  Similarly, it would be wrong
to fall back to GlobalConfig.

Finally, existing objects should always be reused.  This reduces network
lag, and also authentication prompts (e.g. over ftp).  There are also
caching issues; the current copy of the branch is write-locked, and so
its information could be newer than the copy on disk.

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

iD8DBQFH/5J30F+nu1YWqI0RAg/mAJ9pEeCmdQjTbEJbdse6apo8BP2PiACcCnVe
PcOUT1cm16FYDkAVXOW/EV4=
=ZPdE
-----END PGP SIGNATURE-----