diff --git a/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md b/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md index ae20632..6c076a2 100644 --- a/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md +++ b/skill-data/drupalorg-cli/references/gitlab-mr-contribution.md @@ -30,6 +30,17 @@ drupalorg issue:setup-remote This is idempotent: if the remote already exists it skips the `git remote add` step and always runs `git fetch` to update remote refs. +The project that owns the fork is resolved in this order: + +1. An explicit qualifier (`project#` or a work-item URL). No Drupal.org + request is made. +2. The `project/` remote of the current repository, checked against the + Drupal.org node when one exists. A mismatch is an error. +3. The Drupal.org node lookup. + +GitLab work-item ids on migrated projects can collide with unrelated Drupal.org +node ids, so pass `project#` for work items whenever you know the project. + ### 3. Check out an issue branch ```bash diff --git a/skills/drupalorg-cli/references/gitlab-mr-contribution.md b/skills/drupalorg-cli/references/gitlab-mr-contribution.md index ae20632..6c076a2 100644 --- a/skills/drupalorg-cli/references/gitlab-mr-contribution.md +++ b/skills/drupalorg-cli/references/gitlab-mr-contribution.md @@ -30,6 +30,17 @@ drupalorg issue:setup-remote This is idempotent: if the remote already exists it skips the `git remote add` step and always runs `git fetch` to update remote refs. +The project that owns the fork is resolved in this order: + +1. An explicit qualifier (`project#` or a work-item URL). No Drupal.org + request is made. +2. The `project/` remote of the current repository, checked against the + Drupal.org node when one exists. A mismatch is an error. +3. The Drupal.org node lookup. + +GitLab work-item ids on migrated projects can collide with unrelated Drupal.org +node ids, so pass `project#` for work items whenever you know the project. + ### 3. Check out an issue branch ```bash diff --git a/src/Api/Action/Issue/GetIssueForkAction.php b/src/Api/Action/Issue/GetIssueForkAction.php index 8079a6a..97a610d 100644 --- a/src/Api/Action/Issue/GetIssueForkAction.php +++ b/src/Api/Action/Issue/GetIssueForkAction.php @@ -5,6 +5,7 @@ use mglaman\DrupalOrg\Action\ActionInterface; use mglaman\DrupalOrg\Client; use mglaman\DrupalOrg\GitLab\Client as GitLabClient; +use mglaman\DrupalOrg\IssueProjectResolver; use mglaman\DrupalOrg\Result\Issue\IssueForkResult; class GetIssueForkAction implements ActionInterface @@ -15,12 +16,21 @@ public function __construct( ) { } - public function __invoke(string $nid, ?string $projectMachineName = null): IssueForkResult - { - if ($projectMachineName === null) { - $issue = $this->client->getNode($nid); - $projectMachineName = $issue->fieldProjectMachineName; - } + /** + * @param string|null $projectMachineName + * Project from an explicit qualifier; skips the Drupal.org lookup. + * @param string|null $repositoryProject + * Project of the git repository the command runs in. + * + * @see IssueProjectResolver for the resolution order. + */ + public function __invoke( + string $nid, + ?string $projectMachineName = null, + ?string $repositoryProject = null, + ): IssueForkResult { + $projectMachineName = (new IssueProjectResolver($this->client)) + ->resolve($nid, $projectMachineName, $repositoryProject); $remoteName = $projectMachineName . '-' . $nid; $gitLabProjectPath = 'issue/' . $remoteName; diff --git a/src/Api/Action/Issue/SetupIssueRemoteAction.php b/src/Api/Action/Issue/SetupIssueRemoteAction.php index 3321384..842890c 100644 --- a/src/Api/Action/Issue/SetupIssueRemoteAction.php +++ b/src/Api/Action/Issue/SetupIssueRemoteAction.php @@ -17,10 +17,19 @@ public function __construct( ) { } - public function __invoke(string $nid): SetupIssueRemoteResult - { + /** + * @param string|null $projectMachineName + * Project from an explicit qualifier; skips the Drupal.org lookup. + * @param string|null $repositoryProject + * Project of the git repository the remote is added to. + */ + public function __invoke( + string $nid, + ?string $projectMachineName = null, + ?string $repositoryProject = null, + ): SetupIssueRemoteResult { $getFork = new GetIssueForkAction($this->client, $this->gitLabClient); - $fork = $getFork($nid); + $fork = $getFork($nid, $projectMachineName, $repositoryProject); $remoteName = $fork->remoteName; $sshUrl = $fork->sshUrl; diff --git a/src/Api/GitLab/WorkItemRef.php b/src/Api/GitLab/WorkItemRef.php index e03d3d6..e81ca1d 100644 --- a/src/Api/GitLab/WorkItemRef.php +++ b/src/Api/GitLab/WorkItemRef.php @@ -23,6 +23,17 @@ public function __construct( ) { } + /** + * The Drupal.org project machine name, e.g. "campaign" for "project/campaign". + */ + public function projectMachineName(): string + { + if (str_starts_with($this->projectPath, 'project/')) { + return substr($this->projectPath, strlen('project/')); + } + return basename($this->projectPath); + } + public static function tryParse(string $input): ?self { $input = trim($input); diff --git a/src/Api/IssueProjectResolver.php b/src/Api/IssueProjectResolver.php new file mode 100644 index 0000000..35142fe --- /dev/null +++ b/src/Api/IssueProjectResolver.php @@ -0,0 +1,86 @@ +resolveAgainstRepository($nid, $repositoryProject); + } + + try { + $nodeProject = $this->client->getNode($nid)->fieldProjectMachineName; + } catch (\RuntimeException $e) { + throw new \RuntimeException( + sprintf('%s %s', $e->getMessage(), self::qualifierHint($nid)), + 0, + $e + ); + } + if ($nodeProject === '') { + throw new \RuntimeException(sprintf( + 'Could not resolve a project for issue %s. %s', + $nid, + self::qualifierHint($nid) + )); + } + return $nodeProject; + } + + private function resolveAgainstRepository(string $nid, string $repositoryProject): string + { + try { + $nodeProject = $this->client->getNode($nid)->fieldProjectMachineName; + } catch (\RuntimeException) { + // Not a Drupal.org issue node (for example a migrated work item), + // so the repository is the only source for the project. + return $repositoryProject; + } + + if ($nodeProject === '' || $nodeProject === $repositoryProject) { + return $repositoryProject; + } + + throw new \RuntimeException(sprintf( + 'Issue %1$s belongs to project "%2$s" on Drupal.org, but this repository is project "%3$s". ' + . 'Pass %3$s#%1$s for the GitLab work item or %2$s#%1$s for the Drupal.org issue.', + $nid, + $nodeProject, + $repositoryProject + )); + } + + private static function qualifierHint(string $nid): string + { + return sprintf('Pass the project explicitly as project#%s.', $nid); + } +} diff --git a/src/Api/ProjectRemote.php b/src/Api/ProjectRemote.php index 4db9ed0..cf3b4ff 100644 --- a/src/Api/ProjectRemote.php +++ b/src/Api/ProjectRemote.php @@ -4,6 +4,8 @@ namespace mglaman\DrupalOrg; +use Symfony\Component\Process\Process; + /** * The Drupal.org project a git remote URL points at. */ @@ -27,4 +29,56 @@ public static function tryParse(string $remoteUrl): ?self } return new self($matches['name']); } + + /** + * Picks the project remote from a repository's remotes. "origin" wins + * when it matches; issue forks and personal forks never match. + * + * @param array $remoteUrls + * Fetch URLs keyed by remote name. + */ + public static function fromRemotes(array $remoteUrls): ?self + { + if (isset($remoteUrls['origin'])) { + $fromOrigin = self::tryParse($remoteUrls['origin']); + if ($fromOrigin !== null) { + return $fromOrigin; + } + } + foreach ($remoteUrls as $url) { + $remote = self::tryParse($url); + if ($remote !== null) { + return $remote; + } + } + return null; + } + + /** + * Detects the project from the remotes of the repository at $cwd. + * Returns null outside a git repository or when no remote matches. + */ + public static function detect(?string $cwd = null): ?self + { + $process = new Process(['git', 'remote', '-v'], $cwd); + $process->run(); + if (!$process->isSuccessful()) { + return null; + } + return self::fromRemotes(self::parseRemoteList($process->getOutput())); + } + + /** + * @return array + */ + private static function parseRemoteList(string $output): array + { + $remoteUrls = []; + foreach (explode("\n", $output) as $line) { + if (preg_match('/^(\S+)\s+(\S+)\s+\(fetch\)$/', trim($line), $matches) === 1) { + $remoteUrls[$matches[1]] = $matches[2]; + } + } + return $remoteUrls; + } } diff --git a/src/Cli/Command/Issue/Checkout.php b/src/Cli/Command/Issue/Checkout.php index ebe342d..611da4e 100644 --- a/src/Cli/Command/Issue/Checkout.php +++ b/src/Cli/Command/Issue/Checkout.php @@ -4,6 +4,7 @@ use mglaman\DrupalOrg\Action\Issue\GetIssueForkAction; use mglaman\DrupalOrg\Action\Issue\SetupIssueRemoteAction; +use mglaman\DrupalOrg\ProjectRemote; use mglaman\DrupalOrg\GitLab\Client as GitLabClient; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -25,8 +26,10 @@ protected function configure(): void protected function execute(InputInterface $input, OutputInterface $output): int { $gitLabClient = new GitLabClient(); + $explicitProject = $this->explicitProjectMachineName(); + $repositoryProject = ProjectRemote::detect()?->machineName; $action = new GetIssueForkAction($this->client, $gitLabClient); - $fork = $action($this->nid); + $fork = $action($this->nid, $explicitProject, $repositoryProject); // Verify the remote exists locally; offer to set it up if missing. $checkRemote = new Process(['git', 'remote', 'get-url', $fork->remoteName]); @@ -58,7 +61,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } try { $setupAction = new SetupIssueRemoteAction($this->client, $gitLabClient); - $setupResult = $setupAction($this->nid); + $setupResult = $setupAction($this->nid, $explicitProject, $repositoryProject); } catch (\RuntimeException $e) { $this->stdErr->writeln( sprintf('Failed to set up remote: %s', $e->getMessage()) @@ -69,7 +72,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int sprintf('Remote %s added and fetched.', $setupResult->remoteName) ); // Refresh fork data after setup so branches are populated. - $fork = $action($this->nid); + $fork = $action($this->nid, $explicitProject, $repositoryProject); } else { // Remote already exists; fetch to ensure tracking refs are up-to-date. $fetchProcess = new Process(['git', 'fetch', $fork->remoteName]); diff --git a/src/Cli/Command/Issue/GetFork.php b/src/Cli/Command/Issue/GetFork.php index 99e022a..0c056d9 100644 --- a/src/Cli/Command/Issue/GetFork.php +++ b/src/Cli/Command/Issue/GetFork.php @@ -31,10 +31,7 @@ protected function configure(): void protected function execute(InputInterface $input, OutputInterface $output): int { $action = new GetIssueForkAction($this->client, new GitLabClient()); - $machineName = $this->workItemRef !== null - ? substr($this->workItemRef->projectPath, strlen('project/')) - : null; - $result = $action($this->nid, $machineName); + $result = $action($this->nid, $this->explicitProjectMachineName()); $format = (string) $this->stdIn->getOption('format'); if ($this->writeFormatted($result, $format)) { diff --git a/src/Cli/Command/Issue/IssueCommandBase.php b/src/Cli/Command/Issue/IssueCommandBase.php index 72f014d..4206753 100644 --- a/src/Cli/Command/Issue/IssueCommandBase.php +++ b/src/Cli/Command/Issue/IssueCommandBase.php @@ -68,6 +68,14 @@ protected function initialize( } } + /** + * The project named by the nid argument's qualifier, if one was given. + */ + protected function explicitProjectMachineName(): ?string + { + return $this->workItemRef?->projectMachineName(); + } + /** * Initializes repository for current directory. */ diff --git a/src/Cli/Command/Issue/SetupRemote.php b/src/Cli/Command/Issue/SetupRemote.php index d83879f..36a475b 100644 --- a/src/Cli/Command/Issue/SetupRemote.php +++ b/src/Cli/Command/Issue/SetupRemote.php @@ -3,6 +3,7 @@ namespace mglaman\DrupalOrgCli\Command\Issue; use mglaman\DrupalOrg\Action\Issue\SetupIssueRemoteAction; +use mglaman\DrupalOrg\ProjectRemote; use mglaman\DrupalOrg\GitLab\Client as GitLabClient; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -21,7 +22,7 @@ protected function configure(): void protected function execute(InputInterface $input, OutputInterface $output): int { $action = new SetupIssueRemoteAction($this->client, new GitLabClient()); - $result = $action($this->nid); + $result = $action($this->nid, $this->explicitProjectMachineName(), ProjectRemote::detect()?->machineName); if ($result->alreadyExists) { $this->stdOut->writeln( diff --git a/tests/src/Action/Issue/GetIssueForkActionTest.php b/tests/src/Action/Issue/GetIssueForkActionTest.php index f1341f0..48f300c 100644 --- a/tests/src/Action/Issue/GetIssueForkActionTest.php +++ b/tests/src/Action/Issue/GetIssueForkActionTest.php @@ -64,6 +64,38 @@ public function testForkWithBranches(): void self::assertSame(['3383637-test-issue', 'main'], $result->branches); } + public function testExplicitProjectSkipsNodeLookup(): void + { + $client = $this->createMock(Client::class); + $client->expects(self::never())->method('getNode'); + + $gitLabClient = $this->createMock(GitLabClient::class); + $gitLabClient->method('getProject')->willThrowException(new \Exception('Not Found', 404)); + + $action = new GetIssueForkAction($client, $gitLabClient); + $result = $action('3615648', 'campaign'); + + self::assertSame('campaign-3615648', $result->remoteName); + self::assertSame('git@git.drupal.org:issue/campaign-3615648.git', $result->sshUrl); + self::assertSame('issue/campaign-3615648', $result->gitLabProjectPath); + } + + public function testRepositoryProjectForWorkItem(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode') + ->willThrowException(new \RuntimeException('Node 3615635 was not found on Drupal.org.')); + + $gitLabClient = $this->createMock(GitLabClient::class); + $gitLabClient->method('getProject')->willThrowException(new \Exception('Not Found', 404)); + + $action = new GetIssueForkAction($client, $gitLabClient); + $result = $action('3615635', null, 'campaign'); + + self::assertSame('campaign-3615635', $result->remoteName); + self::assertSame('issue/campaign-3615635', $result->gitLabProjectPath); + } + public function testForkNotYetCreated(): void { $client = $this->createMock(Client::class); diff --git a/tests/src/GitLab/WorkItemRefTest.php b/tests/src/GitLab/WorkItemRefTest.php new file mode 100644 index 0000000..3d44706 --- /dev/null +++ b/tests/src/GitLab/WorkItemRefTest.php @@ -0,0 +1,56 @@ +projectPath); + self::assertSame($expectedId, $ref->issueId); + self::assertSame($expectedMachineName, $ref->projectMachineName()); + } + + /** + * @return array + */ + public static function parseProvider(): array + { + return [ + 'shorthand' => ['campaign#3615648', 'project/campaign', 3615648, 'campaign'], + 'project path' => ['project/campaign#3615648', 'project/campaign', 3615648, 'campaign'], + 'work item URL' => [ + 'https://git.drupalcode.org/project/campaign/-/work_items/3615635', + 'project/campaign', + 3615635, + 'campaign', + ], + 'issue URL' => [ + 'https://git.drupalcode.org/project/canvas/-/issues/3591806', + 'project/canvas', + 3591806, + 'canvas', + ], + 'bare nid' => ['3615648', null, null, null], + 'empty' => ['', null, null, null], + 'unknown URL' => ['https://git.drupalcode.org/project/campaign', null, null, null], + ]; + } +} diff --git a/tests/src/IssueProjectResolverTest.php b/tests/src/IssueProjectResolverTest.php new file mode 100644 index 0000000..35a5e8b --- /dev/null +++ b/tests/src/IssueProjectResolverTest.php @@ -0,0 +1,133 @@ +createMock(Client::class); + $client->expects(self::never())->method('getNode'); + + $resolver = new IssueProjectResolver($client); + + self::assertSame('campaign', $resolver->resolve('3615648', 'campaign')); + self::assertSame('campaign', $resolver->resolve('3615648', 'campaign', 'sdx')); + } + + public function testRepositoryProjectWinsWhenNodeIsMissing(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode') + ->willThrowException(new \RuntimeException('Node 3615635 was not found on Drupal.org.')); + + $resolver = new IssueProjectResolver($client); + + self::assertSame('campaign', $resolver->resolve('3615635', null, 'campaign')); + } + + public function testRepositoryProjectConfirmedByNode(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode')->willReturn(self::makeIssueNode('3383637', 'campaign')); + + $resolver = new IssueProjectResolver($client); + + self::assertSame('campaign', $resolver->resolve('3383637', null, 'campaign')); + } + + public function testRepositoryProjectWinsWhenNodeHasNoProject(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode')->willReturn(self::makeIssueNode('3383637', '')); + + $resolver = new IssueProjectResolver($client); + + self::assertSame('campaign', $resolver->resolve('3383637', null, 'campaign')); + } + + public function testCollisionBetweenNodeAndRepositoryFails(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode')->willReturn(self::makeIssueNode('3615648', 'sdx')); + + $resolver = new IssueProjectResolver($client); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage( + 'Issue 3615648 belongs to project "sdx" on Drupal.org, but this repository is project "campaign". ' + . 'Pass campaign#3615648 for the GitLab work item or sdx#3615648 for the Drupal.org issue.' + ); + $resolver->resolve('3615648', null, 'campaign'); + } + + public function testBareNidUsesNodeProject(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode')->with('3383637')->willReturn(self::makeIssueNode('3383637', 'drupal')); + + $resolver = new IssueProjectResolver($client); + + self::assertSame('drupal', $resolver->resolve('3383637')); + } + + public function testBareNidWithoutProjectFails(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode')->willReturn(self::makeIssueNode('3591806', '')); + + $resolver = new IssueProjectResolver($client); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage( + 'Could not resolve a project for issue 3591806. Pass the project explicitly as project#3591806.' + ); + $resolver->resolve('3591806'); + } + + public function testBareNidForMissingNodeFailsWithHint(): void + { + $client = $this->createMock(Client::class); + $client->method('getNode') + ->willThrowException(new \RuntimeException('Node 3615635 was not found on Drupal.org.')); + + $resolver = new IssueProjectResolver($client); + + $this->expectException(\RuntimeException::class); + $this->expectExceptionMessage( + 'Node 3615635 was not found on Drupal.org. Pass the project explicitly as project#3615635.' + ); + $resolver->resolve('3615635'); + } +} diff --git a/tests/src/ProjectRemoteTest.php b/tests/src/ProjectRemoteTest.php index fd97ed2..828564c 100644 --- a/tests/src/ProjectRemoteTest.php +++ b/tests/src/ProjectRemoteTest.php @@ -24,6 +24,7 @@ public static function remoteUrlProvider(): array 'trailing newline from git output' => ["git@git.drupal.org:project/json_form_widget.git\n", 'json_form_widget'], 'github remote' => ['git@github.com:mglaman/drupalorg-cli.git', null], 'issue fork' => ['git@git.drupal.org:issue/json_form_widget-3000000.git', null], + 'personal fork' => ['git@git.drupal.org:mglaman/campaign.git', null], 'empty' => ['', null], ]; } @@ -33,4 +34,38 @@ public function testTryParse(string $remoteUrl, ?string $expected): void { self::assertSame($expected, ProjectRemote::tryParse($remoteUrl)?->machineName); } + + public function testOriginWinsOverOtherRemotes(): void + { + $remote = ProjectRemote::fromRemotes([ + 'upstream' => 'git@git.drupal.org:project/drupal.git', + 'origin' => 'https://git.drupalcode.org/project/campaign.git', + ]); + + self::assertSame('campaign', $remote?->machineName); + } + + public function testFallsBackToAnyProjectRemote(): void + { + $remote = ProjectRemote::fromRemotes([ + 'origin' => 'git@github.com:mglaman/campaign.git', + 'campaign-3615648' => 'git@git.drupal.org:issue/campaign-3615648.git', + 'drupal' => 'git@git.drupal.org:project/campaign.git', + ]); + + self::assertSame('campaign', $remote?->machineName); + } + + public function testNoProjectRemote(): void + { + self::assertNull(ProjectRemote::fromRemotes([])); + self::assertNull(ProjectRemote::fromRemotes([ + 'origin' => 'git@git.drupal.org:issue/campaign-3615648.git', + ])); + } + + public function testDetectOutsideRepository(): void + { + self::assertNull(ProjectRemote::detect(sys_get_temp_dir())); + } }