Page MenuHomePhabricator

T4670 Remove the any-author flag from the which command, make any-author the default, and annotate the username of the owner of the revision.
ClosedPublic

Authored by gabe on Apr 3 2014, 1:42 PM.

Details

Summary

Remove any-author flag from 'arc which' make any-author be the default behavior. Annotate revision with the owners username.

Test Plan

Apply a patch that you don't own (arc patch Dxxx) run 'arc which' verify that:

  1. You see the revision
  2. You see the original authors username next to the revision (owned by sjobs) etc.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

gabe updated this revision to Diff 20603.Apr 3 2014, 1:42 PM
gabe retitled this revision from to T4670 Remove the any-author flag from the which command, make any-author the default, and annotate the username of the owner of the revision..
gabe updated this object.
gabe edited the test plan for this revision. (Show Details)
gabe added a reviewer: epriestley.
Herald added subscribers: Korvin, epriestley.
epriestley requested changes to this revision.Apr 3 2014, 5:00 PM
epriestley edited edge metadata.

This suffers from the N+1 Query Problem. Although it's not a big deal, it's easy to fix. A cleaner approach (which makes a single service call in the worst case, and thus performs better) would look like this:

$other_author_phids = array();
foreach ($revisions as $revision) {
  if ($revision['authorPHID'] != $this->getUserPHID()) {
    $other_author_phids[] = $revision['authorPHID'];
  }
}

$other_authors = array();
if ($other_author_phids) {
  $other_authors = $conduit->callMethodSynchronous(...);
  $other_authors = ipull($other_authors, 'userName', 'phid');
}

This also avoids the service call entirely in the common case.

In the output, let's not show anything special if the user is the author, and preface the revisions if they aren't to make it easier to see the author annotation. With the current approach, it can get lost at the end of a long line. For example, if the user owns D123 but not D124, output something like this:

D123 Add feature
D124 (alincoln) Add other feature
src/workflow/ArcanistWhichWorkflow.php
140–141

You can just omit this parameter.

This revision now requires changes to proceed.Apr 3 2014, 5:00 PM
gabe updated this revision to Diff 20613.Apr 3 2014, 6:25 PM
gabe edited edge metadata.

Implement requested changes, avoid N+1 Query Problem, and clean up output.

gabe added a comment.Apr 3 2014, 6:26 PM

I didn't see an easy way around having 2 foreach loops now... but, I might just be tired.

epriestley accepted this revision.Apr 3 2014, 6:28 PM
epriestley edited edge metadata.

The two loops are fine. Thanks!

This revision is now accepted and ready to land.Apr 3 2014, 6:28 PM
epriestley closed this revision.Apr 3 2014, 6:31 PM
epriestley updated this revision to Diff 20614.
epriestley added a subscriber: gabeguz.

Closed by commit rARC0e5bea940c59 (authored by @gabeguz, committed by @epriestley).

(I made some very minor formatting changes in the pull, and added a missing "\n" that I caught.)