revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-19 16:58:48 +02:00
Code Issues Releases Wiki Activity

Update templates used with mercurial to remove '--debug'

Browse source 
Summary:
Refer to discussion on D21677#275541

Refs D21681 (arcanist changes)

Phabricator has several uses of the `--debug` flag being used with Mercurial. Use of this flag causes additional output to be added which Phabricator needs, however the behavior of `--debug` is not guaranteed to be stable, and in newer versions of Mercurial there have been additional output that has caused Phabricator to choke on parsing the output. This change removes several uses of `--debug` in favor of using `--template` with the `hg log` or `hg annotate` commands in combination with the `{p1.node}` or `{p2.node}` template format.

The use of `{p1node}` format in templates was added in [[ https://www.mercurial-scm.org/wiki/WhatsNew/Archive#Mercurial_2.4_.282012-11-01.29 | Mercurial 2.4 (2012) ]]. This format was deprecated in [[ https://www.mercurial-scm.org/wiki/WhatsNew#Mercurial_4.9_.282019-02-01.29 | Mercurial 4.9 (2019) ]] in favor of using `{p1.node}` format which is unclear when this new format was added (presumably earlier than Mercurial 4.9).

The use of `--template` with `hg annotate` is only officially supported in [[ https://www.mercurial-scm.org/wiki/Release4.6 | Mercurial 4.6 (2018) ]], though does appear to work in 4.5 but is not documented.

Since the `{p1node}` format was introduced in 2.4 this bumps the required version of `hg` to 2.4 (from 1.9). Since the `annotate --template` feature wasn't added until 4.6 (which is still fairly recent), the use of it is gated on a capability test, but still preferred for use where possible to avoid extraneous output from `--debug` flag.

Test Plan:
I verified I could do the following in a mercurial repository, while having mercurial 5.8 installed:
1. Navigate and view files in Diffusion under e.g. `/source/test-repo/`.
2. While viewing a file in Diffusion verified that I could view the blame of the file and the history/annotations looked accurate for the files I was browsing.
3. From the blame sidebar, select to view a commit which loaded and displayed changes properly.
4. View the history of the repository under e.g. `/source/test-repo/history/default/`. I verified the history looked correct and the tree-like structure showing relationship of commits also looked accurate.

I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D21679
This commit is contained in:
Christopher Speck 2021-07-06 20:58:36 -04:00
parent 5521f76fe4
commit e5de7f0843
6 changed files with 121 additions and 112 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
src/applications/config/check/PhabricatorBinariesSetupCheck.php
View file

@ -120,17 +120,11 @@ final class PhabricatorBinariesSetupCheck extends PhabricatorSetupCheck {
break;
case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL:
$bad_versions = array(
// We need 1.9 for HTTP cloning, see T3046.
'< 1.9' => pht(
'The minimum supported version of Mercurial is 1.9, which was '.
'released in 2011.'),
'= 2.1' => pht(
'This version of Mercurial returns a bad exit code '.
'after a successful pull.'),
'= 2.2' => pht(
'This version of Mercurial has a significant memory leak, fixed '.
'in 2.2.1. Pushing fails with this version as well; see %s.',
'T3046#54922'),
// We need 2.4 for utilizing `{p1node}` keyword in templates, see
// D21679 and D21681.
'< 2.4' => pht(
'The minimum supported version of Mercurial is 2.4, which was '.
'released in 2012.'),
);
break;
}

59
src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
View file

@ -136,28 +136,33 @@ final class DiffusionHistoryQueryConduitAPIMethod
// stop history (this is more consistent with the Mercurial worldview of
// branches).
$path_args = array();
if (strlen($path)) {
$path_arg = csprintf('%s', $path);
$path_args[] = $path;
$revset_arg = hgsprintf(
'reverse(ancestors(%s))',
$commit_hash);
} else {
$path_arg = '';
$revset_arg = hgsprintf(
'reverse(ancestors(%s)) and branch(%s)',
$drequest->getBranch(),
$commit_hash);
}
$hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg');
if ($hg_analyzer->isMercurialTemplatePnodeAvailable()) {
$hg_log_template = '{node} {p1.node} {p2.node}\\n';
} else {
$hg_log_template = '{node} {p1node} {p2node}\\n';
}
list($stdout) = $repository->execxLocalCommand(
'log --debug --template %s --limit %d --rev %s -- %C',
'{node};{parents}\\n',
'log --template %s --limit %d --rev %s -- %Ls',
$hg_log_template,
($offset + $limit), // No '--skip' in Mercurial.
$revset_arg,
$path_arg);
$path_args);
$stdout = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(
$stdout);
$lines = explode("\n", trim($stdout));
$lines = array_slice($lines, $offset);
@ -166,36 +171,18 @@ final class DiffusionHistoryQueryConduitAPIMethod
$last = null;
foreach (array_reverse($lines) as $line) {
// In the event additional log output is included in future mercurial
// updates, if the line does not contain any semi-colon then log it and
// ignore it.
if (strpos($line, ';') === false) {
phlog(pht(
'Unexpected output from mercurial "log --debug" command: %s',
$line));
continue;
$parts = explode(' ', trim($line));
$hash = $parts[0];
$parents = array_slice($parts, 1, 2);
foreach ($parents as $parent) {
if (!preg_match('/^0+\z/', $parent)) {
$parent_map[$hash][] = $parent;
}
}
list($hash, $parents) = explode(';', $line);
$parents = trim($parents);
if (!$parents) {
if ($last === null) {
$parent_map[$hash] = array('...');
} else {
$parent_map[$hash] = array($last);
}
} else {
$parents = preg_split('/\s+/', $parents);
foreach ($parents as $parent) {
list($plocal, $phash) = explode(':', $parent);
if (!preg_match('/^0+$/', $phash)) {
$parent_map[$hash][] = $phash;
}
}
// This may happen for the zeroth commit in repository, both hashes
// are "000000000...".
if (empty($parent_map[$hash])) {
$parent_map[$hash] = array('...');
}
// This may happen for the zeroth commit in repository, both hashes
// are "000000000...".
if (empty($parent_map[$hash])) {
$parent_map[$hash] = array('...');
}
// The rendering code expects the first commit to be "mainline", like

47
src/applications/diffusion/protocol/__tests__/DiffusionMercurialCommandEngineTests.php
View file

@ -3,6 +3,53 @@
final class DiffusionMercurialCommandEngineTests extends PhabricatorTestCase {
public function testFilteringDebugOutput() {
$map = array(
'' => '',
"quack\n" => "quack\n",
"ignoring untrusted configuration option x.y = z\nquack\n" =>
"quack\n",
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"quack\n" =>
"quack\n",
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"quack\n" =>
"quack\n",
"quack\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n" =>
"quack\n",
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"duck\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"bread\n".
"ignoring untrusted configuration option x.y = z\n".
"quack\n" =>
"duck\nbread\nquack\n",
"ignoring untrusted configuration option x.y = z\n".
"duckignoring untrusted configuration option x.y = z\n".
"quack" =>
'duckquack',
);
foreach ($map as $input => $expect) {
$actual = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(
$input);
$this->assertEqual($expect, $actual, $input);
}
// Output that should be filtered out from the results
$output =
"ignoring untrusted configuration option\n".

36
src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php
View file

@ -6,11 +6,26 @@ final class DiffusionMercurialBlameQuery extends DiffusionBlameQuery {
$repository = $request->getRepository();
$commit = $request->getCommit();
// NOTE: We're using "--debug" to make "--changeset" give us the full
// commit hashes.
// NOTE: Using "--template" or "--debug" to get the full commit hashes.
$hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg');
if ($hg_analyzer->isMercurialAnnotateTemplatesAvailable()) {
// See `hg help annotate --verbose` for more info on the template format.
// Use array of arguments so the template line does not need wrapped in
// quotes.
$template = array(
'--template',
"{lines % '{node}: {line}'}",
);
} else {
$template = array(
'--debug',
'--changeset',
);
}
return $repository->getLocalCommandFuture(
'annotate --debug --changeset --rev %s -- %s',
'annotate %Ls --rev %s -- %s',
$template,
$commit,
$path);
}
@ -26,6 +41,21 @@ final class DiffusionMercurialBlameQuery extends DiffusionBlameQuery {
$lines = phutil_split_lines($stdout);
foreach ($lines as $line) {
// If the `--debug` flag was used above instead of `--template` then
// there's a good change additional output was included which is not
// relevant to the information we want. It should be safe to call this
// regardless of whether we used `--debug` or `--template` so there isn't
// a need to track which argument was used.
$line = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(
$line);
// Just in case new versions of Mercurial add arbitrary output when using
// the `--debug`, do a quick sanity check that this line is formatted in
// a way we're expecting.
if (strpos($line, ':') === false) {
phlog(pht('Unexpected output from hg annotate: %s', $line));
continue;
}
list($commit) = explode(':', $line, 2);
$result[] = $commit;
}

26
src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php
View file

@ -47,23 +47,23 @@ final class DiffusionLowLevelParentsQuery
private function loadMercurialParents() {
$repository = $this->getRepository();
list($stdout) = $repository->execxLocalCommand(
'log --debug --limit 1 --template={parents} --rev %s',
$this->identifier);
$hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg');
if ($hg_analyzer->isMercurialTemplatePnodeAvailable()) {
$hg_log_template = '{p1.node} {p2.node}';
} else {
$hg_log_template = '{p1node} {p2node}';
}
$stdout = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(
$stdout);
list($stdout) = $repository->execxLocalCommand(
'log --limit 1 --template %s --rev %s',
$hg_log_template,
$this->identifier);
$hashes = preg_split('/\s+/', trim($stdout));
foreach ($hashes as $key => $value) {
// Mercurial parents look like "23:ad9f769d6f786fad9f76d9a" -- we want
// to strip out the local rev part.
list($local, $global) = explode(':', $value);
$hashes[$key] = $global;
// With --debug we get 40-character hashes but also get the "000000..."
// hash for missing parents; ignore it.
if (preg_match('/^0+$/', $global)) {
// We get 40-character hashes but also get the "000000..." hash for
// missing parents; ignore it.
if (preg_match('/^0+\z/', $value)) {
unset($hashes[$key]);
}
}

49
src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php
View file

@ -100,55 +100,6 @@ final class PhabricatorRepositoryTestCase
}
public function testFilterMercurialDebugOutput() {
$map = array(
'' => '',
"quack\n" => "quack\n",
"ignoring untrusted configuration option x.y = z\nquack\n" =>
"quack\n",
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"quack\n" =>
"quack\n",
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"quack\n" =>
"quack\n",
"quack\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n" =>
"quack\n",
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"duck\n".
"ignoring untrusted configuration option x.y = z\n".
"ignoring untrusted configuration option x.y = z\n".
"bread\n".
"ignoring untrusted configuration option x.y = z\n".
"quack\n" =>
"duck\nbread\nquack\n",
"ignoring untrusted configuration option x.y = z\n".
"duckignoring untrusted configuration option x.y = z\n".
"quack" =>
'duckquack',
);
foreach ($map as $input => $expect) {
$actual = DiffusionMercurialCommandEngine::filterMercurialDebugOutput(
$input);
$this->assertEqual($expect, $actual, $input);
}
}
public function testRepositoryShortNameValidation() {
$good = array(
'sensible-repository',