From 2be16d2242def62c49346d77afdb9f6b7ed27c3c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 21 Sep 2025 19:38:14 -0700 Subject: [PATCH] xdg: treat empty env vars as not existing (#8830) Replaces #8786 The author of the original PR used AI agents to create that PR. To the extent that this PR borrows code from that PR (mostly in the tests) AI was used in the creation of this PR. --- src/os/env.zig | 15 +++++ src/os/xdg.zig | 154 ++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 149 insertions(+), 20 deletions(-) diff --git a/src/os/env.zig b/src/os/env.zig index 1916053b3..8f01a14d0 100644 --- a/src/os/env.zig +++ b/src/os/env.zig @@ -95,6 +95,21 @@ pub fn getenv(alloc: Allocator, key: []const u8) Error!?GetEnvResult { }; } +/// Gets the value of an environment variable. Returns null if not found or the +/// value is empty. This will allocate on Windows but not on other platforms. +/// The returned value should have deinit called to do the proper cleanup no +/// matter what platform you are on. +pub fn getenvNotEmpty(alloc: Allocator, key: []const u8) !?GetEnvResult { + const result_ = try getenv(alloc, key); + if (result_) |result| { + if (result.value.len == 0) { + result.deinit(alloc); + return null; + } + } + return result_; +} + pub fn setenv(key: [:0]const u8, value: [:0]const u8) c_int { return switch (builtin.os.tag) { .windows => c._putenv_s(key.ptr, value.ptr), diff --git a/src/os/xdg.zig b/src/os/xdg.zig index 1383679fe..e120ed857 100644 --- a/src/os/xdg.zig +++ b/src/os/xdg.zig @@ -7,6 +7,7 @@ const assert = std.debug.assert; const Allocator = std.mem.Allocator; const posix = std.posix; const homedir = @import("homedir.zig"); +const env_os = @import("env.zig"); pub const Options = struct { /// Subdirectories to join to the base. This avoids extra allocations @@ -70,36 +71,22 @@ fn dir( // First check the env var. On Windows we have to allocate so this tracks // both whether we have the env var and whether we own it. // on Windows we treat `LOCALAPPDATA` as a fallback for `XDG_CONFIG_HOME` - const env_, const owned = switch (builtin.os.tag) { - else => .{ posix.getenv(internal_opts.env), false }, - .windows => windows: { - if (std.process.getEnvVarOwned(alloc, internal_opts.env)) |env| { - break :windows .{ env, true }; - } else |err| switch (err) { - error.EnvironmentVariableNotFound => { - if (std.process.getEnvVarOwned(alloc, internal_opts.windows_env)) |env| { - break :windows .{ env, true }; - } else |err2| switch (err2) { - error.EnvironmentVariableNotFound => break :windows .{ null, false }, - else => return err, - } - }, - else => return err, - } - }, + const env_ = try env_os.getenvNotEmpty(alloc, internal_opts.env) orelse switch (builtin.os.tag) { + else => null, + .windows => try env_os.getenvNotEmpty(alloc, internal_opts.windows_env), }; - defer if (owned) if (env_) |v| alloc.free(v); + defer if (env_) |env| env.deinit(alloc); if (env_) |env| { // If we have a subdir, then we use the env as-is to avoid a copy. if (opts.subdir) |subdir| { return try std.fs.path.join(alloc, &[_][]const u8{ - env, + env.value, subdir, }); } - return try alloc.dupe(u8, env); + return try alloc.dupe(u8, env.value); } // Get our home dir @@ -169,6 +156,133 @@ test "cache directory paths" { } } +test "fallback when xdg env empty" { + if (builtin.os.tag == .windows) return error.SkipZigTest; + + const alloc = std.testing.allocator; + + const saved_home = home: { + const home = std.posix.getenv("HOME") orelse break :home null; + break :home try alloc.dupeZ(u8, home); + }; + defer env: { + const home = saved_home orelse { + _ = env_os.unsetenv("HOME"); + break :env; + }; + _ = env_os.setenv("HOME", home); + std.testing.allocator.free(home); + } + const temp_home = "/tmp/ghostty-test-home"; + _ = env_os.setenv("HOME", temp_home); + + const DirCase = struct { + name: [:0]const u8, + func: fn (Allocator, Options) anyerror![]u8, + default_subdir: []const u8, + }; + + const cases = [_]DirCase{ + .{ .name = "XDG_CONFIG_HOME", .func = config, .default_subdir = ".config" }, + .{ .name = "XDG_CACHE_HOME", .func = cache, .default_subdir = ".cache" }, + .{ .name = "XDG_STATE_HOME", .func = state, .default_subdir = ".local/state" }, + }; + + inline for (cases) |case| { + // Save and restore each environment variable + const saved_env = blk: { + const value = std.posix.getenv(case.name) orelse break :blk null; + break :blk try alloc.dupeZ(u8, value); + }; + defer env: { + const value = saved_env orelse { + _ = env_os.unsetenv(case.name); + break :env; + }; + _ = env_os.setenv(case.name, value); + alloc.free(value); + } + + const expected = try std.fs.path.join(alloc, &[_][]const u8{ + temp_home, + case.default_subdir, + }); + defer alloc.free(expected); + + // Test with empty string - should fallback to home + _ = env_os.setenv(case.name, ""); + const actual = try case.func(alloc, .{}); + defer alloc.free(actual); + + try std.testing.expectEqualStrings(expected, actual); + } +} + +test "fallback when xdg env empty and subdir" { + if (builtin.os.tag == .windows) return error.SkipZigTest; + + const env = @import("env.zig"); + const alloc = std.testing.allocator; + + const saved_home = home: { + const home = std.posix.getenv("HOME") orelse break :home null; + break :home try alloc.dupeZ(u8, home); + }; + defer env: { + const home = saved_home orelse { + _ = env.unsetenv("HOME"); + break :env; + }; + _ = env.setenv("HOME", home); + std.testing.allocator.free(home); + } + + const temp_home = "/tmp/ghostty-test-home"; + _ = env.setenv("HOME", temp_home); + + const DirCase = struct { + name: [:0]const u8, + func: fn (Allocator, Options) anyerror![]u8, + default_subdir: []const u8, + }; + + const cases = [_]DirCase{ + .{ .name = "XDG_CONFIG_HOME", .func = config, .default_subdir = ".config" }, + .{ .name = "XDG_CACHE_HOME", .func = cache, .default_subdir = ".cache" }, + .{ .name = "XDG_STATE_HOME", .func = state, .default_subdir = ".local/state" }, + }; + + inline for (cases) |case| { + // Save and restore each environment variable + const saved_env = blk: { + const value = std.posix.getenv(case.name) orelse break :blk null; + break :blk try alloc.dupeZ(u8, value); + }; + defer env: { + const value = saved_env orelse { + _ = env.unsetenv(case.name); + break :env; + }; + _ = env.setenv(case.name, value); + alloc.free(value); + } + + const expected = try std.fs.path.join(alloc, &[_][]const u8{ + temp_home, + case.default_subdir, + "ghostty", + }); + defer alloc.free(expected); + + // Test with empty string - should fallback to home + _ = env.setenv(case.name, ""); + const actual = try case.func(alloc, .{ .subdir = "ghostty" }); + defer alloc.free(actual); + + try std.testing.expectEqualStrings(expected, actual); + } +} + test parseTerminalExec { const testing = std.testing;