Re: [PATCH v3] prune-packed: remove USE_THE_REPOSITORY_VARIABLE
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 26, 2026, 08:38 UTC
- Message-ID
- <at8RfoXGrC6s3km_@pks.im>
- In-Reply-To
- <pull.2266.v3.git.1792833741913.gitgitgadget@gmail.com>
On Sat, Oct 24, 2026 at 09:22:21AM +0000, Shashank S via GitGitGadget wrote:
Show 9 quoted lines
> Range-diff vs v2: > > 1: 6b583e1d46 ! 1: 0b474e3433 [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE > @@ Metadata > Author: Shashank S Mogaveera <shashu8660@gmail.com> > > ## Commit message ## > - [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE > + prune-packed: remove USE_THE_REPOSITORY_VARIABLE
Good.
Show 7 quoted lines
> @@ Commit message > to hold both of them together, and update the prune_subdir() and > prune_object() callbacks to read from this new struct. > > - Update the builtins that call this function (builtin/prune-packed.c, > - builtin/prune.c, and builtin/repack.c) to pass their local 'repo' > + Update the builtins that call this function to pass their local 'repo'
Yup.
Show 6 quoted lines
> variables. We can safely remove the UNUSED macro from them because > - these commands are all declared with the RUN_SETUP flag in git.c, > - which guarantees that the 'repo' pointer will never be NULL. > + while 'repo' may be NULL if the command is invoked with "-h", the > + call to parse_options() will give the short help and exit before the > + dereference occurs.
This still doesn't quite put the finger on what's important. What you need to verify is that there is nothing between the `cmd_foobar()` entrypoint and the call to `parse_options()` that may dereference the NULL pointer. How abouT this:
Update the builtins that call this function to pass their local
`repo` parameters. As all of those builtins are declared with
`RUN_SETUP` we know that they won't execute outside a repository
and thus the repository parameter will be set. Unless the command
was invoked with "-h" to print help, but all new uses of the `repo`
variable are after a call to `parse_options()` which knows to exit
early in case it sees "-h".Show 9 quoted lines
> @@ builtin/prune-packed.c: int cmd_prune_packed(int argc, > > ## builtin/prune.c ## > @@ builtin/prune.c: int cmd_prune(int argc, > - for_each_loose_file_in_source(repo->objects->sources, > + for_each_loose_file_in_source(repo->objects->source, > prune_object, prune_cruft, prune_subdir, &revs); > > - prune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);
Okay, seems like you switched the base of this topic. That's important to point out as part of the changes to the previous version.
Show 55 quoted lines
> diff --git a/prune-packed.c b/prune-packed.c
> index b6749b8d13..e02d5d7a01 100644
> --- a/prune-packed.c
> +++ b/prune-packed.c
> @@ -8,42 +6,52 @@
> #include "prune-packed.h"
> #include "repository.h"
>
> +struct prune_data {
> + struct repository *r;
> + int opts;
> +};
> +
> static struct progress *progress;
>
> static int prune_subdir(unsigned int nr, const char *path, void *data)
> {
> - int *opts = data;
> + struct prune_data *d = data;
> display_progress(progress, nr + 1);
> - if (!(*opts & PRUNE_PACKED_DRY_RUN))
> + if (!(d->opts & PRUNE_PACKED_DRY_RUN))
> rmdir(path);
> return 0;
> }
>
> static int prune_object(const struct object_id *oid, const char *path,
> - void *data)
> + void *data)
> {
> - int *opts = data;
> + struct prune_data *d = data;
>
> - if (!has_object_pack(the_repository, oid))
> + if (!has_object_pack(d->r, oid))
> return 0;
>
> - if (*opts & PRUNE_PACKED_DRY_RUN)
> + if (d->opts & PRUNE_PACKED_DRY_RUN)
> printf("rm -f %s\n", path);
> else
> unlink_or_warn(path);
> return 0;
> }
>
> -void prune_packed_objects(int opts)
> +void prune_packed_objects(struct repository *r, int opts)
> {
> - if (opts & PRUNE_PACKED_VERBOSE)
> - progress = start_delayed_progress(the_repository,
> - _("Removing duplicate objects"), 256);
> + struct prune_data cb_data = {
> + .r = r,
> + .opts = opts
> + };Indentation is wrong here now, even though it was correct in v2.
Show 7 quoted lines
> + if (opts & PRUNE_PACKED_VERBOSE)
> + progress = start_delayed_progress(r,
> + _("Removing duplicate objects"), 256);
>
> - for_each_loose_file_in_source(the_repository->objects->source,
> - prune_object, NULL, prune_subdir, &opts);
> + for_each_loose_file_in_source(r->objects->source,Likewise, this is using spaces for indentation now.
Show 8 quoted lines
> + prune_object, NULL, prune_subdir, &cb_data); > > - /* Ensure we show 100% before finishing progress */ > - display_progress(progress, 256); > - stop_progress(&progress); > + /* Ensure we show 100% before finishing progress */ > + display_progress(progress, 256); > + stop_progress(&progress);
Same here.
Patrick