prune-packed: remove USE_THE_REPOSITORY_VARIABLE

PATCH Superseded Shashank S via GitGitGadget 4 participants 14 messages Oct 18 – Oct 28

lore

Share & tools

lore

Shashank S via GitGitGadget

author

2026-10-18 16:31 UTC #0 lore raw

PATCH 1/1 prune-packed: remove USE_THE_REPOSITORY_VARIABLE

From: Shashank <[email protected]>

Right now, the prune_packed_objects() function uses the global the_repository variable. To help with the libification effort, change this function so it takes a 'struct repository' pointer as an argument instead.

The function for_each_loose_file_in_source() uses a callback that only takes a single 'void *data' pointer. This means we cannot pass both the repository pointer and the 'opts' integer at the same time. To fix this, create a new 'struct prune_data' 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 variables. Because we are now actually using the 'repo' parameter in these files, we can safely remove the UNUSED macro from them.

finally, drop the USE_THE_REPOSITORY_VARIABLE macro from prune-packed.c.

Signed-off-by Shashank <[email protected]>

---
[Outreachy PATCH] prune-packed: remove use_the_repository_variable

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2266%2Fshashu8660%2Foutreachy2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2266/shashu8660/outreachy2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2266

 builtin/prune-packed.c |  4 ++--
 builtin/prune.c        |  2 +-
 builtin/repack.c       |  2 +-
 prune-packed.c         | 32 ++++++++++++++++++++------------
 prune-packed.h         |  4 +++-
 5 files changed, 27 insertions(+), 17 deletions(-)

builtin/prune-packed.c +2 -2

