refactor: use command.command_names index and improve logging of commands

We now use the command name index more consistently to allow for
pre-allocating command IDs and better logging when commands are not
found.

This is a major, but hopefully non-breaking, change to command execution.

src/command.zig

@@ -3,6 +3,7 @@ const tp = @import("thespian");
 const log = @import("log");
 const cbor = @import("cbor");
 
+pub var log_execute: bool = false;
 pub var context_check: ?*const fn () void = null;
 
 pub const ID = usize;
@@ -69,15 +70,10 @@ pub fn Closure(comptime T: type) type {
         }
 
         pub fn register(self: *Self) !void {
-            if (command_names.get(self.vtbl.name)) |id| {
+            if (command_names.get(self.vtbl.name)) |id|
-                self.vtbl.id = id;
+                reAddCommand(id, &self.vtbl) catch |e| return log.err("cmd", "reAddCommand", e)
-                reAddCommand(&self.vtbl) catch |e| return log.err("cmd", "reAddCommand", e);
+            else
-                // log.print("cmd", "reAddCommand({s}) => {d}", .{ self.vtbl.name, self.vtbl.id });
+                addCommand(&self.vtbl);
-            } else {
-                self.vtbl.id = try addCommand(&self.vtbl);
-                command_names.put(self.vtbl.name, self.vtbl.id) catch |e| return log.err("cmd", "addCommand", e);
-                // log.print("cmd", "addCommand({s}) => {d}", .{ self.vtbl.name, self.vtbl.id });
-            }
         }
 
         pub fn unregister(self: *Self) void {
@@ -100,21 +96,31 @@ pub var commands: CommandTable = .empty;
 var command_names: std.StringHashMap(ID) = std.StringHashMap(ID).init(command_table_allocator);
 const command_table_allocator = std.heap.c_allocator;
 
-fn addCommand(cmd: *Vtable) !ID {
+fn assignCommandId(name: []const u8) ID {
-    try commands.append(command_table_allocator, cmd);
+    commands.append(command_table_allocator, null) catch |e| std.debug.panic("assignCommandId: {t}", .{e});
-    return commands.items.len - 1;
+    const id = commands.items.len - 1;
+    command_names.put(name, id) catch |e| std.debug.panic("assignCommandId: {t}", .{e});
+    return id;
 }
 
-fn reAddCommand(cmd: *Vtable) !void {
+fn addCommand(cmd: *Vtable) void {
-    if (commands.items[cmd.id] != null) return error.DuplicateCommand;
+    commands.append(command_table_allocator, cmd) catch |e| std.debug.panic("addCommand: {t}", .{e});
-    commands.items[cmd.id] = cmd;
+    const id = commands.items.len - 1;
+    cmd.id = id;
+    command_names.put(cmd.name, id) catch |e| std.debug.panic("assignCommandId: {t}", .{e});
+}
+
+fn reAddCommand(id: ID, cmd: *Vtable) !void {
+    cmd.id = id;
+    if (commands.items[id] != null) return error.DuplicateCommand;
+    commands.items[id] = cmd;
 }
 
 pub fn removeCommand(id: ID) void {
     commands.items[id] = null;
 }
 
-pub fn execute(id: ID, ctx: Context) tp.result {
+pub fn execute(id: ID, name: []const u8, ctx: Context) tp.result {
     if (tp.env.get().enabled(tp.channel.debug)) trace: {
         var iter = ctx.args.buf;
         var len = cbor.decodeArrayHeader(&iter) catch break :trace;
@@ -140,20 +146,26 @@ pub fn execute(id: ID, ctx: Context) tp.result {
     }
     if (context_check) |check| check();
     if (id >= commands.items.len)
-        return tp.exit_fmt("CommandNotFound: {d}", .{id});
+        return notFoundError(id, name);
     const cmd = commands.items[id];
     if (cmd) |p| {
-        // var buf: [tp.max_message_size]u8 = undefined;
+        if (log_execute) {
-        // log.print("cmd", "execute({s}) {s}", .{ p.name, ctx.args.to_json(&buf) catch "" }) catch |e| return tp.exit_error(e, @errorReturnTrace());
+            var buf: [tp.max_message_size]u8 = undefined;
+            log.print("cmd", "execute({d}) {s} {s}", .{ id, p.name, if (ctx.args.buf.len > 0) ctx.args.to_json(&buf) catch "(error)" else "" });
+        }
         return p.run(p, ctx);
     } else {
-        return tp.exit_fmt("CommandNotAvailable: {d}", .{id});
+        return notFoundError(id, name);
     }
 }
 
 pub fn get_id(name: []const u8) ?ID {
-    var id: ?ID = null;
+    const id = get_name_id(name);
-    return get_id_cache(name, &id);
+    return if (commands.items[id]) |_| id else null;
+}
+
+pub fn get_name_id(name: []const u8) ID {
+    return command_names.get(name) orelse assignCommandId(name);
 }
 
 pub fn get_name(id: ID) ?[]const u8 {
@@ -164,19 +176,17 @@ pub fn get_name(id: ID) ?[]const u8 {
             tp.trace(tp.channel.debug, .{ "command", "get_name", "null", id });
     }
     if (id >= commands.items.len) return null;
-    return (commands.items[id] orelse return null).name;
+    if (commands.items[id]) |cmd| return cmd.name;
+    var iter = command_names.iterator();
+    while (iter.next()) |kv| if (kv.value_ptr.* == id)
+        return kv.key_ptr.*;
+    return null;
 }
 
-pub fn get_id_cache(name: []const u8, id: *?ID) ?ID {
+pub fn get_id_cache(name: []const u8, cached_id: *?ID) ID {
-    for (commands.items) |cmd| {
+    const id = get_name_id(name);
-        if (cmd) |p|
+    cached_id.* = id;
-            if (std.mem.eql(u8, p.name, name)) {
+    return id;
-                id.* = p.id;
-                return p.id;
-            };
-    }
-    tp.trace(tp.channel.debug, .{ "command", "get_id_cache", "failed", name });
-    return null;
 }
 
 pub fn get_description(id: ID) ?[]const u8 {
@@ -201,14 +211,12 @@ const suppressed_errors = std.StaticStringMap(void).initComptime(.{
 });
 
 pub fn executeName(name: []const u8, ctx: Context) tp.result {
-    const id = get_id(name);
+    return execute(get_name_id(name), name, ctx);
-    if (id) |id_| return execute(id_, ctx);
-    return notFoundError(name);
 }
 
-pub fn notFoundError(name: []const u8) !void {
+fn notFoundError(id: ID, name: []const u8) !void {
     if (!suppressed_errors.has(name))
-        return tp.exit_fmt("CommandNotFound: {s}", .{name});
+        return tp.exit_fmt("CommandNotFound: {s}({d})", .{ name, id });
 }
 
 fn CmdDef(comptime T: type) type {

src/keybind/keybind.zig

@@ -349,19 +349,17 @@ const Command = struct {
 
     fn execute(self: *@This()) !void {
         const id = self.command_id orelse
-            command.get_id_cache(self.command, &self.command_id) orelse {
+            command.get_id_cache(self.command, &self.command_id);
-            return command.notFoundError(self.command);
-        };
         var buf: [2048]u8 = undefined;
         @memcpy(buf[0..self.args.len], self.args);
         if (integer_argument) |int_arg| {
             if (cbor.match(self.args, .{}) catch false and has_integer_argument(id)) {
                 integer_argument = null;
-                try command.execute(id, command.fmt(.{int_arg}));
+                try command.execute(id, self.command, command.fmt(.{int_arg}));
                 return;
             }
         }
-        try command.execute(id, .{ .args = .{ .buf = buf[0..self.args.len] } });
+        try command.execute(id, self.command, .{ .args = .{ .buf = buf[0..self.args.len] } });
     }
 
     fn execute_const(self: *const @This()) void {
@@ -662,11 +660,9 @@ const BindingSet = struct {
             if (enable_insert_events)
                 self.send_insert_event(globals.insert_command, globals.input_buffer.items);
             const id = globals.insert_command_id orelse
-                command.get_id_cache(globals.insert_command, &globals.insert_command_id) orelse {
+                command.get_id_cache(globals.insert_command, &globals.insert_command_id);
-                return tp.exit_error(error.InputTargetNotFound, null);
-            };
             if (!builtin.is_test) {
-                try command.execute(id, command.fmt(.{globals.input_buffer.items}));
+                try command.execute(id, globals.insert_command, command.fmt(.{globals.input_buffer.items}));
             }
         }
     }

src/tui/editor.zig

@@ -5734,7 +5734,7 @@ pub const Editor = struct {
     pub fn goto_next_diagnostic(self: *Self, _: Context) Result {
         if (self.diagnostics.items.len == 0) {
             if (command.get_id("goto_next_file")) |id|
-                return command.execute(id, .{});
+                return command.execute(id, "goto_next_file", .{});
             return;
         }
         self.sort_diagnostics();
@@ -5750,7 +5750,7 @@ pub const Editor = struct {
     pub fn goto_prev_diagnostic(self: *Self, _: Context) Result {
         if (self.diagnostics.items.len == 0) {
             if (command.get_id("goto_prev_file")) |id|
-                return command.execute(id, .{});
+                return command.execute(id, "goto_prev_file", .{});
             return;
         }
         self.sort_diagnostics();
@@ -6224,7 +6224,7 @@ pub const Editor = struct {
     }
 
     fn add_default_symbol_triggers(self: *Self) void {
-        const id = command.get_id("completion") orelse return;
+        const id = command.get_name_id("completion");
         self.add_symbol_trigger('.', id, .insert) catch {};
     }
 

src/tui/tui.zig

@@ -416,7 +416,7 @@ fn receive_safe(self: *Self, from: tp.pid_ref, m: tp.message) !void {
     if (try m.match(.{ "cmd", tp.extract(&cmd) }))
         return command.executeName(cmd, ctx) catch |e| self.logger.err(cmd, e);
     if (try m.match(.{ "cmd", tp.extract(&cmd_id) }))
-        return command.execute(cmd_id, ctx) catch |e| self.logger.err("command", e);
+        return command.execute(cmd_id, command.get_name(cmd_id) orelse "(unknown)", ctx) catch |e| self.logger.err("command", e);
 
     var arg: []const u8 = undefined;
 
@@ -426,7 +426,7 @@ fn receive_safe(self: *Self, from: tp.pid_ref, m: tp.message) !void {
     }
     if (try m.match(.{ "cmd", tp.extract(&cmd_id), tp.extract_cbor(&arg) })) {
         ctx.args = .{ .buf = arg };
-        return command.execute(cmd_id, ctx) catch |e| self.logger.err("command", e);
+        return command.execute(cmd_id, command.get_name(cmd_id) orelse "(unknown)", ctx) catch |e| self.logger.err("command", e);
     }
     if (try m.match(.{"quit"})) {
         project_manager.shutdown();
@@ -736,7 +736,7 @@ fn dispatch_event(ctx: *anyopaque, cbor_msg: []const u8) void {
 
 fn handle_system_clipboard(self: *Self, text: []const u8) !void {
     if (command.get_id("mini_mode_paste")) |id|
-        return command.execute(id, command.fmt(.{text}));
+        return command.execute(id, "mini_mode_paste", command.fmt(.{text}));
 
     {
         const text_ = try clipboard_system_clipboard_text(self.allocator);