From c90ed293417add9bab0ac52ccef9eae0efd3e0cb Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Fri, 18 Oct 2024 12:53:32 -0700 Subject: [PATCH] cli: skip argv0 and actions when parsing CLI flags This fixes a regression from #2454. In that PR, we added an error when positional arguments are detected. I believe that's correct, but we were silently relying on the previous behavior in the CLI commands. This commit changes the CLI commands to use a new argsIterator function that creates an iterator that skips the first argument (argv0). This is the same behavior that the config parsing does and now uses this shared logic. This also makes it so the argsIterator ignores actions (`+things`) and we document that we expect those to be handled earlier. --- src/App.zig | 2 +- src/cli/args.zig | 41 +++++++++++++++++++++++++++++++++++++ src/cli/crash_report.zig | 4 ++-- src/cli/list_actions.zig | 2 +- src/cli/list_colors.zig | 2 +- src/cli/list_fonts.zig | 2 +- src/cli/list_keybinds.zig | 2 +- src/cli/list_themes.zig | 2 +- src/cli/show_config.zig | 2 +- src/cli/validate_config.zig | 3 +-- src/config/Config.zig | 14 +++---------- 11 files changed, 54 insertions(+), 22 deletions(-) diff --git a/src/App.zig b/src/App.zig index 0f9a0d89b..c54c67167 100644 --- a/src/App.zig +++ b/src/App.zig @@ -238,7 +238,7 @@ fn drainMailbox(self: *App, rt_app: *apprt.App) !void { // If we're quitting, then we set the quit flag and stop // draining the mailbox immediately. This lets us defer // mailbox processing to the next tick so that the apprt - // can try to quick as quickly as possible. + // can try to quit as quickly as possible. .quit => { log.info("quit message received, short circuiting mailbox drain", .{}); self.setQuit(); diff --git a/src/cli/args.zig b/src/cli/args.zig index 3dcc08dac..bfd40c633 100644 --- a/src/cli/args.zig +++ b/src/cli/args.zig @@ -4,6 +4,7 @@ const assert = std.debug.assert; const Allocator = mem.Allocator; const ArenaAllocator = std.heap.ArenaAllocator; const diags = @import("diagnostics.zig"); +const internal_os = @import("../os/main.zig"); const Diagnostic = diags.Diagnostic; const DiagnosticList = diags.DiagnosticList; @@ -894,6 +895,9 @@ test "parseIntoField: tagged union missing tag" { /// An iterator that considers its location to be CLI args. It /// iterates through an underlying iterator and increments a counter /// to track the current CLI arg index. +/// +/// This also ignores any argument that starts with `+`. It assumes that +/// actions were parsed out before this iterator was created. pub fn ArgsIterator(comptime Iterator: type) type { return struct { const Self = @This(); @@ -906,9 +910,21 @@ pub fn ArgsIterator(comptime Iterator: type) type { /// values yet. index: usize = 0, + pub fn deinit(self: *Self) void { + if (@hasDecl(Iterator, "deinit")) { + self.iterator.deinit(); + } + } + pub fn next(self: *Self) ?[]const u8 { const value = self.iterator.next() orelse return null; self.index += 1; + + // We ignore any argument that starts with "+". This is used + // to indicate actions and are expected to be parsed out before + // this iterator is created. + if (value.len > 0 and value[0] == '+') return self.next(); + return value; } @@ -919,6 +935,31 @@ pub fn ArgsIterator(comptime Iterator: type) type { }; } +/// Create an args iterator for the process args. This will skip argv0. +pub fn argsIterator(alloc_gpa: Allocator) internal_os.args.ArgIterator.InitError!ArgsIterator(internal_os.args.ArgIterator) { + var iter = try internal_os.args.iterator(alloc_gpa); + errdefer iter.deinit(); + _ = iter.next(); // skip argv0 + return .{ .iterator = iter }; +} + +test "ArgsIterator" { + const testing = std.testing; + + const child = try std.process.ArgIteratorGeneral(.{}).init( + testing.allocator, + "--what +list-things --a=42", + ); + const Iter = ArgsIterator(@TypeOf(child)); + var iter: Iter = .{ .iterator = child }; + defer iter.deinit(); + + try testing.expectEqualStrings("--what", iter.next().?); + try testing.expectEqualStrings("--a=42", iter.next().?); + try testing.expectEqual(@as(?[]const u8, null), iter.next()); + try testing.expectEqual(@as(?[]const u8, null), iter.next()); +} + /// Returns an iterator (implements "next") that reads CLI args by line. /// Each CLI arg is expected to be a single line. This is used to implement /// configuration files. diff --git a/src/cli/crash_report.zig b/src/cli/crash_report.zig index 0ec6a8ce0..dd5fe99cc 100644 --- a/src/cli/crash_report.zig +++ b/src/cli/crash_report.zig @@ -33,9 +33,9 @@ pub fn run(alloc_gpa: Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc_gpa); defer iter.deinit(); - try args.parse(Options, alloc, &opts, &iter); + try args.parse(Options, alloc_gpa, &opts, &iter); } const crash_dir = try crash.defaultDir(alloc); diff --git a/src/cli/list_actions.zig b/src/cli/list_actions.zig index c6a5cf240..8dbadc65a 100644 --- a/src/cli/list_actions.zig +++ b/src/cli/list_actions.zig @@ -29,7 +29,7 @@ pub fn run(alloc: Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc); defer iter.deinit(); try args.parse(Options, alloc, &opts, &iter); } diff --git a/src/cli/list_colors.zig b/src/cli/list_colors.zig index b9a250519..bfe17df7c 100644 --- a/src/cli/list_colors.zig +++ b/src/cli/list_colors.zig @@ -22,7 +22,7 @@ pub fn run(alloc: std.mem.Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc); defer iter.deinit(); try args.parse(Options, alloc, &opts, &iter); } diff --git a/src/cli/list_fonts.zig b/src/cli/list_fonts.zig index 397c85064..aba596b64 100644 --- a/src/cli/list_fonts.zig +++ b/src/cli/list_fonts.zig @@ -53,7 +53,7 @@ pub const Config = struct { /// specific styles. It is not guaranteed that only those styles are returned, /// it will just prioritize fonts that match those styles. pub fn run(alloc: Allocator) !u8 { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc); defer iter.deinit(); return try runArgs(alloc, &iter); } diff --git a/src/cli/list_keybinds.zig b/src/cli/list_keybinds.zig index 7e0bbd692..ccd6dfd21 100644 --- a/src/cli/list_keybinds.zig +++ b/src/cli/list_keybinds.zig @@ -52,7 +52,7 @@ pub fn run(alloc: Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc); defer iter.deinit(); try args.parse(Options, alloc, &opts, &iter); } diff --git a/src/cli/list_themes.zig b/src/cli/list_themes.zig index feec6fcb0..9782951db 100644 --- a/src/cli/list_themes.zig +++ b/src/cli/list_themes.zig @@ -91,7 +91,7 @@ pub fn run(gpa_alloc: std.mem.Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(gpa_alloc); + var iter = try args.argsIterator(gpa_alloc); defer iter.deinit(); try args.parse(Options, gpa_alloc, &opts, &iter); } diff --git a/src/cli/show_config.zig b/src/cli/show_config.zig index e3f1341e8..cbcd2486d 100644 --- a/src/cli/show_config.zig +++ b/src/cli/show_config.zig @@ -60,7 +60,7 @@ pub fn run(alloc: Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc); defer iter.deinit(); try args.parse(Options, alloc, &opts, &iter); } diff --git a/src/cli/validate_config.zig b/src/cli/validate_config.zig index ef1dd3ecc..1615ef66b 100644 --- a/src/cli/validate_config.zig +++ b/src/cli/validate_config.zig @@ -32,7 +32,7 @@ pub fn run(alloc: std.mem.Allocator) !u8 { defer opts.deinit(); { - var iter = try std.process.argsWithAllocator(alloc); + var iter = try args.argsIterator(alloc); defer iter.deinit(); try args.parse(Options, alloc, &opts, &iter); } @@ -46,7 +46,6 @@ pub fn run(alloc: std.mem.Allocator) !u8 { if (opts.@"config-file") |config_path| { var buf: [std.fs.max_path_bytes]u8 = undefined; const abs_path = try std.fs.cwd().realpath(config_path, &buf); - try cfg.loadFile(alloc, abs_path); try cfg.loadRecursiveFiles(alloc); } else { diff --git a/src/config/Config.zig b/src/config/Config.zig index 6c21b6dfc..9097051ad 100644 --- a/src/config/Config.zig +++ b/src/config/Config.zig @@ -2365,18 +2365,10 @@ pub fn loadCliArgs(self: *Config, alloc_gpa: Allocator) !void { counter[i] = @field(self, field).list.items.len; } - // Initialize our CLI iterator. The first argument is always assumed - // to be the program name so we skip over that. - var iter = try internal_os.args.iterator(alloc_gpa); + // Initialize our CLI iterator. + var iter = try cli.args.argsIterator(alloc_gpa); defer iter.deinit(); - if (iter.next()) |argv0| log.debug("skipping argv0 value={s}", .{argv0}); - - // Parse the config from the CLI args - { - const ArgsIter = cli.args.ArgsIterator(@TypeOf(iter)); - var args_iter: ArgsIter = .{ .iterator = iter }; - try self.loadIter(alloc_gpa, &args_iter); - } + try self.loadIter(alloc_gpa, &iter); // If we are not loading the default files, then we need to // replay the steps up to this point so that we can rebuild