index 4d63f26b0a..d44e7f81d4 100644
@@ -11,7 +11,7 @@ static const char * const prune_packed_usage[] = {
 int cmd_prune_packed(int argc,
 		     const char **argv,
 		     const char *prefix,
-		     struct repository *repo UNUSED)
+		     struct repository *repo)
 {
 	int opts = isatty(2) ? PRUNE_PACKED_VERBOSE : 0;
 	const struct option prune_packed_options[] = {

@@ -30,6 +30,6 @@ int cmd_prune_packed(int argc,
 			      prune_packed_usage,
 			      prune_packed_options);
 
-	prune_packed_objects(opts);
+	prune_packed_objects(repo, opts);
 	return 0;
 }

builtin/prune.c +1 -1

index a7e4678d11..4eee5a2e27 100644
@@ -201,7 +201,7 @@ int cmd_prune(int argc,
 	for_each_loose_file_in_source(repo->objects->sources,
 				      prune_object, prune_cruft, prune_subdir, &revs);
 
-	prune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);
+	prune_packed_objects(repo, show_only ? PRUNE_PACKED_DRY_RUN : 0);
 	remove_temporary_files(repo_get_object_directory(repo));
 	s = mkpathdup("%s/pack", repo_get_object_directory(repo));
 	remove_temporary_files(s);

builtin/repack.c +1 -1

index c4360382c1..e8970e8eb7 100644
@@ -757,7 +757,7 @@ int cmd_repack(int argc,
 						       wrote_incremental_midx);
 		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
-		prune_packed_objects(opts);
+		prune_packed_objects(repo, opts);
 
 		if (!keep_unreachable &&
 		    (!(pack_everything & LOOSEN_UNREACHABLE) ||

prune-packed.c +20 ~~-12~~

index d49dc11957..a53b7031b5 100644
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "gettext.h"
 #include "object-file.h"

@@ -8,40 +6,50 @@
 #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)
 {
+	struct prune_data cb_data = {
+		.r = r,
+		.opts = opts
+	};
+
 	if (opts & PRUNE_PACKED_VERBOSE)
-		progress = start_delayed_progress(the_repository,
+		progress = start_delayed_progress(r,
 						  _("Removing duplicate objects"), 256);
 
-	for_each_loose_file_in_source(the_repository->objects->sources,
-				      prune_object, NULL, prune_subdir, &opts);
+	for_each_loose_file_in_source(r->objects->sources,
+				      prune_object, NULL, prune_subdir, &cb_data);
 
 	/* Ensure we show 100% before finishing progress */
 	display_progress(progress, 256);

prune-packed.h +3 -1

index 936fa9df23..9ec7ff0d60 100644
@@ -1,9 +1,11 @@
 #ifndef PRUNE_PACKED_H
 #define PRUNE_PACKED_H
 
+struct repository;
+
 #define PRUNE_PACKED_DRY_RUN 01
 #define PRUNE_PACKED_VERBOSE 02
 
-void prune_packed_objects(int);
+void prune_packed_objects(struct repository *r, int opts);
 
 #endif

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2026-10-19 07:09 UTC #1 lore raw

Quoted 3 lines · Shashank S via GitGitGadget

From: Shashank <[email protected]>
Right now, the prune_packed_objects() function uses the global

Nit: we typically refrain from saying "now", as it's implicitly clear that you're describing the status quo in your commit message unless you explicitly refer to an older commit.

Quoted 11 lines

the_repository variable. To help with the libification effort, change this function so it takes a 'struct repository' pointer as an argument instead.
The function for_each_loose_file_in_source() uses a callback that only takes a single 'void *data' pointer. This means we cannot pass both the repository pointer and the 'opts' integer at the same time. To fix this, create a new 'struct prune_data' to hold both of them together, and update the prune_subdir() and prune_object() callbacks to read from this new struct.

Quoted 4 lines

Update the builtins that call this function (builtin/prune-packed.c, builtin/prune.c, and builtin/repack.c) to pass their local repo variables. Because we are now actually using the 'repo' parameter in these files, we can safely remove the UNUSED macro from them.

The question isn't really whether we can safely remove the UNUSED macro. The question is rather whether we can assume that repo always points to a proper repository. And for that you need to research whether these commands can be invoked outside of a repository, because if so the repo may be NULL.

Quoted 1 line

finally, drop the USE_THE_REPOSITORY_VARIABLE macro from

s/finally/Finally

Quoted 3 lines

prune-packed.c.
Signed-off-by Shashank <[email protected]>

We typically ask contributors to spell out their full name, see Documentation/SubmittingPatches [[real-name]].

The changes all look sensible to me, thanks!

Patrick

Shashank S

2026-10-19 16:39 UTC #2 lore raw

Quoted 7 lines · Patrick Steinhardt

Quoted 1 line · Shashank S via GitGitGadget
Right now, the prune_packed_objects() function uses the global
Nit: we typically refrain from saying "now", as it's implicitly clear that you're describing the status quo in your commit message unless you explicitly refer to an older commit.

thank you for the review and point the mistakes i have done i will correct all the mistake and i will submit the changed commit in the V2 patch.

Quoted 8 lines

Quoted 1 line
in these files, we can safely remove the UNUSED macro from them.
The question isn't really whether we can safely remove the UNUSED macro. The question is rather whether we can assume that repo always points to a proper repository. And for that you need to research whether these commands can be invoked outside of a repository, because if so the repo may be NULL.

I have checked git.c file in that file I went through this commands . prune , prune-packed and repack which are .
{ "prune", cmd_prune, RUN_SETUP },
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
{ "repack", cmd_repack, RUN_SETUP },
All this commands are all registered with a special flags ' RUN_SETUP'. This flags search for .git folder . If it finds a .git folder Git successfully initializes the repo struct and passes that to valid pointer to your builtin . If it does not find a .git folder (if they ran it outside the repository) Git immediately stops and shows the error . It never run the code .

Quoted 3 lines

Quoted 1 line
finally, drop the USE_THE_REPOSITORY_VARIABLE macro from
s/finally/Finally

i will change this too.

Quoted 11 lines

Quoted 4 lines
prune-packed.c.
Signed-off-by Shashank <[email protected]>
We typically ask contributors to spell out their full name, see Documentation/SubmittingPatches [[real-name]].
The changes all look sensible to me, thanks!
Patrick

Thank you

Shashank S via GitGitGadget

author

2026-10-19 16:53 UTC #5 lore raw

PATCH v2 1/1 prune-packed: remove USE_THE_REPOSITORY_VARIABLE

In reply to Shashank S via GitGitGadget

From: Shashank S Mogaveera <[email protected]>

The prune_packed_objects() function uses the global the_repository variable. To help with the libification effort, change this function so it takes a 'struct repository' pointer as an argument instead.

Update the builtins that call this function (builtin/prune-packed.c, builtin/prune.c, and builtin/repack.c) to pass their local 'repo' 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.

Finally, drop the USE_THE_REPOSITORY_VARIABLE macro from prune-packed.c.

Signed-off-by Shashank S Mogaveera <[email protected]>

---
[Outreachy PATCH] prune-packed: remove use_the_repository_variable

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2266%2Fshashu8660%2Foutreachy2-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2266/shashu8660/outreachy2-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2266

Range-diff vs v1:

1: 5d9ba0cf3a ! 1: 6b583e1d46 prune-packed: remove USE_THE_REPOSITORY_VARIABLE
@@
## Metadata ##
-Author: Shashank <[email protected]>
+Author: Shashank S Mogaveera <[email protected]>

## Commit message ##
- prune-packed: remove USE_THE_REPOSITORY_VARIABLE
+ [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE

- Right now, the prune_packed_objects() function uses the global
+ The prune_packed_objects() function uses the global
the_repository variable. To help with the libification effort, change this function so it takes a 'struct repository' pointer as an argument instead.
@@ Commit message
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
- variables. Because we are now actually using the 'repo' parameter
- in these files, we can safely remove the UNUSED macro from them.
+ builtin/prune.c, and builtin/repack.c) to pass their local 'repo'
+ 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.

- finally, drop the USE_THE_REPOSITORY_VARIABLE macro from
+ Finally, drop the USE_THE_REPOSITORY_VARIABLE macro from
prune-packed.c.

- Signed-off-by: Shashank <[email protected]>
+ Signed-off-by: Shashank S Mogaveera <[email protected]>

## builtin/prune-packed.c ##
@@ builtin/prune-packed.c: static const char * const prune_packed_usage[] = {

 builtin/prune-packed.c |  4 ++--
 builtin/prune.c        |  2 +-
 builtin/repack.c       |  2 +-
 prune-packed.c         | 32 ++++++++++++++++++++------------
 prune-packed.h         |  4 +++-
 5 files changed, 27 insertions(+), 17 deletions(-)

builtin/prune-packed.c +2 -2

index 4d63f26b0a..d44e7f81d4 100644
@@ -11,7 +11,7 @@ static const char * const prune_packed_usage[] = {
 int cmd_prune_packed(int argc,
 		     const char **argv,
 		     const char *prefix,
-		     struct repository *repo UNUSED)
+		     struct repository *repo)
 {
 	int opts = isatty(2) ? PRUNE_PACKED_VERBOSE : 0;
 	const struct option prune_packed_options[] = {

@@ -30,6 +30,6 @@ int cmd_prune_packed(int argc,
 			      prune_packed_usage,
 			      prune_packed_options);
 
-	prune_packed_objects(opts);
+	prune_packed_objects(repo, opts);
 	return 0;
 }

builtin/prune.c +1 -1

index a7e4678d11..4eee5a2e27 100644
@@ -201,7 +201,7 @@ int cmd_prune(int argc,
 	for_each_loose_file_in_source(repo->objects->sources,
 				      prune_object, prune_cruft, prune_subdir, &revs);
 
-	prune_packed_objects(show_only ? PRUNE_PACKED_DRY_RUN : 0);
+	prune_packed_objects(repo, show_only ? PRUNE_PACKED_DRY_RUN : 0);
 	remove_temporary_files(repo_get_object_directory(repo));
 	s = mkpathdup("%s/pack", repo_get_object_directory(repo));
 	remove_temporary_files(s);

builtin/repack.c +1 -1

index c4360382c1..e8970e8eb7 100644
@@ -757,7 +757,7 @@ int cmd_repack(int argc,
 						       wrote_incremental_midx);
 		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
-		prune_packed_objects(opts);
+		prune_packed_objects(repo, opts);
 
 		if (!keep_unreachable &&
 		    (!(pack_everything & LOOSEN_UNREACHABLE) ||

prune-packed.c +20 ~~-12~~

index d49dc11957..a53b7031b5 100644
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "gettext.h"
 #include "object-file.h"

@@ -8,40 +6,50 @@
 #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)
 {
+	struct prune_data cb_data = {
+		.r = r,
+		.opts = opts
+	};
+
 	if (opts & PRUNE_PACKED_VERBOSE)
-		progress = start_delayed_progress(the_repository,
+		progress = start_delayed_progress(r,
 						  _("Removing duplicate objects"), 256);
 
-	for_each_loose_file_in_source(the_repository->objects->sources,
-				      prune_object, NULL, prune_subdir, &opts);
+	for_each_loose_file_in_source(r->objects->sources,
+				      prune_object, NULL, prune_subdir, &cb_data);
 
 	/* Ensure we show 100% before finishing progress */
 	display_progress(progress, 256);

prune-packed.h +3 -1

index 936fa9df23..9ec7ff0d60 100644
@@ -1,9 +1,11 @@
 #ifndef PRUNE_PACKED_H
 #define PRUNE_PACKED_H
 
+struct repository;
+
 #define PRUNE_PACKED_DRY_RUN 01
 #define PRUNE_PACKED_VERBOSE 02
 
-void prune_packed_objects(int);
+void prune_packed_objects(struct repository *r, int opts);
 
 #endif

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Signature · 1 line

gitgitgadget

Junio C Hamano

2026-10-19 21:05 UTC #3 lore raw

In reply to Shashank S

Quoted 6 lines · Shashank S

Quoted 5 lines
The question isn't really whether we can safely remove the UNUSED macro. The question is rather whether we can assume that repo always points to a proper repository. And for that you need to research whether these commands can be invoked outside of a repository, because if so the repo may be NULL.

Quoted 11 lines

I have checked git.c file in that file I went through this commands . prune , prune-packed and repack which are .
{ "prune", cmd_prune, RUN_SETUP },
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
{ "repack", cmd_repack, RUN_SETUP },
All this commands are all registered with a special flags ' RUN_SETUP'. This flags search for .git folder . If it finds a .git folder Git successfully initializes the repo struct and passes that to valid pointer to your builtin . If it does not find a .git folder (if they ran it outside the repository) Git immediately stops and shows the error . It never run the code .

The analysis is not entirely correct. Assuming that your system administrator is not insane enough and did not make the entire filesystem as a working tree of a repository,

    $ cd / && git repack -h

does call cmd_repack() with "repo" set to NULL.

The call to parse_options() will give the short help and exit. Becaues the references to "repo" you added all happen after the code already called parse_options(), your update will not make the code dereference NULL. Some subcommands with tricky call sequence into parse_options() _may_ have to be a bit more careful than just checking "are we using RUN_SETUP?". Of course RUN_SETUP_GENTLY is an explicit sign that you have to be prepared to see NULL in "repo".

Thanks.

Shashank S

2026-10-20 16:27 UTC #4 lore raw

Quoted 31 lines · Junio C Hamano

Quoted 11 lines · Shashank S
I have checked git.c file in that file I went through this commands . prune , prune-packed and repack which are .
{ "prune", cmd_prune, RUN_SETUP },
{ "prune-packed", cmd_prune_packed, RUN_SETUP },
{ "repack", cmd_repack, RUN_SETUP },
All this commands are all registered with a special flags ' RUN_SETUP'. This flags search for .git folder . If it finds a .git folder Git successfully initializes the repo struct and passes that to valid pointer to your builtin . If it does not find a .git folder (if they ran it outside the repository) Git immediately stops and shows the error . It never run the code .
The analysis is not entirely correct. Assuming that your system administrator is not insane enough and did not make the entire filesystem as a working tree of a repository,
    $ cd / && git repack -h
does call cmd_repack() with "repo" set to NULL.
The call to parse_options() will give the short help and exit. Becaues the references to "repo" you added all happen after the code already called parse_options(), your update will not make the code dereference NULL. Some subcommands with tricky call sequence into parse_options() _may_ have to be a bit more careful than just checking "are we using RUN_SETUP?". Of course RUN_SETUP_GENTLY is an explicit sign that you have to be prepared to see NULL in "repo".

Quoted 1 line

Thanks.

Thank you for the correction and the detailed explanation.

I had not considered the parse_options() and the behavior of the -h flags outside of a working tree. It is great to know that while RUN_SETUP isn't an absolute guarantee against NULL on its own (unlike RUN_SETUP_GENTLY), the fact that parse_options() exits before my modified function calls ensures repo is never dereferenced when NULL.

I appreciate you taking the time to explain this to me.

Thanks.

Patrick Steinhardt

2026-10-23 07:02 UTC #6 lore raw

In reply to Shashank S via GitGitGadget

Quoted 11 lines · Shashank S via GitGitGadget

Range-diff vs v1:
1: 5d9ba0cf3a ! 1: 6b583e1d46 prune-packed: remove USE_THE_REPOSITORY_VARIABLE
@@
## Metadata ##
-Author: Shashank <[email protected]>
+Author: Shashank S Mogaveera <[email protected]>
## Commit message ##
- prune-packed: remove USE_THE_REPOSITORY_VARIABLE
+ [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE

This prefix shouldn't be part of the patch subject itself.

Quoted 10 lines

- Right now, the prune_packed_objects() function uses the global
+ The prune_packed_objects() function uses the global
the_repository variable. To help with the libification effort, change this function so it takes a 'struct repository' pointer as an argument instead.
@@ Commit message
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

One small nit: I don't think we need to have the list here.

Quoted 6 lines

     -    variables. Because we are now actually using the 'repo'  parameter
     -    in these files, we can safely remove the UNUSED macro from them.
     +    builtin/prune.c, and builtin/repack.c) to pass their local 'repo'
     +    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.

As mentioned by Junio, this isn't true. The repo pointer _may_ be NULL in the case where "-h" is passed.

Thanks!

Patrick

Shashank S

2026-10-24 04:17 UTC #7 lore raw

Quoted 8 lines · Patrick Steinhardt

On Mon, Oct 19, 2026 at 04:53:31PM +0000, Shashank S via
Quoted 3 lines
## Commit message ##
- prune-packed: remove USE_THE_REPOSITORY_VARIABLE
+ [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE
This prefix shouldn't be part of the patch subject itself.

I will remove it

Quoted 9 lines

Quoted 9 lines
- Right now, the prune_packed_objects() function uses the global
+ The prune_packed_objects() function uses the global
the_repository variable. To help with the libification effort, change this function so it takes a 'struct repository' pointer as an argument instead.
@@ Commit message
prune_object() callbacks to read from this new struct.

Quoted 4 lines

Quoted 2 lines
Update the builtins that call this function (builtin/prune-packed.c,
- builtin/prune.c, and builtin/repack.c) to pass their local repo
One small nit: I don't think we need to have the list here.

I will remove it too.

Quoted 9 lines

Quoted 6 lines

     -    variables. Because we are now actually using the 'repo'  parameter
     -    in these files, we can safely remove the UNUSED macro from them.
     +    builtin/prune.c, and builtin/repack.c) to pass their local 'repo'
     +    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.

As mentioned by Junio, this isn't true. The repo pointer _may_ be NULL in the case where "-h" is passed.

I will improvise the commit and submit a patch v3

Quoted 4 lines

Thanks!
Patrick

Thanks!

Shashank S Mogaveera

Shashank S via GitGitGadget

author

2026-10-24 09:22 UTC #8 lore raw

PATCH v3 1/1 prune-packed: remove USE_THE_REPOSITORY_VARIABLE

In reply to Shashank S via GitGitGadget

From: Shashank S Mogaveera <[email protected]>

Update the builtins that call this function to pass their local 'repo' variables. We can safely remove the UNUSED macro from them because 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.

Finally, drop the USE_THE_REPOSITORY_VARIABLE macro from prune-packed.c.

Signed-off-by Shashank S Mogaveera <[email protected]>

---
[Outreachy] prune-packed: remove use_the_repository_variable

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2266%2Fshashu8660%2Foutreachy2-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2266/shashu8660/outreachy2-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2266

Range-diff vs v2:

1: 6b583e1d46 ! 1: 0b474e3433 [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE
@@ Metadata
Author: Shashank S Mogaveera <[email protected]>

## Commit message ##
- [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE
+ prune-packed: remove USE_THE_REPOSITORY_VARIABLE

The prune_packed_objects() function uses the global the_repository variable. To help with the libification effort,
@@ 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'
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.

Finally, drop the USE_THE_REPOSITORY_VARIABLE macro from prune-packed.c.
@@ 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);
@@ prune-packed.c
-void prune_packed_objects(int opts)
+void prune_packed_objects(struct repository *r, int opts)
{
-+ struct prune_data cb_data = {
-+ .r = r,
-+ .opts = opts
-+ };
-+
- if (opts & PRUNE_PACKED_VERBOSE)
+- if (opts & PRUNE_PACKED_VERBOSE)
- progress = start_delayed_progress(the_repository,
-+ progress = start_delayed_progress(r,
- _("Removing duplicate objects"), 256);
+- _("Removing duplicate objects"), 256);
++ struct prune_data cb_data = {
++ .r = r,
++ .opts = opts
++ };
++
++ if (opts & PRUNE_PACKED_VERBOSE)
++ progress = start_delayed_progress(r,
++ _("Removing duplicate objects"), 256);

-- for_each_loose_file_in_source(the_repository->objects->sources,
+- for_each_loose_file_in_source(the_repository->objects->source,
- prune_object, NULL, prune_subdir, &opts);
-+ for_each_loose_file_in_source(r->objects->sources,
-+ prune_object, NULL, prune_subdir, &cb_data);
++ for_each_loose_file_in_source(r->objects->source,
++ prune_object, NULL, prune_subdir, &cb_data);

- /* Ensure we show 100% before finishing progress */
- display_progress(progress, 256);
+- /* 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);
+ }

## prune-packed.h ##
@@

 builtin/prune-packed.c |  4 ++--
 builtin/prune.c        |  2 +-
 builtin/repack.c       |  2 +-
 prune-packed.c         | 42 +++++++++++++++++++++++++-----------------
 prune-packed.h         |  4 +++-
 5 files changed, 32 insertions(+), 22 deletions(-)

builtin/prune-packed.c +2 -2

index 4d63f26b0a..d44e7f81d4 100644
@@ -11,7 +11,7 @@ static const char * const prune_packed_usage[] = {
 int cmd_prune_packed(int argc,
 		     const char **argv,
 		     const char *prefix,
-		     struct repository *repo UNUSED)
+		     struct repository *repo)
 {
 	int opts = isatty(2) ? PRUNE_PACKED_VERBOSE : 0;
 	const struct option prune_packed_options[] = {

@@ -30,6 +30,6 @@ int cmd_prune_packed(int argc,
 			      prune_packed_usage,
 			      prune_packed_options);
 
-	prune_packed_objects(opts);
+	prune_packed_objects(repo, opts);
 	return 0;
 }

builtin/prune.c +1 -1

index 97f4c1418b..d543c3ad2d 100644
@@ -201,7 +201,7 @@ int cmd_prune(int argc,
 	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);
+	prune_packed_objects(repo, show_only ? PRUNE_PACKED_DRY_RUN : 0);
 	remove_temporary_files(repo_get_object_directory(repo));
 	s = mkpathdup("%s/pack", repo_get_object_directory(repo));
 	remove_temporary_files(s);

builtin/repack.c +1 -1

index 5d06872d77..585cdd3d1d 100644
@@ -757,7 +757,7 @@ int cmd_repack(int argc,
 						       wrote_incremental_midx);
 		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
-		prune_packed_objects(opts);
+		prune_packed_objects(repo, opts);
 
 		if (!keep_unreachable &&
 		    (!(pack_everything & LOOSEN_UNREACHABLE) ||

prune-packed.c +25 ~~-17~~

72 lines not on this page · raw message

prune-packed.h +3 -1

index 936fa9df23..9ec7ff0d60 100644
@@ -1,9 +1,11 @@
 #ifndef PRUNE_PACKED_H
 #define PRUNE_PACKED_H
 
+struct repository;
+
 #define PRUNE_PACKED_DRY_RUN 01
 #define PRUNE_PACKED_VERBOSE 02
 
-void prune_packed_objects(int);
+void prune_packed_objects(struct repository *r, int opts);
 
 #endif

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2026-10-26 08:38 UTC #9 lore raw

Quoted 9 lines · Shashank S via GitGitGadget

Range-diff vs v2:
1: 6b583e1d46 ! 1: 0b474e3433 [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE
@@ Metadata
Author: Shashank S Mogaveera <[email protected]>
## Commit message ##
- [Outreachy Patch] prune-packed: remove USE_THE_REPOSITORY_VARIABLE
+ prune-packed: remove USE_THE_REPOSITORY_VARIABLE

Good.

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

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

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

Quoted 45 lines

prune-packed.c +11 -6

index b6749b8d13..e02d5d7a01 100644
@@ -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;
 }

prune-packed.c line 45

Quoted 10 lines

prune-packed.c +5 -4

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

prune-packed.c line 51

Quoted 7 lines

prune-packed.c +4 -2

+    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-packed.c line 56

Quoted 8 lines

prune-packed.c +4 -3

+		      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

Shashank S via GitGitGadget

author

2026-10-27 16:59 UTC #11 lore raw

PATCH v4 1/1 prune-packed: remove USE_THE_REPOSITORY_VARIABLE

In reply to Shashank S via GitGitGadget

From: Shashank S Mogaveera <[email protected]>

Finally, drop the USE_THE_REPOSITORY_VARIABLE macro from prune-packed.c.

Signed-off-by Shashank S Mogaveera <[email protected]>

---
[Outreachy] prune-packed: remove use_the_repository_variable

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2266%2Fshashu8660%2Foutreachy2-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2266/shashu8660/outreachy2-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/2266

Range-diff vs v3:

1: 0b474e3433 ! 1: 66f3becea8 prune-packed: remove USE_THE_REPOSITORY_VARIABLE
@@ 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 to pass their local 'repo'
- variables. We can safely remove the UNUSED macro from them because
- 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.
+ 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"

Finally, drop the USE_THE_REPOSITORY_VARIABLE macro from prune-packed.c.
@@ prune-packed.c
-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
-+ };
++ struct prune_data cb_data = {
++ .r = r,
++ .opts = opts
++ };
+
-+ if (opts & PRUNE_PACKED_VERBOSE)
-+ progress = start_delayed_progress(r,
-+ _("Removing duplicate objects"), 256);
+ if (opts & PRUNE_PACKED_VERBOSE)
+- progress = start_delayed_progress(the_repository,
++ 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,
-+ prune_object, NULL, prune_subdir, &cb_data);
++ for_each_loose_file_in_source(r->objects->source,
++ 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);
- }
+ /* Ensure we show 100% before finishing progress */
+ display_progress(progress, 256);

## prune-packed.h ##
@@

 builtin/prune-packed.c |  4 ++--
 builtin/prune.c        |  2 +-
 builtin/repack.c       |  2 +-
 prune-packed.c         | 32 ++++++++++++++++++++------------
 prune-packed.h         |  4 +++-
 5 files changed, 27 insertions(+), 17 deletions(-)

builtin/prune-packed.c +2 -2

index 4d63f26b0a..d44e7f81d4 100644
@@ -11,7 +11,7 @@ static const char * const prune_packed_usage[] = {
 int cmd_prune_packed(int argc,
 		     const char **argv,
 		     const char *prefix,
-		     struct repository *repo UNUSED)
+		     struct repository *repo)
 {
 	int opts = isatty(2) ? PRUNE_PACKED_VERBOSE : 0;
 	const struct option prune_packed_options[] = {

@@ -30,6 +30,6 @@ int cmd_prune_packed(int argc,
 			      prune_packed_usage,
 			      prune_packed_options);
 
-	prune_packed_objects(opts);
+	prune_packed_objects(repo, opts);
 	return 0;
 }

builtin/prune.c +1 -1

index 97f4c1418b..d543c3ad2d 100644
@@ -201,7 +201,7 @@ int cmd_prune(int argc,
 	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);
+	prune_packed_objects(repo, show_only ? PRUNE_PACKED_DRY_RUN : 0);
 	remove_temporary_files(repo_get_object_directory(repo));
 	s = mkpathdup("%s/pack", repo_get_object_directory(repo));
 	remove_temporary_files(s);

builtin/repack.c +1 -1

index 5d06872d77..585cdd3d1d 100644
@@ -757,7 +757,7 @@ int cmd_repack(int argc,
 						       wrote_incremental_midx);
 		if (show_progress)
 			opts |= PRUNE_PACKED_VERBOSE;
-		prune_packed_objects(opts);
+		prune_packed_objects(repo, opts);
 
 		if (!keep_unreachable &&
 		    (!(pack_everything & LOOSEN_UNREACHABLE) ||

prune-packed.c +20 ~~-12~~

index b6749b8d13..00a770795f 100644
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "gettext.h"
 #include "object-file.h"

@@ -8,40 +6,50 @@
 #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)
 {
+	struct prune_data cb_data = {
+		.r = r,
+		.opts = opts
+	};
+
 	if (opts & PRUNE_PACKED_VERBOSE)
-		progress = start_delayed_progress(the_repository,
+		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,
+				      prune_object, NULL, prune_subdir, &cb_data);
 
 	/* Ensure we show 100% before finishing progress */
 	display_progress(progress, 256);

prune-packed.h +3 -1

index 936fa9df23..9ec7ff0d60 100644
@@ -1,9 +1,11 @@
 #ifndef PRUNE_PACKED_H
 #define PRUNE_PACKED_H
 
+struct repository;
+
 #define PRUNE_PACKED_DRY_RUN 01
 #define PRUNE_PACKED_VERBOSE 02
 
-void prune_packed_objects(int);
+void prune_packed_objects(struct repository *r, int opts);
 
 #endif

base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e

Signature · 1 line

gitgitgadget

Shashank S

2026-10-27 17:12 UTC #10 lore raw

In reply to Patrick Steinhardt

Quoted 8 lines · Patrick Steinhardt

Quoted 2 lines
+ 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:

Quoted 7 lines

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

Thank you i have update this into the v4 patch

Quoted 11 lines

prune-packed.c line 45

Quoted 9 lines

prune-packed.c +4 -3

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

Sorry, I have done the correction in v4 patch .

Quoted 20 lines

prune-packed.c line 51

Quoted 8 lines

prune-packed.c +4 -2

+    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-packed.c line 56

Quoted 8 lines

prune-packed.c +4 -3

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