Commit 4151e6c31b50ffaab0c9b68106f7903c3d99c810 | zig

author	Igor Anić <igor.anic@gmail.com> 2024-04-09 14:47:34+02:00
committer	Igor Anić <igor.anic@gmail.com> 2024-04-09 15:00:22+02:00
log	4151e6c31b50ffaab0c9b68106f7903c3d99c810
tree	a93c7b4b946a3e3508ebfc3938a97dea16886d46
parent	f8dd2a1064154e5e8223c4d52e123b582497a40d

fetch: use arena allocator for diagnostic/UnpackResult

Reference:
https://github.com/ziglang/zig/pull/19500#discussion_r1556476973

Arena is now used for Diagnostic (tar and git). `deinit` is not called on Diagnostic
allowing us to reference strings from Diagnostic in UnpackResult without
dupe.

That seamed reasonable to me. Instead of using gpa for Diagnostic, and
then dupe to arena. Or using arena for both and making dupe so we can deinit
Diagnostic.

1 files changed, 87 insertions(+), 81 deletions(-)

src
- Package
  - Fetch.zig

src/Package/Fetch.zig+87-81

 @@ -463,7 +463,6 @@ fn runResource(
         // Fetch and unpack a resource into a temporary directory.
         var unpack_result = try unpackResource(f, resource, uri_path, tmp_directory);
-        defer unpack_result.deinit();
         var pkg_path: Cache.Path = .{ .root_dir = tmp_directory, .sub_path = unpack_result.root_dir };
 @@ -487,11 +486,7 @@ fn runResource(
         // Ignore errors that were excluded by manifest, such as failure to
         // create symlinks that weren't supposed to be included anyway.
-        try unpack_result.filterErrors(filter);
+        try unpack_result.validate(f, filter);
-        if (unpack_result.hasErrors()) {
-            try unpack_result.bundleErrors(eb, try f.srcLoc(f.location_tok));
-            return error.FetchFailed;
-        }
         // Apply the manifest's inclusion rules to the temporary directory by
         // deleting excluded files.
 @@ -1117,8 +1112,7 @@ fn unpackResource(
                     .{ uri_path, @errorName(err) },
                 ));
             };
