diff mbox series

[01/20] packfile: pass down repository to `odb_pack_name`

Message ID c8cc21c2eeb9d1e0bc261143e642eff38fe33888.1729504641.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. 21, 2024, 9:57 a.m. UTC 
The function `odb_pack_name` currently relies on the global variable
`the_repository`. To eliminate global variable usage in `packfile.c`, we
should progressively shift the dependency on the_repository to higher
layers. Let's remove its usage from this function and any related ones.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fast-import.c    |  8 ++++----
 builtin/index-pack.c     |  4 ++--
 builtin/pack-redundant.c |  2 +-
 http.c                   | 15 +++++++++------
 packfile.c               | 27 ++++++++++++++-------------
 packfile.h               | 13 ++++++++-----
 6 files changed, 38 insertions(+), 31 deletions(-)

Comments

Taylor Blau Oct. 21, 2024, 9:06 p.m. UTC | #1 
On Mon, Oct 21, 2024 at 11:57:44AM +0200, Karthik Nayak wrote:
> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> index 5809613002..60f806e672 100644
> --- a/builtin/pack-redundant.c
> +++ b/builtin/pack-redundant.c
> @@ -688,7 +688,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
>  	pl = red = pack_list_difference(local_packs, min);
>  	while (pl) {
>  		printf("%s\n%s\n",
> -		       sha1_pack_index_name(pl->pack->hash),
> +		       sha1_pack_index_name(the_repository, pl->pack->hash),
>  		       pl->pack->pack_name);
>  		pl = pl->next;
>  	}

I am a little surprised to see sha1_pack_index_name() converted
similarly here, as this patch promises only to touch the
'odb_pack_name()' function.

> diff --git a/packfile.h b/packfile.h
> index 0f78658229..507ac602b5 100644
> --- a/packfile.h
> +++ b/packfile.h

Indeed, it looks like odb_pack_name(), sha1_pack_name(),
sha1_pack_index_name(), parse_pack_index(), and has_pack_index() are all
modified. Were these meant to go in separate patches?

Thanks,
Taylor
Karthik Nayak Oct. 22, 2024, 8:51 a.m. UTC | #2 
Taylor Blau <me@ttaylorr.com> writes:

> On Mon, Oct 21, 2024 at 11:57:44AM +0200, Karthik Nayak wrote:
>> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
>> index 5809613002..60f806e672 100644
>> --- a/builtin/pack-redundant.c
>> +++ b/builtin/pack-redundant.c
>> @@ -688,7 +688,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
>>  	pl = red = pack_list_difference(local_packs, min);
>>  	while (pl) {
>>  		printf("%s\n%s\n",
>> -		       sha1_pack_index_name(pl->pack->hash),
>> +		       sha1_pack_index_name(the_repository, pl->pack->hash),
>>  		       pl->pack->pack_name);
>>  		pl = pl->next;
>>  	}
>
> I am a little surprised to see sha1_pack_index_name() converted
> similarly here, as this patch promises only to touch the
> 'odb_pack_name()' function.
>
>> diff --git a/packfile.h b/packfile.h
>> index 0f78658229..507ac602b5 100644
>> --- a/packfile.h
>> +++ b/packfile.h
>
> Indeed, it looks like odb_pack_name(), sha1_pack_name(),
> sha1_pack_index_name(), parse_pack_index(), and has_pack_index() are all
> modified. Were these meant to go in separate patches?
>

Nope this is intentional, each commit tries to pick a base function and
modifies all layers above it (I should have explicitly specified my
approach in the cover).

In this commit, we try to modify `odb_pack_name()` and therefore
sha1_pack_name(), sha1_pack_index_name(), parse_pack_index(), and
has_pack_index() too. Otherwise, we'd have a lot lot more commits.

This simplies the review too, but yes, will add more instructions to the
next version.

> Thanks,
> Taylor

