diff mbox series

[v2,02/10] midx-write: pass down repository to static functions

Message ID 20241119-374-refactor-midx-c-and-midx-write-c-to-not-depend-on-global-state-v2-2-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 
In 'midx-write.c' there are a lot of static functions which use global
variables `the_repository` or `the_hash_algo`. In a follow up commit,
the repository variable will be added to `write_midx_context`, which
some of the functions can use. But for functions which do not have
access to this struct, pass down the required information from
non-static functions `write_midx_file` and `write_midx_file_only`.

This ensures that the usage of global variables is limited to these
non-static functions, which will be cleaned up in a follow up commits.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 midx-write.c | 57 +++++++++++++++++++++++++++++++--------------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

Comments

Christian Couder Nov. 20, 2024, 6:15 p.m. UTC | #1 
On Tue, Nov 19, 2024 at 5:36 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>
> In 'midx-write.c' there are a lot of static functions which use global
> variables `the_repository` or `the_hash_algo`. In a follow up commit,
> the repository variable will be added to `write_midx_context`, which
> some of the functions can use. But for functions which do not have
> access to this struct, pass down the required information from
> non-static functions `write_midx_file` and `write_midx_file_only`.
>
> This ensures that the usage of global variables is limited to these
> non-static functions, which will be cleaned up in a follow up commits.

s/commits/commit/

There are a few places in the patch where hash_to_hex() is replaced
with hash_to_hex_algop(). However hash_to_hex() is not quite a static
function and is not defined in 'midx-write.c'. So you might want to
mention this additional change in the commit message.

Thanks.
Taylor Blau Nov. 20, 2024, 7:41 p.m. UTC | #2 
On Wed, Nov 20, 2024 at 07:15:04PM +0100, Christian Couder wrote:
> On Tue, Nov 19, 2024 at 5:36 PM Karthik Nayak <karthik.188@gmail.com> wrote:
> >
> > In 'midx-write.c' there are a lot of static functions which use global
> > variables `the_repository` or `the_hash_algo`. In a follow up commit,
> > the repository variable will be added to `write_midx_context`, which
> > some of the functions can use. But for functions which do not have
> > access to this struct, pass down the required information from
> > non-static functions `write_midx_file` and `write_midx_file_only`.
> >
> > This ensures that the usage of global variables is limited to these
> > non-static functions, which will be cleaned up in a follow up commits.
>
> s/commits/commit/

Good eyes.

> There are a few places in the patch where hash_to_hex() is replaced
> with hash_to_hex_algop(). However hash_to_hex() is not quite a static
> function and is not defined in 'midx-write.c'. So you might want to
> mention this additional change in the commit message.

I think this is fine to leave out personally. Since hash_to_hex()
relies on the_hash_algo only, it's natural to expect that when a caller
has access to a new variable pointing at a 'struct git_hash_algo' that
they would convert that function to hash_to_hex_algop().

Thanks,
Taylor
Taylor Blau Nov. 20, 2024, 7:43 p.m. UTC | #3 
On Tue, Nov 19, 2024 at 04:36:41PM +0100, Karthik Nayak wrote:
> ---
>  midx-write.c | 57 +++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 31 insertions(+), 26 deletions(-)

This patch all seems reasonable to me. When reviewing it in this way, it
is a little unclear to see which functions need just a repository, just
a hash_algo, or the whole write_midx_context structure.

I think I might have erred on the side of just passing the
write_midx_context structure throughout the midx-write.c internals. That
exposes more of the implementation details than is strictly necessary,
but it does so to the benefit of making the code (and this patch) easier
to follow.

I don't feel strongly about it, and I trust that you modified the right
functions to pass the right data. I just wanted to mention it in case
you hadn't considered an alternative way to structure this patch.

Thanks,
Taylor
karthik nayak Nov. 21, 2024, 2:57 p.m. UTC | #4 
Christian Couder <christian.couder@gmail.com> writes:

