| Message ID | 20230614192541.1599256-5-christian.couder@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Repack objects into separate packfiles based on a filter | expand |
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.
On Thu, Jun 15, 2023 at 04:46:43PM -0700, Junio C Hamano 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. I agree, the implementation of `write_oid_hex_cmd()` seems too magical to me. Perhaps there was some awkwardness with using the pre-image w.r.t some later change? Let's see... Thanks, Taylor
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.
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); + + xwrite(cmd->in, oid_hex, the_hash_algo->hexsz); + xwrite(cmd->in, "\n", 1); +} + /* * Write oid to the given struct child_process's stdin, starting it first if * necessary. @@ -192,13 +203,8 @@ static int write_oid(const struct object_id *oid, { struct child_process *cmd = data; - if (cmd->in == -1) { - if (start_command(cmd)) - 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); + write_oid_hex_cmd(oid_to_hex(oid), cmd, + _("could not start pack-objects to repack promisor objects")); return 0; }
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(-)