[Webkit-unassigned] [Bug 135104] Commit Queue clears the review flag, making it hard to tell whether a patch was reviewed and by whom

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135104





--- Comment #4 from mitz at webkit.org <mitz at webkit.org>  2014-07-21 17:10:19 PST ---
(In reply to comment #1)
> (In reply to comment #0)
> > When Commit Queue commits a patch, it clears the r+ flag from the patch. Then to see which patches in a bug were reviewed and by whom, one must (a) find the patch’s attachment ID (b) navigate to the bug’s History and (c) look for history entries related to the attachment ID found in step (a).
> 
> I can't see the conrete issue you would like to solve here. 
> Could you specify the details please?

I thought I made it quite clear. It takes information from the bug page and hides it in history.

> If one follows the normal development work-flow, it shouldn't be a problem.
> There is only one patch in one bug report (and maybe more r- patches until
> the final r+ ) If the commit queue or somebody else lands the final r+ -ed
> patch with "webkit-patch land /land-from-bug / land-attachemnt", the name
> of the reviewer is added to the changelog and commit log too. In this case
> there is no need to check the bug's history to determine who was the reviewer.

Neither the change log nor the commit message appear on the bug page.

> If we leave the r+ flags untouched, it can cause many problems in the future:
> - After a rollout the bug will be reopened. In this case the remaining r+
> is confusing, because this patch shouldn't be landed again and it appears
> on http://webkit.org/pending-review , but it shouldn't.
> - After r+ with comments, folks usually do two different things:
> --- Land manually after review comments addressed.
> --- Upload a patch with fixes + "Reviewed by X.Y." changelog entry + cq+ flag
> - Sometimes folks upload different patches to one bug report. In this
> case webkit-patch won't be able to handle more r+ flags properly.
> 
> I can imagine an improvement which can make folks and scripts happy too.
> What do you think if CQ landed a patch, it would remove the r+ flag
> and change the "patch" / "proposed patch" / ... description to
> "landed in r123456, r=X.Y." ?

Like Alexey said, many humans (including him, Darin and myself) don’t clear the r+ when committing a patch, and it’s never seemed to cause any problems. If, however, that’s creating some situation that the robots can’t handle, then that’ll better be solved by fixing the robots than by changing the humans’ behavior.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.