hook: Propagate error's via `ProcReceiveHandler`

Currently the `Close()` function of the `ProcReceiveHandler` only
provides information on whether the handler's job is completed or not.
But it would be useful for the handler to also propagate errors, this
could be used by the ProcReceive server to return the appropriate exit
code to git-receive-pack(1).

So let's convert the `Close()` function to accept an error and propagate
this error via the `doneCh`.

4 files changed, 73 insertions, 24 deletions

diff --git a/internal/gitaly/hook/procreceive_handler.go b/internal/gitaly/hook/procreceive_handler.go
index 943447442..26c905a1f 100644
--- a/internal/gitaly/hook/procreceive_handler.go
+++ b/internal/gitaly/hook/procreceive_handler.go
@@ -12,7 +12,7 @@ import (
 
 type procReceiveHandler struct {
 	stdout        io.Writer
-	doneCh        chan<- struct{}
+	doneCh        chan<- error
 	updates       []ReferenceUpdate
 	transactionID storage.TransactionID
 	atomic        bool
@@ -28,7 +28,7 @@ type procReceiveHandler struct {
 //
 // The handler is transmitted to RPCs which executed git-receive-pack(1), so they
 // can accept or reject individual reference updates.
-func NewProcReceiveHandler(env []string, stdin io.Reader, stdout io.Writer) (ProcReceiveHandler, <-chan struct{}, error) {
+func NewProcReceiveHandler(env []string, stdin io.Reader, stdout io.Writer) (ProcReceiveHandler, <-chan error, error) {
 	payload, err := git.HooksPayloadFromEnv(env)
 	if err != nil {
 		return nil, nil, fmt.Errorf("extracting hooks payload: %w", err)
@@ -98,7 +98,7 @@ func NewProcReceiveHandler(env []string, stdin io.Reader, stdout io.Writer) (Pro
 		return nil, nil, fmt.Errorf("parsing stdin: %w", err)
 	}
 
-	ch := make(chan struct{})
+	ch := make(chan error, 1)
 
 	return &procReceiveHandler{
 		transactionID: payload.TransactionID,
@@ -143,9 +143,18 @@ func (h *procReceiveHandler) RejectUpdate(referenceName git.ReferenceName, reaso
 	return nil
 }
 
-// Close must be called to clean up the proc-receive hook.
-func (h *procReceiveHandler) Close() error {
-	defer close(h.doneCh)
+// Close must be called to clean up the proc-receive hook. If the user
+// of the handler encounters an error, it should be transferred to the
+// hook too.
+func (h *procReceiveHandler) Close(rpcErr error) error {
+	defer func() {
+		h.doneCh <- rpcErr
+	}()
+
+	// When we have an error, there is no need to flush.
+	if rpcErr != nil {
+		return nil
+	}
 
 	if err := pktline.WriteFlush(h.stdout); err != nil {
 		return fmt.Errorf("flushing updates: %w", err)
diff --git a/internal/gitaly/hook/procreceive_handler_test.go b/internal/gitaly/hook/procreceive_handler_test.go
index c038adfc0..e6fceafa7 100644
--- a/internal/gitaly/hook/procreceive_handler_test.go
+++ b/internal/gitaly/hook/procreceive_handler_test.go
@@ -46,14 +46,15 @@ func TestProcReceiveHandler(t *testing.T) {
 	require.NoError(t, err)
 
 	type setupData struct {
-		env             []string
-		ctx             context.Context
-		stdin           string
-		expectedErr     error
-		expectedStdout  string
-		expectedUpdates []ReferenceUpdate
-		expectedAtomic  bool
-		handlerSteps    func(handler ProcReceiveHandler) error
+		env              []string
+		ctx              context.Context
+		stdin            string
+		expectedErr      error
+		expectedCloseErr error
+		expectedStdout   string
+		expectedUpdates  []ReferenceUpdate
+		expectedAtomic   bool
+		handlerSteps     func(handler ProcReceiveHandler) error
 	}
 
 	for _, tc := range []struct {
@@ -124,7 +125,7 @@ func TestProcReceiveHandler(t *testing.T) {
 					},
 					handlerSteps: func(handler ProcReceiveHandler) error {
 						require.NoError(t, handler.AcceptUpdate("refs/heads/main"))
-						return handler.Close()
+						return handler.Close(nil)
 					},
 				}
 			},
@@ -167,7 +168,46 @@ func TestProcReceiveHandler(t *testing.T) {
 					},
 					handlerSteps: func(handler ProcReceiveHandler) error {
 						require.NoError(t, handler.AcceptUpdate("refs/heads/main"))
-						return handler.Close()
+						return handler.Close(nil)
+					},
+				}
+			},
+		},
+		{
+			desc: "single reference but close midway with error",
+			setup: func(t *testing.T, ctx context.Context) setupData {
+				var stdin bytes.Buffer
+				_, err := pktline.WriteString(&stdin, "version=1\000push-options")
+				require.NoError(t, err)
+				err = pktline.WriteFlush(&stdin)
+				require.NoError(t, err)
+				_, err = pktline.WriteString(&stdin, fmt.Sprintf("%s %s %s",
+					gittest.DefaultObjectHash.ZeroOID, gittest.DefaultObjectHash.EmptyTreeOID, "refs/heads/main"))
+				require.NoError(t, err)
+				err = pktline.WriteFlush(&stdin)
+				require.NoError(t, err)
+
+				var stdout bytes.Buffer
+				_, err = pktline.WriteString(&stdout, "version=1\000")
+				require.NoError(t, err)
+				err = pktline.WriteFlush(&stdout)
+				require.NoError(t, err)
+
+				return setupData{
+					env:              []string{payload},
+					ctx:              ctx,
+					stdin:            stdin.String(),
+					expectedStdout:   stdout.String(),
+					expectedCloseErr: errors.New("season ticket on a one way ride"),
+					expectedUpdates: []ReferenceUpdate{
+						{
+							Ref:    "refs/heads/main",
+							OldOID: gittest.DefaultObjectHash.ZeroOID,
+							NewOID: gittest.DefaultObjectHash.EmptyTreeOID,
+						},
+					},
+					handlerSteps: func(handler ProcReceiveHandler) error {
+						return handler.Close(errors.New("season ticket on a one way ride"))
 					},
 				}
 			},
@@ -221,7 +261,7 @@ func TestProcReceiveHandler(t *testing.T) {
 					handlerSteps: func(handler ProcReceiveHandler) error {
 						require.NoError(t, handler.AcceptUpdate("refs/heads/main"))
 						require.NoError(t, handler.RejectUpdate("refs/heads/branch", "for fun"))
-						return handler.Close()
+						return handler.Close(nil)
 					},
 				}
 			},
@@ -241,9 +281,6 @@ func TestProcReceiveHandler(t *testing.T) {
 				return
 			}
 
-			updates := handler.ReferenceUpdates()
-			require.Equal(t, setup.expectedUpdates, updates)
-
 			select {
 			case <-doneCh:
 				t.Fatal("done returned before handler called Close()")
@@ -252,7 +289,8 @@ func TestProcReceiveHandler(t *testing.T) {
 
 			require.NoError(t, setup.handlerSteps(handler))
 			// When Close() is called, we must receive a confirmation.
-			<-doneCh
+			err = <-doneCh
+			require.Equal(t, setup.expectedCloseErr, err)
 
 			require.Equal(t, setup.expectedStdout, stdout.String())
 		})
diff --git a/internal/gitaly/hook/procreceive_registry.go b/internal/gitaly/hook/procreceive_registry.go
index b62fa6ae2..56021d311 100644
--- a/internal/gitaly/hook/procreceive_registry.go
+++ b/internal/gitaly/hook/procreceive_registry.go
@@ -36,8 +36,10 @@ type ProcReceiveHandler interface {
 	// with a reason.
 	RejectUpdate(referenceName git.ReferenceName, reason string) error
 
-	// Close must be called to clean up the proc-receive hook.
-	Close() error
+	// Close must be called to clean up the proc-receive hook. If the user
+	// of the handler encounters an error, it should be transferred to the
+	// hook too.
+	Close(rpcErr error) error
 }
 
 // ProcReceiveRegistry is the registry which provides the proc-receive handlers
diff --git a/internal/gitaly/hook/procreceive_registry_test.go b/internal/gitaly/hook/procreceive_registry_test.go
index e2fd1f3f1..339e4a6f1 100644
--- a/internal/gitaly/hook/procreceive_registry_test.go
+++ b/internal/gitaly/hook/procreceive_registry_test.go
@@ -31,7 +31,7 @@ func TestProcReceiveRegistry(t *testing.T) {
 		SkipCreationViaService: true,
 	})
 
-	newHandler := func(id storage.TransactionID) (ProcReceiveHandler, <-chan struct{}) {
+	newHandler := func(id storage.TransactionID) (ProcReceiveHandler, <-chan error) {
 		payload, err := git.NewHooksPayload(
 			cfg,
 			repo,