From a7b67af50e34720f936db2aa80758ee0918373a4 Mon Sep 17 00:00:00 2001 From: clawbot Date: Mon, 21 Sep 2026 07:32:05 +0000 Subject: [PATCH] Use one duration parser and fix the --older-than months example (closes #123) Two functions named parseDuration existed with different grammars. --older-than and --keep-newer-than both already parsed through the one in internal/vaultik; the richer copy in internal/cli/duration.go was reachable only from its own test. Kept the live-path parser and deleted the unused one, so no flag's accepted grammar changes. README documented 6m as the months example for --older-than, but m is minutes, so that command deleted every snapshot older than six minutes; corrected it to 6mo and put both flags' help on one example list stating m is minutes and mo is months. The parser now rejects negatives it used to accept (-5h) or silently made positive (-5d). Model: opus-4-8 --- README.md | 3 +- TODO.md | 18 ++ internal/cli/duration.go | 126 ------------- internal/cli/duration_test.go | 299 ------------------------------- internal/cli/snapshot.go | 6 +- internal/vaultik/helpers.go | 12 +- internal/vaultik/helpers_test.go | 31 +++- 7 files changed, 58 insertions(+), 437 deletions(-) delete mode 100644 internal/cli/duration.go delete mode 100644 internal/cli/duration_test.go diff --git a/README.md b/README.md index 2fcf894..b411fd4 100644 --- a/README.md +++ b/README.md @@ -251,7 +251,8 @@ local index alone, and still exits zero. per-snapshot-name (`--keep-latest` keeps the latest of each name, not the latest globally). * `--keep-latest`: Keep only the most recent snapshot of each name -* `--older-than `: Remove snapshots older than duration (e.g. `30d`, `6m`, `1y`) +* `--older-than `: Remove snapshots older than duration (e.g. `30d`, + `4w`, `6mo`, `1y`; `m` is minutes, `mo` is months) * `--snapshot `: Restrict to specific snapshot names (repeat for multiple) * `--force`: Skip confirmation prompt diff --git a/TODO.md b/TODO.md index 5896f00..6b32d12 100644 --- a/TODO.md +++ b/TODO.md @@ -50,6 +50,24 @@ release" is exactly the contradiction keeps that exact compiler from auto-switching. Bumping Go now touches `go.mod`, the checksum, and the `Dockerfile` `golang` digest together. +- 2026-09-21: Collapsed the two duration parsers into one and fixed the + `--older-than` months example + ([issue #123](https://git.eeqj.de/sneak/vaultik/issues/123)). Two + functions named `parseDuration` existed with different grammars; + `snapshot purge --older-than` and `--keep-newer-than` both already went + through the one in `internal/vaultik`, while the richer copy in + `internal/cli/duration.go` was reachable only from its own test. Kept + the live-path parser and deleted the unused one, so no flag's accepted + grammar changes. The trap the issue was filed over: `README.md` + documented `6m` as the months example for `--older-than`, but `m` is + minutes, so the documented command deleted every snapshot older than + six minutes on a destructive flag. Corrected the doc to `6mo` and put + both flags' help text on one example list that states `m` is minutes + and `mo` is months. The surviving parser now rejects negatives, which + it previously accepted (`-5h`) or silently made positive (`-5d`). + Table-driven tests cover every unit, `6m` as six minutes, `6mo` as 180 + days, and rejection of a bare number, an unknown unit, and a negative. + - 2026-08-10: Moved every lint run into its own container, as a build step ([issue #113](https://git.eeqj.de/sneak/vaultik/issues/113)). New root `Dockerfile.lint`, built by `script/lint`, runs diff --git a/internal/cli/duration.go b/internal/cli/duration.go deleted file mode 100644 index ee54257..0000000 --- a/internal/cli/duration.go +++ /dev/null @@ -1,126 +0,0 @@ -package cli - -import ( - "errors" - "fmt" - "regexp" - "strconv" - "strings" - "time" -) - -// Approximate lengths of the extended calendar units accepted by -// parseDuration. -const ( - durationDay = 24 * time.Hour - durationWeek = 7 * durationDay - durationMonth = 30 * durationDay - durationYear = 365 * durationDay -) - -var ( - errNegativeDuration = errors.New("negative durations are not supported") - errInvalidDuration = errors.New("invalid duration format") - errUnknownTimeUnit = errors.New("unknown time unit") -) - -// parseDuration parses duration strings. Supports standard Go duration format -// (e.g., "3h30m", "1h45m30s") as well as extended units: -// - d: days (e.g., "30d", "7d") -// - w: weeks (e.g., "2w", "4w") -// - mo: months (30 days) (e.g., "6mo", "1mo") -// - y: years (365 days) (e.g., "1y", "2y") -// -// Can combine units: "1y6mo", "2w3d", "1d12h30m" -func parseDuration(s string) (time.Duration, error) { - // First try standard Go duration parsing - d, err := time.ParseDuration(s) - if err == nil { - return d, nil - } - - // Extended duration parsing - // Check for negative values - if strings.HasPrefix(strings.TrimSpace(s), "-") { - return 0, errNegativeDuration - } - - // Pattern matches: number + unit, repeated - re := regexp.MustCompile(`(\d+(?:\.\d+)?)\s*([a-zA-Z]+)`) - matches := re.FindAllStringSubmatch(s, -1) - - if len(matches) == 0 { - return 0, fmt.Errorf("%w: %q", errInvalidDuration, s) - } - - var total time.Duration - - for _, match := range matches { - valueStr := match[1] - unit := strings.ToLower(match[2]) - - value, err := strconv.ParseFloat(valueStr, 64) - if err != nil { - return 0, fmt.Errorf("invalid number %q: %w", valueStr, err) - } - - d, err := durationForUnit(value, unit) - if err != nil { - return 0, err - } - - total += d - } - - return total, nil -} - -// durationForUnit converts a value with a (case-normalized) unit suffix -// into a time.Duration, accepting Go's standard units plus the extended -// calendar units. -func durationForUnit(value float64, unit string) (time.Duration, error) { - switch unit { - // Standard time units - case "ns", "nanosecond", "nanoseconds": - return time.Duration(value), nil - case "us", "µs", "microsecond", "microseconds": - return time.Duration(value * float64(time.Microsecond)), nil - case "ms", "millisecond", "milliseconds": - return time.Duration(value * float64(time.Millisecond)), nil - case "s", "sec", "second", "seconds": - return time.Duration(value * float64(time.Second)), nil - case "m", "min", "minute", "minutes": - return time.Duration(value * float64(time.Minute)), nil - case "h", "hr", "hour", "hours": - return time.Duration(value * float64(time.Hour)), nil - // Extended units - case "d", "day", "days": - return time.Duration(value * float64(durationDay)), nil - case "w", "week", "weeks": - return time.Duration(value * float64(durationWeek)), nil - case "mo", "month", "months": - // Using 30 days as approximation - return time.Duration(value * float64(durationMonth)), nil - case "y", "year", "years": - // Using 365 days as approximation - return time.Duration(value * float64(durationYear)), nil - default: - // Try parsing as standard Go duration unit - testStr := "1" + unit - - _, err := time.ParseDuration(testStr) - if err != nil { - return 0, fmt.Errorf("%w: %q", errUnknownTimeUnit, unit) - } - - // It's a valid Go duration unit, parse the full value - fullStr := fmt.Sprintf("%g%s", value, unit) - - d, err := time.ParseDuration(fullStr) - if err != nil { - return 0, fmt.Errorf("invalid duration %q: %w", fullStr, err) - } - - return d, nil - } -} diff --git a/internal/cli/duration_test.go b/internal/cli/duration_test.go deleted file mode 100644 index cc077d3..0000000 --- a/internal/cli/duration_test.go +++ /dev/null @@ -1,299 +0,0 @@ -package cli //nolint:testpackage // needs access to unexported parseDuration - -import ( - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -type parseDurationCase struct { - name string - input string - expected time.Duration - wantErr bool -} - -// runParseDurationCases executes a table of parseDuration cases as -// parallel subtests. -func runParseDurationCases(t *testing.T, tests []parseDurationCase) { - t.Helper() - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - got, err := parseDuration(tt.input) - - if tt.wantErr { - require.Error(t, err, "expected error for input %q", tt.input) - - return - } - - require.NoError(t, err, "unexpected error for input %q", tt.input) - assert.Equal(t, tt.expected, got, "duration mismatch for input %q", tt.input) - }) - } -} - -func TestParseDurationStandard(t *testing.T) { - t.Parallel() - - runParseDurationCases(t, []parseDurationCase{ - { - name: "standard seconds", - input: "30s", - expected: 30 * time.Second, - }, - { - name: "standard minutes", - input: "45m", - expected: 45 * time.Minute, - }, - { - name: "standard hours", - input: "2h", - expected: 2 * time.Hour, - }, - { - name: "standard combined", - input: "3h30m", - expected: 3*time.Hour + 30*time.Minute, - }, - { - name: "standard complex", - input: "1h45m30s", - expected: 1*time.Hour + 45*time.Minute + 30*time.Second, - }, - { - name: "standard with milliseconds", - input: "1s500ms", - expected: 1*time.Second + 500*time.Millisecond, - }, - }) -} - -func TestParseDurationExtendedUnits(t *testing.T) { - t.Parallel() - - runParseDurationCases(t, []parseDurationCase{ - // Extended units - days - { - name: "single day", - input: "1d", - expected: 24 * time.Hour, - }, - { - name: "multiple days", - input: "7d", - expected: 7 * 24 * time.Hour, - }, - { - name: "fractional days", - input: "1.5d", - expected: 36 * time.Hour, - }, - { - name: "days spelled out", - input: "3days", - expected: 3 * 24 * time.Hour, - }, - // Extended units - weeks - { - name: "single week", - input: "1w", - expected: 7 * 24 * time.Hour, - }, - { - name: "multiple weeks", - input: "4w", - expected: 4 * 7 * 24 * time.Hour, - }, - { - name: "weeks spelled out", - input: "2weeks", - expected: 2 * 7 * 24 * time.Hour, - }, - // Extended units - months - { - name: "single month", - input: "1mo", - expected: 30 * 24 * time.Hour, - }, - { - name: "multiple months", - input: "6mo", - expected: 6 * 30 * 24 * time.Hour, - }, - { - name: "months spelled out", - input: "3months", - expected: 3 * 30 * 24 * time.Hour, - }, - // Extended units - years - { - name: "single year", - input: "1y", - expected: 365 * 24 * time.Hour, - }, - { - name: "multiple years", - input: "2y", - expected: 2 * 365 * 24 * time.Hour, - }, - { - name: "years spelled out", - input: "1year", - expected: 365 * 24 * time.Hour, - }, - }) -} - -func TestParseDurationCombinedAndErrors(t *testing.T) { - t.Parallel() - - runParseDurationCases(t, []parseDurationCase{ - // Combined extended units - { - name: "weeks and days", - input: "2w3d", - expected: 2*7*24*time.Hour + 3*24*time.Hour, - }, - { - name: "years and months", - input: "1y6mo", - expected: 365*24*time.Hour + 6*30*24*time.Hour, - }, - { - name: "days and hours", - input: "1d12h", - expected: 24*time.Hour + 12*time.Hour, - }, - { - name: "complex combination", - input: "1y2mo3w4d5h6m7s", - expected: 365*24*time.Hour + 2*30*24*time.Hour + - 3*7*24*time.Hour + 4*24*time.Hour + - 5*time.Hour + 6*time.Minute + 7*time.Second, - }, - { - name: "with spaces", - input: "1d 12h 30m", - expected: 24*time.Hour + 12*time.Hour + 30*time.Minute, - }, - // Edge cases - { - name: "zero duration", - input: "0s", - expected: 0, - }, - { - name: "large duration", - input: "10y", - expected: 10 * 365 * 24 * time.Hour, - }, - // Error cases - { - name: "empty string", - input: "", - wantErr: true, - }, - { - name: "invalid format", - input: "abc", - wantErr: true, - }, - { - name: "unknown unit", - input: "5x", - wantErr: true, - }, - { - name: "invalid number", - input: "xyzd", - wantErr: true, - }, - { - name: "negative not supported", - input: "-5d", - wantErr: true, - }, - }) -} - -func TestParseDurationSpecialCases(t *testing.T) { - t.Parallel() - - // Test that standard Go durations work exactly as expected - standardDurations := []string{ - "300ms", - "1.5h", - "2h45m", - "72h", - "1us", - "1µs", - "1ns", - } - - for _, d := range standardDurations { - expected, err := time.ParseDuration(d) - require.NoError(t, err) - - got, err := parseDuration(d) - require.NoError(t, err) - assert.Equal(t, expected, got, "standard duration %q should parse identically", d) - } -} - -func TestParseDurationRealWorldExamples(t *testing.T) { - t.Parallel() - - // Test real-world snapshot purge scenarios - tests := []struct { - description string - input string - olderThan time.Duration - }{ - { - description: "keep snapshots from last 30 days", - input: "30d", - olderThan: 30 * 24 * time.Hour, - }, - { - description: "keep snapshots from last 6 months", - input: "6mo", - olderThan: 6 * 30 * 24 * time.Hour, - }, - { - description: "keep snapshots from last year", - input: "1y", - olderThan: 365 * 24 * time.Hour, - }, - { - description: "keep snapshots from last week and a half", - input: "1w3d", - olderThan: 10 * 24 * time.Hour, - }, - { - description: "keep snapshots from last 90 days", - input: "90d", - olderThan: 90 * 24 * time.Hour, - }, - } - - for _, tt := range tests { - t.Run(tt.description, func(t *testing.T) { - t.Parallel() - - got, err := parseDuration(tt.input) - require.NoError(t, err) - assert.Equal(t, tt.olderThan, got) - - // Verify the duration makes sense for snapshot purging - assert.Greater(t, got, time.Hour, - "snapshot purge duration should be at least an hour") - }) - } -} diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 7460169..a702386 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -141,7 +141,8 @@ specifying a path using --config or by setting VAULTIK_CONFIG to a path.`, "orphaned blobs") cmd.Flags().StringVar(&opts.KeepNewerThan, "keep-newer-than", "", "With --prune: keep snapshots newer than this duration "+ - "(e.g. 4w, 30d, 6mo) instead of only the latest") + "(e.g. 30d, 4w, 6mo, 1y; m is minutes, mo is months) "+ + "instead of only the latest") return cmd } @@ -204,7 +205,8 @@ restrict the operation to specific snapshot names.`, cmd.Flags().BoolVar(&opts.KeepLatest, "keep-latest", false, "Keep only the latest snapshot of each name") cmd.Flags().StringVar(&opts.OlderThan, "older-than", "", - "Remove snapshots older than duration (e.g., 30d, 6m, 1y)") + "Remove snapshots older than duration "+ + "(e.g. 30d, 4w, 6mo, 1y; m is minutes, mo is months)") cmd.Flags().BoolVar(&opts.Force, "force", false, "Skip confirmation prompt") cmd.Flags().StringArrayVar(&opts.Names, "snapshot", nil, "Restrict to snapshots with these names (repeat for multiple)") diff --git a/internal/vaultik/helpers.go b/internal/vaultik/helpers.go index 00ebc20..64a1d85 100644 --- a/internal/vaultik/helpers.go +++ b/internal/vaultik/helpers.go @@ -33,8 +33,9 @@ func ubytes(n int64) string { var ( errMalformedSnapshotID = errors.New( "invalid snapshot ID format: expected hostname_snapshotname_timestamp") - errInvalidDuration = errors.New("invalid duration") - errUnknownTimeUnit = errors.New("unknown time unit") + errInvalidDuration = errors.New("invalid duration") + errUnknownTimeUnit = errors.New("unknown time unit") + errNegativeDuration = errors.New("negative durations are not supported") ) // Time-unit lengths used by parseDuration. @@ -138,8 +139,13 @@ func parseSnapshotName(snapshotID string) string { // parseDuration parses a duration string with support for human-friendly units: // d/day/days, w/week/weeks, mo/month/months, y/year/years, plus standard Go -// duration units (h, m, s). +// duration units. Following Go, m is minutes and mo is months. A bare number, +// an unknown unit, and a negative value are all rejected. func parseDuration(s string) (time.Duration, error) { + if strings.HasPrefix(strings.TrimSpace(s), "-") { + return 0, errNegativeDuration + } + d, err := time.ParseDuration(s) if err == nil { return d, nil diff --git a/internal/vaultik/helpers_test.go b/internal/vaultik/helpers_test.go index 53e6746..eb58781 100644 --- a/internal/vaultik/helpers_test.go +++ b/internal/vaultik/helpers_test.go @@ -51,13 +51,32 @@ func TestParseDuration(t *testing.T) { want time.Duration err bool }{ - {"30d", 30 * 24 * time.Hour, false}, - {"4w", 4 * 7 * 24 * time.Hour, false}, - {"6mo", 6 * 30 * 24 * time.Hour, false}, - {"1y", 365 * 24 * time.Hour, false}, - {"2w3d", 2*7*24*time.Hour + 3*24*time.Hour, false}, - {"1h", time.Hour, false}, + // Go units, including the m-is-minutes / mo-is-months distinction + // that this parser exists to keep straight. + {"10ns", 10 * time.Nanosecond, false}, + {"10us", 10 * time.Microsecond, false}, + {"500ms", 500 * time.Millisecond, false}, {"30s", 30 * time.Second, false}, + {"6m", 6 * time.Minute, false}, + {"1h", time.Hour, false}, + // Extended calendar units. + {"30d", 30 * 24 * time.Hour, false}, + {"3days", 3 * 24 * time.Hour, false}, + {"4w", 4 * 7 * 24 * time.Hour, false}, + {"2weeks", 2 * 7 * 24 * time.Hour, false}, + {"6mo", 180 * 24 * time.Hour, false}, + {"1month", 30 * 24 * time.Hour, false}, + {"1y", 365 * 24 * time.Hour, false}, + {"2years", 2 * 365 * 24 * time.Hour, false}, + // Combined units. + {"2w3d", 2*7*24*time.Hour + 3*24*time.Hour, false}, + {"1y6mo", 365*24*time.Hour + 180*24*time.Hour, false}, + // Rejected inputs. + {"6", 0, true}, // bare number, no unit + {"5x", 0, true}, // unknown unit + {"-5d", 0, true}, // negative, extended unit + {"-5h", 0, true}, // negative, Go unit + {"", 0, true}, // empty {"garbage", 0, true}, } -- 2.54.0