Just for those who might not know how Differential works: If the Differential revision has an associated task then it automatically shows up, see T146602 and the associated diff D399. (That task also has the patch-for-review tag because of an associated gerrit change).

I'm ambivalent to adding patch-for-review to task with Differential revisions.

One question. I did some patching for scap using differential. I think the Patch-For-Review should be added when we do arc diff (so it's up for review) and remove it once we do arc land (so it's merged). Am I right?

@Ladsgroup Yes, you're right! I think I've wrote that back then late night.

The removal doesn't happen automatically with gerrit patches as it is harder.

Really, now this is where my true opinion comes out: the Patch-For-Review tag is a clunky replacement for what Phabricator has built in with Differential. See eg this task in the upstream instance: https://secure.phabricator.com/T8887 It has two diffs attached to it. As they were being reviewed and merged you could quickly glance at that section and see what is left to do/merge.

The Patch-For-Review bot will never be that intelligent.

@greg And all of that is understandable and rightful. But maybe my English in the task description is confusing. The Patch-For-Review tag on Kanban workboards is great to move certain things forward and keep overview of what's happening, compare OOUI workboard. Differential currently misses that.
So I'm not talking about the task view in Phabricator, I'm talking about the workboard view.

Yeah, I was just responding to @Ladsgroup's comment above.

The visual cue (in tasks lists or workboards) the tag gives is a nice thing, you're right. I was hoping it could be a simple Herald rule, but I can't work it out how to do it.

In T150510#2988339, @greg wrote:

Really, now this is where my true opinion comes out: the Patch-For-Review tag is a clunky replacement for what Phabricator has built in with Differential.

Not really, Differential changesets are not exposed to Maniphest in any intelligent way (you cannot filter by them, cannot see them in a list or board). Gerritbot adding comments about changes being uploaded/merged/aborted *is* a clunky replacement for Differential, but that's a different aspect.

In T150510#2988431, @Tgr wrote:

Not really, Differential changesets are not exposed to Maniphest in any intelligent way (you cannot filter by them, cannot see them in a list or board).

Yeah, I'm actually surprised by that myself.

@Volker_E we will begin adding proposals to a MediaWiki page for the developer wishlist's voting phase soon, could you elaborate more on the task, and structure it as follows: Problem, Who would Benefit and Proposed Solution?

In T150510#2988288, @greg wrote:

If the Differential revision has an associated task then it automatically shows up

...and that's why I'm looking forward to get rid of error-prone Patch-For-Review. :)

In T150510#2988381, @Volker_E wrote:

The Patch-For-Review tag on Kanban workboards is great to move certain things forward and keep overview of what's happening, compare OOUI workboard. Differential currently misses that.

I wonder if upstream could be interested in introducing something like a "This task has a patch proposal in Differential associated" icon in the card on the workboard, if convincing use cases are provided. (Related upstream task.)

To provide a little upstream perspective: we don't currently have support for this because we haven't seen (much? any?) interest from other installs.

On most installs, I believe that it's typical for code to be reviewed within a day or so, and most of the related feedback I can recall is aimed at driving this number down and making review turnaround faster, with a goal measurable in hours.

For example, https://secure.phabricator.com/T12177 shows review latency measurable in hours, with a request to improve charting to help drive it down. https://secure.phabricator.com/T9367 is a huge novel of a task, but a major driving goal was reducing ~12-16 hours of latency added in some cases by timezone differences. My recollection is that shaving hours off review turnarounds was also a goal at Facebook. All three of these cases are from larger, established engineering companies, so I believe this is usually an achievable goal in at least some realistic engineering environments at scale.

From discussion in https://secure.phabricator.com/T10584 and elsewhere, I believe the median review time for WMF was higher (weeks?), at least some time in the past (last several years?). I'm not sure if this has changed since then or if whatever charts I was looking at were accurate or comprehensive, but if you want to bring this upstream it would be helpful to understand:

• How long do you currently expect tasks to sit in "Patch-for-Review"?
• If it's much longer than "about a day", would "Patch-for-Review" still be important if I cast a magic spell that reduced median review to about a day?
• If the magic spell could moot this, what are the major barriers you face to driving median review times down into the "about a day" range? Are there technical approaches we could take to try to tackle those instead, and moot this?

