Page MenuHomePhabricator

Provide a link from revisions that are blamees to their blamers
Closed, ResolvedPublic

Description

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.

Related Objects

StatusAssignedTask
Resolvedjoshuaspence
Resolvedepriestley
Resolvedepriestley
Resolvedepriestley

Event Timeline

aran added a subscriber: aran.
epriestley triaged this task as Low priority.Feb 15 2013, 3:07 PM

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?)

epriestley added a subscriber: epriestley.
epriestley edited this Maniphest Task.Feb 19 2013, 2:55 PM
epriestley edited this Maniphest Task.Feb 28 2013, 8:20 PM

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:

  1. someone lands a revision, or makes a commit (no revision)
  2. bug is discovered
  3. source of bug is found, and concern raised on relevant commit (which links back to revision, if there is one)
  4. 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:

  1. The person to raise the concern (ostensibly, the person who discovered the bug) can be automatically suggested as a reviewer of the authored revision
  2. arc land can create a branch on the fixed commit, apply the patch, then merge that into master (DaggyFixes)
  3. 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.

metellius raised the priority of this task from Low to Normal.Aug 7 2014, 4:03 PM
metellius lowered the priority of this task from Normal to Low.
avivey added a subscriber: avivey.Feb 16 2015, 1:13 AM

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.

eadler added a subscriber: eadler.Apr 29 2015, 6:29 AM
chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 3:57 AM
epriestley closed this task as Resolved.Dec 12 2016, 9:18 PM
epriestley claimed this task.

"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.