From 2aaf7f0de37c70a3504b5834c91a85affa83df77 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 21:27:16 +0100 Subject: [PATCH 01/10] Improve env config parsing and upgrade logic Refactor config parsing and upgrade flow to better handle comments, casing and multiline values. - setEnvValue: skip commented lines, preserve indentation and inline comments when replacing values. - parseEnvFile/parseEnvValues: ignore comment-only lines inside multi-line block values; return comment text and a caseMap (UPPER->original) for case-insensitive matching; add comment storage to envValue. - splitKeyValueRaw: return parsed inline comment, handle legacy "export " prefixes, and more robustly extract quoted/unquoted values and trailing comments. - computeConfigUpgrade: use case-insensitive lookups via caseMap, track processed user keys, preserve user key casing for extras, count preserved values correctly, and detect no-op upgrades by comparing newContent with originalContent. - renderEnvValue: include preserved inline comments when rendering single-line values. - Added import of utils.IsComment where needed. Also updated internal/orchestrator/.backup.lock (pid/time) as part of the changeset. --- cmd/proxsave/config_helpers.go | 42 +++++++++--- internal/config/config.go | 4 +- internal/config/upgrade.go | 105 ++++++++++++++++++++++------- internal/orchestrator/.backup.lock | 4 +- 4 files changed, 118 insertions(+), 37 deletions(-) diff --git a/cmd/proxsave/config_helpers.go b/cmd/proxsave/config_helpers.go index 9644de0..e5dca47 100644 --- a/cmd/proxsave/config_helpers.go +++ b/cmd/proxsave/config_helpers.go @@ -5,6 +5,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/tis24dev/proxsave/pkg/utils" ) type configStatusLogger interface { @@ -47,27 +49,44 @@ func ensureConfigExists(path string, logger configStatusLogger) error { } func setEnvValue(template, key, value string) string { - target := key + "=" lines := strings.Split(template, "\n") replaced := false for i, line := range lines { trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, target) { + if utils.IsComment(trimmed) { + continue + } + + parts := strings.SplitN(trimmed, "=", 2) + if len(parts) >= 1 && strings.TrimSpace(parts[0]) == key { + // Found match! + // We try to preserve the indentation and comments from the original line. leadingLen := len(line) - len(strings.TrimLeft(line, " \t")) leading := "" if leadingLen > 0 { leading = line[:leadingLen] } - rest := line[leadingLen:] - commentSpacing := "" + + // Extract comment if present in the original line logic + // The original logic extracted comment from 'rest' after target match. + // Here we can re-parse the line specifically for comment. comment := "" - if idx := strings.Index(rest, "#"); idx >= 0 { - before := rest[:idx] - comment = rest[idx:] - trimmedBefore := strings.TrimRight(before, " \t") - commentSpacing = before[len(trimmedBefore):] - rest = trimmedBefore + commentSpacing := "" + + if idx := strings.Index(line, "#"); idx >= 0 { + // Verify # is not part of the key or value? + // Assuming standard comment + commentPart := line[idx:] + // Ensure it's not inside quotes? The original logic didn't check quotes carefully but let's be safe(r). + // For setEnvValue we are replacing the value, so we just want to keep the comment at the end. + comment = commentPart + + // Find spacing before comment + beforeComment := line[:idx] + trimmedBefore := strings.TrimRight(beforeComment, " \t") + commentSpacing = beforeComment[len(trimmedBefore):] } + newLine := leading + key + "=" + value if comment != "" { spacing := commentSpacing @@ -78,6 +97,9 @@ func setEnvValue(template, key, value string) string { } lines[i] = newLine replaced = true + // We stop after first match? Original code didn't break, but typically keys are unique. + // Let's break to avoid multiple replacements if file is messy (or continue to fix all?) + // Original didn't break. Let's not break to match behavior. } } if !replaced { diff --git a/internal/config/config.go b/internal/config/config.go index a566c72..171784c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1228,7 +1228,9 @@ func parseEnvFile(path string) (map[string]string, error) { terminated = true break } - blockLines = append(blockLines, next) + if !utils.IsComment(strings.TrimSpace(next)) { + blockLines = append(blockLines, next) + } } if !terminated { return nil, fmt.Errorf("unterminated multi-line value for %s", key) diff --git a/internal/config/upgrade.go b/internal/config/upgrade.go index 2b59107..76774cc 100644 --- a/internal/config/upgrade.go +++ b/internal/config/upgrade.go @@ -20,6 +20,7 @@ type envValue struct { kind envValueKind rawValue string blockLines []string + comment string } // UpgradeResult describes the outcome of a configuration upgrade. @@ -155,7 +156,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er originalLines := strings.Split(normalizedOriginal, "\n") // 1. Collect user values: for each KEY we store all VALUE entries in order. - userValues, userKeyOrder, err := parseEnvValues(originalLines) + userValues, userKeyOrder, caseMap, err := parseEnvValues(originalLines) if err != nil { return result, "", originalContent, fmt.Errorf("failed to parse config %s: %w", configPath, err) } @@ -168,6 +169,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er templateKeys := make(map[string]bool) missingKeys := make([]string, 0) newLines := make([]string, 0, len(templateLines)+len(userValues)) + processedUserKeys := make(map[string]bool) // Track which user keys (original case) have been used for i := 0; i < len(templateLines); i++ { line := templateLines[i] @@ -177,7 +179,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er continue } - key, _, ok := splitKeyValueRaw(line) + key, _, _, ok := splitKeyValueRaw(line) if !ok || key == "" { newLines = append(newLines, line) continue @@ -185,14 +187,27 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er templateKeys[key] = true + // Logic to find the user's values for this key. + // 1. Try exact match + targetUserKey := key + if _, ok := userValues[key]; !ok { + // 2. Try case-insensitive match + if mappedKey, ok := caseMap[strings.ToUpper(key)]; ok { + targetUserKey = mappedKey + } + } + + // Handle block values if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { blockEnd, err := findClosingQuoteLine(templateLines, i+1) if err != nil { return result, "", originalContent, fmt.Errorf("template %s block invalid: %w", key, err) } - if values, ok := userValues[key]; ok && len(values) > 0 { + if values, ok := userValues[targetUserKey]; ok && len(values) > 0 { + processedUserKeys[targetUserKey] = true for _, v := range values { + // Use TEMPLATE Key casing to enforce consistency newLines = append(newLines, renderEnvValue(key, v)...) } } else { @@ -204,8 +219,10 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er continue } - if values, ok := userValues[key]; ok && len(values) > 0 { + if values, ok := userValues[targetUserKey]; ok && len(values) > 0 { + processedUserKeys[targetUserKey] = true for _, v := range values { + // Use TEMPLATE Key casing to enforce consistency newLines = append(newLines, renderEnvValue(key, v)...) } } else { @@ -220,15 +237,28 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er extraLines := make([]string, 0) for _, key := range userKeyOrder { + if processedUserKeys[key] { + continue + } + // If exact match was in template keys (should have been processed above), skip if templateKeys[key] { continue } + // If case-insensitive match was in template keys (should have been processed), skip + // Check by upper casing the user key and seeing if it exists in templateKeys? + // But wait, templateKeys stores exact keys. + // If user has "Backup_Enabled", and template has "BACKUP_ENABLED". + // We processed "BACKUP_ENABLED", found "Backup_Enabled" via caseMap, and marked "Backup_Enabled" as processed. + // So `processedUserKeys["Backup_Enabled"]` is true. We skip. + // Correct. + values := userValues[key] if len(values) == 0 { continue } extraKeys = append(extraKeys, key) for _, v := range values { + // Preserve USER's original key casing for extras extraLines = append(extraLines, renderEnvValue(key, v)...) } } @@ -243,20 +273,17 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er newLines = append(newLines, extraLines...) } - // Count preserved values: key=value pairs coming from user config for - // keys that exist in the template. + // Count preserved values preserved := 0 - for key, values := range userValues { - if templateKeys[key] { - preserved += len(values) - } + for key := range processedUserKeys { + preserved += len(userValues[key]) } // If nothing changed (no missing keys and no extras), we can return early. - if len(missingKeys) == 0 && len(extraKeys) == 0 { - result.PreservedValues = preserved - return result, "", originalContent, nil - } + // BUT checking "nothing changed" is harder now because we might have renamed keys. + // If we renamed a key, the content CHANGED. + // So we should compare normalized content? + // Or just assume if we parsed everything and re-rendered, and it matches original string... newContent := strings.Join(newLines, lineEnding) // Preserve trailing newline if template had one. @@ -264,6 +291,12 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er newContent += lineEnding } + if newContent == string(originalContent) { + result.Changed = false + result.PreservedValues = preserved + return result, "", originalContent, nil + } + result.MissingKeys = missingKeys result.ExtraKeys = extraKeys result.PreservedValues = preserved @@ -271,9 +304,10 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er return result, newContent, originalContent, nil } -func parseEnvValues(lines []string) (map[string][]envValue, []string, error) { +func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string]string, error) { userValues := make(map[string][]envValue) userKeyOrder := make([]string, 0) + caseMap := make(map[string]string) // UPPER -> original for i := 0; i < len(lines); i++ { line := lines[i] @@ -282,7 +316,7 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, error) { continue } - key, rawValue, ok := splitKeyValueRaw(line) + key, rawValue, comment, ok := splitKeyValueRaw(line) if !ok || key == "" { continue } @@ -291,12 +325,13 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, error) { blockLines := make([]string, 0) blockEnd, err := findClosingQuoteLine(lines, i+1) if err != nil { - return nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1) + return nil, nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1) } blockLines = append(blockLines, lines[i+1:blockEnd]...) if _, seen := userValues[key]; !seen { userKeyOrder = append(userKeyOrder, key) + caseMap[strings.ToUpper(key)] = key } userValues[key] = append(userValues[key], envValue{kind: envValueKindBlock, blockLines: blockLines}) @@ -306,39 +341,57 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, error) { if _, seen := userValues[key]; !seen { userKeyOrder = append(userKeyOrder, key) + caseMap[strings.ToUpper(key)] = key } - userValues[key] = append(userValues[key], envValue{kind: envValueKindLine, rawValue: rawValue}) + userValues[key] = append(userValues[key], envValue{kind: envValueKindLine, rawValue: rawValue, comment: comment}) } - return userValues, userKeyOrder, nil + return userValues, userKeyOrder, caseMap, nil } -func splitKeyValueRaw(line string) (string, string, bool) { +func splitKeyValueRaw(line string) (string, string, string, bool) { parts := strings.SplitN(line, "=", 2) if len(parts) != 2 { - return "", "", false + return "", "", "", false } key := strings.TrimSpace(parts[0]) + // Handle legacy "export KEY=VALUE" lines + if strings.HasPrefix(key, "export ") { + key = strings.TrimSpace(strings.TrimPrefix(key, "export ")) + } + // Also handle tab separation just in case "export\tKEY" + if strings.HasPrefix(key, "export\t") { + key = strings.TrimSpace(strings.TrimPrefix(key, "export\t")) + } + valuePart := strings.TrimSpace(parts[1]) // Remove inline comments (but respect quotes) value := valuePart + comment := "" + if strings.HasPrefix(valuePart, "\"") || strings.HasPrefix(valuePart, "'") { quote := valuePart[0] endIdx := strings.IndexByte(valuePart[1:], quote) if endIdx >= 0 { value = valuePart[:endIdx+2] + // Check for comment after quote + rest := valuePart[endIdx+2:] + if idx := strings.Index(rest, "#"); idx >= 0 { + comment = strings.TrimSpace(rest[idx:]) + } } - return key, value, true + return key, value, comment, true } // Not quoted, remove everything after # if idx := strings.Index(valuePart, "#"); idx >= 0 { value = strings.TrimSpace(valuePart[:idx]) + comment = strings.TrimSpace(valuePart[idx:]) } - return key, value, true + return key, value, comment, true } func findClosingQuoteLine(lines []string, start int) (int, error) { @@ -357,5 +410,9 @@ func renderEnvValue(key string, value envValue) []string { lines = append(lines, "\"") return lines } - return []string{fmt.Sprintf("%s=%s", key, value.rawValue)} + line := fmt.Sprintf("%s=%s", key, value.rawValue) + if value.comment != "" { + line += " " + value.comment + } + return []string{line} } diff --git a/internal/orchestrator/.backup.lock b/internal/orchestrator/.backup.lock index 2893e45..fffdac2 100644 --- a/internal/orchestrator/.backup.lock +++ b/internal/orchestrator/.backup.lock @@ -1,3 +1,3 @@ -pid=17956 +pid=99930 host=pve -time=2026-02-03T00:07:31+01:00 +time=2026-02-03T19:42:03+01:00 From da176df22d3f1dab77cad0a2bd59cecdac83d7bd Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 22:02:42 +0100 Subject: [PATCH 02/10] Report config upgrade warnings Add non-fatal warnings support to the config upgrade flow. Introduce a Warnings []string field on UpgradeResult and propagate warning messages from parseEnvValues through computeConfigUpgrade. Detect and warn about duplicate template keys that differ only by case, case-collision between user keys and template keys, and ignored non-KEY=VALUE lines; preserve behavior for multi-line values. Surface these warnings in the CLI (cmd/proxsave/main.go and cmd/proxsave/upgrade.go) so they are printed during plan/upgrade operations. Update parseEnvValues signature to return case conflict info and warnings, and add tests verifying case-collision and ignored-line warnings. --- cmd/proxsave/main.go | 12 +++++++++ cmd/proxsave/upgrade.go | 8 ++++++ internal/config/upgrade.go | 43 ++++++++++++++++++++++++++----- internal/config/upgrade_test.go | 45 +++++++++++++++++++++++++++++++++ 4 files changed, 102 insertions(+), 6 deletions(-) diff --git a/cmd/proxsave/main.go b/cmd/proxsave/main.go index d7e9c34..0daa06d 100644 --- a/cmd/proxsave/main.go +++ b/cmd/proxsave/main.go @@ -334,6 +334,12 @@ func run() int { bootstrap.Error("ERROR: Failed to plan configuration upgrade: %v", err) return types.ExitConfigError.Int() } + if len(result.Warnings) > 0 { + bootstrap.Warning("Config upgrade warnings (%d):", len(result.Warnings)) + for _, warning := range result.Warnings { + bootstrap.Warning(" - %s", warning) + } + } if !result.Changed { bootstrap.Println("Configuration is already up to date with the embedded template; no changes are required.") return types.ExitSuccess.Int() @@ -437,6 +443,12 @@ func run() int { bootstrap.Error("ERROR: Failed to upgrade configuration: %v", err) return types.ExitConfigError.Int() } + if len(result.Warnings) > 0 { + bootstrap.Warning("Config upgrade warnings (%d):", len(result.Warnings)) + for _, warning := range result.Warnings { + bootstrap.Warning(" - %s", warning) + } + } if !result.Changed { bootstrap.Println("Configuration is already up to date with the embedded template; no changes were made.") return types.ExitSuccess.Int() diff --git a/cmd/proxsave/upgrade.go b/cmd/proxsave/upgrade.go index 9a09107..6a35f7b 100644 --- a/cmd/proxsave/upgrade.go +++ b/cmd/proxsave/upgrade.go @@ -589,6 +589,14 @@ func printUpgradeFooter(upgradeErr error, version, configPath, baseDir, telegram } } + if cfgUpgradeResult != nil && len(cfgUpgradeResult.Warnings) > 0 { + fmt.Printf("Configuration warnings (%d):\n", len(cfgUpgradeResult.Warnings)) + for _, warning := range cfgUpgradeResult.Warnings { + fmt.Printf(" - %s\n", warning) + } + fmt.Println() + } + fmt.Println("Next steps:") if strings.TrimSpace(configPath) != "" { fmt.Printf("1. Verify configuration: %s\n", configPath) diff --git a/internal/config/upgrade.go b/internal/config/upgrade.go index 76774cc..ac736a4 100644 --- a/internal/config/upgrade.go +++ b/internal/config/upgrade.go @@ -33,6 +33,8 @@ type UpgradeResult struct { // ExtraKeys are keys that were present in the user's config but not in the // template. They are preserved in a dedicated "Custom keys" section. ExtraKeys []string + // Warnings includes non-fatal parsing or merge issues detected while upgrading. + Warnings []string // PreservedValues is the number of existing key=value pairs from the user's // configuration that were kept during the merge for keys present in the // template. @@ -156,7 +158,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er originalLines := strings.Split(normalizedOriginal, "\n") // 1. Collect user values: for each KEY we store all VALUE entries in order. - userValues, userKeyOrder, caseMap, err := parseEnvValues(originalLines) + userValues, userKeyOrder, caseMap, caseConflicts, warnings, err := parseEnvValues(originalLines) if err != nil { return result, "", originalContent, fmt.Errorf("failed to parse config %s: %w", configPath, err) } @@ -167,6 +169,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er templateLines := strings.Split(normalizedTemplate, "\n") templateKeys := make(map[string]bool) + templateKeyByUpper := make(map[string]string) missingKeys := make([]string, 0) newLines := make([]string, 0, len(templateLines)+len(userValues)) processedUserKeys := make(map[string]bool) // Track which user keys (original case) have been used @@ -186,6 +189,14 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er } templateKeys[key] = true + upperKey := strings.ToUpper(key) + if existing, ok := templateKeyByUpper[upperKey]; ok { + if existing != key { + warnings = append(warnings, fmt.Sprintf("Template contains duplicate keys differing only by case: %q and %q", existing, key)) + } + } else { + templateKeyByUpper[upperKey] = key + } // Logic to find the user's values for this key. // 1. Try exact match @@ -252,6 +263,13 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er // So `processedUserKeys["Backup_Enabled"]` is true. We skip. // Correct. + upperKey := strings.ToUpper(key) + if templateKey, ok := templateKeyByUpper[upperKey]; ok && templateKey != key { + if caseConflicts == nil || !caseConflicts[upperKey] { + warnings = append(warnings, fmt.Sprintf("Key %q differs only by case from template key %q; preserved as custom entry", key, templateKey)) + } + } + values := userValues[key] if len(values) == 0 { continue @@ -293,6 +311,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er if newContent == string(originalContent) { result.Changed = false + result.Warnings = warnings result.PreservedValues = preserved return result, "", originalContent, nil } @@ -300,14 +319,17 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er result.MissingKeys = missingKeys result.ExtraKeys = extraKeys result.PreservedValues = preserved + result.Warnings = warnings result.Changed = true return result, newContent, originalContent, nil } -func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string]string, error) { +func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string]string, map[string]bool, []string, error) { userValues := make(map[string][]envValue) userKeyOrder := make([]string, 0) caseMap := make(map[string]string) // UPPER -> original + caseConflicts := make(map[string]bool) + warnings := make([]string, 0) for i := 0; i < len(lines); i++ { line := lines[i] @@ -318,20 +340,30 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string key, rawValue, comment, ok := splitKeyValueRaw(line) if !ok || key == "" { + if trimmed != "" { + warnings = append(warnings, fmt.Sprintf("Ignored line %d: not a KEY=VALUE entry", i+1)) + } continue } + upperKey := strings.ToUpper(key) + if existing, ok := caseMap[upperKey]; ok && existing != key { + caseConflicts[upperKey] = true + warnings = append(warnings, fmt.Sprintf("Duplicate keys differ only by case: %q and %q (using last occurrence %q)", existing, key, key)) + } + + caseMap[upperKey] = key + if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { blockLines := make([]string, 0) blockEnd, err := findClosingQuoteLine(lines, i+1) if err != nil { - return nil, nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1) + return nil, nil, nil, nil, nil, fmt.Errorf("unterminated multi-line value for %s starting at line %d", key, i+1) } blockLines = append(blockLines, lines[i+1:blockEnd]...) if _, seen := userValues[key]; !seen { userKeyOrder = append(userKeyOrder, key) - caseMap[strings.ToUpper(key)] = key } userValues[key] = append(userValues[key], envValue{kind: envValueKindBlock, blockLines: blockLines}) @@ -341,12 +373,11 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string if _, seen := userValues[key]; !seen { userKeyOrder = append(userKeyOrder, key) - caseMap[strings.ToUpper(key)] = key } userValues[key] = append(userValues[key], envValue{kind: envValueKindLine, rawValue: rawValue, comment: comment}) } - return userValues, userKeyOrder, caseMap, nil + return userValues, userKeyOrder, caseMap, caseConflicts, warnings, nil } func splitKeyValueRaw(line string) (string, string, string, bool) { diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index 822e4da..9b9eed4 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -232,3 +232,48 @@ CUSTOM_BACKUP_PATHS=" } }) } + +func TestPlanUpgradeConfigWarnsOnCaseCollision(t *testing.T) { + template := "BACKUP_PATH=/default/backup\n" + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + content := "backup_path=/lower\nBACKUP_PATH=/upper\n" + if err := os.WriteFile(configPath, []byte(content), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + + result, err := PlanUpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("PlanUpgradeConfigFile returned error: %v", err) + } + if len(result.ExtraKeys) != 1 || result.ExtraKeys[0] != "backup_path" { + t.Fatalf("ExtraKeys = %v; want [backup_path]", result.ExtraKeys) + } + warnings := strings.Join(result.Warnings, "\n") + if !strings.Contains(warnings, "Duplicate keys differ only by case") { + t.Fatalf("expected case-collision warning, got: %s", warnings) + } + }) +} + +func TestPlanUpgradeConfigWarnsOnIgnoredLine(t *testing.T) { + template := "BACKUP_PATH=/default/backup\n" + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + content := "BACKUP_PATH=/legacy\nNOT_A_KEY\n" + if err := os.WriteFile(configPath, []byte(content), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + + result, err := PlanUpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("PlanUpgradeConfigFile returned error: %v", err) + } + warnings := strings.Join(result.Warnings, "\n") + if !strings.Contains(warnings, "Ignored line 2") { + t.Fatalf("expected ignored-line warning, got: %s", warnings) + } + }) +} From 25de99fa7a84ecc851af523734b034437c55784c Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:18:10 +0100 Subject: [PATCH 03/10] Add env parsing utilities and unify setEnvValue Introduce utils string helpers (FindInlineCommentIndex, FindClosingQuoteIndex, SetEnvValue) to centralize handling of inline comments, escaped hashes and quoted values. Replace local setEnvValue implementations to call utils.SetEnvValue and update SplitKeyValue/splitKeyValueRaw to use the new helpers for robust parsing. Fix case-insensitive handling of block value keys in config upgrade logic (use upperKey). Add unit tests across packages to cover escaped hashes, preserved comments, quoted values, and SetEnvValue behavior. Minor reformatting/import adjustments in the install wizard to integrate the new utils usage. --- cmd/proxsave/config_helpers.go | 58 +------------ cmd/proxsave/helpers_test.go | 18 ++++ internal/config/upgrade.go | 33 +++----- internal/config/upgrade_test.go | 41 +++++++++ internal/tui/wizard/install.go | 104 ++++++++++------------- internal/tui/wizard/install_test.go | 22 +++++ pkg/utils/strings.go | 126 ++++++++++++++++++++++++---- pkg/utils/strings_test.go | 80 ++++++++++++++++++ 8 files changed, 330 insertions(+), 152 deletions(-) diff --git a/cmd/proxsave/config_helpers.go b/cmd/proxsave/config_helpers.go index e5dca47..0d3d0ca 100644 --- a/cmd/proxsave/config_helpers.go +++ b/cmd/proxsave/config_helpers.go @@ -49,63 +49,7 @@ func ensureConfigExists(path string, logger configStatusLogger) error { } func setEnvValue(template, key, value string) string { - lines := strings.Split(template, "\n") - replaced := false - for i, line := range lines { - trimmed := strings.TrimSpace(line) - if utils.IsComment(trimmed) { - continue - } - - parts := strings.SplitN(trimmed, "=", 2) - if len(parts) >= 1 && strings.TrimSpace(parts[0]) == key { - // Found match! - // We try to preserve the indentation and comments from the original line. - leadingLen := len(line) - len(strings.TrimLeft(line, " \t")) - leading := "" - if leadingLen > 0 { - leading = line[:leadingLen] - } - - // Extract comment if present in the original line logic - // The original logic extracted comment from 'rest' after target match. - // Here we can re-parse the line specifically for comment. - comment := "" - commentSpacing := "" - - if idx := strings.Index(line, "#"); idx >= 0 { - // Verify # is not part of the key or value? - // Assuming standard comment - commentPart := line[idx:] - // Ensure it's not inside quotes? The original logic didn't check quotes carefully but let's be safe(r). - // For setEnvValue we are replacing the value, so we just want to keep the comment at the end. - comment = commentPart - - // Find spacing before comment - beforeComment := line[:idx] - trimmedBefore := strings.TrimRight(beforeComment, " \t") - commentSpacing = beforeComment[len(trimmedBefore):] - } - - newLine := leading + key + "=" + value - if comment != "" { - spacing := commentSpacing - if spacing == "" { - spacing = " " - } - newLine += spacing + comment - } - lines[i] = newLine - replaced = true - // We stop after first match? Original code didn't break, but typically keys are unique. - // Let's break to avoid multiple replacements if file is messy (or continue to fix all?) - // Original didn't break. Let's not break to match behavior. - } - } - if !replaced { - lines = append(lines, key+"="+value) - } - return strings.Join(lines, "\n") + return utils.SetEnvValue(template, key, value) } func sanitizeEnvValue(value string) string { diff --git a/cmd/proxsave/helpers_test.go b/cmd/proxsave/helpers_test.go index abcbb40..e27d735 100644 --- a/cmd/proxsave/helpers_test.go +++ b/cmd/proxsave/helpers_test.go @@ -36,6 +36,24 @@ func TestSetEnvValue_PreserveComment(t *testing.T) { assertContains(t, result, "# this is a comment") } +func TestSetEnvValue_PreserveCommentAfterQuotedValue(t *testing.T) { + template := `FOO="old # keep" # trailing comment` + + result := setEnvValue(template, "FOO", "new") + + assertContains(t, result, "FOO=new") + assertContains(t, result, "# trailing comment") +} + +func TestSetEnvValue_PreserveCommentWithEscapedHash(t *testing.T) { + template := `FOO=old\#keep # trailing comment` + + result := setEnvValue(template, "FOO", "new") + + assertContains(t, result, "FOO=new") + assertContains(t, result, "# trailing comment") +} + func TestSetEnvValue_AddNew(t *testing.T) { template := "FOO=bar" diff --git a/internal/config/upgrade.go b/internal/config/upgrade.go index ac736a4..2faef7a 100644 --- a/internal/config/upgrade.go +++ b/internal/config/upgrade.go @@ -209,7 +209,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er } // Handle block values - if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { + if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) { blockEnd, err := findClosingQuoteLine(templateLines, i+1) if err != nil { return result, "", originalContent, fmt.Errorf("template %s block invalid: %w", key, err) @@ -354,7 +354,7 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string caseMap[upperKey] = key - if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { + if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) { blockLines := make([]string, 0) blockEnd, err := findClosingQuoteLine(lines, i+1) if err != nil { @@ -381,7 +381,14 @@ func parseEnvValues(lines []string) (map[string][]envValue, []string, map[string } func splitKeyValueRaw(line string) (string, string, string, bool) { - parts := strings.SplitN(line, "=", 2) + comment := "" + body := line + if commentIdx := utils.FindInlineCommentIndex(line); commentIdx >= 0 { + comment = strings.TrimSpace(line[commentIdx:]) + body = line[:commentIdx] + } + + parts := strings.SplitN(body, "=", 2) if len(parts) != 2 { return "", "", "", false } @@ -398,28 +405,12 @@ func splitKeyValueRaw(line string) (string, string, string, bool) { valuePart := strings.TrimSpace(parts[1]) - // Remove inline comments (but respect quotes) value := valuePart - comment := "" - if strings.HasPrefix(valuePart, "\"") || strings.HasPrefix(valuePart, "'") { quote := valuePart[0] - endIdx := strings.IndexByte(valuePart[1:], quote) - if endIdx >= 0 { - value = valuePart[:endIdx+2] - // Check for comment after quote - rest := valuePart[endIdx+2:] - if idx := strings.Index(rest, "#"); idx >= 0 { - comment = strings.TrimSpace(rest[idx:]) - } + if endIdx := utils.FindClosingQuoteIndex(valuePart, quote); endIdx >= 0 { + value = valuePart[:endIdx+1] } - return key, value, comment, true - } - - // Not quoted, remove everything after # - if idx := strings.Index(valuePart, "#"); idx >= 0 { - value = strings.TrimSpace(valuePart[:idx]) - comment = strings.TrimSpace(valuePart[idx:]) } return key, value, comment, true diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index 9b9eed4..cb1a25f 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -277,3 +277,44 @@ func TestPlanUpgradeConfigWarnsOnIgnoredLine(t *testing.T) { } }) } + +func TestUpgradeConfigPreservesBlockValuesCaseInsensitive(t *testing.T) { + template := `BACKUP_PATH=/default/backup +LOG_PATH=/default/log +CUSTOM_BACKUP_PATHS=" +# /template/example +" +` + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + userConfig := `BACKUP_PATH=/legacy/backup +Custom_Backup_Paths=" +/etc/custom.conf +" +` + if err := os.WriteFile(configPath, []byte(userConfig), 0600); err != nil { + t.Fatalf("failed to seed config: %v", err) + } + + result, err := UpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("UpgradeConfigFile returned error: %v", err) + } + if !result.Changed { + t.Fatal("expected result.Changed=true when template has missing keys") + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("failed to read upgraded config: %v", err) + } + // strings.ReplaceAll might not be available if "strings" isn't imported, but it is imported in upgrade_test.go + content := strings.ReplaceAll(string(data), "\r\n", "\n") + + expectedBlock := "CUSTOM_BACKUP_PATHS=\"\n/etc/custom.conf\n\"\n" + if !strings.Contains(content, expectedBlock) { + t.Fatalf("upgraded config missing preserved block with fixed casing:\nGot:\n%s\nWant contains:\n%s", content, expectedBlock) + } + }) +} diff --git a/internal/tui/wizard/install.go b/internal/tui/wizard/install.go index f446cb3..e82c220 100644 --- a/internal/tui/wizard/install.go +++ b/internal/tui/wizard/install.go @@ -15,6 +15,7 @@ import ( "github.com/tis24dev/proxsave/internal/config" "github.com/tis24dev/proxsave/internal/tui" "github.com/tis24dev/proxsave/internal/tui/components" + "github.com/tis24dev/proxsave/pkg/utils" ) // InstallWizardData holds the collected installation data @@ -54,10 +55,10 @@ var ( func RunInstallWizard(ctx context.Context, configPath string, baseDir string, buildSig string) (*InstallWizardData, error) { defaultFirewallRules := false data := &InstallWizardData{ - BaseDir: baseDir, - ConfigPath: configPath, - CronTime: "02:00", - EnableEncryption: false, // Default to disabled + BaseDir: baseDir, + ConfigPath: configPath, + CronTime: "02:00", + EnableEncryption: false, // Default to disabled BackupFirewallRules: &defaultFirewallRules, } @@ -190,34 +191,34 @@ func RunInstallWizard(ctx context.Context, configPath string, baseDir string, bu SetLabel(" └─ Rclone Log Remote"). SetText("myremote:pbs-logs"). SetFieldWidth(40) - rcloneLogField.SetDisabled(true) - form.Form.AddFormItem(rcloneLogField) - - // Firewall rules backup (system collection) - firewallEnabled := false - firewallDropdown := tview.NewDropDown(). - SetLabel("Backup Firewall Rules (iptables/nftables)"). - SetOptions([]string{"No", "Yes"}, func(option string, index int) { - firewallEnabled = (option == "Yes") - dropdownOpen = false - }). - SetCurrentOption(0) - - firewallDropdown.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey { - if event.Key() == tcell.KeyEnter { - dropdownOpen = !dropdownOpen - } else if event.Key() == tcell.KeyEscape { - dropdownOpen = false - } - return event - }) + rcloneLogField.SetDisabled(true) + form.Form.AddFormItem(rcloneLogField) + + // Firewall rules backup (system collection) + firewallEnabled := false + firewallDropdown := tview.NewDropDown(). + SetLabel("Backup Firewall Rules (iptables/nftables)"). + SetOptions([]string{"No", "Yes"}, func(option string, index int) { + firewallEnabled = (option == "Yes") + dropdownOpen = false + }). + SetCurrentOption(0) + + firewallDropdown.SetInputCapture(func(event *tcell.EventKey) *tcell.EventKey { + if event.Key() == tcell.KeyEnter { + dropdownOpen = !dropdownOpen + } else if event.Key() == tcell.KeyEscape { + dropdownOpen = false + } + return event + }) - form.Form.AddFormItem(firewallDropdown) + form.Form.AddFormItem(firewallDropdown) - // Notifications (header + two toggles) - var telegramEnabled, emailEnabled bool - notificationHeader := tview.NewInputField(). - SetLabel("Notifications"). + // Notifications (header + two toggles) + var telegramEnabled, emailEnabled bool + notificationHeader := tview.NewInputField(). + SetLabel("Notifications"). SetFieldWidth(0). SetText(""). SetDisabled(true) @@ -293,10 +294,10 @@ func RunInstallWizard(ctx context.Context, configPath string, baseDir string, bu form.Form.AddFormItem(cronField) // Set up form submission - form.SetOnSubmit(func(values map[string]string) error { - // Collect data - data.EnableSecondaryStorage = secondaryEnabled - if secondaryEnabled { + form.SetOnSubmit(func(values map[string]string) error { + // Collect data + data.EnableSecondaryStorage = secondaryEnabled + if secondaryEnabled { data.SecondaryPath = secondaryPathField.GetText() data.SecondaryLogPath = secondaryLogField.GetText() @@ -309,8 +310,8 @@ func RunInstallWizard(ctx context.Context, configPath string, baseDir string, bu } } - data.EnableCloudStorage = cloudEnabled - if cloudEnabled { + data.EnableCloudStorage = cloudEnabled + if cloudEnabled { data.RcloneBackupRemote = rcloneBackupField.GetText() data.RcloneLogRemote = rcloneLogField.GetText() @@ -321,14 +322,14 @@ func RunInstallWizard(ctx context.Context, configPath string, baseDir string, bu if !strings.Contains(data.RcloneLogRemote, ":") { return fmt.Errorf("rclone log remote must be in format 'remote:path'") } - } + } - data.BackupFirewallRules = &firewallEnabled + data.BackupFirewallRules = &firewallEnabled - // Get notification mode from two toggles - switch { - case telegramEnabled && emailEnabled: - data.NotificationMode = "both" + // Get notification mode from two toggles + switch { + case telegramEnabled && emailEnabled: + data.NotificationMode = "both" case telegramEnabled: data.NotificationMode = "telegram" case emailEnabled: @@ -530,24 +531,7 @@ func ApplyInstallData(baseTemplate string, data *InstallWizardData) (string, err // setEnvValue sets or updates an environment variable in the template func setEnvValue(template, key, value string) string { - lines := strings.Split(template, "\n") - found := false - - for i, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, key+"=") { - lines[i] = key + "=" + value - found = true - break - } - } - - if !found { - // Add at the end - lines = append(lines, key+"="+value) - } - - return strings.Join(lines, "\n") + return utils.SetEnvValue(template, key, value) } // CheckExistingConfig checks if config file exists and asks how to proceed diff --git a/internal/tui/wizard/install_test.go b/internal/tui/wizard/install_test.go index 8a21e8f..0b94b8d 100644 --- a/internal/tui/wizard/install_test.go +++ b/internal/tui/wizard/install_test.go @@ -23,6 +23,28 @@ func TestSetEnvValueUpdateAndAppend(t *testing.T) { } } +func TestSetEnvValuePreservesComments(t *testing.T) { + template := "FOO=old # comment" + updated := setEnvValue(template, "FOO", "new") + if !strings.Contains(updated, "FOO=new") { + t.Fatalf("expected FOO updated, got %q", updated) + } + if !strings.Contains(updated, "# comment") { + t.Fatalf("expected comment preserved, got %q", updated) + } +} + +func TestSetEnvValuePreservesCommentAfterQuotedValue(t *testing.T) { + template := `FOO="old # keep" # trailing comment` + updated := setEnvValue(template, "FOO", "new") + if !strings.Contains(updated, "FOO=new") { + t.Fatalf("expected FOO updated, got %q", updated) + } + if !strings.Contains(updated, "# trailing comment") { + t.Fatalf("expected trailing comment preserved, got %q", updated) + } +} + func TestApplyInstallDataRespectsBaseTemplate(t *testing.T) { baseTemplate := "BASE_DIR=\nMARKER=1\nTELEGRAM_ENABLED=false\nEMAIL_ENABLED=false\nENCRYPT_ARCHIVE=false\n" backupFirewallRules := false diff --git a/pkg/utils/strings.go b/pkg/utils/strings.go index 93753c7..9da308d 100644 --- a/pkg/utils/strings.go +++ b/pkg/utils/strings.go @@ -38,6 +38,62 @@ func TrimQuotes(s string) string { return s } +// FindInlineCommentIndex returns the index of a # that starts an inline comment. +// A # inside quotes or escaped with a backslash is ignored. +func FindInlineCommentIndex(line string) int { + inQuote := false + var quoteChar byte + escaped := false + + for i := 0; i < len(line); i++ { + ch := line[i] + if escaped { + escaped = false + continue + } + if ch == '\\' { + escaped = true + continue + } + if inQuote { + if ch == quoteChar { + inQuote = false + } + continue + } + if ch == '"' || ch == '\'' { + inQuote = true + quoteChar = ch + continue + } + if ch == '#' { + return i + } + } + return -1 +} + +// FindClosingQuoteIndex returns the index of the closing quote in s, +// honoring backslash escapes. Assumes s[0] is the opening quote. +func FindClosingQuoteIndex(s string, quote byte) int { + escaped := false + for i := 1; i < len(s); i++ { + ch := s[i] + if escaped { + escaped = false + continue + } + if ch == '\\' { + escaped = true + continue + } + if ch == quote { + return i + } + } + return -1 +} + // SplitKeyValue splits a "key=value" string into key and value. // Supports inline comments too: KEY="value" # comment func SplitKeyValue(line string) (string, string, bool) { @@ -48,28 +104,70 @@ func SplitKeyValue(line string) (string, string, bool) { key := strings.TrimSpace(parts[0]) valuePart := strings.TrimSpace(parts[1]) - // Remove inline comments (but respect quotes) - // If the value is quoted, take everything inside quotes - // Otherwise, stop at the first # - value := valuePart if strings.HasPrefix(valuePart, "\"") || strings.HasPrefix(valuePart, "'") { - // Find the closing quote quote := valuePart[0] - endIdx := strings.IndexByte(valuePart[1:], quote) - if endIdx >= 0 { - value = valuePart[:endIdx+2] // Include both quotes - } - } else { - // Not quoted, remove everything after # - if idx := strings.Index(valuePart, "#"); idx >= 0 { - value = strings.TrimSpace(valuePart[:idx]) + if endIdx := FindClosingQuoteIndex(valuePart, quote); endIdx >= 0 { + valuePart = valuePart[:endIdx+1] } + } else if idx := FindInlineCommentIndex(valuePart); idx >= 0 { + valuePart = strings.TrimSpace(valuePart[:idx]) } - value = TrimQuotes(strings.TrimSpace(value)) + value := TrimQuotes(strings.TrimSpace(valuePart)) return key, value, true } +// SetEnvValue sets or updates a KEY=VALUE line in a template, preserving indentation and comments. +func SetEnvValue(template, key, value string) string { + lines := strings.Split(template, "\n") + replaced := false + for i, line := range lines { + trimmed := strings.TrimSpace(line) + if IsComment(trimmed) { + continue + } + + parts := strings.SplitN(trimmed, "=", 2) + if len(parts) >= 1 && strings.TrimSpace(parts[0]) == key { + // Found match. + leadingLen := len(line) - len(strings.TrimLeft(line, " \t")) + leading := "" + if leadingLen > 0 { + leading = line[:leadingLen] + } + + comment := "" + commentSpacing := "" + + commentIdx := FindInlineCommentIndex(line) + if commentIdx >= 0 { + comment = line[commentIdx:] + + beforeComment := line[:commentIdx] + trimmedBefore := strings.TrimRight(beforeComment, " \t") + if len(trimmedBefore) < len(beforeComment) { + commentSpacing = beforeComment[len(trimmedBefore):] + } + } + + newLine := leading + key + "=" + value + if comment != "" { + spacing := commentSpacing + if spacing == "" { + spacing = " " + } + newLine += spacing + comment + } + lines[i] = newLine + replaced = true + } + } + if !replaced { + lines = append(lines, key+"="+value) + } + return strings.Join(lines, "\n") +} + // IsComment checks whether a line is a comment (starts with #). func IsComment(line string) bool { trimmed := strings.TrimSpace(line) diff --git a/pkg/utils/strings_test.go b/pkg/utils/strings_test.go index c3208ad..f960cba 100644 --- a/pkg/utils/strings_test.go +++ b/pkg/utils/strings_test.go @@ -92,6 +92,8 @@ func TestSplitKeyValue(t *testing.T) { {"empty value", "KEY=", "KEY", "", true}, {"inline comment", "KEY=value # comment", "KEY", "value", true}, {"quoted hash", `KEY="value # keep"`, "KEY", "value # keep", true}, + {"escaped hash", `KEY=value\#keep`, "KEY", `value\#keep`, true}, + {"escaped quote", `KEY="value\"quoted" # comment`, "KEY", `value\"quoted`, true}, } for _, tt := range tests { @@ -112,6 +114,84 @@ func TestSplitKeyValue(t *testing.T) { } } +func TestFindInlineCommentIndex(t *testing.T) { + tests := []struct { + name string + line string + expected int + }{ + {"no comment", "KEY=value", -1}, + {"simple comment", "KEY=value # comment", 10}, + {"quoted hash", `KEY="value # keep"`, -1}, + {"escaped hash", `KEY=value\#keep # comment`, 16}, + {"escaped quote", `KEY="value\"#still" # comment`, 20}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FindInlineCommentIndex(tt.line) + if got != tt.expected { + t.Errorf("FindInlineCommentIndex(%q) = %d; want %d", tt.line, got, tt.expected) + } + }) + } +} + +func TestSetEnvValue(t *testing.T) { + tests := []struct { + name string + input string + key string + value string + expected string + }{ + { + name: "update existing", + input: "FOO=old\nBAR=keep", + key: "FOO", + value: "new", + expected: "FOO=new\nBAR=keep", + }, + { + name: "append new", + input: "FOO=old", + key: "BAR", + value: "val", + expected: "FOO=old\nBAR=val", + }, + { + name: "preserve indentation and comment spacing", + input: " FOO=old # comment", + key: "FOO", + value: "new", + expected: " FOO=new # comment", + }, + { + name: "preserve comment after quoted hash", + input: `FOO="old # keep" # trailing`, + key: "FOO", + value: "new", + expected: `FOO=new # trailing`, + }, + { + name: "preserve comment after escaped hash", + input: `FOO=old\#keep # trailing`, + key: "FOO", + value: "new", + expected: `FOO=new # trailing`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := SetEnvValue(tt.input, tt.key, tt.value) + if got != tt.expected { + t.Fatalf("SetEnvValue() = %q; want %q", got, tt.expected) + } + }) + } +} + func TestIsComment(t *testing.T) { tests := []struct { name string From 972c656ee8273b467688718d0accf5bab797de49 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:25:39 +0100 Subject: [PATCH 04/10] Handle 'export' prefix with arbitrary whitespace Replace manual HasPrefix checks with a Fields-based parse to handle legacy "export KEY=VALUE" lines that use arbitrary whitespace (spaces or tabs). This prevents accidentally stripping keys like "exporter" while still recognizing true "export" prefixes. Added tests to ensure export-with-whitespace is accepted and that keys named "exporter" are not treated as exports. --- internal/config/upgrade.go | 10 +++----- internal/config/upgrade_test.go | 43 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/internal/config/upgrade.go b/internal/config/upgrade.go index 2faef7a..aa87c83 100644 --- a/internal/config/upgrade.go +++ b/internal/config/upgrade.go @@ -394,13 +394,9 @@ func splitKeyValueRaw(line string) (string, string, string, bool) { } key := strings.TrimSpace(parts[0]) - // Handle legacy "export KEY=VALUE" lines - if strings.HasPrefix(key, "export ") { - key = strings.TrimSpace(strings.TrimPrefix(key, "export ")) - } - // Also handle tab separation just in case "export\tKEY" - if strings.HasPrefix(key, "export\t") { - key = strings.TrimSpace(strings.TrimPrefix(key, "export\t")) + // Handle legacy "export KEY=VALUE" lines with arbitrary whitespace + if fields := strings.Fields(key); len(fields) >= 2 && fields[0] == "export" { + key = fields[1] } valuePart := strings.TrimSpace(parts[1]) diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index cb1a25f..d6a77b0 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -233,6 +233,49 @@ CUSTOM_BACKUP_PATHS=" }) } +func TestPlanUpgradeConfigHandlesExportWhitespace(t *testing.T) { + template := "KEY1=default\n" + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + content := "export KEY1=custom\n" + if err := os.WriteFile(configPath, []byte(content), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + + result, err := PlanUpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("PlanUpgradeConfigFile returned error: %v", err) + } + if len(result.MissingKeys) != 0 { + t.Fatalf("MissingKeys = %v; want []", result.MissingKeys) + } + }) +} + +func TestPlanUpgradeConfigDoesNotStripExporterKey(t *testing.T) { + template := "exporter=default\n" + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + content := "exporter=custom\n" + if err := os.WriteFile(configPath, []byte(content), 0600); err != nil { + t.Fatalf("failed to write config: %v", err) + } + + result, err := PlanUpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("PlanUpgradeConfigFile returned error: %v", err) + } + if len(result.MissingKeys) != 0 { + t.Fatalf("MissingKeys = %v; want []", result.MissingKeys) + } + if len(result.ExtraKeys) != 0 { + t.Fatalf("ExtraKeys = %v; want []", result.ExtraKeys) + } + }) +} + func TestPlanUpgradeConfigWarnsOnCaseCollision(t *testing.T) { template := "BACKUP_PATH=/default/backup\n" withTemplate(t, template, func() { From a7bae0c762797c5292e0d86dae8e221fcd398842 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:29:55 +0100 Subject: [PATCH 05/10] Add test to preserve inline comments in upgrade Add TestUpgradeConfigPreservesInlineComments which verifies UpgradeConfigFile preserves inline comments when adding missing keys. The test seeds a temporary config with inline and quoted '#' comments, runs UpgradeConfigFile, asserts result.Changed is true, and checks the upgraded file still contains the original inline comments for KEY1 and KEY2. --- internal/config/upgrade_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index d6a77b0..207f6a8 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -276,6 +276,38 @@ func TestPlanUpgradeConfigDoesNotStripExporterKey(t *testing.T) { }) } +func TestUpgradeConfigPreservesInlineComments(t *testing.T) { + template := "KEY1=default1\nKEY2=default2\nKEY3=default3\n" + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + userConfig := "KEY1=value # keep\nKEY2=\"quoted # keep\" # trailing\n" + if err := os.WriteFile(configPath, []byte(userConfig), 0600); err != nil { + t.Fatalf("failed to seed config: %v", err) + } + + result, err := UpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("UpgradeConfigFile returned error: %v", err) + } + if !result.Changed { + t.Fatal("expected result.Changed=true when keys missing") + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("failed to read upgraded config: %v", err) + } + content := string(data) + if !strings.Contains(content, "KEY1=value # keep") { + t.Fatalf("expected inline comment preserved for KEY1, got:\n%s", content) + } + if !strings.Contains(content, "KEY2=\"quoted # keep\" # trailing") { + t.Fatalf("expected inline comment preserved for KEY2, got:\n%s", content) + } + }) +} + func TestPlanUpgradeConfigWarnsOnCaseCollision(t *testing.T) { template := "BACKUP_PATH=/default/backup\n" withTemplate(t, template, func() { From 3e4b12b228f6255389e18b97b4f5070c1acb46d3 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:34:17 +0100 Subject: [PATCH 06/10] Add tabbed whitespace case to config test Extend TestPlanUpgradeConfigHandlesExportWhitespace in internal/config/upgrade_test.go to include an export line with tab characters (export\t\tKEY1=custom-tabbed). This ensures the upgrade parsing handles export lines with tabs as well as spaces. --- internal/config/upgrade_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index 207f6a8..5166cf8 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -238,7 +238,7 @@ func TestPlanUpgradeConfigHandlesExportWhitespace(t *testing.T) { withTemplate(t, template, func() { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "backup.env") - content := "export KEY1=custom\n" + content := "export KEY1=custom\nexport\t\tKEY1=custom-tabbed\n" if err := os.WriteFile(configPath, []byte(content), 0600); err != nil { t.Fatalf("failed to write config: %v", err) } From 5116cc11d1799ee5671ae4389d93c1f0d4ccd0c0 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:40:22 +0100 Subject: [PATCH 07/10] Simplify caseConflicts nil check in upgrade Remove the redundant `caseConflicts == nil` check when warning about keys that differ only by case. Reading a nil map in Go is safe, so using `!caseConflicts[upperKey]` preserves behavior while simplifying the condition and reducing clutter in internal/config/upgrade.go. --- internal/config/upgrade.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/upgrade.go b/internal/config/upgrade.go index aa87c83..c2779c8 100644 --- a/internal/config/upgrade.go +++ b/internal/config/upgrade.go @@ -265,7 +265,7 @@ func computeConfigUpgrade(configPath string) (*UpgradeResult, string, []byte, er upperKey := strings.ToUpper(key) if templateKey, ok := templateKeyByUpper[upperKey]; ok && templateKey != key { - if caseConflicts == nil || !caseConflicts[upperKey] { + if !caseConflicts[upperKey] { warnings = append(warnings, fmt.Sprintf("Key %q differs only by case from template key %q; preserved as custom entry", key, templateKey)) } } From e6209d03a418494362868eb73bf6ed23c201fbd4 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:44:13 +0100 Subject: [PATCH 08/10] Add test to preserve block comments on upgrade Adds TestUpgradeConfigPreservesBlockComments to internal/config/upgrade_test.go. The new test seeds a user config containing a multi-line variable with inline comment lines, runs UpgradeConfigFile, asserts that an upgrade occurs (result.Changed == true) and verifies the block-style comment lines and content are preserved in the upgraded file. --- internal/config/upgrade_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index 5166cf8..196d89d 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -308,6 +308,35 @@ func TestUpgradeConfigPreservesInlineComments(t *testing.T) { }) } +func TestUpgradeConfigPreservesBlockComments(t *testing.T) { + template := "CUSTOM_BACKUP_PATHS=\"\n# template comment\n\"\nKEY1=default\n" + withTemplate(t, template, func() { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "backup.env") + userConfig := "CUSTOM_BACKUP_PATHS=\"\n# keep this\n/path/one\n# keep too\n\"\n" + if err := os.WriteFile(configPath, []byte(userConfig), 0600); err != nil { + t.Fatalf("failed to seed config: %v", err) + } + + result, err := UpgradeConfigFile(configPath) + if err != nil { + t.Fatalf("UpgradeConfigFile returned error: %v", err) + } + if !result.Changed { + t.Fatal("expected result.Changed=true when keys missing") + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("failed to read upgraded config: %v", err) + } + content := strings.ReplaceAll(string(data), "\r\n", "\n") + if !strings.Contains(content, "# keep this\n/path/one\n# keep too\n") { + t.Fatalf("expected block comments preserved, got:\n%s", content) + } + }) +} + func TestPlanUpgradeConfigWarnsOnCaseCollision(t *testing.T) { template := "BACKUP_PATH=/default/backup\n" withTemplate(t, template, func() { From 97f6615ce49fe255cb59cefb843a5d02f68383e4 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Tue, 3 Feb 2026 23:54:36 +0100 Subject: [PATCH 09/10] Normalize env keys to uppercase and handle export parseEnvFile: normalize parsed environment variable names to uppercase and support lines beginning with "export". Lookups against blockValueKeys and multiValueKeys now use the uppercased key, and stored map keys are uppercased so legacy/lowercase env files are handled consistently. Added tests verifying lowercase keys are normalized (including multi-line block values and multi-value accumulation) and that lines prefixed with various spacing for "export" are parsed correctly. --- internal/config/config.go | 18 +++++++---- internal/config/migration_test.go | 54 +++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index 171784c..5f02609 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1218,8 +1218,12 @@ func parseEnvFile(path string) (map[string]string, error) { if !ok { continue } + if fields := strings.Fields(key); len(fields) >= 2 && fields[0] == "export" { + key = fields[1] + } + upperKey := strings.ToUpper(key) - if blockValueKeys[key] && trimmed == fmt.Sprintf("%s=\"", key) { + if blockValueKeys[upperKey] && trimmed == fmt.Sprintf("%s=\"", key) { var blockLines []string terminated := false for scanner.Scan() { @@ -1235,18 +1239,18 @@ func parseEnvFile(path string) (map[string]string, error) { if !terminated { return nil, fmt.Errorf("unterminated multi-line value for %s", key) } - raw[key] = strings.Join(blockLines, "\n") + raw[upperKey] = strings.Join(blockLines, "\n") continue } - if multiValueKeys[key] { - if existing, ok := raw[key]; ok && existing != "" { - raw[key] = existing + "\n" + value + if multiValueKeys[upperKey] { + if existing, ok := raw[upperKey]; ok && existing != "" { + raw[upperKey] = existing + "\n" + value } else { - raw[key] = value + raw[upperKey] = value } } else { - raw[key] = value + raw[upperKey] = value } } diff --git a/internal/config/migration_test.go b/internal/config/migration_test.go index 26917fa..450d770 100644 --- a/internal/config/migration_test.go +++ b/internal/config/migration_test.go @@ -50,6 +50,60 @@ BACKUP_BLACKLIST=" } } +func TestParseEnvFileHandlesLowercaseKeys(t *testing.T) { + tmpDir := t.TempDir() + envFile := filepath.Join(tmpDir, "legacy.env") + content := ` +custom_backup_paths=" +/etc/custom +" +backup_blacklist=" +/tmp +" +backup_exclude_patterns=*.log +backup_exclude_patterns=*.tmp +` + if err := os.WriteFile(envFile, []byte(content), 0600); err != nil { + t.Fatalf("failed to write temp legacy env: %v", err) + } + + values, err := parseEnvFile(envFile) + if err != nil { + t.Fatalf("parseEnvFile returned error: %v", err) + } + + if got := values["CUSTOM_BACKUP_PATHS"]; got != "/etc/custom" { + t.Fatalf("CUSTOM_BACKUP_PATHS = %q; want block content", got) + } + if got := values["BACKUP_BLACKLIST"]; got != "/tmp" { + t.Fatalf("BACKUP_BLACKLIST = %q; want block content", got) + } + if got := values["BACKUP_EXCLUDE_PATTERNS"]; got != "*.log\n*.tmp" { + t.Fatalf("BACKUP_EXCLUDE_PATTERNS = %q; want \"*.log\\n*.tmp\"", got) + } +} + +func TestParseEnvFileHandlesExportLines(t *testing.T) { + tmpDir := t.TempDir() + envFile := filepath.Join(tmpDir, "legacy.env") + content := "export BACKUP_PATH=/data\nexport\t\tLOG_PATH=/logs\n" + if err := os.WriteFile(envFile, []byte(content), 0600); err != nil { + t.Fatalf("failed to write temp legacy env: %v", err) + } + + values, err := parseEnvFile(envFile) + if err != nil { + t.Fatalf("parseEnvFile returned error: %v", err) + } + + if got := values["BACKUP_PATH"]; got != "/data" { + t.Fatalf("BACKUP_PATH = %q; want %q", got, "/data") + } + if got := values["LOG_PATH"]; got != "/logs" { + t.Fatalf("LOG_PATH = %q; want %q", got, "/logs") + } +} + const testTemplate = `# test template BACKUP_PATH=/default/backup LOG_PATH=/default/log From 8ffbb2188daa037a419061c134aed6629f73ce90 Mon Sep 17 00:00:00 2001 From: tis24dev Date: Wed, 4 Feb 2026 00:14:42 +0100 Subject: [PATCH 10/10] Preserve 'export' prefix in SetEnvValue SetEnvValue now detects an optional leading "export" token when parsing env lines and preserves it when rewriting the line. The function trims and normalizes the export prefix (e.g. "export FOO=...") and only updates the value when the parsed key matches. A unit test was added to verify the export prefix is kept and normalized in the resulting line. --- internal/config/upgrade_test.go | 1 - pkg/utils/strings.go | 14 ++++++++++++-- pkg/utils/strings_test.go | 7 +++++++ 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/internal/config/upgrade_test.go b/internal/config/upgrade_test.go index 196d89d..eb9bfa9 100644 --- a/internal/config/upgrade_test.go +++ b/internal/config/upgrade_test.go @@ -413,7 +413,6 @@ Custom_Backup_Paths=" if err != nil { t.Fatalf("failed to read upgraded config: %v", err) } - // strings.ReplaceAll might not be available if "strings" isn't imported, but it is imported in upgrade_test.go content := strings.ReplaceAll(string(data), "\r\n", "\n") expectedBlock := "CUSTOM_BACKUP_PATHS=\"\n/etc/custom.conf\n\"\n" diff --git a/pkg/utils/strings.go b/pkg/utils/strings.go index 9da308d..378dfb6 100644 --- a/pkg/utils/strings.go +++ b/pkg/utils/strings.go @@ -128,7 +128,17 @@ func SetEnvValue(template, key, value string) string { } parts := strings.SplitN(trimmed, "=", 2) - if len(parts) >= 1 && strings.TrimSpace(parts[0]) == key { + if len(parts) >= 1 { + parsedKey := strings.TrimSpace(parts[0]) + exportPrefix := "" + if fields := strings.Fields(parsedKey); len(fields) >= 2 && fields[0] == "export" { + parsedKey = fields[1] + exportPrefix = "export " + } + + if parsedKey != key { + continue + } // Found match. leadingLen := len(line) - len(strings.TrimLeft(line, " \t")) leading := "" @@ -150,7 +160,7 @@ func SetEnvValue(template, key, value string) string { } } - newLine := leading + key + "=" + value + newLine := leading + exportPrefix + key + "=" + value if comment != "" { spacing := commentSpacing if spacing == "" { diff --git a/pkg/utils/strings_test.go b/pkg/utils/strings_test.go index f960cba..691a316 100644 --- a/pkg/utils/strings_test.go +++ b/pkg/utils/strings_test.go @@ -180,6 +180,13 @@ func TestSetEnvValue(t *testing.T) { value: "new", expected: `FOO=new # trailing`, }, + { + name: "preserve export prefix", + input: "export FOO=old # comment", + key: "FOO", + value: "new", + expected: "export FOO=new # comment", + }, } for _, tt := range tests {