-            const gpa = f.arena.child_allocator;
+            return .{};
-            return UnpackResult.init(gpa);
         },
     };
 @@ -1166,10 +1160,9 @@ fn unpackResource(
 fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackResult {
     const eb = &f.error_bundle;
-    const gpa = f.arena.child_allocator;
+    const arena = f.arena.allocator();
-    var diagnostics: std.tar.Diagnostics = .{ .allocator = gpa };
+    var diagnostics: std.tar.Diagnostics = .{ .allocator = arena };
-    defer diagnostics.deinit();
     std.tar.pipeToFileSystem(out_dir, reader, .{
         .diagnostics = &diagnostics,
 @@ -1181,17 +1174,14 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes
         .{@errorName(err)},
     ));
-    var res = UnpackResult.init(gpa);
+    var res: UnpackResult = .{ .root_dir = diagnostics.root_dir };
-    if (diagnostics.root_dir.len > 0) {
-        res.root_dir = try gpa.dupe(u8, diagnostics.root_dir);
-    }
     if (diagnostics.errors.items.len > 0) {
-        try res.rootErrorMessage("unable to unpack tarball");
+        try res.allocErrors(arena, diagnostics.errors.items.len, "unable to unpack tarball");
         for (diagnostics.errors.items) |item| {
             switch (item) {
-                .unable_to_create_file => |i| try res.unableToCreateFile(stripRoot(i.file_name, res.root_dir), i.code),
+                .unable_to_create_file => |i| res.unableToCreateFile(stripRoot(i.file_name, res.root_dir), i.code),
-                .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(stripRoot(i.file_name, res.root_dir), i.link_name, i.code),
+                .unable_to_create_sym_link => |i| res.unableToCreateSymLink(stripRoot(i.file_name, res.root_dir), i.link_name, i.code),
-                .unsupported_file_type => |i| try res.unsupportedFileType(stripRoot(i.file_name, res.root_dir), @intFromEnum(i.file_type)),
+                .unsupported_file_type => |i| res.unsupportedFileType(stripRoot(i.file_name, res.root_dir), @intFromEnum(i.file_type)),
             }
         }
     }
 @@ -1199,11 +1189,12 @@ fn unpackTarball(f: *Fetch, out_dir: fs.Dir, reader: anytype) RunError!UnpackRes
 }
 fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!UnpackResult {
+    const arena = f.arena.allocator();
     const gpa = f.arena.child_allocator;
     const want_oid = resource.git.want_oid;
     const reader = resource.git.fetch_stream.reader();
-    var res = UnpackResult.init(gpa);
+    var res: UnpackResult = .{};
     // The .git directory is used to store the packfile and associated index, but
     // we do not attempt to replicate the exact structure of a real .git
     // directory, since that isn't relevant for fetching a package.
 @@ -1234,16 +1225,15 @@ fn unpackGitPack(f: *Fetch, out_dir: fs.Dir, resource: *Resource) anyerror!Unpac
             checkout_prog_node.activate();
             var repository = try git.Repository.init(gpa, pack_file, index_file);
             defer repository.deinit();
-            var diagnostics: git.Diagnostics = .{ .allocator = gpa };
+            var diagnostics: git.Diagnostics = .{ .allocator = arena };
-            defer diagnostics.deinit();
             try repository.checkout(out_dir, want_oid, &diagnostics);
             if (diagnostics.errors.items.len > 0) {
-                try res.rootErrorMessage("unable to unpack packfile");
+                try res.allocErrors(arena, diagnostics.errors.items.len, "unable to unpack packfile");
                 for (diagnostics.errors.items) |item| {
                     switch (item) {
-                        .unable_to_create_file => |i| try res.unableToCreateFile(i.file_name, i.code),
+                        .unable_to_create_file => |i| res.unableToCreateFile(i.file_name, i.code),
-                        .unable_to_create_sym_link => |i| try res.unableToCreateSymLink(i.file_name, i.link_name, i.code),
+                        .unable_to_create_sym_link => |i| res.unableToCreateSymLink(i.file_name, i.link_name, i.code),
                     }
                 }
             }
 @@ -1701,6 +1691,7 @@ const native_os = builtin.os.tag;
 test {
     _ = Filter;
     _ = FileType;
+    _ = UnpackResult;
 }
 // Detects executable header: ELF magic header or shebang line.
 @@ -1741,8 +1732,8 @@ test FileHeader {
 // tar/git diagnostic, filtering that errors by manifest inclusion rules and
 // emitting remaining errors to an `ErrorBundle`.
 const UnpackResult = struct {
-    allocator: std.mem.Allocator,
+    errors: []Error = undefined,
-    errors: std.ArrayListUnmanaged(Error) = .{},
+    errors_count: usize = 0,
     root_error_message: []const u8 = "",
     // A non empty value means that the package contents are inside a
 @@ -1772,97 +1763,82 @@ const UnpackResult = struct {
             };
             return !filter.includePath(file_name);
         }
-        fn free(self: Error, allocator: std.mem.Allocator) void {
-            switch (self) {
-                .unable_to_create_sym_link => |info| {
-                    allocator.free(info.file_name);
-                    allocator.free(info.link_name);
-                },
-                .unable_to_create_file => |info| {
-                    allocator.free(info.file_name);
-                },
-                .unsupported_file_type => |info| {
-                    allocator.free(info.file_name);
-                },
-            }
-        }
     };
-    fn init(allocator: std.mem.Allocator) UnpackResult {
+    fn allocErrors(self: *UnpackResult, arena: std.mem.Allocator, n: usize, root_error_message: []const u8) !void {
-        return .{ .allocator = allocator };
+        self.root_error_message = try arena.dupe(u8, root_error_message);
-    }
+        self.errors = try arena.alloc(UnpackResult.Error, n);
-    fn deinit(self: *UnpackResult) void {
-        for (self.errors.items) |item| {
-            item.free(self.allocator);
-        }
-        self.errors.deinit(self.allocator);
-        self.allocator.free(self.root_error_message);
-        self.allocator.free(self.root_dir);
-        self.* = undefined;
     }
     fn hasErrors(self: *UnpackResult) bool {
-        return self.errors.items.len > 0;
+        return self.errors_count > 0;
     }
-    fn unableToCreateFile(self: *UnpackResult, file_name: []const u8, err: anyerror) !void {
+    fn unableToCreateFile(self: *UnpackResult, file_name: []const u8, err: anyerror) void {
-        try self.errors.append(self.allocator, .{ .unable_to_create_file = .{
+        self.errors[self.errors_count] = .{ .unable_to_create_file = .{
             .code = err,
-            .file_name = try self.allocator.dupe(u8, file_name),
+            .file_name = file_name,
-        } });
+        } };
+        self.errors_count += 1;
     }
-    fn unableToCreateSymLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) !void {
+    fn unableToCreateSymLink(self: *UnpackResult, file_name: []const u8, link_name: []const u8, err: anyerror) void {
-        try self.errors.append(self.allocator, .{ .unable_to_create_sym_link = .{
+        self.errors[self.errors_count] = .{ .unable_to_create_sym_link = .{
             .code = err,
-            .file_name = try self.allocator.dupe(u8, file_name),
+            .file_name = file_name,
-            .link_name = try self.allocator.dupe(u8, link_name),
+            .link_name = link_name,
-        } });
+        } };
+        self.errors_count += 1;
     }
-    fn unsupportedFileType(self: *UnpackResult, file_name: []const u8, file_type: u8) !void {
+    fn unsupportedFileType(self: *UnpackResult, file_name: []const u8, file_type: u8) void {
-        try self.errors.append(self.allocator, .{ .unsupported_file_type = .{
+        self.errors[self.errors_count] = .{ .unsupported_file_type = .{
-            .file_name = try self.allocator.dupe(u8, file_name),
+            .file_name = file_name,
             .file_type = file_type,
-        } });
+        } };
+        self.errors_count += 1;
+    }
+    fn validate(self: *UnpackResult, f: *Fetch, filter: Filter) !void {
+        self.filterErrors(filter);
+        if (self.hasErrors()) {
+            const eb = &f.error_bundle;
+            try self.bundleErrors(eb, try f.srcLoc(f.location_tok));
+            return error.FetchFailed;
+        }
     }
     // Filter errors by manifest inclusion rules.
-    fn filterErrors(self: *UnpackResult, filter: Filter) !void {
+    fn filterErrors(self: *UnpackResult, filter: Filter) void {
-        var i = self.errors.items.len;
+        var i = self.errors_count;
         while (i > 0) {
             i -= 1;
-            const item = self.errors.items[i];
+            if (self.errors[i].excluded(filter)) {
-            if (item.excluded(filter)) {
+                self.errors_count -= 1;
-                _ = self.errors.swapRemove(i);
+                const tmp = self.errors[i];
-                item.free(self.allocator);
+                self.errors[i] = self.errors[self.errors_count];
+                self.errors[self.errors_count] = tmp;
             }
         }
     }
-    fn rootErrorMessage(self: *UnpackResult, msg: []const u8) !void {
-        self.root_error_message = try self.allocator.dupe(u8, msg);
-    }
     // Emmit errors to an `ErrorBundle`.
     fn bundleErrors(
         self: *UnpackResult,
         eb: *ErrorBundle.Wip,
         src_loc: ErrorBundle.SourceLocationIndex,
     ) !void {
-        if (self.errors.items.len == 0 and self.root_error_message.len == 0)
+        if (self.errors_count == 0 and self.root_error_message.len == 0)
             return;
-        const notes_len: u32 = @intCast(self.errors.items.len);
+        const notes_len: u32 = @intCast(self.errors_count);
         try eb.addRootErrorMessage(.{
             .msg = try eb.addString(self.root_error_message),
             .src_loc = src_loc,
             .notes_len = notes_len,
         });
         const notes_start = try eb.reserveNotes(notes_len);
-        for (self.errors.items, notes_start..) |item, note_i| {
+        for (self.errors, notes_start..) |item, note_i| {
             switch (item) {
                 .unable_to_create_sym_link => |info| {
                     eb.extra.items[note_i] = @intFromEnum(try eb.addErrorMessage(.{
 @@ -1888,6 +1864,36 @@ const UnpackResult = struct {
             }
         }
     }
+    test filterErrors {
+        var arena_instance = std.heap.ArenaAllocator.init(std.testing.allocator);
+        defer arena_instance.deinit();
+        const arena = arena_instance.allocator();
+        // init
+        var res: UnpackResult = .{};
+        try res.allocErrors(arena, 4, "error");
+        try std.testing.expectEqual(0, res.errors_count);
+        // create errors
+        res.unableToCreateFile("dir1/file1", error.File1);
+        res.unableToCreateSymLink("dir2/file2", "", error.File2);
+        res.unableToCreateFile("dir1/file3", error.File3);
+        res.unsupportedFileType("dir2/file4", 'x');
+        try std.testing.expectEqual(4, res.errors_count);
+        // filter errors
+        var filter: Filter = .{};
+        try filter.include_paths.put(arena, "dir2", {});
+        res.filterErrors(filter);
+        try std.testing.expectEqual(2, res.errors_count);
+        try std.testing.expect(res.errors[0] == Error.unsupported_file_type);
+        try std.testing.expect(res.errors[1] == Error.unable_to_create_sym_link);
+        // filtered: moved to the list end
+        try std.testing.expect(res.errors[2] == Error.unable_to_create_file);
+        try std.testing.expect(res.errors[3] == Error.unable_to_create_file);
+    }
 };
 test "tarball with duplicate paths" {