git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 05/10] packfile: move packfile store into object source

From
Justin Tobler <jltobler@gmail.com>
Date
Dec 18, 2025, 00:52 UTC
Message-ID
<coihoxu2loroo4xrlog226c6ib7hy6wfsvlbfvanhp3xksezie@buisls2ii3d5>
In-Reply-To
<20251215-b4-pks-pack-store-via-source-v1-5-433aac465295@pks.im>
On 25/12/15 08:36AM, Patrick Steinhardt wrote:
Show 7 quoted lines
> The packfile store is a member of `struct object_database`, which means
> that we have a single store per database. This doesn't really make much
> sense though: each source connected to the database has its own set of
> packfiles, so there is a conceptual mismatch here. This hasn't really
> caused much of a problem in the past, but with the advent of pluggable
> object databases this is becoming more of a problem because some of the
> sources may not even use packfiles in the first place.

So since there there is only a single packfile store per ODB, this means that all sources use the same packfile store a thus there is a single place to find all packfiles. I suppose this means access patterns must be changed to account for alternate sources that would now each have there own packfile store. Overall this change sounds reasonable to me.

Show 5 quoted lines
> Move the packfile store down by one level from the object database into
> the object database source. This ensures that each source now has its
> own packfile store, and we can eventually start to abstract it away
> entirely so that the caller doesn't even know what kind of store it
> uses.
Makes sense.
Show 39 quoted lines
> Note that we only need to adjust a relatively small number of callers,
> way less than one might expect. This is because most callers are using
> `repo_for_each_pack()`, which handles enumeration of all packfiles that
> exist in the repository. So for now, none of these callers need to be
> adapted. The remaining callers that iterate through the packfiles
> directly and that need adjustment are those that are a bit more tangled
> with packfiles. These will be adjusted over time.
> 
> Note that this patch only moves the packfile store, and there is still a
> bunch of functions that seemingly operate on a packfile store but that
> end up iterating over all sources. These will be adjusted in subsequent
> commits.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fast-import.c  |  37 ++++++++------
>  builtin/grep.c         |   6 ++-
>  builtin/index-pack.c   |   2 +-
>  builtin/pack-objects.c |  96 +++++++++++++++++++------------------
>  http.c                 |   2 +-
>  midx.c                 |   5 +-
>  odb.c                  |  36 +++++++-------
>  odb.h                  |   6 +--
>  odb/streaming.c        |   9 ++--
>  packfile.c             | 127 +++++++++++++++++++++++++++++++------------------
>  packfile.h             |  62 ++++++++++++++++++++----
>  11 files changed, 243 insertions(+), 145 deletions(-)
> 
> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 7849005ccb..b8a7757cfd 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -900,7 +900,7 @@ static void end_packfile(void)
>  		idx_name = keep_pack(create_index());
>  
>  		/* Register the packfile with core git's machinery. */
> -		new_p = packfile_store_load_pack(pack_data->repo->objects->packfiles,
> +		new_p = packfile_store_load_pack(pack_data->repo->objects->sources->packfiles,
>  						 idx_name, 1);

Naive question: it looks likes we are only using the primary source's packfile store here. Is that fine?

Show 21 quoted lines
>  		if (!new_p)
>  			die(_("core Git rejected index %s"), idx_name);
> @@ -955,7 +955,7 @@ static int store_object(
>  	struct object_id *oidout,
>  	uintmax_t mark)
>  {
> -	struct packfile_store *packs = the_repository->objects->packfiles;
> +	struct odb_source *source;
>  	void *out, *delta;
>  	struct object_entry *e;
>  	unsigned char hdr[96];
> @@ -979,7 +979,11 @@ static int store_object(
>  	if (e->idx.offset) {
>  		duplicate_count_by_type[type]++;
>  		return 1;
> -	} else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) {
> +	}
> +
> +	for (source = the_repository->objects->sources; source; source = source->next) {
> +		if (!packfile_list_find_oid(packfile_store_get_packs(source->packfiles), &oid))
> +			continue;

Here we now iterate across each ODB source to check each of the packfile stores to find the OID. This matches the previous behavior.

Show 26 quoted lines
>  		e->type = type;
>  		e->pack_id = MAX_PACK_ID;
>  		e->idx.offset = 1; /* just not zero! */
> @@ -1096,10 +1100,10 @@ static void truncate_pack(struct hashfile_checkpoint *checkpoint)
>  
>  static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
>  {
> -	struct packfile_store *packs = the_repository->objects->packfiles;
>  	size_t in_sz = 64 * 1024, out_sz = 64 * 1024;
>  	unsigned char *in_buf = xmalloc(in_sz);
>  	unsigned char *out_buf = xmalloc(out_sz);
> +	struct odb_source *source;
>  	struct object_entry *e;
>  	struct object_id oid;
>  	unsigned long hdrlen;
> @@ -1179,24 +1183,29 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)
>  	if (e->idx.offset) {
>  		duplicate_count_by_type[OBJ_BLOB]++;
>  		truncate_pack(&checkpoint);
> +		goto out;
> +	}
>  
> -	} else if (packfile_list_find_oid(packfile_store_get_packs(packs), &oid)) {
> +	for (source = the_repository->objects->sources; source; source = source->next) {
> +		if (!packfile_list_find_oid(packfile_store_get_packs(source->packfiles), &oid))
> +			continue;
Same here. Looks good.
Show 57 quoted lines
>  		e->type = OBJ_BLOB;
>  		e->pack_id = MAX_PACK_ID;
>  		e->idx.offset = 1; /* just not zero! */
>  		duplicate_count_by_type[OBJ_BLOB]++;
>  		truncate_pack(&checkpoint);
> -
> -	} else {
> -		e->depth = 0;
> -		e->type = OBJ_BLOB;
> -		e->pack_id = pack_id;
> -		e->idx.offset = offset;
> -		e->idx.crc32 = crc32_end(pack_file);
> -		object_count++;
> -		object_count_by_type[OBJ_BLOB]++;
> +		goto out;
>  	}
>  
> +	e->depth = 0;
> +	e->type = OBJ_BLOB;
> +	e->pack_id = pack_id;
> +	e->idx.offset = offset;
> +	e->idx.crc32 = crc32_end(pack_file);
> +	object_count++;
> +	object_count_by_type[OBJ_BLOB]++;
> +
> +out:
>  	free(in_buf);
>  	free(out_buf);
>  }
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 53cccf2d25..4855b871dd 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1213,8 +1213,12 @@ int cmd_grep(int argc,
>  		 */
>  		if (recurse_submodules)
>  			repo_read_gitmodules(the_repository, 1);
> +		/*
> +		 * Note: `packfile_store_prepare()` prepares stores from all
> +		 * sources. This will be fixed in a subsequent commit.
> +		 */
>  		if (startup_info->have_repository)
> -			packfile_store_prepare(the_repository->objects->packfiles);
> +			packfile_store_prepare(the_repository->objects->sources->packfiles);
>  
>  		start_threads(&opt);
>  	} else {
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a7e901e49c..b67fb0256c 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1638,7 +1638,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  			    hash, "idx", 1);
>  
>  	if (do_fsck_object && startup_info->have_repository)
> -		packfile_store_load_pack(the_repository->objects->packfiles,
> +		packfile_store_load_pack(the_repository->objects->sources->packfiles,
Does packfile_store_load_pack() also load stores from all sources?
Show 96 quoted lines
>  					 final_index_name, 0);
>  
>  	if (!from_stdin) {
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index e86b8f387a..7fd90a9996 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1529,49 +1529,53 @@ static int want_cruft_object_mtime(struct repository *r,
>  				   const struct object_id *oid,
>  				   unsigned flags, uint32_t mtime)
>  {
> -	struct packed_git **cache = packfile_store_get_kept_pack_cache(r->objects->packfiles, flags);
> +	struct odb_source *source;
>  
> -	for (; *cache; cache++) {
> -		struct packed_git *p = *cache;
> -		off_t ofs;
> -		uint32_t candidate_mtime;
> +	for (source = r->objects->sources; source; source = source->next) {
> +		struct packed_git **cache = packfile_store_get_kept_pack_cache(source->packfiles, flags);
>  
> -		ofs = find_pack_entry_one(oid, p);
> -		if (!ofs)
> -			continue;
> +		for (; *cache; cache++) {
> +			struct packed_git *p = *cache;
> +			off_t ofs;
> +			uint32_t candidate_mtime;
>  
> -		/*
> -		 * We have a copy of the object 'oid' in a non-cruft
> -		 * pack. We can avoid packing an additional copy
> -		 * regardless of what the existing copy's mtime is since
> -		 * it is outside of a cruft pack.
> -		 */
> -		if (!p->is_cruft)
> -			return 0;
> -
> -		/*
> -		 * If we have a copy of the object 'oid' in a cruft
> -		 * pack, then either read the cruft pack's mtime for
> -		 * that object, or, if that can't be loaded, assume the
> -		 * pack's mtime itself.
> -		 */
> -		if (!load_pack_mtimes(p)) {
> -			uint32_t pos;
> -			if (offset_to_pack_pos(p, ofs, &pos) < 0)
> +			ofs = find_pack_entry_one(oid, p);
> +			if (!ofs)
>  				continue;
> -			candidate_mtime = nth_packed_mtime(p, pos);
> -		} else {
> -			candidate_mtime = p->mtime;
> -		}
>  
> -		/*
> -		 * We have a surviving copy of the object in a cruft
> -		 * pack whose mtime is greater than or equal to the one
> -		 * we are considering. We can thus avoid packing an
> -		 * additional copy of that object.
> -		 */
> -		if (mtime <= candidate_mtime)
> -			return 0;
> +			/*
> +			 * We have a copy of the object 'oid' in a non-cruft
> +			 * pack. We can avoid packing an additional copy
> +			 * regardless of what the existing copy's mtime is since
> +			 * it is outside of a cruft pack.
> +			 */
> +			if (!p->is_cruft)
> +				return 0;
> +
> +			/*
> +			 * If we have a copy of the object 'oid' in a cruft
> +			 * pack, then either read the cruft pack's mtime for
> +			 * that object, or, if that can't be loaded, assume the
> +			 * pack's mtime itself.
> +			 */
> +			if (!load_pack_mtimes(p)) {
> +				uint32_t pos;
> +				if (offset_to_pack_pos(p, ofs, &pos) < 0)
> +					continue;
> +				candidate_mtime = nth_packed_mtime(p, pos);
> +			} else {
> +				candidate_mtime = p->mtime;
> +			}
> +
> +			/*
> +			 * We have a surviving copy of the object in a cruft
> +			 * pack whose mtime is greater than or equal to the one
> +			 * we are considering. We can thus avoid packing an
> +			 * additional copy of that object.
> +			 */
> +			if (mtime <= candidate_mtime)
> +				return 0;
> +		}
Ok, this all looks the same, but repeated for each source.

Naive question: If a the same OID were to exist in multiple ODB sources, could this effect the behavior now that there are separate packfile stores?

Show 36 quoted lines
>  	}
>  
>  	return -1;
> @@ -1749,13 +1753,15 @@ static int want_object_in_pack_mtime(const struct object_id *oid,
>  		}
>  	}
>  
> -	for (e = the_repository->objects->packfiles->packs.head; e; e = e->next) {
> -		struct packed_git *p = e->pack;
> -		want = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);
> -		if (!exclude && want > 0)
> -			packfile_list_prepend(&the_repository->objects->packfiles->packs, p);
> -		if (want != -1)
> -			return want;
> +	for (source = the_repository->objects->sources; source; source = source->next) {
> +		for (e = source->packfiles->packs.head; e; e = e->next) {
> +			struct packed_git *p = e->pack;
> +			want = want_object_in_pack_one(p, oid, exclude, found_pack, found_offset, found_mtime);
> +			if (!exclude && want > 0)
> +				packfile_list_prepend(&source->packfiles->packs, p);
> +			if (want != -1)
> +				return want;
> +		}
>  	}
>  
>  	if (uri_protocols.nr) {
> diff --git a/http.c b/http.c
> index 41f850db16..7815f144de 100644
> --- a/http.c
> +++ b/http.c
> @@ -2544,7 +2544,7 @@ void http_install_packfile(struct packed_git *p,
>  			   struct packfile_list *list_to_remove_from)
>  {
>  	packfile_list_remove(list_to_remove_from, p);
> -	packfile_store_add_pack(the_repository->objects->packfiles, p);
> +	packfile_store_add_pack(the_repository->objects->sources->packfiles, p);

Here we are always adding the packfile to the primary packfile store. A thus relies of the primary source having a packfile store. In order to move to pluggable ODBs I suppose this is one of spots that will have to be resolved later.

>  }
>  
>  struct http_pack_request *new_http_pack_request(
[snip]
Show 10 quoted lines
> diff --git a/odb.h b/odb.h
> index 014cd9585a..c97b41c58c 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -51,6 +51,9 @@ struct odb_source {
>  	/* Private state for loose objects. */
>  	struct odb_source_loose *loose;
>  
> +	/* Should only be accessed directly by packfile.c and midx.c. */
> +	struct packfile_store *packfiles;
As mentioned, now the packfile store is moved to the ODB source.
-Justin
Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 12 of 52 in “Start tracking packfiles per object database source”
  1. 00/10 Start tracking packfiles per object database sourcePatrick Steinhardt, Dec 15, 2025
  2. 01/10 packfile: create store via its owning sourcePatrick Steinhardt, Dec 15, 2025
  3. Justin ToblerDec 15, 2025
  4. Patrick SteinhardtDec 16, 2025
  5. 02/10 packfile: pass source to `prepare_pack()`Patrick Steinhardt, Dec 15, 2025
  6. Justin ToblerDec 15, 2025
  7. 03/10 packfile: refactor kept-pack cache to work with packfile storesPatrick Steinhardt, Dec 15, 2025
  8. Justin ToblerDec 15, 2025
  9. Patrick SteinhardtDec 16, 2025
  10. 04/10 packfile: refactor misleading code when unusing pack windowsPatrick Steinhardt, Dec 15, 2025
  11. 05/10 packfile: move packfile store into object sourcePatrick Steinhardt, Dec 15, 2025
  12. Justin ToblerDec 18, 2025
  13. Patrick SteinhardtDec 18, 2025
  14. 06/10 packfile: only prepare owning store in `packfile_store_get_packs()`Patrick Steinhardt, Dec 15, 2025
  15. Justin ToblerDec 18, 2025
  16. 07/10 packfile: only prepare owning store in `packfile_store_prepare()`Patrick Steinhardt, Dec 15, 2025
  17. 08/10 packfile: inline `find_kept_pack_entry()`Patrick Steinhardt, Dec 15, 2025
  18. Justin ToblerDec 18, 2025
  19. Patrick SteinhardtDec 18, 2025
  20. 09/10 packfile: refactor `find_pack_entry()` to work on the packfile storePatrick Steinhardt, Dec 15, 2025
  21. 10/10 packfile: move MIDX into packfile storePatrick Steinhardt, Dec 15, 2025
  22. 00/10 Start tracking packfiles per object database sourcePatrick Steinhardt, Dec 18, 2025
  23. 01/10 packfile: create store via its owning sourcePatrick Steinhardt, Dec 18, 2025
  24. 02/10 packfile: pass source to `prepare_pack()`Patrick Steinhardt, Dec 18, 2025
  25. 03/10 packfile: refactor kept-pack cache to work with packfile storesPatrick Steinhardt, Dec 18, 2025
  26. Toon ClaesJan 6, 2026
  27. 04/10 packfile: refactor misleading code when unusing pack windowsPatrick Steinhardt, Dec 18, 2025
  28. Toon ClaesJan 7, 2026
  29. 05/10 packfile: move packfile store into object sourcePatrick Steinhardt, Dec 18, 2025
  30. Toon ClaesJan 7, 2026
  31. 06/10 packfile: only prepare owning store in `packfile_store_get_packs()`Patrick Steinhardt, Dec 18, 2025
  32. 07/10 packfile: only prepare owning store in `packfile_store_prepare()`Patrick Steinhardt, Dec 18, 2025
  33. Toon ClaesJan 7, 2026
  34. 08/10 packfile: inline `find_kept_pack_entry()`Patrick Steinhardt, Dec 18, 2025
  35. Kristoffer HaugsbakkJan 8, 2026
  36. Patrick SteinhardtJan 9, 2026
  37. 09/10 packfile: refactor `find_pack_entry()` to work on the packfile storePatrick Steinhardt, Dec 18, 2025
  38. 10/10 packfile: move MIDX into packfile storePatrick Steinhardt, Dec 18, 2025
  39. 00/10 Start tracking packfiles per object database sourcePatrick Steinhardt, Jan 9, 2026
  40. 01/10 packfile: create store via its owning sourcePatrick Steinhardt, Jan 9, 2026
  41. 02/10 packfile: pass source to `prepare_pack()`Patrick Steinhardt, Jan 9, 2026
  42. 03/10 packfile: refactor kept-pack cache to work with packfile storesPatrick Steinhardt, Jan 9, 2026
  43. 04/10 packfile: refactor misleading code when unusing pack windowsPatrick Steinhardt, Jan 9, 2026
  44. Karthik NayakJan 12, 2026
  45. 05/10 packfile: move packfile store into object sourcePatrick Steinhardt, Jan 9, 2026
  46. 06/10 packfile: only prepare owning store in `packfile_store_get_packs()`Patrick Steinhardt, Jan 9, 2026
  47. 07/10 packfile: only prepare owning store in `packfile_store_prepare()`Patrick Steinhardt, Jan 9, 2026
  48. 08/10 packfile: inline `find_kept_pack_entry()`Patrick Steinhardt, Jan 9, 2026
  49. 09/10 packfile: refactor `find_pack_entry()` to work on the packfile storePatrick Steinhardt, Jan 9, 2026
  50. 10/10 packfile: move MIDX into packfile storePatrick Steinhardt, Jan 9, 2026
  51. Junio C HamanoJan 11, 2026
  52. Justin ToblerJan 12, 2026

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.