Karthik
Taylor Blau Oct. 22, 2024, 4:37 p.m. UTC | #3 
On Tue, Oct 22, 2024 at 04:51:04AM -0400, karthik nayak wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > On Mon, Oct 21, 2024 at 11:57:44AM +0200, Karthik Nayak wrote:
> >> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
> >> index 5809613002..60f806e672 100644
> >> --- a/builtin/pack-redundant.c
> >> +++ b/builtin/pack-redundant.c
> >> @@ -688,7 +688,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
> >>  	pl = red = pack_list_difference(local_packs, min);
> >>  	while (pl) {
> >>  		printf("%s\n%s\n",
> >> -		       sha1_pack_index_name(pl->pack->hash),
> >> +		       sha1_pack_index_name(the_repository, pl->pack->hash),
> >>  		       pl->pack->pack_name);
> >>  		pl = pl->next;
> >>  	}
> >
> > I am a little surprised to see sha1_pack_index_name() converted
> > similarly here, as this patch promises only to touch the
> > 'odb_pack_name()' function.
> >
> >> diff --git a/packfile.h b/packfile.h
> >> index 0f78658229..507ac602b5 100644
> >> --- a/packfile.h
> >> +++ b/packfile.h
> >
> > Indeed, it looks like odb_pack_name(), sha1_pack_name(),
> > sha1_pack_index_name(), parse_pack_index(), and has_pack_index() are all
> > modified. Were these meant to go in separate patches?
> >
>
> Nope this is intentional, each commit tries to pick a base function and
> modifies all layers above it (I should have explicitly specified my
> approach in the cover).
>
> In this commit, we try to modify `odb_pack_name()` and therefore
> sha1_pack_name(), sha1_pack_index_name(), parse_pack_index(), and
> has_pack_index() too. Otherwise, we'd have a lot lot more commits.
>
> This simplies the review too, but yes, will add more instructions to the
> next version.

Makes sense, thanks for clarifying. I agree that this would be good
information to have in the patch message to avoid confusion.


Thanks,
Taylor
diff mbox series

Patch

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 1e7ab67f6e..7ad950627c 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -805,7 +805,7 @@  static char *keep_pack(const char *curr_index_name)
 	struct strbuf name = STRBUF_INIT;
 	int keep_fd;
 
-	odb_pack_name(&name, pack_data->hash, "keep");
+	odb_pack_name(the_repository, &name, pack_data->hash, "keep");
 	keep_fd = odb_pack_keep(name.buf);
 	if (keep_fd < 0)
 		die_errno("cannot create keep file");
@@ -813,11 +813,11 @@  static char *keep_pack(const char *curr_index_name)
 	if (close(keep_fd))
 		die_errno("failed to write keep file");
 
-	odb_pack_name(&name, pack_data->hash, "pack");
+	odb_pack_name(the_repository, &name, pack_data->hash, "pack");
 	if (finalize_object_file(pack_data->pack_name, name.buf))
 		die("cannot store pack file");
 
-	odb_pack_name(&name, pack_data->hash, "idx");
+	odb_pack_name(the_repository, &name, pack_data->hash, "idx");
 	if (finalize_object_file(curr_index_name, name.buf))
 		die("cannot store index file");
 	free((void *)curr_index_name);
