From cac776a994032e3e4f84db2aba1749b7f4c9d977 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 9 Dec 2024 14:16:05 -0800 Subject: [PATCH] config: "-e" arguments must stay at the end of replay steps Fixes #2908 When loading `config-file`, we need to ensure that all loaded configuration is loaded _prior_ to any `-e` values from the CLI. To do this, I inserted a new `-e` special tag type in our replay steps. This can be used to find when `-e` starts and ensure it remains at the end of replay steps when the replay steps are being modified. This commit also found a similar (but not exercised) issue where this could happen with light/dark themeing. --- src/config/Config.zig | 131 +++++++++++++++++++++++++++++------------- 1 file changed, 91 insertions(+), 40 deletions(-) diff --git a/src/config/Config.zig b/src/config/Config.zig index d05f494101..fa531dc7e1 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2468,7 +2468,7 @@ pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void { // First, we add an artificial "-e" so that if we // replay the inputs to rebuild the config (i.e. if // a theme is set) then we will get the same behavior. - try self._replay_steps.append(arena_alloc, .{ .arg = "-e" }); + try self._replay_steps.append(arena_alloc, .@"-e"); // Next, take all remaining args and use that to build up // a command to execute. @@ -2576,6 +2576,24 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { const cwd = std.fs.cwd(); + // We need to insert all of our loaded config-file values + // PRIOR to the "-e" in our replay steps, since everything + // after "-e" becomes an "initial-command". To do this, we + // dupe the values if we find it. + var replay_suffix = std.ArrayList(Replay.Step).init(alloc_gpa); + defer replay_suffix.deinit(); + for (self._replay_steps.items, 0..) |step, i| if (step == .@"-e") { + // We don't need to clone the steps because they should + // all be allocated in our arena and we're keeping our + // arena. + try replay_suffix.appendSlice(self._replay_steps.items[i..]); + + // Remove our old values. Again, don't need to free any + // memory here because its all part of our arena. + self._replay_steps.shrinkRetainingCapacity(i); + break; + }; + // We must use a while below and not a for(items) because we // may add items to the list while iterating for recursive // config-file entries. @@ -2627,6 +2645,14 @@ pub fn loadRecursiveFiles(self: *Config, alloc_gpa: Allocator) !void { try self.loadIter(alloc_gpa, &iter); try self.expandPaths(std.fs.path.dirname(path).?); } + + // If we have a suffix, add that back. + if (replay_suffix.items.len > 0) { + try self._replay_steps.appendSlice( + arena_alloc, + replay_suffix.items, + ); + } } /// Change the state of conditionals and reload the configuration @@ -2754,39 +2780,46 @@ fn loadTheme(self: *Config, theme: Theme) !void { try new_config.loadIter(alloc_gpa, &iter); // Setup our replay to be conditional. - for (new_config._replay_steps.items) |*item| switch (item.*) { - .expand => {}, - - // Change our arg to be conditional on our theme. - .arg => |v| { - const alloc_arena = new_config._arena.?.allocator(); - const conds = try alloc_arena.alloc(Conditional, 1); - conds[0] = .{ - .key = .theme, - .op = .eq, - .value = @tagName(self._conditional_state.theme), - }; - item.* = .{ .conditional_arg = .{ - .conditions = conds, - .arg = v, - } }; - }, + conditional: for (new_config._replay_steps.items) |*item| { + switch (item.*) { + .expand => {}, + + // If we see "-e" then we do NOT make the following arguments + // conditional since they are supposed to be part of the + // initial command. + .@"-e" => break :conditional, + + // Change our arg to be conditional on our theme. + .arg => |v| { + const alloc_arena = new_config._arena.?.allocator(); + const conds = try alloc_arena.alloc(Conditional, 1); + conds[0] = .{ + .key = .theme, + .op = .eq, + .value = @tagName(self._conditional_state.theme), + }; + item.* = .{ .conditional_arg = .{ + .conditions = conds, + .arg = v, + } }; + }, - .conditional_arg => |v| { - const alloc_arena = new_config._arena.?.allocator(); - const conds = try alloc_arena.alloc(Conditional, v.conditions.len + 1); - conds[0] = .{ - .key = .theme, - .op = .eq, - .value = @tagName(self._conditional_state.theme), - }; - @memcpy(conds[1..], v.conditions); - item.* = .{ .conditional_arg = .{ - .conditions = conds, - .arg = v.arg, - } }; - }, - }; + .conditional_arg => |v| { + const alloc_arena = new_config._arena.?.allocator(); + const conds = try alloc_arena.alloc(Conditional, v.conditions.len + 1); + conds[0] = .{ + .key = .theme, + .op = .eq, + .value = @tagName(self._conditional_state.theme), + }; + @memcpy(conds[1..], v.conditions); + item.* = .{ .conditional_arg = .{ + .conditions = conds, + .arg = v.arg, + } }; + }, + } + } // Replay our previous inputs so that we can override values // from the theme. @@ -2978,10 +3011,12 @@ pub fn parseManuallyHook( arg: []const u8, iter: anytype, ) !bool { - // Keep track of our input args no matter what.. - try self._replay_steps.append(alloc, .{ .arg = try alloc.dupe(u8, arg) }); - if (std.mem.eql(u8, arg, "-e")) { + // Add the special -e marker. This prevents: + // (1) config-file from adding args to the end (see #2908) + // (2) dark/light theme from making this conditional + try self._replay_steps.append(alloc, .@"-e"); + // Build up the command. We don't clean this up because we take // ownership in our allocator. var command = std.ArrayList(u8).init(alloc); @@ -3017,6 +3052,12 @@ pub fn parseManuallyHook( return false; } + // Keep track of our input args for replay + try self._replay_steps.append( + alloc, + .{ .arg = try alloc.dupe(u8, arg) }, + ); + // If we didn't find a special case, continue parsing normally return true; } @@ -3284,11 +3325,22 @@ const Replay = struct { arg: []const u8, }, + /// The start of a "-e" argument. This marks the end of + /// traditional configuration and the beginning of the + /// "-e" initial command magic. This is separate from "arg" + /// because there are some behaviors unique to this (i.e. + /// we want to keep this at the end for config-file). + /// + /// Note: when "-e" is used, ONLY this is present and + /// not an additional "arg" with "-e" value. + @"-e", + fn clone( self: Step, alloc: Allocator, ) Allocator.Error!Step { return switch (self) { + .@"-e" => self, .arg => |v| .{ .arg = try alloc.dupe(u8, v) }, .expand => |v| .{ .expand = try alloc.dupe(u8, v) }, .conditional_arg => |v| conditional: { @@ -3324,10 +3376,6 @@ const Replay = struct { log.warn("error expanding paths err={}", .{err}); }, - .arg => |arg| { - return arg; - }, - .conditional_arg => |v| conditional: { // All conditions must match. for (v.conditions) |cond| { @@ -3338,6 +3386,9 @@ const Replay = struct { return v.arg; }, + + .arg => |arg| return arg, + .@"-e" => return "-e", } } }