Skip to content

fix(betterer 🐛): don't reinitialize an existing git repo (corrupts shared config in worktrees)#1251

Open
mattmorganpdx wants to merge 2 commits into
phenomnomnominal:masterfrom
mattmorganpdx:fix/git-init-worktree
Open

fix(betterer 🐛): don't reinitialize an existing git repo (corrupts shared config in worktrees)#1251
mattmorganpdx wants to merge 2 commits into
phenomnomnominal:masterfrom
mattmorganpdx:fix/git-init-worktree

Conversation

@mattmorganpdx

Copy link
Copy Markdown

What

BettererGitΩ._init() ran git init on every Betterer run. This PR skips it when we're already inside a git repository, and fixes the retry loop so init errors are actually surfaced.

Why

init() calls _findGitRoot() first, which throws unless a .git already exists — so by the time _init() runs, the repository is guaranteed to be initialized and the git init is redundant.

It's also harmful in git worktree setups. When Betterer runs inside a linked worktree (<worktree>/.git is a gitfile pointing into a shared common git dir), the redundant git init reinitializes against the common git dir and rewrites shared config — flipping core.bare and breaking every worktree on the repo.

This is reliably reproducible with the popular "bare clone + worktree" layout:

git clone --bare <url> repo.git        # core.bare = true
git -C repo.git worktree add ../wt main
# core.bare is still true here...
git -C ../wt init                      # Reinitialized existing Git repository
# core.bare in repo.git/config is now FALSE — every worktree is broken

Separately, the retry loop could never rethrow:

for (let i = 0; i < retries; i++) {
  try { await git.init(); }
  catch (error) { if (i >= retries) throw error; }  // i maxes at retries-1, so never true
}

so any git init failure (e.g. parallel workers colliding on the config lock — error: could not lock config file …/config: File exists) was silently swallowed.

Changes

  • packages/betterer/src/fs/git.ts: guard git.init() behind git.checkIsRepo() so an already-initialized repo is never reinitialized; fix the retry condition (i === retries - 1) and return on success.
private async _init(git: SimpleGit): Promise<void> {
  if (await git.checkIsRepo()) {
    return;
  }

  const retries = 3;
  for (let i = 0; i < retries; i++) {
    try {
      await git.init();
      return;
    } catch (error) {
      if (i === retries - 1) {
        throw error;
      }
    }
  }
}
  • test/worktree.spec.ts: new regression test that builds a bare-repo + linked-worktree layout, runs Betterer inside the worktree, and asserts the shared common-dir core.bare is untouched. It fails against the current code (core.bare flips true → false) and passes with this change.

Behavior impact

  • Normal (already-initialized) repos: git init is no longer invoked — previously a no-op, now skipped entirely. No functional change to Betterer's results.
  • Worktree / shared-common-dir repos: the shared config is no longer mutated, so core.bare can't be flipped.
  • Genuinely uninitialized directory: unchanged — init() still runs (and now correctly throws after exhausting retries instead of swallowing the error). Note that _findGitRoot() already throws before reaching here unless a .git exists, so this path is effectively unreachable today; the guard is defensive and self-documenting.

Note on v6

This targets the master (5.x) line, where the bug lives. The v6/nx rewrite already removed this code path — BettererGitΩ no longer calls git init and version control is only created in precommit mode — so there's nothing to port forward. This is purely a 5.x fix.

Possibly related: #1129 (same _findGitRoot/init area).

…ared config in worktrees)

`BettererGitΩ._init()` ran `git init` on every run, even though `_findGitRoot()`
has already established that we're inside a git repository. Re-initialising is
redundant, and in a linked-worktree setup (where `<worktree>/.git` is a gitfile
pointing into a shared common git dir) it reinitialises against the *common* git
dir and can rewrite shared config — flipping `core.bare` and breaking every
worktree on the repo (`fatal: this operation must be run in a work tree`).

Guard the init behind `git.checkIsRepo()` so an already-initialised repo is never
reinitialised. Also fix the retry loop, whose `i >= retries` guard could never be
true and so silently swallowed all `git init` errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
// linked worktree (where `<worktree>/.git` is a gitfile pointing into a shared common git dir)
// it reinitialises against the *common* git dir and can rewrite shared config, flipping
// `core.bare` and breaking every worktree on the repo:
if (await git.checkIsRepo()) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think the code is self explanatory here, would you mind removing the comments?

return;
} catch (error) {
if (i >= retries) {
if (i === retries - 1) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks, oops 😅

Comment thread test/worktree.spec.ts Outdated
const { paths, cleanup } = await createFixture('worktree', {});

const root = paths.cwd;
const sourcePath = path.posix.join(root, 'source');

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

the fixture should have utilities for making paths and writing files etc, which the cleanup will then handle.

Comment thread test/worktree.spec.ts Outdated
].join('\n')
);
await fs.writeFile(path.posix.join(sourcePath, 'src', 'index.ts'), `// HACK:`);
await git(sourcePath, 'init -q');

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Not a huge fan of the direct git calls, is it possible to do this with simple-git which should be available? I understand it's more complex git behaviour so it might just be necessary.

Comment thread test/worktree.spec.ts Outdated
it('should not reinitialise the repository when run inside a git worktree', async () => {
const { betterer } = await import('@betterer/betterer');

// The fixture directory is our scratch space. Inside it we build the popular "bare repo +

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Don't think the comments here are necessary either.

@phenomnomnominal

Copy link
Copy Markdown
Owner

The E2E tests failing is "normal" ftr

…ls and simple-git

Remove the explanatory comments from `_init` and the worktree test, write the
test fixtures via `createFixture` utilities (cleanup handles teardown), and
drive the git setup through `simple-git` instead of direct `exec` calls.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mattmorganpdx

Copy link
Copy Markdown
Author

Thanks for the quick review! Addressed in 2269888:

  • Removed the comments from _init and the test.
  • Test now uses the createFixture utilities for paths/files (cleanup handles teardown) instead of manual fs calls.
  • Switched the git setup to simple-git.init/.addConfig/.add/.commit/.revparse/.clone, with .raw(...) only for worktree add and reading core.bare since those don't have dedicated methods.

Re-verified it fails against the original _init and passes with the fix.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants