diff mbox series

[v2,2/8] packfile: use `repository` from `packed_git` directly

Message ID ca033556866cbb6cdff49507eb27ed5ef57cf44f.1730122499.git.karthik.188@gmail.com (mailing list archive)
State Superseded
Headers show
Series packfile: avoid using the 'the_repository' global variable | expand

Commit Message

Karthik Nayak Oct. 28, 2024, 1:43 p.m. UTC 
In the previous commit, we introduced the `repository` structure inside
`packed_git`. This provides an alternative route instead of using the
global `the_repository` variable. Let's modify `packfile.c` now to use
this field wherever possible instead of relying on the global state.
There are still a few instances of `the_repository` usage in the file,
where there is no struct `packed_git` locally available, which will be
fixed in the following commits.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 packfile.c | 50 +++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

Comments

Taylor Blau Oct. 28, 2024, 4:08 p.m. UTC | #1 
On Mon, Oct 28, 2024 at 02:43:40PM +0100, Karthik Nayak wrote:
> In the previous commit, we introduced the `repository` structure inside
> `packed_git`. This provides an alternative route instead of using the
> global `the_repository` variable. Let's modify `packfile.c` now to use
> this field wherever possible instead of relying on the global state.
> There are still a few instances of `the_repository` usage in the file,
> where there is no struct `packed_git` locally available, which will be
> fixed in the following commits.
>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  packfile.c | 50 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 27 insertions(+), 23 deletions(-)

Very nice, and indeed much less disruptive than the RFC version of these
patches. All of the first few transformations look correct to me.

