From 280e6643a52850bec0d1f01ab593e8357eaf1ae3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Fri, 2 Aug 2024 10:36:31 +0200
Subject: [PATCH] Fix #104331: Path separator normalisation replaces too much

There was a missing condition, which meant that configuring any two-way
variable caused path separator normalisation on all job/task/command
parameters.
---
 internal/manager/api_impl/varrepl_test.go | 79 +++++++++++++++++++++++
 internal/manager/config/variables.go      |  4 ++
 2 files changed, 83 insertions(+)

diff --git a/internal/manager/api_impl/varrepl_test.go b/internal/manager/api_impl/varrepl_test.go
index 5eaa080c..854a9a08 100644
--- a/internal/manager/api_impl/varrepl_test.go
+++ b/internal/manager/api_impl/varrepl_test.go
@@ -243,6 +243,85 @@ func TestReplaceTwoWayVariables(t *testing.T) {
 	}
 }
 
+// TestReplaceTwoWayVariablesFFmpegExpression tests that slashes (for division)
+// in an FFmpeg filter expression are NOT replaced with backslashes when sending
+// to a Windows worker.
+func TestReplaceTwoWayVariablesFFmpegExpression(t *testing.T) {
+	c := config.DefaultConfig(func(c *config.Conf) {
+		// Mock that the Manager is running Linux.
+		c.MockCurrentGOOSForTests("linux")
+
+		// Trigger a translation of a path in the FFmpeg command arguments.
+		c.Variables["project"] = config.Variable{
+			IsTwoWay: true,
+			Values: []config.VariableValue{
+				{Value: "/projects/charge", Platform: config.VariablePlatformLinux, Audience: config.VariableAudienceAll},
+				{Value: `P:\charge`, Platform: config.VariablePlatformWindows, Audience: config.VariableAudienceAll},
+			},
+		}
+	})
+
+	task := api.AssignedTask{
+		Job:         "f0bde4d0-eaaf-4ee0-976b-802a86aa2d02",
+		JobPriority: 50,
+		JobType:     "simple-blender-render",
+		Name:        "preview-video",
+		Priority:    50,
+		Status:      api.TaskStatusQueued,
+		TaskType:    "ffmpeg",
+		Uuid:        "fd963a82-2e98-4a39-9bd4-c302e5b8814f",
+		Commands: []api.Command{
+			{
+				Name: "frames-to-video",
+				Parameters: map[string]interface{}{
+					"exe":        "ffmpeg",                             // Should not change.
+					"fps":        24,                                   // Should not change type.
+					"inputGlob":  "/projects/charge/renders/*.webp",    // Path, should change.
+					"outputFile": "/projects/charge/renders/video.mp4", // Path, should change.
+					"args": []string{
+						"-vf", "pad=ceil(iw/2)*2:ceil(ih/2)*2", // Should not change.
+						"-fake-lut", `/projects/charge/ffmpeg.lut`, // Path, should change.
+					},
+				},
+			},
+		},
+	}
+
+	worker := persistence.Worker{Platform: "windows"}
+	replacedTask := replaceTaskVariables(&c, task, worker)
+
+	expectTask := api.AssignedTask{
+		Job:         "f0bde4d0-eaaf-4ee0-976b-802a86aa2d02",
+		JobPriority: 50,
+		JobType:     "simple-blender-render",
+		Name:        "preview-video",
+		Priority:    50,
+		Status:      api.TaskStatusQueued,
+		TaskType:    "ffmpeg",
+		Uuid:        "fd963a82-2e98-4a39-9bd4-c302e5b8814f",
+		Commands: []api.Command{
+			{
+				Name: "frames-to-video",
+				Parameters: map[string]interface{}{
+					"exe": "ffmpeg",
+					"fps": 24,
+					// These two parameters matched a two-way variable:
+					"inputGlob":  `P:\charge\renders\*.webp`,
+					"outputFile": `P:\charge\renders\video.mp4`,
+					"args": []string{
+						// This parameter should not change:
+						"-vf", "pad=ceil(iw/2)*2:ceil(ih/2)*2",
+						// This parameter should change:
+						"-fake-lut", `P:\charge\ffmpeg.lut`,
+					},
+				},
+			},
+		},
+	}
+
+	assert.Equal(t, expectTask, replacedTask)
+}
+
 func varReplSubmittedJob() api.SubmittedJob {
 	return api.SubmittedJob{
 		Type:              "simple-blender-render",
diff --git a/internal/manager/config/variables.go b/internal/manager/config/variables.go
index 04fc7aca..6d866c56 100644
--- a/internal/manager/config/variables.go
+++ b/internal/manager/config/variables.go
@@ -106,6 +106,10 @@ func (ve *VariableExpander) Expand(valueToExpand string) string {
 	isPathValue := false
 	for varname, varvalue := range ve.targetTwoWayVars {
 		placeholder := fmt.Sprintf("{%s}", varname)
+		if !strings.Contains(expanded, placeholder) {
+			continue
+		}
+
 		expanded = strings.Replace(expanded, placeholder, varvalue, -1)
 
 		// Since two-way variables are meant for path replacement, we know this