diff --git a/README.md b/README.md index 6ccdb36..62ae2f0 100644 --- a/README.md +++ b/README.md @@ -58,6 +58,12 @@ cbzconverter optimize [folder] --format=webp cbzconverter optimize [folder] --format WEBP ``` +Preserve original page filenames in the output CBZ instead of sequential renumbering: + +```sh +cbzconverter optimize [folder] --keep-filenames --quality 85 --format webp +``` + With timeout to avoid hanging on problematic chapters: ```sh @@ -103,6 +109,7 @@ docker run -v /path/to/comics:/comics ghcr.io/belphemur/cbzoptimizer:latest watc - `--format`, `-f`: Format to convert the images to (currently supports: webp). Default is webp. - Can be specified as: `--format webp`, `-f webp`, or `--format=webp` - Case-insensitive: `webp`, `WEBP`, and `WebP` are all valid +- `--keep-filenames`: Preserve each page's original base filename in the output CBZ (with the extension swapped for format conversion) instead of renumbering pages to `0000`, `0001`, ... Default is false. When two source pages share the same base filename, the second one falls back to the indexed form so the output stays a valid zip. - `--timeout`, `-t`: Maximum time allowed for converting a single chapter (e.g., 30s, 5m, 1h). 0 means no timeout. Default is 0. - `--backfill`: *(`watch` only)* Optimize CBZ/CBR files that already exist in the watched folder at startup, before watching for new changes. Default is false. - `--log`, `-l`: Set log level; can be 'panic', 'fatal', 'error', 'warn', 'info', 'debug', or 'trace'. Default is info. diff --git a/cmd/cbzoptimizer/commands/flags.go b/cmd/cbzoptimizer/commands/flags.go index 81b26fb..15c080f 100644 --- a/cmd/cbzoptimizer/commands/flags.go +++ b/cmd/cbzoptimizer/commands/flags.go @@ -70,6 +70,19 @@ func setupSplitFlag(cmd *cobra.Command, defaultValue bool, bindViper bool) { } } +// setupKeepFilenamesFlag sets up the keep-filenames flag for a command. +// +// Parameters: +// - cmd: The Cobra command to add the keep-filenames flag to +// - defaultValue: The default keep-filenames value +// - bindViper: If true, binds the flag to viper for configuration file support +func setupKeepFilenamesFlag(cmd *cobra.Command, defaultValue bool, bindViper bool) { + cmd.Flags().Bool("keep-filenames", defaultValue, "Preserve original page filenames instead of renumbering to sequential indices") + if bindViper { + _ = viper.BindPFlag("keep-filenames", cmd.Flags().Lookup("keep-filenames")) + } +} + // setupTimeoutFlag sets up the timeout flag for a command. // // Parameters: @@ -96,5 +109,6 @@ func setupCommonFlags(cmd *cobra.Command, converterType *constant.ConversionForm setupQualityFlag(cmd, qualityDefault, bindViper) setupOverrideFlag(cmd, overrideDefault, bindViper) setupSplitFlag(cmd, splitDefault, bindViper) + setupKeepFilenamesFlag(cmd, false, bindViper) setupTimeoutFlag(cmd, bindViper) } diff --git a/cmd/cbzoptimizer/commands/optimize_command.go b/cmd/cbzoptimizer/commands/optimize_command.go index 0ab6427..7ca0f4b 100644 --- a/cmd/cbzoptimizer/commands/optimize_command.go +++ b/cmd/cbzoptimizer/commands/optimize_command.go @@ -24,10 +24,10 @@ func init() { RunE: ConvertCbzCommand, Args: cobra.ExactArgs(1), } - + // Setup common flags (format, quality, override, split, timeout) setupCommonFlags(command, &converterType, 85, false, false, false) - + // Setup optimize-specific flags command.Flags().IntP("parallelism", "n", 2, "Number of chapters to convert in parallel") @@ -73,6 +73,13 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error { } log.Debug().Bool("split", split).Msg("Split parameter parsed") + keepFilenames, err := cmd.Flags().GetBool("keep-filenames") + if err != nil { + log.Error().Err(err).Msg("Failed to parse keep-filenames flag") + return fmt.Errorf("invalid keep-filenames value") + } + log.Debug().Bool("keep-filenames", keepFilenames).Msg("Keep-filenames parameter parsed") + timeout, err := cmd.Flags().GetDuration("timeout") if err != nil { log.Error().Err(err).Msg("Failed to parse timeout flag") @@ -127,6 +134,7 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error { Quality: quality, Override: override, Split: split, + KeepFilenames: keepFilenames, Timeout: timeout, }) if err != nil { diff --git a/cmd/cbzoptimizer/commands/optimize_command_test.go b/cmd/cbzoptimizer/commands/optimize_command_test.go index 08fee33..0854754 100644 --- a/cmd/cbzoptimizer/commands/optimize_command_test.go +++ b/cmd/cbzoptimizer/commands/optimize_command_test.go @@ -86,6 +86,7 @@ func TestConvertCbzCommand(t *testing.T) { cmd.Flags().IntP("parallelism", "n", 2, "Number of chapters to convert in parallel") cmd.Flags().BoolP("override", "o", false, "Override the original CBZ/CBR files") cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") + cmd.Flags().Bool("keep-filenames", false, "Preserve original page filenames instead of renumbering to sequential indices") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter (e.g., 30s, 5m, 1h). 0 means no timeout") // Execute the command @@ -197,6 +198,7 @@ func setupTestCommand(t *testing.T) (*cobra.Command, func()) { cmd.Flags().IntP("parallelism", "n", 1, "Number of chapters to convert in parallel") cmd.Flags().BoolP("override", "o", false, "Override the original CBZ/CBR files") cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") + cmd.Flags().Bool("keep-filenames", false, "Preserve original page filenames instead of renumbering to sequential indices") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter") // Reset converterType to default before test for consistency @@ -433,6 +435,7 @@ func TestConvertCbzCommand_ManyFiles_NoDeadlock(t *testing.T) { cmd.Flags().IntP("parallelism", "n", 2, "Number of chapters to convert in parallel") cmd.Flags().BoolP("override", "o", false, "Override the original CBZ/CBR files") cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") + cmd.Flags().Bool("keep-filenames", false, "Preserve original page filenames instead of renumbering to sequential indices") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter") converterType = constant.DefaultConversion @@ -535,6 +538,7 @@ func TestConvertCbzCommand_HighParallelism_NoDeadlock(t *testing.T) { cmd.Flags().IntP("parallelism", "n", 8, "Number of chapters to convert in parallel") cmd.Flags().BoolP("override", "o", false, "Override the original CBZ/CBR files") cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") + cmd.Flags().Bool("keep-filenames", false, "Preserve original page filenames instead of renumbering to sequential indices") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter") converterType = constant.DefaultConversion diff --git a/cmd/cbzoptimizer/commands/watch_command.go b/cmd/cbzoptimizer/commands/watch_command.go index 3e82d8e..d49935e 100644 --- a/cmd/cbzoptimizer/commands/watch_command.go +++ b/cmd/cbzoptimizer/commands/watch_command.go @@ -65,6 +65,8 @@ func WatchCommand(_ *cobra.Command, args []string) error { split := viper.GetBool("split") + keepFilenames := viper.GetBool("keep-filenames") + timeout := viper.GetDuration("timeout") backfill := viper.GetBool("backfill") @@ -79,7 +81,7 @@ func WatchCommand(_ *cobra.Command, args []string) error { if err != nil { return fmt.Errorf("failed to prepare converter: %w", err) } - log.Info().Str("path", path).Bool("override", override).Uint8("quality", quality).Str("format", converterType.String()).Bool("split", split).Bool("backfill", backfill).Msg("Watching directory") + log.Info().Str("path", path).Bool("override", override).Uint8("quality", quality).Str("format", converterType.String()).Bool("split", split).Bool("keep_filenames", keepFilenames).Bool("backfill", backfill).Msg("Watching directory") watcher, err := fsnotify.NewWatcher() if err != nil { @@ -98,6 +100,7 @@ func WatchCommand(_ *cobra.Command, args []string) error { Quality: quality, Override: override, Split: split, + KeepFilenames: keepFilenames, Timeout: timeout, }) defer queue.Stop() diff --git a/internal/cbz/cbz_creator.go b/internal/cbz/cbz_creator.go index 2a4b6c0..9b543a9 100644 --- a/internal/cbz/cbz_creator.go +++ b/internal/cbz/cbz_creator.go @@ -5,6 +5,8 @@ import ( "fmt" "io" "os" + "path/filepath" + "strings" "time" "github.com/belphemur/CBZOptimizer/v2/internal/manga" @@ -12,6 +14,56 @@ import ( "github.com/rs/zerolog/log" ) +// resolvePageName picks the final archive entry name for a page. +// +// Order of resolution: +// 1. When page.OriginalName is set (keep-filenames mode), use its stem with +// the current page.Extension. Split pages append the -NN suffix so all +// parts of the same source still land in the archive. +// 2. If that final name has already been used in this archive, fall back to +// the indexed form (stem + "_%04d" + extension) so the zip stays valid. +// 3. Otherwise (OriginalName is empty), use the historical %04d / %04d-NN +// sequential naming. +// +// usedNames tracks every name already chosen for this archive and is mutated +// in place so the caller can keep a single map across the whole chapter. +func resolvePageName(page *manga.PageFile, usedNames map[string]struct{}) string { + if page.OriginalName != "" { + stem := strings.TrimSuffix(page.OriginalName, filepath.Ext(page.OriginalName)) + var candidate string + if page.IsSplitted { + candidate = fmt.Sprintf("%s-%02d%s", stem, page.SplitPartIndex, page.Extension) + } else { + candidate = stem + page.Extension + } + if _, taken := usedNames[candidate]; !taken { + usedNames[candidate] = struct{}{} + return candidate + } + // Collision: fall back to the indexed form so the entry still gets + // written, but make it visibly distinct from the preserved one. + // Loop in case the fallback name is itself already taken (e.g. a + // source file literally named "cover_0001.png" colliding with a + // page-index 1 fallback "cover_0001.webp"). + for suffix := page.Index; ; suffix++ { + fallback := fmt.Sprintf("%s_%04d%s", stem, suffix, page.Extension) + if _, taken := usedNames[fallback]; !taken { + usedNames[fallback] = struct{}{} + return fallback + } + } + } + + if page.IsSplitted { + name := fmt.Sprintf("%04d-%02d%s", page.Index, page.SplitPartIndex, page.Extension) + usedNames[name] = struct{}{} + return name + } + name := fmt.Sprintf("%04d%s", page.Index, page.Extension) + usedNames[name] = struct{}{} + return name +} + // WriteChapterToCBZ creates a CBZ file from a Chapter by streaming page files // from disk directly into the zip archive. No image data is held in memory. func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) (err error) { @@ -34,14 +86,14 @@ func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) (err error zipWriter := zip.NewWriter(zipFile) defer errs.Capture(&err, zipWriter.Close, "failed to close .cbz writer") - // Write each page to the archive by streaming from disk + // Write each page to the archive by streaming from disk. + // Final name resolution: when a page carries an OriginalName (recorded by + // ExtractChapter when --keep-filenames is on), preserve its stem and only + // swap the extension to the current page.Extension. Duplicates in the + // archive fall back to the indexed naming so the output stays a valid zip. + usedNames := make(map[string]struct{}, len(chapter.Pages)) for _, page := range chapter.Pages { - var fileName string - if page.IsSplitted { - fileName = fmt.Sprintf("%04d-%02d%s", page.Index, page.SplitPartIndex, page.Extension) - } else { - fileName = fmt.Sprintf("%04d%s", page.Index, page.Extension) - } + fileName := resolvePageName(page, usedNames) log.Debug(). Str("output_path", outputFilePath). diff --git a/internal/cbz/cbz_creator_test.go b/internal/cbz/cbz_creator_test.go index 8d3ad64..d37dc50 100644 --- a/internal/cbz/cbz_creator_test.go +++ b/internal/cbz/cbz_creator_test.go @@ -101,6 +101,70 @@ func TestWriteChapterToCBZ(t *testing.T) { }, expectedFiles: []string{"0000-01.jpg"}, }, + { + // --keep-filenames: the page stem is reused as-is with the + // current Extension, regardless of the source extension. + name: "Preserve original filename, extension swap", + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + { + Index: 0, + Extension: ".webp", + FilePath: createTempPage(t, dir, "image data", ".webp"), + OriginalName: "page01.png", + }, + }, + } + }, + expectedFiles: []string{"page01.webp"}, + }, + { + // --keep-filenames for a split page: stem is preserved, the + // split part index still gets appended to keep parts distinct. + name: "Split page with OriginalName", + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + { + Index: 0, + Extension: ".webp", + FilePath: createTempPage(t, dir, "split data", ".webp"), + IsSplitted: true, + SplitPartIndex: 2, + OriginalName: "tall.png", + }, + }, + } + }, + expectedFiles: []string{"tall-02.webp"}, + }, + { + // Two pages share the same OriginalName (rare but possible + // with subdirectories): the first keeps the stem, the second + // falls back to the indexed form to avoid duplicate zip + // entries. + name: "Duplicate OriginalName falls back to indexed form", + chapter: func(t *testing.T, dir string) *manga.Chapter { + return &manga.Chapter{ + Pages: []*manga.PageFile{ + { + Index: 0, + Extension: ".webp", + FilePath: createTempPage(t, dir, "first", ".webp"), + OriginalName: "cover.png", + }, + { + Index: 1, + Extension: ".webp", + FilePath: createTempPage(t, dir, "second", ".webp"), + OriginalName: "cover.png", + }, + }, + } + }, + expectedFiles: []string{"cover.webp", "cover_0001.webp"}, + }, } for _, tc := range testCases { diff --git a/internal/cbz/cbz_loader.go b/internal/cbz/cbz_loader.go index 01c3af4..bd50048 100644 --- a/internal/cbz/cbz_loader.go +++ b/internal/cbz/cbz_loader.go @@ -142,7 +142,13 @@ func IsAlreadyConverted(ctx context.Context, filePath string) (converted bool, e // ExtractChapter extracts an archive (CBZ/CBR) to a temp directory on disk. // Pages are streamed directly to files — no image data is held in memory. // Returns a Chapter with PageFile entries pointing to extracted files. -func ExtractChapter(ctx context.Context, filePath string) (*manga.Chapter, error) { +// +// When keepFilenames is true, each PageFile has its OriginalName set to the +// base filename of the entry inside the archive. Downstream code uses that +// name to preserve the original page identity in the output CBZ (with the +// extension swapped for format conversion). When false, OriginalName stays +// empty and the sequential %04d naming convention is used instead. +func ExtractChapter(ctx context.Context, filePath string, keepFilenames bool) (*manga.Chapter, error) { log.Debug().Str("file_path", filePath).Msg("Extracting chapter to disk") // Create temp directory for extraction @@ -162,6 +168,22 @@ func ExtractChapter(ctx context.Context, filePath string) (*manga.Chapter, error TempDir: tempDir, } + // usedOriginalStems tracks the STEM (filename without extension) of every + // OriginalName handed out in this chapter so stems stay unique across + // pages. Tracking stems (not full names) prevents a downstream race in + // pkg/converter/webp: the converter strips the OriginalName extension + // and appends a target-format suffix (e.g. ".webp"), so two pages that + // share a stem but differ in extension — e.g. a/page.png and b/page.jpg + // (legal in zip) — would otherwise race on a single shared intermediate + // output path. The map covers both same-stem-same-ext collisions (e.g. + // a/page.png + b/page.png) and same-stem-different-ext collisions (e.g. + // a/page.png + b/page.jpg). Allocated lazily so the keepFilenames=false + // path stays allocation-free. + var usedOriginalStems map[string]struct{} + if keepFilenames { + usedOriginalStems = make(map[string]struct{}) + } + // For CBZ files, read metadata from zip comment pathLower := strings.ToLower(filepath.Ext(filePath)) if pathLower == ".cbz" { @@ -279,6 +301,9 @@ func ExtractChapter(ctx context.Context, filePath string) (*manga.Chapter, error Extension: ext, FilePath: outputPath, } + if keepFilenames { + page.OriginalName = allocateUniqueBaseName(archiveBaseName(path), pageIndex, usedOriginalStems) + } chapter.Pages = append(chapter.Pages, page) log.Debug(). @@ -319,9 +344,76 @@ func isJunkFile(path string) bool { return false } -// LoadChapter extracts the chapter from a CBZ/CBR file to disk. -// It delegates to ExtractChapter and always extracts all pages. -// Use IsAlreadyConverted for a fast conversion status check without extraction. -func LoadChapter(filePath string) (*manga.Chapter, error) { - return ExtractChapter(context.Background(), filePath) +// archiveBaseName returns the bare base name of an archive entry, with +// Windows-style backslash separators normalized to forward slashes before +// the last-segment split. The archives library surfaces zip entry names +// verbatim, so a name like "..\evil.png" (common from Windows-created +// archives) would otherwise pass through filepath.Base unchanged on non- +// Windows hosts and be written into the output CBZ as a path-traversal +// shape. Normalizing the separators first and then taking the segment +// after the final one guarantees the returned name is a bare base name +// with no directory components, no backslashes, and no forward slashes — +// which is what the downstream writer expects as a safe ZIP entry name. +// +// This intentionally does NOT touch filepath.Base calls in isJunkFile or +// the ComicInfo.xml / converted.txt detection: those comparisons just +// fail to match, and the entry then falls through to the non-image +// extension filter, so no traversal-shaped data escapes the loader. +func archiveBaseName(path string) string { + normalized := strings.ReplaceAll(path, "\\", "/") + if idx := strings.LastIndex(normalized, "/"); idx >= 0 { + return normalized[idx+1:] + } + return normalized +} + +// allocateUniqueBaseName returns baseName when its stem is not already taken +// by an earlier page in this chapter, or a collision-resolved variant +// otherwise. +// +// Stem-uniqueness contract: the function guarantees that the STEM +// (filename without extension) of the returned OriginalName is unique +// among all names previously handed out from this chapter. That contract +// matches what downstream code relies on — +// pkg/converter/webp.intermediatePageName strips the OriginalName's +// extension and appends a target-format suffix (e.g. ".webp"), so two +// OriginalNames that share a stem would otherwise race on a single +// intermediate output path. The stem is computed the same way downstream +// does it (strings.TrimSuffix(name, filepath.Ext(name))) to keep both +// definitions in lock-step. +// +// On collision the resolved name matches the existing fallback style used +// by cbz_creator's resolvePageName: stem + "_%04d" + extension. The loop +// checks the CANDIDATE's stem (not the full generated name) so two files +// that share a stem but differ in extension — e.g. "page.png" and +// "page.jpg" — resolve to, e.g., "page.png" and "page_0001.jpg", each +// preserving its original extension. The starting suffix is pageIndex so +// the resolved name stays in the same neighborhood as the page's archive +// position. If the candidate's stem is itself already taken +// (astronomically rare: the source archive would need both the original +// name and a matching indexed name), the suffix is incremented until a +// free stem is found. The chosen name's stem is recorded in usedStems so +// subsequent calls cannot pick it again. +func allocateUniqueBaseName(baseName string, pageIndex uint16, usedStems map[string]struct{}) string { + ext := filepath.Ext(baseName) + stem := strings.TrimSuffix(baseName, ext) + if _, taken := usedStems[stem]; !taken { + usedStems[stem] = struct{}{} + return baseName + } + for suffix := int(pageIndex); ; suffix++ { + candidateStem := fmt.Sprintf("%s_%04d", stem, suffix) + if _, taken := usedStems[candidateStem]; !taken { + usedStems[candidateStem] = struct{}{} + return candidateStem + ext + } + } +} + +// LoadChapter extracts the chapter from a CBZ/CBR file to disk. +// It delegates to ExtractChapter with keepFilenames=false and always +// extracts all pages. Use IsAlreadyConverted for a fast conversion status +// check without extraction. +func LoadChapter(filePath string) (*manga.Chapter, error) { + return ExtractChapter(context.Background(), filePath, false) } diff --git a/internal/cbz/cbz_loader_test.go b/internal/cbz/cbz_loader_test.go index 588bce2..6ccae6a 100644 --- a/internal/cbz/cbz_loader_test.go +++ b/internal/cbz/cbz_loader_test.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "path/filepath" + "regexp" + "strings" "testing" "time" @@ -157,12 +159,12 @@ func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) { } func TestExtractChapter_NonexistentFile(t *testing.T) { - _, err := ExtractChapter(context.Background(), "/nonexistent/file.cbz") + _, err := ExtractChapter(context.Background(), "/nonexistent/file.cbz", false) require.Error(t, err) } func TestExtractChapter_PageExtensions(t *testing.T) { - chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz") + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false) require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() @@ -174,17 +176,260 @@ func TestExtractChapter_PageExtensions(t *testing.T) { } func TestExtractChapter_PagesHaveSequentialIndices(t *testing.T) { - chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz") + t.Run("default sequential naming", func(t *testing.T) { + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + for i, page := range chapter.Pages { + assert.Equal(t, uint16(i), page.Index) + assert.Empty(t, page.OriginalName, "keep-filenames disabled: OriginalName must stay empty") + } + }) + + t.Run("keep-filenames records OriginalName", func(t *testing.T) { + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", true) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + require.NotEmpty(t, chapter.Pages) + for i, page := range chapter.Pages { + assert.Equal(t, uint16(i), page.Index) + assert.NotEmpty(t, page.OriginalName, "keep-filenames enabled: OriginalName must be set for every page") + // OriginalName must be a bare base name (no directory prefix), even + // when the source archive nests pages in subdirectories. + assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName, + "OriginalName should be a base filename with no path components") + } + }) +} + +// writeCollisionCBZ builds a CBZ containing the same image base name in two +// different subdirectories plus a normal unique page. The test fixture is +// specifically crafted to reproduce the race fixed in ExtractChapter: when +// keep-filenames naively copied filepath.Base(path) into OriginalName, both +// a/page.png and b/page.png would have ended up with the same stem and the +// WebP converter would have raced on a single shared intermediate path. +func writeCollisionCBZ(t *testing.T, dir string) string { + t.Helper() + cbzPath := filepath.Join(dir, "collisions.cbz") + f, err := os.Create(cbzPath) + require.NoError(t, err) + w := zip.NewWriter(f) + + for _, entry := range []string{"a/page.png", "b/page.png", "cover.png"} { + fw, err := w.Create(entry) + require.NoError(t, err) + _, err = fw.Write([]byte("dummy image bytes for " + entry)) + require.NoError(t, err) + } + require.NoError(t, w.Close()) + require.NoError(t, f.Close()) + return cbzPath +} + +func TestExtractChapter_KeepFilenames_DeduplicatesCollidingNames(t *testing.T) { + tmpDir := t.TempDir() + cbzPath := writeCollisionCBZ(t, tmpDir) + + t.Run("keep-filenames resolves colliding base names", func(t *testing.T) { + chapter, err := ExtractChapter(context.Background(), cbzPath, true) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + require.Len(t, chapter.Pages, 3, "expected 3 pages: two colliding + one unique") + + // All OriginalNames must be non-empty bare base names and unique. + seen := make(map[string]struct{}, len(chapter.Pages)) + for _, page := range chapter.Pages { + assert.NotEmpty(t, page.OriginalName, "page %d must have OriginalName set", page.Index) + assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName, + "page %d OriginalName should be a bare base filename", page.Index) + if _, dup := seen[page.OriginalName]; dup { + t.Errorf("duplicate OriginalName across pages: %q", page.OriginalName) + } + seen[page.OriginalName] = struct{}{} + } + + // The unique page keeps its original name; the two colliding pages + // must come out as one bare "page.png" and one indexed variant. + assert.Contains(t, seen, "page.png", "one colliding page should keep the bare page.png name") + assert.Contains(t, seen, "cover.png", "the unique page should keep its original name") + + indexedPattern := regexp.MustCompile(`^page_\d{4}\.png$`) + indexedCount := 0 + for name := range seen { + if indexedPattern.MatchString(name) { + indexedCount++ + } + } + assert.Equal(t, 1, indexedCount, + "exactly one colliding page should be resolved to the page_NNNN.png pattern, got %v", seen) + }) + + t.Run("keep-filenames off leaves OriginalName empty (regression guard)", func(t *testing.T) { + // Same collision-prone archive, but with keep-filenames off: the + // dedup logic must not run, so OriginalName stays empty for every + // page. This is the historical default and must not regress. + chapter, err := ExtractChapter(context.Background(), cbzPath, false) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + require.Len(t, chapter.Pages, 3) + for _, page := range chapter.Pages { + assert.Empty(t, page.OriginalName, "page %d must have empty OriginalName when keep-filenames is off", page.Index) + } + }) +} + +// writeBackslashCBZ builds a CBZ whose entry names mix Windows-style +// backslash separators with a normal forward-slash page. The fixture +// reproduces the CodeRabbit finding for PR #217: archive entries like +// "subdir\page.png" and "..\evil.png" must not surface verbatim as the +// OriginalName written into the output CBZ, since Windows ZIP consumers +// can interpret backslashes as path traversal. +func writeBackslashCBZ(t *testing.T, dir string) string { + t.Helper() + cbzPath := filepath.Join(dir, "backslash.cbz") + f, err := os.Create(cbzPath) + require.NoError(t, err) + w := zip.NewWriter(f) + + // archive/zip stores entry names verbatim, so the bytes on the wire + // carry the backslashes through to fs.WalkDir in ExtractChapter. + for _, entry := range []string{`subdir\page.png`, `..\evil.png`, "cover.png"} { + fw, err := w.Create(entry) + require.NoError(t, err) + _, err = fw.Write([]byte("dummy image bytes for " + entry)) + require.NoError(t, err) + } + require.NoError(t, w.Close()) + require.NoError(t, f.Close()) + return cbzPath +} + +func TestExtractChapter_KeepFilenames_NormalizesWindowsSeparators(t *testing.T) { + tmpDir := t.TempDir() + cbzPath := writeBackslashCBZ(t, tmpDir) + + chapter, err := ExtractChapter(context.Background(), cbzPath, true) require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() - for i, page := range chapter.Pages { - assert.Equal(t, uint16(i), page.Index) + require.Len(t, chapter.Pages, 3, "expected 3 pages: two backslash entries + one normal") + + // Every OriginalName must be a bare base name: no backslashes, no + // forward slashes, and unique across the chapter. + seen := make(map[string]struct{}, len(chapter.Pages)) + for _, page := range chapter.Pages { + assert.NotEmpty(t, page.OriginalName, "page %d must have OriginalName set", page.Index) + assert.NotContains(t, page.OriginalName, `\`, "page %d OriginalName must not contain backslash, got %q", page.Index, page.OriginalName) + assert.NotContains(t, page.OriginalName, "/", "page %d OriginalName must not contain forward slash, got %q", page.Index, page.OriginalName) + assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName, + "page %d OriginalName should be a bare base filename", page.Index) + if _, dup := seen[page.OriginalName]; dup { + t.Errorf("duplicate OriginalName across pages: %q", page.OriginalName) + } + seen[page.OriginalName] = struct{}{} } + + // The two backslash entries must collapse to their bare bases; the + // "..\evil.png" entry's leading "..\evil" must not be carried through. + assert.Contains(t, seen, "page.png", "subdir\\page.png should normalize to bare page.png") + assert.Contains(t, seen, "evil.png", "..\\evil.png should normalize to bare evil.png") + assert.Contains(t, seen, "cover.png", "cover.png should pass through unchanged") +} + +// writeSameStemDifferentExtCBZ builds a CBZ containing two same-stem pages +// with different extensions plus a distinct unique page. The fixture +// reproduces the CodeRabbit finding for PR #217: a/page.png and b/page.jpg +// share a stem but do NOT share a full filename, so naive full-name +// deduplication lets both OriginalNames through and the WebP converter then +// races on a single shared intermediate output path (outputDir/page.webp). +// The fix flips the dedup tracking to STEM-uniqueness, so one of the +// colliding pair must come out as a bare "page." and the other as +// "page_NNNN." — each preserving its original extension. +func writeSameStemDifferentExtCBZ(t *testing.T, dir string) string { + t.Helper() + cbzPath := filepath.Join(dir, "same_stem.cbz") + f, err := os.Create(cbzPath) + require.NoError(t, err) + w := zip.NewWriter(f) + + for _, entry := range []string{"a/page.png", "b/page.jpg", "cover.png"} { + fw, err := w.Create(entry) + require.NoError(t, err) + _, err = fw.Write([]byte("dummy image bytes for " + entry)) + require.NoError(t, err) + } + require.NoError(t, w.Close()) + require.NoError(t, f.Close()) + return cbzPath +} + +func TestExtractChapter_KeepFilenames_DeduplicatesSameStemDifferentExtensions(t *testing.T) { + tmpDir := t.TempDir() + cbzPath := writeSameStemDifferentExtCBZ(t, tmpDir) + + chapter, err := ExtractChapter(context.Background(), cbzPath, true) + require.NoError(t, err) + defer func() { _ = chapter.Cleanup() }() + + require.Len(t, chapter.Pages, 3, "expected 3 pages: two same-stem + one unique") + + // Every OriginalName must be a bare base name; full names AND stems + // must each be unique across the chapter. The stem-uniqueness check is + // the contract the WebP converter relies on (it strips the extension + // and appends ".webp" when computing intermediatePageName). + seenNames := make(map[string]struct{}, len(chapter.Pages)) + seenStems := make(map[string]struct{}, len(chapter.Pages)) + for _, page := range chapter.Pages { + assert.NotEmpty(t, page.OriginalName, "page %d must have OriginalName set", page.Index) + assert.Equal(t, filepath.Base(page.OriginalName), page.OriginalName, + "page %d OriginalName should be a bare base filename", page.Index) + if _, dup := seenNames[page.OriginalName]; dup { + t.Errorf("duplicate OriginalName full name across pages: %q", page.OriginalName) + } + seenNames[page.OriginalName] = struct{}{} + + stem := strings.TrimSuffix(page.OriginalName, filepath.Ext(page.OriginalName)) + if _, dup := seenStems[stem]; dup { + t.Errorf("duplicate OriginalName stem across pages: %q (from %q)", stem, page.OriginalName) + } + seenStems[stem] = struct{}{} + } + + // The unique page keeps its original name untouched. + assert.Contains(t, seenNames, "cover.png", "the unique page should keep its original name") + + // The colliding pair must split into one bare "page." and one + // "page_NNNN.". The extensions must differ — each must keep the + // extension of the archive entry it came from. Walk order is not + // guaranteed, so the test checks both shapes regardless of which + // entry comes first. + barePattern := regexp.MustCompile(`^page\.(png|jpg)$`) + indexedPattern := regexp.MustCompile(`^page_\d{4}\.(png|jpg)$`) + + var bareExt, indexedExt string + for name := range seenNames { + if barePattern.MatchString(name) { + bareExt = filepath.Ext(name) + } + if indexedPattern.MatchString(name) { + indexedExt = filepath.Ext(name) + } + } + assert.NotEmpty(t, bareExt, + "expected one bare page. OriginalName from the colliding pair, got %v", seenNames) + assert.NotEmpty(t, indexedExt, + "expected one page_NNNN. OriginalName from the colliding pair, got %v", seenNames) + assert.NotEqual(t, bareExt, indexedExt, + "the bare and indexed variants must keep their original different extensions, got bare=%q indexed=%q", + bareExt, indexedExt) } func TestExtractChapter_Cleanup(t *testing.T) { - chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz") + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 128.cbz", false) require.NoError(t, err) tempDir := chapter.TempDir @@ -199,7 +444,7 @@ func TestExtractChapter_Cleanup(t *testing.T) { } func TestExtractChapter_CBR(t *testing.T) { - chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 1.cbr") + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 1.cbr", false) require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() @@ -212,7 +457,7 @@ func TestExtractChapter_CBR(t *testing.T) { } func TestExtractChapter_ConvertedStatus(t *testing.T) { - chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 10_converted.cbz") + chapter, err := ExtractChapter(context.Background(), "../../testdata/Chapter 10_converted.cbz", false) require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() @@ -356,7 +601,7 @@ func TestExtractChapter_WithConvertedTxt(t *testing.T) { _ = w.Close() _ = f.Close() - chapter, err := ExtractChapter(context.Background(), cbzPath) + chapter, err := ExtractChapter(context.Background(), cbzPath, false) require.NoError(t, err) defer func() { _ = chapter.Cleanup() }() diff --git a/internal/manga/page.go b/internal/manga/page.go index 5d42ea6..a3dae2d 100644 --- a/internal/manga/page.go +++ b/internal/manga/page.go @@ -13,4 +13,10 @@ type PageFile struct { IsSplitted bool // SplitPartIndex is the part index when the page was split. SplitPartIndex uint16 + // OriginalName is the base filename (e.g. "page01.png") of the page as + // it appeared in the source archive, recorded when the --keep-filenames + // flag is enabled. Empty when the flag is off or the source name is + // unknown. When set, downstream code uses this stem (with the final + // Extension swapped in) instead of the default %04d sequential name. + OriginalName string } diff --git a/internal/utils/optimize.go b/internal/utils/optimize.go index b0c380d..d8f2591 100644 --- a/internal/utils/optimize.go +++ b/internal/utils/optimize.go @@ -21,7 +21,12 @@ type OptimizeOptions struct { Quality uint8 Override bool Split bool - Timeout time.Duration + // KeepFilenames preserves the original base filename of each page inside + // the output CBZ (with the extension swapped for format conversion) + // instead of the historical %04d sequential naming. Off by default so + // existing behavior is unchanged. + KeepFilenames bool + Timeout time.Duration } // Optimize optimizes a CBZ/CBR file using the specified converter. @@ -38,6 +43,7 @@ func Optimize(options *OptimizeOptions) error { Uint8("quality", options.Quality). Bool("override", options.Override). Bool("split", options.Split). + Bool("keep_filenames", options.KeepFilenames). Msg("Optimization parameters") // Step 1: Fast conversion check before extracting (new requirement) @@ -64,7 +70,7 @@ func Optimize(options *OptimizeOptions) error { extractCtx = context.Background() } - chapter, err := cbz.ExtractChapter(extractCtx, options.Path) + chapter, err := cbz.ExtractChapter(extractCtx, options.Path, options.KeepFilenames) if err != nil { log.Error().Str("file", options.Path).Err(err).Msg("Failed to extract chapter") return fmt.Errorf("failed to extract chapter: %w", err) diff --git a/pkg/converter/webp/webp_converter.go b/pkg/converter/webp/webp_converter.go index bf04d1a..97d400e 100644 --- a/pkg/converter/webp/webp_converter.go +++ b/pkg/converter/webp/webp_converter.go @@ -25,6 +25,26 @@ import ( const webpMaxHeight = 16383 +// intermediatePageName returns the on-disk filename used for a page's WebP +// intermediate during conversion. When the page carries an OriginalName +// (recorded by --keep-filenames), its stem is reused so the temp file line +// up with the name the archive writer will pick. Otherwise the historical +// %04d indexed naming is kept. The splitSuffix argument is appended verbatim +// after the stem (e.g. "-00", "-01") for split parts and is empty for the +// happy-path single output. A leading dash is only added when both the +// split suffix and the original-name stem are present, so the indexed form +// stays as %04d-%02d. +func intermediatePageName(page *manga.PageFile, splitSuffix string) string { + if page.OriginalName != "" { + stem := strings.TrimSuffix(page.OriginalName, filepath.Ext(page.OriginalName)) + return stem + splitSuffix + ".webp" + } + if splitSuffix == "" { + return fmt.Sprintf("%04d.webp", page.Index) + } + return fmt.Sprintf("%04d%s.webp", page.Index, splitSuffix) +} + type Converter struct { maxHeight int cropHeight int @@ -218,22 +238,26 @@ func (converter *Converter) convertPageFile(ctx context.Context, page *manga.Pag Str("input", page.FilePath). Msg("Converting page file") - // If the page is already WebP, just return it as-is + // If the page is already WebP, just return it as-is. The returned page + // keeps any OriginalName set during extraction so the archive writer can + // honor --keep-filenames for the final entry name. if strings.ToLower(page.Extension) == ".webp" { log.Debug().Uint16("page_index", page.Index).Msg("Page already WebP, skipping") return []*manga.PageFile{page}, nil } // Try direct file-to-file conversion first (happy path — no memory allocation) - outputPath := filepath.Join(outputDir, fmt.Sprintf("%04d.webp", page.Index)) + outputPath := filepath.Join(outputDir, intermediatePageName(page, "")) err := EncodeFile(page.FilePath, outputPath, uint(quality)) if err == nil { - // Success! No image decoding needed. + // Success! No image decoding needed. Preserve OriginalName so + // --keep-filenames carries through to the final zip entry name. return []*manga.PageFile{{ - Index: page.Index, - Extension: ".webp", - FilePath: outputPath, + Index: page.Index, + Extension: ".webp", + FilePath: outputPath, + OriginalName: page.OriginalName, }}, nil } @@ -316,7 +340,7 @@ func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.Pag partHeight = height - yOffset } - outputPath := filepath.Join(outputDir, fmt.Sprintf("%04d-%02d.webp", page.Index, i)) + outputPath := filepath.Join(outputDir, intermediatePageName(page, fmt.Sprintf("-%02d", i))) err := EncodeFileWithCrop(page.FilePath, outputPath, uint(quality), 0, yOffset, width, partHeight) if err != nil { @@ -334,6 +358,7 @@ func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.Pag FilePath: outputPath, IsSplitted: true, SplitPartIndex: uint16(i), + OriginalName: page.OriginalName, }) } diff --git a/pkg/converter/webp/webp_converter_test.go b/pkg/converter/webp/webp_converter_test.go index cc28a59..76af3d1 100644 --- a/pkg/converter/webp/webp_converter_test.go +++ b/pkg/converter/webp/webp_converter_test.go @@ -485,3 +485,36 @@ func TestEncodeFileWithCrop(t *testing.T) { require.NoError(t, err) assert.Greater(t, info.Size(), int64(0)) } + +// TestIntermediatePageName_StemsAreUnique pins the contract that +// intermediatePageName must produce a different on-disk filename for two +// pages whose OriginalNames share a stem via the original archive entry +// name. ExtractChapter guarantees stem-unique OriginalNames per chapter +// (so, e.g., a/page.png and b/page.jpg come out as "page.png" and +// "page_0001.jpg"), and the converter must reflect that: stripping the +// extension and appending ".webp" must not collapse two distinct stems +// onto the same intermediate path. +func TestIntermediatePageName_StemsAreUnique(t *testing.T) { + tests := []struct { + name string + originalName string + splitSuffix string + }{ + {name: "bare stem", originalName: "page.png", splitSuffix: ""}, + {name: "indexed stem (post-fix)", originalName: "page_0001.jpg", splitSuffix: ""}, + {name: "bare stem + split suffix", originalName: "page.png", splitSuffix: "-00"}, + {name: "indexed stem + split suffix", originalName: "page_0001.jpg", splitSuffix: "-00"}, + } + + names := make(map[string]struct{}, len(tests)) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + page := &manga.PageFile{Index: 0, OriginalName: tt.originalName} + got := intermediatePageName(page, tt.splitSuffix) + if _, dup := names[got]; dup { + t.Errorf("intermediate name %q collides with an earlier page's intermediate name", got) + } + names[got] = struct{}{} + }) + } +}