search

sdefresne / gerrit-monitor

Source for Gerrit Monitor Chrome extension
Apache License 2.0
21 stars 16 forks source link

Change set marked as stale if reviewer assigned but no message from author #3

Closed PeteGillin closed 5 years ago

PeteGillin commented 5 years ago

It seems like, whenever I upload a change to gerrit and add a reviewer, gerrit-monitor immediately considers it to be stale. For example, it is currently showing that http://r.android.com/889173, http://r.android.com/889174, and http://r.android.com/889154 are stale, even though I uploaded them less than 20 minutes ago, and I am waiting for the reviewer's first response.

(There was a fourth change, http://r.android.com/889153, which I uploaded at the same time which is not showing as stale, presumably either because I 'said something nice' when I added the reviewers, or because I commented myself since. That's not the normal workflow, at least for me, though.)

sdefresne commented 5 years ago

For Chromium, the workflow is to send a message to ask for review (i.e. adding reviewers is not enough to request review). This is why the CLs are marked as stale (because there are no comments from you, thus the extension does not see that you've sent a review).

So, this is working as intended (as far as it correspond to the workflow I had in mind when implementing this). However, I don't think it would break my workflow if I changed the extension to remove from pending CLs that have an assigned reviewer, even if there are no message from author (worse case, it would appear stale 24h later if not reviewed).

I'll try to see how I can change this behaviour when I have some time.

PeteGillin commented 5 years ago

Thanks for the reply. I can't speak for the whole of Android (or even for the parts of Android that work mostl on AOSP) but on my team, at least, adding reviewers without comment seems to be pretty normal, so the change you suggest would be beneficial to us.

By the way, thanks for writing the tool! I have been recommending it to my team-mates as I don't think it's widely known in Android and I've found it useful.

sdefresne commented 5 years ago

I found some time to take a look at this and finally found a way to reproduce. It was tricky because when using the web version of gerrit used by Chromium, if no message is entered when assigning a reviewer, then a default message is generated.

Using git cl upload --reviewer xxx@example.com though allow to assign a reviewer without sending a message, and the CL shows up as stale. The function isStale() ignores auto-generated messages but the CL only has one message which is auto-generated.

The simplest fix would be to consider auto-generated messages if they are all auto-generated.

sdefresne commented 5 years ago

This should be fixed in version 1.0.4 (which should be deployed soon).