Re: [PATCH 4/9] repack: refactor piping an oid to a command

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

Christian Couder <christian.couder@gmail.com> writes:

> Create a new write_oid_hex_cmd() function to send an oid to the standard
> input of a running command. This new function will be used in a
> following commit.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  builtin/repack.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index 0541c3ce15..e591c295cf 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -182,6 +182,17 @@ static void prepare_pack_objects(struct child_process *cmd,
>  	cmd->out = -1;
>  }
>  
> +static void write_oid_hex_cmd(const char *oid_hex,
> +			      struct child_process *cmd,
> +			      const char *err_msg)
> +{
> +	if (cmd->in == -1 && start_command(cmd))
> +		die("%s", err_msg);

I am not sure why we would want to conflate the "if we haven't
started the command, auto-start it upon our first attempt to write"
in these low-level "I am designed to do one thing, which is to feed
the object name to the process, and do it well" function.

The caller in the original shares the same issue, so we could say
that this patch is not creating a new problem, but this somehow
feels it is mak ng the existing problem even worse.

And I think the error handling here shows why the API feels wrong.
When auto-start fails, we have a message, but when write fails,
there is no custom message---it makes as if write_oid_hex_cmd() is
primarily about starting, which is so important relative to its
other functionalities and deserves a custom error message, but that
is not the message you want to be conveying.

> +	xwrite(cmd->in, oid_hex, the_hash_algo->hexsz);
> +	xwrite(cmd->in, "\n", 1);

I would have expected that the "refactor" at least would reduce the
number of system calls by combining these two writes into one using
an on-stack local variable char buf[GIT_MAX_HEZSZ+1] or something.