Fix build path for absolute paths.

[ghost]

Q	A
Branch	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Documented?	no
Fixed tickets

When you create a git worktree of your project and then in that project perform a grumphp git:init my .git file contains an absolute path for gitdir: which is then appended to another absolute path: https://github.com/phpro/grumphp/blob/5b696233890ac35e4a8f8a8ef29cafc85669eb93/src/Locator/GitRepositoryDirLocator.php#L31-L41

Easy fix to first check if the path is absolute or not. Then return appended path or not accordingly.

Steps to reproduce:

• create a composer project with phpro/grumphp as a dependency in a git repository.
• git clone yourproject
• cd yourproject
• git worktree add -B yourprojectnewtree
• composer install -d yourprojectnewtree

Whenever you run grumphp git:init (which it does on composer install) it will concatenate two absolute paths. Example build here: https://github.com/verbruggenalex/gdc/runs/3883357864?check_suite_focus=true#step:6:464

[veewee]

Thanks for the PR. Can you add some steps to reproduce this issue?

I have the feeling that it might be better to do the absolute check in the locator instead of in the buildPath method. The buildPath is not meant for adding absolute paths to a base path.

[ghost]

Hi @veewee , I've updated the description with steps to reproduce. This is the command I'm running in the build: https://github.com/verbruggenalex/gdc/blob/main/taskman.yml#L15-L16

I added the check inside of buildPath() because I didn't want to check all the places where it's used. You're right that it doesn't really need to be in there. But it could still use a check to not try to concatenate two absolute paths.

[veewee]

Thanks, will have to look into it when I have more time.

I'dd assume that a buildPath('/base', '/dir') would resolve in /base/dir instead in /dir The symfony filesystem just checks the start of the string to see if it is an absolute path: https://github.com/symfony/filesystem/blob/343f4fe324383ca46792cae728a3b6e2f708fb32/Filesystem.php#L594-L603

So /dir would be an absolute path, even if it doesn't exist in the example above.

[veewee]

Sorry for the late response @verbruggenalex.

Just tested it out and it is indeed an issue. As mentioned above, I don't think that the fix above is a good idea. The function buildPath() should be used to build absolute paths based on 2 parts.

We already have a function for either building or using an absolute path. Basically what you implemented in this PR: https://github.com/phpro/grumphp/blob/master/src/Util/Filesystem.php#L44-L51

A proper fix would be to change the path building inside the GitRepositoryDirLocator class. Something like this:

return $this->filesystem->makePathAbsolute( 
   $gitRepositoryDir,
   dirname($gitDir)
); 

I also notice that we don't have test cases covering this specific class and all of its cases. Since we now already have 4 different cases, I'dd suggest to introduce a test that covers all of them.

[veewee]

Got fixed in https://github.com/phpro/grumphp/pull/1003