Skip to content

Separate git directory support - #201

Open
chris114782 wants to merge 3 commits into
gitonomy:1.3from
chris114782:feature/separate-git-directory-support
Open

Separate git directory support#201
chris114782 wants to merge 3 commits into
gitonomy:1.3from
chris114782:feature/separate-git-directory-support

Conversation

@chris114782

Copy link
Copy Markdown

In #200 I highlighted that the library does not support submodules.

This is because in a submodule does not store its .git folder inside the checked out repository, instead .git is a file that has a reference to the actual location of the .git directory.

This is documented here

You can manually reproduce this by doing git clone --separate-git-dir=/somewhere/else your-repo-url

This PR adds support for such a set up by detecting that .git is a file instead of a directory and attempting to read the path to the real directory from it.

@chris114782

Copy link
Copy Markdown
Author

I was aware of the failing formatting check, but that is existing bad formatting and I was loathe to change formatting outside of code I was modifying myself.

@lyrixx

lyrixx commented May 22, 2023

Copy link
Copy Markdown
Member

Can you rebase your PR? thanks

@chris114782
chris114782 force-pushed the feature/separate-git-directory-support branch from 2694243 to 71b85b8 Compare July 4, 2023 12:59
if (!preg_match('/^gitdir: ?(.+)$/', file_get_contents($realGitDir . '/.git'), $matches)) {
throw new InvalidArgumentException(sprintf('Directory "%s" contains a .git file, but it is not in the expected format', $realGitDir));
}
$foundGitPath = realpath($realGitDir . DIRECTORY_SEPARATOR . $matches[1]);

@tsterker tsterker Feb 26, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris114782 This assumes that the gitdir is a relative path, which might not be true.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try and take a look this weekend, it's been a few months (nearly a year now!) since I looked at this and I need to get my head back into it.

@lyrixx

lyrixx commented Sep 7, 2026

Copy link
Copy Markdown
Member

Thanks for working on this! I tested the actual code against real repositories (git init/clone --separate-git-dir=..., git worktree add, git submodule add) and found a few issues that I think block merging as-is.

Blocking: the feature doesn't work for its own headline use case

Repository::initDir() (src/Gitonomy/Git/Repository.php:173):

$foundGitPath = realpath($realGitDir . DIRECTORY_SEPARATOR . $matches[1]);

This blindly concatenates the checkout directory with the path read from the .git file. But git init/clone --separate-git-dir=<path> and git worktree add — the two use cases this PR is meant to support — always write an absolute path after gitdir: (verified with hexdump). So the join produces a bogus path like /checkout//tmp/elsewhere, realpath() returns false, and the code throws InvalidArgumentException: ... the directory it points to cannot be found even though the target directory exists.

Only the submodule case works, because git submodule add happens to write a relative path. The two cases actually named in the PR description don't.

The added test never exercises the new code path

  • AbstractTest::createFoobarRepository()'s $args (containing --separate-git-dir=...) are only used inside if (null === self::$localRepository) — but self::$localRepository is a shared static cache, and provideFoobar() calls the plain variant before the (false, true) variant, so by the time the separate-git-dir call runs the cache is already populated and the branch is skipped. Even on a cold cache, the $repository object actually returned to the test (self::$localRepository->cloneTo(...)) is never given those args at all.
  • Independently, tempnam(sys_get_temp_dir(), 'gitlib_') (line 81) pre-creates an empty file at that path, and git refuses to clone into a --separate-git-dir path that already exists and isn't an empty directory (fatal: repository path '...' already exists and is not an empty directory). createTempDir() in the same file already has the correct tempnam() + unlink() + mkdir() pattern for this exact problem, but it wasn't reused here.

I confirmed empirically that running BlameTest (which uses provideFoobar) passes 6/6 without ever touching initDir's new is_file() branch — so the blocking bug above ships with no test coverage at all.

  • createEmptyRepository($bare = true, $separateGitDir = false) (line 42) also adds a $separateGitDir parameter that's never read in the method body — dead code.

Other real bugs

  • Admin::cloneTo (src/Gitonomy/Git/Admin.php:107) — array_merge($args, $bare ? ['--bare'] : []). Since $bare defaults to true, any caller adding --separate-git-dir=... via the new $args parameter without also passing $bare = false hits fatal: options '--bare' and '--separate-git-dir' cannot be used together (verified), surfaced as an opaque RuntimeException.
  • CRLF handling (Repository.php:170) — /^gitdir: ?(.+)$/ doesn't exclude \r, so a CRLF-terminated .git file leaves a trailing \r in the captured path, breaking realpath() for an otherwise valid setup.
  • A permission error reading .git (e.g. chmod 000) gets silently turned into "not in the expected format" instead of surfacing the real I/O issue, since file_get_contents() returning false just makes preg_match fail to match.

Minor / cleanup

  • DIRECTORY_SEPARATOR (line 173) is inconsistent with the hardcoded / used everywhere else in the same method and codebase.
  • $realGitDir . '/.git' is built three separate times instead of once.
  • --bare is now appended three different ways across Admin::init, cloneTo, and cloneBranchTo.
  • The new .git-pointer parsing (~10 lines: regex, realpath, two exceptions) is inlined into initDir() rather than factored into a small, independently testable method — which is probably why the absolute-path and CRLF bugs went unnoticed.

Suggested direction

In initDir(), resolve $matches[1] as absolute-or-relative-to-$realGitDir (and trim the captured value) instead of unconditionally joining. For the test fixture, reuse the existing tempnam()+unlink()+mkdir() pattern from createTempDir(), and make sure the separate-git-dir args actually reach the repository object returned to the test — right now the feature this PR adds isn't actually verified by CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants