fix: match .git as complete path component, not suffix

Searching for `.git/` in file paths incorrectly matched directory names
ending with `.git` (e.g., `personal.git/cheat/hello`), causing sheets
under such paths to be silently skipped. Fix by requiring the path
separator on both sides (`/.git/`), so `.git` is only matched as a
complete path component.

Rewrites test suite with comprehensive coverage for all six documented
edge cases, including the #711 scenario and combination cases (e.g.,
a real .git directory inside a .git-suffixed parent).

Closes #711

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Christopher Allen Lane
2026-02-15 07:32:35 -05:00
parent 1969423b5c
commit 97e80beceb
2 changed files with 320 additions and 127 deletions

View File

@@ -6,6 +6,11 @@ import (
"strings" "strings"
) )
// gitSep is the `.git` path component surrounded by path separators.
// Used to match `.git` as a complete path component, not as a suffix
// of a directory name (e.g., `personal.git`).
var gitSep = string(os.PathSeparator) + ".git" + string(os.PathSeparator)
// GitDir returns `true` if we are iterating over a directory contained within // GitDir returns `true` if we are iterating over a directory contained within
// a repositories `.git` directory. // a repositories `.git` directory.
func GitDir(path string) (bool, error) { func GitDir(path string) (bool, error) {
@@ -50,9 +55,20 @@ func GitDir(path string) (bool, error) {
See: https://github.com/cheat/cheat/issues/699 See: https://github.com/cheat/cheat/issues/699
Accounting for all of the above (hopefully?), the current solution is Accounting for all of the above, the next solution was to search not
not to search for `.git`, but `.git/` (including the directory for `.git`, but `.git/` (including the directory separator), and then
separator), and then only ceasing to walk the directory on a match. only ceasing to walk the directory on a match.
This, however, also had a bug: searching for `.git/` also matched
directory names that *ended with* `.git`, like `personal.git/`. This
caused cheatsheets stored under such paths to be silently skipped.
See: https://github.com/cheat/cheat/issues/711
The current (and hopefully final) solution requires the path separator
on *both* sides of `.git`, i.e., searching for `/.git/`. This ensures
that `.git` is matched only as a complete path component, not as a
suffix of a directory name.
To summarize, this code must account for the following possibilities: To summarize, this code must account for the following possibilities:
@@ -61,17 +77,16 @@ func GitDir(path string) (bool, error) {
3. A cheatpath is a repository, and contains a `.git*` file 3. A cheatpath is a repository, and contains a `.git*` file
4. A cheatpath is a submodule 4. A cheatpath is a submodule
5. A cheatpath is a hidden directory 5. A cheatpath is a hidden directory
6. A cheatpath is inside a directory whose name ends with `.git`
Care must be taken to support the above on both Unix and Windows Care must be taken to support the above on both Unix and Windows
systems, which have different directory separators and line-endings. systems, which have different directory separators and line-endings.
There is a lot of nuance to all of this, and it would be worthwhile to NB: `filepath.Walk` always passes absolute paths to the walk function,
do two things to stop writing bugs here: so `.git` will never appear as the first path component. This is what
makes the "separator on both sides" approach safe.
1. Build integration tests around all of this A reasonable smoke-test for ensuring that skipping is being applied
2. Discard string-matching solutions entirely, and use `go-git` instead
NB: A reasonable smoke-test for ensuring that skipping is being applied
correctly is to run the following command: correctly is to run the following command:
make && strace ./dist/cheat -l | wc -l make && strace ./dist/cheat -l | wc -l
@@ -83,8 +98,8 @@ func GitDir(path string) (bool, error) {
of syscalls should be significantly lower with the skip check enabled. of syscalls should be significantly lower with the skip check enabled.
*/ */
// determine if the literal string `.git` appears within `path` // determine if `.git` appears as a complete path component
pos := strings.Index(path, fmt.Sprintf(".git%s", string(os.PathSeparator))) pos := strings.Index(path, gitSep)
// if it does not, we know for certain that we are not within a `.git` // if it does not, we know for certain that we are not within a `.git`
// directory. // directory.

View File

@@ -1,137 +1,191 @@
package repo package repo
import ( import (
"fmt"
"os" "os"
"path/filepath" "path/filepath"
"testing" "testing"
) )
func TestGitDir(t *testing.T) { // setupGitDirTestTree creates a temporary directory structure that exercises
// Create a temporary directory for testing // every case documented in GitDir's comment block. The caller must defer
tempDir, err := os.MkdirTemp("", "cheat-test-*") // os.RemoveAll on the returned root.
if err != nil { //
t.Fatalf("failed to create temp dir: %v", err) // Layout:
} //
defer os.RemoveAll(tempDir) // root/
// ├── plain/ # not a repository
// │ └── sheet
// ├── repo/ # a repository (.git is a directory)
// │ ├── .git/
// │ │ ├── HEAD
// │ │ ├── objects/
// │ │ │ └── pack/
// │ │ └── refs/
// │ │ └── heads/
// │ ├── .gitignore
// │ ├── .gitattributes
// │ └── sheet
// ├── submodule/ # a submodule (.git is a file)
// │ ├── .git # file, not directory
// │ └── sheet
// ├── dotgit-suffix.git/ # directory name ends in .git (#711)
// │ └── cheat/
// │ └── sheet
// ├── dotgit-mid.git/ # .git suffix mid-path (#711)
// │ └── nested/
// │ └── sheet
// ├── .github/ # .github directory (not .git)
// │ └── workflows/
// │ └── ci.yml
// └── .hidden/ # generic hidden directory
// └── sheet
func setupGitDirTestTree(t *testing.T) string {
t.Helper()
// Create test directory structure root := t.TempDir()
testDirs := []string{
filepath.Join(tempDir, ".git"), dirs := []string{
filepath.Join(tempDir, ".git", "objects"), // case 1: not a repository
filepath.Join(tempDir, ".git", "refs"), filepath.Join(root, "plain"),
filepath.Join(tempDir, "regular"),
filepath.Join(tempDir, "regular", ".git"), // case 2: a repository (.git directory with contents)
filepath.Join(tempDir, "submodule"), filepath.Join(root, "repo", ".git", "objects", "pack"),
filepath.Join(root, "repo", ".git", "refs", "heads"),
// case 4: a submodule (.git is a file)
filepath.Join(root, "submodule"),
// case 6: directory name ending in .git (#711)
filepath.Join(root, "dotgit-suffix.git", "cheat"),
filepath.Join(root, "dotgit-mid.git", "nested"),
// .github (should not be confused with .git)
filepath.Join(root, ".github", "workflows"),
// generic hidden directory
filepath.Join(root, ".hidden"),
} }
for _, dir := range testDirs { for _, dir := range dirs {
if err := os.MkdirAll(dir, 0755); err != nil { if err := os.MkdirAll(dir, 0755); err != nil {
t.Fatalf("failed to create dir %s: %v", dir, err) t.Fatalf("failed to create dir %s: %v", dir, err)
} }
} }
// Create test files files := map[string]string{
testFiles := map[string]string{ // sheets
filepath.Join(tempDir, ".gitignore"): "*.tmp\n", filepath.Join(root, "plain", "sheet"): "plain sheet",
filepath.Join(tempDir, ".gitattributes"): "* text=auto\n", filepath.Join(root, "repo", "sheet"): "repo sheet",
filepath.Join(tempDir, "submodule", ".git"): "gitdir: ../.git/modules/submodule\n", filepath.Join(root, "submodule", "sheet"): "submod sheet",
filepath.Join(tempDir, "regular", "sheet.txt"): "content\n", filepath.Join(root, "dotgit-suffix.git", "cheat", "sheet"): "dotgit sheet",
filepath.Join(root, "dotgit-mid.git", "nested", "sheet"): "dotgit nested",
filepath.Join(root, ".hidden", "sheet"): "hidden sheet",
// git metadata
filepath.Join(root, "repo", ".git", "HEAD"): "ref: refs/heads/main\n",
filepath.Join(root, "repo", ".gitignore"): "*.tmp\n",
filepath.Join(root, "repo", ".gitattributes"): "* text=auto\n",
filepath.Join(root, "submodule", ".git"): "gitdir: ../.git/modules/sub\n",
filepath.Join(root, ".github", "workflows", "ci.yml"): "name: CI\n",
} }
for file, content := range testFiles { for path, content := range files {
if err := os.WriteFile(file, []byte(content), 0644); err != nil { if err := os.WriteFile(path, []byte(content), 0644); err != nil {
t.Fatalf("failed to create file %s: %v", file, err) t.Fatalf("failed to write %s: %v", path, err)
} }
} }
return root
}
func TestGitDir(t *testing.T) {
root := setupGitDirTestTree(t)
tests := []struct { tests := []struct {
name string name string
path string path string
want bool want bool
wantErr bool
}{ }{
// Case 1: not a repository — no .git anywhere in path
{ {
name: "not in git directory", name: "plain directory, no repo",
path: filepath.Join(tempDir, "regular", "sheet.txt"), path: filepath.Join(root, "plain", "sheet"),
want: false, want: false,
}, },
// Case 2: a repository — paths *inside* .git/ should be detected
{ {
name: "in .git directory", name: "inside .git directory",
path: filepath.Join(tempDir, ".git", "objects", "file"), path: filepath.Join(root, "repo", ".git", "HEAD"),
want: true, want: true,
}, },
{ {
name: "in .git/refs directory", name: "inside .git/objects",
path: filepath.Join(tempDir, ".git", "refs", "heads", "main"), path: filepath.Join(root, "repo", ".git", "objects", "pack", "somefile"),
want: true, want: true,
}, },
{
name: "inside .git/refs",
path: filepath.Join(root, "repo", ".git", "refs", "heads", "main"),
want: true,
},
// Case 2 (cont.): files *alongside* .git should NOT be detected
{
name: "sheet in repo root (beside .git dir)",
path: filepath.Join(root, "repo", "sheet"),
want: false,
},
// Case 3: .git* files (like .gitignore) should NOT trigger
{ {
name: ".gitignore file", name: ".gitignore file",
path: filepath.Join(tempDir, ".gitignore"), path: filepath.Join(root, "repo", ".gitignore"),
want: false, want: false,
}, },
{ {
name: ".gitattributes file", name: ".gitattributes file",
path: filepath.Join(tempDir, ".gitattributes"), path: filepath.Join(root, "repo", ".gitattributes"),
want: false, want: false,
}, },
{
name: "submodule with .git file",
path: filepath.Join(tempDir, "submodule", "sheet.txt"),
want: false,
},
{
name: "path with .git in middle",
path: filepath.Join(tempDir, "regular", ".git", "sheet.txt"),
want: true,
},
{
name: "nonexistent path without .git",
path: filepath.Join(tempDir, "nonexistent", "file"),
want: false,
},
}
for _, tt := range tests { // Case 4: submodule — .git is a file, not a directory
t.Run(tt.name, func(t *testing.T) { {
got, err := GitDir(tt.path) name: "sheet in submodule (where .git is a file)",
if (err != nil) != tt.wantErr { path: filepath.Join(root, "submodule", "sheet"),
t.Errorf("GitDir() error = %v, wantErr %v", err, tt.wantErr) want: false,
return },
}
if got != tt.want {
t.Errorf("GitDir() = %v, want %v", got, tt.want)
}
})
}
}
func TestGitDirEdgeCases(t *testing.T) { // Case 6: directory name ends with .git (#711)
// Test with paths that have .git but not as a directory separator
tests := []struct {
name string
path string
want bool
}{
{ {
name: "file ending with .git", name: "sheet under directory ending in .git",
path: "/tmp/myfile.git", path: filepath.Join(root, "dotgit-suffix.git", "cheat", "sheet"),
want: false, want: false,
}, },
{ {
name: "directory ending with .git", name: "sheet under .git-suffixed dir, nested deeper",
path: "/tmp/myrepo.git", path: filepath.Join(root, "dotgit-mid.git", "nested", "sheet"),
want: false, want: false,
}, },
// .github directory — must not be confused with .git
{ {
name: ".github directory", name: "file inside .github directory",
path: "/tmp/.github/workflows", path: filepath.Join(root, ".github", "workflows", "ci.yml"),
want: false, want: false,
}, },
// Hidden directory that is not .git
{ {
name: "legitimate.git-repo name", name: "file inside generic hidden directory",
path: "/tmp/legitimate.git-repo/file", path: filepath.Join(root, ".hidden", "sheet"),
want: false,
},
// Path with no .git at all
{
name: "path with no .git component whatsoever",
path: filepath.Join(root, "nonexistent", "file"),
want: false, want: false,
}, },
} }
@@ -140,8 +194,7 @@ func TestGitDirEdgeCases(t *testing.T) {
t.Run(tt.name, func(t *testing.T) { t.Run(tt.name, func(t *testing.T) {
got, err := GitDir(tt.path) got, err := GitDir(tt.path)
if err != nil { if err != nil {
// It's ok if the path doesn't exist for these edge case tests t.Fatalf("GitDir(%q) returned unexpected error: %v", tt.path, err)
return
} }
if got != tt.want { if got != tt.want {
t.Errorf("GitDir(%q) = %v, want %v", tt.path, got, tt.want) t.Errorf("GitDir(%q) = %v, want %v", tt.path, got, tt.want)
@@ -150,28 +203,153 @@ func TestGitDirEdgeCases(t *testing.T) {
} }
} }
func TestGitDirPathSeparator(t *testing.T) { // TestGitDirWithNestedGitDir tests a repo inside a .git-suffixed parent
// Test that the function correctly uses os.PathSeparator // directory. This is the nastiest combination: a real .git directory that
// This is important for cross-platform compatibility // appears *after* a .git suffix in the path.
func TestGitDirWithNestedGitDir(t *testing.T) {
root := t.TempDir()
// Create a path with the wrong separator for the current OS // Create: root/cheats.git/repo/.git/HEAD
var wrongSep string // root/cheats.git/repo/sheet
if os.PathSeparator == '/' { gitDir := filepath.Join(root, "cheats.git", "repo", ".git")
wrongSep = `\` if err := os.MkdirAll(gitDir, 0755); err != nil {
} else { t.Fatal(err)
wrongSep = `/` }
if err := os.WriteFile(filepath.Join(gitDir, "HEAD"), []byte("ref: refs/heads/main\n"), 0644); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(root, "cheats.git", "repo", "sheet"), []byte("content"), 0644); err != nil {
t.Fatal(err)
} }
// Path with wrong separator should not be detected as git dir tests := []struct {
path := fmt.Sprintf("some%spath%s.git%sfile", wrongSep, wrongSep, wrongSep) name string
isGit, err := GitDir(path) path string
want bool
}{
{
name: "sheet beside .git in .git-suffixed parent",
path: filepath.Join(root, "cheats.git", "repo", "sheet"),
want: false,
},
{
name: "file inside .git inside .git-suffixed parent",
path: filepath.Join(root, "cheats.git", "repo", ".git", "HEAD"),
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := GitDir(tt.path)
if err != nil { if err != nil {
// Path doesn't exist, which is fine t.Fatalf("GitDir(%q) returned unexpected error: %v", tt.path, err)
return }
if got != tt.want {
t.Errorf("GitDir(%q) = %v, want %v", tt.path, got, tt.want)
}
})
}
} }
// TestGitDirSubmoduleInsideDotGitSuffix tests a submodule (.git file)
// inside a .git-suffixed parent directory.
func TestGitDirSubmoduleInsideDotGitSuffix(t *testing.T) {
root := t.TempDir()
// Create: root/personal.git/submod/.git (file)
// root/personal.git/submod/sheet
subDir := filepath.Join(root, "personal.git", "submod")
if err := os.MkdirAll(subDir, 0755); err != nil {
t.Fatal(err)
}
// .git as a file (submodule pointer)
if err := os.WriteFile(filepath.Join(subDir, ".git"), []byte("gitdir: ../../.git/modules/sub\n"), 0644); err != nil {
t.Fatal(err)
}
if err := os.WriteFile(filepath.Join(subDir, "sheet"), []byte("content"), 0644); err != nil {
t.Fatal(err)
}
got, err := GitDir(filepath.Join(subDir, "sheet"))
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if got {
t.Error("GitDir should return false for sheet in submodule under .git-suffixed parent")
}
}
// TestGitDirIntegrationWalk simulates what sheets.Load does: walking a
// directory tree and checking each path with GitDir. This verifies that
// the function works correctly in the context of filepath.Walk, which is
// how it is actually called.
func TestGitDirIntegrationWalk(t *testing.T) {
root := setupGitDirTestTree(t)
// Walk the tree and collect which paths GitDir says to skip
var skipped []string
var visited []string
err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
isGit, err := GitDir(path)
if err != nil {
return err
}
if isGit { if isGit {
t.Errorf("GitDir() incorrectly detected git dir with wrong path separator") skipped = append(skipped, path)
} else {
visited = append(visited, path)
}
return nil
})
if err != nil {
t.Fatalf("Walk failed: %v", err)
}
// Files inside .git/ should be skipped
expectSkipped := []string{
filepath.Join(root, "repo", ".git", "HEAD"),
}
for _, want := range expectSkipped {
found := false
for _, got := range skipped {
if got == want {
found = true
break
}
}
if !found {
t.Errorf("expected %q to be skipped, but it was not", want)
}
}
// Sheets should NOT be skipped — including the #711 case
expectVisited := []string{
filepath.Join(root, "plain", "sheet"),
filepath.Join(root, "repo", "sheet"),
filepath.Join(root, "submodule", "sheet"),
filepath.Join(root, "dotgit-suffix.git", "cheat", "sheet"),
filepath.Join(root, "dotgit-mid.git", "nested", "sheet"),
filepath.Join(root, ".hidden", "sheet"),
}
for _, want := range expectVisited {
found := false
for _, got := range visited {
if got == want {
found = true
break
}
}
if !found {
t.Errorf("expected %q to be visited (not skipped), but it was not found in visited paths", want)
}
} }
} }