| Message ID | 1dd4136f6199ac050cec5eb671c36ae05fbf3bdd.1666636974.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4e7b65ba8e7b4174c4ab249b64c6cb3ad0009732 |
| Headers | show |
| Series | repack: implement `--expire-to` option | expand |
Taylor Blau <me@ttaylorr.com> writes: > `builtin/repack.c`'s `prepare_pack_objects()` is used to prepare a set > of arguments to a `pack-objects` process which will generate a desired > pack. > > A future patch will add an `--expire-to` option which allows `git > repack` to write a cruft pack containing the pruned objects out to a > separate repository. Prepare for this by teaching that function to write > packs to an arbitrary location specified by the caller. > > All existing callers of `prepare_pack_objects()` will pass `packtmp` for > `out`, retaining the existing behavior. It does make sense to allow the caller to specify the name of the temporary file to be used, but is "out" a good name for that? The other two arguments are self explanatory both by their type and the name, but this is of type "const char *" that does not convey what the string is about at all, so giging a good name to the parameter is more important than for others. The patch text itself is very straight-forward. Thanks. > static void prepare_pack_objects(struct child_process *cmd, > - const struct pack_objects_args *args) > + const struct pack_objects_args *args, > + const char *out) > { > strvec_push(&cmd->args, "pack-objects"); > if (args->window) > @@ -211,7 +212,7 @@ static void prepare_pack_objects(struct child_process *cmd, > strvec_push(&cmd->args, "--quiet"); > if (delta_base_offset) > strvec_push(&cmd->args, "--delta-base-offset"); > - strvec_push(&cmd->args, packtmp); > + strvec_push(&cmd->args, out); > cmd->git_cmd = 1; > cmd->out = -1; > } > @@ -275,7 +276,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args, > FILE *out; > struct strbuf line = STRBUF_INIT; > > - prepare_pack_objects(&cmd, args); > + prepare_pack_objects(&cmd, args, packtmp); > cmd.in = -1; > > /* > @@ -673,7 +674,7 @@ static int write_cruft_pack(const struct pack_objects_args *args, > FILE *in, *out; > int ret; > > - prepare_pack_objects(&cmd, args); > + prepare_pack_objects(&cmd, args, packtmp); > > strvec_push(&cmd.args, "--cruft"); > if (cruft_expiration) > @@ -861,7 +862,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > > sigchain_push_common(remove_pack_on_signal); > > - prepare_pack_objects(&cmd, &po_args); > + prepare_pack_objects(&cmd, &po_args, packtmp); > > show_progress = !po_args.quiet && isatty(2);
On 10/24/22 4:47 PM, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > >> `builtin/repack.c`'s `prepare_pack_objects()` is used to prepare a set >> of arguments to a `pack-objects` process which will generate a desired >> pack. >> >> A future patch will add an `--expire-to` option which allows `git >> repack` to write a cruft pack containing the pruned objects out to a >> separate repository. Prepare for this by teaching that function to write >> packs to an arbitrary location specified by the caller. >> >> All existing callers of `prepare_pack_objects()` will pass `packtmp` for >> `out`, retaining the existing behavior. > > It does make sense to allow the caller to specify the name of the > temporary file to be used, but is "out" a good name for that? The > other two arguments are self explanatory both by their type and the > name, but this is of type "const char *" that does not convey what > the string is about at all, so giging a good name to the parameter > is more important than for others. > > The patch text itself is very straight-forward. Thanks. I agree that the patch is nice and simple. As for a name, this parameter specifies a file prefix. Perhaps 'pack_prefix' would be a good name for this? Thanks, -Stolee
diff --git a/builtin/repack.c b/builtin/repack.c index a5bacc7797..0a7bd57636 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -188,7 +188,8 @@ static void remove_redundant_pack(const char *dir_name, const char *base_name) } static void prepare_pack_objects(struct child_process *cmd, - const struct pack_objects_args *args) + const struct pack_objects_args *args, + const char *out) { strvec_push(&cmd->args, "pack-objects"); if (args->window) @@ -211,7 +212,7 @@ static void prepare_pack_objects(struct child_process *cmd, strvec_push(&cmd->args, "--quiet"); if (delta_base_offset) strvec_push(&cmd->args, "--delta-base-offset"); - strvec_push(&cmd->args, packtmp); + strvec_push(&cmd->args, out); cmd->git_cmd = 1; cmd->out = -1; } @@ -275,7 +276,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args, FILE *out; struct strbuf line = STRBUF_INIT; - prepare_pack_objects(&cmd, args); + prepare_pack_objects(&cmd, args, packtmp); cmd.in = -1; /* @@ -673,7 +674,7 @@ static int write_cruft_pack(const struct pack_objects_args *args, FILE *in, *out; int ret; - prepare_pack_objects(&cmd, args); + prepare_pack_objects(&cmd, args, packtmp); strvec_push(&cmd.args, "--cruft"); if (cruft_expiration) @@ -861,7 +862,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) sigchain_push_common(remove_pack_on_signal); - prepare_pack_objects(&cmd, &po_args); + prepare_pack_objects(&cmd, &po_args, packtmp); show_progress = !po_args.quiet && isatty(2);
`builtin/repack.c`'s `prepare_pack_objects()` is used to prepare a set of arguments to a `pack-objects` process which will generate a desired pack. A future patch will add an `--expire-to` option which allows `git repack` to write a cruft pack containing the pruned objects out to a separate repository. Prepare for this by teaching that function to write packs to an arbitrary location specified by the caller. All existing callers of `prepare_pack_objects()` will pass `packtmp` for `out`, retaining the existing behavior. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/repack.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)