Support for git submodules

[kristos80]

Hi there and thanks for this wonderful project.

It seems that submodules are not supported.

I have a directory structure as such

.git
|_ modules
   |_ plugin
      |_ ... actual git files
|_ plugin
   |_ .git
   |_ vendor
   |_ ... module files
... root repo files

Where the /plugin/.git file contains the standard module entry: gitdir: ../.git/modules/plugin

If I try to run the vendor/bin/roave-backward-compatibility-check command from within the plugin directory, I get the error: Directory "/plugin" is not a GIT repository, because as far as I can understand /plugin/.git in my case needs to be a directory and not a file.

I can see that a possible patch would be to have a statement as such $sourceRepo = CheckedOutRepository::fromModuleFileOrPath(Env\current_dir()) in AssertBackwardsCompatible::execute where the CheckedOutRepository code could include something as such:

public static function fromModuleFileOrPath(string $path): self {
  if($fromModuleFile = self::fromModuleFile($path)) {
	return $fromModuleFile;
  }

  return self::fromPath($path);
}

public static function fromModuleFile(string $path): ?self {
  if(Psl\Filesystem\is_file("$path/.git")) {
	$content = Psl\File\read("$path/.git");
	// Check that content contains a map to an actual git repo and get it
	// 1. It starts with the gitdir: wording (?)
	// 2. Grab and normalize the content after the gitdir: and assign it to an $actualPath variable
	// 3. Is it relative? If yes concatenate the two paths "$path/$actualPath" otherwise replace $path with $actualPath
	// 4. Does the map folder exists and contains a .git folder?
	// 5. Return new self($path);
  }

  return NULL;
}

I could try to create a PR but I am not sure that the proposed approach fits in your current design

[Ocramius]

Are you able to perhaps make a gist with what you have as a project? Could it be that / is just interpreted as an absolute path start?

[kristos80]

Sure.

Here's a root project with a submodule:

https://github.com/kristos80/test-root-project

The submodule: https://github.com/kristos80/test-submodule in my local setup contains a .git file with that content:

gitdir: ../.git/modules/test-submodule

If I now run vendor/bin/roave-backward-compatibility-check from within the folder containing the submodule, I get the error: Directory "/test-root-project/test-submodule" is not a GIT repository.

[Ocramius]

Ah, I see what's happening :-)

Indeed, vendor/bin/roave-backward-compatibility-check does not download git submodules, nor run any commands around them: it's just a much more complicated problem space.

This bit here is cloning your project to a temporary location, then performing a clean composer install there to analyze it:

https://github.com/Roave/BackwardCompatibilityCheck/blob/92d33e416d59d316dc174ba675baeb54ffa9e483/src/Command/AssertBackwardsCompatible.php#L142C1-L150

That does not include any kind of git submodule handling.

[kristos80]

I am sorry if I don't get it correctly, but I think we are talking about something else :-)

Indeed, vendor/bin/roave-backward-compatibility-check does not download git submodules, nor run any commands around them: it's just a much more complicated problem space.

There's no need to download any submodule, as I am not running the command from the root project but rather from within the submodule:

• I clone the root repo
• Download submodule(s)
• cd to the submodule folder
• Install updates
• Run vendor/bin/roave-backward-compatibility-check
• Since I am within a submodule folder the .git folder does not exist but rather a .git file which points to the root repo git folder and this is where the following lines throw the error

BackwardCompatibilityCheck/src/Command/AssertBackwardsCompatible.php

Line 116 in 92d33e4

$sourceRepo = CheckedOutRepository::fromPath(Env\current_dir());

BackwardCompatibilityCheck/src/Git/CheckedOutRepository.php

Line 21 in 92d33e4

Psl\invariant(Filesystem\is_directory($path . '/.git'), 'Directory "%s" is not a GIT repository.', $path);

and not

https://github.com/Roave/BackwardCompatibilityCheck/blob/92d33e416d59d316dc174ba675baeb54ffa9e483/src/Command/AssertBackwardsCompatible.php#L142C1-L150

as it never reaches that code

If I add this very crappy code (protect your eyes people) and run vendor/bin/roave-backward-compatibility-check it gets executed correctly:

File /vendor/roave/backward-compatibility-check/src/Command/AssertBackwardsCompatible.php

// Instead of $sourceRepo = CheckedOutRepository::fr(Env\current_dir());
$sourceRepo = CheckedOutRepository::fromModuleFileOrPath(Env\current_dir());

File: /vendor/roave/backward-compatibility-check/src/Git/CheckedOutRepository.php

	public static function fromModuleFileOrPath(string $path): self {
		if($fromModuleFile = self::fromModuleFile($path)) {
			return $fromModuleFile;
		}

		return self::fromPath($path);
	}

	private static function fromModuleFile(string $path): ?self {
		if(Psl\Filesystem\is_file("$path/.git")) {
			$content = Psl\File\read("$path/.git");
			if(str_contains($content, "gitdir:")) {
				$modulePath = trim(str_replace("gitdir:", "", $content));
				$path = "$path/$modulePath";
				return new self($path);
			}
		}

		return NULL;
	}

I hope I made myself clear!

[Ocramius]

@kristos80 ah, I see, but that's only the first problem then, no?

The problems are two, if I understand correctly:

1. this project seems to assume $CWD/.git and $CWD/composer.json exist
2. this project will not recursively clone submodules during analysis

Instead of building support for this into this particular project, I'd probably rather document the extension points to make this work, as this is really scratching the edges of what's needed.

