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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Jun 16, 2023 at 1:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

Ok. I think I will rework this patch from version 2 of this series to
remove that code. It will perhaps look like there is a bit of
duplicated code, but I don't think it will be too bad.

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

Right.

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

Ok, I will change it to reduce the number of system calls.