diff --git a/src/applications/config/check/PhabricatorBinariesSetupCheck.php b/src/applications/config/check/PhabricatorBinariesSetupCheck.php --- a/src/applications/config/check/PhabricatorBinariesSetupCheck.php +++ b/src/applications/config/check/PhabricatorBinariesSetupCheck.php @@ -120,17 +120,11 @@ 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; } diff --git a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php --- a/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php +++ b/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php @@ -136,28 +136,33 @@ // 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,37 +171,19 @@ $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; - } - 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('...'); + $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; } } + // 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 // Git. Flip the order so it does the right thing. diff --git a/src/applications/diffusion/protocol/__tests__/DiffusionMercurialCommandEngineTests.php b/src/applications/diffusion/protocol/__tests__/DiffusionMercurialCommandEngineTests.php --- a/src/applications/diffusion/protocol/__tests__/DiffusionMercurialCommandEngineTests.php +++ b/src/applications/diffusion/protocol/__tests__/DiffusionMercurialCommandEngineTests.php @@ -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". diff --git a/src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php b/src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php --- a/src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php +++ b/src/applications/diffusion/query/blame/DiffusionMercurialBlameQuery.php @@ -6,11 +6,26 @@ $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 @@ $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; } diff --git a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php --- a/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php +++ b/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php @@ -47,23 +47,23 @@ private function loadMercurialParents() { $repository = $this->getRepository(); + $hg_analyzer = PhutilBinaryAnalyzer::getForBinary('hg'); + if ($hg_analyzer->isMercurialTemplatePnodeAvailable()) { + $hg_log_template = '{p1.node} {p2.node}'; + } else { + $hg_log_template = '{p1node} {p2node}'; + } + list($stdout) = $repository->execxLocalCommand( - 'log --debug --limit 1 --template={parents} --rev %s', + 'log --limit 1 --template %s --rev %s', + $hg_log_template, $this->identifier); - $stdout = DiffusionMercurialCommandEngine::filterMercurialDebugOutput( - $stdout); - $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]); } } diff --git a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php --- a/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php +++ b/src/applications/repository/storage/__tests__/PhabricatorRepositoryTestCase.php @@ -100,55 +100,6 @@ } - 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',