diff mbox series

[v2,08/10] midx: pass down `hash_algo` to `get_midx_filename[_ext]`

Message ID 20241119-374-refactor-midx-c-and-midx-write-c-to-not-depend-on-global-state-v2-8-e2f607174efc@gmail.com (mailing list archive)
State New
Headers show
Series Change midx.c and midx-write.c to not use global variables | expand

Commit Message

karthik nayak Nov. 19, 2024, 3:36 p.m. UTC 
The function `get_midx_filename_ext` uses `hash_to_hex` which internally
uses the global variable `the_repository`. To remove this dependency,
pass down the `hash_algo` to both `get_midx_filename` and
`get_midx_filename_ext`. This adds `the_repository` variable usage to
`midx-write.c`, which will be resolved in a future commit.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 midx-write.c    | 15 +++++++--------
 midx.c          | 16 +++++++++-------
 midx.h          |  7 +++++--
 pack-bitmap.c   |  6 +++---
 pack-revindex.c |  2 +-
 5 files changed, 25 insertions(+), 21 deletions(-)

Comments

Christian Couder Nov. 20, 2024, 6:15 p.m. UTC | #1 
On Tue, Nov 19, 2024 at 4:39 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> The function `get_midx_filename_ext` uses `hash_to_hex` which internally
> uses the global variable `the_repository`. To remove this dependency,
> pass down the `hash_algo` to both `get_midx_filename` and
> `get_midx_filename_ext`.

Technically passing down the `hash_algo` to both functions is not
enough to remove the dependency, as replacing `hash_to_hex()` with
`hash_to_hex_algop*()` is also needed. It's a bit unclear if
`hash_to_hex()` calls are replaced with other calls in this patch or
not though.

So you might want to add something like: "Replacing `hash_to_hex` with
functions that take an `hash_algo` as argument will be done in a
followup commit."


> This adds `the_repository` variable usage to
> `midx-write.c`, which will be resolved in a future commit.
karthik nayak Nov. 21, 2024, 3:35 p.m. UTC | #2 
Christian Couder <christian.couder@gmail.com> writes:

> On Tue, Nov 19, 2024 at 4:39 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> The function `get_midx_filename_ext` uses `hash_to_hex` which internally
>> uses the global variable `the_repository`. To remove this dependency,
>> pass down the `hash_algo` to both `get_midx_filename` and
>> `get_midx_filename_ext`.
>
> Technically passing down the `hash_algo` to both functions is not
> enough to remove the dependency, as replacing `hash_to_hex()` with
> `hash_to_hex_algop*()` is also needed. It's a bit unclear if
> `hash_to_hex()` calls are replaced with other calls in this patch or
> not though.
>

I see what you mean, this patch itself swaps `hash_to_hex()` with
`hash_to_hex_algop()`, but only in `get_midx_filename[_ext]`.

> So you might want to add something like: "Replacing `hash_to_hex` with
> functions that take an `hash_algo` as argument will be done in a
> followup commit."

While we also do something similar in the next patch, I'll amend this
commit message to be less vague and clarify the intention. Thanks.

>
>> This adds `the_repository` variable usage to
>> `midx-write.c`, which will be resolved in a future commit.

I think this is stale too and will remove it ;)
diff mbox series

Patch

diff --git a/midx-write.c b/midx-write.c
index 1bc2f5256916e69924245951f654c1047ffeab84..2f0c09211282fa651af8f9a342f0564729468306 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -991,8 +991,8 @@  static int link_midx_to_chain(struct multi_pack_index *m)
 	for (i = 0; i < ARRAY_SIZE(midx_exts); i++) {
 		const unsigned char *hash = get_midx_checksum(m);
 
-		get_midx_filename_ext(&from, m->object_dir, hash,
-				      midx_exts[i].non_split);
+		get_midx_filename_ext(m->repo->hash_algo, &from, m->object_dir,
+				      hash, midx_exts[i].non_split);
 		get_split_midx_filename_ext(&to, m->object_dir, hash,
 					    midx_exts[i].split);
 
@@ -1012,9 +1012,8 @@  static int link_midx_to_chain(struct multi_pack_index *m)
 	return ret;
 }
 
