moz-phab should require meaningful comments on revision updates
Categories
(Conduit :: moz-phab, defect, P3)
Tracking
(Not tracked)
People
(Reporter: bzbarsky, Unassigned)
References
Details
(Keywords: conduit-triaged)
Right now moz-phab just does "Revision updated." when someone updates a revision. As a reviewer, this is completely useless: I have no way to tell whether that's a "just merge to tip" or "I addressed your review comments", and I end up wasting a lot of time loading revisions and skimming through interdiffs for the "just merge to tip" case, because some people do it all the time.
Ideally people would actually indicate in the Phabricator comment that goes with a revision update why they are updating...
I agree this is desirable, however, the current implementation would force the same reason on every commit in a stack.
From Bug 1491190:
getting the ux right around this is tricky; if there's multiple commits in a stack it's unlikely the same reason should be bound to each commit, so a simple command line option wouldn't work.
possible options include (not mutually exclusive):
- support embedding it into the commit message, following arc's template rules, parsing it out and removing it from the commit message during submission
- add a command line switch that requires a revision identifier as well as the message
eg.-m . 'did the thing'# revset
-m -1 'some other thing'# relative to top of stack
-m 28ec379 'yet more things'# sha
-m 12345 'i am drowning in things'# revset- show an interactive editor once, pre-populated with spots for each commit in the stack to specify a reason
I'd prefer to see this deficiency addressed, at least in part, before we make update reasons mandatory.
In the short term we could display a warning if an update reason isn't provided when submitting a stack, and make the reason mandatory for single-commit submissions.
|
Reporter | |
Comment 2•6 years ago
|
||
I agree that this is a little tricky.
Ideally, as a reviewer, when people do an update they would in fact make it either a merge (for everything in the stack, presumably) or addressing review comments, but not mix the two together. Enforcing that is not simple, sadly.
This will be easier to do once we no longer rely on arc.
|
||
Comment 4•6 years ago
|
||
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #0)
Ideally people would actually indicate in the Phabricator comment that goes with a revision update why they are updating...
Would you have to do that per-revision, or per-stack? Doing it per-revision in a large commit series might be a lot of overhead.
|
||
Comment 5•6 years ago
|
||
I've noticed that if I use --no-arc, I don't get the "Revision updated." message. Is that intentional? I do try to use -m "Foo" and I find a single message per stack works well. If you want per-revision messages you can always use moz-phab $startRev $endRev -m "..."
|
|
Reporter | |
Comment 6•6 years ago
|
||
See comment 2. Per-stack is fine; as a reviewer I really want something like 2 bits of information out of a revision update: (1) are there substantive changes or is this a merge to tip? (2) If there are substantive changes, is this now ready for review or are there more changes still to come? The second bit could be provided if people used the "planned changes" state in Phabricator, but in practice they mostly don't...
(In reply to Jan de Mooij [:jandem] from comment #5)
I've noticed that if I use
--no-arc, I don't get the "Revision updated." message. Is that intentional?
Yes - the "Revision updated" message only exists because arc won't let us update a revision without a message :(
Description
•