1351545 - Create new bugzilla attachments instead of changing the names of attachments

Status: RESOLVED DUPLICATE of bug 1295338

(MozReview Graveyard :: Integration: Bugzilla, defect)

Description

[Karl Tomlinson (:karlt)]

MozReview seems to reuse the same attachment, even when a completely
different changeset is pushed to replace a previous revision.

This changes the name of the attachment, and existing comments on the first
revision are updated for the new name, causing them to claim to be comments on
something quite different.

A solution would be to create a new bugzilla attachment when the first line of
the commit message changes.

Comment 1

[Karl Tomlinson (:karlt)]

Or perhaps create an attachment for every changeset, and have each attachment link to the associated revision.

Comment 2

[:glob ✱]

(In reply to Karl Tomlinson (:karlt) from comment #0)
> MozReview seems to reuse the same attachment, even when a completely
> different changeset is pushed to replace a previous revision.

this is really bug 1295338.

https://reviewboard.mozilla.org/r/120128/diff/3-4/ shows MozReview-Commit-ID was changed, but an existing review request was used instead of creating a new one.

Comment 3

[Karl Tomlinson (:karlt)]

(In reply to Byron Jones ‹:glob› from comment #2)
> https://reviewboard.mozilla.org/r/120128/diff/3-4/ shows MozReview-Commit-ID
> was changed, but an existing review request was used instead of creating a
> new one.

That reviewboard link is for bug 1351546, and so is not really related to this bug.
The MozReview-Commit-ID was not changed, but mozreview assigned one new changeset to two review requests including overriding the one that did not change.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Byron Jones ‹:glob› from comment #2)
> (In reply to Karl Tomlinson (:karlt) from comment #0)
> > MozReview seems to reuse the same attachment, even when a completely
> > different changeset is pushed to replace a previous revision.
> 
> this is really bug 1295338.

Fixing bug 1295338 would deal with most of this bug, but we would still have attachment names changing after review comments, when commit messages change after review.  The key problem is that the content of attachments can change when the attachment tracks several revisions of a reviewboard request.

If bug 1295338 is going to be fixed, however, then that could be expressed better than comment 0 here.

Comment 5

[:glob ✱]

(In reply to Karl Tomlinson (:karlt) from comment #4)
> Fixing bug 1295338 would deal with most of this bug, but we would still have
> attachment names changing after review comments, when commit messages change
> after review.  The key problem is that the content of attachments can change
> when the attachment tracks several revisions of a reviewboard request.

essentially you're proposing using the first line of a commit description as a unique identifier.  this would mean fixing a typo in a commit desc would be treated as a new change, which is not the right thing to do.

the MozReview-Commit-ID is what's used for that; if a developer takes a different approach with their change they should create new commits instead of amending an existing one (which causes reuse of the MozReview-Commit-ID).

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Byron Jones ‹:glob› from comment #5)
> essentially you're proposing using the first line of a commit description as
> a unique identifier.

Yes, that is what comment 0 proposed.  Comment 1 is actually a better solution IMO, so that attachments link to the associated revision of the changeset, but the workaround is to follow the review link, expand, and then the revision link, which is missing for revision 1 (bug 1249468).  The workaround gets more complicated when there is no review link.

> the MozReview-Commit-ID is what's used for that; if a developer takes a
> different approach with their change they should create new commits instead
> of amending an existing one (which causes reuse of the MozReview-Commit-ID).

Perhaps.  Thanks, I hadn't understood that was the intention.  I thought it was just to tie review requests together, and I thought review requests could follow goals rather than approaches.  There are situations when it would be preferable to track the discussion and issues from the first approach on the same review request as the second approach to achieving the same goal.  A single-patch series is one such situation where there is little value in separate review requests for separate approaches to the same goal.  It also gets complicated if one changeset includes several approaches, some of which change, but there will always be corner cases where many to many mappings are not one to one.

Comment 7

[:glob ✱]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> Perhaps.  Thanks, I hadn't understood that was the intention.  I thought it
> was just to tie review requests together, and I thought review requests
> could follow goals rather than approaches.

we agree there's issues with the current approach.
these are being worked on in 'conduit', which will replace parts of mozreview.
see https://mrcote.info/blog/2017/03/21/conduits-commit-index/ for details.