On Mon, Feb 26, 2024 at 05:05:37PM -0500, Randall S. Becker wrote:
> From: "Randall S. Becker" <rsbecker@xxxxxxxxxxxxx>
>
> 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:
> @@ -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
