| Message ID | cover.1713442061.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | global: drop `the_index` variable | expand |
Patrick Steinhardt <ps@pks.im> writes: > this is the second version of my patch series that aims to drop > `the_index`. > > Changes compared to v1: > > - This version goes a bit further now and completely drops the > static `the_index` variable, as well. The repository's index gets > allocated dynamically now, like all the other sub-structures like > the ODB. > > - This also allows this series to remove `initialize_the_repository()` > now. Instead, callers call `initialize_repository()` now. > > There is still quite an ugly hack in `initialize_repository()` which > requires us to treat `the_repository` specially. This is because > `the_hash_algo` maps to `the_repository->hash_algo`, and we rely on it > being initialized to SHA1. So we need call `repo_set_hash_algo()` on > `the_repository`. On the other hand, we cannot set the hash algo on > repos which are not `the_repository`, because that breaks stuff, as > well. > > I'm currently prepping another patch series that builds on top of this > series and cleans up this mess. It surfaces several bugs that got masked > by our setup, like for example `git rev-parse --short=` not working > correctly with SHA256 because we always truncate to the maximum length > of SHA1. I'll take a look, and may even comment on them later, but let me otherwise place this series on the back burner, not because I am in love with the_index (I am not) and not because I think the "index" member in the repository struct is a mistake (I suspect it is, but I am not convinced either way), but because I'd prefer to see our tree to be quiescent when we apply a tree-wide patch like [2/6], but we'll be in -rc period soonish, during which time we'd want to be able to concentrate on fixing regressions without having to worry about being able to reapply such tree-wide changes to keep 'next' and 'seen' building. Thanks.
On Thu, Apr 18, 2024 at 12:36:32PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > this is the second version of my patch series that aims to drop > > `the_index`. > > > > Changes compared to v1: > > > > - This version goes a bit further now and completely drops the > > static `the_index` variable, as well. The repository's index gets > > allocated dynamically now, like all the other sub-structures like > > the ODB. > > > > - This also allows this series to remove `initialize_the_repository()` > > now. Instead, callers call `initialize_repository()` now. > > > > There is still quite an ugly hack in `initialize_repository()` which > > requires us to treat `the_repository` specially. This is because > > `the_hash_algo` maps to `the_repository->hash_algo`, and we rely on it > > being initialized to SHA1. So we need call `repo_set_hash_algo()` on > > `the_repository`. On the other hand, we cannot set the hash algo on > > repos which are not `the_repository`, because that breaks stuff, as > > well. > > > > I'm currently prepping another patch series that builds on top of this > > series and cleans up this mess. It surfaces several bugs that got masked > > by our setup, like for example `git rev-parse --short=` not working > > correctly with SHA256 because we always truncate to the maximum length > > of SHA1. > > I'll take a look, and may even comment on them later, but let me > otherwise place this series on the back burner, not because I am in > love with the_index (I am not) and not because I think the "index" > member in the repository struct is a mistake (I suspect it is, but I > am not convinced either way), but because I'd prefer to see our tree > to be quiescent when we apply a tree-wide patch like [2/6], but > we'll be in -rc period soonish, during which time we'd want to be > able to concentrate on fixing regressions without having to worry > about being able to reapply such tree-wide changes to keep 'next' > and 'seen' building. Makes sense, thanks! Patrick
Hi, this is the second version of my patch series that aims to drop `the_index`. Changes compared to v1: - This version goes a bit further now and completely drops the static `the_index` variable, as well. The repository's index gets allocated dynamically now, like all the other sub-structures like the ODB. - This also allows this series to remove `initialize_the_repository()` now. Instead, callers call `initialize_repository()` now. There is still quite an ugly hack in `initialize_repository()` which requires us to treat `the_repository` specially. This is because `the_hash_algo` maps to `the_repository->hash_algo`, and we rely on it being initialized to SHA1. So we need call `repo_set_hash_algo()` on `the_repository`. On the other hand, we cannot set the hash algo on repos which are not `the_repository`, because that breaks stuff, as well. I'm currently prepping another patch series that builds on top of this series and cleans up this mess. It surfaces several bugs that got masked by our setup, like for example `git rev-parse --short=` not working correctly with SHA256 because we always truncate to the maximum length of SHA1. Patrick Patrick Steinhardt (6): t/helper: stop using `the_index` builtin: stop using `the_index` repository: initialize index in `repo_init()` builtin/clone: stop using `the_index` repository: drop `the_index` variable repository: drop `initialize_the_repository()` builtin/add.c | 48 +++++------ builtin/am.c | 36 ++++---- builtin/cat-file.c | 4 +- builtin/check-attr.c | 5 +- builtin/check-ignore.c | 7 +- builtin/checkout-index.c | 22 ++--- builtin/checkout.c | 87 ++++++++++--------- builtin/clean.c | 7 +- builtin/clone.c | 7 +- builtin/commit.c | 81 +++++++++--------- builtin/describe.c | 3 +- builtin/diff-tree.c | 3 +- builtin/diff.c | 6 +- builtin/difftool.c | 4 +- builtin/merge-index.c | 17 ++-- builtin/merge-tree.c | 3 +- builtin/merge.c | 31 ++++--- builtin/mv.c | 68 +++++++-------- builtin/pull.c | 4 +- builtin/read-tree.c | 15 ++-- builtin/rebase.c | 3 +- builtin/replay.c | 1 - builtin/reset.c | 32 +++---- builtin/rev-parse.c | 6 +- builtin/rm.c | 40 ++++----- builtin/stash.c | 45 +++++----- builtin/submodule--helper.c | 21 +++-- builtin/update-index.c | 122 +++++++++++++-------------- builtin/write-tree.c | 6 +- common-main.c | 2 +- oss-fuzz/fuzz-commit-graph.c | 3 +- repository.c | 50 ++++++----- repository.h | 5 +- t/helper/test-cache-tree.c | 17 ++-- t/helper/test-dump-cache-tree.c | 5 +- t/helper/test-dump-split-index.c | 11 ++- t/helper/test-dump-untracked-cache.c | 3 +- t/helper/test-lazy-init-name-hash.c | 39 +++++---- t/helper/test-read-cache.c | 11 ++- t/helper/test-scrap-cache-tree.c | 7 +- t/helper/test-write-cache.c | 3 +- 41 files changed, 435 insertions(+), 455 deletions(-) Range-diff against v1: 1: 7b5ec04efc = 1: 1baacc1fd8 t/helper: stop using `the_index` 2: 9d5f2eff8a ! 2: 371f69a2c9 builtin: stop using `the_index` @@ builtin/cat-file.c * Copyright (C) Linus Torvalds, 2005 */ -#define USE_THE_INDEX_VARIABLE ++ #include "builtin.h" #include "config.h" #include "convert.h" @@ builtin/commit.c: static const char *prepare_index(const char **argv, const char } else warning(_("Failed to update main cache tree")); @@ builtin/commit.c: static const char *prepare_index(const char **argv, const char *prefix, - add_files_to_cache(the_repository, also ? prefix : NULL, - &pathspec, 0, 0); + exit(128); + refresh_cache_or_die(refresh_flags); - cache_tree_update(&the_index, WRITE_TREE_SILENT); - if (write_locked_index(&the_index, &index_lock, 0)) 3: 96256f9eb3 = 3: 416612e7e3 repository: initialize index in `repo_init()` 4: 881b6d2f7d = 4: 6fb1492018 builtin/clone: stop using `the_index` 5: 80c36225bc < -: ---------- repository: drop global `the_index` variable -: ---------- > 5: 549f8c048f repository: drop `the_index` variable -: ---------- > 6: ed722b9b4b repository: drop `initialize_the_repository()`