feat: optional photos.filename_format: simple (boredazfcuk migration support)#457
feat: optional photos.filename_format: simple (boredazfcuk migration support)#457epheterson wants to merge 4 commits into
Conversation
Enables zero-redownload migration from boredazfcuk/docker-icloudpd by producing the same plain IMG_1234.HEIC filenames that boredazfcuk writes. Since check_photo_exists compares by file size (not filename suffix), an existing tree of boredazfcuk-format files is correctly recognised as already-present when this container points at it. Features: - New config: photos.filename_format: metadata|simple (default 'metadata', no behaviour change for existing users) - generate_photo_filename_with_metadata branches on the format - Threading via module-level _DEFAULT_FILENAME_FORMAT set by sync_photos at run start — avoids touching every collect_download_task / generate_photo_path signature - Collision fallback in collect_download_task: when two distinct iCloud photos share a human filename and the plain path is already taken, the colliding photo routes to the metadata-suffix path so both files coexist. Cross-sync stable. 20 new tests covering: - tests/test_filename_format.py (15): config helper, generator branching, module-default singleton, end-to-end via generate_photo_path - tests/test_collision_fallback.py (5): no-collision, same-photo re-sync, collision-with-fallback (first file untouched), suffix already downloaded skip, metadata-mode bypass
3 SLF001 errors on assertions that read the module-private _DEFAULT_FILENAME_FORMAT constant — the test legitimately needs to verify the default value flipped after config load. Marked with # noqa: SLF001 inline (matches project's existing convention). Plus 3 trailing-comma auto-fixes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds an optional photos.filename_format config (metadata default vs simple) that lets users migrate from boredazfcuk/docker-icloudpd's plain IMG_1234.HEIC naming without re-downloading, with a fallback that routes filename-collisions in simple mode to the existing metadata-suffix path.
Changes:
- New
get_photos_filename_format()config helper and a module-level_DEFAULT_FILENAME_FORMATstate inphoto_path_utils.pyset once per sync run. generate_photo_filename_with_metadata()branches on format;collect_download_task()adds a collision fallback that switches to the metadata-suffix filename when the plain path is occupied by a different photo (size mismatch).- 20 new tests covering the config helper, generator branching, end-to-end path generation, and the collision fallback scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/config_parser.py | Adds get_photos_filename_format() returning "metadata"/"simple" with normalization and fallback. |
| src/photo_path_utils.py | Introduces _DEFAULT_FILENAME_FORMAT and set_default_filename_format(); generate_photo_filename_with_metadata now branches by format. |
| src/photo_download_manager.py | In collect_download_task, reorders the files set update around check_photo_exists and adds the simple-mode collision fallback to the suffix path. |
| src/sync_photos.py | Calls set_default_filename_format() at sync start with the configured value. |
| tests/test_filename_format.py | Covers config helper, generator branching, module-default behavior, and end-to-end generate_photo_path. |
| tests/test_collision_fallback.py | Covers no-collision, same-photo resync, collision-to-suffix, suffix-already-downloaded, and metadata-mode bypass. |
Three real issues flagged on this PR: 1. **In-flight collision (data-loss path)** — collision detection only checked ``os.path.isfile(photo_path)``. Within a single sync the collector runs sequentially through albums; two distinct iCloud photos sharing a human filename, both new this sync, would both pass the on-disk check (neither yet downloaded), both task the same plain path, and the later parallel download would silently overwrite the earlier one. Bad for a backup tool. Added an in-flight check against the shared ``files`` set under ``files_lock``: a path already claimed by an earlier collect_download_task call this sync counts as a collision and routes to the metadata-suffix path. New regression test covers the same-sync-both-new scenario. 2. **Redundant inline reimports** — ``create_folder_path_if_needed``, ``generate_photo_filename_with_metadata``, and ``normalize_file_path`` were imported at module top AND re-imported inside ``collect_download_task``. The only reason for the inline import was to read the live value of ``_DEFAULT_FILENAME_FORMAT`` (a ``from x import y`` at module top binds at import time and misses subsequent ``set_default_filename_format`` updates). Brittle — a future cleanup pass that hoists the inline import would silently break ``filename_format: simple`` with no test failure. Added a public ``get_default_filename_format()`` accessor in ``photo_path_utils`` and replaced the inline reimport with a normal top-level import of the accessor. 3. **Undocumented config knob** — added ``photos.filename_format`` to both ``README.md`` and the sample ``config.yaml`` alongside the other ``photos.*`` options (``folder_format``, ``use_hardlinks``). Without docs, users migrating from boredazfcuk would never discover the knob. Verified on python:3.10 docker mirroring CI: ruff clean, 100% coverage. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I guess everyone have different preferences about filenames, so maybe we can make them configurable? Simple one like this: or something more advanced like: @epheterson what do you think? |
Adds optional photos.file_format: one ${photo.*} template applied to every
downloaded version (mirrors the existing folder_format). Overrides
filename_format when set; falls back to it when unset.
Tokens: ${photo.filename} ${photo.ext} ${photo.id} ${photo.file_size}
${photo.year/month/day}, plus variant tokens -- empty for the primary versions
(original/full), the version name otherwise: ${photo.variant} (bare) and
${photo.variant_suffix} (separator + variant, emitted ONLY when there is a
variant, so originals stay un-suffixed). Separator via photos.variant_separator
(default "_").
Example "${photo.filename}${photo.variant_suffix}.${photo.ext}" reproduces plain
Apple/boredazfcuk names for originals, so an existing simple-format library is
recognised (no re-download). A templated name that collides falls back to the
unique metadata name.
- photo_path_utils: render_filename_template + set/get_file_format; template
path in generate (bypassed when metadata is forced)
- photo_download_manager: collision fallback covers file_format
- config_parser: get_photos_file_format + get_photos_variant_separator
- sync_photos: apply at run start; docs (README + config.yaml) + tests
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hey @olfway, good point I was trying to match the file structure from iCloudPD to avoid re-downloading but we can do that while making it more flexible. I also favor the simple map for all types over defining per type. I think this commit is an elegant way to do it, take a look and let me know what you think. |
Summary
Enables zero-redownload migration from boredazfcuk/docker-icloudpd by
producing the same plain
IMG_1234.HEICfilenames that boredazfcukwrites. Since
check_photo_existscompares by file size (not filenamesuffix), an existing tree of boredazfcuk-format files is correctly
recognised as already-present when this container points at it.
Also includes a collision-safe fallback: when simple format would put
two distinct iCloud photos at the same path (rare but happens when the
same human filename appears on multiple photos), the colliding photo
routes to the metadata-suffix path so both files coexist on disk.
Features
photos.filename_format(new optional config, defaultmetadata)metadata(default):name__filesize__base64id.extension— historicalmandarons format. Backward-compatible, no behaviour change for existing
users.
simple:name.extension— boredazfcuk/Apple convention.Collision fallback (active automatically when filename_format is simple)
In
collect_download_task, when the plainsimple-format path alreadyexists and the existing file has a DIFFERENT size from the current photo's
CloudKit
versions[file_size].size, the colliding photo's destination isswitched to the metadata-suffix form. First photo keeps its plain name;
subsequent colliders get unique suffixes. Cross-sync stable.
Implementation
_DEFAULT_FILENAME_FORMATinphoto_path_utils.pysetonce per sync run by
sync_photos.sync_photos()via the newset_default_filename_format()setter. Avoids threading filename_formatthrough every
collect_download_task/generate_photo_pathsignaturecall.
generate_photo_filename_with_metadatabranches on the format.collect_download_taskusesos.path.isfilecheck_photo_existssize comparison.Tests
20 new tests total:
tests/test_filename_format.py(15 tests): config helper defaults /normalisation / fallback on unknown value, generator branching, the
module-default singleton, end-to-end via
generate_photo_path.tests/test_collision_fallback.py(5 tests): no collision uses plainpath, same photo re-sync skips, collision falls back to suffix (with
the first file untouched), collision + suffix already downloaded skips,
metadata mode bypasses the collision logic entirely.
Notes
filename_formatcannot be safely changed mid-flight on an existinginstall — switching means mandarons won't match previously-downloaded
files and will re-download. Documented in the config helper docstring.
collect_download_taskrecompiles the suffix path withexplicit
filename_format="metadata"so the fallback works regardlessof the module-level default.