| Message ID | c0f4ec92a057fdab905447bb917ff09e9bcaaab3.1666636974.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | repack: implement `--expire-to` option | expand |
Taylor Blau <me@ttaylorr.com> writes: > 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. > - 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? > @@ -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.
On Mon, Oct 24, 2022 at 02:30:49PM -0700, Junio C Hamano wrote: > > - 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? It would be nice if it were possible to avoid passing `names` entirely here, but we still need it to determine the set of packs we already wrote a few lines above when we write the input to `pack-objects --cruft`. Thanks, Taylor
On 10/24/22 2:43 PM, Taylor Blau wrote: > @@ -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); > + /* This line looked oddly out-of-alignment with the next one. It seems that the comment is preceded by spaces and not tabs. Perhaps Junio fixed this during his application of the patch to keep the builds happy. > + * avoid putting packs written outside of the repository in the > + * list of names > + */ > + if (local) > + string_list_append(names, line.buf); LGTM. Thanks, -Stolee
On Mon, Nov 07, 2022 at 02:32:05PM -0500, Derrick Stolee wrote: > On 10/24/22 2:43 PM, Taylor Blau wrote: > > @@ -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); > > + /* > > This line looked oddly out-of-alignment with the next one. > > It seems that the comment is preceded by spaces and not > tabs. Perhaps Junio fixed this during his application of > the patch to keep the builds happy. Odd. In my copy, which I got from gitster/git before Junio went offline, the alignment looks fine. Thanks for double checking, though. Thanks, Taylor
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); - 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); } fclose(out); @@ -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);
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(-)