Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pick higher revision number to guarantee successful file retrieval #11350

Merged
merged 1 commit into from Mar 17, 2023

Conversation

AnrDaemon
Copy link
Contributor

SvnDriver::getTags and SvnDriver::getBranches both fail to compose valid reference to composer.json if repository root was moved after their creation. Pick highest available revision from the list to raise chances for success.

@AnrDaemon
Copy link
Contributor Author

$ svn ls -v https://svn.rootdir.org/misc/php-tools/library/branches/
   1034 anrdaemon              сен 14 2019 ./
   1005 anrdaemon              мар 11 2019 1.12/

$ svn log https://svn.rootdir.org/misc/php-tools/library/branches/1.12/ | head -n 10
------------------------------------------------------------------------
r1034 | anrdaemon | 2019-09-14 13:13:49 +0300 (Сб, 14 сен 2019) | 2 lines

= Moved remaining sources to the separate path.

------------------------------------------------------------------------
r1005 | anrdaemon | 2019-03-11 03:31:07 +0300 (Пн, 11 мар 2019) | 2 lines

. Import changelog.

$ svn cat https://svn.rootdir.org/misc/php-tools/library/branches/1.12/composer.json@1005
svn: warning: W160013: '/misc/!svn/rvr/1005/php-tools/library/branches/1.12/composer.json' path not found
svn: E200009: Could not cat all targets because some targets don't exist
svn: E200009: Illegal target for the requested operation

$ svn cat https://svn.rootdir.org/misc/php-tools/library/branches/1.12/composer.json@1034
{
…success

@AnrDaemon
Copy link
Contributor Author

I think it deserves a backport to LTS. Say if you need it rebased.

@AnrDaemon
Copy link
Contributor Author

As an afterthought, the revision is useless in the task of retrieving a single file. Thus the patch is really only 3 lines.

diff --git a/src/Composer/Repository/Vcs/SvnDriver.php b/src/Composer/Repository/Vcs/SvnDriver.php
index f2bfad0f7..371faaf13 100644
--- a/src/Composer/Repository/Vcs/SvnDriver.php
+++ b/src/Composer/Repository/Vcs/SvnDriver.php
@@ -180,15 +180,13 @@ public function getFileContent(string $file, string $identifier): ?string
         Preg::match('{^(.+?)(@\d+)?/$}', $identifier, $match);
         if (!empty($match[2])) {
             $path = $match[1];
-            $rev = $match[2];
         } else {
             $path = $identifier;
-            $rev = '';
         }

         try {
             $resource = $path.$file;
-            $output = $this->execute('svn cat', $this->baseUrl . $resource . $rev);
+            $output = $this->execute('svn cat', $this->baseUrl . $resource);
             if (!trim($output)) {
                 return null;
             }

@stof
Copy link
Contributor

stof commented Feb 24, 2023

The revision being useless to retrieve a single file looks like a weird argument to me. the whole point of a VCS system is to track changes of files over revisions. Saying that getting a file at any revision is equivalent implies that you never modify any file after adding them in your VCS.

@AnrDaemon
Copy link
Contributor Author

Except when you meet a VCS that follows different versioning model, than what you are used to in Git.
In Subversion, it is entirely possible to move a tree and be unable to refer to the file by @revision it is apparently attributed to, since it is a revision, in which it was changed, but not necessarily at the place where it is located now.

@stof
Copy link
Contributor

stof commented Feb 24, 2023

@AnrDaemon but ignoring revisions entirely does not seem the right way to fix this issue to me, as it ignores the fact that you wanted an older version of the file.

@AnrDaemon
Copy link
Contributor Author

Where do I want an older version, since at every point in code it refers to current one?

@AnrDaemon
Copy link
Contributor Author

To make it perfectly clear: in Subversion, every directory is a repository in itself. Thus, 'trunk', 'tags', 'branches' - merely a point of view. For VCS, they are all separate directories, with their own revision history.

@Seldaek Seldaek added this to the 2.6 milestone Mar 17, 2023
@Seldaek
Copy link
Member

Seldaek commented Mar 17, 2023

I'd rather not merge this as bugfix as I have no way to really try this out.. but let's do it in 2.6 as the change seems reasonable to me. Thanks

@Seldaek Seldaek merged commit 3b16937 into composer:main Mar 17, 2023
@Seldaek Seldaek added the Bug label Mar 17, 2023
@AnrDaemon
Copy link
Contributor Author

I will provide a test case once I get a few free hours. And I'm still leaning to the idea that specifying @rev is not relevant to what the code does. I'll check with my copy of svnbook.

@Seldaek
Copy link
Member

Seldaek commented Mar 17, 2023

Yes I believe you might be right there, but that's a bigger change that I am not so keen on making in this codepath that few people use as it might lead to regressions we won't notice for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants