diff --git a/src/browse/query/ArcanistBrowsePathURIHardpointQuery.php b/src/browse/query/ArcanistBrowsePathURIHardpointQuery.php --- a/src/browse/query/ArcanistBrowsePathURIHardpointQuery.php +++ b/src/browse/query/ArcanistBrowsePathURIHardpointQuery.php @@ -55,10 +55,26 @@ $path = Filesystem::readablePath($full_path, $working_root); } + // Define the repository root path, probably just for Subversion. + $repo_root_path = null; + if ($ref->getBranchSupported() === false) { + // In software like SVN there is no native branch support. Really. + // So, in SVN, we use getBranchName() to get the SVN + // "branch", that is, the repository root path, and that's + // a root relative path of the repository, like 'trunk' or 'tags/lol'. + // Note that the empty string '' is a valid SVN repo root path + // that may be returned by "svn info". + $repo_root_path = $this + ->getRepositoryApi() + ->getBranchName(); // lol - not really a branch + } + $params = array( 'path' => $path, 'lines' => $lines, 'branch' => $ref->getBranch(), + 'branchSupported' => $ref->getBranchSupported(), + 'repoRootPath' => $repo_root_path, ); $uri = $repository_ref->newBrowseURI($params); diff --git a/src/browse/ref/ArcanistBrowseRef.php b/src/browse/ref/ArcanistBrowseRef.php --- a/src/browse/ref/ArcanistBrowseRef.php +++ b/src/browse/ref/ArcanistBrowseRef.php @@ -8,8 +8,23 @@ private $token; private $types = array(); + + /** + * Branch name (e.g. 'main') + * @var string|null + */ private $branch; + /** + * Branch support + * This is useful to avoid ambiguity, especially when + * the branch attribute is null. + * @var bool|null True: supported. + * False: unsupported. + * Null: unknown support. + */ + private $branchSupported; + public function getRefDisplayName() { return pht('Browse Query "%s"', $this->getToken()); } @@ -49,13 +64,44 @@ return !$this->types; } - public function setBranch($branch) { - $this->branch = $branch; + /** + * Set a branch name. + * See also setBranchSupported() and use it accordingly. + * @param string|null $branch Branch name like 'main' or 'master' + * @return self + */ + public function setBranch($branch) { + $this->branch = $branch; + return $this; + } + + /** + * Get the branch name. + * You may want to also check getBranchSupported() first. + * @return string|null Branch name like 'main' or 'master' + */ + public function getBranch() { + return $this->branch; + } + + /** + * Set if the server really supports branches, or not. + * Generally we assume yes. In Subversion it's a nope. + * @param bool|null $branch_supported + * @return self + */ + public function setBranchSupported($branch_supported) { + $this->branchSupported = $branch_supported; return $this; } - public function getBranch() { - return $this->branch; + /** + * Check if the server really supports branches, or not. + * Generally we assume yes. In Subversion it's a nope. + * @return bool|null If the info is unknown you may get null. + */ + public function getBranchSupported() { + return $this->branchSupported; } public function getURIs() { diff --git a/src/browse/workflow/ArcanistBrowseWorkflow.php b/src/browse/workflow/ArcanistBrowseWorkflow.php --- a/src/browse/workflow/ArcanistBrowseWorkflow.php +++ b/src/browse/workflow/ArcanistBrowseWorkflow.php @@ -100,10 +100,35 @@ $ref->setTypes($types); } + // Some software like Subversion does not really support a branch. + $api_supports_branches = null; + if ($this->hasRepositoryAPI()) { + $api_supports_branches = $this->getRepositoryAPI()->supportsBranches(); + foreach ($refs as $ref) { + $ref->setBranchSupported($api_supports_branches); + } + } + + // Allow the user to choose a remote branch, so the specified file + // is opened there on the web, if it has sense to do so. $branch = $this->getArgument('branch'); if ($branch) { - foreach ($refs as $ref) { - $ref->setBranch($branch); + if ($api_supports_branches) { + foreach ($refs as $ref) { + $ref->setBranch($branch); + } + } else { + // Dear Subversion users, + // we have a vague idea about what "--branch" should do in your context, + // and that idea makes no sense at the moment. Nothing personal! + // https://we.phorge.it/T15541 + $this->writeWarn( + pht('BRANCH OPTION NOT AVAILABLE'), + pht( + 'Argument "--branch" for "arc browse" is not available '. + 'in your software version control. For example, in SVN, '. + 'a branch is just a directory with a special meaning for '. + 'your team. Please omit the "--branch" argument.')); } } diff --git a/src/ref/ArcanistRepositoryRef.php b/src/ref/ArcanistRepositoryRef.php --- a/src/ref/ArcanistRepositoryRef.php +++ b/src/ref/ArcanistRepositoryRef.php @@ -62,18 +62,31 @@ array( 'path' => 'optional string|null', 'branch' => 'optional string|null', + 'branchSupported' => 'optional bool|null', + 'repoRootPath' => 'optional string|null', // This loves an empty string. 'lines' => 'optional string|null', )); + // For all arguments, drop NULL. + // For some arguments (indexed here), drop empty strings. + $params_with_str_nonempty = array( + 'path' => 1, + 'branch' => 1, + 'lines' => 1, + ); foreach ($params as $key => $value) { - if ($value === null || !strlen($value)) { + if ($value === null) { + unset($params[$key]); + } else if (isset($params_with_str_nonempty[$key]) && !strlen($value)) { unset($params[$key]); } } $defaults = array( 'path' => '/', - 'branch' => $this->getDefaultBranch(), + 'branch' => null, + 'branchSupported' => null, + 'repoRootPath' => null, 'lines' => null, ); @@ -82,7 +95,24 @@ $uri_base = coalesce($this->browseURI, ''); $uri_base = rtrim($uri_base, '/'); - $uri_branch = phutil_escape_uri_path_component($params['branch']); + // Build the repo root path of the URI. + // This is a feature used by Subversion to only clone a single + // sub-path like "trunk" or "tags/foo/whatever". + // It's unclear whenever the original value may end with a slash. + // Anyway, the final URI should end with just one slash (or be empty). + $uri_root = ''; + if ($params['repoRootPath'] !== null) { + $uri_root = trim($params['repoRootPath'], '/'); + $uri_root .= '/'; + } + + // Build the branch part of the URI. This usually becomes 'master/'. + $uri_branch = ''; + if ($params['branchSupported']) { + $uri_branch = coalesce($params['branch'], $this->getDefaultBranch()); + $uri_branch = phutil_escape_uri_path_component($uri_branch); + $uri_branch .= '/'; + } $uri_path = ltrim($params['path'], '/'); $uri_path = phutil_escape_uri($uri_path); @@ -92,10 +122,8 @@ $uri_lines = '$'.phutil_escape_uri($params['lines']); } - // TODO: This construction, which includes a branch, is probably wrong for - // Subversion. - - return "{$uri_base}/browse/{$uri_branch}/{$uri_path}{$uri_lines}"; + // This construction supports both git and Subversion. + return "{$uri_base}/browse/{$uri_root}{$uri_branch}{$uri_path}{$uri_lines}"; } public function getDefaultBranch() { diff --git a/src/repository/api/ArcanistRepositoryAPI.php b/src/repository/api/ArcanistRepositoryAPI.php --- a/src/repository/api/ArcanistRepositoryAPI.php +++ b/src/repository/api/ArcanistRepositoryAPI.php @@ -567,6 +567,20 @@ /* -( Base Commits )------------------------------------------------------- */ + /** + * In some environments (like Subversion) the concept of "branch" + * is not really integrated in any standard server and it's just + * a path on your local filesystem. Like "trunk/" etc. + * + * @return bool True if the branch is supported by this API. + */ + public function supportsBranches() { + // Assume a decent default. + // In the future we may want to make this method abstract. But not now, + // to avoid breaking changes in alien class extensions. + return true; + } + abstract public function supportsCommitRanges(); final public function setBaseCommit($symbolic_commit) { diff --git a/src/repository/api/ArcanistSubversionAPI.php b/src/repository/api/ArcanistSubversionAPI.php --- a/src/repository/api/ArcanistSubversionAPI.php +++ b/src/repository/api/ArcanistSubversionAPI.php @@ -624,6 +624,15 @@ return false; } + /** + * @return bool + */ + public function supportsBranches() { + // Even if you are the best SVN salesman, a little cute + // directory cannot really be considered a "branch" feature. + return false; + } + public function supportsCommitRanges() { return false; } diff --git a/src/utils/utils.php b/src/utils/utils.php --- a/src/utils/utils.php +++ b/src/utils/utils.php @@ -55,7 +55,7 @@ * * For example, `idxv($dict, array('a', 'b', 'c'))` accesses the key at * `$dict['a']['b']['c']`, if it exists. If it does not, or any intermediate - * value is not itself an array, it returns the defualt value. + * value is not itself an array, it returns the default value. * * @param array $map Array to access. * @param list $path List of keys to access, in sequence.