From 37a312e50532a84d33087f97695f925984642614 Mon Sep 17 00:00:00 2001
From: Enrico204 <enrico204@gmail.com>
Date: Tue, 29 Aug 2023 09:19:17 +0200
Subject: [PATCH] restic-from-command: use standard behavior when no output and
 exit code 0 from command

The behavior of the new option should reflect the behavior of normal backups: when the command exit code is zero and there is no output in the stdout, emit a warning but create the snapshot. This commit fixes the integration tests and the ReadCloserCommand struct.
---
 cmd/restic/cmd_backup_integration_test.go | 29 +++++++++---
 internal/fs/fs_reader_command.go          | 55 ++++++++++++++++-------
 2 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go
index 550834972..c60e9c543 100644
--- a/cmd/restic/cmd_backup_integration_test.go
+++ b/cmd/restic/cmd_backup_integration_test.go
@@ -579,13 +579,13 @@ func TestStdinFromCommand(t *testing.T) {
 		StdinFilename: "stdin",
 	}
 
-	testRunBackup(t, filepath.Dir(env.testdata), []string{"ls"}, opts, env.gopts)
+	testRunBackup(t, filepath.Dir(env.testdata), []string{"python", "-c", "import sys; print('something'); sys.exit(0)"}, opts, env.gopts)
 	testListSnapshots(t, env.gopts, 1)
 
 	testRunCheck(t, env.gopts)
 }
 
-func TestStdinFromCommandFailNoOutput(t *testing.T) {
+func TestStdinFromCommandNoOutput(t *testing.T) {
 	env, cleanup := withTestEnvironment(t)
 	defer cleanup()
 
@@ -595,10 +595,9 @@ func TestStdinFromCommandFailNoOutput(t *testing.T) {
 		StdinFilename: "stdin",
 	}
 
-	err := testRunBackupAssumeFailure(t, filepath.Dir(env.testdata), []string{"python", "-c", "import sys; sys.exit(1)"}, opts, env.gopts)
-	rtest.Assert(t, err != nil, "Expected error while backing up")
-
-	testListSnapshots(t, env.gopts, 0)
+	err := testRunBackupAssumeFailure(t, filepath.Dir(env.testdata), []string{"python", "-c", "import sys; sys.exit(0)"}, opts, env.gopts)
+	rtest.Assert(t, err != nil && err.Error() == "at least one source file could not be read", "No data error expected")
+	testListSnapshots(t, env.gopts, 1)
 
 	testRunCheck(t, env.gopts)
 }
@@ -620,3 +619,21 @@ func TestStdinFromCommandFailExitCode(t *testing.T) {
 
 	testRunCheck(t, env.gopts)
 }
+
+func TestStdinFromCommandFailNoOutputAndExitCode(t *testing.T) {
+	env, cleanup := withTestEnvironment(t)
+	defer cleanup()
+
+	testSetupBackupData(t, env)
+	opts := BackupOptions{
+		StdinCommand:  true,
+		StdinFilename: "stdin",
+	}
+
+	err := testRunBackupAssumeFailure(t, filepath.Dir(env.testdata), []string{"python", "-c", "import sys; sys.exit(1)"}, opts, env.gopts)
+	rtest.Assert(t, err != nil, "Expected error while backing up")
+
+	testListSnapshots(t, env.gopts, 0)
+
+	testRunCheck(t, env.gopts)
+}
diff --git a/internal/fs/fs_reader_command.go b/internal/fs/fs_reader_command.go
index e456ee965..bc377ab96 100644
--- a/internal/fs/fs_reader_command.go
+++ b/internal/fs/fs_reader_command.go
@@ -13,31 +13,54 @@ type ReadCloserCommand struct {
 	Cmd    *exec.Cmd
 	Stdout io.ReadCloser
 
-	bytesRead bool
+	// We should call exec.Wait() once. waitHandled is taking care of storing
+	// whether we already called that function in Read() to avoid calling it
+	// again in Close().
+	waitHandled bool
+
+	// alreadyClosedReadErr is the error that we should return if we try to
+	// read the pipe again after closing. This works around a Read() call that
+	// is issued after a previous Read() with `io.EOF` (but some bytes were
+	// read in the past).
+	alreadyClosedReadErr error
 }
 
 // Read populate the array with data from the process stdout.
 func (fp *ReadCloserCommand) Read(p []byte) (int, error) {
-	// We may encounter two different error conditions here:
-	// - EOF with no bytes read: the program terminated prematurely, so we send
-	//   a fatal error to cancel the snapshot;
-	// - an error that is not EOF: something bad happened, we need to abort the
-	//   snapshot.
-	b, err := fp.Stdout.Read(p)
-	if b == 0 && errors.Is(err, io.EOF) && !fp.bytesRead {
-		// The command terminated with no output at all. Raise a fatal error.
-		return 0, errors.Fatalf("command terminated with no output")
-	} else if err != nil && !errors.Is(err, io.EOF) {
-		// The command terminated with an error that is not EOF. Raise a fatal
-		// error.
-		return 0, errors.Fatal(err.Error())
-	} else if b > 0 {
-		fp.bytesRead = true
+	if fp.alreadyClosedReadErr != nil {
+		return 0, fp.alreadyClosedReadErr
 	}
+	b, err := fp.Stdout.Read(p)
+
+	// If the error is io.EOF, the program terminated. We need to check the
+	// exit code here because, if the program terminated with no output, the
+	// error in `Close()` is ignored.
+	if errors.Is(err, io.EOF) {
+		// Check if the command terminated successfully. If not, return the
+		// error.
+		fp.waitHandled = true
+		errw := fp.Cmd.Wait()
+		if errw != nil {
+			// If we have information about the exit code, let's use it in the
+			// error message. Otherwise, send the error message along.
+			// In any case, use a fatal error to abort the snapshot.
+			var err2 *exec.ExitError
+			if errors.As(errw, &err2) {
+				err = errors.Fatalf("command terminated with exit code %d", err2.ExitCode())
+			} else {
+				err = errors.Fatal(errw.Error())
+			}
+		}
+	}
+	fp.alreadyClosedReadErr = err
 	return b, err
 }
 
 func (fp *ReadCloserCommand) Close() error {
+	if fp.waitHandled {
+		return nil
+	}
+
 	// No need to close fp.Stdout as Wait() closes all pipes.
 	err := fp.Cmd.Wait()
 	if err != nil {