Changeset View
Standalone View
src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
| Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines | protected function getMercurialResult(ConduitAPIRequest $request) { | ||||
| // entirely to avoid these inconsistencies. | // entirely to avoid these inconsistencies. | ||||
| // NOTE: When viewing the history of a file, we don't use "-b", because | // NOTE: When viewing the history of a file, we don't use "-b", because | ||||
| // Mercurial stops history at the branchpoint but we're interested in all | // Mercurial stops history at the branchpoint but we're interested in all | ||||
| // ancestors. When viewing history of a branch, we do use "-b", and thus | // ancestors. When viewing history of a branch, we do use "-b", and thus | ||||
| // stop history (this is more consistent with the Mercurial worldview of | // stop history (this is more consistent with the Mercurial worldview of | ||||
| // branches). | // branches). | ||||
| $path_args = array(); | |||||
| if (strlen($path)) { | if (strlen($path)) { | ||||
| $path_arg = csprintf('%s', $path); | $path_args[] = $path; | ||||
| $revset_arg = hgsprintf( | $revset_arg = hgsprintf( | ||||
| 'reverse(ancestors(%s))', | 'reverse(ancestors(%s))', | ||||
| $commit_hash); | $commit_hash); | ||||
| } else { | } else { | ||||
| $path_arg = ''; | |||||
| $revset_arg = hgsprintf( | $revset_arg = hgsprintf( | ||||
| 'reverse(ancestors(%s)) and branch(%s)', | 'reverse(ancestors(%s)) and branch(%s)', | ||||
| $drequest->getBranch(), | $drequest->getBranch(), | ||||
| $commit_hash); | $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'; | |||||
epriestley: The loop below might become more readable now if the pattern is just `{node} {p1node} {p2node}`… | |||||
Done Inline ActionsI'll try coming up with something cspeckmim: I'll try coming up with something | |||||
| } | |||||
| list($stdout) = $repository->execxLocalCommand( | list($stdout) = $repository->execxLocalCommand( | ||||
| 'log --debug --template %s --limit %d --rev %s -- %C', | 'log --template %s --limit %d --rev %s -- %Ls', | ||||
| '{node};{parents}\\n', | $hg_log_template, | ||||
| ($offset + $limit), // No '--skip' in Mercurial. | ($offset + $limit), // No '--skip' in Mercurial. | ||||
| $revset_arg, | $revset_arg, | ||||
| $path_arg); | $path_args); | ||||
Not Done Inline ActionsNot your doing, but %C ("raw, unescaped comment") is unsafe in the general case. This particular use is safe, but it's vaguely desirable to use only as much %C as necessary, and it isn't strictly necessary here. When %Ls was first implemented, it did not accept empty lists, so it could not be used in this situation. It was later updated to emit nothing if the value passed is an empty array, so this could be rewritten more safely to use %Ls now: $path_args = array();
if (strlen($path)) {
$path_args[] = $path;
}
// ...
'log ... -- %Ls',
...,
$path_arg);epriestley: Not your doing, but `%C` ("raw, unescaped comment") is unsafe in the general case. This… | |||||
Done Inline ActionsWill do! cspeckmim: Will do! | |||||
| $stdout = DiffusionMercurialCommandEngine::filterMercurialDebugOutput( | |||||
| $stdout); | |||||
| $lines = explode("\n", trim($stdout)); | $lines = explode("\n", trim($stdout)); | ||||
| $lines = array_slice($lines, $offset); | $lines = array_slice($lines, $offset); | ||||
| $hash_list = array(); | $hash_list = array(); | ||||
| $parent_map = array(); | $parent_map = array(); | ||||
| $last = null; | $last = null; | ||||
| foreach (array_reverse($lines) as $line) { | foreach (array_reverse($lines) as $line) { | ||||
| // In the event additional log output is included in future mercurial | $parts = explode(' ', trim($line)); | ||||
Not Done Inline Actions(Per below, this is slightly cleaner as \z.) epriestley: (Per below, this is slightly cleaner as `\z`.) | |||||
Not Done Inline ActionsThis case might be impossible now, I think it was a trick of the behavior of {parents}? epriestley: This case might be impossible now, I think it was a trick of the behavior of `{parents}`? | |||||
Done Inline ActionsAs in some edge cases where the first commit of a repo wouldn't print out 0000 for the parents? I tested the first commit and it looks to print what we need always, and I'm not aware of another situation this could come up. I'll remove. cspeckmim: As in some edge cases where the first commit of a repo wouldn't print out `0000` for the… | |||||
Done Inline Actionsaccidentally removed the + when swapping in the \z cspeckmim: accidentally removed the `+` when swapping in the `\z` | |||||
Done Inline ActionsSo it turns out that the gaps in the display show up if you don't trim() this. Previously the entire line was trimmed -- maybe the trim should be added during the explode $parts = explode(' ', trim($line));? cspeckmim: So it turns out that the gaps in the display show up if you don't `trim()` this. Previously the… | |||||
| // updates, if the line does not contain any semi-colon then log it and | $hash = $parts[0]; | ||||
Not Done Inline ActionsThis array_filter() is possibly unnecessary -- I think both {p2node} and {p2.node} always emit a string with 000... for empty, right? epriestley: This `array_filter()` is possibly unnecessary -- I think both `{p2node}` and `{p2.node}` always… | |||||
Done Inline ActionsI was testing out different test cases in a php playground I noticed that with an input of qwer vs. qwer resulted in different values for $parts here, empty array vs. array with a single empty item so I did this as a precaution. But I think you're right that since we're passing an explicit template to include both that those edge cases don't come up $ hg log --template "{node} {p1.node} {p2.node}\n" --rev 0 48af2fc3b1a5ba27efd24041bc091f133551d61b 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 $ hg log --template "{node};{p1.node} {p2.node}\n" --rev 1 7a7d04be4a3ddea4f04fd3a84e921eec45f2bc04 48af2fc3b1a5ba27efd24041bc091f133551d61b 0000000000000000000000000000000000000000 (same exact output with {p1node} format cspeckmim: I was testing out different test cases in a php playground
http://sandbox.onlinephpfunctions. | |||||
| // ignore it. | $parents = array_slice($parts, 1, 2); | ||||
| 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) { | foreach ($parents as $parent) { | ||||
| list($plocal, $phash) = explode(':', $parent); | if (!preg_match('/^0+\z/', $parent)) { | ||||
| if (!preg_match('/^0+$/', $phash)) { | $parent_map[$hash][] = $parent; | ||||
| $parent_map[$hash][] = $phash; | |||||
| } | } | ||||
| } | } | ||||
| // This may happen for the zeroth commit in repository, both hashes | // This may happen for the zeroth commit in repository, both hashes | ||||
| // are "000000000...". | // are "000000000...". | ||||
| if (empty($parent_map[$hash])) { | if (empty($parent_map[$hash])) { | ||||
| $parent_map[$hash] = array('...'); | $parent_map[$hash] = array('...'); | ||||
| } | } | ||||
| } | |||||
| // The rendering code expects the first commit to be "mainline", like | // The rendering code expects the first commit to be "mainline", like | ||||
| // Git. Flip the order so it does the right thing. | // Git. Flip the order so it does the right thing. | ||||
| $parent_map[$hash] = array_reverse($parent_map[$hash]); | $parent_map[$hash] = array_reverse($parent_map[$hash]); | ||||
| $hash_list[] = $hash; | $hash_list[] = $hash; | ||||
| $last = $hash; | $last = $hash; | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 102 Lines • Show Last 20 Lines | |||||
The loop below might become more readable now if the pattern is just {node} {p1node} {p2node} with a single explode on spaces, but this is a quibble.