Hi,
this patch series contains a handful of cleanups to the object store
subsystem:
- A couple of definitions are moved out of "object-store.h" as they
belong to other subsystems.
- Some functions are dropped and/or renamed.
- The biggest part is the removal of `repo_has_object_file()`. This
function and its `_with_flags()` variant are marked as deprecated,
with the replacement being `has_object()`. The benefit of that
function is that it doesn't reload packfiles and doesn't fetch
promisor objects by default so that it becomes more explicit when
one really wants to do so.
These cleanups are in preparation for getting rid of `the_repository` in
"object-store.c".
The patch series is built on top of 4bbb303af69 (The seventh batch,
2025-04-17) with ps/object-file-cleanup at 68cd492a3e6 (object-store:
merge "object-store-ll.h" and "object-store.h", 2025-04-15) merged into
it.
Changes in v2:
- A handful of improvements for commit messages.
- Link to v1: https://lore.kernel.org/r/20250423-pks-object-store-cleanups-v1-0-81f8411a5d08@xxxxxx
Thanks!
Patrick
---
Patrick Steinhardt (13):
object-store: move `struct packed_git` into "packfile.h"
object-store: drop `loose_object_path()`
object-store: move and rename `odb_pack_keep()`
object-store: move function declarations to their respective subsystems
object-store: allow fetching objects via `has_object()`
treewide: trivial conversions of `repo_has_object_file()`
builtin/index-pack: don't fetch promised objects for collision check
builtin/show-ref: don't fetch objects when printing refs
refs: don't fetch promisor objects in `ref_resolves_to_object()`
http-walker: don't fetch objects via promisor remotes
list-objects: clarify how promised blobs are excluded
bulk-checkin: don't fetch promised objects on write
object-store: drop `repo_has_object_file()`
builtin/cat-file.c | 3 +-
builtin/clone.c | 4 +-
builtin/count-objects.c | 2 +-
builtin/fast-import.c | 3 +-
builtin/fetch.c | 15 ++--
builtin/gc.c | 2 +-
builtin/index-pack.c | 6 +-
builtin/receive-pack.c | 4 +-
builtin/remote.c | 3 +-
builtin/show-ref.c | 2 +-
builtin/unpack-objects.c | 3 +-
bulk-checkin.c | 2 +-
cache-tree.c | 13 +++-
fetch-pack.c | 7 +-
http-push.c | 11 ++-
http-walker.c | 7 +-
http.c | 4 +-
list-objects.c | 3 +-
notes.c | 3 +-
object-file.c | 4 +-
object-file.h | 77 +++++++++++++++++++
object-name.c | 2 +-
object-store.c | 44 ++---------
object-store.h | 191 +++--------------------------------------------
pack-objects.h | 1 +
packfile.h | 78 ++++++++++++++++++-
path.c | 14 ++++
path.h | 7 ++
prune-packed.c | 2 +-
reachable.c | 2 +-
reflog.c | 3 +-
refs.c | 2 +-
remote.c | 2 +-
send-pack.c | 5 +-
shallow.c | 9 ++-
upload-pack.c | 3 +-
walker.c | 3 +-
37 files changed, 265 insertions(+), 281 deletions(-)
Range-diff versus v1:
1: b4f8a00f4c4 = 1: 019c27227dc object-store: move `struct packed_git` into "packfile.h"
2: 8684c481949 = 2: b372e8214de object-store: drop `loose_object_path()`
3: f4f5127f44f ! 3: fa51af2ee24 object-store: move and rename `odb_pack_keep()`
@@ Commit message
The function `odb_pack_keep()` creates a file at the passed-in path. If
this fails, then the function re-tries by first creating any potentially
- missing leading directoriesk and then trying to create the file once
+ missing leading directories and then trying to create the file once
again. As such, this function doesn't host any kind of logic that is
specific to the object store, but is rather a generic helper function.
4: 9f94d3c4780 = 4: 04ad9a1b228 object-store: move function declarations to their respective subsystems
5: 0a187fe90db = 5: 3d45b334f4b object-store: allow fetching objects via `has_object()`
6: 4ebdf7510d2 = 6: 6101dfc393a treewide: trivial conversions of `repo_has_object_file()`
7: 739de6f8c67 ! 7: 35eca639ba4 builtin/index-pack: don't fetch promised objects for collision check
@@ Commit message
whether we already have an object with the same object ID, but different
contents in the repository.
- The check whether the collision check is really needed is performed via
- `repo_has_object_file_with_flags(..., OBJECT_INFO_QUICK)`. But unless
- explicitly told otherwise via `OBJECT_INFO_SKIP_FETCH_OBJECT`, this
- function will also cause us to fetch the object ID in case it is part of
- a promisor pack. As such, we may end up fetching the object only to
- check whether the fetched object and the object that we're indexing have
- the same content.
+ The check whether the collision check is really needed is only performed
+ in case `repo_has_object_file_with_flags(..., OBJECT_INFO_QUICK)` tells
+ us that the object exists. But unless explicitly told otherwise by
+ passing `OBJECT_INFO_SKIP_FETCH_OBJECT`, this function will also cause
+ us to fetch the object in case it is part of a promisor pack. As such,
+ we may end up fetching the object only to check whether the fetched
+ object and the object that we're indexing have the same content.
This behaviour is highly dubious and more likely than not unintended.
Fix it by converting to `has_object()`, which knows to neither reload
8: 0a79bfdbf14 = 8: 9975d86c59d builtin/show-ref: don't fetch objects when printing refs
9: 3a1ebffcdb0 ! 9: 41ad3e7ede2 refs: don't fetch promisor objects in `ref_resolves_to_object()`
@@ Commit message
Similar to the preceding commit, don't try to fetch objects pointed to
by references. Any reference whose object does not exist is broken by
- definition an, so we should report it accordingly.
+ definition, so we should report it accordingly.
Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
10: a21cfe3dc91 = 10: e7bb54ba5bd http-walker: don't fetch objects via promisor remotes
11: b939734e9e6 = 11: bef052ff785 list-objects: clarify how promised blobs are excluded
12: 8e8a5041af3 = 12: 45023c6e96f bulk-checkin: don't fetch promised objects on write
13: 68deca60383 = 13: c263ae0f4cf object-store: drop `repo_has_object_file()`
---
base-commit: ca819c0751cedd1713334882e4c83687f8478a54
change-id: 20250422-pks-object-store-cleanups-5a6077014155
