Page MenuHomePhabricator

Expose only canonical Clone URI in Diffusion when repo hosted externally and Differential disabled
Closed, ResolvedPublicFeature

Description

Upstream: https://we.phorge.it/T15780

Since T330797 we do not use Phabricator Differential anymore for code review.
Since T191182 we do not host any repositories canonically anymore in Phabricator Diffusion - all repositories are mirrored into Diffusion from another place (Gerrit, GitHub, GitLab, etc) where code development actually takes place outside of Phabricator.

Clicking on the "Clone" button in a Diffusion repository, it makes sense not encourage folks to clone code repositories from Phabricator Diffusion but instead from the canonical code repository location: That would allow developers to do developer stuff (proposing changes via pushing custom branches etc) on their journey into a bright development contribution future.

Screenshot from 2024-04-06 13-56-06.png (80×314 px, 4 KB)

Implementing this would also hide/fix T347408: Do not expose dead git-ssh.wikimedia.org service as repo Clone URLs (defined in diffusion.ssh-host setting) and T347476: Set security.require-https to true on the user side (though not really T244907: Reduce the number of six default URIs in Diffusion on the management side).

It would still be possible to clone from Diffusion (needed until non-public {T213246} and T349921: integration-agent-docker machines excessively pull some Wikibase related Git repos in Diffusion get resolved) - the repository URIs provided by Diffusion itself would just not be advertised anymore via the "Clone" button in Diffusion's repository web view.

Wondering whether this should go along with some diffusion.disallow-cloning-observed-repositories setting or if my argumentation is... convincing™.

Details

TitleReferenceAuthorSource BranchDest Branch
Expose only canonical Clone URI in Diffusion when repo hosted externallyrepos/phabricator/phabricator!53aklappert361997repoCloneUriswmf/stable
Customize query in GitLab

Event Timeline

Aklapper created this task.

1diff --git a/src/applications/diffusion/controller/DiffusionCloneController.php b/src/applications/diffusion/controller/DiffusionCloneController.php
2index bf118bcf6d..7378383b25 100644
3--- a/src/applications/diffusion/controller/DiffusionCloneController.php
4+++ b/src/applications/diffusion/controller/DiffusionCloneController.php
5@@ -23,24 +23,44 @@ final class DiffusionCloneController extends DiffusionController {
6 $warning = null;
7
8 $uris = $repository->getURIs();
9- foreach ($uris as $uri) {
10- if ($uri->getIsDisabled()) {
11- continue;
12- }
13
14- if ($uri->getEffectiveDisplayType() == $display_never) {
15- continue;
16+ // If Differential is uninstalled and the repo is observed (=not hosted),
17+ // only expose its external canonical URI, ignoring each URI's display
18+ // preferences and ignoring if the canonical URI to observe is enabled.
19+ if (!id(new PhabricatorDifferentialApplication())->isInstalled() &&
20+ !$repository->isHosted()) {
21+ foreach ($uris as $uri) {
22+ if ($uri->getEffectiveIOType() == PhabricatorRepositoryURI::IO_OBSERVE) {
23+ if ($repository->isSVN()) {
24+ $label = phutil_tag_div('diffusion-clone-label', pht('Checkout'));
25+ } else {
26+ $label = phutil_tag_div('diffusion-clone-label', pht('Clone'));
27+ }
28+ $view->addProperty(
29+ $label,
30+ $this->renderCloneURI($repository, $uri));
31+ break;
32+ }
33 }
34-
35- if ($repository->isSVN()) {
36- $label = phutil_tag_div('diffusion-clone-label', pht('Checkout'));
37- } else {
38- $label = phutil_tag_div('diffusion-clone-label', pht('Clone'));
39+ } else {
40+ foreach ($uris as $uri) {
41+ if ($uri->getIsDisabled()) {
42+ continue;
43+ }
44+
45+ if ($uri->getEffectiveDisplayType() == $display_never) {
46+ continue;
47+ }
48+
49+ if ($repository->isSVN()) {
50+ $label = phutil_tag_div('diffusion-clone-label', pht('Checkout'));
51+ } else {
52+ $label = phutil_tag_div('diffusion-clone-label', pht('Clone'));
53+ }
54+
55+ $view->addProperty(
56+ $label,
57+ $this->renderCloneURI($repository, $uri));
58 }
59-
60- $view->addProperty(
61- $label,
62- $this->renderCloneURI($repository, $uri));
63 }
64
65 if (!$view->hasAnyProperties()) {

(might welcome a little bit of refactoring but I'm too stupid for that)

Before:

Screenshot from 2024-04-06 14-06-10.png (506×1 px, 79 KB)

After (no matter if the GitHub URI is actually set as visible or not):
Screenshot from 2024-04-06 14-06-50.png (506×1 px, 54 KB)

Aklapper updated the task description. (Show Details)
Aklapper moved this task from Backlog to Upstreamed on the Phabricator (Upstream) board.
Aklapper moved this task from Backlog to Patch proposed upstream on the Upstream board.
Aklapper changed the subtype of this task from "Task" to "Feature Request".May 17 2024, 1:34 PM

Note to myself: Once merged and deployed, decline T347408, T347476, T151965.

aklapper merged https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/merge_requests/53

Expose only canonical Clone URI in Diffusion when repo hosted externally

This got deployed/resolved on 2024-05-28.