From 27baed919d45d1251945bdb026492a6058daa5ae Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= <mitr@redhat.com>
Date: Mon, 2 Dec 2024 20:31:25 +0100
Subject: [PATCH] Fix handling of errorShouldDisplayUsage
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Exit with a non-zero exit code
- Actually report the error we detected
- Write usage to stderr

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 cmd/skopeo/copy_test.go      | 18 ++++++++++++++++++
 cmd/skopeo/list_tags_test.go | 14 ++++++++++++++
 cmd/skopeo/sync_test.go      | 15 +++++++++++++++
 cmd/skopeo/utils.go          |  5 +++--
 4 files changed, 50 insertions(+), 2 deletions(-)
 create mode 100644 cmd/skopeo/copy_test.go

diff --git a/cmd/skopeo/copy_test.go b/cmd/skopeo/copy_test.go
new file mode 100644
index 0000000000..68248dc889
--- /dev/null
+++ b/cmd/skopeo/copy_test.go
@@ -0,0 +1,18 @@
+package main
+
+import "testing"
+
+func TestCopy(t *testing.T) {
+	// Invalid command-line arguments
+	for _, args := range [][]string{
+		{},
+		{"a1"},
+		{"a1", "a2", "a3"},
+	} {
+		out, err := runSkopeo(append([]string{"--insecure-policy", "copy"}, args...)...)
+		assertTestFailed(t, out, err, "Exactly two arguments expected")
+	}
+
+	// FIXME: Much more test coverage
+	// Actual feature tests exist in integration and systemtest
+}
diff --git a/cmd/skopeo/list_tags_test.go b/cmd/skopeo/list_tags_test.go
index 2b3cc2d5b4..b1707f7315 100644
--- a/cmd/skopeo/list_tags_test.go
+++ b/cmd/skopeo/list_tags_test.go
@@ -54,3 +54,17 @@ func TestDockerRepositoryReferenceParserDrift(t *testing.T) {
 		}
 	}
 }
+
+func TestListTags(t *testing.T) {
+	// Invalid command-line arguments
+	for _, args := range [][]string{
+		{},
+		{"a1", "a2"},
+	} {
+		out, err := runSkopeo(append([]string{"list-tags"}, args...)...)
+		assertTestFailed(t, out, err, "Exactly one non-option argument expected")
+	}
+
+	// FIXME: Much more test coverage
+	// Actual feature tests exist in systemtest
+}
diff --git a/cmd/skopeo/sync_test.go b/cmd/skopeo/sync_test.go
index 734abfedd1..17ba4c2f6f 100644
--- a/cmd/skopeo/sync_test.go
+++ b/cmd/skopeo/sync_test.go
@@ -44,3 +44,18 @@ func TestTLSVerifyConfig(t *testing.T) {
 	err := yaml.Unmarshal([]byte(`tls-verify: "not a valid bool"`), &config)
 	assert.Error(t, err)
 }
+
+func TestSync(t *testing.T) {
+	// Invalid command-line arguments
+	for _, args := range [][]string{
+		{},
+		{"a1"},
+		{"a1", "a2", "a3"},
+	} {
+		out, err := runSkopeo(append([]string{"sync"}, args...)...)
+		assertTestFailed(t, out, err, "Exactly two arguments expected")
+	}
+
+	// FIXME: Much more test coverage
+	// Actual feature tests exist in integration and systemtest
+}
diff --git a/cmd/skopeo/utils.go b/cmd/skopeo/utils.go
index 75a2fa823b..fbf4e62836 100644
--- a/cmd/skopeo/utils.go
+++ b/cmd/skopeo/utils.go
@@ -26,7 +26,7 @@ import (
 	"golang.org/x/term"
 )
 
-// errorShouldDisplayUsage is a subtype of error used by command handlers to indicate that cli.ShowSubcommandHelp should be called.
+// errorShouldDisplayUsage is a subtype of error used by command handlers to indicate that the command’s help should be included.
 type errorShouldDisplayUsage struct {
 	error
 }
@@ -62,7 +62,8 @@ func commandAction(handler func(args []string, stdout io.Writer) error) func(cmd
 		err := handler(args, c.OutOrStdout())
 		var shouldDisplayUsage errorShouldDisplayUsage
 		if errors.As(err, &shouldDisplayUsage) {
-			return c.Help()
+			c.SetOut(c.ErrOrStderr()) // This mutates c, but we are failing anyway.
+			_ = c.Help()              // Even if this failed, we prefer to report the original error
 		}
 		return err
 	}