fix(sync): address code review issues in manual conflict resolution
Fixes from code review: - Fix duplicate check in merge logic (use else clause) - Handle io.EOF gracefully (treat as quit) - Add quit (q) option to abort resolution early - Add accept-all (a) option to auto-merge remaining conflicts - Fix skipped conflicts to keep local version (not auto-merge) - Handle json.MarshalIndent errors properly - Fix truncateText to use rune count for UTF-8 safety - Update help text with new options - Add UTF-8 and emoji test cases Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -45,7 +45,9 @@ The --manual flag shows a diff for each conflict and prompts you to choose:
|
|||||||
l/local - Keep local version
|
l/local - Keep local version
|
||||||
r/remote - Keep remote version
|
r/remote - Keep remote version
|
||||||
m/merge - Auto-merge (LWW for scalars, union for collections)
|
m/merge - Auto-merge (LWW for scalars, union for collections)
|
||||||
s/skip - Skip and leave unresolved
|
s/skip - Skip (keep local, conflict remains for later)
|
||||||
|
a/all - Accept auto-merge for all remaining conflicts
|
||||||
|
q/quit - Quit and skip all remaining conflicts
|
||||||
d/diff - Show full JSON diff
|
d/diff - Show full JSON diff
|
||||||
|
|
||||||
The --full flag provides the legacy full sync behavior for backwards compatibility.`,
|
The --full flag provides the legacy full sync behavior for backwards compatibility.`,
|
||||||
@@ -1149,22 +1151,14 @@ func resolveSyncConflictsManually(ctx context.Context, jsonlPath, beadsDir strin
|
|||||||
var mergedIssues []*beads.Issue
|
var mergedIssues []*beads.Issue
|
||||||
for id := range allIDSet {
|
for id := range allIDSet {
|
||||||
if conflictIDSet[id] {
|
if conflictIDSet[id] {
|
||||||
// This was a conflict - use the resolved version if available
|
// This was a conflict
|
||||||
if resolved, ok := resolvedMap[id]; ok {
|
if resolved, ok := resolvedMap[id]; ok {
|
||||||
|
// User resolved this conflict - use their choice
|
||||||
mergedIssues = append(mergedIssues, resolved)
|
mergedIssues = append(mergedIssues, resolved)
|
||||||
}
|
} else {
|
||||||
// If not in resolvedMap, it was skipped - use the automatic merge result
|
// Skipped - keep local version in output, conflict remains for later
|
||||||
if _, ok := resolvedMap[id]; !ok {
|
if local := localMap[id]; local != nil {
|
||||||
// Fall back to field-level merge for skipped conflicts
|
|
||||||
local := localMap[id]
|
|
||||||
remote := remoteMap[id]
|
|
||||||
base := baseMap[id]
|
|
||||||
if local != nil && remote != nil {
|
|
||||||
mergedIssues = append(mergedIssues, mergeFieldLevel(base, local, remote))
|
|
||||||
} else if local != nil {
|
|
||||||
mergedIssues = append(mergedIssues, local)
|
mergedIssues = append(mergedIssues, local)
|
||||||
} else if remote != nil {
|
|
||||||
mergedIssues = append(mergedIssues, remote)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -4,8 +4,10 @@ import (
|
|||||||
"bufio"
|
"bufio"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"io"
|
||||||
"os"
|
"os"
|
||||||
"strings"
|
"strings"
|
||||||
|
"unicode/utf8"
|
||||||
|
|
||||||
"github.com/steveyegge/beads/internal/beads"
|
"github.com/steveyegge/beads/internal/beads"
|
||||||
"github.com/steveyegge/beads/internal/ui"
|
"github.com/steveyegge/beads/internal/ui"
|
||||||
@@ -21,8 +23,8 @@ type InteractiveConflict struct {
|
|||||||
|
|
||||||
// InteractiveResolution represents the user's choice for a conflict
|
// InteractiveResolution represents the user's choice for a conflict
|
||||||
type InteractiveResolution struct {
|
type InteractiveResolution struct {
|
||||||
Choice string // "local", "remote", "merged", "skip"
|
Choice string // "local", "remote", "merged", "skip", "quit", "accept-all"
|
||||||
Issue *beads.Issue // The resolved issue (nil if skipped)
|
Issue *beads.Issue // The resolved issue (nil if skipped/quit)
|
||||||
}
|
}
|
||||||
|
|
||||||
// resolveConflictsInteractively handles manual conflict resolution with user prompts.
|
// resolveConflictsInteractively handles manual conflict resolution with user prompts.
|
||||||
@@ -54,15 +56,42 @@ func resolveConflictsInteractively(conflicts []InteractiveConflict) ([]*beads.Is
|
|||||||
}
|
}
|
||||||
|
|
||||||
switch resolution.Choice {
|
switch resolution.Choice {
|
||||||
|
case "quit":
|
||||||
|
// Quit - skip all remaining conflicts
|
||||||
|
remaining := len(conflicts) - i
|
||||||
|
skipped += remaining
|
||||||
|
fmt.Printf(" %s Quit - skipping %d remaining conflict(s)\n\n", ui.RenderMuted("⏹"), remaining)
|
||||||
|
return resolved, skipped, nil
|
||||||
|
|
||||||
|
case "accept-all":
|
||||||
|
// Auto-merge all remaining conflicts
|
||||||
|
fmt.Printf(" %s Auto-merging %d remaining conflict(s)...\n", ui.RenderAccent("⚡"), len(conflicts)-i)
|
||||||
|
for j := i; j < len(conflicts); j++ {
|
||||||
|
c := conflicts[j]
|
||||||
|
if c.Local != nil && c.Remote != nil {
|
||||||
|
merged := mergeFieldLevel(c.Base, c.Local, c.Remote)
|
||||||
|
resolved = append(resolved, merged)
|
||||||
|
} else if c.Local != nil {
|
||||||
|
resolved = append(resolved, c.Local)
|
||||||
|
} else if c.Remote != nil {
|
||||||
|
resolved = append(resolved, c.Remote)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
fmt.Printf(" %s Done\n\n", ui.RenderPass("✓"))
|
||||||
|
return resolved, skipped, nil
|
||||||
|
|
||||||
case "skip":
|
case "skip":
|
||||||
skipped++
|
skipped++
|
||||||
fmt.Printf(" %s Skipped\n\n", ui.RenderMuted("⏭"))
|
fmt.Printf(" %s Skipped (will keep local, conflict remains)\n\n", ui.RenderMuted("⏭"))
|
||||||
|
|
||||||
case "local":
|
case "local":
|
||||||
resolved = append(resolved, conflict.Local)
|
resolved = append(resolved, conflict.Local)
|
||||||
fmt.Printf(" %s Kept local version\n\n", ui.RenderPass("✓"))
|
fmt.Printf(" %s Kept local version\n\n", ui.RenderPass("✓"))
|
||||||
|
|
||||||
case "remote":
|
case "remote":
|
||||||
resolved = append(resolved, conflict.Remote)
|
resolved = append(resolved, conflict.Remote)
|
||||||
fmt.Printf(" %s Kept remote version\n\n", ui.RenderPass("✓"))
|
fmt.Printf(" %s Kept remote version\n\n", ui.RenderPass("✓"))
|
||||||
|
|
||||||
case "merged":
|
case "merged":
|
||||||
resolved = append(resolved, resolution.Issue)
|
resolved = append(resolved, resolution.Issue)
|
||||||
fmt.Printf(" %s Used field-level merge\n\n", ui.RenderPass("✓"))
|
fmt.Printf(" %s Used field-level merge\n\n", ui.RenderPass("✓"))
|
||||||
@@ -187,7 +216,7 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict
|
|||||||
|
|
||||||
// Build options based on what's available
|
// Build options based on what's available
|
||||||
var options []string
|
var options []string
|
||||||
var optionMap = make(map[string]string)
|
optionMap := make(map[string]string)
|
||||||
|
|
||||||
if local != nil {
|
if local != nil {
|
||||||
options = append(options, "l")
|
options = append(options, "l")
|
||||||
@@ -205,9 +234,13 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict
|
|||||||
optionMap["merge"] = "merged"
|
optionMap["merge"] = "merged"
|
||||||
optionMap["merged"] = "merged"
|
optionMap["merged"] = "merged"
|
||||||
}
|
}
|
||||||
options = append(options, "s", "d", "?")
|
options = append(options, "s", "a", "q", "d", "?")
|
||||||
optionMap["s"] = "skip"
|
optionMap["s"] = "skip"
|
||||||
optionMap["skip"] = "skip"
|
optionMap["skip"] = "skip"
|
||||||
|
optionMap["a"] = "accept-all"
|
||||||
|
optionMap["all"] = "accept-all"
|
||||||
|
optionMap["q"] = "quit"
|
||||||
|
optionMap["quit"] = "quit"
|
||||||
optionMap["d"] = "diff"
|
optionMap["d"] = "diff"
|
||||||
optionMap["diff"] = "diff"
|
optionMap["diff"] = "diff"
|
||||||
optionMap["?"] = "help"
|
optionMap["?"] = "help"
|
||||||
@@ -217,6 +250,10 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict
|
|||||||
fmt.Printf(" Choice [%s]: ", strings.Join(options, "/"))
|
fmt.Printf(" Choice [%s]: ", strings.Join(options, "/"))
|
||||||
input, err := reader.ReadString('\n')
|
input, err := reader.ReadString('\n')
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
if err == io.EOF {
|
||||||
|
// Treat EOF as quit
|
||||||
|
return InteractiveResolution{Choice: "quit", Issue: nil}, nil
|
||||||
|
}
|
||||||
return InteractiveResolution{}, err
|
return InteractiveResolution{}, err
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -261,6 +298,12 @@ func promptConflictResolution(reader *bufio.Reader, conflict InteractiveConflict
|
|||||||
|
|
||||||
case "skip":
|
case "skip":
|
||||||
return InteractiveResolution{Choice: "skip", Issue: nil}, nil
|
return InteractiveResolution{Choice: "skip", Issue: nil}, nil
|
||||||
|
|
||||||
|
case "accept-all":
|
||||||
|
return InteractiveResolution{Choice: "accept-all", Issue: nil}, nil
|
||||||
|
|
||||||
|
case "quit":
|
||||||
|
return InteractiveResolution{Choice: "quit", Issue: nil}, nil
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -278,7 +321,9 @@ func printResolutionHelp(hasLocal, hasRemote bool) {
|
|||||||
if hasLocal && hasRemote {
|
if hasLocal && hasRemote {
|
||||||
fmt.Println(" m, merge - Auto-merge (LWW for scalars, union for collections)")
|
fmt.Println(" m, merge - Auto-merge (LWW for scalars, union for collections)")
|
||||||
}
|
}
|
||||||
fmt.Println(" s, skip - Skip this conflict (leave unresolved)")
|
fmt.Println(" s, skip - Skip this conflict (keep local, conflict remains)")
|
||||||
|
fmt.Println(" a, all - Accept auto-merge for all remaining conflicts")
|
||||||
|
fmt.Println(" q, quit - Quit and skip all remaining conflicts")
|
||||||
fmt.Println(" d, diff - Show detailed JSON diff")
|
fmt.Println(" d, diff - Show detailed JSON diff")
|
||||||
fmt.Println(" ?, help - Show this help")
|
fmt.Println(" ?, help - Show this help")
|
||||||
fmt.Println()
|
fmt.Println()
|
||||||
@@ -292,8 +337,12 @@ func showDetailedDiff(conflict InteractiveConflict) {
|
|||||||
|
|
||||||
if conflict.Local != nil {
|
if conflict.Local != nil {
|
||||||
fmt.Printf(" %s\n", ui.RenderAccent("LOCAL:"))
|
fmt.Printf(" %s\n", ui.RenderAccent("LOCAL:"))
|
||||||
localJSON, _ := json.MarshalIndent(conflict.Local, " ", " ")
|
localJSON, err := json.MarshalIndent(conflict.Local, " ", " ")
|
||||||
|
if err != nil {
|
||||||
|
fmt.Printf(" (error marshaling: %v)\n", err)
|
||||||
|
} else {
|
||||||
fmt.Println(string(localJSON))
|
fmt.Println(string(localJSON))
|
||||||
|
}
|
||||||
fmt.Println()
|
fmt.Println()
|
||||||
} else {
|
} else {
|
||||||
fmt.Printf(" %s (deleted)\n", ui.RenderMuted("LOCAL:"))
|
fmt.Printf(" %s (deleted)\n", ui.RenderMuted("LOCAL:"))
|
||||||
@@ -301,8 +350,12 @@ func showDetailedDiff(conflict InteractiveConflict) {
|
|||||||
|
|
||||||
if conflict.Remote != nil {
|
if conflict.Remote != nil {
|
||||||
fmt.Printf(" %s\n", ui.RenderAccent("REMOTE:"))
|
fmt.Printf(" %s\n", ui.RenderAccent("REMOTE:"))
|
||||||
remoteJSON, _ := json.MarshalIndent(conflict.Remote, " ", " ")
|
remoteJSON, err := json.MarshalIndent(conflict.Remote, " ", " ")
|
||||||
|
if err != nil {
|
||||||
|
fmt.Printf(" (error marshaling: %v)\n", err)
|
||||||
|
} else {
|
||||||
fmt.Println(string(remoteJSON))
|
fmt.Println(string(remoteJSON))
|
||||||
|
}
|
||||||
fmt.Println()
|
fmt.Println()
|
||||||
} else {
|
} else {
|
||||||
fmt.Printf(" %s (deleted)\n", ui.RenderMuted("REMOTE:"))
|
fmt.Printf(" %s (deleted)\n", ui.RenderMuted("REMOTE:"))
|
||||||
@@ -318,6 +371,8 @@ func valueOrNone(s string) string {
|
|||||||
return s
|
return s
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// truncateText truncates a string to maxLen runes (not bytes) for proper UTF-8 handling.
|
||||||
|
// Replaces newlines with spaces for single-line display.
|
||||||
func truncateText(s string, maxLen int) string {
|
func truncateText(s string, maxLen int) string {
|
||||||
if s == "" {
|
if s == "" {
|
||||||
return "(empty)"
|
return "(empty)"
|
||||||
@@ -325,8 +380,17 @@ func truncateText(s string, maxLen int) string {
|
|||||||
// Replace newlines with spaces for display
|
// Replace newlines with spaces for display
|
||||||
s = strings.ReplaceAll(s, "\n", " ")
|
s = strings.ReplaceAll(s, "\n", " ")
|
||||||
s = strings.ReplaceAll(s, "\r", "")
|
s = strings.ReplaceAll(s, "\r", "")
|
||||||
if len(s) > maxLen {
|
|
||||||
return s[:maxLen-3] + "..."
|
// Count runes, not bytes, for proper UTF-8 handling
|
||||||
}
|
runeCount := utf8.RuneCountInString(s)
|
||||||
|
if runeCount <= maxLen {
|
||||||
return s
|
return s
|
||||||
|
}
|
||||||
|
|
||||||
|
// Truncate by runes
|
||||||
|
runes := []rune(s)
|
||||||
|
if maxLen <= 3 {
|
||||||
|
return "..."
|
||||||
|
}
|
||||||
|
return string(runes[:maxLen-3]) + "..."
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -45,6 +45,36 @@ func TestTruncateText(t *testing.T) {
|
|||||||
maxLen: 30,
|
maxLen: 30,
|
||||||
want: "line1 line2 line3",
|
want: "line1 line2 line3",
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "very short max",
|
||||||
|
input: "hello world",
|
||||||
|
maxLen: 3,
|
||||||
|
want: "...",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "UTF-8 characters preserved",
|
||||||
|
input: "Hello 世界!This is a test",
|
||||||
|
maxLen: 12,
|
||||||
|
want: "Hello 世界!...",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "UTF-8 exact length",
|
||||||
|
input: "日本語テスト",
|
||||||
|
maxLen: 6,
|
||||||
|
want: "日本語テスト",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "UTF-8 truncate",
|
||||||
|
input: "日本語テストです",
|
||||||
|
maxLen: 6,
|
||||||
|
want: "日本語...",
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "emoji handling",
|
||||||
|
input: "Hello 🌍🌎🌏 World",
|
||||||
|
maxLen: 12,
|
||||||
|
want: "Hello 🌍🌎🌏...",
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
@@ -164,6 +194,14 @@ func TestInteractiveConflictDisplay(t *testing.T) {
|
|||||||
},
|
},
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "both nil (edge case)",
|
||||||
|
conflict: InteractiveConflict{
|
||||||
|
IssueID: "test-6",
|
||||||
|
Local: nil,
|
||||||
|
Remote: nil,
|
||||||
|
},
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
@@ -177,7 +215,13 @@ func TestInteractiveConflictDisplay(t *testing.T) {
|
|||||||
func TestShowDetailedDiff(t *testing.T) {
|
func TestShowDetailedDiff(t *testing.T) {
|
||||||
now := time.Now()
|
now := time.Now()
|
||||||
|
|
||||||
conflict := InteractiveConflict{
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
conflict InteractiveConflict
|
||||||
|
}{
|
||||||
|
{
|
||||||
|
name: "both exist",
|
||||||
|
conflict: InteractiveConflict{
|
||||||
IssueID: "test-1",
|
IssueID: "test-1",
|
||||||
Local: &beads.Issue{
|
Local: &beads.Issue{
|
||||||
ID: "test-1",
|
ID: "test-1",
|
||||||
@@ -189,10 +233,40 @@ func TestShowDetailedDiff(t *testing.T) {
|
|||||||
Title: "Remote",
|
Title: "Remote",
|
||||||
UpdatedAt: now,
|
UpdatedAt: now,
|
||||||
},
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "local nil",
|
||||||
|
conflict: InteractiveConflict{
|
||||||
|
IssueID: "test-2",
|
||||||
|
Local: nil,
|
||||||
|
Remote: &beads.Issue{
|
||||||
|
ID: "test-2",
|
||||||
|
Title: "Remote",
|
||||||
|
UpdatedAt: now,
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "remote nil",
|
||||||
|
conflict: InteractiveConflict{
|
||||||
|
IssueID: "test-3",
|
||||||
|
Local: &beads.Issue{
|
||||||
|
ID: "test-3",
|
||||||
|
Title: "Local",
|
||||||
|
UpdatedAt: now,
|
||||||
|
},
|
||||||
|
Remote: nil,
|
||||||
|
},
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
// Just make sure it doesn't panic
|
// Just make sure it doesn't panic
|
||||||
showDetailedDiff(conflict)
|
showDetailedDiff(tt.conflict)
|
||||||
|
})
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestPrintResolutionHelp(t *testing.T) {
|
func TestPrintResolutionHelp(t *testing.T) {
|
||||||
@@ -272,3 +346,34 @@ func TestInteractiveResolutionMerge(t *testing.T) {
|
|||||||
t.Errorf("Expected labels to contain 'bug' and 'feature', got %v", merged.Labels)
|
t.Errorf("Expected labels to contain 'bug' and 'feature', got %v", merged.Labels)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestInteractiveResolutionChoices(t *testing.T) {
|
||||||
|
// Test InteractiveResolution struct values
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
choice string
|
||||||
|
issue *beads.Issue
|
||||||
|
}{
|
||||||
|
{"local", "local", &beads.Issue{ID: "test"}},
|
||||||
|
{"remote", "remote", &beads.Issue{ID: "test"}},
|
||||||
|
{"merged", "merged", &beads.Issue{ID: "test"}},
|
||||||
|
{"skip", "skip", nil},
|
||||||
|
{"quit", "quit", nil},
|
||||||
|
{"accept-all", "accept-all", nil},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
res := InteractiveResolution{Choice: tt.choice, Issue: tt.issue}
|
||||||
|
if res.Choice != tt.choice {
|
||||||
|
t.Errorf("Expected choice %q, got %q", tt.choice, res.Choice)
|
||||||
|
}
|
||||||
|
if tt.issue == nil && res.Issue != nil {
|
||||||
|
t.Errorf("Expected nil issue, got %v", res.Issue)
|
||||||
|
}
|
||||||
|
if tt.issue != nil && res.Issue == nil {
|
||||||
|
t.Errorf("Expected non-nil issue, got nil")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user