Code review comment for lp:~jtv/launchpad/bug-643345

I didn't realise Tim was already reviewing this, but here were my notes up until I did:

> === modified file 'lib/lp/code/model/directbranchcommit.py'
> --- lib/lp/code/model/directbranchcommit.py 2010-09-10 14:44:43 +0000
> +++ lib/lp/code/model/directbranchcommit.py 2010-09-22 07:16:44 +0000
> @@ -74,9 +75,13 @@
> `DirectBranchCommit`.
>
> :param db_branch: a Launchpad `Branch` object.
> - :param committer: the `Person` writing to the branch.
> + :param committer: the `Person` writing to the branch. Defaults to
> + the branch owner.
> :param no_race_check: don't check for other commits before committing
> our changes, for use in tests.
> + :param committer_string: Optional description (typically with email
> + address) of the committer for use in bzr. If not given, the
> + `committer`'s email address will be used instead.

It wasn't obvious to me without looking below that this isn't an actual commit
string provided by the committer.

> """
> self.db_branch = db_branch
>
> @@ -85,6 +90,7 @@
> if committer is None:
> committer = db_branch.owner
> self.committer = committer
> + self.committer_string = committer_string
>
> self.no_race_check = no_race_check
>
> @@ -176,6 +182,17 @@
> raise ConcurrentUpdateError(
> "Branch has been changed. Not committing.")
>
> + def getBzrCommitterID(self):
> + """Find the committer id to pass to bzr."""
> + if self.committer_string is not None:
> + return self.committer_string
> + elif self.committer.preferredemail is not None:
> + return format_address_for_person(self.committer)

The bug history shows Aaron changing the title of the bug to:
"Using preferredemail as a public email id is wrong and broken."

If so, should the above elif be included?

> + else:
> + return '"%s" <%s>' % (
> + self.committer.displayname,
> + config.canonical.noreply_from_address)
> +
> def commit(self, commit_message, txn=None):
> """Commit to branch.
>

IRC log:
09:26 < noodles775> jtv: only things I've thought so far are:
09:27 < noodles775> 1) It wasn't obvious to me without reading the code below that committer_string isn't an actual commit string provided (somehow) by the committer. Not sure if there's a better name, or maybe its just me.
09:28 < noodles775> 2) In getBzrCommitterID you're using self.committer.preferredemail, even though abentley's change of the related bug title seems to indicate this is wrong?
09:28 < noodles775> (but I don't know the background)
09:29 < jtv> noodles775: I'd be happy to come up with a better name for 1. As for 2, that falls under the "laying groundwork" part—there must be better fixes, but for now I just want the failures out of the way.
09:30 < noodles775> Yes, but (regarding 2) couldn't you just remove that elif for the moment to get the failures out of the way?
09:30 < thumper> jtv: reviewed
09:30 < jtv> noodles775: well AIUI it's wrong _because_ it's not guaranteed to be present.
09:31 < jtv> Not inherently wrong otherwise that I know of—people on that end made a deliberate decision to use email addresses, and I don't want to controvert them wholesale.
09:31 < jtv> thumper: thanks! I also have noodles775 looking at it just now
09:31 < thumper> jtv: ok
09:31 < noodles775> Ah, I'll just add my review so far as a comment FTR - I didn't realise thumper was/had looked at it.
09:32 < thumper> noodles775: that's fine
09:32 < jtv> thumper: but very happy to have your view on it
09:32 < thumper> I didn't read all the comments after jtv pinged
09:32 < jtv> there were too many :)
}}}