> @@ -751,9 +752,13 @@ struct packed_git *add_packed_git(struct repository *repo, const char *path,
>  	p->pack_size = st.st_size;
>  	p->pack_local = local;
>  	p->mtime = st.st_mtime;
> -	if (path_len < the_hash_algo->hexsz ||
> -	    get_hash_hex(path + path_len - the_hash_algo->hexsz, p->hash))
> -		hashclr(p->hash, the_repository->hash_algo);
> +	if (path_len < repo->hash_algo->hexsz ||
> +	    get_oid_hex_algop(path + path_len - repo->hash_algo->hexsz, &oid,
> +			      repo->hash_algo))
> +		hashclr(p->hash, repo->hash_algo);
> +	else
> +		memcpy(p->hash, oid.hash, repo->hash_algo->rawsz);

This should be:

    hashcpy(p->hash, oid.hash, repo->hash_algo);

instead of a bare memcpy().

Everything else is looking good.

Thanks,
Taylor
Karthik Nayak Oct. 29, 2024, 11:48 a.m. UTC | #2 
Taylor Blau <me@ttaylorr.com> writes:

> On Mon, Oct 28, 2024 at 02:43:40PM +0100, Karthik Nayak wrote:
>> In the previous commit, we introduced the `repository` structure inside
>> `packed_git`. This provides an alternative route instead of using the
>> global `the_repository` variable. Let's modify `packfile.c` now to use
>> this field wherever possible instead of relying on the global state.
>> There are still a few instances of `the_repository` usage in the file,
>> where there is no struct `packed_git` locally available, which will be
>> fixed in the following commits.
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  packfile.c | 50 +++++++++++++++++++++++++++-----------------------
>>  1 file changed, 27 insertions(+), 23 deletions(-)
>
> Very nice, and indeed much less disruptive than the RFC version of these
> patches. All of the first few transformations look correct to me.
>
>> @@ -751,9 +752,13 @@ struct packed_git *add_packed_git(struct repository *repo, const char *path,
>>  	p->pack_size = st.st_size;
>>  	p->pack_local = local;
>>  	p->mtime = st.st_mtime;
>> -	if (path_len < the_hash_algo->hexsz ||
>> -	    get_hash_hex(path + path_len - the_hash_algo->hexsz, p->hash))
>> -		hashclr(p->hash, the_repository->hash_algo);
>> +	if (path_len < repo->hash_algo->hexsz ||
>> +	    get_oid_hex_algop(path + path_len - repo->hash_algo->hexsz, &oid,
>> +			      repo->hash_algo))
>> +		hashclr(p->hash, repo->hash_algo);
>> +	else
>> +		memcpy(p->hash, oid.hash, repo->hash_algo->rawsz);
>
> This should be:
>
>     hashcpy(p->hash, oid.hash, repo->hash_algo);
>
> instead of a bare memcpy().
>

Indeed, didn't know of the function, will change.

> Everything else is looking good.
>
> Thanks,
> Taylor
diff mbox series

Patch

diff --git a/packfile.c b/packfile.c
index 45f300e5e1..9b353db331 100644
--- a/packfile.c
+++ b/packfile.c
@@ -79,7 +79,7 @@  static int check_packed_git_idx(const char *path, struct packed_git *p)
 	size_t idx_size;
 	int fd = git_open(path), ret;
 	struct stat st;
-	const unsigned int hashsz = the_hash_algo->rawsz;
+	const unsigned int hashsz = p->repo->hash_algo->rawsz;
 
 	if (fd < 0)
 		return -1;
@@ -243,7 +243,7 @@  struct packed_git *parse_pack_index(struct repository *repo,
 
 	memcpy(p->pack_name, path, alloc); /* includes NUL */
 	free(path);
-	hashcpy(p->hash, sha1, the_repository->hash_algo);
+	hashcpy(p->hash, sha1, p->repo->hash_algo);
 	if (check_packed_git_idx(idx_path, p)) {
 		free(p);
 		return NULL;
@@ -278,7 +278,7 @@  static int unuse_one_window(struct packed_git *current)
 
 	if (current)
 		scan_windows(current, &lru_p, &lru_w, &lru_l);
-	for (p = the_repository->objects->packed_git; p; p = p->next)
+	for (p = current->repo->objects->packed_git; p; p = p->next)
 		scan_windows(p, &lru_p, &lru_w, &lru_l);
 	if (lru_p) {
 		munmap(lru_w->base, lru_w->len);
@@ -540,7 +540,7 @@  static int open_packed_git_1(struct packed_git *p)
 	unsigned char hash[GIT_MAX_RAWSZ];
 	unsigned char *idx_hash;
 	ssize_t read_result;
-	const unsigned hashsz = the_hash_algo->rawsz;
+	const unsigned hashsz = p->repo->hash_algo->rawsz;
 
 	if (open_pack_index(p))
 		return error("packfile %s index unavailable", p->pack_name);
@@ -597,7 +597,7 @@  static int open_packed_git_1(struct packed_git *p)
 	if (read_result != hashsz)
 		return error("packfile %s signature is unavailable", p->pack_name);
 	idx_hash = ((unsigned char *)p->index_data) + p->index_size - hashsz * 2;
-	if (!hasheq(hash, idx_hash, the_repository->hash_algo))
+	if (!hasheq(hash, idx_hash, p->repo->hash_algo))
 		return error("packfile %s does not match index", p->pack_name);
 	return 0;
 }
@@ -637,7 +637,7 @@  unsigned char *use_pack(struct packed_git *p,
 	 */
 	if (!p->pack_size && p->pack_fd == -1 && open_packed_git(p))
 		die("packfile %s cannot be accessed", p->pack_name);
-	if (offset > (p->pack_size - the_hash_algo->rawsz))
+	if (offset > (p->pack_size - p->repo->hash_algo->rawsz))
 		die("offset beyond end of packfile (truncated pack?)");
 	if (offset < 0)
 		die(_("offset before end of packfile (broken .idx?)"));
@@ -711,6 +711,7 @@  struct packed_git *add_packed_git(struct repository *repo, const char *path,
 	struct stat st;
 	size_t alloc;
 	struct packed_git *p;
+	struct object_id oid;
 
 	/*
 	 * Make sure a corresponding .pack file exists and that
@@ -751,9 +752,13 @@  struct packed_git *add_packed_git(struct repository *repo, const char *path,
 	p->pack_size = st.st_size;
 	p->pack_local = local;
 	p->mtime = st.st_mtime;
-	if (path_len < the_hash_algo->hexsz ||
-	    get_hash_hex(path + path_len - the_hash_algo->hexsz, p->hash))
-		hashclr(p->hash, the_repository->hash_algo);
+	if (path_len < repo->hash_algo->hexsz ||
+	    get_oid_hex_algop(path + path_len - repo->hash_algo->hexsz, &oid,
+			      repo->hash_algo))
+		hashclr(p->hash, repo->hash_algo);
+	else
+		memcpy(p->hash, oid.hash, repo->hash_algo->rawsz);
+
 	return p;
 }
 
@@ -1243,9 +1248,9 @@  off_t get_delta_base(struct packed_git *p,
 	} else if (type == OBJ_REF_DELTA) {
 		/* The base entry _must_ be in the same pack */
 		struct object_id oid;
-		oidread(&oid, base_info, the_repository->hash_algo);
+		oidread(&oid, base_info, p->repo->hash_algo);
 		base_offset = find_pack_entry_one(&oid, p);
-		*curpos += the_hash_algo->rawsz;
+		*curpos += p->repo->hash_algo->rawsz;
 	} else
 		die("I am totally screwed");
 	return base_offset;
@@ -1266,7 +1271,7 @@  static int get_delta_base_oid(struct packed_git *p,
 {
 	if (type == OBJ_REF_DELTA) {
 		unsigned char *base = use_pack(p, w_curs, curpos, NULL);
-		oidread(oid, base, the_repository->hash_algo);
+		oidread(oid, base, p->repo->hash_algo);
 		return 0;
 	} else if (type == OBJ_OFS_DELTA) {
 		uint32_t base_pos;
@@ -1608,7 +1613,7 @@  int packed_object_info(struct repository *r, struct packed_git *p,
 				goto out;
 			}
 		} else
-			oidclr(oi->delta_base_oid, the_repository->hash_algo);
+			oidclr(oi->delta_base_oid, p->repo->hash_algo);
 	}
 
 	oi->whence = in_delta_base_cache(p, obj_offset) ? OI_DBCACHED :
@@ -1897,7 +1902,7 @@  int bsearch_pack(const struct object_id *oid, const struct packed_git *p, uint32
 {
 	const unsigned char *index_fanout = p->index_data;
 	const unsigned char *index_lookup;
-	const unsigned int hashsz = the_hash_algo->rawsz;
+	const unsigned int hashsz = p->repo->hash_algo->rawsz;
 	int index_lookup_width;
 
 	if (!index_fanout)
@@ -1922,7 +1927,7 @@  int nth_packed_object_id(struct object_id *oid,
 			 uint32_t n)
 {
 	const unsigned char *index = p->index_data;
-	const unsigned int hashsz = the_hash_algo->rawsz;
+	const unsigned int hashsz = p->repo->hash_algo->rawsz;
 	if (!index) {
 		if (open_pack_index(p))
 			return -1;
@@ -1933,11 +1938,10 @@  int nth_packed_object_id(struct object_id *oid,
 	index += 4 * 256;
 	if (p->index_version == 1) {
 		oidread(oid, index + st_add(st_mult(hashsz + 4, n), 4),
-			the_repository->hash_algo);
+			p->repo->hash_algo);
 	} else {
 		index += 8;
-		oidread(oid, index + st_mult(hashsz, n),
-			the_repository->hash_algo);
+		oidread(oid, index + st_mult(hashsz, n), p->repo->hash_algo);
 	}
 	return 0;
 }
@@ -1959,7 +1963,7 @@  void check_pack_index_ptr(const struct packed_git *p, const void *vptr)
 off_t nth_packed_object_offset(const struct packed_git *p, uint32_t n)
 {
 	const unsigned char *index = p->index_data;
-	const unsigned int hashsz = the_hash_algo->rawsz;
+	const unsigned int hashsz = p->repo->hash_algo->rawsz;
 	index += 4 * 256;
 	if (p->index_version == 1) {
 		return ntohl(*((uint32_t *)(index + st_mult(hashsz + 4, n))));
@@ -2159,7 +2163,7 @@  int for_each_object_in_pack(struct packed_git *p,
 	int r = 0;
 
 	if (flags & FOR_EACH_OBJECT_PACK_ORDER) {
-		if (load_pack_revindex(the_repository, p))
+		if (load_pack_revindex(p->repo, p))
 			return -1;
 	}
 
@@ -2227,7 +2231,7 @@  int for_each_packed_object(each_packed_object_fn cb, void *data,
 }
 
 static int add_promisor_object(const struct object_id *oid,
-			       struct packed_git *pack UNUSED,
+			       struct packed_git *pack,
 			       uint32_t pos UNUSED,
 			       void *set_)
 {
@@ -2235,12 +2239,12 @@  static int add_promisor_object(const struct object_id *oid,
 	struct object *obj;
 	int we_parsed_object;
 
-	obj = lookup_object(the_repository, oid);
+	obj = lookup_object(pack->repo, oid);
 	if (obj && obj->parsed) {
 		we_parsed_object = 0;
 	} else {
 		we_parsed_object = 1;
-		obj = parse_object(the_repository, oid);
+		obj = parse_object(pack->repo, oid);
 	}
 
 	if (!obj)