Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion internal/gitvolume/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,9 @@ func TestCheckStatus(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
tc.setup(t, tc.vol)
defer os.RemoveAll(tc.vol.TargetPath)
t.Cleanup(func() {
require.NoError(t, os.RemoveAll(tc.vol.TargetPath))
})

status := tc.vol.CheckStatus()
assert.Equal(t, tc.wantStatus, status.Status)
Expand Down
6 changes: 4 additions & 2 deletions internal/gitvolume/edit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ func TestGitVolume_GlobalEdit(t *testing.T) {
// Mock EDITOR to a script that modifies the file
// We use printf to avoid portability issues with echo -n
originalEditor := os.Getenv("EDITOR")
defer os.Setenv("EDITOR", originalEditor)
os.Setenv("EDITOR", "sh -c 'printf \" - edited\" >> \"$1\"' --")
require.NoError(t, os.Setenv("EDITOR", "sh -c 'printf \" - edited\" >> \"$1\"' --"))
t.Cleanup(func() {
require.NoError(t, os.Setenv("EDITOR", originalEditor))
})

err = gv.GlobalEdit("config.txt")
assert.NoError(t, err)
Expand Down
4 changes: 3 additions & 1 deletion internal/gitvolume/git_repro_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ func TestFindCommonDir_Repro(t *testing.T) {
// Create a temporary directory for our test environment
tmpDir, err := os.MkdirTemp("", "git-volume-repro-*")
require.NoError(t, err)
defer os.RemoveAll(tmpDir)
t.Cleanup(func() {
require.NoError(t, os.RemoveAll(tmpDir))
})

// 1. Test Case: Bare Repository + Worktree
t.Run("Bare Repository with Worktree", func(t *testing.T) {
Expand Down
19 changes: 18 additions & 1 deletion internal/gitvolume/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,31 @@ func (g *GitVolume) beforeInit(state *initState) error {
if err != nil {
return fmt.Errorf("failed to get current directory: %w", err)
}
gitRoot, err := FindWorktreeRoot(cwd)
gitRoot, err := findInitRoot(cwd)
if err != nil {
return fmt.Errorf("failed to find git repository root: %w", err)
}
state.configPath = filepath.Join(gitRoot, ConfigFileName)
return nil
}

// findInitRoot returns the directory where git-volume.yaml should be created.
// In a normal repository or worktree, that is the current worktree root.
// In a bare repository, that is the bare repository root itself.
func findInitRoot(startDir string) (string, error) {
worktreeRoot, err := FindWorktreeRoot(startDir)
if err == nil {
return worktreeRoot, nil
}

isBare, bareErr := isBareRepository(startDir)
if bareErr == nil && isBare {
return findCommonDir(startDir)
}

return "", err
}
Comment on lines +41 to +53

Choose a reason for hiding this comment

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

medium

The current error handling logic discards the error from isBareRepository (bareErr) if it fails, and instead returns the initial error from FindWorktreeRoot (err). This could be misleading for the user. For example, if checking for a bare repository fails due to a permissions issue, the user will see a generic "not a git worktree" error instead of the more specific permissions error.

To improve error diagnostics, it would be better to return the error from the isBareRepository check if it fails. This would help the user understand the root cause of the failure more easily when they are running init in what they believe is a bare repository.

func findInitRoot(startDir string) (string, error) {
	worktreeRoot, worktreeErr := FindWorktreeRoot(startDir)
	if worktreeErr == nil {
		return worktreeRoot, nil
	}

	isBare, bareErr := isBareRepository(startDir)
	if bareErr != nil {
		return "", fmt.Errorf("failed to check for bare repository: %w", bareErr)
	}

	if isBare {
		return findCommonDir(startDir)
	}

	return "", worktreeErr
}
References
  1. The style guide recommends wrapping errors to add context using fmt.Errorf("...: %w", err). The suggested change improves error handling by capturing and wrapping a previously discarded error, providing more specific context to the user upon failure. (link)


func (g *GitVolume) init(state *initState) error {
if err := os.MkdirAll(g.ctx.GlobalDir, DefaultDirPerm); err != nil {
return fmt.Errorf("failed to create global directory %s: %w", g.ctx.GlobalDir, err)
Expand Down
34 changes: 30 additions & 4 deletions internal/gitvolume/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestInit(t *testing.T) {
// Change CWD to repo
wd, err := os.Getwd()
require.NoError(t, err)
defer func() { _ = os.Chdir(wd) }() // Restore CWD
t.Cleanup(func() { require.NoError(t, os.Chdir(wd)) }) // Restore CWD
require.NoError(t, os.Chdir(repoDir))

// Setup GitVolume (GlobalDir will be in tmp)
Expand Down Expand Up @@ -60,7 +60,7 @@ func TestInit_OutsideGit(t *testing.T) {
// Change CWD
wd, err := os.Getwd()
require.NoError(t, err)
defer func() { _ = os.Chdir(wd) }()
t.Cleanup(func() { require.NoError(t, os.Chdir(wd)) })
require.NoError(t, os.Chdir(tmpDir))

gv := createTestGitVolume(tmpDir, tmpDir, filepath.Join(tmpDir, "global"), nil)
Expand All @@ -72,6 +72,32 @@ func TestInit_OutsideGit(t *testing.T) {
assert.Contains(t, err.Error(), "failed to find git repository root")
}

func TestInit_BareRepository(t *testing.T) {
tmpDir := t.TempDir()
bareDir := filepath.Join(tmpDir, "repo.git")

cmd := exec.Command("git", "init", "--bare", bareDir)
require.NoError(t, cmd.Run())

wd, err := os.Getwd()
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, os.Chdir(wd)) })
require.NoError(t, os.Chdir(bareDir))

globalDir := filepath.Join(tmpDir, "global")
gv := createTestGitVolume(bareDir, bareDir, globalDir, nil)

err = gv.Init()
require.NoError(t, err)

assert.DirExists(t, globalDir)
assert.FileExists(t, filepath.Join(bareDir, "git-volume.yaml"))

content, err := os.ReadFile(filepath.Join(bareDir, "git-volume.yaml"))
require.NoError(t, err)
assert.Contains(t, string(content), "volumes:")
}

func TestInit_NonQuiet(t *testing.T) {
tmpDir := t.TempDir()
repoDir := filepath.Join(tmpDir, "repo")
Expand All @@ -81,7 +107,7 @@ func TestInit_NonQuiet(t *testing.T) {

wd, err := os.Getwd()
require.NoError(t, err)
defer func() { _ = os.Chdir(wd) }()
t.Cleanup(func() { require.NoError(t, os.Chdir(wd)) })
require.NoError(t, os.Chdir(repoDir))

globalDir := filepath.Join(tmpDir, "global")
Expand All @@ -104,7 +130,7 @@ func TestInit_NonQuiet_Error(t *testing.T) {

wd, err := os.Getwd()
require.NoError(t, err)
defer func() { _ = os.Chdir(wd) }()
t.Cleanup(func() { require.NoError(t, os.Chdir(wd)) })
require.NoError(t, os.Chdir(tmpDir))

gv := createTestGitVolume(tmpDir, tmpDir, filepath.Join(tmpDir, "global"), nil)
Expand Down
4 changes: 3 additions & 1 deletion internal/gitvolume/remove_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ func TestGlobalRemove(t *testing.T) {
func TestGlobalRemove_NotInitialized(t *testing.T) {
tmpDir, err := os.MkdirTemp("", "git-volume-test")
require.NoError(t, err)
defer os.RemoveAll(tmpDir)
t.Cleanup(func() {
require.NoError(t, os.RemoveAll(tmpDir))
})

// Point to a non-existent directory
globalDir := filepath.Join(tmpDir, "non_existent_global")
Expand Down
Loading