| Message ID | 20250307-b4-pks-objects-without-the-repository-v3-0-7bccf408731e@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Stop depending on `the_repository` in object-related subsystems | expand |
On Fri, Mar 7, 2025 at 6:19 AM Patrick Steinhardt <ps@pks.im> wrote: > > Hi, > > this patch series is another step to remove our dependency on the global > `the_repository` variable. The series focusses on subsystems related to > objects. > > The intent here is to work towards libification of the whole subsystem > so that we can start splitting out something like an object "backend". > It is thus part of a set of refactorings aimed at allowing pluggable > object databases eventually. I'm not discussing that bigger effort yet, > mostly because it's still taking shape. So these patch series contains > things that make sense standalone, even if pluggable ODBs never get to > be a thing. Sounds like a good goal; and the series is helpful regardless of whether we get there. > Note that this patch series stop short of dropping `the_repository` in > "object-file.c". This is a bigger undertaking, so I'm pushing that into > the next patch series. > > The series is built on top of cb0ae672aea (A bit more post -rc0, > 2025-02-27) with ps/path-sans-the-repository at 028f618658e (path: > adjust last remaining users of `the_repository`, 2025-02-07) merged into > it. > > Changes in v2: > - Point out why t1050 had to be adapted. > - Drop the rename of `get_max_object_index()` and > `get_indexed_object()`. > - Fix a couple of commit message typos. > - Link to v1: https://lore.kernel.org/r/20250303-b4-pks-objects-without-the-repository-v1-0-c5dd43f2476e@pks.im > > Changes in v3: > - Fix a semantic conflict with an in-flight patch series. > - Link to v2: https://lore.kernel.org/r/20250306-b4-pks-objects-without-the-repository-v2-0-f3465327be69@pks.im > > Thanks! > > Patrick > > --- > Patrick Steinhardt (12): > csum-file: stop depending on `the_repository` > object: stop depending on `the_repository` > pack-write: stop depending on `the_repository` and `the_hash_algo` > environment: move access to "core.bigFileThreshold" into repo settings > pack-check: stop depending on `the_repository` > pack-revindex: stop depending on `the_repository` > pack-bitmap-write: stop depending on `the_repository` > object-file-convert: stop depending on `the_repository` > delta-islands: stop depending on `the_repository` > object-file: split out logic regarding hash algorithms > hash: fix "-Wsign-compare" warnings > hash: stop depending on `the_repository` in `null_oid()` I read through the series. I had a couple small comments on patch 12. Overall, I really like how you've divided things up; it makes it easy for review, especially combined with --color-diff.
Hi, this patch series is another step to remove our dependency on the global `the_repository` variable. The series focusses on subsystems related to objects. The intent here is to work towards libification of the whole subsystem so that we can start splitting out something like an object "backend". It is thus part of a set of refactorings aimed at allowing pluggable object databases eventually. I'm not discussing that bigger effort yet, mostly because it's still taking shape. So these patch series contains things that make sense standalone, even if pluggable ODBs never get to be a thing. Note that this patch series stop short of dropping `the_repository` in "object-file.c". This is a bigger undertaking, so I'm pushing that into the next patch series. The series is built on top of cb0ae672aea (A bit more post -rc0, 2025-02-27) with ps/path-sans-the-repository at 028f618658e (path: adjust last remaining users of `the_repository`, 2025-02-07) merged into it. Changes in v2: - Point out why t1050 had to be adapted. - Drop the rename of `get_max_object_index()` and `get_indexed_object()`. - Fix a couple of commit message typos. - Link to v1: https://lore.kernel.org/r/20250303-b4-pks-objects-without-the-repository-v1-0-c5dd43f2476e@pks.im Changes in v3: - Fix a semantic conflict with an in-flight patch series. - Link to v2: https://lore.kernel.org/r/20250306-b4-pks-objects-without-the-repository-v2-0-f3465327be69@pks.im Thanks! Patrick --- Patrick Steinhardt (12): csum-file: stop depending on `the_repository` object: stop depending on `the_repository` pack-write: stop depending on `the_repository` and `the_hash_algo` environment: move access to "core.bigFileThreshold" into repo settings pack-check: stop depending on `the_repository` pack-revindex: stop depending on `the_repository` pack-bitmap-write: stop depending on `the_repository` object-file-convert: stop depending on `the_repository` delta-islands: stop depending on `the_repository` object-file: split out logic regarding hash algorithms hash: fix "-Wsign-compare" warnings hash: stop depending on `the_repository` in `null_oid()` Makefile | 1 + archive.c | 4 +- blame.c | 2 +- branch.c | 2 +- builtin/checkout.c | 6 +- builtin/clone.c | 2 +- builtin/describe.c | 2 +- builtin/diff.c | 5 +- builtin/fast-export.c | 10 +- builtin/fast-import.c | 8 +- builtin/fsck.c | 6 +- builtin/grep.c | 4 +- builtin/index-pack.c | 16 +- builtin/log.c | 2 +- builtin/ls-files.c | 3 +- builtin/name-rev.c | 4 +- builtin/pack-objects.c | 17 +- builtin/prune.c | 2 +- builtin/rebase.c | 2 +- builtin/receive-pack.c | 2 +- builtin/submodule--helper.c | 36 ++-- builtin/tag.c | 2 +- builtin/unpack-objects.c | 5 +- builtin/update-ref.c | 2 +- builtin/worktree.c | 2 +- bulk-checkin.c | 4 +- combine-diff.c | 2 +- commit-graph.c | 9 +- commit.c | 2 +- config.c | 5 - csum-file.c | 28 +-- csum-file.h | 12 +- delta-islands.c | 14 +- delta-islands.h | 2 +- diff-lib.c | 10 +- diff-no-index.c | 28 +-- diff.c | 14 +- diff.h | 2 +- dir.c | 2 +- environment.c | 1 - environment.h | 1 - grep.c | 2 +- hash.c | 277 +++++++++++++++++++++++++ hash.h | 4 +- log-tree.c | 2 +- merge-ort.c | 26 +-- merge-recursive.c | 12 +- meson.build | 1 + midx-write.c | 12 +- midx.c | 3 +- notes-merge.c | 2 +- notes.c | 2 +- object-file-convert.c | 29 +-- object-file-convert.h | 3 +- object-file.c | 292 +-------------------------- object.c | 21 +- object.h | 10 +- pack-bitmap-write.c | 36 ++-- pack-bitmap.c | 15 +- pack-bitmap.h | 1 + pack-check.c | 12 +- pack-revindex.c | 35 ++-- pack-write.c | 55 +++-- pack.h | 11 +- parse-options-cb.c | 2 +- range-diff.c | 2 +- reachable.c | 6 +- read-cache.c | 4 +- refs.c | 12 +- refs/debug.c | 2 +- refs/files-backend.c | 2 +- repo-settings.c | 20 ++ repo-settings.h | 5 + reset.c | 2 +- revision.c | 3 +- sequencer.c | 10 +- shallow.c | 10 +- streaming.c | 3 +- submodule-config.c | 2 +- submodule.c | 28 +-- t/helper/test-ref-store.c | 2 +- t/helper/test-submodule-nested-repo-config.c | 2 +- t/t1050-large.sh | 3 +- tree-diff.c | 4 +- upload-pack.c | 14 +- wt-status.c | 4 +- xdiff-interface.c | 2 +- 87 files changed, 677 insertions(+), 613 deletions(-) Range-diff versus v2: 1: 94dfdecf59d = 1: 2b8ac989731 csum-file: stop depending on `the_repository` 2: c456c1a057b = 2: f70f85a194b object: stop depending on `the_repository` 3: 4eb651128e2 = 3: af03d83d7f0 pack-write: stop depending on `the_repository` and `the_hash_algo` 4: 0632bc5c186 = 4: 19a8ee01385 environment: move access to "core.bigFileThreshold" into repo settings 5: 86214482421 = 5: ec9b0012242 pack-check: stop depending on `the_repository` 6: 3fda249b16a = 6: f15148e3aee pack-revindex: stop depending on `the_repository` 7: 99dbafed649 = 7: ce1255da4aa pack-bitmap-write: stop depending on `the_repository` 8: a3843270ce0 = 8: 2f6d7b93dc9 object-file-convert: stop depending on `the_repository` 9: 8d141a24804 = 9: 28e2dac294d delta-islands: stop depending on `the_repository` 10: 4671a252952 = 10: 813da442eee object-file: split out logic regarding hash algorithms 11: 2b81a7d91fd = 11: 7432ecb550b hash: fix "-Wsign-compare" warnings 12: 6e8f5f8987b ! 12: 8d88ef2aab7 hash: stop depending on `the_repository` in `null_oid()` @@ Commit message - "grep.c" - "refs/debug.c" - There is also a single non-trivial exception with "diff-no-index.c". - Here we know that we may not have a repository initialized at all, so we - cannot rely on `the_repository`. Instead, we adapt `diff_no_index()` to - get a `struct git_hash_algo` as parameter. The only caller is located in - "builtin/diff.c", where we know to call `repo_set_hash_algo()` in case - we're running outside of a Git repository. Consequently, it is fine to - continue passing `the_repository->hash_algo` even in this case. + There are also two non-trivial exceptions: + + - "diff-no-index.c": Here we know that we may not have a repository + initialized at all, so we cannot rely on `the_repository`. Instead, + we adapt `diff_no_index()` to get a `struct git_hash_algo` as + parameter. The only caller is located in "builtin/diff.c", where we + know to call `repo_set_hash_algo()` in case we're running outside of + a Git repository. Consequently, it is fine to continue passing + `the_repository->hash_algo` even in this case. + + - "builtin/ls-files.c": There is an in-flight patch series that drops + `USE_THE_REPOSITORY_VARIABLE` in this file, which causes a semantic + conflict because we use `null_oid()` in `show_submodule()`. The + value is passed to `repo_submodule_init()`, which may use the object + ID to resolve a tree-ish in the superproject from which we want to + read the submodule config. As such, the object ID should refer to an + object in the superproject, and consequently we need to use its hash + algorithm. This means that we could in theory just not bother about this edge case at all and just use `the_repository` in "diff-no-index.c". But doing so @@ builtin/ls-files.c: static void show_submodule(struct repository *superproject, struct repository subrepo; - if (repo_submodule_init(&subrepo, superproject, path, null_oid())) -+ if (repo_submodule_init(&subrepo, superproject, path, null_oid(the_hash_algo))) ++ if (repo_submodule_init(&subrepo, superproject, path, ++ null_oid(superproject->hash_algo))) return; if (repo_read_index(&subrepo) < 0) --- base-commit: e2cb568e11f4ceb427ba4205e6b8a4426d26be12 change-id: 20250210-b4-pks-objects-without-the-repository-6ba8398f7cc0