@@ -831,7 +831,7 @@  static void unkeep_all_packs(void)
 
 	for (k = 0; k < pack_id; k++) {
 		struct packed_git *p = all_packs[k];
-		odb_pack_name(&name, p->hash, "keep");
+		odb_pack_name(the_repository, &name, p->hash, "keep");
 		unlink_or_warn(name.buf);
 	}
 	strbuf_release(&name);
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 9d23b41b3a..97afc69625 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1479,7 +1479,7 @@  static void write_special_file(const char *suffix, const char *msg,
 	if (pack_name)
 		filename = derive_filename(pack_name, "pack", suffix, &name_buf);
 	else
-		filename = odb_pack_name(&name_buf, hash, suffix);
+		filename = odb_pack_name(the_repository, &name_buf, hash, suffix);
 
 	fd = odb_pack_keep(filename);
 	if (fd < 0) {
@@ -1507,7 +1507,7 @@  static void rename_tmp_packfile(const char **final_name,
 {
 	if (!*final_name || strcmp(*final_name, curr_name)) {
 		if (!*final_name)
-			*final_name = odb_pack_name(name, hash, ext);
+			*final_name = odb_pack_name(the_repository, name, hash, ext);
 		if (finalize_object_file(curr_name, *final_name))
 			die(_("unable to rename temporary '*.%s' file to '%s'"),
 			    ext, *final_name);
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 5809613002..60f806e672 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -688,7 +688,7 @@  int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
 	pl = red = pack_list_difference(local_packs, min);
 	while (pl) {
 		printf("%s\n%s\n",
-		       sha1_pack_index_name(pl->pack->hash),
+		       sha1_pack_index_name(the_repository, pl->pack->hash),
 		       pl->pack->pack_name);
 		pl = pl->next;
 	}
diff --git a/http.c b/http.c
index d59e59f66b..309669c203 100644
--- a/http.c
+++ b/http.c
@@ -2388,7 +2388,7 @@  static char *fetch_pack_index(unsigned char *hash, const char *base_url)
 	strbuf_addf(&buf, "objects/pack/pack-%s.idx", hash_to_hex(hash));
 	url = strbuf_detach(&buf, NULL);
 
-	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(hash));
+	strbuf_addf(&buf, "%s.temp", sha1_pack_index_name(the_repository, hash));
 	tmp = strbuf_detach(&buf, NULL);
 
 	if (http_get_file(url, tmp, NULL) != HTTP_OK) {
@@ -2407,8 +2407,10 @@  static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	char *tmp_idx = NULL;
 	int ret;
 
-	if (has_pack_index(sha1)) {
-		new_pack = parse_pack_index(sha1, sha1_pack_index_name(sha1));
+	if (has_pack_index(the_repository, sha1)) {
+		new_pack = parse_pack_index(the_repository, sha1,
+					    sha1_pack_index_name(the_repository,
+								 sha1));
 		if (!new_pack)
 			return -1; /* parse_pack_index() already issued error message */
 		goto add_pack;
@@ -2418,7 +2420,7 @@  static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	if (!tmp_idx)
 		return -1;
 
-	new_pack = parse_pack_index(sha1, tmp_idx);
+	new_pack = parse_pack_index(the_repository, sha1, tmp_idx);
 	if (!new_pack) {
 		unlink(tmp_idx);
 		free(tmp_idx);
@@ -2429,7 +2431,7 @@  static int fetch_and_setup_pack_index(struct packed_git **packs_head,
 	ret = verify_pack_index(new_pack);
 	if (!ret) {
 		close_pack_index(new_pack);
-		ret = finalize_object_file(tmp_idx, sha1_pack_index_name(sha1));
+		ret = finalize_object_file(tmp_idx, sha1_pack_index_name(the_repository, sha1));
 	}
 	free(tmp_idx);
 	if (ret)
@@ -2563,7 +2565,8 @@  struct http_pack_request *new_direct_http_pack_request(
 
 	preq->url = url;
 
-	strbuf_addf(&preq->tmpfile, "%s.temp", sha1_pack_name(packed_git_hash));
+	strbuf_addf(&preq->tmpfile, "%s.temp", sha1_pack_name(the_repository,
+							      packed_git_hash));
 	preq->packfile = fopen(preq->tmpfile.buf, "a");
 	if (!preq->packfile) {
 		error("Unable to open local file %s for pack",
diff --git a/packfile.c b/packfile.c
index df4ba67719..e4569ea29d 100644
--- a/packfile.c
+++ b/packfile.c
@@ -25,26 +25,25 @@ 
 #include "pack-revindex.h"
 #include "promisor-remote.h"
 
-char *odb_pack_name(struct strbuf *buf,
-		    const unsigned char *hash,
-		    const char *ext)
+char *odb_pack_name(struct repository *repo, struct strbuf *buf,
+		    const unsigned char *hash, const char *ext)
 {
 	strbuf_reset(buf);
-	strbuf_addf(buf, "%s/pack/pack-%s.%s", repo_get_object_directory(the_repository),
+	strbuf_addf(buf, "%s/pack/pack-%s.%s", repo_get_object_directory(repo),
 		    hash_to_hex(hash), ext);
 	return buf->buf;
 }
 
-char *sha1_pack_name(const unsigned char *sha1)
+char *sha1_pack_name(struct repository *repo, const unsigned char *sha1)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	return odb_pack_name(&buf, sha1, "pack");
+	return odb_pack_name(repo, &buf, sha1, "pack");
 }
 
-char *sha1_pack_index_name(const unsigned char *sha1)
+char *sha1_pack_index_name(struct repository *repo, const unsigned char *sha1)
 {
 	static struct strbuf buf = STRBUF_INIT;
-	return odb_pack_name(&buf, sha1, "idx");
+	return odb_pack_name(repo, &buf, sha1, "idx");
 }
 
 static unsigned int pack_used_ctr;
@@ -237,14 +236,16 @@  static struct packed_git *alloc_packed_git(int extra)
 	return p;
 }
 
-struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path)
+struct packed_git *parse_pack_index(struct repository *repo,
+				    unsigned char *sha1,
+				    const char *idx_path)
 {
-	const char *path = sha1_pack_name(sha1);
+	const char *path = sha1_pack_name(repo, sha1);
 	size_t alloc = st_add(strlen(path), 1);
 	struct packed_git *p = alloc_packed_git(alloc);
 
 	memcpy(p->pack_name, path, alloc); /* includes NUL */
-	hashcpy(p->hash, sha1, the_repository->hash_algo);
+	hashcpy(p->hash, sha1, repo->hash_algo);
 	if (check_packed_git_idx(idx_path, p)) {
 		free(p);
 		return NULL;
@@ -2151,10 +2152,10 @@  int has_object_kept_pack(const struct object_id *oid, unsigned flags)
 	return find_kept_pack_entry(the_repository, oid, flags, &e);
 }
 
-int has_pack_index(const unsigned char *sha1)
+int has_pack_index(struct repository *repo, const unsigned char *sha1)
 {
 	struct stat st;
-	if (stat(sha1_pack_index_name(sha1), &st))
+	if (stat(sha1_pack_index_name(repo, sha1), &st))
 		return 0;
 	return 1;
 }
diff --git a/packfile.h b/packfile.h
index 0f78658229..507ac602b5 100644
--- a/packfile.h
+++ b/packfile.h
@@ -29,21 +29,22 @@  struct pack_entry {
  *
  * Example: odb_pack_name(out, sha1, "idx") => ".git/objects/pack/pack-1234..idx"
  */
-char *odb_pack_name(struct strbuf *buf, const unsigned char *sha1, const char *ext);
+char *odb_pack_name(struct repository *repo, struct strbuf *buf,
+		    const unsigned char *sha1, const char *ext);
 
 /*
  * Return the name of the (local) packfile with the specified sha1 in
  * its name.  The return value is a pointer to memory that is
  * overwritten each time this function is called.
  */
-char *sha1_pack_name(const unsigned char *sha1);
+char *sha1_pack_name(struct repository *repo, const unsigned char *sha1);
 
 /*
  * Return the name of the (local) pack index file with the specified
  * sha1 in its name.  The return value is a pointer to memory that is
  * overwritten each time this function is called.
  */
-char *sha1_pack_index_name(const unsigned char *sha1);
+char *sha1_pack_index_name(struct repository *repo, const unsigned char *sha1);
 
 /*
  * Return the basename of the packfile, omitting any containing directory
@@ -51,7 +52,9 @@  char *sha1_pack_index_name(const unsigned char *sha1);
  */
 const char *pack_basename(struct packed_git *p);
 
-struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
+struct packed_git *parse_pack_index(struct repository *repo,
+				    unsigned char *sha1,
+				    const char *idx_path);
 
 typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,
 				      const char *file_name, void *data);
@@ -193,7 +196,7 @@  int find_kept_pack_entry(struct repository *r, const struct object_id *oid, unsi
 int has_object_pack(const struct object_id *oid);
 int has_object_kept_pack(const struct object_id *oid, unsigned flags);
 
-int has_pack_index(const unsigned char *sha1);
+int has_pack_index(struct repository *repo, const unsigned char *sha1);
 
 /*
  * Return 1 if an object in a promisor packfile is or refers to the given