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

[Junio C Hamano <gitster@pobox.com>]

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

> 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).

> 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.