> On Tue, Nov 19, 2024 at 5:36 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> In 'midx-write.c' there are a lot of static functions which use global
>> variables `the_repository` or `the_hash_algo`. In a follow up commit,
>> the repository variable will be added to `write_midx_context`, which
>> some of the functions can use. But for functions which do not have
>> access to this struct, pass down the required information from
>> non-static functions `write_midx_file` and `write_midx_file_only`.
>>
>> This ensures that the usage of global variables is limited to these
>> non-static functions, which will be cleaned up in a follow up commits.
>
> s/commits/commit/
>

Will fix.

> There are a few places in the patch where hash_to_hex() is replaced
> with hash_to_hex_algop(). However hash_to_hex() is not quite a static
> function and is not defined in 'midx-write.c'. So you might want to
> mention this additional change in the commit message.
>

While I think it is obvious, that might not be the case, so I'll add in
a line to explain why we swap those functions.

> Thanks.
karthik nayak Nov. 21, 2024, 3:06 p.m. UTC | #5 
Taylor Blau <me@ttaylorr.com> writes:

> On Tue, Nov 19, 2024 at 04:36:41PM +0100, Karthik Nayak wrote:
>> ---
>>  midx-write.c | 57 +++++++++++++++++++++++++++++++--------------------------
>>  1 file changed, 31 insertions(+), 26 deletions(-)
>
> This patch all seems reasonable to me. When reviewing it in this way, it
> is a little unclear to see which functions need just a repository, just
> a hash_algo, or the whole write_midx_context structure.
>
> I think I might have erred on the side of just passing the
> write_midx_context structure throughout the midx-write.c internals. That
> exposes more of the implementation details than is strictly necessary,
> but it does so to the benefit of making the code (and this patch) easier
> to follow.
>
> I don't feel strongly about it, and I trust that you modified the right
> functions to pass the right data. I just wanted to mention it in case
> you hadn't considered an alternative way to structure this patch.
>

I remember doing something like that and it seemed a lot more noisy than
the current iteration.

Regarding `write_midx_context`, I wonder if all these functions need to
receive it. Passing only what’s necessary might make the functions'
requirements clearer, instead of bundling everything into a large
struct.

But then again, if you feel otherwise, I'm happy to change it, I don't
feel so strongly about it :)

> Thanks,
> Taylor
diff mbox series

Patch

