Page MenuHomePhabricator
Differential D21697 Diff 51680 src/repository/api/ArcanistMercurialAPI.php

Changeset View

Standalone View

View Options

src/repository/api/ArcanistMercurialAPI.php

<?php <?php
/** /**
* Interfaces with the Mercurial working copies. * Interfaces with the Mercurial working copies.
*/ */
final class ArcanistMercurialAPI extends ArcanistRepositoryAPI { final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
/**
* Mercurial deceptively indicates that the default encoding is UTF-8 however
* however the actual default appears to be "something else", at least on
* Windows systems. Force all mercurial commands to use UTF-8 encoding.
*/
const ROOT_HG_COMMAND = 'hg --encoding utf-8 ';
private $branch; private $branch;
private $localCommitInfo; private $localCommitInfo;
private $rawDiffCache = array(); private $rawDiffCache = array();
private $featureResults = array(); private $featureResults = array();
private $featureFutures = array(); private $featureFutures = array();
protected function buildLocalFuture(array $argv) { protected function buildLocalFuture(array $argv) {
$env = $this->getMercurialEnvironmentVariables(); $env = $this->getMercurialEnvironmentVariables();
// Mercurial deceptively indicates that the default encoding is UTF-8 $argv[0] = self::ROOT_HG_COMMAND.$argv[0];
// however the actual default appears to be "something else", at least on
// Windows systems. Force all mercurial commands to use UTF-8 encoding.
$argv[0] = 'hg --encoding utf-8 '.$argv[0];
$future = newv('ExecFuture', $argv) $future = newv('ExecFuture', $argv)
->setEnv($env) ->setEnv($env)
->setCWD($this->getPath()); ->setCWD($this->getPath());
return $future; return $future;
} }
public function newPassthru($pattern /* , ... */) { public function newPassthru($pattern /* , ... */) {
epriestleyUnsubmitted
Done
Inline Actions

PhutilExecPassthru and ExecFuture both extend PhutilExecutableFuture, which provides setEnv() and setCWD(), so this could be further simplified into:

protected function buildLocalFuture(...) {
  return $this->newConfiguredFuture(new ExecFuture(...));
}

protected function newPassthru(...) {
  return $this->newConfiguredFuture(new PhutilExecPassthru(...));
}

private function newConfiguredFuture(PhutilExecutableFuture $future) {
  return $future
    ->setCWD(...)
    ->setEnv(...)
}

I can shoot you a diff for that, though.

epriestley: `PhutilExecPassthru` and `ExecFuture` both extend `PhutilExecutableFuture`, which provides…
$args = func_get_args(); $args = func_get_args();
$env = $this->getMercurialEnvironmentVariables(); $env = $this->getMercurialEnvironmentVariables();
$args[0] = 'hg '.$args[0]; $args[0] = self::ROOT_HG_COMMAND.$args[0];
cspeckmimAuthorUnsubmitted
Done
Inline Actions

Missed this in D21676

cspeckmim: Missed this in D21676
return newv('PhutilExecPassthru', $args) return newv('PhutilExecPassthru', $args)
->setEnv($env) ->setEnv($env)
->setCWD($this->getPath()); ->setCWD($this->getPath());
} }
public function getSourceControlSystemName() { public function getSourceControlSystemName() {
return 'hg'; return 'hg';
} }
▲ Show 20 Lines • Show All 680 Lines • ▼ Show 20 Linesfinal class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
* @param array The list of child changesets off the original commit. * @param array The list of child changesets off the original commit.
* @param file The file containing the new commit message. * @param file The file containing the new commit message.
*/ */
private function amendNonHeadCommit($child_nodes, $tmp_file) { private function amendNonHeadCommit($child_nodes, $tmp_file) {
list($current) = $this->execxLocal( list($current) = $this->execxLocal(
'log --template %s --rev . --', 'log --template %s --rev . --',
'{node}'); '{node}');
$argv = array(); $this->execxLocalWithExtension(
foreach ($this->getMercurialExtensionArguments() as $arg) { 'arc-hg',
$argv[] = $arg; 'arc-amend --logfile %s',
} $tmp_file);
$argv[] = 'arc-amend';
$argv[] = '--logfile';
$argv[] = $tmp_file;
$this->execxLocal('%Ls', $argv);
list($new_commit) = $this->execxLocal( list($new_commit) = $this->execxLocal(
'log --rev tip --template %s --', 'log --rev tip --template %s --',
'{node}'); '{node}');
try { try {
$rebase_args = array( $rebase_args = array(
'--dest', '--dest',
$new_commit, $new_commit,
); );
foreach ($child_nodes as $child) { foreach ($child_nodes as $child) {
$rebase_args[] = '--source'; $rebase_args[] = '--source';
$rebase_args[] = $child; $rebase_args[] = $child;
} }
$this->execxLocal('rebase %Ls --', $rebase_args); $this->execxLocalWithExtension(
'rebase',
'rebase %Ls --',
$rebase_args);
} catch (CommandException $ex) { } catch (CommandException $ex) {
$this->execxLocal('rebase --abort --'); $this->execxLocalWithExtension(
'rebase',
'rebase --abort --');
throw $ex; throw $ex;
} }
$this->execxLocal('--config extensions.strip= strip --rev %s --', $this->execxLocalWithExtension(
'strip',
'strip --rev %s --',
$current); $current);
} }
public function getCommitSummary($commit) { public function getCommitSummary($commit) {
if ($commit == 'null') { if ($commit == 'null') {
return pht('(The Empty Void)'); return pht('(The Empty Void)');
} }
▲ Show 20 Lines • Show All 258 Lines • ▼ Show 20 Lines public function willTestMercurialFeature($feature) {
$this->executeMercurialFeatureTest($feature, false); $this->executeMercurialFeatureTest($feature, false);
return $this; return $this;
} }
public function getMercurialFeature($feature) { public function getMercurialFeature($feature) {
return $this->executeMercurialFeatureTest($feature, true); return $this->executeMercurialFeatureTest($feature, true);
} }
public function buildMercurialExtensionCommand($pattern /* , ... */) {
cspeckmimAuthorUnsubmitted
Done
Inline Actions

Agh I think I can simplify this, slight change incoming

cspeckmim: Agh I think I can simplify this, slight change incoming
$args = func_get_args();
$extension = $args[0];
$ext_pattern = $args[1];
$ext_pattern_args = array_slice($args, 2);
switch ($extension) {
case 'arc-hg':
$path = phutil_get_library_root('arcanist');
$path = dirname($path);
epriestleyUnsubmitted
Done
Inline Actions

Can this be "private"?

epriestley: Can this be "private"?
cspeckmimAuthorUnsubmitted
Done
Inline Actions

Ah, yes, will update

cspeckmim: Ah, yes, will update
$path = $path.'/support/hg/arc-hg.py';
$ext_config = 'extensions.arg-hg='.$path;
break;
case 'rebase':
$ext_config = 'extensions.rebase=';
break;
case 'shelve':
$ext_config = 'extensions.shelve=';
break;
case 'strip':
$ext_config = 'extensions.strip=';
break;
default:
throw new Exception(
pht('Unknown Mercurial Extension: "%s".', $extension));
}
// Escape potential spaces in path to arg-hg.py
$config_arg = csprintf('--config %s', $ext_config);
// Surely this is safe
$hg_cmd = vcsprintf($ext_pattern, $ext_pattern_args);
Lint: Unsafe Usage of Dynamic String
Inline Actions

Parameter 1 of vcsprintf should be a scalar string, otherwise it's not safe.

Lint: Unsafe Usage of Dynamic String: Parameter 1 of `vcsprintf` should be a scalar string, otherwise it's not safe.
return $config_arg.' '.$hg_cmd;
}
public function execxLocalWithExtension($pattern /* , ... */) {
$raw_cmd = call_user_func_array(
array($this, 'buildMercurialExtensionCommand'),
epriestleyUnsubmitted
Done
Inline Actions

The first param is a "@param string", and it has the "extension name" (not "command name") to enable, right?

epriestley: The first param is a "@param string", and it has the "extension name" (not "command name") to…
cspeckmimAuthorUnsubmitted
Done
Inline Actions

Oops yea, I forgot to update these after the last bit of cleanup.

cspeckmim: Oops yea, I forgot to update these after the last bit of cleanup.
func_get_args());
return $this->execxLocal($raw_cmd);
}
public function execFutureLocalWithExtension($pattern /* , ... */) {
$raw_cmd = call_user_func_array(
array($this, 'buildMercurialExtensionCommand'),
func_get_args());
epriestleyUnsubmitted
Done
Inline Actions

This parameter signature is really $extension_name, $pattern, ..., I think?

epriestley: This parameter signature is really `$extension_name, $pattern, ...`, I think?
epriestleyUnsubmitted
Not Done
Inline Actions

Can this be "private"?

epriestley: Can this be "private"?
cspeckmimAuthorUnsubmitted
Done
Inline Actions

This parameter signature is really $extension_name, $pattern, ..., I think?

Yea - in that case would I just use those variables directly but still use array_slice(func_get_args(), 2) to get the varargs after those two?

Can this be "private"?

This one is called by ArcanistMercurialRepositoryMarkerQuery to build up the arguments and invoke them with a passthru. I wasn't sure if that behavior (calls passthru->execute() was the same behavior as the newPassthru()->resolve() that the API uses. If so I can make a executePassthruWithExtension() variant and have it use that, then make this private.

cspeckmim: >This parameter signature is really `$extension_name, $pattern, ...`, I think? Yea - in that…
epriestleyUnsubmitted
Not Done
Inline Actions

You don't have to use the variables if you don't want to -- func_get_args() will still grab everything. Making the signature more precise only improves what the documentation generator can do. $args[0] or $extension_name will both work fine to access the value.

epriestley: You don't have to use the variables if you don't want to -- `func_get_args()` will still grab…
cspeckmimAuthorUnsubmitted
Done
Inline Actions

I went ahead and created executePassthruWithExtension() which is a slightly different code path for the passthru (it now calls passthru->resolve() instead of passthru->execute() but I did test the error case with the hg extension blowing up and it does catch the error still (and also logs the command in --trace)

cspeckmim: I went ahead and created `executePassthruWithExtension()` which is a slightly different code…
cspeckmimAuthorUnsubmitted
Done
Inline Actions

Gotcha - yea I think it's better clarity to name these. I wasn't sure if there's a better pattern to use but I went ahead and updated and it works better. I was initially confused reading some existing code as to what was functionally working vs. convention. I think I'm getting a better understanding of the reflection.

cspeckmim: Gotcha - yea I think it's better clarity to name these. I wasn't sure if there's a better…
return $this->execFutureLocal($raw_cmd);
}
private function executeMercurialFeatureTest($feature, $resolve) { private function executeMercurialFeatureTest($feature, $resolve) {
if (array_key_exists($feature, $this->featureResults)) { if (array_key_exists($feature, $this->featureResults)) {
return $this->featureResults[$feature]; return $this->featureResults[$feature];
} }
if (!array_key_exists($feature, $this->featureFutures)) { if (!array_key_exists($feature, $this->featureFutures)) {
$future = $this->newMercurialFeatureFuture($feature); $future = $this->newMercurialFeatureFuture($feature);
$future->start(); $future->start();
$this->featureFutures[$feature] = $future; $this->featureFutures[$feature] = $future;
} }
if (!$resolve) { if (!$resolve) {
return; return;
} }
$future = $this->featureFutures[$feature]; $future = $this->featureFutures[$feature];
$result = $this->resolveMercurialFeatureFuture($feature, $future); $result = $this->resolveMercurialFeatureFuture($feature, $future);
epriestleyUnsubmitted
Done
Inline Actions

Passing func_get_args() directly may not work in the general case when the called function includes reference parameters:

https://3v4l.org/ICX97

Phabricator doesn't[1] use reference parameters and this is always safe if you know the function you're calling doesn't use reference parameters, which you do in this case, but most/all of the func_get_args() elsewhere in the codebase does $args = ... to avoid hiccups here in the most general case.

Of course, this is kind of silly since it's not really anything specific to func_get_args() (any other function has the same problem). I thought there was some specific func_get_args() issue beyond this, but I can't come up with a reproduction case that does anything unusual in any version of PHP, so maybe I imagined there was a more general problem here very early on when I was learning PHP and never disabused myself of that notion.

So: this is safe unless I've double-tricked myself, and the $args = func_get_args();-even-when-you-don't-need-it pattern I've used in the rest of the codebase is almost certainly some pointless mumbo-jumbo I invented well more than a decade ago.

[1] It uses them in one place I think, inside xsprintf().

epriestley: Passing `func_get_args()` directly may not work in the general case when the called function…
cspeckmimAuthorUnsubmitted
Done
Inline Actions

I'll update this. I don't quite catch on to the difference though. Does inlining the function call not implicitly store the return value in a temporary variable that it could then pass the reference to?

cspeckmim: I'll update this. I don't quite catch on to the difference though. Does inlining the function…
epriestleyUnsubmitted
Not Done
Inline Actions

Right: this is PHP, so inlining the function call just breaks rather than using a temporary variable as you might reasonably expect from a language which had gone through more of a "design" or "thinking about it" phase.

(PHP is better these days, this behavior is a carryover from long long ago.)

epriestley: Right: this is PHP, so inlining the function call just breaks rather than using a temporary…
cspeckmimAuthorUnsubmitted
Done
Inline Actions

I have used PHP previously back in ~2002 but I don't really remember much from back then and I'm not sure I understood it more at the time. Somehow I managed to write a forum board and music catalog system. I might have that code somewhere...

cspeckmim: I have used PHP previously back in ~2002 but I don't really remember much from back then and…
$this->featureResults[$feature] = $result; $this->featureResults[$feature] = $result;
return $result; return $result;
} }
private function newMercurialFeatureFuture($feature) { private function newMercurialFeatureFuture($feature) {
switch ($feature) { switch ($feature) {
case 'shelve': case 'shelve':
return $this->execFutureLocal( return $this->execFutureLocalWithExtension(
'--config extensions.shelve= shelve --help --'); 'shelve',
'shelve --help --');
case 'evolve': case 'evolve':
return $this->execFutureLocal('prune --help --'); return $this->execFutureLocal('prune --help --');
default: default:
throw new Exception( throw new Exception(
pht( pht(
'Unknown Mercurial feature "%s".', 'Unknown Mercurial feature "%s".',
$feature)); $feature));
} }
Show All 17 Lines
protected function newMarkerRefQueryTemplate() { protected function newMarkerRefQueryTemplate() {
return new ArcanistMercurialRepositoryMarkerQuery(); return new ArcanistMercurialRepositoryMarkerQuery();
} }
protected function newRemoteRefQueryTemplate() { protected function newRemoteRefQueryTemplate() {
return new ArcanistMercurialRepositoryRemoteQuery(); return new ArcanistMercurialRepositoryRemoteQuery();
} }
public function getMercurialExtensionArguments() {
$path = phutil_get_library_root('arcanist');
$path = dirname($path);
$path = $path.'/support/hg/arc-hg.py';
return array(
'--config',
'extensions.arc-hg='.$path,
);
}
protected function newNormalizedURI($uri) { protected function newNormalizedURI($uri) {
return new ArcanistRepositoryURINormalizer( return new ArcanistRepositoryURINormalizer(
ArcanistRepositoryURINormalizer::TYPE_MERCURIAL, ArcanistRepositoryURINormalizer::TYPE_MERCURIAL,
$uri); $uri);
} }
protected function newCommitGraphQueryTemplate() { protected function newCommitGraphQueryTemplate() {
return new ArcanistMercurialCommitGraphQuery(); return new ArcanistMercurialCommitGraphQuery();
Show All 23 Lines
Phabricator · Built by Phacility