Re: [PATCH v1 3/4] builtin/repack.c: change xwrite to write_in_full to allow large sizes.
From: Jeff King <hidden>
Date: 2024-02-27 08:20:28
On Mon, Feb 26, 2024 at 05:05:37PM -0500, Randall S. Becker wrote:
From: "Randall S. Becker" <redacted> This change is required because some platforms do not support file writes of arbitrary sizes (e.g, NonStop). xwrite ends up truncating the output to the maximum single I/O size possible for the destination device. The result of write_in_full() is also passed to the caller, which was previously ignored.
These are going to be tiny compared to single-write() I/O limits, I'd think, but in general we should be on guard for the OS returning short reads (this is a pipe and so for most systems PIPE_BUF would guarantee atomicity, I think, but IMHO it is simpler to just make things obviously-correct by looping with write_in_full). So I'd be surprised if this spot was the cause of a visible bug, but I think it's worth changing regardless. The error detection is a separate question, though. I think it is good to check the result of the write here, as an error here means that the child pack-objects misses some objects we wanted it to pack, which could lead to a corrupt repository. But I don't think what you have here is quite enough:
quoted hunk ↗ jump to hunk
@@ -314,8 +315,12 @@ static int write_oid(const struct object_id *oid, die(_("could not start pack-objects to repack promisor objects")); } - xwrite(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); - xwrite(cmd->in, "\n", 1); + err = write_in_full(cmd->in, oid_to_hex(oid), the_hash_algo->hexsz); + if (err <= 0) + return err; + err = write_in_full(cmd->in, "\n", 1); + if (err <= 0) + return err; return 0;
OK, so we detect the error and return it to the caller. Who is the
caller? The only use of this function is in repack_promisor_objects(),
which calls:
for_each_packed_object(write_oid, &cmd,
FOR_EACH_OBJECT_PROMISOR_ONLY);
So when we return the error, now for_each_packed_object() will stop
traversing, and propagate that error up to the caller. But as we can see
above, the caller ignores it!
So I think you'd either want to die directly (perhaps using
write_or_die). Or you'd need to additionally check the return from
for_each_packed_object(). That would also catch cases where that
function failed to open a pack (I'm not sure how important that is to
this code).
But as it is, your patch just causes a write error to truncate the list
of oids send to the child process (though that is probably not
materially different from the current behavior, as the subsequent calls
would presumably fail, too).
-Peff