Bug 64127 – Committer for r90588 is [e-mail], but should have been [e-mail]

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

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 [e-mail]. But since Adam Barth is a committer, and he is the author of the patch, he should be listed as the committer instead.

[Adam Roben (:aroben)]

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 ([e-mail]) and committer ([e-mail]) are not the same, inserts a "Patch by Adam Barth <[e-mail]>" 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 [e-mail] 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).

[Adam Roben (:aroben)]

Adam Barth discovered the following:

http://queues.webkit.org/results/8995536

Failed to determine email address for ChangeLog.
Either:
  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'

[Adam Roben (:aroben)]

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?

[Adam Roben (:aroben)]

Whoops, comment 2 and comment 3 were actually about the attachment 100028 [details], not about the patch this bug is about. Please disregard.

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

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 [e-mail]. 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!

[Adam Roben (:aroben)]

Ah, here's the answer:

http://trac.webkit.org/browser/trunk/Tools/Scripts/commit-log-editor?rev=90439#L182

The "Patch by" line is only added if the patch contains "Reviewed by" or "Rubber-stamped by"! Rollout patches do not contain those words.

[Adam Roben (:aroben)]

We just need to decide where to insert "Patch by" when there's no "Reviewed by" line.

[Adam Roben (:aroben)]

I'll look into this tomorrow. Other people should feel free to take a crack at it.

[Adam Roben (:aroben)]

Created attachment 100117 [details]
Ensure commit-log-editor adds a "Patch by" line when the author and committer are different

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.