Lando says a reviewer "reviewed a prior diff" for patches that have landed, even if reviewer actually reviewed the most recent `moz-phab submit`-ed patch
Categories
(Conduit :: Lando, defect)
Tracking
(Not tracked)
People
(Reporter: Gijs, Unassigned)
Details
STR:
- person A uploads patch
- person B reviews
- person B lands patch
- person A looks at lando a while later
- lando says "person B accepted a prior diff"
As is often the case, though Technically Correct, this is kinda the worst kind of correct in this instance - you have to know that the second diff was created when the patch landed.
It would be much more helpful if lando simply said the reviewer accepted the patch, or even positively indicated that the reviewer had definitely reviewed the very last version of the patch before it landed, if we decide that signalling this vs. the "prior diff" bit is so important.
(I don't know how much lando really knows here, so I guess this may be tricky to fix. But the language here confused a first-time lando/phabricator user and so I decided to report it anyway...)
|
||
Comment 1•1 year ago
|
||
(I don't know how much lando really knows here, so I guess this may be tricky to fix. But the language here confused a first-time lando/phabricator user and so I decided to report it anyway...)
Lando does actually know the revision and diff ID that landed. But in some cases (e.g., backouts) a revision that has landed could be landed again under a different diff ID. In that case, the "@someone accepted a prior diff" warning makes more sense. The main difference between this and the case you mentioned (i.e., the changeset/diff that closes the revision) is the status of the revision, which we use in Lando as well to determine whether something can land or not.
It would be much more helpful if lando simply said the reviewer accepted the patch, or even positively indicated that the reviewer had definitely reviewed the very last version of the patch before it landed, if we decide that signalling this vs. the "prior diff" bit is so important.
I agree there is some improvement we can do here in terms of figuring out when we can exclude these warning with the available data (ideally without having to compare diffs).
Description
•