* [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`
2025-01-17 9:20 ` [PATCH v2 0/5] pack-write: cleanup usage of global variables Karthik Nayak
@ 2025-01-17 9:20 ` Karthik Nayak
2025-01-17 16:38 ` Toon Claes
2025-01-17 9:20 ` [PATCH v2 2/5] pack-write: pass repository to `index_pack_lockfile()` Karthik Nayak
` (4 subsequent siblings)
5 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-01-17 9:20 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`
2025-01-17 9:20 ` [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()` Karthik Nayak
@ 2025-01-17 16:38 ` Toon Claes
2025-01-17 18:06 ` Junio C Hamano
0 siblings, 1 reply; 36+ messages in thread
From: Toon Claes @ 2025-01-17 16:38 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`
2025-01-17 16:38 ` Toon Claes
@ 2025-01-17 18:06 ` Junio C Hamano
2025-01-19 11:07 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Junio C Hamano @ 2025-01-17 18:06 UTC (permalink / raw)
To: Toon Claes; +Cc: Karthik Nayak, git, ps
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?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`
2025-01-17 18:06 ` Junio C Hamano
@ 2025-01-19 11:07 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:07 UTC (permalink / raw)
To: Junio C Hamano, Toon Claes; +Cc: git, ps
[-- 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 --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] pack-write: pass repository to `index_pack_lockfile()`
2025-01-17 9:20 ` [PATCH v2 0/5] pack-write: cleanup usage of global variables Karthik Nayak
2025-01-17 9:20 ` [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()` Karthik Nayak
@ 2025-01-17 9:20 ` Karthik Nayak
2025-01-17 9:20 ` [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()` Karthik Nayak
` (3 subsequent siblings)
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-17 9:20 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()`
2025-01-17 9:20 ` [PATCH v2 0/5] pack-write: cleanup usage of global variables Karthik Nayak
2025-01-17 9:20 ` [PATCH v2 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()` Karthik Nayak
2025-01-17 9:20 ` [PATCH v2 2/5] pack-write: pass repository to `index_pack_lockfile()` Karthik Nayak
@ 2025-01-17 9:20 ` Karthik Nayak
2025-01-17 16:40 ` Toon Claes
2025-01-17 9:20 ` [PATCH v2 4/5] pack-write: pass hash_algo to `write_rev_file()` Karthik Nayak
` (2 subsequent siblings)
5 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-01-17 9:20 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()`
2025-01-17 9:20 ` [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()` Karthik Nayak
@ 2025-01-17 16:40 ` Toon Claes
2025-01-19 11:10 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Toon Claes @ 2025-01-17 16:40 UTC (permalink / raw)
To: Karthik Nayak, git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()`
2025-01-17 16:40 ` Toon Claes
@ 2025-01-19 11:10 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:10 UTC (permalink / raw)
To: Toon Claes, git; +Cc: ps, gitster
[-- 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 --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 4/5] pack-write: pass hash_algo to `write_rev_file()`
2025-01-17 9:20 ` [PATCH v2 0/5] pack-write: cleanup usage of global variables Karthik Nayak
` (2 preceding siblings ...)
2025-01-17 9:20 ` [PATCH v2 3/5] pack-write: pass hash_algo to `write_idx_file()` Karthik Nayak
@ 2025-01-17 9:20 ` Karthik Nayak
2025-01-17 9:20 ` [PATCH v2 5/5] pack-write: pass hash_algo to internal functions Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-17 9:20 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v2 5/5] pack-write: pass hash_algo to internal functions
2025-01-17 9:20 ` [PATCH v2 0/5] pack-write: cleanup usage of global variables Karthik Nayak
` (3 preceding siblings ...)
2025-01-17 9:20 ` [PATCH v2 4/5] pack-write: pass hash_algo to `write_rev_file()` Karthik Nayak
@ 2025-01-17 9:20 ` Karthik Nayak
2025-01-17 9:46 ` Patrick Steinhardt
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
5 siblings, 1 reply; 36+ messages in thread
From: Karthik Nayak @ 2025-01-17 9:20 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v2 5/5] pack-write: pass hash_algo to internal functions
2025-01-17 9:20 ` [PATCH v2 5/5] pack-write: pass hash_algo to internal functions Karthik Nayak
@ 2025-01-17 9:46 ` Patrick Steinhardt
2025-01-17 9:55 ` Karthik Nayak
0 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-01-17 9:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/5] pack-write: pass hash_algo to internal functions
2025-01-17 9:46 ` Patrick Steinhardt
@ 2025-01-17 9:55 ` Karthik Nayak
0 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-17 9:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, gitster
[-- 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 --]
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 0/5] pack-write: cleanup usage of global variables
2025-01-17 9:20 ` [PATCH v2 0/5] pack-write: cleanup usage of global variables Karthik Nayak
` (4 preceding siblings ...)
2025-01-17 9:20 ` [PATCH v2 5/5] pack-write: pass hash_algo to internal functions Karthik Nayak
@ 2025-01-19 11:19 ` Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()` Karthik Nayak
` (5 more replies)
5 siblings, 6 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:19 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster, toon
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
^ permalink raw reply [flat|nested] 36+ messages in thread* [PATCH v3 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()`
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
@ 2025-01-19 11:19 ` Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 2/5] pack-write: pass repository to `index_pack_lockfile()` Karthik Nayak
` (4 subsequent siblings)
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:19 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster, toon
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 2/5] pack-write: pass repository to `index_pack_lockfile()`
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()` Karthik Nayak
@ 2025-01-19 11:19 ` Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 3/5] pack-write: pass hash_algo to `write_idx_file()` Karthik Nayak
` (3 subsequent siblings)
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:19 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster, toon
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 3/5] pack-write: pass hash_algo to `write_idx_file()`
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 1/5] pack-write: pass hash_algo to `fixup_pack_header_footer()` Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 2/5] pack-write: pass repository to `index_pack_lockfile()` Karthik Nayak
@ 2025-01-19 11:19 ` Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 4/5] pack-write: pass hash_algo to `write_rev_file()` Karthik Nayak
` (2 subsequent siblings)
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:19 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster, toon
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 4/5] pack-write: pass hash_algo to `write_rev_file()`
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
` (2 preceding siblings ...)
2025-01-19 11:19 ` [PATCH v3 3/5] pack-write: pass hash_algo to `write_idx_file()` Karthik Nayak
@ 2025-01-19 11:19 ` Karthik Nayak
2025-01-19 11:19 ` [PATCH v3 5/5] pack-write: pass hash_algo to internal functions Karthik Nayak
2025-01-24 5:46 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Patrick Steinhardt
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:19 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster, toon
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 5/5] pack-write: pass hash_algo to internal functions
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
` (3 preceding siblings ...)
2025-01-19 11:19 ` [PATCH v3 4/5] pack-write: pass hash_algo to `write_rev_file()` Karthik Nayak
@ 2025-01-19 11:19 ` Karthik Nayak
2025-01-24 5:46 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Patrick Steinhardt
5 siblings, 0 replies; 36+ messages in thread
From: Karthik Nayak @ 2025-01-19 11:19 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, ps, gitster, toon
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 0/5] pack-write: cleanup usage of global variables
2025-01-19 11:19 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Karthik Nayak
` (4 preceding siblings ...)
2025-01-19 11:19 ` [PATCH v3 5/5] pack-write: pass hash_algo to internal functions Karthik Nayak
@ 2025-01-24 5:46 ` Patrick Steinhardt
2025-01-24 15:47 ` Junio C Hamano
5 siblings, 1 reply; 36+ messages in thread
From: Patrick Steinhardt @ 2025-01-24 5:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, gitster, toon
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
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 0/5] pack-write: cleanup usage of global variables
2025-01-24 5:46 ` [PATCH v3 0/5] pack-write: cleanup usage of global variables Patrick Steinhardt
@ 2025-01-24 15:47 ` Junio C Hamano
0 siblings, 0 replies; 36+ messages in thread
From: Junio C Hamano @ 2025-01-24 15:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git, toon
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.
^ permalink raw reply [flat|nested] 36+ messages in thread