My belief is that driving review times down is a powerful change for good with many desirable second-order effects, so I'd prefer to focus effort on that -- instead of this -- if this is largely a bandaid for long review times and there are technical changes we could pursue to help attack the root cause instead.

This project is selected for the Developer-Wishlist voting round and will be added to a MediaWiki page very soon. To the subscribers, or proposer of this task: please help modify the task description: add a brief summary (10-12 lines) of the problem that this proposal raises, topics discussed in the comments, and a proposed solution (if there is any yet). Remember to add a header with a title "Description," to your content. Please do so before February 5th, 12:00 pm UTC.

In T150510#2989973, @epriestley wrote:

To provide a little upstream perspective: we don't currently have support for this because we haven't seen (much? any?) interest from other installs.

On most installs, I believe that it's typical for code to be reviewed within a day or so, and most of the related feedback I can recall is aimed at driving this number down and making review turnaround faster, with a goal measurable in hours.

We aren't there, and as a big messy open source project with hundreds of repos with varying actively/maintenance level we never will be (except for in a small number, relatively, of repos). See: https://wikimedia.biterg.io:443/goto/d9538e34728400b7dbf26d4be52631f9

Average Time Open in Days per Changeset (Status: NEW): 184.96 days

That's half a year.

For example, https://secure.phabricator.com/T12177 shows review latency measurable in hours, with a request to improve charting to help drive it down. https://secure.phabricator.com/T9367 is a huge novel of a task, but a major driving goal was reducing ~12-16 hours of latency added in some cases by timezone differences. My recollection is that shaving hours off review turnarounds was also a goal at Facebook. All three of these cases are from larger, established engineering companies, so I believe this is usually an achievable goal in at least some realistic engineering environments at scale.

Large, established engineering organizations with only a small set of products/repos and not much if any really large open source projects (in the since of community owned and not driven by the org's PMs/engineers) to speak of.

From discussion in https://secure.phabricator.com/T10584 and elsewhere, I believe the median review time for WMF was higher (weeks?), at least some time in the past (last several years?). I'm not sure if this has changed since then or if whatever charts I was looking at were accurate or comprehensive, but if you want to bring this upstream it would be helpful to understand:

• How long do you currently expect tasks to sit in "Patch-for-Review"?

Expect anywhere from 1 second to a year. :)

• If it's much longer than "about a day", would "Patch-for-Review" still be important if I cast a magic spell that reduced median review to about a day?

I'd love whatever you're smoking :)

• If the magic spell could moot this, what are the major barriers you face to driving median review times down into the "about a day" range? Are there technical approaches we could take to try to tackle those instead, and moot this?

cc @mmodell for ideas here and we're exploring options to improve this generally (see T129842) but I don't foresee this being a "solved" problem in anything less than 1.5 years from now. And that's optimistic :)

My belief is that driving review times down is a powerful change for good with many desirable second-order effects, so I'd prefer to focus effort on that -- instead of this -- if this is largely a bandaid for long review times and there are technical changes we could pursue to help attack the root cause instead.

I want to push back on this idea a bit :). I believe about 90+% of the changes to be made to get us to lower code review times are mostly social/policy, not technical (correct me if I'm wrong). Those "social issues" are in some ways not seen as issues in our community. Things like "a repository only has one or two veeeery part time volunteers working on it and no PMs or any organizational support". We have a lot of those :) And we could be jerks and just have a policy of "close all open changesets with no activity older than 3 months". That will definitely help with the time to review average, but it will also reduce the number of gifts we receive from our community (every patch is a gift of someone's time). Having your patch closed by a bot because it was too old is demoralizing.

I don't think "closing old patches" is the only thing that could be done, of course. Maybe I just believe that "really old patchsets are a reality for us, in some way" and we should have our tooling support that use case.

(for the record, I think *just* being able to A) query for tasks with associated open Diffs and B) query for open Diffs with(out) associated tasks would be hugely beneficial.)

In T150510#3006728, @greg wrote:

(for the record, I think *just* being able to A) query for tasks with associated open Diffs and B) query for open Diffs with(out) associated tasks would be hugely beneficial.)

