[PATCH 0/5] pack-write: cleanup usage of global variables

[PATCH 0/5] pack-write: cleanup usage of global variables

[Karthik Nayak via B4 Relay]

This is a small series to remove global variable usage from
`pack-write.c`. Mostly it bubble's up the usage of global variables to
upper layers. The only exception is in `write-midx.c`, which was cleaned
of global variable usage, so there, we use the repo that is in available
in the context.

This series is based on fbe8d3079d (Git 2.48, 2025-01-10) with
'ps/more-sign-compare' and 'ps/the-repository' merged in.

There are no conflicts with topics in 'next', however there is a
conflict with 'tb/incremental-midx-part-2' in 'seen', the fix is simple
but happy to merge that in too if necessary.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Karthik Nayak (5):
      pack-write: pass hash_algo to `fixup_pack_header_footer()`
      pack-write: pass repository to `index_pack_lockfile()`
      pack-write: pass hash_algo to `write_idx_file()`
      pack-write: pass hash_algo to `write_rev_file()`
      pack-write: pass hash_algo to `write_rev_*()`

 builtin/fast-import.c  | 11 +++---
 builtin/index-pack.c   | 11 +++---
 builtin/pack-objects.c | 12 +++---
 builtin/receive-pack.c |  2 +-
 bulk-checkin.c         |  7 ++--
 fetch-pack.c           |  4 +-
 midx-write.c           |  4 +-
 pack-write.c           | 99 +++++++++++++++++++++++++++-----------------------
 pack.h                 | 30 ++++++++++++---
 9 files changed, 106 insertions(+), 74 deletions(-)
---



---

base-commit: 8b2efc058aaa3d1437678616bccf7c5f7ce1f92b
change-id: 20250110-kn-the-repo-cleanup-44144fa42dc3

Thanks
- Karthik

[PATCH 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Karthik Nayak via B4 Relay]

From: Karthik Nayak <karthik.188@gmail.com>

The `fixup_pack_header_footer()` function uses the global
`the_hash_algo` variable to access the repository's hash function. To
avoid global variable usage, pass the hash function from the layers
above.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c  |  7 ++++---
 builtin/index-pack.c   |  2 +-
 builtin/pack-objects.c |  5 +++--
 bulk-checkin.c         |  2 +-
 pack-write.c           | 28 ++++++++++++++--------------
 pack.h                 |  4 +++-
 6 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa..6baf2b1b71e2443a7987a41e0cd246076682bf58 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -878,9 +878,10 @@ static void end_packfile(void)
 
 		close_pack_windows(pack_data);
 		finalize_hashfile(pack_file, cur_pack_oid.hash, FSYNC_COMPONENT_PACK, 0);
