Another wishlist item from me... let's say I have one commit, with Differential Revision D1 and commit rE1. If I mark rE1 as the blame rev for a new D2, then it'd be cool if there was a link from rE1 and D1 to D2. It's currently convention at Facebook (but not always followed) to manually go back to D1 and post a link to D2 in this situation, this would formalize that.
Description
Description
| Status | Assigned | Task | ||
|---|---|---|---|---|
| Resolved | joshuaspence | T1751 Consider ways to improve revert handling | ||
| Resolved | epriestley | T2542 Provide a link from revisions that are blamees to their blamers | ||
| Resolved | epriestley | T2222 Implement ApplicationTransactions in Differential | ||
| Restricted Maniphest Task | ||||
| Restricted Maniphest Task | ||||
| Restricted Maniphest Task | ||||
| Restricted Maniphest Task | ||||
| Restricted Maniphest Task | ||||
| Restricted Maniphest Task | ||||
| Resolved | epriestley | T4481 Differential Update Feedback / Rough Edges / Status |
Event Timeline
Comment Actions
One thing that makes this complicated is that I think "Blame Rev" has a lot of freeform text in it. The best way forward might be:
- Define a new field, which accepts only resolvable object names.
- Make Audits and Differential aware of it (solves various other issues in the Audit workflow).
- Port/deprecate "Blame Rev".
I think the field name "Blame Rev" is also somewhat confusing, particularly in a Git world.
(Maybe this field should just be called "Related:" and/or "Fixes:", and really be the "Maniphest Tasks:" field, except that we accept any object?)
Comment Actions
Probably if there is a thing being blamed, it should be commits, not revisions. It may be that a commit was made without review that broke something, but never will it be the case that an un-committed revision broke anything. Also if you subscribe to DaggyFixes, then blaming a commit makes it clear from where the fix should be based. The workflow might look something like this:
- someone lands a revision, or makes a commit (no revision)
- bug is discovered
- source of bug is found, and concern raised on relevant commit (which links back to revision, if there is one)
- fix is authored as a revision or new commit. A field in the description references the commit that it's fixing.
There's now the potential for for some smartness:
- The person to raise the concern (ostensibly, the person who discovered the bug) can be automatically suggested as a reviewer of the authored revision
- arc land can create a branch on the fixed commit, apply the patch, then merge that into master (DaggyFixes)
- the audit can be automatically closed if the auditor reviewed the fixing revision, else the auditor can be automatically pinged
To the extent that Phabricator knows about release branches, it could also calculate which releases have this bug, and which incorporate the fix, and do neat stuff there.
Comment Actions
I'm not 100% sure what "blame revision" actually means, but current "mentioned" transactions might have solved this issue?
A comment/description in D2 would say "This fixes big bug introduced in D1" should(?) now show up in D1 page as "useer mentioned this in D2".
I think this should also be the case for mentioning commits.
Comment Actions
"Blame Revision" was a freeform text field at Facebook (from before DiffCamp, even) where you were ideally supposed to enter the revision or commit that introduced the bug you're fixing, if you were fixing a bug introduced by some known change.
I agree that the modern ability to mention revisions and commits probably satisfies this in essentially all cases. I could possibly see some kind of new relationship in the future like "Addresses (the audit concerns raised on) <commit>", that would work like "Fixes Txxx" but for closing audits, but we should do T10978 first. Generally, I think anything we add to Differential at this point should just be to improve consistency between the pre-commit and post-commit flows once those relationships get added to Audit.
Upshot:
- Mentions are pretty good at this now.
- When Audit gets an iteration, it will probably get some more formal relationships between commits.
- After that, we may mirror (or create analogs of) some of those relationships into Differential for consistency and flexibility.