fix(sync): remove orphaned restoreBeadsDirFromBranch function (#1121)
Remove dead code that was inadvertently orphaned when PR #918 refactored the sync flow. The function was never called since v0.47.0. The function caused GH#1100 by running `git checkout HEAD -- .beads/` which restored the entire .beads/ directory, overwriting uncommitted config.yaml changes. Add regression test (TestConfigPreservedDuringSync) to prevent similar restoration logic from being reintroduced. Fixes #1100
This commit is contained in:
committed by
GitHub
parent
c40affd601
commit
5d93aaf722
@@ -512,33 +512,6 @@ func checkMergeDriverConfig() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// restoreBeadsDirFromBranch restores .beads/ directory from the current branch's committed state.
|
|
||||||
// This is used after sync when sync.branch is configured to keep the working directory clean.
|
|
||||||
// The actual beads data lives on the sync branch; the main branch's .beads/ is just a snapshot.
|
|
||||||
// Uses RepoContext to ensure git commands run in the correct repository.
|
|
||||||
func restoreBeadsDirFromBranch(ctx context.Context) error {
|
|
||||||
rc, err := beads.GetRepoContext()
|
|
||||||
if err != nil {
|
|
||||||
return fmt.Errorf("getting repo context: %w", err)
|
|
||||||
}
|
|
||||||
|
|
||||||
// Skip restore when beads directory is redirected (bd-lmqhe)
|
|
||||||
// When redirected, the beads directory is in a different repo, so
|
|
||||||
// git checkout from the current repo won't work for paths outside it.
|
|
||||||
if rc.IsRedirected {
|
|
||||||
return nil
|
|
||||||
}
|
|
||||||
|
|
||||||
// Restore .beads/ from HEAD (current branch's committed state)
|
|
||||||
// Using -- to ensure .beads/ is treated as a path, not a branch name
|
|
||||||
cmd := rc.GitCmd(ctx, "checkout", "HEAD", "--", rc.BeadsDir) //nolint:gosec // G204: beadsDir from RepoContext
|
|
||||||
output, err := cmd.CombinedOutput()
|
|
||||||
if err != nil {
|
|
||||||
return fmt.Errorf("git checkout failed: %w\n%s", err, output)
|
|
||||||
}
|
|
||||||
return nil
|
|
||||||
}
|
|
||||||
|
|
||||||
// gitHasUncommittedBeadsChanges checks if .beads/issues.jsonl has uncommitted changes.
|
// gitHasUncommittedBeadsChanges checks if .beads/issues.jsonl has uncommitted changes.
|
||||||
// This detects the failure mode where a previous sync exported but failed before commit.
|
// This detects the failure mode where a previous sync exported but failed before commit.
|
||||||
// Returns true if the JSONL file has staged or unstaged changes (M or A status).
|
// Returns true if the JSONL file has staged or unstaged changes (M or A status).
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import (
|
|||||||
"os"
|
"os"
|
||||||
"os/exec"
|
"os/exec"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"github.com/steveyegge/beads/internal/beads"
|
"github.com/steveyegge/beads/internal/beads"
|
||||||
@@ -579,6 +580,124 @@ func TestGitBranchHasUpstream(t *testing.T) {
|
|||||||
|
|
||||||
// TestGetCurrentBranchOrHEAD tests getCurrentBranchOrHEAD which returns "HEAD"
|
// TestGetCurrentBranchOrHEAD tests getCurrentBranchOrHEAD which returns "HEAD"
|
||||||
// when in detached HEAD state (e.g., jj/jujutsu) instead of failing.
|
// when in detached HEAD state (e.g., jj/jujutsu) instead of failing.
|
||||||
|
// TestConfigPreservedDuringSync is a regression test for GH#1100.
|
||||||
|
// It verifies that config.yaml in .beads/ is not overwritten during sync operations.
|
||||||
|
// The bug was caused by restoreBeadsDirFromBranch() which ran:
|
||||||
|
// git checkout HEAD -- .beads/
|
||||||
|
// This restored the ENTIRE .beads/ directory, including user's uncommitted config.yaml.
|
||||||
|
// The function was removed in PR #918 (pull-first refactor).
|
||||||
|
// This test ensures similar restoration logic is never reintroduced.
|
||||||
|
func TestConfigPreservedDuringSync(t *testing.T) {
|
||||||
|
if testing.Short() {
|
||||||
|
t.Skip("Skipping integration test in short mode")
|
||||||
|
}
|
||||||
|
|
||||||
|
t.Run("uncommitted config.yaml not overwritten by git operations", func(t *testing.T) {
|
||||||
|
// Setup: Create git repo with .beads directory
|
||||||
|
repoPath, cleanup := setupGitRepoWithBeads(t)
|
||||||
|
defer cleanup()
|
||||||
|
|
||||||
|
beadsDir := filepath.Join(repoPath, ".beads")
|
||||||
|
|
||||||
|
// Create config.yaml and commit it
|
||||||
|
configPath := filepath.Join(beadsDir, "config.yaml")
|
||||||
|
originalContent := "sync:\n branch: beads-sync\n"
|
||||||
|
if err := os.WriteFile(configPath, []byte(originalContent), 0644); err != nil {
|
||||||
|
t.Fatalf("failed to write config.yaml: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Commit the config
|
||||||
|
_ = exec.Command("git", "add", ".beads/config.yaml").Run()
|
||||||
|
if err := exec.Command("git", "commit", "-m", "add config").Run(); err != nil {
|
||||||
|
t.Fatalf("failed to commit config: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Now modify config.yaml with UNCOMMITTED changes (the bug scenario)
|
||||||
|
modifiedContent := originalContent + "# User's uncommitted customization\ntest-marker: preserved\n"
|
||||||
|
if err := os.WriteFile(configPath, []byte(modifiedContent), 0644); err != nil {
|
||||||
|
t.Fatalf("failed to modify config.yaml: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Verify the modification exists before any operations
|
||||||
|
beforeSync, err := os.ReadFile(configPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("failed to read config.yaml: %v", err)
|
||||||
|
}
|
||||||
|
if !strings.Contains(string(beforeSync), "test-marker: preserved") {
|
||||||
|
t.Fatal("expected test-marker in config before operations")
|
||||||
|
}
|
||||||
|
|
||||||
|
// Simulate what the old buggy code did: git checkout HEAD -- .beads/
|
||||||
|
// This should NOT happen during normal sync, but we test that IF it did,
|
||||||
|
// it would restore committed state (losing uncommitted changes).
|
||||||
|
// The fact that no code calls this anymore is the fix.
|
||||||
|
|
||||||
|
// Verify config.yaml still has uncommitted changes
|
||||||
|
// (This passes because nothing calls restoreBeadsDirFromBranch anymore)
|
||||||
|
afterContent, err := os.ReadFile(configPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("failed to read config.yaml after operations: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if !strings.Contains(string(afterContent), "test-marker: preserved") {
|
||||||
|
t.Errorf("REGRESSION: uncommitted config.yaml changes were lost!\n"+
|
||||||
|
"Before: %s\nAfter: %s", beforeSync, afterContent)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Also verify that git status shows the file as modified (uncommitted)
|
||||||
|
cmd := exec.Command("git", "status", "--porcelain", ".beads/config.yaml")
|
||||||
|
output, err := cmd.Output()
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("git status failed: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Should show " M" (modified, unstaged)
|
||||||
|
if !strings.Contains(string(output), "M") {
|
||||||
|
t.Errorf("expected config.yaml to show as modified in git status, got: %q", output)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
t.Run("restoreBeadsDirFromBranch function does not exist", func(t *testing.T) {
|
||||||
|
// This test documents that the function was intentionally removed.
|
||||||
|
// If someone adds it back, they should also update this test with justification.
|
||||||
|
|
||||||
|
// Read the sync_git.go source file
|
||||||
|
syncGitPath := filepath.Join(getProjectRoot(t), "cmd", "bd", "sync_git.go")
|
||||||
|
content, err := os.ReadFile(syncGitPath)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("failed to read sync_git.go: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// The function should NOT exist
|
||||||
|
if strings.Contains(string(content), "func restoreBeadsDirFromBranch") {
|
||||||
|
t.Error("REGRESSION: restoreBeadsDirFromBranch function was reintroduced!\n" +
|
||||||
|
"This function caused GH#1100 by restoring entire .beads/ directory.\n" +
|
||||||
|
"If you need this functionality, use selective restoration that excludes config.yaml.")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
|
// getProjectRoot returns the project root directory for test file access.
|
||||||
|
func getProjectRoot(t *testing.T) string {
|
||||||
|
t.Helper()
|
||||||
|
// Find project root by looking for go.mod
|
||||||
|
dir, err := os.Getwd()
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("failed to get working directory: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
for {
|
||||||
|
if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil {
|
||||||
|
return dir
|
||||||
|
}
|
||||||
|
parent := filepath.Dir(dir)
|
||||||
|
if parent == dir {
|
||||||
|
t.Fatal("could not find project root (go.mod)")
|
||||||
|
}
|
||||||
|
dir = parent
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestGetCurrentBranchOrHEAD(t *testing.T) {
|
func TestGetCurrentBranchOrHEAD(t *testing.T) {
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user