improve how discover detect a repo, for safety reasons

README.md

@@ -50,7 +50,7 @@ func main() {
 	// Find the repository and where git reads its configuration from.
 	env, err := gitconfig.Discover(".")
 	if err != nil {
-		log.Fatal(err) // ErrUntrusted, as git refuses it
+		log.Fatal(err) // ErrNotRepository, or ErrUntrusted as git refuses it
 	}
 
 	// Read the effective configuration: system, global, local, worktree

discover.go

@@ -94,23 +94,83 @@ func discover(dir string, env environ) (Env, error) {
 // it when started there, and the paths whose owner git checks. In a working
 // tree (dir holds .git, or is a "gitdir:" file), they are the working tree,
 // its .git and the git directory; elsewhere, dir alone. The first is the
-// path safe.directory lists.
+// path safe.directory lists. A git directory that checkGitDir refuses fails
+// with ErrNotRepository.
 func findRepo(dir string) (gitDir string, owned []string, err error) {
 	if dir, err = filepath.Abs(dir); err != nil {
 		return "", nil, err
 	}
 	dot := filepath.Join(dir, ".git")
 	if fi, err := os.Stat(dir); err == nil && fi.Mode().IsRegular() {
 		dir, dot = filepath.Dir(dir), dir
-	} else if _, err := os.Stat(dot); errors.Is(err, fs.ErrNotExist) {
-		// Also a directory that does not exist (yet): loading finds no
-		// files in it.
+	} else if _, err := os.Stat(dot); isMissing(err) {
+		if err := checkGitDir(dir); err != nil {
+			return "", nil, err
+		}
 		return dir, []string{dir}, nil
 	} else if err != nil {
 		return "", nil, &FileError{Path: dot, Err: err}
 	}
-	gitDir, err = resolveGitDir(dot)
-	return gitDir, []string{dir, dot, gitDir}, err
+	if gitDir, err = resolveGitDir(dot); err != nil {
+		return "", nil, err
+	}
+	if err := checkGitDir(gitDir); err != nil {
+		return "", nil, err
+	}
+	return gitDir, []string{dir, dot, gitDir}, nil
+}
+
+// checkGitDir fails with ErrNotRepository unless gitDir is a git directory
+// as git recognizes one: it holds a HEAD file, and its common directory
+// (gitDir itself, or the one its commondir file names in a linked worktree)
+// holds objects and refs directories. Any other directory would have its
+// config read and written as the local scope. HEAD may also be a symbolic
+// link to a branch (core.preferSymlinkRefs), whose target does not exist
+// while the branch is unborn or packed: as for git, the link's text must
+// start with "refs/". What HEAD holds is not checked, where git also
+// requires a ref or an object name.
+func checkGitDir(gitDir string) error {
+	head := filepath.Join(gitDir, "HEAD")
+	fi, err := os.Lstat(head)
+	if err != nil {
+		return notRepository(gitDir, err)
+	}
+	switch {
+	case fi.Mode().IsRegular():
+	case fi.Mode()&fs.ModeSymlink != 0:
+		target, err := os.Readlink(head)
+		if err != nil {
+			return &FileError{Path: head, Err: err}
+		}
+		if !strings.HasPrefix(filepath.ToSlash(target), "refs/") {
+			return &FileError{Path: gitDir, Err: ErrNotRepository}
+		}
+	default:
+		return &FileError{Path: gitDir, Err: ErrNotRepository}
+	}
+	r, err := Env{GitDir: gitDir}.repo()
+	if err != nil {
+		return err
+	}
+	for _, name := range []string{"objects", "refs"} {
+		fi, err := os.Stat(filepath.Join(r.commonDir, name))
+		if err != nil {
+			return notRepository(gitDir, err)
+		}
+		if !fi.IsDir() {
+			return &FileError{Path: gitDir, Err: ErrNotRepository}
+		}
+	}
+	return nil
+}
+
+// notRepository wraps the error met checking the git directory gitDir: a
+// missing path means it is not a repository.
+func notRepository(gitDir string, err error) error {
+	if isMissing(err) {
+		err = ErrNotRepository
+	}
+	return &FileError{Path: gitDir, Err: err}
 }
 
 // checkTrust fails with ErrUntrusted unless the current user owns every

doc.go

@@ -14,7 +14,7 @@
 // Reading the effective configuration of a repository and changing a value:
 //
 //	env, err := gitconfig.Discover(".") // the working tree
-//	if err != nil { ... }               // ErrUntrusted, as git refuses it
+//	if err != nil { ... }               // ErrNotRepository, or ErrUntrusted
 //
 //	cfg, err := env.Load()
 //	if err != nil { ... }

docs/COMPATIBILITY.md

@@ -76,6 +76,7 @@ in [GIT-BEHAVIOUR.md](GIT-BEHAVIOUR.md).
 | Repository ownership check | done when git finds the repository, not for `GIT_DIR` | done by `Discover`, not for an `Env` filled by hand | 1 (X5) |
 | What is checked, and listed in `safe.directory` | depends on where git starts: a working tree, or a git directory | the same, from the path given to `Discover` | 1 — the caller should pass the working tree, which users list |
 | Untrusted repository | `git config` silently skips the local scope; other commands fail | `Discover` fails with `ErrUntrusted` | a silently missing scope surprises; reading less is not safer than refusing — *safe default* |
+| `Discover` given a directory that is not a repository | git searches the parent directories, and fails with "not a git repository" when there is none | `ErrNotRepository`: a git directory must hold a `HEAD` file, and its common directory `objects/` and `refs/` directories, as for git, which also requires `HEAD` to hold a ref or an object name | `Discover` does not search; reading any directory's `config` as the local scope would read and write a file that is not git's — *safe default* |
 | `SUDO_UID` = `' 1000'`, `+1000`, or beyond 32 bits | accepted; a value beyond 32 bits wraps around (2³² + 1000 is 1000) | ignored: digits only, at most 32 bits | the wrap-around trusts a user nobody named — *safe default* |
 | Administrators group on Windows | trusted for an administrator (Git for Windows commit messages, 2022 and 2025) | the same | 1 — not observed: no Windows git to run but CI's |
 | UNC paths on Windows | must be listed in `safe.directory` since Git for Windows 2.54 (release notes) | owner checked like any path | the release note is not precise enough to follow; not observed |

docs/GIT-BEHAVIOUR.md

@@ -300,3 +300,37 @@ tree; started in a git directory, it checks that directory alone, and
 | the same, `/r/dots` owned by `bob`, `safe.directory=/r/dots` / `=/r/home` | accepted / refused, at `/r/dots` |
 | `--separate-git-dir=/r/sep/x` for `/r/w`, `/r/w` owned by `bob`, `git -C /r/sep/x` | accepted |
 | linked worktree `/r/lw` owned by `bob`, `git -C /r/w/.git/worktrees/lw` / `git -C /r/lw` | accepted / refused, at `/r/lw` |
+
+## What is a repository
+
+Observed with git 2.47.3, as `git rev-parse --git-dir` run in `/r/w` with
+`GIT_CEILING_DIRECTORIES=/r`. "Refused" means `fatal: not a git repository`.
+Unless stated, `/r/w/.git` holds `HEAD` (`ref: refs/heads/main`) and the
+directories `objects` and `refs`.
+
+| `/r/w/.git` | Result |
+|---|---|
+| as stated | accepted |
+| `HEAD` alone | refused |
+| no `objects`, or no `refs` | refused |
+| `objects` or `refs` a regular file | refused |
+| `objects` a symbolic link to a directory | accepted |
+| `HEAD` a directory | refused |
+| `HEAD` holding `garbage` | refused |
+| `HEAD` holding an object name | accepted |
+| `HEAD` a symbolic link to `refs/heads/main`, which does not exist | accepted |
+| `HEAD` a symbolic link to `refs/heads`, a directory | accepted |
+| `HEAD` a symbolic link to `refs/../../outside`, which does not exist | accepted: only the link's text is checked, which must start with `refs/` |
+| `HEAD` a symbolic link to `./refs/heads/main`, to `refs`, to `nothere`, or to the absolute path of `/r/w/.git/refs/heads/main` | refused |
+| `HEAD` a symbolic link to a directory outside `refs` (`/etc`) | refused |
+| created by `git init --ref-format=reftable`: `refs` is a directory holding a regular file `heads`, `HEAD` holds `ref: refs/heads/.invalid` | accepted |
+
+A linked worktree's git directory, `/r/w/.git/worktrees/lw`, holds `HEAD`
+and `commondir` (`../..`) but no `objects`:
+
+| Linked worktree `/r/lw` | Result |
+|---|---|
+| as created by `git worktree add` | accepted |
+| no `refs` in `worktrees/lw` | accepted |
+| no `refs`, or no `objects`, in `/r/w/.git` | refused |
+| no `commondir` | refused |

env.go

@@ -112,6 +112,13 @@ type CommandEntry struct {
 // followed. Finding the repository is left to the caller: Discover does not
 // search parent directories and does not read GIT_DIR. Pass "" when not in
 // a repository: only the system, global and command-line scopes exist then.
+// A dir that is not a repository fails with ErrNotRepository, in a
+// *FileError naming the git directory expected: its .git when it exists (or
+// the directory a gitdir file names), dir itself otherwise. As for git, a
+// git directory must hold a HEAD file, and its common directory objects and
+// refs directories; any other would have its config read and written as
+// the local scope. A caller that wants the repository only when there is
+// one can fall back to Discover("") on ErrNotRepository.
 //
 // The repository must be trusted, as git requires: owned by the current
 // user (on Unix, also the SUDO_UID user for root; on Windows, the

env_test.go

@@ -54,6 +54,8 @@ func initRepo(t testing.TB, gitDir, config string) {
 	t.Helper()
 	writeFile(t, filepath.Join(gitDir, "HEAD"), "ref: refs/heads/main\n")
 	writeFile(t, filepath.Join(gitDir, "config"), config)
+	require.NoError(t, os.MkdirAll(filepath.Join(gitDir, "objects"), 0o755))
+	require.NoError(t, os.MkdirAll(filepath.Join(gitDir, "refs"), 0o755))
 }
 
 // addWorktree registers a linked worktree named name in the repository at
@@ -1309,21 +1311,150 @@ func TestDiscover(t *testing.T) {
 		assert.Equal(t, gitDir, env.GitDir)
 	})
 
-	t.Run("working tree", func(t *testing.T) {
+	t.Run("HEAD a link to a branch with no loose file", func(t *testing.T) {
+		// core.preferSymlinkRefs: the branch is unborn or packed.
+		home := cleanGitEnv(t)
+		gitDir := filepath.Join(home, "repo", ".git")
+		initRepo(t, gitDir, "")
+		require.NoError(t, os.Remove(filepath.Join(gitDir, "HEAD")))
+		symlink(t, filepath.Join("refs", "heads", "main"), filepath.Join(gitDir, "HEAD"))
+		env, err := gitconfig.Discover(filepath.Join(home, "repo"))
+		require.NoError(t, err)
+		assert.Equal(t, gitDir, env.GitDir)
+	})
+
+	t.Run("reftable", func(t *testing.T) {
+		// As git init --ref-format=reftable: refs/heads is a file.
 		home := cleanGitEnv(t)
 		gitDir := filepath.Join(home, "repo", ".git")
 		initRepo(t, gitDir, "")
+		writeFile(t, filepath.Join(gitDir, "HEAD"), "ref: refs/heads/.invalid\n")
+		writeFile(t, filepath.Join(gitDir, "refs", "heads"), "this repository uses the reftable format\n")
+		require.NoError(t, os.MkdirAll(filepath.Join(gitDir, "reftable"), 0o755))
 		env, err := gitconfig.Discover(filepath.Join(home, "repo"))
 		require.NoError(t, err)
 		assert.Equal(t, gitDir, env.GitDir)
 	})
 
-	t.Run("missing directory", func(t *testing.T) {
-		// Nothing to check, nothing to load.
+	t.Run("links accepted", func(t *testing.T) {
+		home := cleanGitEnv(t)
+		// objects and refs links to directories.
+		initRepo(t, filepath.Join(home, "linkdirs", ".git"), "")
+		for _, name := range []string{"objects", "refs"} {
+			p := filepath.Join(home, "linkdirs", ".git", name)
+			require.NoError(t, os.Rename(p, p+"-real"))
+			symlink(t, name+"-real", p)
+		}
+		// HEAD a link to a directory under refs, and to a path that
+		// leaves refs: git checks only that the link's text starts with
+		// "refs/".
+		for dir, target := range map[string]string{
+			"headrefsdir": "refs/heads",
+			"headdotdot":  "refs/../../outside",
+		} {
+			gitDir := filepath.Join(home, dir, ".git")
+			initRepo(t, gitDir, "")
+			require.NoError(t, os.MkdirAll(filepath.Join(gitDir, "refs", "heads"), 0o755))
+			require.NoError(t, os.Remove(filepath.Join(gitDir, "HEAD")))
+			symlink(t, filepath.FromSlash(target), filepath.Join(gitDir, "HEAD"))
+		}
+
+		for _, dir := range []string{"linkdirs", "headrefsdir", "headdotdot"} {
+			env, err := gitconfig.Discover(filepath.Join(home, dir))
+			require.NoError(t, err, dir)
+			assert.Equal(t, filepath.Join(home, dir, ".git"), env.GitDir, dir)
+		}
+	})
+
+	t.Run("HEAD a link not starting with refs/", func(t *testing.T) {
 		home := cleanGitEnv(t)
-		env, err := gitconfig.Discover(filepath.Join(home, "missing"))
+		outside := filepath.Join(home, "outside")
+		require.NoError(t, os.MkdirAll(outside, 0o755))
+		for dir, target := range map[string]string{
+			"dotslash": filepath.FromSlash("./refs/heads/main"),
+			"refs":     "refs",
+			"dangling": "nothere",
+			"absolute": filepath.Join(home, "absolute", ".git", "refs", "heads", "main"),
+			"outdir":   outside,
+		} {
+			gitDir := filepath.Join(home, dir, ".git")
+			initRepo(t, gitDir, "")
+			writeFile(t, filepath.Join(gitDir, "refs", "heads", "main"), "")
+			require.NoError(t, os.Remove(filepath.Join(gitDir, "HEAD")))
+			symlink(t, target, filepath.Join(gitDir, "HEAD"))
+
+			_, err := gitconfig.Discover(filepath.Join(home, dir))
+			require.ErrorIs(t, err, gitconfig.ErrNotRepository, dir)
+			fe, ok := errors.AsType[*gitconfig.FileError](err)
+			require.True(t, ok, dir)
+			assert.Equal(t, gitDir, fe.Path, dir)
+		}
+	})
+
+	t.Run("working tree", func(t *testing.T) {
+		home := cleanGitEnv(t)
+		gitDir := filepath.Join(home, "repo", ".git")
+		initRepo(t, gitDir, "")
+		env, err := gitconfig.Discover(filepath.Join(home, "repo"))
 		require.NoError(t, err)
-		assert.Equal(t, filepath.Join(home, "missing"), env.GitDir)
+		assert.Equal(t, gitDir, env.GitDir)
+	})
+
+	t.Run("not a repository", func(t *testing.T) {
+		home := cleanGitEnv(t)
+		// A plain directory whose config would be read as the local scope.
+		writeFile(t, filepath.Join(home, "plain", "config"), "[a]\n\tb = 1\n")
+		// A config that is not git's, and not a file.
+		require.NoError(t, os.MkdirAll(filepath.Join(home, "dotdir", "config"), 0o755))
+		// A .git without HEAD, and a HEAD that is a directory.
+		initRepo(t, filepath.Join(home, "nohead", ".git"), "")
+		require.NoError(t, os.Remove(filepath.Join(home, "nohead", ".git", "HEAD")))
+		initRepo(t, filepath.Join(home, "headdir"), "")
+		require.NoError(t, os.Remove(filepath.Join(home, "headdir", "HEAD")))
+		require.NoError(t, os.MkdirAll(filepath.Join(home, "headdir", "HEAD"), 0o755))
+		// HEAD alone, objects missing, and refs a file.
+		writeFile(t, filepath.Join(home, "headonly", "HEAD"), "ref: refs/heads/main\n")
+		initRepo(t, filepath.Join(home, "noobjects"), "")
+		require.NoError(t, os.Remove(filepath.Join(home, "noobjects", "objects")))
+		initRepo(t, filepath.Join(home, "reffile"), "")
+		require.NoError(t, os.Remove(filepath.Join(home, "reffile", "refs")))
+		writeFile(t, filepath.Join(home, "reffile", "refs"), "")
+		// A linked worktree whose common directory has no objects.
+		initRepo(t, filepath.Join(home, "main", ".git"), "")
+		addWorktree(t, filepath.Join(home, "main", ".git"), "lw", "main")
+		require.NoError(t, os.Remove(filepath.Join(home, "main", ".git", "objects")))
+		writeFile(t, filepath.Join(home, "lw", ".git"), "gitdir: ../main/.git/worktrees/lw\n")
+		// A linked worktree whose git directory has no commondir.
+		initRepo(t, filepath.Join(home, "main2", ".git"), "")
+		addWorktree(t, filepath.Join(home, "main2", ".git"), "nocd", "main")
+		require.NoError(t, os.Remove(filepath.Join(home, "main2", ".git", "worktrees", "nocd", "commondir")))
+		writeFile(t, filepath.Join(home, "nocd", ".git"), "gitdir: ../main2/.git/worktrees/nocd\n")
+		// A gitdir file naming a directory that is not a repository.
+		writeFile(t, filepath.Join(home, "wt", ".git"), "gitdir: ../plain\n")
+		// A regular file in the way.
+		writeFile(t, filepath.Join(home, "file"), "")
+
+		for _, tc := range []struct{ dir, gitDir string }{
+			{"missing", "missing"},
+			{"plain", "plain"},
+			{"dotdir", "dotdir"},
+			{"nohead", filepath.Join("nohead", ".git")},
+			{filepath.Join("nohead", ".git"), filepath.Join("nohead", ".git")},
+			{"headdir", "headdir"},
+			{"headonly", "headonly"},
+			{"noobjects", "noobjects"},
+			{"reffile", "reffile"},
+			{"lw", filepath.Join("main", ".git", "worktrees", "lw")},
+			{"nocd", filepath.Join("main2", ".git", "worktrees", "nocd")},
+			{"wt", "plain"},
+			{filepath.Join("file", "sub"), filepath.Join("file", "sub")},
+		} {
+			_, err := gitconfig.Discover(filepath.Join(home, tc.dir))
+			require.ErrorIs(t, err, gitconfig.ErrNotRepository, tc.dir)
+			fe, ok := errors.AsType[*gitconfig.FileError](err)
+			require.True(t, ok, tc.dir)
+			assert.Equal(t, filepath.Join(home, tc.gitDir), filepath.Clean(fe.Path), tc.dir)
+		}
 	})
 
 	t.Run("malformed gitdir file", func(t *testing.T) {

errors.go

@@ -21,8 +21,9 @@ import (
 //     unwrapped, as no file is concerned. The error returned by the
 //     callback of Env.UpdateFile or Env.UpdateScope is returned as is.
 //   - Discover also returns an *EnvError for an environment variable it
-//     cannot interpret, and ErrUntrusted in a *FileError for a repository
-//     git would refuse.
+//     cannot interpret, ErrNotRepository in a *FileError for a directory
+//     that is not a repository, and ErrUntrusted in a *FileError for a
+//     repository git would refuse.
 //
 // Error messages name files, whose paths can come from a configuration
 // file someone else wrote. As in git, but stricter, control characters
@@ -82,6 +83,13 @@ var (
 	// safe.directory. The *FileError's Path is the path to list.
 	ErrUntrusted = errors.New("gitconfig: repository not owned by the current user nor listed in safe.directory")
 
+	// ErrNotRepository is returned by Discover for a directory that is not
+	// a repository: the git directory expected, its .git when it exists and
+	// the directory itself otherwise, holds no HEAD file, or its common
+	// directory no objects and refs directories. The *FileError's Path is
+	// that git directory.
+	ErrNotRepository = errors.New("gitconfig: not a git repository")
+
 	// ErrClosed is returned when a File is edited after the callback of
 	// Env.UpdateFile or Env.UpdateScope has returned.
 	ErrClosed = errors.New("gitconfig: file is closed")