From 3b5ed549ebf643af8c3acae0240bf0a97cb99d01 Mon Sep 17 00:00:00 2001
From: ChrisERo <cer95@cornell.edu>
Date: Sat, 8 Jan 2022 14:36:22 -0500
Subject: [PATCH] fix: addressed several concers raised by thockin

- Renamed [x]HookChannel to [x]hookChannel for consistency
- Made hookChannels buffered chanels of size 1
- Added clarifying documentation and renamed functions expalining how I was
    - ensuring that hooks execute at least once, assuming that main thread does not crash
    - make sure chanels are not written to more than once
---
 cmd/git-sync/main.go | 30 +++++++++++++++---------------
 pkg/hook/hook.go     | 14 ++++++++------
 2 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go
index 7a67427..396cb07 100644
--- a/cmd/git-sync/main.go
+++ b/cmd/git-sync/main.go
@@ -174,8 +174,10 @@ var (
 	}, []string{"status"})
 )
 
-// Channels for ensuring hooks execute at least once before terminating due to GIT_SYNC_ONCE
-var execHookChannel, webHookChannel chan bool  // initialising as null to promptly catch logical errors and to avoid unneeded object creation
+// Channels for ensuring hooks execute at least once before terminating.
+// Should be nil if and only if corresponding hook is not defined and if initialised, will only every get written
+// to once.
+var exechookChannel, webhookChannel chan bool
 
 const (
 	metricKeySuccess = "success"
@@ -362,8 +364,7 @@ func main() {
 		if *flExechookBackoff < time.Second {
 			handleError(log, true, "ERROR: --exechook-backoff must be at least 1s")
 		}
-
-		execHookChannel = make(chan bool)
+		exechookChannel = make(chan bool, 1)
 	}
 
 	if *flWebhookURL != "" {
@@ -376,8 +377,7 @@ func main() {
 		if *flWebhookBackoff < time.Second {
 			handleError(log, true, "ERROR: --webhook-backoff must be at least 1s")
 		}
-
-		webHookChannel = make(chan bool)
+		webhookChannel = make(chan bool, 1)
 	}
 
 	if *flPassword != "" && *flPasswordFile != "" {
@@ -563,7 +563,7 @@ func main() {
 			*flWebhookBackoff,
 			hook.NewHookData(),
 			log,
-			webHookChannel,
+			webhookChannel,
 		)
 		go webhookRunner.Run(context.Background())
 	}
@@ -584,7 +584,7 @@ func main() {
 			*flExechookBackoff,
 			hook.NewHookData(),
 			log,
-			execHookChannel,
+			exechookChannel,
 		)
 		go exechookRunner.Run(context.Background())
 	}
@@ -630,17 +630,17 @@ func main() {
 		}
 
 		if initialSync {
-			// Wait for hooks to complete at least once before checking whether to stop
+			// Wait for hooks to complete at least once, if not nil, before checking whether to stop program
 			// Assumes that if hook channels are not nil, they will have at least one value before getting closed
-			if execHookChannel != nil {
-				execHookChannelFinishedSuccessfully:= <-execHookChannel
-				if !execHookChannelFinishedSuccessfully {
+			if exechookChannel != nil {
+				exechookChannelFinishedSuccessfully := <-exechookChannel
+				if !exechookChannelFinishedSuccessfully {
 					log.Error(nil, "exec hook completed with error")
 				}
 			}
-			if webHookChannel != nil {
-				webHookChannelFinishedSuccessfully:= <-webHookChannel
-				if !webHookChannelFinishedSuccessfully {
+			if webhookChannel != nil {
+				webhookChannelFinishedSuccessfully := <-webhookChannel
+				if !webhookChannelFinishedSuccessfully {
 					log.Error(nil, "web hook completed with error")
 				}
 			}
diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go
index ce027f2..86febea 100644
--- a/pkg/hook/hook.go
+++ b/pkg/hook/hook.go
@@ -96,7 +96,8 @@ type HookRunner struct {
 	data *hookData
 	// Logger
 	logger *logging.Logger
-	// Has succeeded once Chanel, sends true if first run executed successfully and false if it failed
+	// hasCompletedOnce is used to send true if and only if first run executed successfully and false otherwise to some receiver.
+	// should be initialised to a buffered channel of size 1. Is only meant to be used within sendCompletedOnceMessageIfApplicable
 	hasCompletedOnce chan bool
 }
 
@@ -126,21 +127,22 @@ func (r *HookRunner) Run(ctx context.Context) {
 				r.logger.Error(err, "hook failed")
 				updateHookRunCountMetric(r.hook.Name(), "error")
 				// don't want to sleep unnecessarily if we are going to terminate anyways
-				r.sendCompletedOnceMessage(false)
+				r.sendCompletedOnceMessageIfApplicable(false)
 				time.Sleep(r.backoff)
 			} else {
 				updateHookRunCountMetric(r.hook.Name(), "success")
 				lastHash = hash
-				r.sendCompletedOnceMessage(true)
+				r.sendCompletedOnceMessageIfApplicable(true)
 				break
 			}
 		}
 	}
 }
 
-// sendCompletedOnceMessage forwards the success status (as a boolean) of the first execution of HookRunner, the first time
-// to the r.hasCompletedOnce channel
-func (r *HookRunner) sendCompletedOnceMessage(completedSuccessfully bool) {
+// 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.
+// Using this function to write to hasCompletedOnce ensures it is only every written to once.
+func (r *HookRunner) sendCompletedOnceMessageIfApplicable(completedSuccessfully bool) {
 	if r.hasCompletedOnce != nil {
 		r.hasCompletedOnce <- completedSuccessfully
 		close(r.hasCompletedOnce)