-		fixup_pack_header_footer(pack_data->pack_fd, pack_data->hash,
-					 pack_data->pack_name, object_count,
-					 cur_pack_oid.hash, pack_size);
+		fixup_pack_header_footer(the_hash_algo, pack_data->pack_fd,
+					 pack_data->hash, pack_data->pack_name,
+					 object_count, cur_pack_oid.hash,
+					 pack_size);
 
 		if (object_count <= unpack_limit) {
 			if (!loosen_small_pack(pack_data)) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0bef61c57232e198ba539cd44ec301d26dcb0eb8..6c5e3483f4fe67fb2e26132c55b1f8395d60c11f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1390,7 +1390,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 		strbuf_release(&msg);
 		finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
 		hashcpy(read_hash, pack_hash, the_repository->hash_algo);
-		fixup_pack_header_footer(output_fd, pack_hash,
+		fixup_pack_header_footer(the_hash_algo, output_fd, pack_hash,
 					 curr_pack, nr_objects,
 					 read_hash, consumed_bytes-the_hash_algo->rawsz);
 		if (!hasheq(read_hash, tail_hash, the_repository->hash_algo))
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d51c021d99d9f470c04b7ec52565ab2f4c1c19ae..ffc62930b68c9b4152057572ede216381a4b0991 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1319,8 +1319,9 @@ static void write_pack_file(void)
 			 */
 
 			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
-			fixup_pack_header_footer(fd, hash, pack_tmp_name,
-						 nr_written, hash, offset);
+			fixup_pack_header_footer(the_hash_algo, fd, hash,
+						 pack_tmp_name, nr_written,
+						 hash, offset);
 			close(fd);
 			if (write_bitmap_index) {
 				if (write_bitmap_index != WRITE_BITMAP_QUIET)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 5044cb7fa083d692a3797e2491c27b61ec44c69c..c4b085f57f74fb8b998576ac9d84fed9e01ed0ed 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -70,7 +70,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 				  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 	} else {
 		int fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);
-		fixup_pack_header_footer(fd, hash, state->pack_tmp_name,
+		fixup_pack_header_footer(the_hash_algo, fd, hash, state->pack_tmp_name,
 					 state->nr_written, hash,
 					 state->offset);
 		close(fd);
diff --git a/pack-write.c b/pack-write.c
index 98a8c0e7853d7b46b5ce9a9672e0249ff051b5f9..fc887850dfb9789132b8642733c6472944dbe32d 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -380,7 +380,8 @@ off_t write_pack_header(struct hashfile *f, uint32_t nr_entries)
  * partial_pack_sha1 can refer to the same buffer if the caller is not
  * interested in the resulting SHA1 of pack data above partial_pack_offset.
  */
-void fixup_pack_header_footer(int pack_fd,
+void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
+			 int pack_fd,
 			 unsigned char *new_pack_hash,
 			 const char *pack_name,
 			 uint32_t object_count,
@@ -393,8 +394,8 @@ void fixup_pack_header_footer(int pack_fd,
 	char *buf;
 	ssize_t read_result;
 
-	the_hash_algo->init_fn(&old_hash_ctx);
-	the_hash_algo->init_fn(&new_hash_ctx);
+	hash_algo->init_fn(&old_hash_ctx);
+	hash_algo->init_fn(&new_hash_ctx);
 
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
@@ -406,9 +407,9 @@ void fixup_pack_header_footer(int pack_fd,
 			  pack_name);
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
-	the_hash_algo->update_fn(&old_hash_ctx, &hdr, sizeof(hdr));
+	hash_algo->update_fn(&old_hash_ctx, &hdr, sizeof(hdr));
 	hdr.hdr_entries = htonl(object_count);
-	the_hash_algo->update_fn(&new_hash_ctx, &hdr, sizeof(hdr));
+	hash_algo->update_fn(&new_hash_ctx, &hdr, sizeof(hdr));
 	write_or_die(pack_fd, &hdr, sizeof(hdr));
 	partial_pack_offset -= sizeof(hdr);
 
@@ -423,7 +424,7 @@ void fixup_pack_header_footer(int pack_fd,
 			break;
 		if (n < 0)
 			die_errno("Failed to checksum '%s'", pack_name);
-		the_hash_algo->update_fn(&new_hash_ctx, buf, n);
+		hash_algo->update_fn(&new_hash_ctx, buf, n);
 
 		aligned_sz -= n;
 		if (!aligned_sz)
@@ -432,13 +433,12 @@ void fixup_pack_header_footer(int pack_fd,
 		if (!partial_pack_hash)
 			continue;
 
-		the_hash_algo->update_fn(&old_hash_ctx, buf, n);
+		hash_algo->update_fn(&old_hash_ctx, buf, n);
 		partial_pack_offset -= n;
 		if (partial_pack_offset == 0) {
 			unsigned char hash[GIT_MAX_RAWSZ];
-			the_hash_algo->final_fn(hash, &old_hash_ctx);
-			if (!hasheq(hash, partial_pack_hash,
-				    the_repository->hash_algo))
+			hash_algo->final_fn(hash, &old_hash_ctx);
+			if (!hasheq(hash, partial_pack_hash, hash_algo))
 				die("Unexpected checksum for %s "
 				    "(disk corruption?)", pack_name);
 			/*
@@ -446,7 +446,7 @@ void fixup_pack_header_footer(int pack_fd,
 			 * pack, which also means making partial_pack_offset
 			 * big enough not to matter anymore.
 			 */
-			the_hash_algo->init_fn(&old_hash_ctx);
+			hash_algo->init_fn(&old_hash_ctx);
 			partial_pack_offset = ~partial_pack_offset;
 			partial_pack_offset -= MSB(partial_pack_offset, 1);
 		}
@@ -454,9 +454,9 @@ void fixup_pack_header_footer(int pack_fd,
 	free(buf);
 
 	if (partial_pack_hash)
-		the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
-	the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
-	write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz);
+		hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
+	hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
+	write_or_die(pack_fd, new_pack_hash, hash_algo->rawsz);
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
diff --git a/pack.h b/pack.h
index a8da0406299bf267205256aaa8efb215f2ff73be..6d9d477adc83e83d9e9175ccf699c100b4c147c6 100644
--- a/pack.h
+++ b/pack.h
@@ -91,7 +91,9 @@ int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offs
 int verify_pack_index(struct packed_git *);
 int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);
 off_t write_pack_header(struct hashfile *f, uint32_t);
-void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
+void fixup_pack_header_footer(const struct git_hash_algo *, int,
+			      unsigned char *, const char *, uint32_t,
+			      unsigned char *, off_t);
 char *index_pack_lockfile(int fd, int *is_well_formed);
 
 struct ref;

-- 
2.47.0

[PATCH 2/5] pack-write: pass repository to `index_pack_lockfile()`

[Karthik Nayak via B4 Relay]

From: Karthik Nayak <karthik.188@gmail.com>

The `index_pack_lockfile()` function uses the global `the_repository`
variable to access the repository. To avoid global variable usage, pass
the repository from the layers above.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 2 +-
 fetch-pack.c           | 4 +++-
 pack-write.c           | 6 +++---
 pack.h                 | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56347a79633505efe8dc05acf1583b4c9995eefe..b83abe5d220cefd3707b701409dc5e6b67566599 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2304,7 +2304,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (status)
 			return "index-pack fork failed";
 
-		lockfile = index_pack_lockfile(child.out, NULL);
+		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
 		if (lockfile) {
 			pack_lockfile = register_tempfile(lockfile);
 			free(lockfile);
diff --git a/fetch-pack.c b/fetch-pack.c
index 3a227721ed0935d1f9c40584c57f54043354c032..824f56ecbca11cd9e4da6a3e4c450c6b2e7078ab 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1036,7 +1036,9 @@ static int get_pack(struct fetch_pack_args *args,
 		die(_("fetch-pack: unable to fork off %s"), cmd_name);
 	if (do_keep && (pack_lockfiles || fsck_objects)) {
 		int is_well_formed;
-		char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);
+		char *pack_lockfile = index_pack_lockfile(the_repository,
+							  cmd.out,
+							  &is_well_formed);
 
 		if (!is_well_formed)
 			die(_("fetch-pack: invalid index-pack output"));
diff --git a/pack-write.c b/pack-write.c
index fc887850dfb9789132b8642733c6472944dbe32d..0cd75d2e55419362a61cf981fc11117ea7a1d88a 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -460,10 +460,10 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
-char *index_pack_lockfile(int ip_out, int *is_well_formed)
+char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed)
 {
 	char packname[GIT_MAX_HEXSZ + 6];
-	const int len = the_hash_algo->hexsz + 6;
+	const int len = r->hash_algo->hexsz + 6;
 
 	/*
 	 * The first thing we expect from index-pack's output
@@ -480,7 +480,7 @@ char *index_pack_lockfile(int ip_out, int *is_well_formed)
 		packname[len-1] = 0;
 		if (skip_prefix(packname, "keep\t", &name))
 			return xstrfmt("%s/pack/pack-%s.keep",
-				       repo_get_object_directory(the_repository), name);
+				       repo_get_object_directory(r), name);
 		return NULL;
 	}
 	if (is_well_formed)
diff --git a/pack.h b/pack.h
index 6d9d477adc83e83d9e9175ccf699c100b4c147c6..46d85e5bec787c90af69700fd4b328b1ebf1d606 100644
--- a/pack.h
+++ b/pack.h
@@ -94,7 +94,7 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(const struct git_hash_algo *, int,
 			      unsigned char *, const char *, uint32_t,
 			      unsigned char *, off_t);
-char *index_pack_lockfile(int fd, int *is_well_formed);
+char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);
 
 struct ref;
 

-- 
2.47.0

[PATCH 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Karthik Nayak via B4 Relay]

From: Karthik Nayak <karthik.188@gmail.com>

The `write_idx_file()` function uses the global `the_hash_algo` variable
to access the repository's hash function. To avoid global variable
usage, pass the hash function from the layers above.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c  |  4 ++--
 builtin/index-pack.c   |  3 ++-
 builtin/pack-objects.c |  7 ++++---
 bulk-checkin.c         |  5 +++--
 pack-write.c           | 14 ++++++++------
 pack.h                 | 10 ++++++++--
 6 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 6baf2b1b71e2443a7987a41e0cd246076682bf58..c4bc52f93c011f34bd7f98c8e9d74a33cf9783bd 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -798,8 +798,8 @@ static const char *create_index(void)
 	if (c != last)
 		die("internal consistency error creating the index");
 
-	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,
-				 pack_data->hash);
+	tmpfile = write_idx_file(the_hash_algo, NULL, idx, object_count,
+				 &pack_idx_opts, pack_data->hash);
 	free(idx);
 	return tmpfile;
 }
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 6c5e3483f4fe67fb2e26132c55b1f8395d60c11f..d73699653a2227f8ecfee2c0f51cd680093ac764 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -2096,7 +2096,8 @@ int cmd_index_pack(int argc,
 	ALLOC_ARRAY(idx_objects, nr_objects);
 	for (i = 0; i < nr_objects; i++)
 		idx_objects[i] = &objects[i].idx;
-	curr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_hash);
+	curr_index = write_idx_file(the_hash_algo, index_name, idx_objects,
+				    nr_objects, &opts, pack_hash);
 	if (rev_index)
 		curr_rev_index = write_rev_file(rev_index_name, idx_objects,
 						nr_objects, pack_hash,
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ffc62930b68c9b4152057572ede216381a4b0991..7b2dacda514c29f5393b604f15d379abb81546c0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1369,9 +1369,10 @@ static void write_pack_file(void)
 			if (cruft)
 				pack_idx_opts.flags |= WRITE_MTIMES;
 
-			stage_tmp_packfiles(&tmpname, pack_tmp_name,
-					    written_list, nr_written,
-					    &to_pack, &pack_idx_opts, hash,
+			stage_tmp_packfiles(the_hash_algo, &tmpname,
+					    pack_tmp_name, written_list,
+					    nr_written, &to_pack,
+					    &pack_idx_opts, hash,
 					    &idx_tmp_name);
 
 			if (write_bitmap_index) {
diff --git a/bulk-checkin.c b/bulk-checkin.c
index c4b085f57f74fb8b998576ac9d84fed9e01ed0ed..0d49889bfbb233d58fc094f5e2c2d2433dad9851 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -44,8 +44,9 @@ static void finish_tmp_packfile(struct strbuf *basename,
 {
 	char *idx_tmp_name = NULL;
 
-	stage_tmp_packfiles(basename, pack_tmp_name, written_list, nr_written,
-			    NULL, pack_idx_opts, hash, &idx_tmp_name);
+	stage_tmp_packfiles(the_hash_algo, basename, pack_tmp_name,
+			    written_list, nr_written, NULL, pack_idx_opts, hash,
+			    &idx_tmp_name);
 	rename_tmp_packfile_idx(basename, &idx_tmp_name);
 
 	free(idx_tmp_name);
diff --git a/pack-write.c b/pack-write.c
index 0cd75d2e55419362a61cf981fc11117ea7a1d88a..f344e78a9ec20cea9812a5eaffc72ae0b7e7424d 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -56,7 +56,8 @@ static int need_large_offset(off_t offset, const struct pack_idx_option *opts)
  * The *sha1 contains the pack content SHA1 hash.
  * The objects array passed in will be sorted by SHA1 on exit.
  */
-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,
+const char *write_idx_file(const struct git_hash_algo *hash_algo,
+			   const char *index_name, struct pack_idx_entry **objects,
 			   int nr_objects, const struct pack_idx_option *opts,
 			   const unsigned char *sha1)
 {
@@ -130,7 +131,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		struct pack_idx_entry *obj = *list++;
 		if (index_version < 2)
 			hashwrite_be32(f, obj->offset);
-		hashwrite(f, obj->oid.hash, the_hash_algo->rawsz);
+		hashwrite(f, obj->oid.hash, hash_algo->rawsz);
 		if ((opts->flags & WRITE_IDX_STRICT) &&
 		    (i && oideq(&list[-2]->oid, &obj->oid)))
 			die("The same object %s appears twice in the pack",
@@ -172,7 +173,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		}
 	}
 
-	hashwrite(f, sha1, the_hash_algo->rawsz);
+	hashwrite(f, sha1, hash_algo->rawsz);
 	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
 			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
 			  ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
@@ -546,7 +547,8 @@ void rename_tmp_packfile_idx(struct strbuf *name_buffer,
 	rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx");
 }
 
-void stage_tmp_packfiles(struct strbuf *name_buffer,
+void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
+			 struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,
 			 uint32_t nr_written,
@@ -561,8 +563,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 	if (adjust_shared_perm(pack_tmp_name))
 		die_errno("unable to make temporary pack file readable");
 
-	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
-					       pack_idx_opts, hash);
+	*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,
+					       nr_written, pack_idx_opts, hash);
 	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
diff --git a/pack.h b/pack.h
index 46d85e5bec787c90af69700fd4b328b1ebf1d606..c650fdbe2dcde8055ad0efe55646338cd0f81df5 100644
--- a/pack.h
+++ b/pack.h
@@ -86,7 +86,12 @@ struct progress;
 /* Note, the data argument could be NULL if object type is blob */
 typedef int (*verify_fn)(const struct object_id *, enum object_type, unsigned long, void*, int*);
 
-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);
+const char *write_idx_file(const struct git_hash_algo *hash_algo,
+			   const char *index_name,
+			   struct pack_idx_entry **objects,
+			   int nr_objects,
+			   const struct pack_idx_option *,
+			   const unsigned char *sha1);
 int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
 int verify_pack_index(struct packed_git *);
 int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);
@@ -119,7 +124,8 @@ int read_pack_header(int fd, struct pack_header *);
 struct packing_data;
 
 struct hashfile *create_tmp_packfile(char **pack_tmp_name);
-void stage_tmp_packfiles(struct strbuf *name_buffer,
+void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
+			 struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,
 			 uint32_t nr_written,

-- 
2.47.0

[PATCH 4/5] pack-write: pass hash_algo to `write_rev_file()`

[Karthik Nayak via B4 Relay]

From: Karthik Nayak <karthik.188@gmail.com>

The `write_rev_file()` function uses the global `the_hash_algo` variable
to access the repository's hash function. To avoid global variable
usage, let's pass the hash function from the layers above.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

However, in `midx-write.c`, since all usage of global variables is
removed, don't reintroduce them and instead use the `repo` available in
the context.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/index-pack.c |  6 +++---
 midx-write.c         |  4 ++--
 pack-write.c         | 21 ++++++++++++---------
 pack.h               | 14 ++++++++++++--
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d73699653a2227f8ecfee2c0f51cd680093ac764..e803cb2444633f937fafc841848dae85341475f1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -2099,9 +2099,9 @@ int cmd_index_pack(int argc,
 	curr_index = write_idx_file(the_hash_algo, index_name, idx_objects,
 				    nr_objects, &opts, pack_hash);
 	if (rev_index)
-		curr_rev_index = write_rev_file(rev_index_name, idx_objects,
-						nr_objects, pack_hash,
-						opts.flags);
+		curr_rev_index = write_rev_file(the_hash_algo, rev_index_name,
+						idx_objects, nr_objects,
+						pack_hash, opts.flags);
 	free(idx_objects);
 
 	if (!verify)
diff --git a/midx-write.c b/midx-write.c
index b3827b936bdb1df12c73fb7d9b98ff65fc875cc3..61b59d557d3ba46a39db74c62393545cabf50f2c 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -658,8 +658,8 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 	strbuf_addf(&buf, "%s-%s.rev", midx_name, hash_to_hex_algop(midx_hash,
 								    ctx->repo->hash_algo));
 
-	tmp_file = write_rev_file_order(NULL, ctx->pack_order, ctx->entries_nr,
-					midx_hash, WRITE_REV);
+	tmp_file = write_rev_file_order(ctx->repo->hash_algo, NULL, ctx->pack_order,
+					ctx->entries_nr, midx_hash, WRITE_REV);
 
 	if (finalize_object_file(tmp_file, buf.buf))
 		die(_("cannot store reverse index file"));
diff --git a/pack-write.c b/pack-write.c
index f344e78a9ec20cea9812a5eaffc72ae0b7e7424d..09ecbcdb069cc9b0383295798ceb49cbdc632b64 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -194,11 +194,12 @@ static int pack_order_cmp(const void *va, const void *vb, void *ctx)
 	return 0;
 }
 
-static void write_rev_header(struct hashfile *f)
+static void write_rev_header(const struct git_hash_algo *hash_algo,
+			     struct hashfile *f)
 {
 	hashwrite_be32(f, RIDX_SIGNATURE);
 	hashwrite_be32(f, RIDX_VERSION);
-	hashwrite_be32(f, oid_version(the_hash_algo));
+	hashwrite_be32(f, oid_version(hash_algo));
 }
 
 static void write_rev_index_positions(struct hashfile *f,
@@ -215,7 +216,8 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
 	hashwrite(f, hash, the_hash_algo->rawsz);
 }
 
-char *write_rev_file(const char *rev_name,
+char *write_rev_file(const struct git_hash_algo *hash_algo,
+		     const char *rev_name,
 		     struct pack_idx_entry **objects,
 		     uint32_t nr_objects,
 		     const unsigned char *hash,
@@ -233,15 +235,16 @@ char *write_rev_file(const char *rev_name,
 		pack_order[i] = i;
 	QSORT_S(pack_order, nr_objects, pack_order_cmp, objects);
 
-	ret = write_rev_file_order(rev_name, pack_order, nr_objects, hash,
-				   flags);
+	ret = write_rev_file_order(hash_algo, rev_name, pack_order, nr_objects,
+				   hash, flags);
 
 	free(pack_order);
 
 	return ret;
 }
 
-char *write_rev_file_order(const char *rev_name,
+char *write_rev_file_order(const struct git_hash_algo *hash_algo,
+			   const char *rev_name,
 			   uint32_t *pack_order,
 			   uint32_t nr_objects,
 			   const unsigned char *hash,
@@ -280,7 +283,7 @@ char *write_rev_file_order(const char *rev_name,
 		return NULL;
 	}
 
-	write_rev_header(f);
+	write_rev_header(hash_algo, f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
 	write_rev_trailer(f, hash);
@@ -568,8 +571,8 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
 	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
-	rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,
-				      pack_idx_opts->flags);
+	rev_tmp_name = write_rev_file(hash_algo, NULL, written_list, nr_written,
+				      hash, pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
 		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
diff --git a/pack.h b/pack.h
index c650fdbe2dcde8055ad0efe55646338cd0f81df5..8a84ea475cda902936ee0b6091c5b4d462606be8 100644
--- a/pack.h
+++ b/pack.h
@@ -105,8 +105,18 @@ struct ref;
 
 void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
 
-char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
-char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
+char *write_rev_file(const struct git_hash_algo *hash_algo,
+		     const char *rev_name,
+		     struct pack_idx_entry **objects,
+		     uint32_t nr_objects,
+		     const unsigned char *hash,
+		     unsigned flags);
+char *write_rev_file_order(const struct git_hash_algo *hash_algo,
+			   const char *rev_name,
+			   uint32_t *pack_order,
+			   uint32_t nr_objects,
+			   const unsigned char *hash,
+			   unsigned flags);
 
 /*
  * The "hdr" output buffer should be at least this big, which will handle sizes

-- 
2.47.0

[PATCH 5/5] pack-write: pass hash_algo to `write_rev_*()`

[Karthik Nayak via B4 Relay]

From: Karthik Nayak <karthik.188@gmail.com>

The `write_rev_*()` functions use the global `the_hash_algo` variable to
access the repository's hash function. Pass the hash from down as we've
added made them available in the previous few commits.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 pack-write.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index 09ecbcdb069cc9b0383295798ceb49cbdc632b64..a2faeb1895e41f4c17281380478f1f2cabcc6f24 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "environment.h"
 #include "gettext.h"
@@ -211,9 +209,10 @@ static void write_rev_index_positions(struct hashfile *f,
 		hashwrite_be32(f, pack_order[i]);
 }
 
-static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
+static void write_rev_trailer(const struct git_hash_algo *hash_algo,
+			      struct hashfile *f, const unsigned char *hash)
 {
-	hashwrite(f, hash, the_hash_algo->rawsz);
+	hashwrite(f, hash, hash_algo->rawsz);
 }
 
 char *write_rev_file(const struct git_hash_algo *hash_algo,
@@ -286,7 +285,7 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,
 	write_rev_header(hash_algo, f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
-	write_rev_trailer(f, hash);
+	write_rev_trailer(hash_algo, f, hash);
 
 	if (adjust_shared_perm(path) < 0)
 		die(_("failed to make %s readable"), path);
@@ -298,11 +297,12 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,
 	return path;
 }
 
-static void write_mtimes_header(struct hashfile *f)
+static void write_mtimes_header(const struct git_hash_algo *hash_algo,
+				struct hashfile *f)
 {
 	hashwrite_be32(f, MTIMES_SIGNATURE);
 	hashwrite_be32(f, MTIMES_VERSION);
-	hashwrite_be32(f, oid_version(the_hash_algo));
+	hashwrite_be32(f, oid_version(hash_algo));
 }
 
 /*
@@ -322,12 +322,14 @@ static void write_mtimes_objects(struct hashfile *f,
 	}
 }
 
-static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)
+static void write_mtimes_trailer(const struct git_hash_algo *hash_algo,
+				 struct hashfile *f, const unsigned char *hash)
 {
-	hashwrite(f, hash, the_hash_algo->rawsz);
+	hashwrite(f, hash, hash_algo->rawsz);
 }
 
-static char *write_mtimes_file(struct packing_data *to_pack,
+static char *write_mtimes_file(const struct git_hash_algo *hash_algo,
+			       struct packing_data *to_pack,
 			       struct pack_idx_entry **objects,
 			       uint32_t nr_objects,
 			       const unsigned char *hash)
@@ -344,9 +346,9 @@ static char *write_mtimes_file(struct packing_data *to_pack,
 	mtimes_name = strbuf_detach(&tmp_file, NULL);
 	f = hashfd(fd, mtimes_name);
 
-	write_mtimes_header(f);
+	write_mtimes_header(hash_algo, f);
 	write_mtimes_objects(f, to_pack, objects, nr_objects);
-	write_mtimes_trailer(f, hash);
+	write_mtimes_trailer(hash_algo, f, hash);
 
 	if (adjust_shared_perm(mtimes_name) < 0)
 		die(_("failed to make %s readable"), mtimes_name);
@@ -575,8 +577,8 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
 				      hash, pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
-		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
-						    nr_written,
+		mtimes_tmp_name = write_mtimes_file(hash_algo, to_pack,
+						    written_list, nr_written,
 						    hash);
 	}
 

-- 
2.47.0

Re: [PATCH 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Patrick Steinhardt]

On Thu, Jan 16, 2025 at 12:35:15PM +0100, Karthik Nayak via B4 Relay wrote:
> @@ -546,7 +547,8 @@ void rename_tmp_packfile_idx(struct strbuf *name_buffer,
>  	rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx");
>  }
>  
> -void stage_tmp_packfiles(struct strbuf *name_buffer,
> +void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
> +			 struct strbuf *name_buffer,
>  			 const char *pack_tmp_name,
>  			 struct pack_idx_entry **written_list,
>  			 uint32_t nr_written,

This change was somewhat unexpected to me as it wasn't mentioned, so it
makes you wonder why it's different than `wried_idx_file()`.

Patrick

Re: [PATCH 4/5] pack-write: pass hash_algo to `write_rev_file()`

[Patrick Steinhardt]

On Thu, Jan 16, 2025 at 12:35:16PM +0100, Karthik Nayak via B4 Relay wrote:
> From: Karthik Nayak <karthik.188@gmail.com>
> 
> The `write_rev_file()` function uses the global `the_hash_algo` variable
> to access the repository's hash function. To avoid global variable
> usage, let's pass the hash function from the layers above.
> 
> Altough the layers above could have access to the hash function
> internally, simply pass in `the_hash_algo`. This avoids any
> compatibility issues and bubbles up global variable usage to upper
> layers which can be eventually resolved.
> 
> However, in `midx-write.c`, since all usage of global variables is
> removed, don't reintroduce them and instead use the `repo` available in
> the context.

Yeah, this feels quite sensible. We know this file is supposedly
`the_repository`-clean, and callers expect it to be, so reintroducing it
wouldn't be sensible.

Patrick

Re: [PATCH 2/5] pack-write: pass repository to `index_pack_lockfile()`

[Junio C Hamano]

Karthik Nayak via B4 Relay
<devnull+karthik.188.gmail.com@kernel.org> writes:

> From: Karthik Nayak <karthik.188@gmail.com>
>
> The `index_pack_lockfile()` function uses the global `the_repository`
> variable to access the repository. To avoid global variable usage, pass
> the repository from the layers above.
>
> Altough the layers above could have access to the hash function

I do not think the choice of the hash algorithm has much to do with
this change, though ;-)

> internally, simply pass in `the_hash_algo`. This avoids any
> compatibility issues and bubbles up global variable usage to upper
> layers which can be eventually resolved.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  builtin/receive-pack.c | 2 +-
>  fetch-pack.c           | 4 +++-
>  pack-write.c           | 6 +++---
>  pack.h                 | 2 +-
>  4 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 56347a79633505efe8dc05acf1583b4c9995eefe..b83abe5d220cefd3707b701409dc5e6b67566599 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -2304,7 +2304,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
>  		if (status)
>  			return "index-pack fork failed";
>  
> -		lockfile = index_pack_lockfile(child.out, NULL);
> +		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
>  		if (lockfile) {
>  			pack_lockfile = register_tempfile(lockfile);
>  			free(lockfile);
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 3a227721ed0935d1f9c40584c57f54043354c032..824f56ecbca11cd9e4da6a3e4c450c6b2e7078ab 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1036,7 +1036,9 @@ static int get_pack(struct fetch_pack_args *args,
>  		die(_("fetch-pack: unable to fork off %s"), cmd_name);
>  	if (do_keep && (pack_lockfiles || fsck_objects)) {
>  		int is_well_formed;
> -		char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);
> +		char *pack_lockfile = index_pack_lockfile(the_repository,
> +							  cmd.out,
> +							  &is_well_formed);
>  
>  		if (!is_well_formed)
>  			die(_("fetch-pack: invalid index-pack output"));
> diff --git a/pack-write.c b/pack-write.c
> index fc887850dfb9789132b8642733c6472944dbe32d..0cd75d2e55419362a61cf981fc11117ea7a1d88a 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -460,10 +460,10 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
>  	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
>  }
>  
> -char *index_pack_lockfile(int ip_out, int *is_well_formed)
> +char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed)
>  {
>  	char packname[GIT_MAX_HEXSZ + 6];
> -	const int len = the_hash_algo->hexsz + 6;
> +	const int len = r->hash_algo->hexsz + 6;
>  
>  	/*
>  	 * The first thing we expect from index-pack's output
> @@ -480,7 +480,7 @@ char *index_pack_lockfile(int ip_out, int *is_well_formed)
>  		packname[len-1] = 0;
>  		if (skip_prefix(packname, "keep\t", &name))
>  			return xstrfmt("%s/pack/pack-%s.keep",
> -				       repo_get_object_directory(the_repository), name);
> +				       repo_get_object_directory(r), name);
>  		return NULL;
>  	}
>  	if (is_well_formed)
> diff --git a/pack.h b/pack.h
> index 6d9d477adc83e83d9e9175ccf699c100b4c147c6..46d85e5bec787c90af69700fd4b328b1ebf1d606 100644
> --- a/pack.h
> +++ b/pack.h
> @@ -94,7 +94,7 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
>  void fixup_pack_header_footer(const struct git_hash_algo *, int,
>  			      unsigned char *, const char *, uint32_t,
>  			      unsigned char *, off_t);
> -char *index_pack_lockfile(int fd, int *is_well_formed);
> +char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);
>  
>  struct ref;

Re: [PATCH 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Junio C Hamano]

Karthik Nayak via B4 Relay
<devnull+karthik.188.gmail.com@kernel.org> writes:

> From: Karthik Nayak <karthik.188@gmail.com>
>
> The `write_idx_file()` function uses the global `the_hash_algo` variable
> to access the repository's hash function. To avoid global variable
> usage, pass the hash function from the layers above.
>
> Altough the layers above could have access to the hash function
> internally, simply pass in `the_hash_algo`. This avoids any
> compatibility issues and bubbles up global variable usage to upper
> layers which can be eventually resolved.
> ...
> -void stage_tmp_packfiles(struct strbuf *name_buffer,
> +void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
> +			 struct strbuf *name_buffer,
>  			 const char *pack_tmp_name,
>  			 struct pack_idx_entry **written_list,
>  			 uint32_t nr_written,
> @@ -561,8 +563,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
>  	if (adjust_shared_perm(pack_tmp_name))
>  		die_errno("unable to make temporary pack file readable");
>  
> -	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
> -					       pack_idx_opts, hash);
> +	*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,
> +					       nr_written, pack_idx_opts, hash);

The proposed log message should mention the reason why this
stage_tmp_packfiles() function needs to be singled out among many
other direct callers of write_idx_file() function.  

In other words, ...

> @@ -798,8 +798,8 @@ static const char *create_index(void)
>  	if (c != last)
>  		die("internal consistency error creating the index");
>  
> -	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,
> -				 pack_data->hash);
> +	tmpfile = write_idx_file(the_hash_algo, NULL, idx, object_count,
> +				 &pack_idx_opts, pack_data->hash);
>  	free(idx);
>  	return tmpfile;
>  }

... this hunk could have made create_index() to take a git_hash_algo
object and pass it down to write_idx_file(), while changing all the
callers of create_index() pass the_hash_algo, but we did not do so.

But stage_tmp_packfiles() got that treatment.  Please tell your
readers in the proposed log message what makes it special.

Thanks.

Re: [PATCH 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Junio C Hamano]

Karthik Nayak via B4 Relay
<devnull+karthik.188.gmail.com@kernel.org> writes:

> From: Karthik Nayak <karthik.188@gmail.com>
>
> The `fixup_pack_header_footer()` function uses the global
> `the_hash_algo` variable to access the repository's hash function. To
> avoid global variable usage, pass the hash function from the layers
> above.
>
> Altough the layers above could have access to the hash function
> internally, simply pass in `the_hash_algo`. This avoids any
> compatibility issues and bubbles up global variable usage to upper
> layers which can be eventually resolved.

OK, so the aim is to make a bug-to-bug compatible rewrite at this
step.  We used to use the_hash_algo at the lowermost callee, so have
it take a hash_algo as parameter, but have everybody pass the_hash_algo
to it, even though a caller might have a more "suitable" hash_algo
for the data it is feeding the lower layer.

Which makes sense.

Naturally, as we are producing pack stream, we ought to have an idea
on what hash function is in use to name these objects in the pack
stream and the hash function used at the csum-file level, so all
callers ought to have the information eventually (and if some of
them are relying on the_hash_algo currently, they need to be fixed).
All of that can be left for some future date and blindly passing
down the_hash_algo from all callers in this patch is a good approach
to establish a solid foundation to build on.

Re: [PATCH 4/5] pack-write: pass hash_algo to `write_rev_file()`

[Junio C Hamano]

Karthik Nayak via B4 Relay
<devnull+karthik.188.gmail.com@kernel.org> writes:

> From: Karthik Nayak <karthik.188@gmail.com>
>
> The `write_rev_file()` function uses the global `the_hash_algo` variable
> to access the repository's hash function. To avoid global variable
> usage, let's pass the hash function from the layers above.

There are a few other functions that got an extra git_hash_algo
parameter in this patch.  The changes to them all look sensible,
but it would be nice to mention them here.

Re: [PATCH 2/5] pack-write: pass repository to `index_pack_lockfile()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak via B4 Relay
> <devnull+karthik.188.gmail.com@kernel.org> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The `index_pack_lockfile()` function uses the global `the_repository`
>> variable to access the repository. To avoid global variable usage, pass
>> the repository from the layers above.
>>
>> Altough the layers above could have access to the hash function
>
> I do not think the choice of the hash algorithm has much to do with
> this change, though ;-)
>

Oops, copy-paste error. Thanks for pointing out, will fix.

[snip]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 805 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Jan 16, 2025 at 12:35:15PM +0100, Karthik Nayak via B4 Relay wrote:
>> @@ -546,7 +547,8 @@ void rename_tmp_packfile_idx(struct strbuf *name_buffer,
>>  	rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx");
>>  }
>>
>> -void stage_tmp_packfiles(struct strbuf *name_buffer,
>> +void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
>> +			 struct strbuf *name_buffer,
>>  			 const char *pack_tmp_name,
>>  			 struct pack_idx_entry **written_list,
>>  			 uint32_t nr_written,
>
> This change was somewhat unexpected to me as it wasn't mentioned, so it
> makes you wonder why it's different than `wried_idx_file()`.
>

Should've mentioned it. This was also modified since it exists in
'pack-write.c'. Will amend the commit message.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak via B4 Relay
> <devnull+karthik.188.gmail.com@kernel.org> writes:
>
>> From: Karthik Nayak <karthik.188@gmail.com>
>>
>> The `write_idx_file()` function uses the global `the_hash_algo` variable
>> to access the repository's hash function. To avoid global variable
>> usage, pass the hash function from the layers above.
>>
>> Altough the layers above could have access to the hash function
>> internally, simply pass in `the_hash_algo`. This avoids any
>> compatibility issues and bubbles up global variable usage to upper
>> layers which can be eventually resolved.
>> ...
>> -void stage_tmp_packfiles(struct strbuf *name_buffer,
>> +void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
>> +			 struct strbuf *name_buffer,
>>  			 const char *pack_tmp_name,
>>  			 struct pack_idx_entry **written_list,
>>  			 uint32_t nr_written,
>> @@ -561,8 +563,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
>>  	if (adjust_shared_perm(pack_tmp_name))
>>  		die_errno("unable to make temporary pack file readable");
>>
>> -	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
>> -					       pack_idx_opts, hash);
>> +	*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,
>> +					       nr_written, pack_idx_opts, hash);
>
> The proposed log message should mention the reason why this
> stage_tmp_packfiles() function needs to be singled out among many
> other direct callers of write_idx_file() function.
>

Agreed, Patrick also pointed out the same.

> In other words, ...
>
>> @@ -798,8 +798,8 @@ static const char *create_index(void)
>>  	if (c != last)
>>  		die("internal consistency error creating the index");
>>
>> -	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,
>> -				 pack_data->hash);
>> +	tmpfile = write_idx_file(the_hash_algo, NULL, idx, object_count,
>> +				 &pack_idx_opts, pack_data->hash);
>>  	free(idx);
>>  	return tmpfile;
>>  }
>
> ... this hunk could have made create_index() to take a git_hash_algo
> object and pass it down to write_idx_file(), while changing all the
> callers of create_index() pass the_hash_algo, but we did not do so.
>
> But stage_tmp_packfiles() got that treatment.  Please tell your
> readers in the proposed log message what makes it special.
>
> Thanks.
>

Will fix it in the next version, but in short, it was because the
function is also part of 'pack-write.c' and we're focusing only on
cleanup of 'pack-write.c'.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2 0/5] pack-write: cleanup usage of global variables

[Karthik Nayak]

This is a small series to remove global variable usage from
`pack-write.c`. Mostly it bubble's up the usage of global variables to
upper layers. The only exception is in `write-midx.c`, which was cleaned
of global variable usage, so there, we use the repo that is in available
in the context.

This series is based on fbe8d3079d (Git 2.48, 2025-01-10) with
'ps/more-sign-compare' and 'ps/the-repository' merged in.

There are no conflicts with topics in 'next', however there is a
conflict with 'tb/incremental-midx-part-2' in 'seen', the fix is simple
but happy to merge that in too if necessary.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v2:
- Fixes to the commit messages to:
  - Fix copy-paste error s/the_hash_algo/the_repository
  - Mention why certain functions are modified
  - Small cleanups
- Link to v1: https://lore.kernel.org/r/20250116-kn-the-repo-cleanup-v1-0-a2f4c8e1c4c3@gmail.com

---
Karthik Nayak (5):
      pack-write: pass hash_algo to `fixup_pack_header_footer()`
      pack-write: pass repository to `index_pack_lockfile()`
      pack-write: pass hash_algo to `write_idx_file()`
      pack-write: pass hash_algo to `write_rev_file()`
      pack-write: pass hash_algo to internal functions

 builtin/fast-import.c  | 11 +++---
 builtin/index-pack.c   | 11 +++---
 builtin/pack-objects.c | 12 +++---
 builtin/receive-pack.c |  2 +-
 bulk-checkin.c         |  7 ++--
 fetch-pack.c           |  4 +-
 midx-write.c           |  4 +-
 pack-write.c           | 99 +++++++++++++++++++++++++++-----------------------
 pack.h                 | 30 ++++++++++++---
 9 files changed, 106 insertions(+), 74 deletions(-)
---

Range-diff versus v1:

1:  4e365523a5 = 1:  ed4ba01c95 pack-write: pass hash_algo to `fixup_pack_header_footer()`
2:  efa224f83b ! 2:  10aeaa5afc pack-write: pass repository to `index_pack_lockfile()`
    @@ Commit message
         variable to access the repository. To avoid global variable usage, pass
         the repository from the layers above.
     
    -    Altough the layers above could have access to the hash function
    -    internally, simply pass in `the_hash_algo`. This avoids any
    -    compatibility issues and bubbles up global variable usage to upper
    -    layers which can be eventually resolved.
    +    Altough the layers above could have access to the repository internally,
    +    simply pass in `the_repository`. This avoids any compatibility issues
    +    and bubbles up global variable usage to upper layers which can be
    +    eventually resolved.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
3:  6cca689d32 ! 3:  a4dbc3906d pack-write: pass hash_algo to `write_idx_file()`
    @@ Commit message
         to access the repository's hash function. To avoid global variable
         usage, pass the hash function from the layers above.
     
    +    Since `stage_tmp_packfiles()` also resides in 'pack-write.c' and calls
    +    `write_idx_file()`, update it to accept `the_hash_algo` as a parameter
    +    and pass it through to the callee.
    +
         Altough the layers above could have access to the hash function
         internally, simply pass in `the_hash_algo`. This avoids any
         compatibility issues and bubbles up global variable usage to upper
4:  b29eb6a305 ! 4:  d06d8ee962 pack-write: pass hash_algo to `write_rev_file()`
    @@ Commit message
     
         The `write_rev_file()` function uses the global `the_hash_algo` variable
         to access the repository's hash function. To avoid global variable
    -    usage, let's pass the hash function from the layers above.
    +    usage, let's pass the hash function from the layers above. Also modify
    +    children functions `write_rev_file_order()` and `write_rev_header()` to
    +    accept 'the_hash_algo'.
     
         Altough the layers above could have access to the hash function
         internally, simply pass in `the_hash_algo`. This avoids any
5:  438bfec4de ! 5:  1e34f5a70c pack-write: pass hash_algo to `write_rev_*()`
    @@ Metadata
     Author: Karthik Nayak <karthik.188@gmail.com>
     
      ## Commit message ##
    -    pack-write: pass hash_algo to `write_rev_*()`
    +    pack-write: pass hash_algo to internal functions
     
    -    The `write_rev_*()` functions use the global `the_hash_algo` variable to
    -    access the repository's hash function. Pass the hash from down as we've
    -    added made them available in the previous few commits.
    +    The internal functions `write_rev_trailer()`, `write_rev_trailer()`,
    +    `write_mtimes_header()` and write_mtimes_trailer()` use the global
    +    `the_hash_algo` variable to access the repository's hash function. Pass
    +    the hash from down as we've added made them available in the previous
    +    few commits.
    +
    +    This removes all global variables from the 'pack-write.c' file, so
    +    remove the 'USE_THE_REPOSITORY_VARIABLE' macro.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     


---

base-commit: 8b2efc058aaa3d1437678616bccf7c5f7ce1f92b
change-id: 20250110-kn-the-repo-cleanup-44144fa42dc3

Thanks
- Karthik

[PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Karthik Nayak]

The `fixup_pack_header_footer()` function uses the global
`the_hash_algo` variable to access the repository's hash function. To
avoid global variable usage, pass the hash function from the layers
above.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c  |  7 ++++---
 builtin/index-pack.c   |  2 +-
 builtin/pack-objects.c |  5 +++--
 bulk-checkin.c         |  2 +-
 pack-write.c           | 28 ++++++++++++++--------------
 pack.h                 |  4 +++-
 6 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa..6baf2b1b71e2443a7987a41e0cd246076682bf58 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -878,9 +878,10 @@ static void end_packfile(void)
 
 		close_pack_windows(pack_data);
 		finalize_hashfile(pack_file, cur_pack_oid.hash, FSYNC_COMPONENT_PACK, 0);
-		fixup_pack_header_footer(pack_data->pack_fd, pack_data->hash,
-					 pack_data->pack_name, object_count,
-					 cur_pack_oid.hash, pack_size);
+		fixup_pack_header_footer(the_hash_algo, pack_data->pack_fd,
+					 pack_data->hash, pack_data->pack_name,
+					 object_count, cur_pack_oid.hash,
+					 pack_size);
 
 		if (object_count <= unpack_limit) {
 			if (!loosen_small_pack(pack_data)) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0bef61c57232e198ba539cd44ec301d26dcb0eb8..6c5e3483f4fe67fb2e26132c55b1f8395d60c11f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1390,7 +1390,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 		strbuf_release(&msg);
 		finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
 		hashcpy(read_hash, pack_hash, the_repository->hash_algo);
-		fixup_pack_header_footer(output_fd, pack_hash,
+		fixup_pack_header_footer(the_hash_algo, output_fd, pack_hash,
 					 curr_pack, nr_objects,
 					 read_hash, consumed_bytes-the_hash_algo->rawsz);
 		if (!hasheq(read_hash, tail_hash, the_repository->hash_algo))
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d51c021d99d9f470c04b7ec52565ab2f4c1c19ae..ffc62930b68c9b4152057572ede216381a4b0991 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1319,8 +1319,9 @@ static void write_pack_file(void)
 			 */
 
 			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
-			fixup_pack_header_footer(fd, hash, pack_tmp_name,
-						 nr_written, hash, offset);
+			fixup_pack_header_footer(the_hash_algo, fd, hash,
+						 pack_tmp_name, nr_written,
+						 hash, offset);
 			close(fd);
 			if (write_bitmap_index) {
 				if (write_bitmap_index != WRITE_BITMAP_QUIET)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 5044cb7fa083d692a3797e2491c27b61ec44c69c..c4b085f57f74fb8b998576ac9d84fed9e01ed0ed 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -70,7 +70,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 				  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 	} else {
 		int fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);
-		fixup_pack_header_footer(fd, hash, state->pack_tmp_name,
+		fixup_pack_header_footer(the_hash_algo, fd, hash, state->pack_tmp_name,
 					 state->nr_written, hash,
 					 state->offset);
 		close(fd);
diff --git a/pack-write.c b/pack-write.c
index 98a8c0e7853d7b46b5ce9a9672e0249ff051b5f9..fc887850dfb9789132b8642733c6472944dbe32d 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -380,7 +380,8 @@ off_t write_pack_header(struct hashfile *f, uint32_t nr_entries)
  * partial_pack_sha1 can refer to the same buffer if the caller is not
  * interested in the resulting SHA1 of pack data above partial_pack_offset.
  */
-void fixup_pack_header_footer(int pack_fd,
+void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
+			 int pack_fd,
 			 unsigned char *new_pack_hash,
 			 const char *pack_name,
 			 uint32_t object_count,
@@ -393,8 +394,8 @@ void fixup_pack_header_footer(int pack_fd,
 	char *buf;
 	ssize_t read_result;
 
-	the_hash_algo->init_fn(&old_hash_ctx);
-	the_hash_algo->init_fn(&new_hash_ctx);
+	hash_algo->init_fn(&old_hash_ctx);
+	hash_algo->init_fn(&new_hash_ctx);
 
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
@@ -406,9 +407,9 @@ void fixup_pack_header_footer(int pack_fd,
 			  pack_name);
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
-	the_hash_algo->update_fn(&old_hash_ctx, &hdr, sizeof(hdr));
+	hash_algo->update_fn(&old_hash_ctx, &hdr, sizeof(hdr));
 	hdr.hdr_entries = htonl(object_count);
-	the_hash_algo->update_fn(&new_hash_ctx, &hdr, sizeof(hdr));
+	hash_algo->update_fn(&new_hash_ctx, &hdr, sizeof(hdr));
 	write_or_die(pack_fd, &hdr, sizeof(hdr));
 	partial_pack_offset -= sizeof(hdr);
 
@@ -423,7 +424,7 @@ void fixup_pack_header_footer(int pack_fd,
 			break;
 		if (n < 0)
 			die_errno("Failed to checksum '%s'", pack_name);
-		the_hash_algo->update_fn(&new_hash_ctx, buf, n);
+		hash_algo->update_fn(&new_hash_ctx, buf, n);
 
 		aligned_sz -= n;
 		if (!aligned_sz)
@@ -432,13 +433,12 @@ void fixup_pack_header_footer(int pack_fd,
 		if (!partial_pack_hash)
 			continue;
 
-		the_hash_algo->update_fn(&old_hash_ctx, buf, n);
+		hash_algo->update_fn(&old_hash_ctx, buf, n);
 		partial_pack_offset -= n;
 		if (partial_pack_offset == 0) {
 			unsigned char hash[GIT_MAX_RAWSZ];
-			the_hash_algo->final_fn(hash, &old_hash_ctx);
-			if (!hasheq(hash, partial_pack_hash,
-				    the_repository->hash_algo))
+			hash_algo->final_fn(hash, &old_hash_ctx);
+			if (!hasheq(hash, partial_pack_hash, hash_algo))
 				die("Unexpected checksum for %s "
 				    "(disk corruption?)", pack_name);
 			/*
@@ -446,7 +446,7 @@ void fixup_pack_header_footer(int pack_fd,
 			 * pack, which also means making partial_pack_offset
 			 * big enough not to matter anymore.
 			 */
-			the_hash_algo->init_fn(&old_hash_ctx);
+			hash_algo->init_fn(&old_hash_ctx);
 			partial_pack_offset = ~partial_pack_offset;
 			partial_pack_offset -= MSB(partial_pack_offset, 1);
 		}
@@ -454,9 +454,9 @@ void fixup_pack_header_footer(int pack_fd,
 	free(buf);
 
 	if (partial_pack_hash)
-		the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
-	the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
-	write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz);
+		hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
+	hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
+	write_or_die(pack_fd, new_pack_hash, hash_algo->rawsz);
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
diff --git a/pack.h b/pack.h
index a8da0406299bf267205256aaa8efb215f2ff73be..6d9d477adc83e83d9e9175ccf699c100b4c147c6 100644
--- a/pack.h
+++ b/pack.h
@@ -91,7 +91,9 @@ int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offs
 int verify_pack_index(struct packed_git *);
 int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);
 off_t write_pack_header(struct hashfile *f, uint32_t);
-void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
+void fixup_pack_header_footer(const struct git_hash_algo *, int,
+			      unsigned char *, const char *, uint32_t,
+			      unsigned char *, off_t);
 char *index_pack_lockfile(int fd, int *is_well_formed);
 
 struct ref;

-- 
2.47.0

[PATCH v2 2/5] pack-write: pass repository to `index_pack_lockfile()`

[Karthik Nayak]

The `index_pack_lockfile()` function uses the global `the_repository`
variable to access the repository. To avoid global variable usage, pass
the repository from the layers above.

Altough the layers above could have access to the repository internally,
simply pass in `the_repository`. This avoids any compatibility issues
and bubbles up global variable usage to upper layers which can be
eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 2 +-
 fetch-pack.c           | 4 +++-
 pack-write.c           | 6 +++---
 pack.h                 | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56347a79633505efe8dc05acf1583b4c9995eefe..b83abe5d220cefd3707b701409dc5e6b67566599 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2304,7 +2304,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (status)
 			return "index-pack fork failed";
 
-		lockfile = index_pack_lockfile(child.out, NULL);
+		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
 		if (lockfile) {
 			pack_lockfile = register_tempfile(lockfile);
 			free(lockfile);
diff --git a/fetch-pack.c b/fetch-pack.c
index 3a227721ed0935d1f9c40584c57f54043354c032..824f56ecbca11cd9e4da6a3e4c450c6b2e7078ab 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1036,7 +1036,9 @@ static int get_pack(struct fetch_pack_args *args,
 		die(_("fetch-pack: unable to fork off %s"), cmd_name);
 	if (do_keep && (pack_lockfiles || fsck_objects)) {
 		int is_well_formed;
-		char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);
+		char *pack_lockfile = index_pack_lockfile(the_repository,
+							  cmd.out,
+							  &is_well_formed);
 
 		if (!is_well_formed)
 			die(_("fetch-pack: invalid index-pack output"));
diff --git a/pack-write.c b/pack-write.c
index fc887850dfb9789132b8642733c6472944dbe32d..0cd75d2e55419362a61cf981fc11117ea7a1d88a 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -460,10 +460,10 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
-char *index_pack_lockfile(int ip_out, int *is_well_formed)
+char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed)
 {
 	char packname[GIT_MAX_HEXSZ + 6];
-	const int len = the_hash_algo->hexsz + 6;
+	const int len = r->hash_algo->hexsz + 6;
 
 	/*
 	 * The first thing we expect from index-pack's output
@@ -480,7 +480,7 @@ char *index_pack_lockfile(int ip_out, int *is_well_formed)
 		packname[len-1] = 0;
 		if (skip_prefix(packname, "keep\t", &name))
 			return xstrfmt("%s/pack/pack-%s.keep",
-				       repo_get_object_directory(the_repository), name);
+				       repo_get_object_directory(r), name);
 		return NULL;
 	}
 	if (is_well_formed)
diff --git a/pack.h b/pack.h
index 6d9d477adc83e83d9e9175ccf699c100b4c147c6..46d85e5bec787c90af69700fd4b328b1ebf1d606 100644
--- a/pack.h
+++ b/pack.h
@@ -94,7 +94,7 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(const struct git_hash_algo *, int,
 			      unsigned char *, const char *, uint32_t,
 			      unsigned char *, off_t);
-char *index_pack_lockfile(int fd, int *is_well_formed);
+char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);
 
 struct ref;
 

-- 
2.47.0

[PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Karthik Nayak]

The `write_idx_file()` function uses the global `the_hash_algo` variable
to access the repository's hash function. To avoid global variable
usage, pass the hash function from the layers above.

Since `stage_tmp_packfiles()` also resides in 'pack-write.c' and calls
`write_idx_file()`, update it to accept `the_hash_algo` as a parameter
and pass it through to the callee.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c  |  4 ++--
 builtin/index-pack.c   |  3 ++-
 builtin/pack-objects.c |  7 ++++---
 bulk-checkin.c         |  5 +++--
 pack-write.c           | 14 ++++++++------
 pack.h                 | 10 ++++++++--
 6 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 6baf2b1b71e2443a7987a41e0cd246076682bf58..c4bc52f93c011f34bd7f98c8e9d74a33cf9783bd 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -798,8 +798,8 @@ static const char *create_index(void)
 	if (c != last)
 		die("internal consistency error creating the index");
 
-	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,
-				 pack_data->hash);
+	tmpfile = write_idx_file(the_hash_algo, NULL, idx, object_count,
+				 &pack_idx_opts, pack_data->hash);
 	free(idx);
 	return tmpfile;
 }
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 6c5e3483f4fe67fb2e26132c55b1f8395d60c11f..d73699653a2227f8ecfee2c0f51cd680093ac764 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -2096,7 +2096,8 @@ int cmd_index_pack(int argc,
 	ALLOC_ARRAY(idx_objects, nr_objects);
 	for (i = 0; i < nr_objects; i++)
 		idx_objects[i] = &objects[i].idx;
-	curr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_hash);
+	curr_index = write_idx_file(the_hash_algo, index_name, idx_objects,
+				    nr_objects, &opts, pack_hash);
 	if (rev_index)
 		curr_rev_index = write_rev_file(rev_index_name, idx_objects,
 						nr_objects, pack_hash,
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ffc62930b68c9b4152057572ede216381a4b0991..7b2dacda514c29f5393b604f15d379abb81546c0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1369,9 +1369,10 @@ static void write_pack_file(void)
 			if (cruft)
 				pack_idx_opts.flags |= WRITE_MTIMES;
 
-			stage_tmp_packfiles(&tmpname, pack_tmp_name,
-					    written_list, nr_written,
-					    &to_pack, &pack_idx_opts, hash,
+			stage_tmp_packfiles(the_hash_algo, &tmpname,
+					    pack_tmp_name, written_list,
+					    nr_written, &to_pack,
+					    &pack_idx_opts, hash,
 					    &idx_tmp_name);
 
 			if (write_bitmap_index) {
diff --git a/bulk-checkin.c b/bulk-checkin.c
index c4b085f57f74fb8b998576ac9d84fed9e01ed0ed..0d49889bfbb233d58fc094f5e2c2d2433dad9851 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -44,8 +44,9 @@ static void finish_tmp_packfile(struct strbuf *basename,
 {
 	char *idx_tmp_name = NULL;
 
-	stage_tmp_packfiles(basename, pack_tmp_name, written_list, nr_written,
-			    NULL, pack_idx_opts, hash, &idx_tmp_name);
+	stage_tmp_packfiles(the_hash_algo, basename, pack_tmp_name,
+			    written_list, nr_written, NULL, pack_idx_opts, hash,
+			    &idx_tmp_name);
 	rename_tmp_packfile_idx(basename, &idx_tmp_name);
 
 	free(idx_tmp_name);
diff --git a/pack-write.c b/pack-write.c
index 0cd75d2e55419362a61cf981fc11117ea7a1d88a..f344e78a9ec20cea9812a5eaffc72ae0b7e7424d 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -56,7 +56,8 @@ static int need_large_offset(off_t offset, const struct pack_idx_option *opts)
  * The *sha1 contains the pack content SHA1 hash.
  * The objects array passed in will be sorted by SHA1 on exit.
  */
-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,
+const char *write_idx_file(const struct git_hash_algo *hash_algo,
+			   const char *index_name, struct pack_idx_entry **objects,
 			   int nr_objects, const struct pack_idx_option *opts,
 			   const unsigned char *sha1)
 {
@@ -130,7 +131,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		struct pack_idx_entry *obj = *list++;
 		if (index_version < 2)
 			hashwrite_be32(f, obj->offset);
-		hashwrite(f, obj->oid.hash, the_hash_algo->rawsz);
+		hashwrite(f, obj->oid.hash, hash_algo->rawsz);
 		if ((opts->flags & WRITE_IDX_STRICT) &&
 		    (i && oideq(&list[-2]->oid, &obj->oid)))
 			die("The same object %s appears twice in the pack",
@@ -172,7 +173,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		}
 	}
 
-	hashwrite(f, sha1, the_hash_algo->rawsz);
+	hashwrite(f, sha1, hash_algo->rawsz);
 	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
 			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
 			  ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
@@ -546,7 +547,8 @@ void rename_tmp_packfile_idx(struct strbuf *name_buffer,
 	rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx");
 }
 
-void stage_tmp_packfiles(struct strbuf *name_buffer,
+void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
+			 struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,
 			 uint32_t nr_written,
@@ -561,8 +563,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 	if (adjust_shared_perm(pack_tmp_name))
 		die_errno("unable to make temporary pack file readable");
 
-	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
-					       pack_idx_opts, hash);
+	*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,
+					       nr_written, pack_idx_opts, hash);
 	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
diff --git a/pack.h b/pack.h
index 46d85e5bec787c90af69700fd4b328b1ebf1d606..c650fdbe2dcde8055ad0efe55646338cd0f81df5 100644
--- a/pack.h
+++ b/pack.h
@@ -86,7 +86,12 @@ struct progress;
 /* Note, the data argument could be NULL if object type is blob */
 typedef int (*verify_fn)(const struct object_id *, enum object_type, unsigned long, void*, int*);
 
-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);
+const char *write_idx_file(const struct git_hash_algo *hash_algo,
+			   const char *index_name,
+			   struct pack_idx_entry **objects,
+			   int nr_objects,
+			   const struct pack_idx_option *,
+			   const unsigned char *sha1);
 int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
 int verify_pack_index(struct packed_git *);
 int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);
@@ -119,7 +124,8 @@ int read_pack_header(int fd, struct pack_header *);
 struct packing_data;
 
 struct hashfile *create_tmp_packfile(char **pack_tmp_name);
-void stage_tmp_packfiles(struct strbuf *name_buffer,
+void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
+			 struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,
 			 uint32_t nr_written,

-- 
2.47.0

[PATCH v2 4/5] pack-write: pass hash_algo to `write_rev_file()`

[Karthik Nayak]

The `write_rev_file()` function uses the global `the_hash_algo` variable
to access the repository's hash function. To avoid global variable
usage, let's pass the hash function from the layers above. Also modify
children functions `write_rev_file_order()` and `write_rev_header()` to
accept 'the_hash_algo'.

Altough the layers above could have access to the hash function
internally, simply pass in `the_hash_algo`. This avoids any
compatibility issues and bubbles up global variable usage to upper
layers which can be eventually resolved.

However, in `midx-write.c`, since all usage of global variables is
removed, don't reintroduce them and instead use the `repo` available in
the context.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/index-pack.c |  6 +++---
 midx-write.c         |  4 ++--
 pack-write.c         | 21 ++++++++++++---------
 pack.h               | 14 ++++++++++++--
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d73699653a2227f8ecfee2c0f51cd680093ac764..e803cb2444633f937fafc841848dae85341475f1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -2099,9 +2099,9 @@ int cmd_index_pack(int argc,
 	curr_index = write_idx_file(the_hash_algo, index_name, idx_objects,
 				    nr_objects, &opts, pack_hash);
 	if (rev_index)
-		curr_rev_index = write_rev_file(rev_index_name, idx_objects,
-						nr_objects, pack_hash,
-						opts.flags);
+		curr_rev_index = write_rev_file(the_hash_algo, rev_index_name,
+						idx_objects, nr_objects,
+						pack_hash, opts.flags);
 	free(idx_objects);
 
 	if (!verify)
diff --git a/midx-write.c b/midx-write.c
index b3827b936bdb1df12c73fb7d9b98ff65fc875cc3..61b59d557d3ba46a39db74c62393545cabf50f2c 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -658,8 +658,8 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 	strbuf_addf(&buf, "%s-%s.rev", midx_name, hash_to_hex_algop(midx_hash,
 								    ctx->repo->hash_algo));
 
-	tmp_file = write_rev_file_order(NULL, ctx->pack_order, ctx->entries_nr,
-					midx_hash, WRITE_REV);
+	tmp_file = write_rev_file_order(ctx->repo->hash_algo, NULL, ctx->pack_order,
+					ctx->entries_nr, midx_hash, WRITE_REV);
 
 	if (finalize_object_file(tmp_file, buf.buf))
 		die(_("cannot store reverse index file"));
diff --git a/pack-write.c b/pack-write.c
index f344e78a9ec20cea9812a5eaffc72ae0b7e7424d..09ecbcdb069cc9b0383295798ceb49cbdc632b64 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -194,11 +194,12 @@ static int pack_order_cmp(const void *va, const void *vb, void *ctx)
 	return 0;
 }
 
-static void write_rev_header(struct hashfile *f)
+static void write_rev_header(const struct git_hash_algo *hash_algo,
+			     struct hashfile *f)
 {
 	hashwrite_be32(f, RIDX_SIGNATURE);
 	hashwrite_be32(f, RIDX_VERSION);
-	hashwrite_be32(f, oid_version(the_hash_algo));
+	hashwrite_be32(f, oid_version(hash_algo));
 }
 
 static void write_rev_index_positions(struct hashfile *f,
@@ -215,7 +216,8 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
 	hashwrite(f, hash, the_hash_algo->rawsz);
 }
 
-char *write_rev_file(const char *rev_name,
+char *write_rev_file(const struct git_hash_algo *hash_algo,
+		     const char *rev_name,
 		     struct pack_idx_entry **objects,
 		     uint32_t nr_objects,
 		     const unsigned char *hash,
@@ -233,15 +235,16 @@ char *write_rev_file(const char *rev_name,
 		pack_order[i] = i;
 	QSORT_S(pack_order, nr_objects, pack_order_cmp, objects);
 
-	ret = write_rev_file_order(rev_name, pack_order, nr_objects, hash,
-				   flags);
+	ret = write_rev_file_order(hash_algo, rev_name, pack_order, nr_objects,
+				   hash, flags);
 
 	free(pack_order);
 
 	return ret;
 }
 
-char *write_rev_file_order(const char *rev_name,
+char *write_rev_file_order(const struct git_hash_algo *hash_algo,
+			   const char *rev_name,
 			   uint32_t *pack_order,
 			   uint32_t nr_objects,
 			   const unsigned char *hash,
@@ -280,7 +283,7 @@ char *write_rev_file_order(const char *rev_name,
 		return NULL;
 	}
 
-	write_rev_header(f);
+	write_rev_header(hash_algo, f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
 	write_rev_trailer(f, hash);
@@ -568,8 +571,8 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
 	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
-	rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,
-				      pack_idx_opts->flags);
+	rev_tmp_name = write_rev_file(hash_algo, NULL, written_list, nr_written,
+				      hash, pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
 		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
diff --git a/pack.h b/pack.h
index c650fdbe2dcde8055ad0efe55646338cd0f81df5..8a84ea475cda902936ee0b6091c5b4d462606be8 100644
--- a/pack.h
+++ b/pack.h
@@ -105,8 +105,18 @@ struct ref;
 
 void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
 
-char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
-char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
+char *write_rev_file(const struct git_hash_algo *hash_algo,
+		     const char *rev_name,
+		     struct pack_idx_entry **objects,
+		     uint32_t nr_objects,
+		     const unsigned char *hash,
+		     unsigned flags);
+char *write_rev_file_order(const struct git_hash_algo *hash_algo,
+			   const char *rev_name,
+			   uint32_t *pack_order,
+			   uint32_t nr_objects,
+			   const unsigned char *hash,
+			   unsigned flags);
 
 /*
  * The "hdr" output buffer should be at least this big, which will handle sizes

-- 
2.47.0

[PATCH v2 5/5] pack-write: pass hash_algo to internal functions

[Karthik Nayak]

The internal functions `write_rev_trailer()`, `write_rev_trailer()`,
`write_mtimes_header()` and write_mtimes_trailer()` use the global
`the_hash_algo` variable to access the repository's hash function. Pass
the hash from down as we've added made them available in the previous
few commits.

This removes all global variables from the 'pack-write.c' file, so
remove the 'USE_THE_REPOSITORY_VARIABLE' macro.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 pack-write.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index 09ecbcdb069cc9b0383295798ceb49cbdc632b64..a2faeb1895e41f4c17281380478f1f2cabcc6f24 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "environment.h"
 #include "gettext.h"
@@ -211,9 +209,10 @@ static void write_rev_index_positions(struct hashfile *f,
 		hashwrite_be32(f, pack_order[i]);
 }
 
-static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
+static void write_rev_trailer(const struct git_hash_algo *hash_algo,
+			      struct hashfile *f, const unsigned char *hash)
 {
-	hashwrite(f, hash, the_hash_algo->rawsz);
+	hashwrite(f, hash, hash_algo->rawsz);
 }
 
 char *write_rev_file(const struct git_hash_algo *hash_algo,
@@ -286,7 +285,7 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,
 	write_rev_header(hash_algo, f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
-	write_rev_trailer(f, hash);
+	write_rev_trailer(hash_algo, f, hash);
 
 	if (adjust_shared_perm(path) < 0)
 		die(_("failed to make %s readable"), path);
@@ -298,11 +297,12 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,
 	return path;
 }
 
-static void write_mtimes_header(struct hashfile *f)
+static void write_mtimes_header(const struct git_hash_algo *hash_algo,
+				struct hashfile *f)
 {
 	hashwrite_be32(f, MTIMES_SIGNATURE);
 	hashwrite_be32(f, MTIMES_VERSION);
-	hashwrite_be32(f, oid_version(the_hash_algo));
+	hashwrite_be32(f, oid_version(hash_algo));
 }
 
 /*
@@ -322,12 +322,14 @@ static void write_mtimes_objects(struct hashfile *f,
 	}
 }
 
-static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)
+static void write_mtimes_trailer(const struct git_hash_algo *hash_algo,
+				 struct hashfile *f, const unsigned char *hash)
 {
-	hashwrite(f, hash, the_hash_algo->rawsz);
+	hashwrite(f, hash, hash_algo->rawsz);
 }
 
-static char *write_mtimes_file(struct packing_data *to_pack,
+static char *write_mtimes_file(const struct git_hash_algo *hash_algo,
+			       struct packing_data *to_pack,
 			       struct pack_idx_entry **objects,
 			       uint32_t nr_objects,
 			       const unsigned char *hash)
@@ -344,9 +346,9 @@ static char *write_mtimes_file(struct packing_data *to_pack,
 	mtimes_name = strbuf_detach(&tmp_file, NULL);
 	f = hashfd(fd, mtimes_name);
 
-	write_mtimes_header(f);
+	write_mtimes_header(hash_algo, f);
 	write_mtimes_objects(f, to_pack, objects, nr_objects);
-	write_mtimes_trailer(f, hash);
+	write_mtimes_trailer(hash_algo, f, hash);
 
 	if (adjust_shared_perm(mtimes_name) < 0)
 		die(_("failed to make %s readable"), mtimes_name);
@@ -575,8 +577,8 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
 				      hash, pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
-		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
-						    nr_written,
+		mtimes_tmp_name = write_mtimes_file(hash_algo, to_pack,
+						    written_list, nr_written,
 						    hash);
 	}
 

-- 
2.47.0

Re: [PATCH v2 5/5] pack-write: pass hash_algo to internal functions

[Patrick Steinhardt]

On Fri, Jan 17, 2025 at 10:20:52AM +0100, Karthik Nayak wrote:
> The internal functions `write_rev_trailer()`, `write_rev_trailer()`,
> `write_mtimes_header()` and write_mtimes_trailer()` use the global
> `the_hash_algo` variable to access the repository's hash function. Pass
> the hash from down as we've added made them available in the previous

This doesn't read quite right -- from where do we want to pass it down?
Other than that the series looks good to me, thanks!

Patrick

Re: [PATCH v2 5/5] pack-write: pass hash_algo to internal functions

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Jan 17, 2025 at 10:20:52AM +0100, Karthik Nayak wrote:
>> The internal functions `write_rev_trailer()`, `write_rev_trailer()`,
>> `write_mtimes_header()` and write_mtimes_trailer()` use the global
>> `the_hash_algo` variable to access the repository's hash function. Pass
>> the hash from down as we've added made them available in the previous
>
> This doesn't read quite right -- from where do we want to pass it down?
> Other than that the series looks good to me, thanks!
>

I should have s/from//, but let me rephrase it to make it clearer.

Thanks for the review.

> Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Toon Claes]

Karthik Nayak <karthik.188@gmail.com> writes:

> The `fixup_pack_header_footer()` function uses the global
> `the_hash_algo` variable to access the repository's hash function. To
> avoid global variable usage, pass the hash function from the layers
> above.

I'm probably being overly pedantic here, so feel free to ignore me. But
you say "pass the hash function", technically that's not correct, you're
passing down the struct that defines several properties of the hashing
algorithm. This includes the hash function, but also other properties
like the hex size. By using "pass the hash function" in the commit
messages (and not only this commit message) it sounds to me like you're
changing the type of the object that desribes the "hash algo". But
again, feel free to ignore this comment.

-- 
Toon

Re: [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Toon Claes]

Karthik Nayak <karthik.188@gmail.com> writes:

> The `write_idx_file()` function uses the global `the_hash_algo` variable
> to access the repository's hash function. To avoid global variable
> usage, pass the hash function from the layers above.
>
> Since `stage_tmp_packfiles()` also resides in 'pack-write.c' and calls
> `write_idx_file()`, update it to accept `the_hash_algo` as a parameter
> and pass it through to the callee.

Technically speaking you're updating it to accept a `struct hash_algo`.

Besides from this nit, and the other nit I've submitted on the first
patch, these changes look good to me. I'm doubtful any of the comments
needs a reroll.

-- 
Toon

Re: [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The `fixup_pack_header_footer()` function uses the global
>> `the_hash_algo` variable to access the repository's hash function. To
>> avoid global variable usage, pass the hash function from the layers
>> above.
>
> I'm probably being overly pedantic here, so feel free to ignore me. But
> you say "pass the hash function", technically that's not correct, you're
> passing down the struct that defines several properties of the hashing
> algorithm. This includes the hash function, but also other properties
> like the hex size. By using "pass the hash function" in the commit
> messages (and not only this commit message) it sounds to me like you're
> changing the type of the object that desribes the "hash algo". But
> again, feel free to ignore this comment.

I think the phrasing in the title that uses hash_algo (instead of
"hash function") is fine, so we can use "pass a hash_algo from the
layers above" in the body, perhaps?

Re: [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Toon Claes <toon@iotcl.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> The `fixup_pack_header_footer()` function uses the global
>>> `the_hash_algo` variable to access the repository's hash function. To
>>> avoid global variable usage, pass the hash function from the layers
>>> above.
>>
>> I'm probably being overly pedantic here, so feel free to ignore me. But
>> you say "pass the hash function", technically that's not correct, you're
>> passing down the struct that defines several properties of the hashing
>> algorithm. This includes the hash function, but also other properties
>> like the hex size. By using "pass the hash function" in the commit
>> messages (and not only this commit message) it sounds to me like you're
>> changing the type of the object that desribes the "hash algo". But
>> again, feel free to ignore this comment.
>
> I think the phrasing in the title that uses hash_algo (instead of
> "hash function") is fine, so we can use "pass a hash_algo from the
> layers above" in the body, perhaps?

This is a good suggestion, I'll amend! Thanks both.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

Toon Claes <toon@iotcl.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> The `write_idx_file()` function uses the global `the_hash_algo` variable
>> to access the repository's hash function. To avoid global variable
>> usage, pass the hash function from the layers above.
>>
>> Since `stage_tmp_packfiles()` also resides in 'pack-write.c' and calls
>> `write_idx_file()`, update it to accept `the_hash_algo` as a parameter
>> and pass it through to the callee.
>
> Technically speaking you're updating it to accept a `struct hash_algo`.
>

Yes, but also the callers pass in `the_hash_algo`. But let me amend to
make it better.

> Besides from this nit, and the other nit I've submitted on the first
> patch, these changes look good to me. I'm doubtful any of the comments
> needs a reroll.
>
> --
> Toon

Thanks for the review! I'll re-roll with the fixes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v3 0/5] pack-write: cleanup usage of global variables

[Karthik Nayak]

This is a small series to remove global variable usage from
`pack-write.c`. Mostly it bubble's up the usage of global variables to
upper layers. The only exception is in `write-midx.c`, which was cleaned
of global variable usage, so there, we use the repo that is in available
in the context.

This series is based on fbe8d3079d (Git 2.48, 2025-01-10) with
'ps/more-sign-compare' and 'ps/the-repository' merged in.

There are no conflicts with topics in 'next', however there is a
conflict with 'tb/incremental-midx-part-2' in 'seen', the fix is simple
but happy to merge that in too if necessary.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v3:
- Fix the ambiguous commit message in the last commit.
- Link to v2: https://lore.kernel.org/r/20250117-kn-the-repo-cleanup-v2-0-a7fdc19688f5@gmail.com

Changes in v2:
- Fixes to the commit messages to:
  - Fix copy-paste error s/the_hash_algo/the_repository
  - Mention why certain functions are modified
  - Small cleanups
- Link to v1: https://lore.kernel.org/r/20250116-kn-the-repo-cleanup-v1-0-a2f4c8e1c4c3@gmail.com

---
Karthik Nayak (5):
      pack-write: pass hash_algo to `fixup_pack_header_footer()`
      pack-write: pass repository to `index_pack_lockfile()`
      pack-write: pass hash_algo to `write_idx_file()`
      pack-write: pass hash_algo to `write_rev_file()`
      pack-write: pass hash_algo to internal functions

 builtin/fast-import.c  | 11 +++---
 builtin/index-pack.c   | 11 +++---
 builtin/pack-objects.c | 12 +++---
 builtin/receive-pack.c |  2 +-
 bulk-checkin.c         |  7 ++--
 fetch-pack.c           |  4 +-
 midx-write.c           |  4 +-
 pack-write.c           | 99 +++++++++++++++++++++++++++-----------------------
 pack.h                 | 30 ++++++++++++---
 9 files changed, 106 insertions(+), 74 deletions(-)
---

Range-diff versus v2:

1:  d3812f88e6 ! 1:  5f646111d0 pack-write: pass hash_algo to `fixup_pack_header_footer()`
    @@ Commit message
     
         The `fixup_pack_header_footer()` function uses the global
         `the_hash_algo` variable to access the repository's hash function. To
    -    avoid global variable usage, pass the hash function from the layers
    -    above.
    +    avoid global variable usage, pass a hash_algo from the layers above.
     
    -    Altough the layers above could have access to the hash function
    -    internally, simply pass in `the_hash_algo`. This avoids any
    -    compatibility issues and bubbles up global variable usage to upper
    -    layers which can be eventually resolved.
    +    Altough the layers above could have access to the hash_algo internally,
    +    simply pass in `the_hash_algo`. This avoids any compatibility issues and
    +    bubbles up global variable usage to upper layers which can be eventually
    +    resolved.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
2:  89255a4822 = 2:  696e991823 pack-write: pass repository to `index_pack_lockfile()`
3:  5ccf220732 ! 3:  21fefb8279 pack-write: pass hash_algo to `write_idx_file()`
    @@ Commit message
         pack-write: pass hash_algo to `write_idx_file()`
     
         The `write_idx_file()` function uses the global `the_hash_algo` variable
    -    to access the repository's hash function. To avoid global variable
    -    usage, pass the hash function from the layers above.
    +    to access the repository's hash_algo. To avoid global variable usage,
    +    pass a hash_algo from the layers above.
     
         Since `stage_tmp_packfiles()` also resides in 'pack-write.c' and calls
    -    `write_idx_file()`, update it to accept `the_hash_algo` as a parameter
    -    and pass it through to the callee.
    +    `write_idx_file()`, update it to accept a `struct git_hash_algo` as a
    +    parameter and pass it through to the callee.
     
    -    Altough the layers above could have access to the hash function
    -    internally, simply pass in `the_hash_algo`. This avoids any
    -    compatibility issues and bubbles up global variable usage to upper
    -    layers which can be eventually resolved.
    +    Altough the layers above could have access to the hash_algo internally,
    +    simply pass in `the_hash_algo`. This avoids any compatibility issues and
    +    bubbles up global variable usage to upper layers which can be eventually
    +    resolved.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
4:  95b1f784e2 ! 4:  3f730b26d1 pack-write: pass hash_algo to `write_rev_file()`
    @@ Commit message
         pack-write: pass hash_algo to `write_rev_file()`
     
         The `write_rev_file()` function uses the global `the_hash_algo` variable
    -    to access the repository's hash function. To avoid global variable
    -    usage, let's pass the hash function from the layers above. Also modify
    -    children functions `write_rev_file_order()` and `write_rev_header()` to
    -    accept 'the_hash_algo'.
    +    to access the repository's hash_algo. To avoid global variable usage,
    +    pass a hash_algo from the layers above. Also modify children functions
    +    `write_rev_file_order()` and `write_rev_header()` to accept
    +    'the_hash_algo'.
     
    -    Altough the layers above could have access to the hash function
    -    internally, simply pass in `the_hash_algo`. This avoids any
    -    compatibility issues and bubbles up global variable usage to upper
    -    layers which can be eventually resolved.
    +    Altough the layers above could have access to the hash_algo internally,
    +    simply pass in `the_hash_algo`. This avoids any compatibility issues and
    +    bubbles up global variable usage to upper layers which can be eventually
    +    resolved.
     
         However, in `midx-write.c`, since all usage of global variables is
         removed, don't reintroduce them and instead use the `repo` available in
5:  50da9bf405 ! 5:  7d1d42ca67 pack-write: pass hash_algo to internal functions
    @@ Commit message
         The internal functions `write_rev_trailer()`, `write_rev_trailer()`,
         `write_mtimes_header()` and write_mtimes_trailer()` use the global
         `the_hash_algo` variable to access the repository's hash function. Pass
    -    the hash from down as we've added made them available in the previous
    -    few commits.
    +    the hash_algo down from callers, all of which already have access to the
    +    variable.
     
         This removes all global variables from the 'pack-write.c' file, so
         remove the 'USE_THE_REPOSITORY_VARIABLE' macro.


---

base-commit: 8b2efc058aaa3d1437678616bccf7c5f7ce1f92b
change-id: 20250110-kn-the-repo-cleanup-44144fa42dc3

Thanks
- Karthik

[PATCH v3 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`

[Karthik Nayak]

The `fixup_pack_header_footer()` function uses the global
`the_hash_algo` variable to access the repository's hash function. To
avoid global variable usage, pass a hash_algo from the layers above.

Altough the layers above could have access to the hash_algo internally,
simply pass in `the_hash_algo`. This avoids any compatibility issues and
bubbles up global variable usage to upper layers which can be eventually
resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c  |  7 ++++---
 builtin/index-pack.c   |  2 +-
 builtin/pack-objects.c |  5 +++--
 bulk-checkin.c         |  2 +-
 pack-write.c           | 28 ++++++++++++++--------------
 pack.h                 |  4 +++-
 6 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761abbe6acb217fef7f4fe7c3ff5ac1fa..6baf2b1b71e2443a7987a41e0cd246076682bf58 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -878,9 +878,10 @@ static void end_packfile(void)
 
 		close_pack_windows(pack_data);
 		finalize_hashfile(pack_file, cur_pack_oid.hash, FSYNC_COMPONENT_PACK, 0);
-		fixup_pack_header_footer(pack_data->pack_fd, pack_data->hash,
-					 pack_data->pack_name, object_count,
-					 cur_pack_oid.hash, pack_size);
+		fixup_pack_header_footer(the_hash_algo, pack_data->pack_fd,
+					 pack_data->hash, pack_data->pack_name,
+					 object_count, cur_pack_oid.hash,
+					 pack_size);
 
 		if (object_count <= unpack_limit) {
 			if (!loosen_small_pack(pack_data)) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0bef61c57232e198ba539cd44ec301d26dcb0eb8..6c5e3483f4fe67fb2e26132c55b1f8395d60c11f 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1390,7 +1390,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 		strbuf_release(&msg);
 		finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
 		hashcpy(read_hash, pack_hash, the_repository->hash_algo);
-		fixup_pack_header_footer(output_fd, pack_hash,
+		fixup_pack_header_footer(the_hash_algo, output_fd, pack_hash,
 					 curr_pack, nr_objects,
 					 read_hash, consumed_bytes-the_hash_algo->rawsz);
 		if (!hasheq(read_hash, tail_hash, the_repository->hash_algo))
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index d51c021d99d9f470c04b7ec52565ab2f4c1c19ae..ffc62930b68c9b4152057572ede216381a4b0991 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1319,8 +1319,9 @@ static void write_pack_file(void)
 			 */
 
 			int fd = finalize_hashfile(f, hash, FSYNC_COMPONENT_PACK, 0);
-			fixup_pack_header_footer(fd, hash, pack_tmp_name,
-						 nr_written, hash, offset);
+			fixup_pack_header_footer(the_hash_algo, fd, hash,
+						 pack_tmp_name, nr_written,
+						 hash, offset);
 			close(fd);
 			if (write_bitmap_index) {
 				if (write_bitmap_index != WRITE_BITMAP_QUIET)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 5044cb7fa083d692a3797e2491c27b61ec44c69c..c4b085f57f74fb8b998576ac9d84fed9e01ed0ed 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -70,7 +70,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 				  CSUM_HASH_IN_STREAM | CSUM_FSYNC | CSUM_CLOSE);
 	} else {
 		int fd = finalize_hashfile(state->f, hash, FSYNC_COMPONENT_PACK, 0);
-		fixup_pack_header_footer(fd, hash, state->pack_tmp_name,
+		fixup_pack_header_footer(the_hash_algo, fd, hash, state->pack_tmp_name,
 					 state->nr_written, hash,
 					 state->offset);
 		close(fd);
diff --git a/pack-write.c b/pack-write.c
index 98a8c0e7853d7b46b5ce9a9672e0249ff051b5f9..fc887850dfb9789132b8642733c6472944dbe32d 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -380,7 +380,8 @@ off_t write_pack_header(struct hashfile *f, uint32_t nr_entries)
  * partial_pack_sha1 can refer to the same buffer if the caller is not
  * interested in the resulting SHA1 of pack data above partial_pack_offset.
  */
-void fixup_pack_header_footer(int pack_fd,
+void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
+			 int pack_fd,
 			 unsigned char *new_pack_hash,
 			 const char *pack_name,
 			 uint32_t object_count,
@@ -393,8 +394,8 @@ void fixup_pack_header_footer(int pack_fd,
 	char *buf;
 	ssize_t read_result;
 
-	the_hash_algo->init_fn(&old_hash_ctx);
-	the_hash_algo->init_fn(&new_hash_ctx);
+	hash_algo->init_fn(&old_hash_ctx);
+	hash_algo->init_fn(&new_hash_ctx);
 
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
@@ -406,9 +407,9 @@ void fixup_pack_header_footer(int pack_fd,
 			  pack_name);
 	if (lseek(pack_fd, 0, SEEK_SET) != 0)
 		die_errno("Failed seeking to start of '%s'", pack_name);
-	the_hash_algo->update_fn(&old_hash_ctx, &hdr, sizeof(hdr));
+	hash_algo->update_fn(&old_hash_ctx, &hdr, sizeof(hdr));
 	hdr.hdr_entries = htonl(object_count);
-	the_hash_algo->update_fn(&new_hash_ctx, &hdr, sizeof(hdr));
+	hash_algo->update_fn(&new_hash_ctx, &hdr, sizeof(hdr));
 	write_or_die(pack_fd, &hdr, sizeof(hdr));
 	partial_pack_offset -= sizeof(hdr);
 
@@ -423,7 +424,7 @@ void fixup_pack_header_footer(int pack_fd,
 			break;
 		if (n < 0)
 			die_errno("Failed to checksum '%s'", pack_name);
-		the_hash_algo->update_fn(&new_hash_ctx, buf, n);
+		hash_algo->update_fn(&new_hash_ctx, buf, n);
 
 		aligned_sz -= n;
 		if (!aligned_sz)
@@ -432,13 +433,12 @@ void fixup_pack_header_footer(int pack_fd,
 		if (!partial_pack_hash)
 			continue;
 
-		the_hash_algo->update_fn(&old_hash_ctx, buf, n);
+		hash_algo->update_fn(&old_hash_ctx, buf, n);
 		partial_pack_offset -= n;
 		if (partial_pack_offset == 0) {
 			unsigned char hash[GIT_MAX_RAWSZ];
-			the_hash_algo->final_fn(hash, &old_hash_ctx);
-			if (!hasheq(hash, partial_pack_hash,
-				    the_repository->hash_algo))
+			hash_algo->final_fn(hash, &old_hash_ctx);
+			if (!hasheq(hash, partial_pack_hash, hash_algo))
 				die("Unexpected checksum for %s "
 				    "(disk corruption?)", pack_name);
 			/*
@@ -446,7 +446,7 @@ void fixup_pack_header_footer(int pack_fd,
 			 * pack, which also means making partial_pack_offset
 			 * big enough not to matter anymore.
 			 */
-			the_hash_algo->init_fn(&old_hash_ctx);
+			hash_algo->init_fn(&old_hash_ctx);
 			partial_pack_offset = ~partial_pack_offset;
 			partial_pack_offset -= MSB(partial_pack_offset, 1);
 		}
@@ -454,9 +454,9 @@ void fixup_pack_header_footer(int pack_fd,
 	free(buf);
 
 	if (partial_pack_hash)
-		the_hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
-	the_hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
-	write_or_die(pack_fd, new_pack_hash, the_hash_algo->rawsz);
+		hash_algo->final_fn(partial_pack_hash, &old_hash_ctx);
+	hash_algo->final_fn(new_pack_hash, &new_hash_ctx);
+	write_or_die(pack_fd, new_pack_hash, hash_algo->rawsz);
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
diff --git a/pack.h b/pack.h
index a8da0406299bf267205256aaa8efb215f2ff73be..6d9d477adc83e83d9e9175ccf699c100b4c147c6 100644
--- a/pack.h
+++ b/pack.h
@@ -91,7 +91,9 @@ int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offs
 int verify_pack_index(struct packed_git *);
 int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);
 off_t write_pack_header(struct hashfile *f, uint32_t);
-void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);
+void fixup_pack_header_footer(const struct git_hash_algo *, int,
+			      unsigned char *, const char *, uint32_t,
+			      unsigned char *, off_t);
 char *index_pack_lockfile(int fd, int *is_well_formed);
 
 struct ref;

-- 
2.47.0

[PATCH v3 2/5] pack-write: pass repository to `index_pack_lockfile()`

[Karthik Nayak]

The `index_pack_lockfile()` function uses the global `the_repository`
variable to access the repository. To avoid global variable usage, pass
the repository from the layers above.

Altough the layers above could have access to the repository internally,
simply pass in `the_repository`. This avoids any compatibility issues
and bubbles up global variable usage to upper layers which can be
eventually resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/receive-pack.c | 2 +-
 fetch-pack.c           | 4 +++-
 pack-write.c           | 6 +++---
 pack.h                 | 2 +-
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 56347a79633505efe8dc05acf1583b4c9995eefe..b83abe5d220cefd3707b701409dc5e6b67566599 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2304,7 +2304,7 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 		if (status)
 			return "index-pack fork failed";
 
-		lockfile = index_pack_lockfile(child.out, NULL);
+		lockfile = index_pack_lockfile(the_repository, child.out, NULL);
 		if (lockfile) {
 			pack_lockfile = register_tempfile(lockfile);
 			free(lockfile);
diff --git a/fetch-pack.c b/fetch-pack.c
index 3a227721ed0935d1f9c40584c57f54043354c032..824f56ecbca11cd9e4da6a3e4c450c6b2e7078ab 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1036,7 +1036,9 @@ static int get_pack(struct fetch_pack_args *args,
 		die(_("fetch-pack: unable to fork off %s"), cmd_name);
 	if (do_keep && (pack_lockfiles || fsck_objects)) {
 		int is_well_formed;
-		char *pack_lockfile = index_pack_lockfile(cmd.out, &is_well_formed);
+		char *pack_lockfile = index_pack_lockfile(the_repository,
+							  cmd.out,
+							  &is_well_formed);
 
 		if (!is_well_formed)
 			die(_("fetch-pack: invalid index-pack output"));
diff --git a/pack-write.c b/pack-write.c
index fc887850dfb9789132b8642733c6472944dbe32d..0cd75d2e55419362a61cf981fc11117ea7a1d88a 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -460,10 +460,10 @@ void fixup_pack_header_footer(const struct git_hash_algo *hash_algo,
 	fsync_component_or_die(FSYNC_COMPONENT_PACK, pack_fd, pack_name);
 }
 
-char *index_pack_lockfile(int ip_out, int *is_well_formed)
+char *index_pack_lockfile(struct repository *r, int ip_out, int *is_well_formed)
 {
 	char packname[GIT_MAX_HEXSZ + 6];
-	const int len = the_hash_algo->hexsz + 6;
+	const int len = r->hash_algo->hexsz + 6;
 
 	/*
 	 * The first thing we expect from index-pack's output
@@ -480,7 +480,7 @@ char *index_pack_lockfile(int ip_out, int *is_well_formed)
 		packname[len-1] = 0;
 		if (skip_prefix(packname, "keep\t", &name))
 			return xstrfmt("%s/pack/pack-%s.keep",
-				       repo_get_object_directory(the_repository), name);
+				       repo_get_object_directory(r), name);
 		return NULL;
 	}
 	if (is_well_formed)
diff --git a/pack.h b/pack.h
index 6d9d477adc83e83d9e9175ccf699c100b4c147c6..46d85e5bec787c90af69700fd4b328b1ebf1d606 100644
--- a/pack.h
+++ b/pack.h
@@ -94,7 +94,7 @@ off_t write_pack_header(struct hashfile *f, uint32_t);
 void fixup_pack_header_footer(const struct git_hash_algo *, int,
 			      unsigned char *, const char *, uint32_t,
 			      unsigned char *, off_t);
-char *index_pack_lockfile(int fd, int *is_well_formed);
+char *index_pack_lockfile(struct repository *r, int fd, int *is_well_formed);
 
 struct ref;
 

-- 
2.47.0

[PATCH v3 3/5] pack-write: pass hash_algo to `write_idx_file()`

[Karthik Nayak]

The `write_idx_file()` function uses the global `the_hash_algo` variable
to access the repository's hash_algo. To avoid global variable usage,
pass a hash_algo from the layers above.

Since `stage_tmp_packfiles()` also resides in 'pack-write.c' and calls
`write_idx_file()`, update it to accept a `struct git_hash_algo` as a
parameter and pass it through to the callee.

Altough the layers above could have access to the hash_algo internally,
simply pass in `the_hash_algo`. This avoids any compatibility issues and
bubbles up global variable usage to upper layers which can be eventually
resolved.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c  |  4 ++--
 builtin/index-pack.c   |  3 ++-
 builtin/pack-objects.c |  7 ++++---
 bulk-checkin.c         |  5 +++--
 pack-write.c           | 14 ++++++++------
 pack.h                 | 10 ++++++++--
 6 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 6baf2b1b71e2443a7987a41e0cd246076682bf58..c4bc52f93c011f34bd7f98c8e9d74a33cf9783bd 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -798,8 +798,8 @@ static const char *create_index(void)
 	if (c != last)
 		die("internal consistency error creating the index");
 
-	tmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts,
-				 pack_data->hash);
+	tmpfile = write_idx_file(the_hash_algo, NULL, idx, object_count,
+				 &pack_idx_opts, pack_data->hash);
 	free(idx);
 	return tmpfile;
 }
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 6c5e3483f4fe67fb2e26132c55b1f8395d60c11f..d73699653a2227f8ecfee2c0f51cd680093ac764 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -2096,7 +2096,8 @@ int cmd_index_pack(int argc,
 	ALLOC_ARRAY(idx_objects, nr_objects);
 	for (i = 0; i < nr_objects; i++)
 		idx_objects[i] = &objects[i].idx;
-	curr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_hash);
+	curr_index = write_idx_file(the_hash_algo, index_name, idx_objects,
+				    nr_objects, &opts, pack_hash);
 	if (rev_index)
 		curr_rev_index = write_rev_file(rev_index_name, idx_objects,
 						nr_objects, pack_hash,
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ffc62930b68c9b4152057572ede216381a4b0991..7b2dacda514c29f5393b604f15d379abb81546c0 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1369,9 +1369,10 @@ static void write_pack_file(void)
 			if (cruft)
 				pack_idx_opts.flags |= WRITE_MTIMES;
 
-			stage_tmp_packfiles(&tmpname, pack_tmp_name,
-					    written_list, nr_written,
-					    &to_pack, &pack_idx_opts, hash,
+			stage_tmp_packfiles(the_hash_algo, &tmpname,
+					    pack_tmp_name, written_list,
+					    nr_written, &to_pack,
+					    &pack_idx_opts, hash,
 					    &idx_tmp_name);
 
 			if (write_bitmap_index) {
diff --git a/bulk-checkin.c b/bulk-checkin.c
index c4b085f57f74fb8b998576ac9d84fed9e01ed0ed..0d49889bfbb233d58fc094f5e2c2d2433dad9851 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -44,8 +44,9 @@ static void finish_tmp_packfile(struct strbuf *basename,
 {
 	char *idx_tmp_name = NULL;
 
-	stage_tmp_packfiles(basename, pack_tmp_name, written_list, nr_written,
-			    NULL, pack_idx_opts, hash, &idx_tmp_name);
+	stage_tmp_packfiles(the_hash_algo, basename, pack_tmp_name,
+			    written_list, nr_written, NULL, pack_idx_opts, hash,
+			    &idx_tmp_name);
 	rename_tmp_packfile_idx(basename, &idx_tmp_name);
 
 	free(idx_tmp_name);
diff --git a/pack-write.c b/pack-write.c
index 0cd75d2e55419362a61cf981fc11117ea7a1d88a..f344e78a9ec20cea9812a5eaffc72ae0b7e7424d 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -56,7 +56,8 @@ static int need_large_offset(off_t offset, const struct pack_idx_option *opts)
  * The *sha1 contains the pack content SHA1 hash.
  * The objects array passed in will be sorted by SHA1 on exit.
  */
-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects,
+const char *write_idx_file(const struct git_hash_algo *hash_algo,
+			   const char *index_name, struct pack_idx_entry **objects,
 			   int nr_objects, const struct pack_idx_option *opts,
 			   const unsigned char *sha1)
 {
@@ -130,7 +131,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		struct pack_idx_entry *obj = *list++;
 		if (index_version < 2)
 			hashwrite_be32(f, obj->offset);
-		hashwrite(f, obj->oid.hash, the_hash_algo->rawsz);
+		hashwrite(f, obj->oid.hash, hash_algo->rawsz);
 		if ((opts->flags & WRITE_IDX_STRICT) &&
 		    (i && oideq(&list[-2]->oid, &obj->oid)))
 			die("The same object %s appears twice in the pack",
@@ -172,7 +173,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec
 		}
 	}
 
-	hashwrite(f, sha1, the_hash_algo->rawsz);
+	hashwrite(f, sha1, hash_algo->rawsz);
 	finalize_hashfile(f, NULL, FSYNC_COMPONENT_PACK_METADATA,
 			  CSUM_HASH_IN_STREAM | CSUM_CLOSE |
 			  ((opts->flags & WRITE_IDX_VERIFY) ? 0 : CSUM_FSYNC));
@@ -546,7 +547,8 @@ void rename_tmp_packfile_idx(struct strbuf *name_buffer,
 	rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx");
 }
 
-void stage_tmp_packfiles(struct strbuf *name_buffer,
+void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
+			 struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,
 			 uint32_t nr_written,
@@ -561,8 +563,8 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 	if (adjust_shared_perm(pack_tmp_name))
 		die_errno("unable to make temporary pack file readable");
 
-	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
-					       pack_idx_opts, hash);
+	*idx_tmp_name = (char *)write_idx_file(hash_algo, NULL, written_list,
+					       nr_written, pack_idx_opts, hash);
 	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
diff --git a/pack.h b/pack.h
index 46d85e5bec787c90af69700fd4b328b1ebf1d606..c650fdbe2dcde8055ad0efe55646338cd0f81df5 100644
--- a/pack.h
+++ b/pack.h
@@ -86,7 +86,12 @@ struct progress;
 /* Note, the data argument could be NULL if object type is blob */
 typedef int (*verify_fn)(const struct object_id *, enum object_type, unsigned long, void*, int*);
 
-const char *write_idx_file(const char *index_name, struct pack_idx_entry **objects, int nr_objects, const struct pack_idx_option *, const unsigned char *sha1);
+const char *write_idx_file(const struct git_hash_algo *hash_algo,
+			   const char *index_name,
+			   struct pack_idx_entry **objects,
+			   int nr_objects,
+			   const struct pack_idx_option *,
+			   const unsigned char *sha1);
 int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);
 int verify_pack_index(struct packed_git *);
 int verify_pack(struct repository *, struct packed_git *, verify_fn fn, struct progress *, uint32_t);
@@ -119,7 +124,8 @@ int read_pack_header(int fd, struct pack_header *);
 struct packing_data;
 
 struct hashfile *create_tmp_packfile(char **pack_tmp_name);
-void stage_tmp_packfiles(struct strbuf *name_buffer,
+void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
+			 struct strbuf *name_buffer,
 			 const char *pack_tmp_name,
 			 struct pack_idx_entry **written_list,
 			 uint32_t nr_written,

-- 
2.47.0

[PATCH v3 4/5] pack-write: pass hash_algo to `write_rev_file()`

[Karthik Nayak]

The `write_rev_file()` function uses the global `the_hash_algo` variable
to access the repository's hash_algo. To avoid global variable usage,
pass a hash_algo from the layers above. Also modify children functions
`write_rev_file_order()` and `write_rev_header()` to accept
'the_hash_algo'.

Altough the layers above could have access to the hash_algo internally,
simply pass in `the_hash_algo`. This avoids any compatibility issues and
bubbles up global variable usage to upper layers which can be eventually
resolved.

However, in `midx-write.c`, since all usage of global variables is
removed, don't reintroduce them and instead use the `repo` available in
the context.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/index-pack.c |  6 +++---
 midx-write.c         |  4 ++--
 pack-write.c         | 21 ++++++++++++---------
 pack.h               | 14 ++++++++++++--
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d73699653a2227f8ecfee2c0f51cd680093ac764..e803cb2444633f937fafc841848dae85341475f1 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -2099,9 +2099,9 @@ int cmd_index_pack(int argc,
 	curr_index = write_idx_file(the_hash_algo, index_name, idx_objects,
 				    nr_objects, &opts, pack_hash);
 	if (rev_index)
-		curr_rev_index = write_rev_file(rev_index_name, idx_objects,
-						nr_objects, pack_hash,
-						opts.flags);
+		curr_rev_index = write_rev_file(the_hash_algo, rev_index_name,
+						idx_objects, nr_objects,
+						pack_hash, opts.flags);
 	free(idx_objects);
 
 	if (!verify)
diff --git a/midx-write.c b/midx-write.c
index b3827b936bdb1df12c73fb7d9b98ff65fc875cc3..61b59d557d3ba46a39db74c62393545cabf50f2c 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -658,8 +658,8 @@ static void write_midx_reverse_index(char *midx_name, unsigned char *midx_hash,
 	strbuf_addf(&buf, "%s-%s.rev", midx_name, hash_to_hex_algop(midx_hash,
 								    ctx->repo->hash_algo));
 
-	tmp_file = write_rev_file_order(NULL, ctx->pack_order, ctx->entries_nr,
-					midx_hash, WRITE_REV);
+	tmp_file = write_rev_file_order(ctx->repo->hash_algo, NULL, ctx->pack_order,
+					ctx->entries_nr, midx_hash, WRITE_REV);
 
 	if (finalize_object_file(tmp_file, buf.buf))
 		die(_("cannot store reverse index file"));
diff --git a/pack-write.c b/pack-write.c
index f344e78a9ec20cea9812a5eaffc72ae0b7e7424d..09ecbcdb069cc9b0383295798ceb49cbdc632b64 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -194,11 +194,12 @@ static int pack_order_cmp(const void *va, const void *vb, void *ctx)
 	return 0;
 }
 
-static void write_rev_header(struct hashfile *f)
+static void write_rev_header(const struct git_hash_algo *hash_algo,
+			     struct hashfile *f)
 {
 	hashwrite_be32(f, RIDX_SIGNATURE);
 	hashwrite_be32(f, RIDX_VERSION);
-	hashwrite_be32(f, oid_version(the_hash_algo));
+	hashwrite_be32(f, oid_version(hash_algo));
 }
 
 static void write_rev_index_positions(struct hashfile *f,
@@ -215,7 +216,8 @@ static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
 	hashwrite(f, hash, the_hash_algo->rawsz);
 }
 
-char *write_rev_file(const char *rev_name,
+char *write_rev_file(const struct git_hash_algo *hash_algo,
+		     const char *rev_name,
 		     struct pack_idx_entry **objects,
 		     uint32_t nr_objects,
 		     const unsigned char *hash,
@@ -233,15 +235,16 @@ char *write_rev_file(const char *rev_name,
 		pack_order[i] = i;
 	QSORT_S(pack_order, nr_objects, pack_order_cmp, objects);
 
-	ret = write_rev_file_order(rev_name, pack_order, nr_objects, hash,
-				   flags);
+	ret = write_rev_file_order(hash_algo, rev_name, pack_order, nr_objects,
+				   hash, flags);
 
 	free(pack_order);
 
 	return ret;
 }
 
-char *write_rev_file_order(const char *rev_name,
+char *write_rev_file_order(const struct git_hash_algo *hash_algo,
+			   const char *rev_name,
 			   uint32_t *pack_order,
 			   uint32_t nr_objects,
 			   const unsigned char *hash,
@@ -280,7 +283,7 @@ char *write_rev_file_order(const char *rev_name,
 		return NULL;
 	}
 
-	write_rev_header(f);
+	write_rev_header(hash_algo, f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
 	write_rev_trailer(f, hash);
@@ -568,8 +571,8 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
 	if (adjust_shared_perm(*idx_tmp_name))
 		die_errno("unable to make temporary index file readable");
 
-	rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,
-				      pack_idx_opts->flags);
+	rev_tmp_name = write_rev_file(hash_algo, NULL, written_list, nr_written,
+				      hash, pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
 		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
diff --git a/pack.h b/pack.h
index c650fdbe2dcde8055ad0efe55646338cd0f81df5..8a84ea475cda902936ee0b6091c5b4d462606be8 100644
--- a/pack.h
+++ b/pack.h
@@ -105,8 +105,18 @@ struct ref;
 
 void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought);
 
-char *write_rev_file(const char *rev_name, struct pack_idx_entry **objects, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
-char *write_rev_file_order(const char *rev_name, uint32_t *pack_order, uint32_t nr_objects, const unsigned char *hash, unsigned flags);
+char *write_rev_file(const struct git_hash_algo *hash_algo,
+		     const char *rev_name,
+		     struct pack_idx_entry **objects,
+		     uint32_t nr_objects,
+		     const unsigned char *hash,
+		     unsigned flags);
+char *write_rev_file_order(const struct git_hash_algo *hash_algo,
+			   const char *rev_name,
+			   uint32_t *pack_order,
+			   uint32_t nr_objects,
+			   const unsigned char *hash,
+			   unsigned flags);
 
 /*
  * The "hdr" output buffer should be at least this big, which will handle sizes

-- 
2.47.0

[PATCH v3 5/5] pack-write: pass hash_algo to internal functions

[Karthik Nayak]

The internal functions `write_rev_trailer()`, `write_rev_trailer()`,
`write_mtimes_header()` and write_mtimes_trailer()` use the global
`the_hash_algo` variable to access the repository's hash function. Pass
the hash_algo down from callers, all of which already have access to the
variable.

This removes all global variables from the 'pack-write.c' file, so
remove the 'USE_THE_REPOSITORY_VARIABLE' macro.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 pack-write.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/pack-write.c b/pack-write.c
index 09ecbcdb069cc9b0383295798ceb49cbdc632b64..a2faeb1895e41f4c17281380478f1f2cabcc6f24 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -1,5 +1,3 @@
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "environment.h"
 #include "gettext.h"
@@ -211,9 +209,10 @@ static void write_rev_index_positions(struct hashfile *f,
 		hashwrite_be32(f, pack_order[i]);
 }
 
-static void write_rev_trailer(struct hashfile *f, const unsigned char *hash)
+static void write_rev_trailer(const struct git_hash_algo *hash_algo,
+			      struct hashfile *f, const unsigned char *hash)
 {
-	hashwrite(f, hash, the_hash_algo->rawsz);
+	hashwrite(f, hash, hash_algo->rawsz);
 }
 
 char *write_rev_file(const struct git_hash_algo *hash_algo,
@@ -286,7 +285,7 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,
 	write_rev_header(hash_algo, f);
 
 	write_rev_index_positions(f, pack_order, nr_objects);
-	write_rev_trailer(f, hash);
+	write_rev_trailer(hash_algo, f, hash);
 
 	if (adjust_shared_perm(path) < 0)
 		die(_("failed to make %s readable"), path);
@@ -298,11 +297,12 @@ char *write_rev_file_order(const struct git_hash_algo *hash_algo,
 	return path;
 }
 
-static void write_mtimes_header(struct hashfile *f)
+static void write_mtimes_header(const struct git_hash_algo *hash_algo,
+				struct hashfile *f)
 {
 	hashwrite_be32(f, MTIMES_SIGNATURE);
 	hashwrite_be32(f, MTIMES_VERSION);
-	hashwrite_be32(f, oid_version(the_hash_algo));
+	hashwrite_be32(f, oid_version(hash_algo));
 }
 
 /*
@@ -322,12 +322,14 @@ static void write_mtimes_objects(struct hashfile *f,
 	}
 }
 
-static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)
+static void write_mtimes_trailer(const struct git_hash_algo *hash_algo,
+				 struct hashfile *f, const unsigned char *hash)
 {
-	hashwrite(f, hash, the_hash_algo->rawsz);
+	hashwrite(f, hash, hash_algo->rawsz);
 }
 
-static char *write_mtimes_file(struct packing_data *to_pack,
+static char *write_mtimes_file(const struct git_hash_algo *hash_algo,
+			       struct packing_data *to_pack,
 			       struct pack_idx_entry **objects,
 			       uint32_t nr_objects,
 			       const unsigned char *hash)
@@ -344,9 +346,9 @@ static char *write_mtimes_file(struct packing_data *to_pack,
 	mtimes_name = strbuf_detach(&tmp_file, NULL);
 	f = hashfd(fd, mtimes_name);
 
-	write_mtimes_header(f);
+	write_mtimes_header(hash_algo, f);
 	write_mtimes_objects(f, to_pack, objects, nr_objects);
-	write_mtimes_trailer(f, hash);
+	write_mtimes_trailer(hash_algo, f, hash);
 
 	if (adjust_shared_perm(mtimes_name) < 0)
 		die(_("failed to make %s readable"), mtimes_name);
@@ -575,8 +577,8 @@ void stage_tmp_packfiles(const struct git_hash_algo *hash_algo,
 				      hash, pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
-		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
-						    nr_written,
+		mtimes_tmp_name = write_mtimes_file(hash_algo, to_pack,
+						    written_list, nr_written,
 						    hash);
 	}
 

-- 
2.47.0

Re: [PATCH v3 0/5] pack-write: cleanup usage of global variables

[Patrick Steinhardt]

On Sun, Jan 19, 2025 at 12:19:25PM +0100, Karthik Nayak wrote:
> This is a small series to remove global variable usage from
> `pack-write.c`. Mostly it bubble's up the usage of global variables to
> upper layers. The only exception is in `write-midx.c`, which was cleaned
> of global variable usage, so there, we use the repo that is in available
> in the context.
> 
> This series is based on fbe8d3079d (Git 2.48, 2025-01-10) with
> 'ps/more-sign-compare' and 'ps/the-repository' merged in.
> 
> There are no conflicts with topics in 'next', however there is a
> conflict with 'tb/incremental-midx-part-2' in 'seen', the fix is simple
> but happy to merge that in too if necessary.

Thanks, this version looks good to me.

Patrick

Re: [PATCH v3 0/5] pack-write: cleanup usage of global variables

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Sun, Jan 19, 2025 at 12:19:25PM +0100, Karthik Nayak wrote:
>> This is a small series to remove global variable usage from
>> `pack-write.c`. Mostly it bubble's up the usage of global variables to
>> upper layers. The only exception is in `write-midx.c`, which was cleaned
>> of global variable usage, so there, we use the repo that is in available
>> in the context.
>> 
>> This series is based on fbe8d3079d (Git 2.48, 2025-01-10) with
>> 'ps/more-sign-compare' and 'ps/the-repository' merged in.
>> 
>> There are no conflicts with topics in 'next', however there is a
>> conflict with 'tb/incremental-midx-part-2' in 'seen', the fix is simple
>> but happy to merge that in too if necessary.
>
> Thanks, this version looks good to me.

Thanks, both of you.  Let me mark it for 'next' then.