Merge pull request #163 from buildkite-plugins/SUP-6353-cleanup-diff-logic

Improve diff() logic, extend tests

Author: mcncl

README.md

@@ -236,11 +236,16 @@ Depending on your use case, you may want to determine the point where the branch
 
 Default: `git diff --name-only HEAD~1`
 
+The diff command must produce **newline-delimited output** with one file path per line. This is the format that `git diff --name-only` produces by default. Newline-delimited output is required for filenames containing spaces to be parsed correctly.
+
+Custom diff scripts should follow the same convention — print one path per line to standard output.
+
 #### Sample output
 
 ```
 README.md
 lib/trigger.bash
+directory/File Name With Spaces.md
 ```
 
 #### Example scripts

e2e/diff-output-realistic

@@ -0,0 +1,5 @@
+normal/path.go
+"path/with\tescape.go"
+directory/File Name With Spaces.md
+"projects/17_\360\237\252\201_emoji.py"
+another dir/some file.txt

e2e/multiple-paths

@@ -1,2 +1,3 @@
 user-service/infrastructure/cloudfront.yaml
+user-service/my config/settings.yaml
 user-service/serverless.yaml

pipeline.go

@@ -105,21 +105,23 @@ func diff(command string) ([]string, error) {
 		return nil, fmt.Errorf("diff command failed: %v", err)
 	}
 
-	trimmed := strings.Trim(output, " \t\r\v\f")
-	if trimmed == "" {
+	hasNewlines := strings.ContainsRune(output, '\n')
+	output = strings.TrimRight(output, "\n")
+	if output == "" {
 		return []string{}, nil
 	}
 
 	var fields []string
-	if strings.Contains(trimmed, "\n") {
-		for _, line := range strings.Split(trimmed, "\n") {
+	if hasNewlines {
+		for _, line := range strings.Split(output, "\n") {
 			line = strings.TrimSpace(line)
 			if line != "" {
 				fields = append(fields, line)
 			}
 		}
 	} else {
-		fields = strings.Fields(trimmed)
+		// Single line without newline — legacy compat for custom diff commands
+		fields = strings.Fields(output)
 	}
 
 	paths := make([]string, 0, len(fields))

pipeline_test.go

@@ -108,13 +108,29 @@ func TestDiff(t *testing.T) {
 func TestDiffWithSubshell(t *testing.T) {
 	want := []string{
 		"user-service/infrastructure/cloudfront.yaml",
+		"user-service/my config/settings.yaml",
 		"user-service/serverless.yaml",
 	}
 	got, err := diff("cat e2e/multiple-paths")
 	assert.NoError(t, err)
 	assert.Equal(t, want, got)
 }
 
+func TestDiffRealisticGitOutput(t *testing.T) {
+	// Fixture mirrors git diff --name-only output: plain paths, C-style
+	// quoted paths (tab, octal emoji), and paths with spaces.
+	want := []string{
+		"normal/path.go",
+		"path/with\tescape.go",
+		"directory/File Name With Spaces.md",
+		"projects/17_🪁_emoji.py",
+		"another dir/some file.txt",
+	}
+	got, err := diff("cat e2e/diff-output-realistic")
+	assert.NoError(t, err)
+	assert.Equal(t, want, got)
+}
+
 func TestDiffWithQuotedPaths(t *testing.T) {
 	want := []string{
 		"projects/test/pages/17_🪁_testfile.py",
@@ -161,6 +177,54 @@ func TestDiffWithSpacesInFilenamesSingleFile(t *testing.T) {
 	assert.Equal(t, want, got)
 }
 
+func TestDiffEmptyOutput(t *testing.T) {
+	got, err := diff(`printf ''`)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{}, got)
+}
+
+func TestDiffWhitespaceOnlyOutput(t *testing.T) {
+	got, err := diff(`printf '\n\n\n'`)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{}, got)
+}
+
+func TestDiffWhitespaceOnlyNoNewlines(t *testing.T) {
+	got, err := diff(`printf '   \t  '`)
+	assert.NoError(t, err)
+	assert.Equal(t, []string{}, got)
+}
+
+func TestDiffSingleFileNoTrailingNewline(t *testing.T) {
+	// Legacy compat: custom diff commands may not emit a trailing newline
+	want := []string{"services/foo/serverless.yml"}
+	got, err := diff(`printf 'services/foo/serverless.yml'`)
+	assert.NoError(t, err)
+	assert.Equal(t, want, got)
+}
+
+func TestDiffWindowsLineEndings(t *testing.T) {
+	want := []string{
+		"services/foo/file.go",
+		"services/bar/file.go",
+	}
+	got, err := diff(`printf 'services/foo/file.go\r\nservices/bar/file.go\r\n'`)
+	assert.NoError(t, err)
+	assert.Equal(t, want, got)
+}
+
+func TestDiffQuotedPathsWithSpaces(t *testing.T) {
+	// Git C-style quotes paths with special chars; spaces alone don't trigger quoting,
+	// but paths with both spaces and special chars will be quoted.
+	want := []string{
+		"projects/my docs/17_🪁_file.py",
+		"normal/file.txt",
+	}
+	got, err := diff(`printf '"projects/my docs/17_\360\237\252\201_file.py"\nnormal/file.txt\n'`)
+	assert.NoError(t, err)
+	assert.Equal(t, want, got)
+}
+
 func TestStepsToTriggerWithEmojiPaths(t *testing.T) {
 	watch := []WatchConfig{
 		{