-static void clear_midx_files(const char *object_dir,
-			     const char **hashes,
-			     uint32_t hashes_nr,
+static void clear_midx_files(struct repository *r, const char *object_dir,
+			     const char **hashes, uint32_t hashes_nr,
 			     unsigned incremental)
 {
 	/*
@@ -1039,7 +1038,7 @@  static void clear_midx_files(const char *object_dir,
 	}
 
 	if (incremental)
-		get_midx_filename(&buf, object_dir);
+		get_midx_filename(r->hash_algo, &buf, object_dir);
 	else
 		get_midx_chain_filename(&buf, object_dir);
 
@@ -1083,7 +1082,7 @@  static int write_midx_internal(struct repository *r, const char *object_dir,
 			    "%s/pack/multi-pack-index.d/tmp_midx_XXXXXX",
 			    object_dir);
 	else
-		get_midx_filename(&midx_name, object_dir);
+		get_midx_filename(r->hash_algo, &midx_name, object_dir);
 	if (safe_create_leading_directories(midx_name.buf))
 		die_errno(_("unable to create leading directories of %s"),
 			  midx_name.buf);
@@ -1474,7 +1473,7 @@  static int write_midx_internal(struct repository *r, const char *object_dir,
 	if (commit_lock_file(&lk) < 0)
 		die_errno(_("could not write multi-pack-index"));
 
-	clear_midx_files(object_dir, keep_hashes,
+	clear_midx_files(r, object_dir, keep_hashes,
 			 ctx.num_multi_pack_indexes_before + 1,
 			 ctx.incremental);
 
diff --git a/midx.c b/midx.c
index 98ee84d4a8bf388906634ad695ff39acdaa2c6d5..9bed4185ff4d44602aedfe0329dd840ff9e85435 100644
--- a/midx.c
+++ b/midx.c
@@ -28,17 +28,19 @@  const unsigned char *get_midx_checksum(struct multi_pack_index *m)
 	return m->data + m->data_len - m->repo->hash_algo->rawsz;
 }
 
-void get_midx_filename(struct strbuf *out, const char *object_dir)
+void get_midx_filename(const struct git_hash_algo *hash_algo,
+		       struct strbuf *out, const char *object_dir)
 {
-	get_midx_filename_ext(out, object_dir, NULL, NULL);
+	get_midx_filename_ext(hash_algo, out, object_dir, NULL, NULL);
 }
 
-void get_midx_filename_ext(struct strbuf *out, const char *object_dir,
+void get_midx_filename_ext(const struct git_hash_algo *hash_algo,
+			   struct strbuf *out, const char *object_dir,
 			   const unsigned char *hash, const char *ext)
 {
 	strbuf_addf(out, "%s/pack/multi-pack-index", object_dir);
 	if (ext)
-		strbuf_addf(out, "-%s.%s", hash_to_hex(hash), ext);
+		strbuf_addf(out, "-%s.%s", hash_to_hex_algop(hash, hash_algo), ext);
 }
 
 static int midx_read_oid_fanout(const unsigned char *chunk_start,
@@ -379,7 +381,7 @@  struct multi_pack_index *load_multi_pack_index(struct repository *r,
 	struct strbuf midx_name = STRBUF_INIT;
 	struct multi_pack_index *m;
 
-	get_midx_filename(&midx_name, object_dir);
+	get_midx_filename(r->hash_algo, &midx_name, object_dir);
 
 	m = load_multi_pack_index_one(r, object_dir,
 				      midx_name.buf, local);
@@ -822,7 +824,7 @@  void clear_midx_file(struct repository *r)
 {
 	struct strbuf midx = STRBUF_INIT;
 
-	get_midx_filename(&midx, r->objects->odb->path);
+	get_midx_filename(r->hash_algo, &midx, r->objects->odb->path);
 
 	if (r->objects && r->objects->multi_pack_index) {
 		close_midx(r->objects->multi_pack_index);
@@ -891,7 +893,7 @@  int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
 		struct stat sb;
 		struct strbuf filename = STRBUF_INIT;
 
-		get_midx_filename(&filename, object_dir);
+		get_midx_filename(r->hash_algo, &filename, object_dir);
 
 		if (!stat(filename.buf, &sb)) {
 			error(_("multi-pack-index file exists, but failed to parse"));
diff --git a/midx.h b/midx.h
index 78efa28d35371795fa33c68660278182debb60ab..7620820d4d0272926af9e4eeb68bfb73404c7ec2 100644
--- a/midx.h
+++ b/midx.h
@@ -7,6 +7,7 @@  struct object_id;
 struct pack_entry;
 struct repository;
 struct bitmapped_pack;
+struct git_hash_algo;
 
 #define MIDX_SIGNATURE 0x4d494458 /* "MIDX" */
 #define MIDX_VERSION 1
@@ -89,8 +90,10 @@  struct multi_pack_index {
 #define MIDX_EXT_MIDX "midx"
 
 const unsigned char *get_midx_checksum(struct multi_pack_index *m);
-void get_midx_filename(struct strbuf *out, const char *object_dir);
-void get_midx_filename_ext(struct strbuf *out, const char *object_dir,
+void get_midx_filename(const struct git_hash_algo *hash_algo,
+		       struct strbuf *out, const char *object_dir);
+void get_midx_filename_ext(const struct git_hash_algo *hash_algo,
+			   struct strbuf *out, const char *object_dir,
 			   const unsigned char *hash, const char *ext);
 void get_midx_chain_dirname(struct strbuf *buf, const char *object_dir);
 void get_midx_chain_filename(struct strbuf *buf, const char *object_dir);
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 0cb1b56c9d5a55936ba53e2ff904ffe46cdcbafc..7b62d099cab5729a60a36b3ad15276fdc351aa97 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -375,8 +375,8 @@  static int load_bitmap_entries_v1(struct bitmap_index *index)
 char *midx_bitmap_filename(struct multi_pack_index *midx)
 {
 	struct strbuf buf = STRBUF_INIT;
-	get_midx_filename_ext(&buf, midx->object_dir, get_midx_checksum(midx),
-			      MIDX_EXT_BITMAP);
+	get_midx_filename_ext(midx->repo->hash_algo, &buf, midx->object_dir,
+			      get_midx_checksum(midx), MIDX_EXT_BITMAP);
 
 	return strbuf_detach(&buf, NULL);
 }
@@ -415,7 +415,7 @@  static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,
 
 	if (bitmap_git->pack || bitmap_git->midx) {
 		struct strbuf buf = STRBUF_INIT;
-		get_midx_filename(&buf, midx->object_dir);
+		get_midx_filename(midx->repo->hash_algo, &buf, midx->object_dir);
 		trace2_data_string("bitmap", bitmap_repo(bitmap_git),
 				   "ignoring extra midx bitmap file", buf.buf);
 		close(fd);
diff --git a/pack-revindex.c b/pack-revindex.c
index 22d3c2346488de6279b6f26a69fe611106c1365a..d3832478d99edffae17db0bbe85aa981c1a3ad30 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -383,7 +383,7 @@  int load_midx_revindex(struct multi_pack_index *m)
 	trace2_data_string("load_midx_revindex", the_repository,
 			   "source", "rev");
 
-	get_midx_filename_ext(&revindex_name, m->object_dir,
+	get_midx_filename_ext(m->repo->hash_algo, &revindex_name, m->object_dir,
 			      get_midx_checksum(m), MIDX_EXT_REV);
 
 	ret = load_revindex_from_disk(revindex_name.buf,