This doesn't seem like a very hard thing to build. Using the patch-for-review tag is an ugly hack and will always risk getting out of sync with reality. Directly querying the relationships (phabricator edges, I believe) is fairly trivial to implement.

This wouldn't be exactly what this task is asking for, however, it would address the need without hacks and without getting out of sync with reality.

Caveat: that doesn't help us with gerrit at all.

every patch is a gift of someone's time

This is largely a philosophical tangent, but:

In the Phabricator upstream, we embraced a similar point of view ("every patch is a gift") at launch, but as we grew we saw a lot of contributions impose heavy costs on the upstream. For a time, we started collecting our own backlog of unreviewed third-party patches that no one was really dealing with.

I think we generally did not deal with the patches for two reasons:

• The cost to upstream (review, maintain and support) the patch often greatly exceeded the implementation cost of the patch: the user had spent an hour writing something that would take us 15 minutes to write, and implicitly wanted us to spend, say, 2+ additional hours of our time upstreaming things (and who knows how much time supporting the change in the long run).
• The cost and emotional toll to reject the patch in an empathetic way very high, and it was emotionally easier to just let it sit without response forever. If we were lucky, we'd implement some replacement feature eventually and then we could say "This is available now in [some different form].", which doesn't lead to a confrontation or anyone getting upset or feeling like they wasted their time too much,

In response to this growing backlog, we've generally required patches to meet a higher bar over time: our philosophy is now more like "[almost] every patch is a [white elephant] gift", and our modern contribution policy is fairly close to "don't send patches".

I think Phacility's "Iron Throne" model and WMF's "Vibrant Community" model are near opposite ends of the spectrum even though we're both relatively open projects in some senses, but I'm curious if the philosophical view of contributions as gifts is nryyrt supported in practice for your projects than it was for ours. Are all these great patches really just sitting unreviewed for mundane reasons, not because they're, say, fundamentally a giant emotional black hole which it's impossible to wade through without getting people upset at you after you spend a great deal of time very gently and empathetically rejecting their patches?

That is, how often is this scenario happening?

• A new contributor spends 2-3 hours of time writing a patch that they're really proud of and invested in.
• They send it for review.
• It's not upstreamable, for whatever reason or reasons.
• It sits unreviewed forever because there is no way to reject it, on average, because doing so is a big emotional drain: there's no way to reject it without hating yourself for being a jerk to the poor innocent contributor who has already invested so heavily in the patch.

My experience with Phabricator, at least, is that this happened a lot. Once a contributor already spent an hour working on something not upstreamable, there was no positive outcome: they have set themselves up for a huge disappointment if the answer is "no". The fix we pursued was to stop contributors from making that huge time investment in the first place, and direct them to file tasks before writing code instead, so that any code we received was already pre-approved as worth spending the time upstreaming.

I'd say this has generally worked out well for us (although it's hard to measure how many users we might be turning away, and every project has different values). Generally, I believe users now spend time discussing problems with us instead of writing patches, and these discussions are significantly more valuable, fruitful, and emotionally positive for both sides than the old "send a patch" dynamic was: we generally learn more about use cases, we can often suggest a workaround which works immediately or explain why we want to pursue a more general solution and why the feature they imagine isn't as good a fit as it seems, and no one feels like their hard work got thrown in the trash.

The inverse scenario might be easier to quantify; how often does this -- where a patch is really just free work sitting on the ground -- happen?

• A patch sits unreviewed for a long time.
• When reviewed, it was accepted upstream promptly and the upstreaming / review / maintenance costs were small compared to the development cost of the patch (e.g., the reviewer didn't just rewrite the patch for the author, and didn't invest tons of time teaching them how to do things and helping them revise it).

Or, more simply: if you spend a morning reviewing patches, do you feel emotionally good about the experience ("I got some work done / I made this software better / I interacted with some enthusiastic new contributors who love the project") or emotionally bad ("I spent four hours crushing hopes and dreams and half those people are going to get upset and yell at me over the next week or tweet about how this project is awful / I upstreamed a bunch of really sketchy stuff that I'm going to have to maintain")?

