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 useful 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(); + } + $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,37 @@ $ref->setTypes($types); } + // Some software like Subversion does not really support a branch. + // We must inform every "ref" (ArcanistRepositoryRef) about this support, + // since they generate the destination URI also thanks to this info. + $api_supports_branches = null; + if ($this->hasRepositoryAPI()) { + $api_supports_branches = $this->getRepositoryAPI()->supportsBranches(); + foreach ($refs as $ref) { + $ref->setBranchSupported($api_supports_branches); + } + } + + // Allow users to specify a (different) remote branch in which the file is + // opened on the web (if branches are supported by the VCS). $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 @@ -60,20 +60,45 @@ PhutilTypeSpec::checkMap( $params, array( + // Path to the file or directory. 'path' => 'optional string|null', + // Destination branch. + // When the branch is null, we guess the default (e.g. 'master' in git). 'branch' => 'optional string|null', + // Branch support capability. + // If this is false, you are probably using svn. + 'branchSupported' => 'optional bool|null', + // Repository root path. + // If this is non-null, you are probably using svn. In svn, this path + // can be '' for the root, or 'trunk', or 'tags/something' etc. + 'repoRootPath' => 'optional string|null', + // Lines to be highlighted. + // This can be null (no line), line '2' or lines '2-3'. 'lines' => 'optional string|null', )); + $params_with_str_nonempty = array( + 'path' => 1, + 'branch' => 1, + 'lines' => 1, + ); + + // For all arguments, drop NULL, + // For some arguments, also drop empty strings. foreach ($params as $key => $value) { - if ($value === null || !strlen($value)) { + if ( + $value === null + || (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 +107,24 @@ $uri_base = coalesce($this->browseURI, ''); $uri_base = rtrim($uri_base, '/'); - $uri_branch = phutil_escape_uri_path_component($params['branch']); + // Build the Subversion repo root path, or empty string. + $uri_root = ''; + if ($params['repoRootPath'] !== null) { + $uri_root = trim($params['repoRootPath'], '/'); + $uri_root = phutil_escape_uri($uri_root); // Don't escape slashes. + if ($uri_root !== '') { + $uri_root .= '/'; + } + } + + // Build the branch part of the URI. In git this usually becomes 'master/'. + // If available, it must end with a slash. + $uri_branch = ''; + if ($params['branchSupported']) { + $uri_branch = coalesce($params['branch'], $this->getDefaultBranch()); + $uri_branch = phutil_escape_uri_path_component($uri_branch); // Escape '/' + $uri_branch .= '/'; + } $uri_path = ltrim($params['path'], '/'); $uri_path = phutil_escape_uri($uri_path); @@ -92,10 +134,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.