Page MenuHomePhabricator

Build a `diffusion.blame` Conduit method
Closed, ResolvedPublic

Description

After T2450, we can expose blame information over Conduit and there's a reasonable chance we'll be faster than native blame.

Revisions and Commits

rP Phabricator
D14957

Related Objects

Event Timeline

epriestley added a subscriber: epriestley.
epriestley edited this Maniphest Task.

For now, we can just build this with the existing (slow) blame query. Once T2450 lands, it should magically get a lot faster. At a high level, you'll implement a new ConduitAPI_diffusion_blame_Method class, which will look a lot like the existing ConduitAPI_diffusion_getcommits_Method class (and other similar calls). The important part is the execute() method, which will basically look like this:

public function execute(ConduitAPIRequest $request) {
  // 1. Read parameters from request: which file should we blame, and which repository is it in?
  // 2. Construct a new DiffusionRequest object using the parameters.
  // 3. Pass the DiffusionRequest from (2) to a new DiffusionFileContentQuery.
  // 4. Execute the query and get the results.
  // 5. Format the results and return them.
}

For (1), the method should take a "path" to describe the path of the file to blame, but also needs to be able to figure out which repository the path exists in. For now, the easiest way to do this might be to take a repository callsign (later, when you integrate with arc, you may need to accept an Arcanist project name instead). You can build a DiffusionRequest given a callsign and a path. You might also want to take a commit parameter.

For (2), once you have enough information to construct the request, you can do so in a straightforward way:

$drequest = DiffusionRequest::newFromDictionary(
  array(
    'callsign' => $callsign,
    'path' => $path,
    'commit' => $commit,
  ));

For (3), you can use DiffusionFileContentQuery to get blame information:

$query = DiffusionFileContentQuery::newFromDiffusionRequest($drequest);
$query->setNeedsBlame(true);

For (4), just execute the query:

$query->loadFileContent();
list($lines, $commits, $blame) = $query->getBlameData();

Internally, this runs "git blame" (or "svn blame", etc.) so it will be slow, but after T2450 it should automatically use the blame cache.

For (5), you probably want to reformat the $lines, $commits and $blame parameters so they're a little easier to use. Something like this might be a good return format:

array(
  array(
    'text' => 'Text of the line',
    'author' => 'authorname',
    'authorPHID' => $author_phid, // if author has a Phabricator account
    'commit' => 'as98df7as98f7', // commit hash or ID
    'commitPHID' => $commit_phid, // Phabricator commit PHID
  ),
  ...
)

i.e., one dictionary for each line. This will be kind of big, but easy for callers to use and it should mostly gzip away anyway.

Let me know if that makes sense or you have any questions.

I wrote the class a ConduitAPI_diffusion_blame_Method class as you suggested. However I`m not sure if I reformat the results properly. extracting the data from the list.

$content_query->loadFileContent();
  list($lines, $commits, $blame) = $content_query->getBlameData();

foreach (list($lines, $commits, $blame) as list($line, $commit, $blame)) {
        
        results.append(array(
          'text' => $line,
          'author' => $blame[$commit]['author'],
          'authorPHID' => $blame[$commit]['author']->authorPHID,
          'commit' => $commit->id,
          'commitPHID' => $commit->commitPHIDs,
        );
      }

And after I finish this class how should I test it. I was thinking to call it in a random place and get it's output.

Thanks.

You should be able to use the web UI to test it -- go to /conduit/ on your local install and the method should appear in the list, then you can run it from the web UI.

For example, you can run the existing diffusion.getcommits method here: https://secure.phabricator.com/conduit/method/diffusion.getcommits/

(Calling it with random-but-valid inputs and inspecting the output should be sufficient.)

The input to diffusion.getcommits should be a JSON-formatted list of strings, like this:

["rP1beda307921c7da0bfdffebc2ac8934c2a0e38ee"]

For example, here's me making a call on secure.phabricator.com:

{F34532}
{F34534}

Before this will work locally, you'll need to import a repository if you haven't already. You can follow the instructions here:

http://www.phabricator.com/docs/phabricator/article/Diffusion_User_Guide.html

The short version is:

  • Repositories -> Create New Repository
  • Type = git, callsign = P, name = Phabricator
  • Click "Tracking"
  • Tracking = Enabled
  • Repository URI = git@github.com:facebook/phabricator.git
  • Local Path = some local path on disk like "/var/repo/phabricator"
  • Save
  • Start daemons with phabricator/ $ ./bin/phd start
epriestley added a subscriber: aba.
chad changed the visibility from "All Users" to "Public (No Login Required)".