Re: [PATCH v2 1/3] builtin/repack.c: change xwrite to write_in_full and report errors.

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2024, 18:49 UTC
Message-ID: <xmqq5xy9spxi.fsf@gitster.g>
In-Reply-To: <20240227150934.7950-2-randall.becker@nexbridge.ca>

"Randall S. Becker" <the.n.e.key@gmail.com> writes:

Show 6 quoted lines

> From: "Randall S. Becker" <rsbecker@nexbridge.com>
>
> 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.

This misleads readers to think that maximum single I/O size is smaller than a single write of oid_to_hex() string on some platforms. I somehow do not think that is why we want to make this change.

Rather, the use of these xwrites() are simply wrong regardless of maximum I/O size of the platforms, as this caller is not prepared to see xwrite() result in a short write(2), and we do want to write all bytes we have even in such a case.

You're right to also point out that we attempt to propagate the errors to the caller (but see below).

Show 31 quoted lines

> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
>  builtin/repack.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ede36328a3..932d24c60b 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -307,6 +307,7 @@ static int write_oid(const struct object_id *oid,
>  		     struct packed_git *pack UNUSED,
>  		     uint32_t pos UNUSED, void *data)
>  {
> +	int err;
>  	struct child_process *cmd = data;
>  
>  	if (cmd->in == -1) {
> @@ -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;
>  }

I think this has already been brought up, but the caller of this helper does not make such an error stand out enough and instead makes the resulting repack silently produce wrong result, which is not an improvement. Perhaps

	if (write_in_full(...) ||
	    write_in_full(...))
		die(_("failed to list promisor objects to repack"));

or something?

Thanks.