Fix stdout/stderr handling (#27)

Author: brikis98

cmd/_testscripts/test-stdout-stderr.sh

@@ -0,0 +1,7 @@
+#!/usr/bin/env bash
+# This script writes some text to stdout and stderr and then exits with an error. This is used to test that git-xargs
+# always logs the stdout and stderr from scripts, even if those scripts exit with an error.
+
+echo 'Hello, from STDOUT'
+>&2 echo 'Hello, from STDERR'
+exit 1

cmd/process.go

@@ -72,13 +72,13 @@ func processRepo(config *GitXargsConfig, repo *github.Repository) error {
 	}
 
 	//Run the specified command
-	commandErr := executeCommand(config, repositoryDir, repo, worktree)
+	commandErr := executeCommand(config, repositoryDir, repo)
 	if commandErr != nil {
 		return commandErr
 	}
 
 	// Commit any untracked files, modified or deleted files that resulted from script execution
-	commitErr := commitLocalChanges(config, worktree, repo, localRepository)
+	commitErr := commitLocalChanges(config, repositoryDir, worktree, repo, localRepository)
 	if commitErr != nil {
 		return commitErr
 	}

cmd/repo-operations.go

@@ -82,11 +82,14 @@ func getLocalRepoHeadRef(config *GitXargsConfig, localRepository *git.Repository
 	return ref, nil
 }
 
-// executeCommand runs the user-supplied command and runs it against the given repository, adding any git changes that may occur
-func executeCommand(config *GitXargsConfig, repositoryDir string, repo *github.Repository, worktree *git.Worktree) error {
-
-	logger := logging.GetLogger("git-xargs")
+// executeCommand runs the user-supplied command against the given repository
+func executeCommand(config *GitXargsConfig, repositoryDir string, repo *github.Repository) error {
+	return executeCommandWithLogger(config, repositoryDir, repo, logging.GetLogger("git-xargs"))
+}
 
+// executeCommandWithLogger runs the user-supplied command against the given repository, and sends the log output
+// to the given logger
+func executeCommandWithLogger(config *GitXargsConfig, repositoryDir string, repo *github.Repository, logger *logrus.Logger) error {
 	if len(config.Args) < 1 {
 		return errors.WithStackTrace(NoCommandSuppliedErr{})
 	}
@@ -104,6 +107,10 @@ func executeCommand(config *GitXargsConfig, repositoryDir string, repo *github.R
 
 	stdoutStdErr, err := cmd.CombinedOutput()
 
+	logger.WithFields(logrus.Fields{
+		"CombinedOutput": string(stdoutStdErr),
+	}).Debug("Received output of command run")
+
 	if err != nil {
 		logger.WithFields(logrus.Fields{
 			"Error": err,
@@ -113,58 +120,6 @@ func executeCommand(config *GitXargsConfig, repositoryDir string, repo *github.R
 		return errors.WithStackTrace(err)
 	}
 
-	logger.WithFields(logrus.Fields{
-		"CombinedOutput": string(stdoutStdErr),
-	}).Debug("Received output of command run")
-
-	status, statusErr := worktree.Status()
-
-	if statusErr != nil {
-		logger.WithFields(logrus.Fields{
-			"Error": statusErr,
-			"Repo":  repo.GetName(),
-			"Dir":   repositoryDir,
-		}).Debug("Error looking up worktree status")
-
-		// Track the status check failure
-		config.Stats.TrackSingle(WorktreeStatusCheckFailedCommand, repo)
-		return errors.WithStackTrace(statusErr)
-	}
-
-	// If the supplied command resulted in any changes, we need to stage, add and commit them
-	if !status.IsClean() {
-		logger.WithFields(logrus.Fields{
-			"Repo": repo.GetName(),
-		}).Debug("Local repository worktree no longer clean, will stage and add new files and commit changes")
-
-		// Track the fact that worktree changes were made following execution
-		config.Stats.TrackSingle(WorktreeStatusDirty, repo)
-
-		for filepath := range status {
-			if status.IsUntracked(filepath) {
-				fmt.Printf("Found untracked file %s. Adding to stage", filepath)
-				_, addErr := worktree.Add(filepath)
-				if addErr != nil {
-					logger.WithFields(logrus.Fields{
-						"Error":    addErr,
-						"Filepath": filepath,
-					}).Debug("Error adding file to git stage")
-					// Track the file staging failure
-					config.Stats.TrackSingle(WorktreeAddFileFailed, repo)
-					return errors.WithStackTrace(addErr)
-				}
-			}
-		}
-
-	} else {
-		logger.WithFields(logrus.Fields{
-			"Repo": repo.GetName(),
-		}).Debug("Local repository status is clean - nothing to stage or commit")
-
-		// Track the fact that repo had no file changes post command execution
-		config.Stats.TrackSingle(WorktreeStatusClean, repo)
-	}
-
 	return nil
 }
 
@@ -251,12 +206,61 @@ func checkoutLocalBranch(config *GitXargsConfig, ref *plumbing.Reference, worktr
 	return branchName, nil
 }
 
-// commitLocalChanges will create a commit using the supplied or default commit message and will add any untracked, deleted
-// or modified files that resulted from script execution
-func commitLocalChanges(config *GitXargsConfig, worktree *git.Worktree, remoteRepository *github.Repository, localRepository *git.Repository) error {
-
+// commitLocalChanges will check for any changes in worktree as a result of script execution, and if any are present,
+// add any untracked, deleted or modified files and create a commit using the supplied or default commit message.
+func commitLocalChanges(config *GitXargsConfig, repositoryDir string, worktree *git.Worktree, remoteRepository *github.Repository, localRepository *git.Repository) error {
 	logger := logging.GetLogger("git-xargs")
 
+	status, statusErr := worktree.Status()
+
+	if statusErr != nil {
+		logger.WithFields(logrus.Fields{
+			"Error": statusErr,
+			"Repo":  remoteRepository.GetName(),
+			"Dir":   repositoryDir,
+		}).Debug("Error looking up worktree status")
+
+		// Track the status check failure
+		config.Stats.TrackSingle(WorktreeStatusCheckFailedCommand, remoteRepository)
+		return errors.WithStackTrace(statusErr)
+	}
+
+	// If there are no changes, we log it, track it, and return
+	if status.IsClean() {
+		logger.WithFields(logrus.Fields{
+			"Repo": remoteRepository.GetName(),
+		}).Debug("Local repository status is clean - nothing to stage or commit")
+
+		// Track the fact that repo had no file changes post command execution
+		config.Stats.TrackSingle(WorktreeStatusClean, remoteRepository)
+
+		return nil
+	}
+
+	// If there are changes, we need to stage, add and commit them
+	logger.WithFields(logrus.Fields{
+		"Repo": remoteRepository.GetName(),
+	}).Debug("Local repository worktree no longer clean, will stage and add new files and commit changes")
+
+	// Track the fact that worktree changes were made following execution
+	config.Stats.TrackSingle(WorktreeStatusDirty, remoteRepository)
+
+	for filepath := range status {
+		if status.IsUntracked(filepath) {
+			fmt.Printf("Found untracked file %s. Adding to stage", filepath)
+			_, addErr := worktree.Add(filepath)
+			if addErr != nil {
+				logger.WithFields(logrus.Fields{
+					"Error":    addErr,
+					"Filepath": filepath,
+				}).Debug("Error adding file to git stage")
+				// Track the file staging failure
+				config.Stats.TrackSingle(WorktreeAddFileFailed, remoteRepository)
+				return errors.WithStackTrace(addErr)
+			}
+		}
+	}
+
 	// With all our untracked files staged, we can now create a commit, passing the All
 	// option when configuring our commit option so that all modified and deleted files
 	// will have their changes committed

cmd/repo-operations_test.go

@@ -1,10 +1,11 @@
 package main
 
 import (
-	"os"
+	"bytes"
+	"github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
 	"testing"
 
-	"github.com/go-git/go-git/v5"
 	"github.com/google/go-github/v32/github"
 )
 
@@ -26,25 +27,25 @@ func getMockGithubRepo() *github.Repository {
 	return repo
 }
 
-func cloneLocalTestRepo(t *testing.T) (string, *git.Repository) {
-	repo := getMockGithubRepo()
+// Test that we can execute a script and that the expected stdout and stderr get written to the logger, even if that
+// script exits with an error (exit status 1).
+func TestExecuteCommandWithLogger(t *testing.T) {
+	t.Parallel()
 
-	config := NewGitXargsTestConfig()
+	cfg := NewGitXargsConfig()
+	cfg.Args = []string{"./_testscripts/test-stdout-stderr.sh"}
 
-	localPath, localRepo, err := cloneLocalRepository(config, repo)
+	repo := getMockGithubRepo()
 
-	if err != nil {
-		t.Logf("Could not clone local repo to localPath: %s\n", localPath)
-		t.Fail()
+	var buffer bytes.Buffer
+	logger := &logrus.Logger{
+		Out:       &buffer,
+		Level:     logrus.TraceLevel,
+		Formatter: new(logrus.TextFormatter),
 	}
 
-	return localPath, localRepo
-}
-
-func cleanupLocalTestRepo(t *testing.T, localPath string) error {
-	removeErr := os.RemoveAll(localPath)
-	if removeErr != nil {
-		t.Logf("Error cleaning up test repo at path: %s err: %+v\n", localPath, removeErr)
-	}
-	return removeErr
+	err := executeCommandWithLogger(cfg, ".", repo, logger)
+	assert.Errorf(t, err, "exit status 1")
+	assert.Contains(t, buffer.String(), "Hello, from STDOUT")
+	assert.Contains(t, buffer.String(), "Hello, from STDERR")
 }