Merge pull request #217 from Belphemur/feat/keep-filenames

feat(converter): add --keep-filenames flag to preserve original page filenames
This commit is contained in:
Antoine Aflalo
2026-07-21 20:26:36 -04:00
committed by GitHub
13 changed files with 593 additions and 34 deletions
+7
View File
@@ -58,6 +58,12 @@ cbzconverter optimize [folder] --format=webp
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: With timeout to avoid hanging on problematic chapters:
```sh ```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. - `--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` - Can be specified as: `--format webp`, `-f webp`, or `--format=webp`
- Case-insensitive: `webp`, `WEBP`, and `WebP` are all valid - 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. - `--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. - `--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. - `--log`, `-l`: Set log level; can be 'panic', 'fatal', 'error', 'warn', 'info', 'debug', or 'trace'. Default is info.
+14
View File
@@ -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. // setupTimeoutFlag sets up the timeout flag for a command.
// //
// Parameters: // Parameters:
@@ -96,5 +109,6 @@ func setupCommonFlags(cmd *cobra.Command, converterType *constant.ConversionForm
setupQualityFlag(cmd, qualityDefault, bindViper) setupQualityFlag(cmd, qualityDefault, bindViper)
setupOverrideFlag(cmd, overrideDefault, bindViper) setupOverrideFlag(cmd, overrideDefault, bindViper)
setupSplitFlag(cmd, splitDefault, bindViper) setupSplitFlag(cmd, splitDefault, bindViper)
setupKeepFilenamesFlag(cmd, false, bindViper)
setupTimeoutFlag(cmd, bindViper) setupTimeoutFlag(cmd, bindViper)
} }
@@ -73,6 +73,13 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error {
} }
log.Debug().Bool("split", split).Msg("Split parameter parsed") 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") timeout, err := cmd.Flags().GetDuration("timeout")
if err != nil { if err != nil {
log.Error().Err(err).Msg("Failed to parse timeout flag") log.Error().Err(err).Msg("Failed to parse timeout flag")
@@ -127,6 +134,7 @@ func ConvertCbzCommand(cmd *cobra.Command, args []string) error {
Quality: quality, Quality: quality,
Override: override, Override: override,
Split: split, Split: split,
KeepFilenames: keepFilenames,
Timeout: timeout, Timeout: timeout,
}) })
if err != nil { if err != nil {
@@ -86,6 +86,7 @@ func TestConvertCbzCommand(t *testing.T) {
cmd.Flags().IntP("parallelism", "n", 2, "Number of chapters to convert in parallel") 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("override", "o", false, "Override the original CBZ/CBR files")
cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") 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") 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 // 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().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("override", "o", false, "Override the original CBZ/CBR files")
cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") 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") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter")
// Reset converterType to default before test for consistency // 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().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("override", "o", false, "Override the original CBZ/CBR files")
cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") 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") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter")
converterType = constant.DefaultConversion 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().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("override", "o", false, "Override the original CBZ/CBR files")
cmd.Flags().BoolP("split", "s", false, "Split long pages into smaller chunks") 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") cmd.Flags().DurationP("timeout", "t", 0, "Maximum time allowed for converting a single chapter")
converterType = constant.DefaultConversion converterType = constant.DefaultConversion
+4 -1
View File
@@ -65,6 +65,8 @@ func WatchCommand(_ *cobra.Command, args []string) error {
split := viper.GetBool("split") split := viper.GetBool("split")
keepFilenames := viper.GetBool("keep-filenames")
timeout := viper.GetDuration("timeout") timeout := viper.GetDuration("timeout")
backfill := viper.GetBool("backfill") backfill := viper.GetBool("backfill")
@@ -79,7 +81,7 @@ func WatchCommand(_ *cobra.Command, args []string) error {
if err != nil { if err != nil {
return fmt.Errorf("failed to prepare converter: %w", err) 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() watcher, err := fsnotify.NewWatcher()
if err != nil { if err != nil {
@@ -98,6 +100,7 @@ func WatchCommand(_ *cobra.Command, args []string) error {
Quality: quality, Quality: quality,
Override: override, Override: override,
Split: split, Split: split,
KeepFilenames: keepFilenames,
Timeout: timeout, Timeout: timeout,
}) })
defer queue.Stop() defer queue.Stop()
+59 -7
View File
@@ -5,6 +5,8 @@ import (
"fmt" "fmt"
"io" "io"
"os" "os"
"path/filepath"
"strings"
"time" "time"
"github.com/belphemur/CBZOptimizer/v2/internal/manga" "github.com/belphemur/CBZOptimizer/v2/internal/manga"
@@ -12,6 +14,56 @@ import (
"github.com/rs/zerolog/log" "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 // 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. // from disk directly into the zip archive. No image data is held in memory.
func WriteChapterToCBZ(chapter *manga.Chapter, outputFilePath string) (err error) { 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) zipWriter := zip.NewWriter(zipFile)
defer errs.Capture(&err, zipWriter.Close, "failed to close .cbz writer") 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 { for _, page := range chapter.Pages {
var fileName string fileName := resolvePageName(page, usedNames)
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)
}
log.Debug(). log.Debug().
Str("output_path", outputFilePath). Str("output_path", outputFilePath).
+64
View File
@@ -101,6 +101,70 @@ func TestWriteChapterToCBZ(t *testing.T) {
}, },
expectedFiles: []string{"0000-01.jpg"}, 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 { for _, tc := range testCases {
+98 -6
View File
@@ -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. // 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. // Pages are streamed directly to files — no image data is held in memory.
// Returns a Chapter with PageFile entries pointing to extracted files. // 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") log.Debug().Str("file_path", filePath).Msg("Extracting chapter to disk")
// Create temp directory for extraction // Create temp directory for extraction
@@ -162,6 +168,22 @@ func ExtractChapter(ctx context.Context, filePath string) (*manga.Chapter, error
TempDir: tempDir, 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 // For CBZ files, read metadata from zip comment
pathLower := strings.ToLower(filepath.Ext(filePath)) pathLower := strings.ToLower(filepath.Ext(filePath))
if pathLower == ".cbz" { if pathLower == ".cbz" {
@@ -279,6 +301,9 @@ func ExtractChapter(ctx context.Context, filePath string) (*manga.Chapter, error
Extension: ext, Extension: ext,
FilePath: outputPath, FilePath: outputPath,
} }
if keepFilenames {
page.OriginalName = allocateUniqueBaseName(archiveBaseName(path), pageIndex, usedOriginalStems)
}
chapter.Pages = append(chapter.Pages, page) chapter.Pages = append(chapter.Pages, page)
log.Debug(). log.Debug().
@@ -319,9 +344,76 @@ func isJunkFile(path string) bool {
return false return false
} }
// LoadChapter extracts the chapter from a CBZ/CBR file to disk. // archiveBaseName returns the bare base name of an archive entry, with
// It delegates to ExtractChapter and always extracts all pages. // Windows-style backslash separators normalized to forward slashes before
// Use IsAlreadyConverted for a fast conversion status check without extraction. // the last-segment split. The archives library surfaces zip entry names
func LoadChapter(filePath string) (*manga.Chapter, error) { // verbatim, so a name like "..\evil.png" (common from Windows-created
return ExtractChapter(context.Background(), filePath) // 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)
} }
+252 -7
View File
@@ -6,6 +6,8 @@ import (
"fmt" "fmt"
"os" "os"
"path/filepath" "path/filepath"
"regexp"
"strings"
"testing" "testing"
"time" "time"
@@ -157,12 +159,12 @@ func TestIsAlreadyConverted_CBZWithNonDateComment(t *testing.T) {
} }
func TestExtractChapter_NonexistentFile(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) require.Error(t, err)
} }
func TestExtractChapter_PageExtensions(t *testing.T) { 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) require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }() defer func() { _ = chapter.Cleanup() }()
@@ -174,17 +176,260 @@ func TestExtractChapter_PageExtensions(t *testing.T) {
} }
func TestExtractChapter_PagesHaveSequentialIndices(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) require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }() defer func() { _ = chapter.Cleanup() }()
for i, page := range chapter.Pages { for i, page := range chapter.Pages {
assert.Equal(t, uint16(i), page.Index) 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() }()
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.<ext>" and the other as
// "page_NNNN.<ext>" — 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.<ext>" and one
// "page_NNNN.<ext>". 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.<ext> OriginalName from the colliding pair, got %v", seenNames)
assert.NotEmpty(t, indexedExt,
"expected one page_NNNN.<ext> 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) { 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) require.NoError(t, err)
tempDir := chapter.TempDir tempDir := chapter.TempDir
@@ -199,7 +444,7 @@ func TestExtractChapter_Cleanup(t *testing.T) {
} }
func TestExtractChapter_CBR(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) require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }() defer func() { _ = chapter.Cleanup() }()
@@ -212,7 +457,7 @@ func TestExtractChapter_CBR(t *testing.T) {
} }
func TestExtractChapter_ConvertedStatus(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) require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }() defer func() { _ = chapter.Cleanup() }()
@@ -356,7 +601,7 @@ func TestExtractChapter_WithConvertedTxt(t *testing.T) {
_ = w.Close() _ = w.Close()
_ = f.Close() _ = f.Close()
chapter, err := ExtractChapter(context.Background(), cbzPath) chapter, err := ExtractChapter(context.Background(), cbzPath, false)
require.NoError(t, err) require.NoError(t, err)
defer func() { _ = chapter.Cleanup() }() defer func() { _ = chapter.Cleanup() }()
+6
View File
@@ -13,4 +13,10 @@ type PageFile struct {
IsSplitted bool IsSplitted bool
// SplitPartIndex is the part index when the page was split. // SplitPartIndex is the part index when the page was split.
SplitPartIndex uint16 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
} }
+7 -1
View File
@@ -21,6 +21,11 @@ type OptimizeOptions struct {
Quality uint8 Quality uint8
Override bool Override bool
Split bool Split bool
// 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 Timeout time.Duration
} }
@@ -38,6 +43,7 @@ func Optimize(options *OptimizeOptions) error {
Uint8("quality", options.Quality). Uint8("quality", options.Quality).
Bool("override", options.Override). Bool("override", options.Override).
Bool("split", options.Split). Bool("split", options.Split).
Bool("keep_filenames", options.KeepFilenames).
Msg("Optimization parameters") Msg("Optimization parameters")
// Step 1: Fast conversion check before extracting (new requirement) // Step 1: Fast conversion check before extracting (new requirement)
@@ -64,7 +70,7 @@ func Optimize(options *OptimizeOptions) error {
extractCtx = context.Background() extractCtx = context.Background()
} }
chapter, err := cbz.ExtractChapter(extractCtx, options.Path) chapter, err := cbz.ExtractChapter(extractCtx, options.Path, options.KeepFilenames)
if err != nil { if err != nil {
log.Error().Str("file", options.Path).Err(err).Msg("Failed to extract chapter") log.Error().Str("file", options.Path).Err(err).Msg("Failed to extract chapter")
return fmt.Errorf("failed to extract chapter: %w", err) return fmt.Errorf("failed to extract chapter: %w", err)
+29 -4
View File
@@ -25,6 +25,26 @@ import (
const webpMaxHeight = 16383 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 { type Converter struct {
maxHeight int maxHeight int
cropHeight int cropHeight int
@@ -218,22 +238,26 @@ func (converter *Converter) convertPageFile(ctx context.Context, page *manga.Pag
Str("input", page.FilePath). Str("input", page.FilePath).
Msg("Converting page file") 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" { if strings.ToLower(page.Extension) == ".webp" {
log.Debug().Uint16("page_index", page.Index).Msg("Page already WebP, skipping") log.Debug().Uint16("page_index", page.Index).Msg("Page already WebP, skipping")
return []*manga.PageFile{page}, nil return []*manga.PageFile{page}, nil
} }
// Try direct file-to-file conversion first (happy path — no memory allocation) // 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)) err := EncodeFile(page.FilePath, outputPath, uint(quality))
if err == nil { 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{{ return []*manga.PageFile{{
Index: page.Index, Index: page.Index,
Extension: ".webp", Extension: ".webp",
FilePath: outputPath, FilePath: outputPath,
OriginalName: page.OriginalName,
}}, nil }}, nil
} }
@@ -316,7 +340,7 @@ func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.Pag
partHeight = height - yOffset 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) err := EncodeFileWithCrop(page.FilePath, outputPath, uint(quality), 0, yOffset, width, partHeight)
if err != nil { if err != nil {
@@ -334,6 +358,7 @@ func (converter *Converter) splitAndConvert(ctx context.Context, page *manga.Pag
FilePath: outputPath, FilePath: outputPath,
IsSplitted: true, IsSplitted: true,
SplitPartIndex: uint16(i), SplitPartIndex: uint16(i),
OriginalName: page.OriginalName,
}) })
} }
+33
View File
@@ -485,3 +485,36 @@ func TestEncodeFileWithCrop(t *testing.T) {
require.NoError(t, err) require.NoError(t, err)
assert.Greater(t, info.Size(), int64(0)) 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{}{}
})
}
}