1578421 - Consider not modifying the status of an individual differential revisions on re-submit

Status: RESOLVED WONTFIX

(Conduit :: moz-phab, enhancement)

Description

[Greg Tatum [:gregtatum]]

I ran into some issues trying to update a stack of commits.

Steps to reproduce:

Run moz-phab on the following changesets:

• Commit A
• Commit B
• Commit C

A revision is created for each, and the status is "Ready to review"

• Commit A - Ready to Review
• Commit B - Ready to Review
• Commit C - Ready to Review

The reviews come back, and I plan on only fixing one of the commits first, so I mark them accordingly

• Commit A - Ready to Review
• Commit B - Plan changes
• Commit C - Plan changes

I rewrite my changeset, then run moz-phab submit

Actual Results:

The revisions are marked as:

• Commit A - Ready to Review
• Commit B - Ready to Review
• Commit C - Ready to Review

Expected results

The revision retain their statuses:

• Commit A - Ready to Review
• Commit B - Plan changes
• Commit C - Plan changes

Side notes

Current behavior:

I believe this should be the default behavior on re-submitting, but currently there is only the two modes:

1 - Mark all as ready for review (by default)
2 - Mark all as changes planned (with --wip)

Desired behavior:

On updating a stack:

1 - Keep the status of the revisions the same (by default)
2 - Mark all as changes planned (with --wip)
3 - (optional) Mark all as needing review (with something like --request-review)

Comment 1

[Piotr Zalewa [:zalun]]

moz-phab is operating on all of the elements in the stack.
Determining if a revision was indeed changed by the user is complicated and might be implement at some point.

Comment 2

[Greg Tatum [:gregtatum]]

Determining if a revision was indeed changed by the user is complicated and might be implement at some point.

I don't believe it needs to be determined if the revision was changed by a user. There's would only be two behaviors here:

• Set initial revision status if it's a first moz-phab submit.
• Update the code, but do not modify revision status on a re-submit, unless explicitly flagged by the user.

For instance, in this revision there was a lot of confusion around the status of it:
https://phabricator.services.mozilla.com/D41637

• I left a review requesting changes
• The author updated some patches in the stack unrelated to the one I was reviewing
• My review reverted to "Review requested"
• The author contacted me on Slack asking for my review, and was confused as to the status of the revision.

Comment 3

[:glob ✱]

(In reply to Greg Tatum [:gregtatum] from comment #2)

I don't believe it needs to be determined if the revision was changed by a user. There's would only be two behaviors here:

• Set initial revision status if it's a first moz-phab submit.
• Update the code, but do not modify revision status on a re-submit, unless explicitly flagged by the user.

Unfortunately determining if a submission is a "re-submit" or not is the part of this suggestion that is complex.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Why doesn't a "Differential Revision: https://phabricator.services.mozilla.com/DNNNNN" in the patch comments tell you if it's a resubmit or not?

Is this really "WONTFIX", or is this "we aren't going to fix this now"? The comment made implied it might be fixed at some point, and that in any case it's a reasonable request and I see no dispute that the behavior would be better than current.

Comment 5

[Piotr Zalewa [:zalun]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #4)

We can say that the revision will be updated by reading the commit description, but deciding whether a change has been done is complex. There would be a sane argument in a discussion if we should change the state of a revision if no change has been made. Unfortunately, since we're currently unable to detect that, there is no point in making such an argument.

Setting the "request review" state after an update is there by design and we don't want to change it.
There are discussions about the way we could handle updates in a more sophisticated way, but the default behavior will not change.

Comment 6

[Greg Tatum [:gregtatum]]

Setting the "request review" state after an update is there by design and we don't want to change it.

What about adding a command line flag like --no-status-update? This way a user could specifically opt-in to this behavior? On moz-phab submit the patches would have their code updated, but their review status would not change. There is already the --wip flag, which is somewhat similar.