Re: [PATCH 01/20] packfile: pass down repository to `odb_pack_name`

[Taylor Blau <me@ttaylorr.com>]

On Tue, Oct 22, 2024 at 04:51:04AM -0400, karthik nayak wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Mon, Oct 21, 2024 at 11:57:44AM +0200, Karthik Nayak wrote:
> >> 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.
> >
> >> 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