refactor: re-organize node loading code to make it easier to follow

Readability is subjective, but while working in this part of the code
I found it difficult to follow the different code paths when loading
a node's content.

In this change I am attempting to make the branches of the decision
tree return early as much as possible.  This way we discard each
possibility as we work our way down, in this order:

1. Non-remote nodes
2. In offline mode, use cached remote nodes
3. On network timeout, use cached remote nodes
4a. Use fetched remote node
4b. On checksum change, handle writing to cache

Tests are added to ensure this functionality does not break.

Author: pbitty

taskfile/reader.go

@@ -221,91 +221,89 @@ func (r *Reader) readNode(node Node) (*ast.Taskfile, error) {
 }
 
 func (r *Reader) loadNodeContent(node Node) ([]byte, error) {
-	var b []byte
-	var err error
-	var cache *Cache
+	if !node.Remote() {
+		ctx, cf := context.WithTimeout(context.Background(), r.timeout)
+		defer cf()
+		return node.Read(ctx)
+	}
 
-	if node.Remote() {
-		cache, err = NewCache(r.tempDir)
-		if err != nil {
-			return nil, err
-		}
+	cache, err := NewCache(r.tempDir)
+	if err != nil {
+		return nil, err
 	}
 
-	// If the file is remote and we're in offline mode, check if we have a cached copy
-	if node.Remote() && r.offline {
-		if b, err = cache.read(node); errors.Is(err, os.ErrNotExist) {
+	if r.offline {
+		// In offline mode try to use cached copy
+		cached, err := cache.read(node)
+		if errors.Is(err, os.ErrNotExist) {
 			return nil, &errors.TaskfileCacheNotFoundError{URI: node.Location()}
 		} else if err != nil {
 			return nil, err
 		}
 		r.logger.VerboseOutf(logger.Magenta, "task: [%s] Fetched cached copy\n", node.Location())
-	} else {
 
-		downloaded := false
-		ctx, cf := context.WithTimeout(context.Background(), r.timeout)
-		defer cf()
+		return cached, nil
+	}
 
-		// Read the file
-		b, err = node.Read(ctx)
-		var taskfileNetworkTimeoutError *errors.TaskfileNetworkTimeoutError
+	ctx, cf := context.WithTimeout(context.Background(), r.timeout)
+	defer cf()
+
+	b, err := node.Read(ctx)
+	if errors.Is(err, &errors.TaskfileNetworkTimeoutError{}) {
 		// If we timed out then we likely have a network issue
-		if node.Remote() && errors.As(err, &taskfileNetworkTimeoutError) {
-			// If a download was requested, then we can't use a cached copy
-			if r.download {
-				return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout}
-			}
-			// Search for any cached copies
-			if b, err = cache.read(node); errors.Is(err, os.ErrNotExist) {
-				return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout, CheckedCache: true}
-			} else if err != nil {
-				return nil, err
-			}
-			r.logger.VerboseOutf(logger.Magenta, "task: [%s] Network timeout. Fetched cached copy\n", node.Location())
+
+		// If a download was requested, then we can't use a cached copy
+		if r.download {
+			return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout}
+		}
+
+		// Search for any cached copies
+		cached, err := cache.read(node)
+		if errors.Is(err, os.ErrNotExist) {
+			return nil, &errors.TaskfileNetworkTimeoutError{URI: node.Location(), Timeout: r.timeout, CheckedCache: true}
 		} else if err != nil {
 			return nil, err
-		} else {
-			downloaded = true
 		}
+		r.logger.VerboseOutf(logger.Magenta, "task: [%s] Network timeout. Fetched cached copy\n", node.Location())
 
-		// If the node was remote, we need to check the checksum
-		if node.Remote() && downloaded {
-			r.logger.VerboseOutf(logger.Magenta, "task: [%s] Fetched remote copy\n", node.Location())
-
-			// Get the checksums
-			checksum := checksum(b)
-			cachedChecksum := cache.readChecksum(node)
-
-			var prompt string
-			if cachedChecksum == "" {
-				// If the checksum doesn't exist, prompt the user to continue
-				prompt = fmt.Sprintf(taskfileUntrustedPrompt, node.Location())
-			} else if checksum != cachedChecksum {
-				// If there is a cached hash, but it doesn't match the expected hash, prompt the user to continue
-				prompt = fmt.Sprintf(taskfileChangedPrompt, node.Location())
-			}
+		return cached, nil
 
-			if prompt != "" {
-				if err := func() error {
-					r.promptMutex.Lock()
-					defer r.promptMutex.Unlock()
-					return r.logger.Prompt(logger.Yellow, prompt, "n", "y", "yes")
-				}(); err != nil {
-					return nil, &errors.TaskfileNotTrustedError{URI: node.Location()}
-				}
-			}
-			// If the hash has changed (or is new)
-			if checksum != cachedChecksum {
-				// Store the checksum
-				if err := cache.writeChecksum(node, checksum); err != nil {
-					return nil, err
-				}
-				// Cache the file
-				r.logger.VerboseOutf(logger.Magenta, "task: [%s] Caching downloaded file\n", node.Location())
-				if err = cache.write(node, b); err != nil {
-					return nil, err
-				}
-			}
+	} else if err != nil {
+		return nil, err
+	}
+	r.logger.VerboseOutf(logger.Magenta, "task: [%s] Fetched remote copy\n", node.Location())
+
+	// Get the checksums
+	checksum := checksum(b)
+	cachedChecksum := cache.readChecksum(node)
+
+	var prompt string
+	if cachedChecksum == "" {
+		// If the checksum doesn't exist, prompt the user to continue
+		prompt = fmt.Sprintf(taskfileUntrustedPrompt, node.Location())
+	} else if checksum != cachedChecksum {
+		// If there is a cached hash, but it doesn't match the expected hash, prompt the user to continue
+		prompt = fmt.Sprintf(taskfileChangedPrompt, node.Location())
+	}
+
+	if prompt != "" {
+		if err := func() error {
+			r.promptMutex.Lock()
+			defer r.promptMutex.Unlock()
+			return r.logger.Prompt(logger.Yellow, prompt, "n", "y", "yes")
+		}(); err != nil {
+			return nil, &errors.TaskfileNotTrustedError{URI: node.Location()}
+		}
+
+		// Store the checksum
+		if err := cache.writeChecksum(node, checksum); err != nil {
+			return nil, err
+		}
+
+		// Cache the file
+		r.logger.VerboseOutf(logger.Magenta, "task: [%s] Caching downloaded file\n", node.Location())
+		if err = cache.write(node, b); err != nil {
+			return nil, err
 		}
 	}