Pushing a single update will obsolete all other

RESOLVED INVALID

Status

defect
RESOLVED INVALID
3 years ago
6 months ago

People

(Reporter: jya, Unassigned)

Tracking

Details

(Reporter)

Description

3 years ago
In bug 1245463, I pushed 5 review requests (P1 to P5)

All got r+

I got some comments about P2 (but still r+), made an update to that commit and pushed that single change.

At this time P1, P3, P4, P5 got removed from the review board, and the attachment obsoleted in bugzilla.

So I re-pushed again, this time P1 to P5 got added to reviewboard, but only P2 had kept the r+.
All others were back as r?, the history that they had been r+ before was lost.
(the email received by the reviewed indicates that it was only notified of an update however, not a full new review)

I personally find that pushing a single update in a block of reviews to obsolete all the others to be counter-intuitive.
Especially as when you re-upload all of the reviews, it shows as having different versions but the diff of the unmodified ones is empty. Makes it hard to tell what has actually changed.

I don't know what the workflow is supposed to be in that case, but it's certainly very different to how I used to do it with patches and bugzilla alone, where I will update a single patch, not all of the,.
2016-02-10 07:15:11,223 - INFO -  - processing BatchReviewRequest for jya
2016-02-10 07:15:11,229 - INFO -  - processing batch submission bz://1245463/jya to gecko with 5 commits
2016-02-10 07:15:11,270 - INFO -  - created squashed review request #34309
2016-02-10 07:15:11,271 - INFO -  - bz://1245463/jya: generating squashed diffset for 34309
2016-02-10 07:15:11,658 - INFO -  - bz://1245463/jya: updated squashed diffset for 34309
2016-02-10 07:15:11,661 - INFO -  - bz://1245463/jya: 0 previous commits; 0 discard on publish; 0 unpublished
2016-02-10 07:15:11,661 - INFO -  - bz://1245463/jya: 0/5 commits mapped exactly
2016-02-10 07:15:11,661 - INFO -  - bz://1245463/jya: 0/5 mapped exactly or to precursors
2016-02-10 07:15:11,662 - INFO -  - bz://1245463/jya: 0/5 mapped after commit ID matching
2016-02-10 07:15:11,662 - INFO -  - bz://1245463/jya: 0 unclaimed review requests
2016-02-10 07:15:11,686 - INFO -  - bz://1245463/jya: created review request 34311 for commit 9daba7b3b45113f94276fdff3536fce68dd7067a
2016-02-10 07:15:13,237 - INFO -  - importing Bugzilla users from query "gerald": gerald13601@juno.com, gerald@hcl.co.in, gsquelart@mozilla.com, gx1202000@yahoo.com, linkin_mrchester@yahoo.com, nfitzgerald@mozilla.com, nickgdolan@hotmail.com
2016-02-10 07:15:13,651 - INFO -  - bz://1245463/jya: created review request 34313 for commit a5292d3f802cdffaf8715910c8bfbe82e14c383c
2016-02-10 07:15:13,961 - INFO -  - bz://1245463/jya: created review request 34315 for commit 1df59c0e738f183a8336d2184cab817188e1d6e1
2016-02-10 07:15:14,172 - INFO -  - bz://1245463/jya: created review request 34317 for commit 811c2f2daacb4829d6897e30d48aca2c7ecfcde9
2016-02-10 07:15:14,424 - INFO -  - bz://1245463/jya: created review request 34319 for commit 4267f75af007a904bfcaae613741afa804f5a0b2
2016-02-10 07:15:14,693 - INFO -  - bz://1245463/jya: 0 unclaimed review requests left over
2016-02-10 07:15:14,765 - INFO -  - bz://1245463/jya: finished processing 34309

