Review status on patches does not carry over correctly on repush



2 years ago
2 years ago


(Reporter: gcp, Unassigned)






2 years ago

In this bug, at some point (indicated above) the patches all had an r+, but a bug was found in one of them. I fixed it and re-pushed the relevant commit to review.

MozReview put the patch up, but went on to obsolete the other 2 patches in the bug. (OK, this was unexpected, but not unreasonable behavior) It did carry over the r+, which was also not what I wanted as the patch had changed.

I then re-pushed the entire branch. This caused the missing 2 patches to reappear, but missing their r+ flags, unlike the first one. So this behavior is actually the exact opposite of what should have happened: there's no reason to not carry forward r+ on patches that didn't change.

Trying to fix this manually by putting r=jld gives the following message:

morbo@mozwell:~/hg/firefox$ hg push  review
pushing to ssh://
searching for appropriate review repository
redirecting push to ssh://
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 3 changesets with 0 changes to 4 files (+1 heads)
remote: recorded push in pushlog
submitting 3 changesets for review
commit message for dc6952696aea has r=jld but they have not granted a ship-it. review will be requested on your behalf <<<<<<<<<<<<<<<<<<<<<
commit message for 2d22e4e45cd1 has r=jld but they have not granted a ship-it. review will be requested on your behalf <<<<<<<<<<<<<<<<<<<<<

changeset:  331250:4a9a13ff4603
summary:    Bug 1273852 - Always add seccomp-bpf socketcall dispatcher. r?jld
review: (draft)

changeset:  331251:dc6952696aea
summary:    Bug 1273852 - Allow getsockopt in EvaluateSocketCall. r=jld
review: (draft)

changeset:  331252:2d22e4e45cd1
summary:    Bug 1273852 - Update chromium's list of linux-x86-32 syscalls. r=jld
review: (draft)

review id:  bz://1273852/gcp
review url: (draft)

Even if this case might be difficult to handle because from MozReview's perspective the two patches were gone at some point, it should still recognize that the relevant commit ids *did* have an r+ on them.

Comment 1

2 years ago
I imagine the r+ didn't carry over because the commit was discarded and then revived.  Normally r+s are carried forward (though we want to make that configurable for the case in which you really want to re-request review; see bug 1195661).

I'm not entirely sure if this is a bug or not... but I can see the case for expecting r+ carry forward if a commit was discarded by accident and has no changes after it is revived.  Might be hard to figure this out on the back end, though, but I'll leave this bug open while I think about it, at least.
You need to log in before you can comment on or make changes to this bug.