Re: [PATCH v3] prune-packed: remove USE_THE_REPOSITORY_VARIABLE

[Patrick Steinhardt <ps@pks.im>]

On Sat, Oct 24, 2026 at 09:22:21AM +0000, Shashank S via GitGitGadget wrote:
> 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.

>      @@ 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.

>           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".

>      @@ 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.

> 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.

> +    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.

> +		      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