2016-02-10 07:58:23,214 - INFO -  - processing BatchReviewRequest for jya
2016-02-10 07:58:23,224 - INFO -  - processing batch submission bz://1245463/jya to gecko with 5 commits
2016-02-10 07:58:23,232 - INFO -  - using squashed review request 34309
2016-02-10 07:58:23,232 - INFO -  - bz://1245463/jya: generating squashed diffset for 34309
2016-02-10 07:58:23,448 - INFO -  - bz://1245463/jya: updated squashed diffset for 34309
2016-02-10 07:58:23,451 - INFO -  - bz://1245463/jya: 1 previous commits; 0 discard on publish; 0 unpublished
2016-02-10 07:58:23,451 - INFO -  - bz://1245463/jya: commit 96b453652419d451e6d614e037f64691d52780de unchanged; using existing request 34313
2016-02-10 07:58:23,459 - INFO -  - bz://1245463/jya: 1/5 commits mapped exactly
2016-02-10 07:58:23,459 - INFO -  - bz://1245463/jya: 1/5 mapped exactly or to precursors
2016-02-10 07:58:23,460 - INFO -  - bz://1245463/jya: 1/5 mapped after commit ID matching
2016-02-10 07:58:23,460 - INFO -  - bz://1245463/jya: 0 unclaimed review requests
2016-02-10 07:58:23,486 - INFO -  - bz://1245463/jya: created review request 34325 for commit 9daba7b3b45113f94276fdff3536fce68dd7067a
2016-02-10 07:58:25,569 - INFO -  - bz://1245463/jya: created review request 34327 for commit cfffbe2c0a3dd8c570be440c0e9e46beb6d2e4c2
2016-02-10 07:58:25,847 - INFO -  - bz://1245463/jya: created review request 34329 for commit f81cdad9e0ffa471bd30e1819c6c2de2c36faa11
2016-02-10 07:58:26,113 - INFO -  - bz://1245463/jya: created review request 34331 for commit e0a5b729f108b16f3a8bada9123991f2072d59f2
2016-02-10 07:58:26,429 - INFO -  - bz://1245463/jya: 0 unclaimed review requests left over
2016-02-10 07:58:26,496 - INFO -  - bz://1245463/jya: finished processing 34309

2016-02-10 08:04:37,879 - INFO -  - r+ from gsquelart@mozilla.com on bug 1245463.

2016-02-10 08:05:12,828 - INFO -  - r+ from gsquelart@mozilla.com on bug 1245463.

2016-02-10 08:06:40,562 - INFO -  - r+ from gsquelart@mozilla.com on bug 1245463.

2016-02-10 08:11:16,093 - INFO -  - processing BatchReviewRequest for jya
2016-02-10 08:11:16,099 - INFO -  - processing batch submission bz://1245463/jya to gecko with 5 commits
2016-02-10 08:11:16,105 - INFO -  - using squashed review request 34309
2016-02-10 08:11:16,105 - INFO -  - bz://1245463/jya: generating squashed diffset for 34309
2016-02-10 08:11:16,289 - INFO -  - bz://1245463/jya: updated squashed diffset for 34309
2016-02-10 08:11:16,292 - INFO -  - bz://1245463/jya: 5 previous commits; 0 discard on publish; 0 unpublished
2016-02-10 08:11:16,292 - INFO -  - bz://1245463/jya: 0/5 commits mapped exactly
2016-02-10 08:11:16,292 - INFO -  - bz://1245463/jya: 0/5 mapped exactly or to precursors
2016-02-10 08:11:16,305 - INFO -  - bz://1245463/jya: commit ID 8A1CMKAUyTq for 63c4b53fe94097686bfbd753eb3ac9bee046c729 found in review request 34325
2016-02-10 08:11:18,260 - INFO -  - bz://1245463/jya: commit ID CHppUOGM1mk for 5b31198acf9e0a5760fdac05ca7b3875ef6a203c found in review request 34313
2016-02-10 08:11:18,632 - INFO -  - bz://1245463/jya: commit ID CXqGoq9Opq0 for 42c8a8021c5edce03d76dcd6b81db40f0cb90384 found in review request 34327
2016-02-10 08:11:18,954 - INFO -  - bz://1245463/jya: commit ID Elnm0WPuqHC for 2b98a0882985e39f975f40e8781b5d32a4c43b11 found in review request 34329
2016-02-10 08:11:19,316 - INFO -  - bz://1245463/jya: commit ID 71hgJ63ksPU for 10580cdd69653585d74db2ab75420c3d884653c1 found in review request 34331
2016-02-10 08:11:19,719 - INFO -  - bz://1245463/jya: 5/5 mapped after commit ID matching
2016-02-10 08:11:19,720 - INFO -  - bz://1245463/jya: 0 unclaimed review requests
2016-02-10 08:11:19,720 - INFO -  - bz://1245463/jya: 0 unclaimed review requests left over
2016-02-10 08:11:19,790 - INFO -  - bz://1245463/jya: finished processing 34309

