On Tue, Oct 22, 2024 at 04:51:04AM -0400, karthik nayak wrote:

Taylor Blau [off-list ref] writes:

quoted

On Mon, Oct 21, 2024 at 11:57:44AM +0200, Karthik Nayak wrote:

quoted

diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 5809613002..60f806e672 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c

@@ -688,7 +688,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
 	pl = red = pack_list_difference(local_packs, min);
 	while (pl) {
 		printf("%s\n%s\n",
-		       sha1_pack_index_name(pl->pack->hash),
+		       sha1_pack_index_name(the_repository, pl->pack->hash),
 		       pl->pack->pack_name);
 		pl = pl->next;
 	}

I am a little surprised to see sha1_pack_index_name() converted
similarly here, as this patch promises only to touch the
'odb_pack_name()' function.

quoted

diff --git a/packfile.h b/packfile.h
index 0f78658229..507ac602b5 100644
--- a/packfile.h
+++ b/packfile.h

Indeed, it looks like odb_pack_name(), sha1_pack_name(),
sha1_pack_index_name(), parse_pack_index(), and has_pack_index() are all
modified. Were these meant to go in separate patches?

Nope this is intentional, each commit tries to pick a base function and
modifies all layers above it (I should have explicitly specified my
approach in the cover).

In this commit, we try to modify `odb_pack_name()` and therefore
sha1_pack_name(), sha1_pack_index_name(), parse_pack_index(), and
has_pack_index() too. Otherwise, we'd have a lot lot more commits.

This simplies the review too, but yes, will add more instructions to the
next version.

Makes sense, thanks for clarifying. I agree that this would be good
information to have in the patch message to avoid confusion.


Thanks,
Taylor