• $CWD/.git and $CWD/composer.json not being at the same level is something I am open for improving.
• submodules is something I wouldn't want to touch, as the rabbit hole goes deep (into other version control systems, even)

[kristos80]

@Ocramius Yes I am talking about the first problem exclusively. To be honest I cannot see how the second case could be solved, since modules are not part of the composer dependencies and can only be understood at the git level and it would require a vast amount of work as you say.

Now getting back to the first problem. Yes, the problem lies in the case where the .git folder is not at the same level with the composer.json and instead there's a .git file which maps to the actual git folder.

From all the info I could gather around regarding git modules here are some (probable) facts about them:

• Submodule Directory: Inside the submodule directory, there is a .git file, which is a plain text file instead of being a folder
• Content of the .git File: This .git file contains a path to the superproject's* git folder where resides the submodule's actual git folder. It typically looks like this: gitdir: ../.git/modules/submodule-name
• I cannot find more info if the gitdir contains always a relative or it can, also, be an absolute path, which makes things more complicated for the latter, especially as, in my case, you need to run everything inside a container, where the original files would contain values given from the host. So if there's to create a solution, it would work (relatively) easy only in the case that the .git file would contain a relative path to the actual git folder.

*When a project contains submodules it's called 'superproject' in git terms: https://git-scm.com/docs/gitsubmodules

From the all the above if I were to add support for submodules, I would propose something like the previous code I sent:

• Check if there's a .git file instead of a folder
• If that's the case, do the following steps or otherwise fallback to the current handling
• Is the .git file an actual submodule .git file? Check (regex? some other string function?) that its content is in the form gitdir: $ACTUAL_MODULE_PATH and grab its value
• Check if $ACTUAL_MODULE_PATH is either relative or absolute
• If relative, append it to the $path variable after some form of normalisation
• If absolute, set $path to that value
• Run the standard CheckedOutRepository::fromPath($path) method

Nevertheless my case might be a very edgy one as I need to do testing (simultaneously) on two different levels at once:

• Unit/coverage, mutation, static analysis, backward compatibility (per module)
• Integration (in a superproject with all modules residing within it)

So it is very convenient to have a container to test everything at once, instead of having to test everything independently especially when all modules are in development phase as well.

I would be happy to propose a PR, but I am not sure if I can compete your level :-)

[Ocramius]

@kristos80 before going in depth in implementation, it is valuable to represent this in a test case.

See https://github.com/Roave/BackwardCompatibilityCheck/tree/8.8.x/test/asset/located-sources

We'd probably create a dummy project in there, with no dependencies, but so that we can point our suit at it and verify that BC breaks are caught 🤔

[kristos80]

@Ocramius sounds like a plan. I'll try my best to contribute in any way I can, as well

[Ocramius]

@kristos80 just to clear some expectations here: I'm unlikely able to implement the change myself in the next weeks.

What you can do to help out is starting with the e2e test case in a PR.

Worth perhaps replicating

BackwardCompatibilityCheck/test/unit/LocateSources/LocateSourcesViaComposerJsonTest.php

Line 27 in 92d33e4

($this->locateSources)(__DIR__ . '/../../asset/located-sources/composer-definition-with-everything'),

with an example of nested composer.json at first?

I think we can probably expand the installation source to recurse parent directories until .git is found 🤔

[kristos80]

@Ocramius yes I can definitely do that in the next couple of days and if I need anything, I will ping you

[kristos80]

@Ocramius tests in my mac have errors and failures.:

1. Error (with 3 failures all of the same nature):

1) RoaveE2ETest\BackwardCompatibility\Command\AssertBackwardsCompatibleTest::testWillRunSuccessfullyOnNoBcBreaks
...
[BC] CHANGED: Value of constant TestArtifact\TheClass::UNCHANGED_CONSTANT changed from '/private/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-fc94ec8ccb35bf3e6656c092336f55de5260bec36634d07366a07/src' to '/private/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-e18dbfbf3a4e3d008adb7fec8ad5cc949bfc45326634d0737cebd/src'
    1 backwards-incompatible changes detected

I guess the __DIR__ constant is calculated differently (due to the nature of tmp folder in OSX?) even if the revision code remains the same. If I change this to something less dynamic, like PHP_EOL or PHP_SAPI, it gets resolved.

2. Failure:

1) RoaveTest\BackwardCompatibility\Git\GitCheckoutRevisionToTemporaryPathTest::testCheckoutDirectoryIsCorrect
Failed asserting that '/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-428327492a803b6e0c612b157a67a50a472754616634d27cb5b44' contains "/tmp/api-compare-".

This is due to the fact that OSX uses the $TMPDIR which resolves to something like /var/folders/xl/84p38nhj405frmrkdpqb3v9c0000gn/T/. If I remove the "/tmp" for the string getting compared, it gets resolved as well.

Should I include those in the PR as well?

[Ocramius]

@kristos80 yes please, a PR with the new file structure for the tests is already awesome 👍

[Ocramius]

Failed asserting that '/var/folders/s7/gkw_9kg564zcw9f7531j6xrr0000gn/T/api-compare-428327492a803b6e0c612b157a67a50a472754616634d27cb5b44' contains "/tmp/api-compare-".

This could be hardened via realpath() around sys_get_tmp_dir() in the test, but running in docker is sufficient to me, since I don't maintain OSX tests, and don't want to anyway (the workers are too slow/too few)