2016-02-10 08:47:40,825 - INFO -  - processing batch submission bz://1245463/jya to gecko with 5 commits
2016-02-10 08:47:40,830 - INFO -  - using squashed review request 34309
2016-02-10 08:47:40,830 - INFO -  - bz://1245463/jya: generating squashed diffset for 34309
2016-02-10 08:47:40,996 - INFO -  - bz://1245463/jya: updated squashed diffset for 34309
2016-02-10 08:47:40,999 - INFO -  - bz://1245463/jya: 5 previous commits; 0 discard on publish; 0 unpublished
2016-02-10 08:47:41,000 - INFO -  - bz://1245463/jya: commit 63c4b53fe94097686bfbd753eb3ac9bee046c729 unchanged; using existing request 34325
2016-02-10 08:47:41,008 - INFO -  - bz://1245463/jya: 1/5 commits mapped exactly
2016-02-10 08:47:41,008 - INFO -  - bz://1245463/jya: 1/5 mapped exactly or to precursors
2016-02-10 08:47:41,019 - INFO -  - bz://1245463/jya: commit ID CHppUOGM1mk for 84846440cd8633f8537209b30e422b8e78ecea34 found in review request 34313
2016-02-10 08:47:42,509 - INFO -  - importing Bugzilla users from query "gerald": gerald13601@juno.com, gerald@hcl.co.in, gsquelart@mozilla.com, gx1202000@yahoo.com, linkin_mrchester@yahoo.com, nfitzgerald@mozilla.com, nickgdolan@hotmail.com
2016-02-10 08:47:42,902 - INFO -  - bz://1245463/jya: commit ID CXqGoq9Opq0 for a3c1cd15138b4eb82c1bcca8c832a0080ba8c68d found in review request 34327
2016-02-10 08:47:43,190 - INFO -  - bz://1245463/jya: commit ID Elnm0WPuqHC for d632a8621f852a206b7c48562e9cf7e3301e0d4c found in review request 34329
2016-02-10 08:47:43,458 - INFO -  - bz://1245463/jya: commit ID 71hgJ63ksPU for 94bcb17402f3c8064b769eecb8f8f608c07904eb found in review request 34331
2016-02-10 08:47:43,788 - INFO -  - bz://1245463/jya: 5/5 mapped after commit ID matching
2016-02-10 08:47:43,788 - INFO -  - bz://1245463/jya: 0 unclaimed review requests
2016-02-10 08:47:43,788 - INFO -  - bz://1245463/jya: 0 unclaimed review requests left over
2016-02-10 08:47:43,847 - INFO -  - bz://1245463/jya: finished processing 34309
Oops - missed a critical block:

2016-02-10 07:55:49,111 - INFO -  - processing batch submission bz://1245463/jya to gecko with 1 commits
2016-02-10 07:55:49,116 - INFO -  - bz://1245463/jya: generating squashed diffset for 34309
2016-02-10 07:55:49,264 - INFO -  - bz://1245463/jya: updated squashed diffset for 34309
2016-02-10 07:55:49,266 - INFO -  - bz://1245463/jya: 5 previous commits; 0 discard on publish; 0 unpublished
2016-02-10 07:55:49,267 - INFO -  - bz://1245463/jya: 0/1 commits mapped exactly
2016-02-10 07:55:49,267 - INFO -  - bz://1245463/jya: 0/1 mapped exactly or to precursors
2016-02-10 07:55:49,281 - INFO -  - bz://1245463/jya: commit ID CHppUOGM1mk for 96b453652419d451e6d614e037f64691d52780de found in review request 34313
2016-02-10 07:55:51,322 - INFO -  - bz://1245463/jya: 1/1 mapped after commit ID matching
2016-02-10 07:55:51,322 - INFO -  - bz://1245463/jya: 4 unclaimed review requests
2016-02-10 07:55:51,322 - INFO -  - bz://1245463/jya: 4 unclaimed review requests left over
2016-02-10 07:55:51,379 - INFO -  - bz://1245463/jya: finished processing 34309
So, the way this works today is that when you push a series smaller than one pushed before, the leftover review requests are marked for discarding on next publish. When published, we discard the old review requests and drop the reference to them. When the series, expanded series comes in, we have no reference to the prior review requests, so we create new review requests.

I think the solution here is to hold a reference to the discarded review requests for a series so we can look at their data and revive them if there is a match.

smacleod: does that make sense to you? Can you see any problems restoring a review request from discarded state? Does Review Board hold onto the original data so we can inspect it for e.g. MozReview-Commit-ID mappings?
Blocks: 1243483
Flags: needinfo?(smacleod)
(Reporter)

Comment 4

3 years ago
additionally, would there be a way to detect that a new commit is identical to the previous one and that just the commit number changed (which are mapped to the review via the MozReviewCommit-ID).

So when you rebase, it doesn't make you believe that changes were made. It would be great if those were totally ignored.

This is assuming of course, that when you only update one commit, you must push all of them all the time.
If we could simply push a single update commit that of course is ideal.

