diff --git a/internal/repo/gitdir.go b/internal/repo/gitdir.go index 06a7f23..648bbfb 100644 --- a/internal/repo/gitdir.go +++ b/internal/repo/gitdir.go @@ -6,6 +6,11 @@ import ( "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 // a repositories `.git` directory. func GitDir(path string) (bool, error) { @@ -50,9 +55,20 @@ func GitDir(path string) (bool, error) { See: https://github.com/cheat/cheat/issues/699 - Accounting for all of the above (hopefully?), the current solution is - not to search for `.git`, but `.git/` (including the directory - separator), and then only ceasing to walk the directory on a match. + Accounting for all of the above, the next solution was to search not + for `.git`, but `.git/` (including the directory separator), and then + 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: @@ -61,17 +77,16 @@ func GitDir(path string) (bool, error) { 3. A cheatpath is a repository, and contains a `.git*` file 4. A cheatpath is a submodule 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 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 - do two things to stop writing bugs here: + NB: `filepath.Walk` always passes absolute paths to the walk function, + 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 - 2. Discard string-matching solutions entirely, and use `go-git` instead - - NB: A reasonable smoke-test for ensuring that skipping is being applied + A reasonable smoke-test for ensuring that skipping is being applied correctly is to run the following command: 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. */ - // determine if the literal string `.git` appears within `path` - pos := strings.Index(path, fmt.Sprintf(".git%s", string(os.PathSeparator))) + // determine if `.git` appears as a complete path component + pos := strings.Index(path, gitSep) // if it does not, we know for certain that we are not within a `.git` // directory. diff --git a/internal/repo/gitdir_test.go b/internal/repo/gitdir_test.go index f2fd1b6..a342e62 100644 --- a/internal/repo/gitdir_test.go +++ b/internal/repo/gitdir_test.go @@ -1,137 +1,191 @@ package repo import ( - "fmt" "os" "path/filepath" "testing" ) -func TestGitDir(t *testing.T) { - // Create a temporary directory for testing - tempDir, err := os.MkdirTemp("", "cheat-test-*") - if err != nil { - t.Fatalf("failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) +// setupGitDirTestTree creates a temporary directory structure that exercises +// every case documented in GitDir's comment block. The caller must defer +// os.RemoveAll on the returned root. +// +// Layout: +// +// 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 - testDirs := []string{ - filepath.Join(tempDir, ".git"), - filepath.Join(tempDir, ".git", "objects"), - filepath.Join(tempDir, ".git", "refs"), - filepath.Join(tempDir, "regular"), - filepath.Join(tempDir, "regular", ".git"), - filepath.Join(tempDir, "submodule"), + root := t.TempDir() + + dirs := []string{ + // case 1: not a repository + filepath.Join(root, "plain"), + + // case 2: a repository (.git directory with contents) + 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 { t.Fatalf("failed to create dir %s: %v", dir, err) } } - // Create test files - testFiles := map[string]string{ - filepath.Join(tempDir, ".gitignore"): "*.tmp\n", - filepath.Join(tempDir, ".gitattributes"): "* text=auto\n", - filepath.Join(tempDir, "submodule", ".git"): "gitdir: ../.git/modules/submodule\n", - filepath.Join(tempDir, "regular", "sheet.txt"): "content\n", + files := map[string]string{ + // sheets + filepath.Join(root, "plain", "sheet"): "plain sheet", + filepath.Join(root, "repo", "sheet"): "repo sheet", + filepath.Join(root, "submodule", "sheet"): "submod sheet", + 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 { - if err := os.WriteFile(file, []byte(content), 0644); err != nil { - t.Fatalf("failed to create file %s: %v", file, err) + for path, content := range files { + if err := os.WriteFile(path, []byte(content), 0644); err != nil { + t.Fatalf("failed to write %s: %v", path, err) } } - tests := []struct { - name string - path string - want bool - wantErr bool - }{ - { - name: "not in git directory", - path: filepath.Join(tempDir, "regular", "sheet.txt"), - want: false, - }, - { - name: "in .git directory", - path: filepath.Join(tempDir, ".git", "objects", "file"), - want: true, - }, - { - name: "in .git/refs directory", - path: filepath.Join(tempDir, ".git", "refs", "heads", "main"), - want: true, - }, - { - name: ".gitignore file", - path: filepath.Join(tempDir, ".gitignore"), - want: false, - }, - { - name: ".gitattributes file", - path: filepath.Join(tempDir, ".gitattributes"), - 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 { - t.Run(tt.name, func(t *testing.T) { - got, err := GitDir(tt.path) - if (err != nil) != tt.wantErr { - t.Errorf("GitDir() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("GitDir() = %v, want %v", got, tt.want) - } - }) - } + return root } -func TestGitDirEdgeCases(t *testing.T) { - // Test with paths that have .git but not as a directory separator +func TestGitDir(t *testing.T) { + root := setupGitDirTestTree(t) + tests := []struct { name string path string want bool }{ + // Case 1: not a repository — no .git anywhere in path { - name: "file ending with .git", - path: "/tmp/myfile.git", + name: "plain directory, no repo", + path: filepath.Join(root, "plain", "sheet"), + want: false, + }, + + // Case 2: a repository — paths *inside* .git/ should be detected + { + name: "inside .git directory", + path: filepath.Join(root, "repo", ".git", "HEAD"), + want: true, + }, + { + name: "inside .git/objects", + path: filepath.Join(root, "repo", ".git", "objects", "pack", "somefile"), + 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", + path: filepath.Join(root, "repo", ".gitignore"), want: false, }, { - name: "directory ending with .git", - path: "/tmp/myrepo.git", + name: ".gitattributes file", + path: filepath.Join(root, "repo", ".gitattributes"), + want: false, + }, + + // Case 4: submodule — .git is a file, not a directory + { + name: "sheet in submodule (where .git is a file)", + path: filepath.Join(root, "submodule", "sheet"), + want: false, + }, + + // Case 6: directory name ends with .git (#711) + { + name: "sheet under directory ending in .git", + path: filepath.Join(root, "dotgit-suffix.git", "cheat", "sheet"), want: false, }, { - name: ".github directory", - path: "/tmp/.github/workflows", + name: "sheet under .git-suffixed dir, nested deeper", + path: filepath.Join(root, "dotgit-mid.git", "nested", "sheet"), want: false, }, + + // .github directory — must not be confused with .git { - name: "legitimate.git-repo name", - path: "/tmp/legitimate.git-repo/file", + name: "file inside .github directory", + path: filepath.Join(root, ".github", "workflows", "ci.yml"), + want: false, + }, + + // Hidden directory that is not .git + { + name: "file inside generic hidden directory", + 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, }, } @@ -140,8 +194,7 @@ func TestGitDirEdgeCases(t *testing.T) { t.Run(tt.name, func(t *testing.T) { got, err := GitDir(tt.path) if err != nil { - // It's ok if the path doesn't exist for these edge case tests - return + t.Fatalf("GitDir(%q) returned unexpected error: %v", tt.path, err) } if 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) { - // Test that the function correctly uses os.PathSeparator - // This is important for cross-platform compatibility +// TestGitDirWithNestedGitDir tests a repo inside a .git-suffixed parent +// directory. This is the nastiest combination: a real .git directory that +// 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 - var wrongSep string - if os.PathSeparator == '/' { - wrongSep = `\` - } else { - wrongSep = `/` + // Create: root/cheats.git/repo/.git/HEAD + // root/cheats.git/repo/sheet + gitDir := filepath.Join(root, "cheats.git", "repo", ".git") + if err := os.MkdirAll(gitDir, 0755); err != nil { + t.Fatal(err) + } + 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 - path := fmt.Sprintf("some%spath%s.git%sfile", wrongSep, wrongSep, wrongSep) - isGit, err := GitDir(path) - - if err != nil { - // Path doesn't exist, which is fine - return + tests := []struct { + name string + 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, + }, } - if isGit { - t.Errorf("GitDir() incorrectly detected git dir with wrong path separator") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := GitDir(tt.path) + if err != nil { + t.Fatalf("GitDir(%q) returned unexpected error: %v", tt.path, err) + } + 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 { + 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) + } } }