Re: [PATCH 01/12] csum-file: stop depending on `the_repository`

From: Karthik Nayak <karthik.188@gmail.com>
Date: Mar 6, 2025, 10:37 UTC
Message-ID: <CAOLa=ZREou-t8m6-HqUXpgB1SygNzP1JXSUV09NzpaoKgEUdwQ@mail.gmail.com>
In-Reply-To: <20250303-b4-pks-objects-without-the-repository-v1-1-c5dd43f2476e@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 39 quoted lines

> There are multiple sites in "csum-file.c" where we use the global
> `the_repository` variable, either explicitly or implicitly by using
> `the_hash_algo`.
>
> Refactor the code to stop using `the_repository` by adapting functions
> to receive required data as parameters. Adapt callsites accordingly by
> either using `the_repository->hash_algo`, or by using a context-provided
> hash algorithm in case the subsystem already got rid of its dependency
> on `the_repository`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fast-import.c  |  2 +-
>  builtin/index-pack.c   |  2 +-
>  builtin/pack-objects.c |  3 ++-
>  commit-graph.c         |  9 ++++++---
>  csum-file.c            | 28 ++++++++++++++++------------
>  csum-file.h            | 12 ++++++++----
>  midx-write.c           |  6 ++++--
>  midx.c                 |  3 ++-
>  pack-bitmap-write.c    |  2 +-
>  pack-bitmap.c          |  9 +++++----
>  pack-check.c           |  2 +-
>  pack-revindex.c        |  3 ++-
>  pack-write.c           | 12 ++++++------
>  read-cache.c           |  2 +-
>  14 files changed, 56 insertions(+), 39 deletions(-)
>
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 397a6f46ad8..86e6e754816 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -770,7 +770,7 @@ static void start_packfile(void)
>  	p->pack_fd = pack_fd;
>  	p->do_not_close = 1;
>  	p->repo = the_repository;
> -	pack_file = hashfd(pack_fd, p->pack_name);
> +	pack_file = hashfd(the_repository->hash_algo, pack_fd, p->pack_name);
>

I recall that `p->repo` should be an option here, but it makes to use `the_repository` directly here and not worry about it, this should apply to the other changes below too.

[snip]

The rest of the changes look good.

Show 23 quoted lines

> diff --git a/pack-bitmap.c b/pack-bitmap.c
> index 6406953d322..f0e2c000252 100644
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -3024,7 +3024,8 @@ int bitmap_is_preferred_refname(struct repository *r, const char *refname)
>  	return 0;
>  }
>
> -static int verify_bitmap_file(const char *name)
> +static int verify_bitmap_file(const struct git_hash_algo *algop,
> +			      const char *name)
>  {
>  	struct stat st;
>  	unsigned char *data;
> @@ -3040,7 +3041,7 @@ static int verify_bitmap_file(const char *name)
>
>  	data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
>  	close(fd);
> -	if (!hashfile_checksum_valid(data, st.st_size))
> +	if (!hashfile_checksum_valid(algop, data, st.st_size))
>  		res = error(_("bitmap file '%s' has invalid checksum"),
>  			    name);
>

Here, we're modifying an internal function since it needs to pass the algo to `hashfile_checksum_valid`. Makes sense.

Show 16 quoted lines

> @@ -3055,14 +3056,14 @@ int verify_bitmap_files(struct repository *r)
>  	for (struct multi_pack_index *m = get_multi_pack_index(r);
>  	     m; m = m->next) {
>  		char *midx_bitmap_name = midx_bitmap_filename(m);
> -		res |= verify_bitmap_file(midx_bitmap_name);
> +		res |= verify_bitmap_file(r->hash_algo, midx_bitmap_name);
>  		free(midx_bitmap_name);
>  	}
>
>  	for (struct packed_git *p = get_all_packs(r);
>  	     p; p = p->next) {
>  		char *pack_bitmap_name = pack_bitmap_filename(p);
> -		res |= verify_bitmap_file(pack_bitmap_name);
> +		res |= verify_bitmap_file(r->hash_algo, pack_bitmap_name);
>  		free(pack_bitmap_name);
>  	}

[snip]