glandium was talking about the need to handle squashed commit, but to me if a commit was indeed the combination of others, you could assume it would have a new MozReviewCommit-ID and should then be treated as a complete new review.

But an update can easily be detected as just that: it doesn't have a different ID, and no other reviews should be discared.
The concept of discarding "leftover reviews" because you pushed a single update is foreign to me.
(In reply to Jean-Yves Avenard [:jya] from comment #4)
> additionally, would there be a way to detect that a new commit is identical
> to the previous one and that just the commit number changed (which are
> mapped to the review via the MozReviewCommit-ID).

We already do this.

See https://hg.mozilla.org/hgcustom/version-control-tools/file/3ac7f2de1e6b/pylib/mozreview/mozreview/resources/batch_review_request.py#l470 if you want to see how the sausage is made.

> So when you rebase, it doesn't make you believe that changes were made. It
> would be great if those were totally ignored.

I'm not sure what you mean here.

> This is assuming of course, that when you only update one commit, you must
> push all of them all the time.
> If we could simply push a single update commit that of course is ideal.

The idea is that you push a working branch/head to MozReview and it figures things out. We want to encourage development to be done in terms of commits on a repository rather than standalone patches. Think GitHub pull requests.

If we offered a mode where you only pushed a single commit to an existing series, how do you distinguish between wanting to just update the review request of that single commit versus dropping the remaining commits? To the server, they look exactly the same! So you need a way to disambiguate and that creates UX complexity. It's much easier to say "whatever you push/select for review is what the review series morphs into."

> glandium was talking about the need to handle squashed commit, but to me if
> a commit was indeed the combination of others, you could assume it would
> have a new MozReviewCommit-ID and should then be treated as a complete new
> review.

Correct. We partially fixed this in bug 1242246. This is likely the next tweak to improve commit mapping because the current behavior is far from ideal because it can re-associate a review request with a logically unrelated commit.

> The concept of discarding "leftover reviews" because you pushed a single
> update is foreign to me.

Reading https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#bug-references-review-identifiers-and-grouping-commits may help.
(Reporter)

Comment 6

3 years ago
(In reply to Gregory Szorc [:gps] from comment #5)
> > So when you rebase, it doesn't make you believe that changes were made. It
> > would be great if those were totally ignored.
> 
> I'm not sure what you mean here.

Ok..

so here I made a few rebase modifying P2.
https://reviewboard.mozilla.org/r/34327/diff/1-3/

it shows 3 diff, but this commit never changed, it was rebased, so commit hash changed, but not the content.
(In reply to Gregory Szorc [:gps] from comment #3)
> So, the way this works today is that when you push a series smaller than one
> pushed before, the leftover review requests are marked for discarding on
> next publish. When published, we discard the old review requests and drop
> the reference to them. When the series, expanded series comes in, we have no
> reference to the prior review requests, so we create new review requests.
> 
> I think the solution here is to hold a reference to the discarded review
> requests for a series so we can look at their data and revive them if there
> is a match.
> 
> smacleod: does that make sense to you? Can you see any problems restoring a
> review request from discarded state? Does Review Board hold onto the
> original data so we can inspect it for e.g. MozReview-Commit-ID mappings?

Restoring from a discarded state should be fine, you just need to re-open the review request before updating it. Right now reviving those old ones would be a very complicated and expensive operation because of how the data is stored (in extra_data / change description blobs on the parent). After Bug 1246691 is finished this would be much easier to handle.
Flags: needinfo?(smacleod)
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> so here I made a few rebase modifying P2.
> https://reviewboard.mozilla.org/r/34327/diff/1-3/
> 
> it shows 3 diff, but this commit never changed, it was rebased, so commit
> hash changed, but not the content.

Computing interdiffs when there are rebases is a fundamentally hard problem. There have been a number of bug reports about Review Board's diff rendering algorithm doing bad things. smacleod is working on fixing these issues upstream.

As a reviewer, it is best to not rely on the Review Board rendered interdiff too much.
Depending on bug 1246691 per comment 7.
Depends on: 1246691
Product: Developer Services → MozReview
This will be, I believe, mitigated by bug 1227121.  gps, should I dupe this over?
Flags: needinfo?(gps)
Yes, bug 1227121 would mitigate this by hopefully preventing it at submission time.

Long term, we'll want to revive discarded review requests. We should keep this bug open to track that.
Flags: needinfo?(gps)

Comment 12

6 months ago
MozReview is now obsolete. Please use Phabricator instead. Closing this bug.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.