hooks: Disallow configuring hooks multiple times

Now that we have a shared hook setup which always sets up the hooks
path, transaction environment as well as the mandatory hooks payload, we
should enforce that hooks aren't being setup multiple times. While it
doesn't hurt right now as the subset of information that's shared across
both hook configurations is exactly the same, this will change in the
future as we put more information into the hooks payload. And as soon as
we do, configuring multiple hooks may easily override information based
on the order in which hooks were configured.

So let'just assert that hooks are always configured exactly once for
each command which may update references.

4 files changed, 13 insertions, 11 deletions

diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index 0259f9adf..71f2abd0f 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -33,6 +33,10 @@ func WithRefTxHook(ctx context.Context, repo *gitalypb.Repository, cfg config.Cf
 // variables required by the reference transaction hook and any other needed
 // options to successfully execute hooks.
 func (cc *cmdCfg) configureHooks(ctx context.Context, repo *gitalypb.Repository, cfg config.Cfg) error {
+	if cc.hooksConfigured {
+		return errors.New("hooks already configured")
+	}
+
 	payload, err := NewHooksPayload(cfg, repo).Env()
 	if err != nil {
 		return err
@@ -46,7 +50,7 @@ func (cc *cmdCfg) configureHooks(ctx context.Context, repo *gitalypb.Repository,
 	cc.env = append(cc.env, payload, "GITALY_BIN_DIR="+cfg.BinDir)
 
 	cc.globals = append(cc.globals, ValueFlag{"-c", fmt.Sprintf("core.hooksPath=%s", hooks.Path(cfg))})
-	cc.refHookConfigured = true
+	cc.hooksConfigured = true
 
 	return nil
 }
diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go
index 4c49f3e5a..24638d4e7 100644
--- a/internal/git/safecmd.go
+++ b/internal/git/safecmd.go
@@ -216,12 +216,12 @@ func ConvertGlobalOptions(options *gitalypb.GlobalOptions) []Option {
 }
 
 type cmdCfg struct {
-	env               []string
-	globals           []Option
-	stdin             io.Reader
-	stdout            io.Writer
-	stderr            io.Writer
-	refHookConfigured bool
+	env             []string
+	globals         []Option
+	stdin           io.Reader
+	stdout          io.Writer
+	stderr          io.Writer
+	hooksConfigured bool
 }
 
 // CmdOpt is an option for running a command
@@ -267,10 +267,10 @@ func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error {
 		}
 	}
 
-	if !cc.refHookConfigured && mayUpdateRef(sc.Subcommand()) {
+	if !cc.hooksConfigured && mayUpdateRef(sc.Subcommand()) {
 		return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookRequired)
 	}
-	if cc.refHookConfigured && !mayUpdateRef(sc.Subcommand()) {
+	if cc.hooksConfigured && !mayUpdateRef(sc.Subcommand()) {
 		return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookNotRequired)
 	}
 	if mayGeneratePackfiles(sc.Subcommand()) {
diff --git a/internal/gitaly/service/smarthttp/receive_pack.go b/internal/gitaly/service/smarthttp/receive_pack.go
index 49db0d394..29b11cb65 100644
--- a/internal/gitaly/service/smarthttp/receive_pack.go
+++ b/internal/gitaly/service/smarthttp/receive_pack.go
@@ -56,7 +56,6 @@ func (s *server) PostReceivePack(stream gitalypb.SmartHTTPService_PostReceivePac
 		},
 		git.WithReceivePackHooks(ctx, config.Config, req, "http"),
 		git.WithGitProtocol(ctx, req),
-		git.WithRefTxHook(ctx, req.Repository, config.Config),
 	)
 
 	if err != nil {
diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index 75247a4fc..9e8ee81f0 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -73,7 +73,6 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
 		},
 		git.WithReceivePackHooks(ctx, config.Config, req, "ssh"),
 		git.WithGitProtocol(ctx, req),
-		git.WithRefTxHook(ctx, req.Repository, config.Config),
 	)
 
 	if err != nil {