Re: [PATCH 3/4] builtin/repack.c: write cruft packs to arbitrary locations
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 24, 2022, 21:30 UTC
- Message-ID
- <xmqq1qqwsyd2.fsf@gitster.g>
- In-Reply-To
- <c0f4ec92a057fdab905447bb917ff09e9bcaaab3.1666636974.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 31 quoted lines
> In the following commit, a new write_cruft_pack() caller will be added > which wants to write a cruft pack to an arbitrary location. Prepare for > this by adding a parameter which controls the destination of the cruft > pack. > > For now, provide "packtmp" so that this commit does not change any > behavior. > > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > builtin/repack.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index 1184e8c257..a5386ac893 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -662,6 +662,7 @@ static int write_midx_included_packs(struct string_list *include, > } > > static int write_cruft_pack(const struct pack_objects_args *args, > + const char *destination, > const char *pack_prefix, > const char *cruft_expiration, > struct string_list *names, > @@ -673,8 +674,10 @@ static int write_cruft_pack(const struct pack_objects_args *args, > struct string_list_item *item; > FILE *in, *out; > int ret; > + const char *scratch; > + int local = skip_prefix(destination, packdir, &scratch);
Hmph. In an earlier step we got rid of the hardcoded assumption on where the packtmp is, and we are passing destination in (where the existing callers call us with packtmp) to make it even better, but we acquire the dependency on packdir global with this step. It's just a couple of file-scope static global variables, so it is not a huge deal.
Show 17 quoted lines
> - prepare_pack_objects(&cmd, args, packtmp);
> + prepare_pack_objects(&cmd, args, destination);
>
> strvec_push(&cmd.args, "--cruft");
> if (cruft_expiration)
> @@ -714,7 +717,12 @@ static int write_cruft_pack(const struct pack_objects_args *args,
> if (line.len != the_hash_algo->hexsz)
> die(_("repack: Expecting full hex object ID lines only "
> "from pack-objects."));
> - string_list_append(names, line.buf);
> + /*
> + * avoid putting packs written outside of the repository in the
> + * list of names
> + */
> + if (local)
> + string_list_append(names, line.buf);
> }Even if we do not want to contaminate the "names" list with packs that are not in the repository, wouldn't our caller still want to be able to tell what packs they are?
What I am wondering is if it makes more sense to have the caller pass &names (which can be NULL to just discard the output from the pack-objects command) so that this function can concentrate on what it does (i.e. formulate the command to write cruft packs and then report the packs that are created), without having to worry about the management of the &names thing, which can be done by the caller of this function? We are already passing &names, so it may be the matter of caller deciding to pass &names or NULL based on the value of destination it passes to the function?
Show 9 quoted lines
> @@ -986,7 +994,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > cruft_po_args.local = po_args.local; > cruft_po_args.quiet = po_args.quiet; > > - ret = write_cruft_pack(&cruft_po_args, pack_prefix, > + ret = write_cruft_pack(&cruft_po_args, packtmp, pack_prefix, > cruft_expiration, &names, > &existing_nonkept_packs, > &existing_kept_packs);
For example, this callsite will always want to pass &names because it is always about local pack, right?
Thanks.