human race between Code-Review+2 and new patchset

[Daniel Martí]

Hi all,

I'm a maintainer in the CUE project, where we use review.gerrithub.io to
review and merge all changes. I've been a big fan of Gerrit for years
thanks to Go, and also loving the recent v3.6+ changes :)

Paul Jolly and I were recently investigating a weird issue that we ran
into. For some context, our Code-Review label is configured as follows:

[label "Code-Review"]
function = NoOp
defaultValue = 0
copyCondition = is:ANY
value = 0 No score
value = +1 Looks good to me, but someone else must approve
value = +2 Looks good to me, approved

Note that its value is always copied between patchsets. If I post a CR+2
on PS1 and the owner posts a PS2, the label stays there.

We managed to reproduce the issue in https://review.gerrithub.io/c/cue-lang/cue/+/543082.
The timeline of events, if you ignore PS1, was:

1. Owner (Paul) uploads PS2
2. Reviewer (Daniel) writes a draft comment, hits "Reply", clicks on
CR+2, but leaves the browser page open without clicking "Send".
3. Owner uploads PS3
4. Reviewer clicks "Send" (without refreshing the page).

Note that I was on the URL above, without viewing a specific patchset,
so the page was showing me the latest patchset (PS2 at the start).

The result, as you can see on the Change Log events at the bottom, is:

Paul Jolly: Uploaded patch set 3. -- Patchset 3
Daniel Martí: Code-Review+2 [1 comment] -- Patchset 2

That is, my review was posted on PS2 rather than PS3. Understandable,
given that PS3 didn't exist at the time that I wrote the review.
However, it's confusing that the CL ends up without any Code-Review+2
label at all, given that we have the copyCondition above.

In other words, if the timeline of events were changed so that I hit
"Send" before Paul uploaded PS3, then the copyCondition would have
carried over the label normally.

We're not sure whether this is a bug or intended behavior, but at least
we've convinced ourselves that this is confusing. In my mind, either of
these two behaviors would have been reasonable:

A. copyCondition does its job in our timeline of events, so CL 543082
above ends up with a Code-Review+2 on PS3.

B. Gerrit notices the race between two humans in our timeline of events,
and when I click "Send" as the last step, it shows me a warning
urging me to refresh the page as a new patchset has been uploaded.
Then I can review the new changes before sending my review.

Any thoughts or input much appreciated :)

[Edwin Kempin]

This is a known issue, that I have fixed here:

https://gerrit-review.googlesource.com/c/gerrit/+/337968

This fix is not part of any release yet.

 

In other words, if the timeline of events were changed so that I hit
"Send" before Paul uploaded PS3, then the copyCondition would have
carried over the label normally.

We're not sure whether this is a bug or intended behavior, but at least
we've convinced ourselves that this is confusing. In my mind, either of
these two behaviors would have been reasonable:

A. copyCondition does its job in our timeline of events, so CL 543082
   above ends up with a Code-Review+2 on PS3.

B. Gerrit notices the race between two humans in our timeline of events,
   and when I click "Send" as the last step, it shows me a warning
   urging me to refresh the page as a new patchset has been uploaded.
   Then I can review the new changes before sending my review.

Any thoughts or input much appreciated :)

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/repo-discuss/20220909092256.5oqaigmnfa6saz6e%40p14s.localdomain.

[Daniel Martí]

> > That is, my review was posted on PS2 rather than PS3. Understandable,
> > given that PS3 didn't exist at the time that I wrote the review.
> > However, it's confusing that the CL ends up without any Code-Review+2
> > label at all, given that we have the copyCondition above.
> >
>
> This is a known issue, that I have fixed here:
> https://gerrit-review.googlesource.com/c/gerrit/+/337968
>
> This fix is not part of any release yet.

Oh wow, that was quick! Thanks so much :) Looking forward to a release.