(I haven't read through T129842 and linked discussion yet -- I'll take a look at them.)

From a purely technical point of view, this is sort of easy to implement but runs into issues with application partitioning (https://secure.phabricator.com/T11044).

Phabricator's application databases can now be separated onto different physical database hosts, so phabricator_maniphest and phabricator_differential may not be on the same physical host. If they are not, performing a JOIN between them is not possible.

The phabricator_maniphest database knows which revisions are connected to each task, but does not know if those revisions are open or not. The phabricator_differential database knows which tasks are connected to a revision, and whether those revisions are open or not, but can not be used to drive task queries.

You could just implement this JOIN anyway (which is perhaps ~50 lines of code, none of which has much risk of changing soon) and accept that you'll have to revisit the approach if you ever partition the physical databases and that this approach is not upstreamable.

A hypothetically upstreamable / partitioning-proof approach would probably involve marking this state formally in Maniphest in some way when a task or revision's relationships, or a revision's status, was updated. We don't currently have anything similar that could be easily extended.

You could do something like this to put some kind of marker on Workboard cards without partitioning peril. This does not allow you to query by these tasks, just marks them visually on the cards (in this patch, by prefixing the title with [Patch-for-Review]).

Some parts of this have some risk of future change (withStatus() will likely stop accepting status categories; workboards may eventually support non-Task cards, all this code may be rewritten for https://secure.phabricator.com/T4863). And this is totally unsupported and voids your warranty, etc. Performance could be improved somewhat but only with a much larger change, and should not be a huge issue.

diff --git a/src/applications/project/engine/PhabricatorBoardRenderingEngine.php b/src/applications/project/engine/PhabricatorBoardRenderingEngine.php
index ca8b0633da..027d7a9d48 100644
--- a/src/applications/project/engine/PhabricatorBoardRenderingEngine.php
+++ b/src/applications/project/engine/PhabricatorBoardRenderingEngine.php
@@ -10,6 +10,7 @@ final class PhabricatorBoardRenderingEngine extends Phobject {
   private $loaded;
   private $handles;
   private $coverFiles;
+  private $hasOpenRevisions;
 
   public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -56,7 +57,8 @@ final class PhabricatorBoardRenderingEngine extends Phobject {
     $card = id(new ProjectBoardTaskCard())
       ->setViewer($viewer)
       ->setTask($object)
-      ->setCanEdit($this->getCanEdit($phid));
+      ->setCanEdit($this->getCanEdit($phid))
+      ->setHasOpenRevisions(!empty($this->hasOpenRevisions[$phid]));
 
     $owner_phid = $object->getOwnerPHID();
     if ($owner_phid) {
@@ -131,6 +133,37 @@ final class PhabricatorBoardRenderingEngine extends Phobject {
     $this->coverFiles = $cover_files;
 
     $this->loaded = true;
+
+
+    $edge_query = id(new PhabricatorEdgeQuery())
+      ->withSourcePHIDs(mpull($this->objects, 'getPHID'))
+      ->withEdgeTypes(
+        array(
+          ManiphestTaskHasRevisionEdgeType::EDGECONST,
+        ));
+
+    $edge_query->execute();
+
+    $revision_phids = $edge_query->getDestinationPHIDs();
+    if ($revision_phids) {
+      $revisions = id(new DifferentialRevisionQuery())
+        ->setViewer($viewer)
+        ->withPHIDs($revision_phids)
+        ->withStatus(DifferentialRevisionQuery::STATUS_OPEN)
+        ->execute();
+      $revisions = mpull($revisions, null, 'getPHID');
+    } else {
+      $revisions = array();
+    }
+
+    $this->hasOpenRevisions = array();
+    foreach ($this->objects as $object) {
+      $task_phid = $object->getPHID();
+      $phids = $edge_query->getDestinationPHIDs(array($task_phid));
+
+      $has_revisions = (bool)array_select_keys($revisions, $phids);
+      $this->hasOpenRevisions[$task_phid] = $has_revisions;
+    }
   }
 
   private function getCanEdit($phid) {
diff --git a/src/applications/project/view/ProjectBoardTaskCard.php b/src/applications/project/view/ProjectBoardTaskCard.php
index ba5213a623..8a16284f9b 100644
--- a/src/applications/project/view/ProjectBoardTaskCard.php
+++ b/src/applications/project/view/ProjectBoardTaskCard.php
@@ -8,6 +8,7 @@ final class ProjectBoardTaskCard extends Phobject {
   private $owner;
   private $canEdit;
   private $coverImageFile;
+  private $hasOpenRevisions;
 
   public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -35,6 +36,15 @@ final class ProjectBoardTaskCard extends Phobject {
     return $this->coverImageFile;
   }
 
+  public function setHasOpenRevisions($has_open_revisions) {
+    $this->hasOpenRevisions = $has_open_revisions;
+    return $this;
+  }
+
+  public function getHasOpenRevisions() {
+    return $this->hasOpenRevisions;
+  }
+
   public function setTask(ManiphestTask $task) {
     $this->task = $task;
     return $this;
@@ -69,11 +79,16 @@ final class ProjectBoardTaskCard extends Phobject {
     $color_map = ManiphestTaskPriority::getColorMap();
     $bar_color = idx($color_map, $task->getPriority(), 'grey');
 
+    $header = $task->getTitle();
+    if ($this->getHasOpenRevisions()) {
+      $header = pht('[Patch-For-Review] %s', $header);
+    }
+
     $card = id(new PHUIObjectItemView())
       ->setObject($task)
       ->setUser($viewer)
       ->setObjectName('T'.$task->getID())
-      ->setHeader($task->getTitle())
+      ->setHeader($header)
       ->setGrippable($can_edit)
       ->setHref('/T'.$task->getID())
       ->addSigil('project-card')

In T150510#3006722, @greg wrote:

Average Time Open in Days per Changeset (Status: NEW): 184.96 days

I don't think that's a useful statistic (closed changesets are not counted so it suffers from survivor bias). Bitergia claims the median open time is around an hour (75th-percentile is a day, 95th-percentile is a month). Which is almost certainly wrong (probably due to l10nbot not being filtered - it has ten times more changes than the next most active user, and zero waiting time). Sadly with the recent death of korma we don't have even remotely reliable stats :(

In T150510#3007384, @Tgr wrote:

In T150510#3006722, @greg wrote:

Average Time Open in Days per Changeset (Status: NEW): 184.96 days

I don't think that's a useful statistic (closed changesets are not counted so it suffers from survivor bias).

[snip]

Sadly with the recent death of korma we don't have even remotely reliable stats :(

Noted. I was told by the Dev Rel team that it's pre-release quality/status right now, fwiw, so I shouldn't have just assumed anything :)

In T150510#3007174, @epriestley wrote:

From a purely technical point of view, this is sort of easy to implement but runs into issues with application partitioning (https://secure.phabricator.com/T11044).

Phabricator's application databases can now be separated onto different physical database hosts, so phabricator_maniphest and phabricator_differential may not be on the same physical host. If they are not, performing a JOIN between them is not possible.

The phabricator_maniphest database knows which revisions are connected to each task, but does not know if those revisions are open or not. The phabricator_differential database knows which tasks are connected to a revision, and whether those revisions are open or not, but can not be used to drive task queries.

You could just implement this JOIN anyway (which is perhaps ~50 lines of code, none of which has much risk of changing soon) and accept that you'll have to revisit the approach if you ever partition the physical databases and that this approach is not upstreamable.

A hypothetically upstreamable / partitioning-proof approach would probably involve marking this state formally in Maniphest in some way when a task or revision's relationships, or a revision's status, was updated. We don't currently have anything similar that could be easily extended.

Or I could do what we used to do at deviantArt and just implement the join in php :O

In T150510#3007090, @epriestley wrote:

(I haven't read through T129842 and linked discussion yet -- I'll take a look at them.)

After some investigation it appears that I have stumbled down a very deep hole. Out of fear that I may be invited to join a committee on the creation of working groups, I rescind all questions I have posed.

In T150510#3019603, @epriestley wrote:

After some investigation it appears that I have stumbled down a very deep hole. Out of fear that I may be invited to join a committee on the creation of working groups, I rescind all questions I have posed.

hahahahahah. Wise man, epriestley, very wise choice.