Bug 135104 – Commit Queue clears the review flag, making it hard to tell whether a patch was reviewed and by whom

• Status: NEW
• Component: Tools / Tests

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).

[Csaba Osztrogonác]

(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?

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.

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." ?

[Alexey Proskuryakov]

When landing manually, I never remove the review+ flag. Never observed any problems caused by this.

Rollouts and having multiple patches in one bug already confuse the tools, so I don't think that these make a very strong argument. I'd like to respond to these in more detail though:

1. Rollout.

I think that when the bug is re-opened due to rollout, it's even more important to have review history intact. It's essentially the same as if a problem was discovered manually, and the patch was never landed.

2. r+ with comments.

Not sure what the concern here is. Uploading a new patch obsoletes the old one.

3. Multiple patches.

I think that the only issue scripts would have with that is that the strange logic to not close the bug with more than one r+ed patch would be broken. This seems acceptable to me, assuming that commit queue will just close the bugs regardless. If someone really needs multiple fixes in a bug, that's an exceptional situation, and the burden is on them to re-open the bug, or to use appropriate arguments with webkit-patch.

[Darin Adler]

(In reply to comment #2)
> When landing manually, I never remove the review+ flag. Never observed any problems caused by this.

My view exactly; I have thought this since the commit queue was created.

(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.