Bug 64127 - Committer for r90588 is commit-queue@webkit.org, but should have been abarth@webkit.org
Summary: Committer for r90588 is commit-queue@webkit.org, but should have been abarth@...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://trac.webkit.org/changeset/90588
Depends on:
Reported: 2011-07-07 14:33 PDT by Adam Roben (:aroben)
Modified: 2011-07-08 08:22 PDT (History)
3 users (show)

See Also:

Ensure commit-log-editor adds a "Patch by" line when the author and committer are different (4.12 KB, patch)
2011-07-08 07:30 PDT, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-07-07 14:33:37 PDT
The set up:

1) sheriffbot was running locally as abarth on one of his machines, due to EC2 connectivity issues
2) ojan requested a rollout of r90581, resulting in bug 64124
3) The patch attached to bug 64124 said that abarth was the author (see (1))
4) The commit-queue committed the patch as http://trac.webkit.org/changeset/90588

The bug:

The committer listed in Subversion history for r90588 is commit-queue@webkit.org. But since Adam Barth is a committer, and he is the author of the patch, he should be listed as the committer instead.
Comment 1 Adam Roben (:aroben) 2011-07-07 14:35:55 PDT
Here's how this is supposed to work:

1) When the commit-queue commits the patch, it runs commit-log-editor to generate the commit message. (This happens in the bowels of webkitpy.)
2) commit-log-editor, upon seeing that the patch author (abarth@webkit.org) and committer (commit-queue@webkit.org) are not the same, inserts a "Patch by Adam Barth <abarth@webkit.org>" line into the commit message
3) When the commit occurs, a script on svn.webkit.org scans the commit message, sees the "Patch by" line, removes it, and sets abarth@webkit.org as the committer for that change.

Something went wrong in this sequence. We already have confirmation from Bill Siegrist that the original commit message did not include the "Patch by" line. So the bug is not with (3). It might be with (1) or (2). Most likely (2).
Comment 2 Adam Roben (:aroben) 2011-07-07 14:45:40 PDT
Adam Barth discovered the following:


Failed to determine email address for ChangeLog.
  set CHANGE_LOG_EMAIL_ADDRESS in your environment
  OR pass --email= on the command line
  OR set EMAIL_ADDRESS in your environment
  OR git users can set 'git config user.email'
Comment 3 Adam Roben (:aroben) 2011-07-07 14:48:38 PDT
In theory, that error case will cause commit-log-editor to exit: http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm?rev=90439#L1773

I wonder how we're ending up with any commit message at all in that case?
Comment 4 Adam Roben (:aroben) 2011-07-07 14:49:48 PDT
Whoops, comment 2 and comment 3 were actually about the attachment 100028 [details], not about the patch this bug is about. Please disregard.
Comment 5 Adam Roben (:aroben) 2011-07-07 14:52:22 PDT
But the fact that the commit queue was able to land the patch in question means the machine that landed it must have one of those environment variables set, or the user.email config variable.
Comment 6 Adam Roben (:aroben) 2011-07-07 15:02:41 PDT
The machine that landed the patch in question was ec2-cq-01. Adam Barth just discovered that it had its user.email variable set to commit-queue@webkit.org. So that explains how it was able to land the patch without commit-log-editor failing. But it still doesn't explain why there wasn't a "Patch by" line in the commit message!
Comment 7 Adam Roben (:aroben) 2011-07-07 15:04:45 PDT
Ah, here's the answer:


The "Patch by" line is only added if the patch contains "Reviewed by" or "Rubber-stamped by"! Rollout patches do not contain those words.
Comment 8 Adam Roben (:aroben) 2011-07-07 15:17:13 PDT
We just need to decide where to insert "Patch by" when there's no "Reviewed by" line.
Comment 9 Adam Roben (:aroben) 2011-07-07 15:25:15 PDT
I'll look into this tomorrow. Other people should feel free to take a crack at it.
Comment 10 Adam Roben (:aroben) 2011-07-08 07:30:32 PDT
Created attachment 100117 [details]
Ensure commit-log-editor adds a "Patch by" line when the author and committer are different
Comment 11 WebKit Review Bot 2011-07-08 08:22:47 PDT
Comment on attachment 100117 [details]
Ensure commit-log-editor adds a "Patch by" line when the author and committer are different

Clearing flags on attachment: 100117

Committed r90632: <http://trac.webkit.org/changeset/90632>
Comment 12 WebKit Review Bot 2011-07-08 08:22:52 PDT
All reviewed patches have been landed.  Closing bug.