diff --git a/midx-write.c b/midx-write.c
index c57726ef9475df693890d61627ce91409c1def7c..22b5233f51ec6c6d99b8f9613818f1581dca5982 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -35,13 +35,13 @@  extern void clear_incremental_midx_files_ext(const char *object_dir,
 extern int cmp_idx_or_pack_name(const char *idx_or_pack_name,
 				const char *idx_name);
 
-static size_t write_midx_header(struct hashfile *f,
-				unsigned char num_chunks,
+static size_t write_midx_header(const struct git_hash_algo *hash_algo,
+				struct hashfile *f, unsigned char num_chunks,
 				uint32_t num_packs)
 {
 	hashwrite_be32(f, MIDX_SIGNATURE);
 	hashwrite_u8(f, MIDX_VERSION);
-	hashwrite_u8(f, oid_version(the_hash_algo));
+	hashwrite_u8(f, oid_version(hash_algo));
 	hashwrite_u8(f, num_chunks);
 	hashwrite_u8(f, 0); /* unused */
 	hashwrite_be32(f, num_packs);
@@ -702,7 +702,7 @@  static int add_ref_to_pending(const char *refname, const char *referent UNUSED,
 		return 0;
 	}
 
-	if (!peel_iterated_oid(the_repository, oid, &peeled))
+	if (!peel_iterated_oid(revs->repo, oid, &peeled))
 		oid = &peeled;
 
 	object = parse_object_or_die(oid, refname);
@@ -827,7 +827,7 @@  static struct commit **find_commits_for_midx_bitmap(uint32_t *indexed_commits_nr
 	return cb.commits;
 }
 
-static int write_midx_bitmap(const char *midx_name,
+static int write_midx_bitmap(struct repository *r, const char *midx_name,
 			     const unsigned char *midx_hash,
 			     struct packing_data *pdata,
 			     struct commit **commits,
@@ -840,9 +840,9 @@  static int write_midx_bitmap(const char *midx_name,
 	struct bitmap_writer writer;
 	struct pack_idx_entry **index;
 	char *bitmap_name = xstrfmt("%s-%s.bitmap", midx_name,
-					hash_to_hex(midx_hash));
+				    hash_to_hex_algop(midx_hash, r->hash_algo));
 
-	trace2_region_enter("midx", "write_midx_bitmap", the_repository);
+	trace2_region_enter("midx", "write_midx_bitmap", r);
 
 	if (flags & MIDX_WRITE_BITMAP_HASH_CACHE)
 		options |= BITMAP_OPT_HASH_CACHE;
@@ -859,7 +859,7 @@  static int write_midx_bitmap(const char *midx_name,
 	for (i = 0; i < pdata->nr_objects; i++)
 		index[i] = &pdata->objects[i].idx;
 
-	bitmap_writer_init(&writer, the_repository, pdata);
+	bitmap_writer_init(&writer, r, pdata);
 	bitmap_writer_show_progress(&writer, flags & MIDX_PROGRESS);
 	bitmap_writer_build_type_index(&writer, index);
 
@@ -892,7 +892,7 @@  static int write_midx_bitmap(const char *midx_name,
 	free(bitmap_name);
 	bitmap_writer_free(&writer);
 
-	trace2_region_leave("midx", "write_midx_bitmap", the_repository);
+	trace2_region_leave("midx", "write_midx_bitmap", r);
 
 	return ret;
 }
@@ -1049,7 +1049,7 @@  static void clear_midx_files(const char *object_dir,
 	strbuf_release(&buf);
 }
 
-static int write_midx_internal(const char *object_dir,
+static int write_midx_internal(struct repository *r, const char *object_dir,
 			       struct string_list *packs_to_include,
 			       struct string_list *packs_to_drop,
 			       const char *preferred_pack_name,
@@ -1070,7 +1070,8 @@  static int write_midx_internal(const char *object_dir,
 	const char **keep_hashes = NULL;
 	struct chunkfile *cf;
 
-	trace2_region_enter("midx", "write_midx_internal", the_repository);
+	trace2_region_enter("midx", "write_midx_internal", r);
+
 
 	ctx.incremental = !!(flags & MIDX_WRITE_INCREMENTAL);
 	if (ctx.incremental && (flags & MIDX_WRITE_BITMAP))
@@ -1087,8 +1088,7 @@  static int write_midx_internal(const char *object_dir,
 			  midx_name.buf);
 
 	if (!packs_to_include || ctx.incremental) {
-		struct multi_pack_index *m = lookup_multi_pack_index(the_repository,
-								     object_dir);
+		struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);
 		if (m && !midx_checksum_valid(m)) {
 			warning(_("ignoring existing multi-pack-index; checksum mismatch"));
 			m = NULL;
@@ -1351,7 +1351,7 @@  static int write_midx_internal(const char *object_dir,
 	add_chunk(cf, MIDX_CHUNKID_OIDFANOUT, MIDX_CHUNK_FANOUT_SIZE,
 		  write_midx_oid_fanout);
 	add_chunk(cf, MIDX_CHUNKID_OIDLOOKUP,
-		  st_mult(ctx.entries_nr, the_hash_algo->rawsz),
+		  st_mult(ctx.entries_nr, r->hash_algo->rawsz),
 		  write_midx_oid_lookup);
 	add_chunk(cf, MIDX_CHUNKID_OBJECTOFFSETS,
 		  st_mult(ctx.entries_nr, MIDX_CHUNK_OFFSET_WIDTH),
@@ -1373,7 +1373,8 @@  static int write_midx_internal(const char *object_dir,
 			  write_midx_bitmapped_packs);
 	}
 
-	write_midx_header(f, get_num_chunks(cf), ctx.nr - dropped_packs);
+	write_midx_header(r->hash_algo, f, get_num_chunks(cf),
+			  ctx.nr - dropped_packs);
 	write_chunkfile(cf, &ctx);
 
 	finalize_hashfile(f, midx_hash, FSYNC_COMPONENT_PACK_METADATA,
@@ -1405,7 +1406,7 @@  static int write_midx_internal(const char *object_dir,
 		FREE_AND_NULL(ctx.entries);
 		ctx.entries_nr = 0;
 
-		if (write_midx_bitmap(midx_name.buf, midx_hash, &pdata,
+		if (write_midx_bitmap(r, midx_name.buf, midx_hash, &pdata,
 				      commits, commits_nr, ctx.pack_order,
 				      flags) < 0) {
 			error(_("could not write multi-pack bitmap"));
@@ -1449,12 +1450,13 @@  static int write_midx_internal(const char *object_dir,
 		strbuf_release(&final_midx_name);
 
 		keep_hashes[ctx.num_multi_pack_indexes_before] =
-			xstrdup(hash_to_hex(midx_hash));
+			xstrdup(hash_to_hex_algop(midx_hash, r->hash_algo));
 
 		for (i = 0; i < ctx.num_multi_pack_indexes_before; i++) {
 			uint32_t j = ctx.num_multi_pack_indexes_before - i - 1;
 
-			keep_hashes[j] = xstrdup(hash_to_hex(get_midx_checksum(m)));
+			keep_hashes[j] = xstrdup(hash_to_hex_algop(get_midx_checksum(m),
+								   r->hash_algo));
 			m = m->base_midx;
 		}
 
@@ -1462,7 +1464,7 @@  static int write_midx_internal(const char *object_dir,
 			fprintf(get_lock_file_fp(&lk), "%s\n", keep_hashes[i]);
 	} else {
 		keep_hashes[ctx.num_multi_pack_indexes_before] =
-			xstrdup(hash_to_hex(midx_hash));
+			xstrdup(hash_to_hex_algop(midx_hash, r->hash_algo));
 	}
 
 	if (ctx.m || ctx.base_midx)
@@ -1495,7 +1497,7 @@  static int write_midx_internal(const char *object_dir,
 	}
 	strbuf_release(&midx_name);
 
-	trace2_region_leave("midx", "write_midx_internal", the_repository);
+	trace2_region_leave("midx", "write_midx_internal", r);
 
 	return result;
 }
@@ -1505,8 +1507,8 @@  int write_midx_file(const char *object_dir,
 		    const char *refs_snapshot,
 		    unsigned flags)
 {
-	return write_midx_internal(object_dir, NULL, NULL, preferred_pack_name,
-				   refs_snapshot, flags);
+	return write_midx_internal(the_repository, object_dir, NULL, NULL,
+				   preferred_pack_name, refs_snapshot, flags);
 }
 
 int write_midx_file_only(const char *object_dir,
@@ -1515,8 +1517,9 @@  int write_midx_file_only(const char *object_dir,
 			 const char *refs_snapshot,
 			 unsigned flags)
 {
-	return write_midx_internal(object_dir, packs_to_include, NULL,
-				   preferred_pack_name, refs_snapshot, flags);
+	return write_midx_internal(the_repository, object_dir, packs_to_include,
+				   NULL, preferred_pack_name, refs_snapshot,
+				   flags);
 }
 
 int expire_midx_packs(struct repository *r, const char *object_dir, unsigned flags)
@@ -1572,7 +1575,8 @@  int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
 	free(count);
 
 	if (packs_to_drop.nr)
-		result = write_midx_internal(object_dir, NULL, &packs_to_drop, NULL, NULL, flags);
+		result = write_midx_internal(r, object_dir, NULL,
+					     &packs_to_drop, NULL, NULL, flags);
 
 	string_list_clear(&packs_to_drop, 0);
 
@@ -1769,7 +1773,8 @@  int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 		goto cleanup;
 	}
 
-	result = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags);
+	result = write_midx_internal(r, object_dir, NULL, NULL, NULL, NULL,
+				     flags);
 
 cleanup:
 	free(include_pack);