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 :) }}} -- https://code.launchpad.net/~jtv/launchpad/bug-643345/+merge/36258 Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-643345 into lp:launchpad/devel. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
