Changeset View
Standalone View
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(); | $argv[0] = self::ROOT_HG_COMMAND.$argv[0]; | ||||
// Mercurial deceptively indicates that the default encoding is UTF-8 | return $this->newConfiguredFuture(newv('ExecFuture', $argv)); | ||||
// 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) | public function newPassthru($pattern /* , ... */) { | ||||
->setEnv($env) | $args = func_get_args(); | ||||
->setCWD($this->getPath()); | $args[0] = self::ROOT_HG_COMMAND.$args[0]; | ||||
return $future; | return $this->newConfiguredFuture(newv('PhutilExecPassthru', $args)); | ||||
} | } | ||||
public function newPassthru($pattern /* , ... */) { | private function newConfiguredFuture(PhutilExecutableFuture $future) { | ||||
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]; | return $future | ||||
Done Inline ActionsMissed this in D21676 cspeckmim: Missed this in D21676 | |||||
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 679 Lines • ▼ Show 20 Lines | final 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); | ||||
} | } | ||||
/** | |||||
Done Inline ActionsAgh I think I can simplify this, slight change incoming cspeckmim: Agh I think I can simplify this, slight change incoming | |||||
* Returns the necessary flag for using a Mercurial extension. This will | |||||
* enable Mercurial built-in extensions and the "arc-hg" extension that is | |||||
* included with Arcanist. This will not enable other extensions, e.g. | |||||
* "evolve". | |||||
* | |||||
* @param string The name of the extension to enable. | |||||
* @return string A new command pattern that includes the necessary flags to | |||||
* enable the specified extension. | |||||
*/ | |||||
private function getMercurialExtensionFlag($extension) { | |||||
Done Inline ActionsCan this be "private"? epriestley: Can this be "private"? | |||||
Done Inline ActionsAh, yes, will update cspeckmim: Ah, yes, will update | |||||
switch ($extension) { | |||||
case 'arc-hg': | |||||
$path = phutil_get_library_root('arcanist'); | |||||
$path = dirname($path); | |||||
$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)); | |||||
} | |||||
return csprintf('--config %s', $ext_config); | |||||
} | |||||
/** | |||||
* Produces the arguments that should be passed to Mercurial command | |||||
* execution that enables a desired extension. | |||||
* | |||||
* @param string The name of the extension to enable. | |||||
Done Inline ActionsThe 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… | |||||
Done Inline ActionsOops 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. | |||||
* @param string The command pattern that will be run with the extension | |||||
* enabled. | |||||
* @param array Parameters for the command pattern argument. | |||||
* @return array An array where the first item is a Mercurial command | |||||
* pattern that includes the necessary flag for enabling the | |||||
* desired extension, and all remaining items are parameters | |||||
* to that command pattern. | |||||
*/ | |||||
private function buildMercurialExtensionCommand( | |||||
Done Inline ActionsThis parameter signature is really $extension_name, $pattern, ..., I think? epriestley: This parameter signature is really `$extension_name, $pattern, ...`, I think? | |||||
Not Done Inline ActionsCan this be "private"? epriestley: Can this be "private"? | |||||
Done Inline Actions
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?
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… | |||||
Not Done Inline ActionsYou 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… | |||||
Done Inline ActionsGotcha - 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… | |||||
Done Inline ActionsI 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… | |||||
$extension, | |||||
$pattern /* , ... */) { | |||||
$args = func_get_args(); | |||||
$pattern_args = array_slice($args, 2); | |||||
$ext_flag = $this->getMercurialExtensionFlag($extension); | |||||
$full_cmd = $ext_flag.' '.$pattern; | |||||
$args = array_merge( | |||||
array($full_cmd), | |||||
$pattern_args); | |||||
return $args; | |||||
} | |||||
public function execxLocalWithExtension( | |||||
$extension, | |||||
$pattern /* , ... */) { | |||||
Done Inline ActionsPassing func_get_args() directly may not work in the general case when the called function includes reference parameters: 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… | |||||
Done Inline ActionsI'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… | |||||
Not Done Inline ActionsRight: 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… | |||||
Done Inline ActionsI 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… | |||||
$args = func_get_args(); | |||||
$extended_args = call_user_func_array( | |||||
array($this, 'buildMercurialExtensionCommand'), | |||||
$args); | |||||
return call_user_func_array( | |||||
array($this, 'execxLocal'), | |||||
$extended_args); | |||||
} | |||||
public function execFutureLocalWithExtension( | |||||
$extension, | |||||
$pattern /* , ... */) { | |||||
$args = func_get_args(); | |||||
$extended_args = call_user_func_array( | |||||
array($this, 'buildMercurialExtensionCommand'), | |||||
$args); | |||||
return call_user_func_array( | |||||
array($this, 'execFutureLocal'), | |||||
$extended_args); | |||||
} | |||||
public function execPassthruWithExtension( | |||||
$extension, | |||||
$pattern /* , ... */) { | |||||
$args = func_get_args(); | |||||
$extended_args = call_user_func_array( | |||||
array($this, 'buildMercurialExtensionCommand'), | |||||
$args); | |||||
return call_user_func_array( | |||||
array($this, 'execPassthru'), | |||||
$extended_args); | |||||
} | |||||
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(); | ||||
Show All 9 Lines | private function executeMercurialFeatureTest($feature, $resolve) { | ||||
$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 |
PhutilExecPassthru and ExecFuture both extend PhutilExecutableFuture, which provides setEnv() and setCWD(), so this could be further simplified into:
I can shoot you a diff for that, though.