From 9faf6b33d0d2140c992c464640ebb5337a7df54d Mon Sep 17 00:00:00 2001
From: Jarek Kowalski <jaak@jkowalski.net>
Date: Sat, 12 Sep 2020 22:36:47 -0700
Subject: [PATCH] cli: fixed snapshot delete to support deleting file (not
 directory) snapshots by object ID (#613)

---
 cli/command_snapshot_delete.go | 20 ++++++++------------
 snapshot/manager.go            |  7 +++++++
 snapshot/snapshot_test.go      |  5 +++++
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/cli/command_snapshot_delete.go b/cli/command_snapshot_delete.go
index 39996e806..b9d724171 100644
--- a/cli/command_snapshot_delete.go
+++ b/cli/command_snapshot_delete.go
@@ -3,7 +3,6 @@ package cli
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/pkg/errors"
 
@@ -21,19 +20,16 @@ var (
 
 func runDeleteCommand(ctx context.Context, rep repo.Repository) error {
 	for _, id := range *snapshotDeleteIDs {
-		if strings.HasPrefix(id, "k") {
-			if err := deleteSnapshotsByRootObjectID(ctx, rep, object.ID(id)); err != nil {
-				return errors.Wrapf(err, "error deleting snapshots by root ID %v", id)
-			}
-		} else {
-			m, err := snapshot.LoadSnapshot(ctx, rep, manifest.ID(id))
-			if err != nil {
-				return errors.Wrapf(err, "error loading snapshot %v", id)
-			}
-
-			if err := deleteSnapshot(ctx, rep, m); err != nil {
+		m, err := snapshot.LoadSnapshot(ctx, rep, manifest.ID(id))
+		if err == nil {
+			// snapshot found by manifest ID, delete it directly.
+			if err = deleteSnapshot(ctx, rep, m); err != nil {
 				return errors.Wrapf(err, "error deleting %v", id)
 			}
+		} else if !errors.Is(err, snapshot.ErrSnapshotNotFound) {
+			return errors.Wrapf(err, "error loading snapshot %v", id)
+		} else if err := deleteSnapshotsByRootObjectID(ctx, rep, object.ID(id)); err != nil {
+			return errors.Wrapf(err, "error deleting snapshots by root ID %v", id)
 		}
 	}
 
diff --git a/snapshot/manager.go b/snapshot/manager.go
index 12b1d346f..c5652fefc 100644
--- a/snapshot/manager.go
+++ b/snapshot/manager.go
@@ -14,6 +14,9 @@ import (
 // ManifestType is the value of the "type" label for snapshot manifests.
 const ManifestType = "snapshot"
 
+// ErrSnapshotNotFound is returned when a snapshot is not found.
+var ErrSnapshotNotFound = errors.Errorf("snapshot not found")
+
 const (
 	typeKey = manifest.TypeLabelKey
 
@@ -73,6 +76,10 @@ func LoadSnapshot(ctx context.Context, rep repo.Repository, manifestID manifest.
 
 	em, err := rep.GetManifest(ctx, manifestID, sm)
 	if err != nil {
+		if errors.Is(err, manifest.ErrNotFound) {
+			return nil, ErrSnapshotNotFound
+		}
+
 		return nil, errors.Wrap(err, "unable to find manifest entries")
 	}
 
diff --git a/snapshot/snapshot_test.go b/snapshot/snapshot_test.go
index b27f9c0ea..37a78dbf7 100644
--- a/snapshot/snapshot_test.go
+++ b/snapshot/snapshot_test.go
@@ -1,6 +1,7 @@
 package snapshot_test
 
 import (
+	"errors"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -32,6 +33,10 @@ func TestSnapshotsAPI(t *testing.T) {
 		Path:     "/some/other/path",
 	}
 
+	if _, err := snapshot.LoadSnapshot(ctx, env.Repository, "no-such-manifest-id"); !errors.Is(err, snapshot.ErrSnapshotNotFound) {
+		t.Errorf("unexpected error when deleting snapshot for manifest that does not exist: %v", err)
+	}
+
 	verifySnapshotManifestIDs(t, env.Repository, nil, nil)
 	verifySnapshotManifestIDs(t, env.Repository, &src1, nil)
 	verifySnapshotManifestIDs(t, env.Repository, &src2, nil)