recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Never use "{branches}" in Mercurial

Summary:
Fixes T5304. Mercurial features a "{branches}" template keyword, documented as:

```
branches List of strings. The name of the branch on which the
changeset was committed. Will be empty if the branch name
was default.
```

At some time long in the past, I misinterpreted this to mean "list of branches where the branch head is a descendant of the commit". It is more like "list of zero or one elements, possibly containing the name of the branch the commit was originally made to, if that branch was not 'default'".

In fact, it seems like this is because a //very// long time in the past, Mercurial worked roughly like I expected:

> Ages ago (2005), we had a very different and ultimately unworkable
> approach to named branches that worked vaguely like .hgtags and allowed
> multiple branch names per revision.

http://marc.info/?l=mercurial-devel&m=129883069414855

This appears to be deprecated in modern Mercurial (it's not in the modern web documentation) although I can't find a commit about it so maybe that's just a documentation issue.

In any case, `{branches}` seems to never be useful: `{branch}` provides the same information without the awkward "default-if-empty" case.

Switch from `{branches}` to either `{branch}` (where that's good enough, notably in the hook engine) or `(descendants(%s) and head())`, which is equivalent to `--contains` in Git.

This fixes pushing to branches with spaces in their names, and makes the "Branches" / "Contains" queries moderately more consistent.

Test Plan:
- Pushed to a Mercurial branch with a space in it.
- Viewed list of branches in a Mercurial repository.
- Viewed containing branches of a Mercurial commit in Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5304

Differential Revision: https://secure.phabricator.com/D9453

epriestley 5660684d a4a8cfa6

+34 -48
+5 -30
src/applications/diffusion/conduit/ConduitAPI_diffusion_branchquery_Method.php
··· 62 62 $drequest = $this->getDiffusionRequest(); 63 63 $repository = $drequest->getRepository(); 64 64 65 - $refs = id(new DiffusionLowLevelMercurialBranchesQuery()) 66 - ->setRepository($repository) 67 - ->execute(); 65 + $query = id(new DiffusionLowLevelMercurialBranchesQuery()) 66 + ->setRepository($repository); 68 67 69 - // If we have a 'contains' query, filter these branches down to just the 70 - // ones which contain the commit. 71 68 $contains = $request->getValue('contains'); 72 69 if (strlen($contains)) { 73 - list($branches_raw) = $repository->execxLocalCommand( 74 - 'log --template %s --limit 1 --rev %s --', 75 - '{branches}', 76 - hgsprintf('%s', $contains)); 70 + $query->withContainsCommit($contains); 71 + } 77 72 78 - $branches_raw = trim($branches_raw); 79 - if (!strlen($branches_raw)) { 80 - $containing_branches = array('default'); 81 - } else { 82 - $containing_branches = explode(' ', $branches_raw); 83 - } 84 - 85 - $containing_branches = array_fuse($containing_branches); 86 - 87 - // NOTE: We get this very slightly wrong: a branch may have multiple 88 - // heads and we'll keep all of the heads of the branch, even if the 89 - // commit is only on some of the heads. This should be rare, is probably 90 - // more clear to users as is, and would potentially be expensive to get 91 - // right since we'd have to do additional checks. 92 - 93 - foreach ($refs as $key => $ref) { 94 - if (empty($containing_branches[$ref->getShortName()])) { 95 - unset($refs[$key]); 96 - } 97 - } 98 - } 73 + $refs = $query->execute(); 99 74 100 75 return $this->processBranchRefs($request, $refs); 101 76 }
+7 -11
src/applications/diffusion/engine/DiffusionCommitHookEngine.php
··· 688 688 foreach (array('old', 'new') as $key) { 689 689 $futures[$key] = $repository->getLocalCommandFuture( 690 690 'heads --template %s', 691 - '{node}\1{branches}\2'); 691 + '{node}\1{branch}\2'); 692 692 } 693 693 // Wipe HG_PENDING out of the old environment so we see the pre-commit 694 694 // state of the repository. ··· 697 697 $futures['commits'] = $repository->getLocalCommandFuture( 698 698 'log --rev %s --template %s', 699 699 hgsprintf('%s:%s', $hg_node, 'tip'), 700 - '{node}\1{branches}\2'); 700 + '{node}\1{branch}\2'); 701 701 702 702 // Resolve all of the futures now. We don't need the 'commits' future yet, 703 703 // but it simplifies the logic to just get it out of the way. ··· 978 978 $commits_lines = array_filter($commits_lines); 979 979 $commit_map = array(); 980 980 foreach ($commits_lines as $commit_line) { 981 - list($node, $branches_raw) = explode("\1", $commit_line); 982 - 983 - if (!strlen($branches_raw)) { 984 - $branches = array('default'); 985 - } else { 986 - $branches = explode(' ', $branches_raw); 987 - } 988 - 989 - $commit_map[$node] = $branches; 981 + list($node, $branch) = explode("\1", $commit_line); 982 + $commit_map[$node] = array($branch); 990 983 } 991 984 992 985 return $commit_map; ··· 1152 1145 case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: 1153 1146 return idx($this->gitCommits, $identifier, array()); 1154 1147 case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: 1148 + // NOTE: This will be "the branch the commit was made to", not 1149 + // "a list of all branch heads which descend from the commit". 1150 + // This is consistent with Mercurial, but possibly confusing. 1155 1151 return idx($this->mercurialCommits, $identifier, array()); 1156 1152 case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: 1157 1153 // Subversion doesn't have branches.
+22 -7
src/applications/diffusion/query/lowlevel/DiffusionLowLevelMercurialBranchesQuery.php
··· 6 6 final class DiffusionLowLevelMercurialBranchesQuery 7 7 extends DiffusionLowLevelQuery { 8 8 9 + private $contains; 10 + 11 + public function withContainsCommit($commit) { 12 + $this->contains = $commit; 13 + return $this; 14 + } 15 + 9 16 protected function executeQuery() { 10 17 $repository = $this->getRepository(); 11 18 12 - // NOTE: `--debug` gives us 40-character hashes. 19 + if ($this->contains !== null) { 20 + $spec = hgsprintf('(descendants(%s) and head())', $this->contains); 21 + } else { 22 + $spec = hgsprintf('head()'); 23 + } 24 + 13 25 list($stdout) = $repository->execxLocalCommand( 14 - '--debug branches'); 15 - $stdout = PhabricatorRepository::filterMercurialDebugOutput($stdout); 26 + 'log --template %s --rev %s', 27 + '{node}\1{branch}\2', 28 + $spec); 16 29 17 30 $branches = array(); 18 31 19 - $lines = ArcanistMercurialParser::parseMercurialBranches($stdout); 20 - foreach ($lines as $name => $spec) { 32 + $lines = explode("\2", $stdout); 33 + $lines = array_filter($lines); 34 + foreach ($lines as $line) { 35 + list($node, $branch) = explode("\1", $line); 21 36 $branches[] = id(new DiffusionRepositoryRef()) 22 - ->setShortName($name) 23 - ->setCommitIdentifier($spec['rev']); 37 + ->setShortName($branch) 38 + ->setCommitIdentifier($node); 24 39 } 25 40 26 41 return $branches;