From 8827bf7489b8c0d8f21aacc22eb98534b895d22d Mon Sep 17 00:00:00 2001
From: ChrisERo <cer95@cornell.edu>
Date: Fri, 14 Jan 2022 13:32:01 -0500
Subject: [PATCH] refactor: imporved efficiency and code style

* changed sendCompletedOnceMessageIfApplicable so that it kills thread
  after executing
* moved logic for waiting on HookRunner into function defined in
  HookRunner itself
---
 cmd/git-sync/main.go | 39 ++++++++++++++-------------------------
 pkg/hook/hook.go     | 25 +++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 27 deletions(-)

diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go
index a71ac69..408798a 100644
--- a/cmd/git-sync/main.go
+++ b/cmd/git-sync/main.go
@@ -174,11 +174,6 @@ var (
 	}, []string{"status"})
 )
 
-// Channels for ensuring hooks execute at least once before terminating when
-// --one-time flag is set.  Should be nil if and only if corresponding hook is
-// not defined and if initialised, will only ever get written to once.
-var exechookChannel, webhookChannel chan bool
-
 const (
 	metricKeySuccess = "success"
 	metricKeyError   = "error"
@@ -364,10 +359,6 @@ func main() {
 		if *flExechookBackoff < time.Second {
 			handleError(log, true, "ERROR: --exechook-backoff must be at least 1s")
 		}
-
-		if *flOneTime {
-			exechookChannel = make(chan bool, 1)
-		}
 	}
 
 	if *flWebhookURL != "" {
@@ -380,10 +371,6 @@ func main() {
 		if *flWebhookBackoff < time.Second {
 			handleError(log, true, "ERROR: --webhook-backoff must be at least 1s")
 		}
-
-		if *flOneTime {
-			webhookChannel = make(chan bool, 1)
-		}
 	}
 
 	if *flPassword != "" && *flPasswordFile != "" {
@@ -557,6 +544,10 @@ func main() {
 	// Startup webhooks goroutine
 	var webhookRunner *hook.HookRunner
 	if *flWebhookURL != "" {
+		var webhookChannel chan bool
+		if *flOneTime {
+			webhookChannel = make(chan bool, 1)
+		}
 		webhook := hook.NewWebhook(
 			*flWebhookURL,
 			*flWebhookMethod,
@@ -577,6 +568,10 @@ func main() {
 	// Startup exechooks goroutine
 	var exechookRunner *hook.HookRunner
 	if *flExechookCommand != "" {
+		var exechookChannel chan bool
+		if *flOneTime {
+			exechookChannel = make(chan bool, 1)
+		}
 		exechook := hook.NewExechook(
 			cmd.NewRunner(log),
 			*flExechookCommand,
@@ -642,19 +637,13 @@ func main() {
 				// checking whether to stop program.
 				// Assumes that if hook channels are not nil, they will have at
 				// least one value before getting closed
-				exitCode := 0  // is 0 if all hooks succeed, else is 1
-				if exechookChannel != nil {
-					exechookChannelFinishedSuccessfully := <-exechookChannel
-					if !exechookChannelFinishedSuccessfully {
-						log.Error(nil, "exechook completed with error")
-						exitCode = 1
-					}
+				exitCode := 0 // is 0 if all hooks succeed, else is 1
+				if err = exechookRunner.WaitForCompletion(); err != nil {
+					log.Error(err, "exechook completed with error")
+					exitCode = 1
 				}
-				if webhookChannel != nil {
-					webhookChannelFinishedSuccessfully := <-webhookChannel
-					if !webhookChannelFinishedSuccessfully {
-						log.Error(nil, "webhook completed with error")
-					}
+				if err = webhookRunner.WaitForCompletion(); err != nil {
+					log.Error(err, "webhook completed with error")
 					exitCode = 1
 				}
 				log.DeleteErrorFile()
diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go
index 06e68cd..d1b89c2 100644
--- a/pkg/hook/hook.go
+++ b/pkg/hook/hook.go
@@ -18,6 +18,8 @@ package hook
 
 import (
 	"context"
+	"fmt"
+	"runtime"
 	"sync"
 	"time"
 
@@ -143,17 +145,36 @@ func (r *HookRunner) Run(ctx context.Context) {
 
 // If hasCompletedOnce is nil, does nothing. Otherwise, forwards the caller
 // provided success status (as a boolean) of HookRunner to receivers of
-// hasCompletedOnce, closes said chanel, and sets hasCompletedOnce to nil.
+// hasCompletedOnce, closes said chanel, and terminates this goroutine.
 // Using this function to write to hasCompletedOnce ensures it is only ever
 // written to once.
 func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) {
 	if r.hasCompletedOnce != nil {
 		r.hasCompletedOnce <- completedSuccessfully
 		close(r.hasCompletedOnce)
-		r.hasCompletedOnce = nil
+		runtime.Goexit()
 	}
 }
 
+// WaitForCompletion waits for HookRunner to send completion message to
+// calling thread and returns either true if HookRunner executed successfully
+// and some error otherwise.
+// Assumes that r.hasCompletedOnce is not nil, but if it is, returns an error
+func (r *HookRunner) WaitForCompletion() error {
+	// Make sure function should be called
+	if r.hasCompletedOnce == nil {
+		return fmt.Errorf("HookRunner.WaitForCompletion called on async runner")
+	}
+
+	// Perform wait on HookRunner
+	exechookChannelFinishedSuccessfully := <-r.hasCompletedOnce
+	if !exechookChannelFinishedSuccessfully {
+		return fmt.Errorf("exechook completed with error")
+	}
+
+	return nil
+}
+
 func updateHookRunCountMetric(name, status string) {
 	hookRunCount.WithLabelValues(name, status).Inc()
 }