Fix review follow-ups (#39)

* wip

* wip

* wip

* wip

* wip

* wip

* wip

Author: gurgeous

src/args.zig

@@ -96,6 +96,7 @@ pub const Args = struct {
         if (std.mem.eql(u8, input, "min")) return .min;
         if (std.mem.eql(u8, input, "max")) return .max;
         const value = std.fmt.parseInt(usize, input, 10) catch return error.InvalidArgument;
+        // 0 works but is undocumented, this is fine
         if (value == 0) return .auto;
         return .{ .chars = value };
     }

src/data.zig

@@ -54,6 +54,7 @@ pub const DataRow = struct {
         var slow = false;
         var len: usize = 0;
         for (row_in, 0..) |field, ii| {
+            // Intentionally strip cells for nice display
             const str = util.strip(u8, field);
             row[ii] = str;
             if (hasControl(str)) {
@@ -73,6 +74,7 @@ pub const DataRow = struct {
         var slow = false;
         var len: usize = 0;
         for (col_order, 0..) |col, ii| {
+            // Intentionally strip cells for nice display
             const str = util.strip(u8, source[col]);
             row[ii] = str;
             if (hasControl(str)) slow = true;

src/detect.zig

@@ -47,6 +47,18 @@ pub fn formatFromFilename(path: []const u8) ?InputFormat {
     return null;
 }
 
+pub fn isSqliteFile(alloc: std.mem.Allocator, filename: ?[]const u8, input: std.fs.File) !bool {
+    // do we have an actual file?
+    const path = filename orelse return false;
+    if (std.mem.eql(u8, path, "-")) return false;
+
+    // sample the first few bytes to look for sqlite3 magic
+    var buf: [32]u8 = undefined;
+    const n = try input.readAll(&buf);
+    try input.seekTo(0);
+    return try detectFormat(alloc, path, buf[0..n]) == .sqlite;
+}
+
 //
 // testing
 //

src/main.zig

@@ -1,4 +1,8 @@
+//
+// main entrypoint
 // Owns process flow, input detection, loading, and top-level CLI behavior.
+//
+
 pub fn main() !void {
     var gpa: std.heap.DebugAllocator(.{}) = .init;
     defer {
@@ -7,28 +11,27 @@ pub fn main() !void {
     }
     const alloc = gpa.allocator();
 
-    if (main0(alloc) catch |err| switch (err) {
-        error.BrokenPipe, error.WriteFailed => {
-            std.process.exit(0);
-        },
-        else => return err,
-    }) |fatal| {
-        defer fatal.deinit(alloc);
-        try fatal.print();
-        flushPipe() catch |err| switch (err) {
-            error.BrokenPipe, error.WriteFailed => std.process.exit(0),
-            else => return err,
-        };
-        std.process.exit(1);
-    }
-    flushPipe() catch |err| switch (err) {
-        error.BrokenPipe, error.WriteFailed => std.process.exit(0),
+    var exit: u8 = 0;
+    const fatal = main0(alloc) catch |err| switch (err) {
+        error.BrokenPipe, error.WriteFailed => null,
         else => return err,
     };
-    std.process.exit(0);
+    if (fatal) |value| {
+        defer value.deinit(alloc);
+        try value.print();
+        exit = 1;
+    }
+
+    util.stdout.flush() catch {};
+    util.stderr.flush() catch {};
+    std.process.exit(exit);
 }
 
+//
+// main0
 // Run the CLI and return a printable failure when the command should fail.
+//
+
 fn main0(alloc: std.mem.Allocator) !?failure.Failure {
     // timer
     var total = try std.time.Timer.start();
@@ -102,27 +105,27 @@ fn main0(alloc: std.mem.Allocator) !?failure.Failure {
     // input => data rows
     var data = load(alloc, config, input) catch |err| {
         if (err == error.SqliteInvalidTable) {
-            const path = config.filename orelse return err;
-            var db = try sqlite.Sqlite.init(alloc, path);
+            var db = try sqlite.Sqlite.init(alloc, config.filename.?);
             defer db.deinit();
             return try failure.Failure.fromSqliteTableError(alloc, config.table, db.tables);
         }
         return failure.Failure.fromError(err) orelse return err;
     };
-    errdefer data.deinit(alloc);
 
     // plug data headers into config, for validation
     config.bind(alloc, data.headers()) catch |err| {
+        const fatal = try failure.Failure.fromTableError(alloc, err, data.headers());
+        data.deinit(alloc);
         config.deinit(alloc);
-        return try failure.Failure.fromTableError(alloc, err, data.headers());
+        return fatal;
     };
 
     //
     // data => table
     //
 
+    // Hand off both config and data here; Table.init owns cleanup from this point on.
     const table = try Table.init(alloc, config, data);
-    data = .{ .rows = &.{} };
     defer table.deinit();
     util.benchmark("table.init", timer.read());
 
@@ -141,21 +144,23 @@ fn main0(alloc: std.mem.Allocator) !?failure.Failure {
     return null;
 }
 
+//
+// loading input data
+//
+
 // Load the configured input into table data, dispatching by detected format.
 fn load(alloc: std.mem.Allocator, config: types.Config, input: std.fs.File) !Data {
     // typically we read the whole file into memory for processing. That won't
-    // work if we are using `sqlite3`, though
-    if (config.filename) |path| {
-        if (detect.formatFromFilename(path) == .sqlite) {
-            var db = try sqlite.Sqlite.init(alloc, path);
-            defer db.deinit();
-            return try db.load(config.table);
-        }
+    // work if we are using `sqlite3`, though.
+    if (try detect.isSqliteFile(alloc, config.filename, input)) {
+        var db = try sqlite.Sqlite.init(alloc, config.filename.?);
+        defer db.deinit();
+        return try db.load(config.table);
     }
 
-    const input_bytes = try input.readToEndAlloc(alloc, std.math.maxInt(usize));
-    defer alloc.free(input_bytes);
-    return try loadBytes(alloc, config, input_bytes);
+    const bytes = try input.readToEndAlloc(alloc, std.math.maxInt(usize));
+    defer alloc.free(bytes);
+    return try loadBytes(alloc, config, bytes);
 }
 
 // Load in-memory bytes into table data using the existing text format loaders.
@@ -166,22 +171,23 @@ fn loadBytes(alloc: std.mem.Allocator, config: types.Config, bytes_in: []const u
         bytes = bytes[3..];
     }
 
+    // sqlite3 and stray --table
     const format = try detect.detectFormat(alloc, config.filename, bytes);
-
-    // sqlite3 concerns
     if (format == .sqlite) return error.SqliteRequiresFile;
     if (config.table.len > 0) return error.SqliteTableRequiresSqlite;
+
+    // json
     if (format == .json) return try json.load(alloc, bytes);
 
+    // csv (our default)
     var delimiter = config.delimiter;
     if (delimiter == 0) delimiter = sniffer.sniff(bytes) orelse ',';
     return try csv.load(alloc, bytes, delimiter);
 }
 
-fn flushPipe() anyerror!void {
-    try util.stdout.flush();
-    try util.stderr.flush();
-}
+//
+// rendering
+//
 
 fn renderToPager(alloc: std.mem.Allocator, config: types.Config, table: *Table) !void {
     const cmd = std.posix.getenv("PAGER") orelse "less";

src/peek.zig

@@ -129,10 +129,7 @@ fn columnStats(alloc: std.mem.Allocator, table: *Table, c: usize, t: ColumnType)
 
     switch (t) {
         .int => {
-            var values: std.ArrayList(i64) = .empty;
-            defer values.deinit(alloc);
-            for (fields.items) |f| try values.append(alloc, try std.fmt.parseInt(i64, f, 10));
-            const mm = util.minmax(i64, values.items);
+            const mm = try parseIntMinmax(alloc, fields.items);
             return .{
                 .fill = fill,
                 .uniq = uniq,
@@ -189,6 +186,18 @@ fn fmtFill(alloc: std.mem.Allocator, fill: usize, nrows: usize) ![]u8 {
     return std.fmt.allocPrint(alloc, "{d}%", .{pct});
 }
 
+// Parse integer fields into i64 stats, returning null when any value overflows i64.
+fn parseIntMinmax(alloc: std.mem.Allocator, fields: []const Field) !?struct { min: i64, max: i64 } {
+    var values: std.ArrayList(i64) = .empty;
+    defer values.deinit(alloc);
+    for (fields) |field| {
+        const value = std.fmt.parseInt(i64, field, 10) catch continue;
+        try values.append(alloc, value);
+    }
+    if (util.minmax(i64, values.items)) |mm| return .{ .min = mm.min, .max = mm.max };
+    return null;
+}
+
 fn fmtIntValue(alloc: std.mem.Allocator, value: ?i64) ![]u8 {
     const num = value orelse return alloc.dupe(u8, dash);
     var buf: [32]u8 = undefined;
@@ -265,6 +274,19 @@ test "buildStatsTable reports basic visible stats" {
     try test_support.expectEqualRows(&.{ "city", "string", "66%", "2", "6 chars", "7 chars" }, stats.row(2));
 }
 
+test "buildStatsTable tolerates oversized ints in peek stats" {
+    const table = try Table.initCsv(testing.allocator, .{}, "id\n99999999999999999999\n");
+    defer table.deinit();
+
+    const stats = try buildStatsTable(testing.allocator, table);
+    defer stats.deinit();
+
+    try testing.expectEqualStrings("id", stats.row(0)[0]);
+    try testing.expectEqualStrings("int", stats.row(0)[1]);
+    try testing.expectEqualStrings("—", stats.row(0)[4]);
+    try testing.expectEqualStrings("—", stats.row(0)[5]);
+}
+
 test "buildStatsTable truncates float min and max to three digits" {
     const table = try Table.initCsv(testing.allocator, .{}, "score\n1.23456\n20.9999\n");
     defer table.deinit();

src/sqlite.zig

@@ -47,7 +47,9 @@ pub const Sqlite = struct {
         // --table
         if (selected_table.len > 0) {
             for (self.tables) |table| {
-                if (std.mem.eql(u8, table, selected_table)) return self.alloc.dupe(u8, table);
+                if (std.ascii.eqlIgnoreCase(table, selected_table)) {
+                    return self.alloc.dupe(u8, table);
+                }
             }
             return error.SqliteInvalidTable;
         }

src/util.zig

@@ -22,6 +22,13 @@ pub fn fileExists(path: []const u8) bool {
     return true;
 }
 
+// Report whether the file handle supports seeking to the current position.
+pub fn isSeekable(file: std.fs.File) bool {
+    const pos = file.getPos() catch return false;
+    file.seekTo(pos) catch return false;
+    return true;
+}
+
 // read a single byte from an fd
 pub fn readByte(fd: std.posix.fd_t) !u8 {
     var buf: [1]u8 = undefined;
@@ -197,25 +204,6 @@ pub fn upperAscii(dest: []u8, src: []const u8) []const u8 {
     return dest[0..src.len];
 }
 
-// write text truncated to width, using an ellipsis when needed
-pub fn truncate(writer: *std.Io.Writer, text: []const u8, stop: usize) !void {
-    if (stop == 0) return;
-
-    var it = std.unicode.Utf8View.init(text) catch {
-        try writer.writeAll(text[0..@min(text.len, stop)]);
-        return;
-    };
-    var iter = it.iterator();
-
-    var used: usize = 0;
-    while (iter.nextCodepointSlice()) |cp_slice| {
-        if (used + 1 >= stop) break;
-        try writer.writeAll(cp_slice);
-        used += 1;
-    }
-    try writer.writeAll("…");
-}
-
 //
 // misc
 //
@@ -277,6 +265,22 @@ test "minmax handles floats" {
     try testing.expectEqual(@as(f64, 9.25), got.max);
 }
 
+test "isSeekable handles file and pipe" {
+    var tmp = std.testing.tmpDir(.{});
+    defer tmp.cleanup();
+
+    const file = try tmp.dir.createFile("seekable.txt", .{ .read = true });
+    defer file.close();
+    try testing.expect(isSeekable(file));
+
+    const pipe_fds = try std.posix.pipe();
+    defer std.posix.close(pipe_fds[0]);
+    defer std.posix.close(pipe_fds[1]);
+
+    const pipe_file = std.fs.File{ .handle = pipe_fds[0] };
+    try testing.expect(!isSeekable(pipe_file));
+}
+
 test "plural returns the right form" {
     try testing.expectEqualStrings("row", plural(1, "row"));
     try testing.expectEqualStrings("rows", plural(2, "row"));
@@ -400,22 +404,6 @@ test "lowerAscii" {
     try testing.expectEqualStrings("ABC123", upperAscii(&buf, "AbC123"));
 }
 
-test "truncate" {
-    var buf: [256]u8 = undefined;
-    var writer = std.Io.Writer.fixed(&buf);
-
-    try truncate(&writer, "this is too long", 8);
-    try testing.expectEqualStrings("this is…", writer.buffered());
-    writer.end = 0;
-
-    try truncate(&writer, "éééé", 3);
-    try testing.expectEqualStrings("éé…", writer.buffered());
-    writer.end = 0;
-
-    try truncate(&writer, "abcdef", 0);
-    try testing.expectEqualStrings("", writer.buffered());
-}
-
 test "sum" {
     try testing.expectEqual(@as(usize, 10), sum(usize, &.{ 1, 2, 3, 4 }));
 }

testdata/smoke.bats

@@ -184,7 +184,19 @@ setup() {
 }
 
 @test "renders named sqlite table" {
-  run "$TENNIS_BIN" --color=off --width 80 --table players "$REPO_ROOT/testdata/sqlite-single.db"
+  run "$TENNIS_BIN" --color=off --width 80 --table PLAYERS "$REPO_ROOT/testdata/sqlite-single.db"
+  [ "$status" -eq 0 ]
+  [[ "$output" == *"name"* ]]
+  [[ "$output" == *"score"* ]]
+  [[ "$output" == *"alice"* ]]
+  [[ "$output" == *"cara"* ]]
+}
+
+@test "detects sqlite by magic bytes for unknown extensions" {
+  local db
+  db="$BATS_TEST_TMPDIR/sqlite-single.bin"
+  cp "$REPO_ROOT/testdata/sqlite-single.db" "$db"
+  run "$TENNIS_BIN" --color=off --width 80 "$db"
   [ "$status" -eq 0 ]
   [[ "$output" == *"name"* ]]
   [[ "$output" == *"score"* ]]