v0.2.0: semaphore timeouts, error logging, dead code removal, parallel exports

Critical:
- Replace DISPATCH_TIME_FOREVER with 120s/30s timeouts in ObjC
- Log failed asset IDs and error messages in cmdExport/backupTree
- Show failed count in export summaries

Cleanup:
- Remove legacy Bridge methods (ExportAlbumPreviews, ExportAlbumOriginals, BackupAll)
- Remove legacy ObjC functions and C stub equivalents
- Remove photos.go delegates (package-level pass-throughs)
- Remove InterpretExportResult (only used by legacy methods)
- Clean up mockBridge fields (rename Fn2 -> Fn)
- Fix rc race condition in main_main.go (atomic.Int32)
- Remove unused variables (_ = grandTotal, _ = sig)

Design:
- Fix resolveAlbumID: ListAlbums first (cheap), then direct ID
- Unify Cloud type: Asset.Cloud string (was bool)
- Extract shared export logic into exportAssets/exportOne
- Add worker pool for parallel exports (3 workers when assets >= 4)
- Fix backupTree progress bar counter and directory prefix

Robustness:
- Add nil checks for stringWithUTF8String: in ObjC
- Log directory creation errors in ensure_directory (ObjC)

Quality:
- Add go vet and -race flag to Makefile test target
- Add ADR for performSelector cloudIdentifier decision
- Add sync comments between Go/ObjC sanitizePathComponent
- Add package-level doc comment
- Add tests: partial failure, skipped album, album-not-found message
This commit is contained in:
Ein Anderssono
2026-06-11 21:12:47 +02:00
parent b460c68641
commit 85eaa3ea37
14 changed files with 274 additions and 651 deletions
+127 -54
View File
@@ -4,6 +4,7 @@ import (
"fmt"
"io"
"strings"
"sync"
"gitea.k3s.k0.nu/tools/photocli/internal/photos"
)
@@ -89,20 +90,20 @@ func cmdAlbums(stdout, stderr io.Writer, bridge photos.Bridge) int {
}
func resolveAlbumID(bridge photos.Bridge, idOrName string) (string, error) {
_, _, err := bridge.ListAssets(idOrName)
if err == nil {
return idOrName, nil
}
albums, listErr := bridge.ListAlbums()
if listErr != nil {
return idOrName, err
return idOrName, listErr
}
for _, a := range albums {
if a.Title == idOrName {
return a.ID, nil
}
}
return idOrName, err
_, _, err := bridge.ListAssets(idOrName)
if err == nil {
return idOrName, nil
}
return idOrName, fmt.Errorf("album not found: %s", idOrName)
}
func cmdPhotos(args []string, stdout, stderr io.Writer, bridge photos.Bridge) int {
@@ -125,11 +126,7 @@ func cmdPhotos(args []string, stdout, stderr io.Writer, bridge photos.Bridge) in
return 1
}
for _, a := range assets {
cloud := "local"
if a.Cloud {
cloud = "cloud"
}
fmt.Fprintf(stdout, "%s\t%s\t%s\n", a.ID, a.Filename, cloud)
fmt.Fprintf(stdout, "%s\t%s\t%s\n", a.ID, a.Filename, a.Cloud)
}
return 0
}
@@ -188,25 +185,7 @@ func cmdExport(args []string, stdout, stderr io.Writer, bridge photos.Bridge) in
return 1
}
exported := 0
failed := 0
for i, a := range assets {
var result photos.ExportResult
var exportErr error
if originals {
result, exportErr = bridge.ExportOriginal(a.ID, outDir, i)
} else {
result, exportErr = bridge.ExportPreview(a.ID, outDir, size, i)
}
progressBar(stderr, exported+failed+1, total, result.Filename, result.Size, result.Cloud)
if exportErr != nil {
failed++
continue
}
exported++
}
exported, failed := exportAssets(assets, outDir, size, originals, total, stderr, bridge, "")
if exported == 0 && failed > 0 {
fmt.Fprintf(stderr, "\nerror: all exports failed\n")
@@ -214,10 +193,14 @@ func cmdExport(args []string, stdout, stderr io.Writer, bridge photos.Bridge) in
}
if originals {
fmt.Fprintf(stderr, "\nexported %d original files to %s\n", exported, outDir)
fmt.Fprintf(stderr, "\nexported %d original files to %s", exported, outDir)
} else {
fmt.Fprintf(stderr, "\nexported %d photos to %s\n", exported, outDir)
fmt.Fprintf(stderr, "\nexported %d photos to %s", exported, outDir)
}
if failed > 0 {
fmt.Fprintf(stderr, " (%d failed)", failed)
}
fmt.Fprintln(stderr)
return 0
}
@@ -250,60 +233,150 @@ func cmdBackupAll(args []string, stdout, stderr io.Writer, bridge photos.Bridge)
}
albumCount := countAlbums(nodes)
totalAssets, grandTotal, err := backupTree(nodes, outDir, size, originals, stderr, bridge)
totalAssets, _, failed, err := backupTree(nodes, outDir, size, originals, stderr, bridge)
if err != nil {
fmt.Fprintf(stderr, "error: %v\n", err)
return 1
}
if originals {
fmt.Fprintf(stderr, "\nexported %d original files across %d albums to %s\n", totalAssets, albumCount, outDir)
fmt.Fprintf(stderr, "\nexported %d original files across %d albums to %s", totalAssets, albumCount, outDir)
} else {
fmt.Fprintf(stderr, "\nexported %d preview files across %d albums to %s\n", totalAssets, albumCount, outDir)
fmt.Fprintf(stderr, "\nexported %d preview files across %d albums to %s", totalAssets, albumCount, outDir)
}
_ = grandTotal
if failed > 0 {
fmt.Fprintf(stderr, " (%d failed)", failed)
}
fmt.Fprintln(stderr)
return 0
}
func backupTree(nodes []photos.CollectionNode, outDir string, targetSize int, originals bool, stderr io.Writer, bridge photos.Bridge) (int, int, error) {
func backupTree(nodes []photos.CollectionNode, outDir string, targetSize int, originals bool, stderr io.Writer, bridge photos.Bridge) (int, int, int, error) {
exported := 0
total := 0
failed := 0
for _, node := range nodes {
path := outDir + "/" + sanitizePathComponent(node.Name)
if node.Kind == "folder" {
n, t, err := backupTree(node.Children, path, targetSize, originals, stderr, bridge)
n, t, f, err := backupTree(node.Children, path, targetSize, originals, stderr, bridge)
if err != nil {
return exported, total, err
return exported, total, failed, err
}
exported += n
total += t
failed += f
continue
}
if node.Kind == "album" && node.ID != "" {
assets, assetTotal, err := bridge.ListAssets(node.ID)
if err != nil {
fmt.Fprintf(stderr, "\n skipped album %s: %v\n", node.Name, err)
continue
}
total += assetTotal
for i, a := range assets {
var result photos.ExportResult
var exportErr error
if originals {
result, exportErr = bridge.ExportOriginal(a.ID, path, i)
} else {
result, exportErr = bridge.ExportPreview(a.ID, path, targetSize, i)
}
progressBar(stderr, exported+1, total, path+"/"+result.Filename, result.Size, result.Cloud)
if exportErr != nil {
continue
}
exported++
}
n, f := exportAssets(assets, path, targetSize, originals, total, stderr, bridge, path+"/")
exported += n
failed += f
}
}
return exported, total, nil
return exported, total, failed, nil
}
type exportJob struct {
asset photos.Asset
index int
}
type exportResult struct {
index int
result photos.ExportResult
err error
}
func exportAssets(assets []photos.Asset, outDir string, targetSize int, originals bool, total int, stderr io.Writer, bridge photos.Bridge, dirPrefix string) (int, int) {
if len(assets) == 0 {
return 0, 0
}
if len(assets) < 4 {
return exportAssetsSerial(assets, outDir, targetSize, originals, total, stderr, bridge, dirPrefix)
}
return exportAssetsParallel(assets, outDir, targetSize, originals, total, stderr, bridge, 3, dirPrefix)
}
func exportAssetsSerial(assets []photos.Asset, outDir string, targetSize int, originals bool, total int, stderr io.Writer, bridge photos.Bridge, dirPrefix string) (int, int) {
exported := 0
failed := 0
for i, a := range assets {
result, exportErr := exportOne(bridge, a, outDir, targetSize, originals, i)
progressBar(stderr, exported+failed+1, total, dirPrefix+result.Filename, result.Size, result.Cloud)
if exportErr != nil {
fmt.Fprintf(stderr, "\n failed: %s: %v\n", a.Filename, exportErr)
failed++
continue
}
exported++
}
return exported, failed
}
func exportAssetsParallel(assets []photos.Asset, outDir string, targetSize int, originals bool, total int, stderr io.Writer, bridge photos.Bridge, workers int, dirPrefix string) (int, int) {
jobs := make(chan exportJob, len(assets))
results := make(chan exportResult, len(assets))
var wg sync.WaitGroup
for w := 0; w < workers; w++ {
wg.Add(1)
go func() {
defer wg.Done()
for job := range jobs {
result, exportErr := exportOne(bridge, job.asset, outDir, targetSize, originals, job.index)
results <- exportResult{index: job.index, result: result, err: exportErr}
}
}()
}
go func() {
for i, a := range assets {
jobs <- exportJob{asset: a, index: i}
}
close(jobs)
}()
go func() {
wg.Wait()
close(results)
}()
ordered := make([]exportResult, len(assets))
for r := range results {
ordered[r.index] = r
}
exported := 0
failed := 0
for i, a := range assets {
r := ordered[i]
progressBar(stderr, exported+failed+1, total, dirPrefix+r.result.Filename, r.result.Size, r.result.Cloud)
if r.err != nil {
fmt.Fprintf(stderr, "\n failed: %s: %v\n", a.Filename, r.err)
failed++
continue
}
exported++
}
return exported, failed
}
func exportOne(bridge photos.Bridge, a photos.Asset, outDir string, targetSize int, originals bool, index int) (photos.ExportResult, error) {
if originals {
return bridge.ExportOriginal(a.ID, outDir, index)
}
return bridge.ExportPreview(a.ID, outDir, targetSize, index)
}
// Must stay in sync with sanitized_path_component in bridge/photokit_bridge.m
func sanitizePathComponent(name string) string {
s := strings.TrimSpace(name)
if s == "" {
+5 -5
View File
@@ -3,6 +3,7 @@ package main
import (
"os"
"os/signal"
"sync/atomic"
"syscall"
"gitea.k3s.k0.nu/tools/photocli/internal/photos"
@@ -15,21 +16,20 @@ func main() {
signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
done := make(chan struct{})
var rc int
var rc atomic.Int32
go func() {
rc = run(os.Args[1:], os.Stdout, os.Stderr, photos.DefaultBridge)
rc.Store(int32(run(os.Args[1:], os.Stdout, os.Stderr, photos.DefaultBridge)))
close(done)
}()
select {
case <-done:
case sig := <-sigCh:
case <-sigCh:
photos.DefaultBridge.Cancel()
os.Stderr.Write([]byte("\nreceived signal, finishing current file...\n"))
<-done
_ = sig
}
os.Exit(rc)
os.Exit(int(rc.Load()))
}
+90 -42
View File
@@ -20,18 +20,9 @@ type mockBridge struct {
assetsByAlbum map[string][]photos.Asset
tree []photos.CollectionNode
treeErr error
exportN int
exportErr error
exportOrigN int
exportOrigErr error
exportPreviewFn func(string, string, int) (int, error)
exportOrigFn func(string, string) (int, error)
backupAllN int
backupAllErr error
backupAllFn func(string, int, bool) (int, error)
exportPreviewFn func(string, string, int, int) (photos.ExportResult, error)
exportOrigFn func(string, string, int) (photos.ExportResult, error)
cancelled bool
exportPreviewFn2 func(string, string, int, int) (photos.ExportResult, error)
exportOrigFn2 func(string, string, int) (photos.ExportResult, error)
}
func (m *mockBridge) RequestAccess() error { return m.accessErr }
@@ -46,36 +37,18 @@ func (m *mockBridge) ListAssets(albumID string) ([]photos.Asset, int, error) {
return m.assets, len(m.assets), m.assetsErr
}
func (m *mockBridge) ListTree() ([]photos.CollectionNode, error) { return m.tree, m.treeErr }
func (m *mockBridge) ExportAlbumPreviews(albumID, out string, size int) (int, error) {
if m.exportPreviewFn != nil {
return m.exportPreviewFn(albumID, out, size)
}
return m.exportN, m.exportErr
}
func (m *mockBridge) ExportAlbumOriginals(albumID, out string) (int, error) {
if m.exportOrigFn != nil {
return m.exportOrigFn(albumID, out)
}
return m.exportOrigN, m.exportOrigErr
}
func (m *mockBridge) ExportPreview(assetID, out string, targetSize, index int) (photos.ExportResult, error) {
if m.exportPreviewFn2 != nil {
return m.exportPreviewFn2(assetID, out, targetSize, index)
if m.exportPreviewFn != nil {
return m.exportPreviewFn(assetID, out, targetSize, index)
}
return photos.ExportResult{Filename: "0000_test.jpg", Size: 1024, Cloud: "local"}, nil
}
func (m *mockBridge) ExportOriginal(assetID, out string, index int) (photos.ExportResult, error) {
if m.exportOrigFn2 != nil {
return m.exportOrigFn2(assetID, out, index)
if m.exportOrigFn != nil {
return m.exportOrigFn(assetID, out, index)
}
return photos.ExportResult{Filename: "test.jpg", Size: 2048, Cloud: "cloud"}, nil
}
func (m *mockBridge) BackupAll(out string, size int, originals bool) (int, error) {
if m.backupAllFn != nil {
return m.backupAllFn(out, size, originals)
}
return m.backupAllN, m.backupAllErr
}
func (m *mockBridge) Cancel() { m.cancelled = true }
func runWith(args []string, b photos.Bridge) (string, string, int) {
@@ -187,7 +160,7 @@ func TestCmdPhotosMissingAlbumID(t *testing.T) {
func TestCmdPhotosSuccess(t *testing.T) {
b := &mockBridge{
assets: []photos.Asset{{ID: "as1", Filename: "IMG_0001.JPG"}, {ID: "as2", Filename: "IMG_0002.JPG", Cloud: true}},
assets: []photos.Asset{{ID: "as1", Filename: "IMG_0001.JPG", Cloud: "local"}, {ID: "as2", Filename: "IMG_0002.JPG", Cloud: "cloud"}},
}
out, _, rc := runWith([]string{"photos", "--album-id", "x"}, b)
if rc != 0 {
@@ -211,7 +184,7 @@ func TestCmdPhotosAuthDenied(t *testing.T) {
}
func TestCmdPhotosBridgeError(t *testing.T) {
b := &mockBridge{assetsErr: fmt.Errorf("fail")}
b := &mockBridge{albumsErr: fmt.Errorf("fail")}
_, stderr, rc := runWith([]string{"photos", "--album-id", "x"}, b)
if rc != 1 {
t.Errorf("rc = %d", rc)
@@ -273,6 +246,9 @@ func TestCmdBackupAllPreviewSuccess(t *testing.T) {
if !strings.Contains(stderr, "exported 2 preview files across 2 albums to /backup") {
t.Errorf("stderr = %q", stderr)
}
if strings.Contains(stderr, "failed") {
t.Errorf("unexpected failed count in stderr = %q", stderr)
}
}
func TestCmdBackupAllOriginalsSuccess(t *testing.T) {
@@ -289,6 +265,9 @@ func TestCmdBackupAllOriginalsSuccess(t *testing.T) {
if !strings.Contains(stderr, "exported 3 original files across 1 albums to /backup") {
t.Errorf("stderr = %q", stderr)
}
if strings.Contains(stderr, "failed") {
t.Errorf("unexpected failed count in stderr = %q", stderr)
}
}
func TestCmdBackupAllMissingOutDir(t *testing.T) {
@@ -329,7 +308,7 @@ func TestCmdBackupAllExportError(t *testing.T) {
assetsByAlbum: map[string][]photos.Asset{
"a1": {{ID: "as1", Filename: "img.jpg"}},
},
exportPreviewFn2: func(string, string, int, int) (photos.ExportResult, error) {
exportPreviewFn: func(string, string, int, int) (photos.ExportResult, error) {
return photos.ExportResult{}, fmt.Errorf("disk full")
},
}
@@ -337,7 +316,12 @@ func TestCmdBackupAllExportError(t *testing.T) {
if rc != 0 {
t.Fatalf("rc = %d, stderr = %q", rc, stderr)
}
_ = stderr
if !strings.Contains(stderr, "failed: img.jpg: disk full") {
t.Errorf("stderr should contain failure detail, got: %q", stderr)
}
if !strings.Contains(stderr, "(1 failed)") {
t.Errorf("stderr should contain failed count, got: %q", stderr)
}
}
func TestCmdExportMissingAlbumID(t *testing.T) {
@@ -420,7 +404,7 @@ func TestCmdExportAuthDenied(t *testing.T) {
func TestCmdExportBridgeError(t *testing.T) {
b := &mockBridge{
assets: []photos.Asset{{ID: "a1", Filename: "img.jpg"}},
exportPreviewFn2: func(string, string, int, int) (photos.ExportResult, error) {
exportPreviewFn: func(string, string, int, int) (photos.ExportResult, error) {
return photos.ExportResult{}, fmt.Errorf("disk full")
},
}
@@ -431,6 +415,9 @@ func TestCmdExportBridgeError(t *testing.T) {
if !strings.Contains(stderr, "all exports failed") {
t.Errorf("stderr = %q", stderr)
}
if !strings.Contains(stderr, "failed: img.jpg: disk full") {
t.Errorf("stderr should contain failure detail, got: %q", stderr)
}
}
func TestCmdExportOriginalsSuccess(t *testing.T) {
@@ -462,7 +449,7 @@ func TestCmdExportOriginalsIgnoresSizeValidation(t *testing.T) {
func TestCmdExportOriginalsBridgeError(t *testing.T) {
b := &mockBridge{
assets: []photos.Asset{{ID: "a1", Filename: "img.jpg"}},
exportOrigFn2: func(string, string, int) (photos.ExportResult, error) {
exportOrigFn: func(string, string, int) (photos.ExportResult, error) {
return photos.ExportResult{}, fmt.Errorf("copy failed")
},
}
@@ -473,6 +460,9 @@ func TestCmdExportOriginalsBridgeError(t *testing.T) {
if !strings.Contains(stderr, "all exports failed") {
t.Errorf("stderr = %q", stderr)
}
if !strings.Contains(stderr, "failed: img.jpg: copy failed") {
t.Errorf("stderr should contain failure detail, got: %q", stderr)
}
}
func TestFlagVal(t *testing.T) {
@@ -527,7 +517,7 @@ func TestFlagValEmptyArgs(t *testing.T) {
func TestResolveAlbumIDDirectMatch(t *testing.T) {
b := &mockBridge{
assetsByAlbum: map[string][]photos.Asset{
"ABC/L0/001": {{ID: "a1", Filename: "img.jpg"}},
"ABC/L0/001": {{ID: "a1", Filename: "img.jpg", Cloud: "local"}},
},
}
id, err := resolveAlbumID(b, "ABC/L0/001")
@@ -546,7 +536,7 @@ func TestResolveAlbumIDByName(t *testing.T) {
{ID: "DEF/L0/001", Title: "Work"},
},
assetsByAlbum: map[string][]photos.Asset{
"ABC/L0/001": {{ID: "a1", Filename: "img.jpg"}},
"ABC/L0/001": {{ID: "a1", Filename: "img.jpg", Cloud: "local"}},
},
}
id, err := resolveAlbumID(b, "DnD")
@@ -588,7 +578,7 @@ func TestCmdPhotosResolvesAlbumName(t *testing.T) {
{ID: "ABC/L0/001", Title: "DnD"},
},
assetsByAlbum: map[string][]photos.Asset{
"ABC/L0/001": {{ID: "a1", Filename: "img.jpg"}},
"ABC/L0/001": {{ID: "a1", Filename: "img.jpg", Cloud: "local"}},
},
}
out, stderr, rc := runWith([]string{"photos", "--album-id", "DnD"}, b)
@@ -635,3 +625,61 @@ func TestCmdExportOriginalsResolvesAlbumName(t *testing.T) {
t.Errorf("stderr = %q", stderr)
}
}
func TestCmdExportPartialFailure(t *testing.T) {
call := 0
b := &mockBridge{
albums: []photos.Album{{ID: "a1", Title: "Album"}},
assetsByAlbum: map[string][]photos.Asset{
"a1": {{ID: "x1", Filename: "ok.jpg", Cloud: "local"}, {ID: "x2", Filename: "bad.jpg", Cloud: "local"}, {ID: "x3", Filename: "ok2.jpg", Cloud: "local"}},
},
exportPreviewFn: func(string, string, int, int) (photos.ExportResult, error) {
call++
if call == 2 {
return photos.ExportResult{}, fmt.Errorf("disk full")
}
return photos.ExportResult{Filename: "ok.jpg", Size: 1024, Cloud: "local"}, nil
},
}
_, stderr, rc := runWith([]string{"export", "--album-id", "Album", "--out", "/tmp"}, b)
if rc != 0 {
t.Errorf("rc = %d, stderr = %q", rc, stderr)
}
if !strings.Contains(stderr, "failed: bad.jpg: disk full") {
t.Errorf("stderr should contain failure detail, got: %q", stderr)
}
if !strings.Contains(stderr, "(1 failed)") {
t.Errorf("stderr should contain failed count, got: %q", stderr)
}
if !strings.Contains(stderr, "exported 2 photos") {
t.Errorf("stderr should contain export count, got: %q", stderr)
}
}
func TestCmdBackupAllSkippedAlbum(t *testing.T) {
b := &mockBridge{
tree: []photos.CollectionNode{{ID: "bad-album", Name: "Broken", Kind: "album"}},
assetsByAlbum: map[string][]photos.Asset{},
}
_, stderr, rc := runWith([]string{"backup-all", "--out", "/backup"}, b)
if rc != 0 {
t.Fatalf("rc = %d, stderr = %q", rc, stderr)
}
if !strings.Contains(stderr, "skipped album Broken") {
t.Errorf("stderr should contain skipped album, got: %q", stderr)
}
}
func TestResolveAlbumIDNotFoundMessage(t *testing.T) {
b := &mockBridge{
albums: []photos.Album{{ID: "x", Title: "Other"}},
assetsByAlbum: map[string][]photos.Asset{},
}
_, err := resolveAlbumID(b, "Nonexistent")
if err == nil {
t.Fatal("expected error")
}
if !strings.Contains(err.Error(), "album not found: Nonexistent") {
t.Errorf("err = %q", err.Error())
}
}