Page MenuHomePhabricator
Create Task
Maniphest T361997

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

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™.

Related Objects
Search...

Event Timeline

Aklapper triaged this task as Low priority.Apr 6 2024, 11:55 AM
Aklapper created this task.
Aklapper updated the task description. (Show Details)Apr 6 2024, 11:57 AM
Aklapper added a comment.EditedApr 6 2024, 11:59 AM

P59764 T361997
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)

Aklapper added a comment.Apr 6 2024, 12:08 PM

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)Apr 6 2024, 12:49 PM
Aklapper added a parent task: T151965: Make "Clone" button show Gerrit clone link instead of two Diffusion clone links.
Aklapper edited projects, added Phabricator (Upstream), Upstream; removed Phabricator.Apr 11 2024, 12:07 PM
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 mentioned this in T347408: Do not expose dead git-ssh.wikimedia.org service as repo Clone URLs (defined in diffusion.ssh-host setting).Apr 16 2024, 4:47 PM
Aklapper added a parent task: T347408: Do not expose dead git-ssh.wikimedia.org service as repo Clone URLs (defined in diffusion.ssh-host setting).
Aklapper mentioned this in T347476: Set security.require-https to true.Apr 16 2024, 4:51 PM
Aklapper added a parent task: T347476: Set security.require-https to true.
Aklapper edited projects, added Phabricator; removed Upstream, Phabricator (Upstream).May 17 2024, 12:51 PM
Aklapper updated the task description. (Show Details)May 17 2024, 1:29 PM
Aklapper changed the subtype of this task from "Task" to "Feature Request".May 17 2024, 1:34 PM
CodeReviewBot added a project: Patch-For-Review.May 17 2024, 1:34 PM

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

Expose only canonical Clone URI in Diffusion when repo hosted externally

Aklapper added a comment.May 17 2024, 1:36 PM

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

CodeReviewBot added a comment.May 27 2024, 11:03 AM

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

Expose only canonical Clone URI in Diffusion when repo hosted externally

Aklapper edited projects, added Phabricator (2024-05-28); removed Patch-For-Review, Phabricator.May 27 2024, 11:04 AM
Aklapper mentioned this in T151965: Make "Clone" button show Gerrit clone link instead of two Diffusion clone links.May 28 2024, 4:41 PM
Aklapper closed this task as Resolved.May 28 2024, 4:45 PM

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

Aklapper mentioned this in T244907: Reduce the number of six default URIs in Diffusion.Aug 14 2024, 5:37 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits