| Message ID | cover.1718347699.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce `USE_THE_REPOSITORY_VARIABLE` macro | expand |
Hi Patrick On 14/06/2024 07:49, Patrick Steinhardt wrote: > Changes compared to v2: > > - Note in a commit message that we aim to have a faithful conversion > when introducing a `struct git_hash_algo` parameter to functions. So > even in case the calling context has a `struct git_hash_algo` > available via a local repository, we still use `the_repository` such > that there cannot be a change in behaviour here. Fixing those sites > will be left for a future patch series such that we can avoid any > kind of regressions caused by this comparatively large refactoring. > I also adapted some conversions to fully follow through with this > intent. Thanks for clarifying that in the commit message of Patch 2. Best Wishes Phillip
Patrick Steinhardt <ps@pks.im> writes: > Hi, > > this is the third version of my patch series that introduces the > `USE_THE_REPOSITORY_VARIABLE` macro. When unset, this will cause us to > hide `the_repository`, `the_hash_algo` and several other functions that > implicitly rely on those global variables from our headers. This is a > first step towards fully getting rid of this global state in favor of > passing it down explicitly via function parameters. > > Changes compared to v2: > > - Note in a commit message that we aim to have a faithful conversion > when introducing a `struct git_hash_algo` parameter to functions. So > even in case the calling context has a `struct git_hash_algo` > available via a local repository, we still use `the_repository` such > that there cannot be a change in behaviour here. Fixing those sites > will be left for a future patch series such that we can avoid any > kind of regressions caused by this comparatively large refactoring. > I also adapted some conversions to fully follow through with this > intent. > > - Fix an issue with sparse by adding another `extern` declaration of > `the_repository` to "repository.c". > > Thanks! > I forgot to reply here earlier. I went through the patches and only found 2 small typos, overall this was an elaborate set of patches. I don't expect a reroll for that however. Thanks!
Hi, this is the third version of my patch series that introduces the `USE_THE_REPOSITORY_VARIABLE` macro. When unset, this will cause us to hide `the_repository`, `the_hash_algo` and several other functions that implicitly rely on those global variables from our headers. This is a first step towards fully getting rid of this global state in favor of passing it down explicitly via function parameters. Changes compared to v2: - Note in a commit message that we aim to have a faithful conversion when introducing a `struct git_hash_algo` parameter to functions. So even in case the calling context has a `struct git_hash_algo` available via a local repository, we still use `the_repository` such that there cannot be a change in behaviour here. Fixing those sites will be left for a future patch series such that we can avoid any kind of regressions caused by this comparatively large refactoring. I also adapted some conversions to fully follow through with this intent. - Fix an issue with sparse by adding another `extern` declaration of `the_repository` to "repository.c". Thanks! Patrick Patrick Steinhardt (20): hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functions hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()` hash: require hash algorithm in `oidread()` and `oidclr()` global: ensure that object IDs are always padded hash: convert `oidcmp()` and `oideq()` to compare whole hash hash: make `is_null_oid()` independent of `the_repository` hash: require hash algorithm in `is_empty_{blob,tree}_oid()` hash: require hash algorithm in `empty_tree_oid_hex()` global: introduce `USE_THE_REPOSITORY_VARIABLE` macro refs: avoid include cycle with "repository.h" hash-ll: merge with "hash.h" http-fetch: don't crash when parsing packfile without a repo oidset: pass hash algorithm when parsing file protocol-caps: use hash algorithm from passed-in repository replace-object: use hash algorithm from passed-in repository compat/fsmonitor: fix socket path in networked SHA256 repos t/helper: use correct object hash in partial-clone helper t/helper: fix segfault in "oid-array" command without repository t/helper: remove dependency on `the_repository` in "proc-receive" hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE` add-interactive.c | 4 +- add-patch.c | 4 +- apply.c | 4 +- apply.h | 2 +- archive-tar.c | 3 + archive-zip.c | 3 + archive.c | 2 + attr.c | 2 + bisect.c | 2 + blame.c | 4 +- bloom.c | 1 + branch.c | 2 + builtin.h | 8 + builtin/am.c | 8 +- builtin/blame.c | 3 +- builtin/fast-export.c | 2 +- builtin/fast-import.c | 43 ++- builtin/fetch-pack.c | 4 +- builtin/index-pack.c | 11 +- builtin/log.c | 4 +- builtin/merge.c | 7 +- builtin/notes.c | 2 +- builtin/pack-objects.c | 3 +- builtin/pack-redundant.c | 10 +- builtin/patch-id.c | 6 +- builtin/pull.c | 6 +- builtin/receive-pack.c | 4 +- builtin/replace.c | 2 +- builtin/rm.c | 2 +- builtin/tag.c | 2 +- builtin/unpack-objects.c | 9 +- builtin/update-ref.c | 8 +- bulk-checkin.c | 3 + bundle-uri.c | 2 + bundle.c | 2 + cache-tree.c | 7 +- checkout.c | 2 + checkout.h | 2 +- chunk-format.c | 2 + chunk-format.h | 2 +- combine-diff.c | 2 + commit-graph.c | 22 +- commit-graph.h | 2 + commit-reach.c | 2 + commit.c | 2 + common-main.c | 2 + compat/fsmonitor/fsm-ipc-darwin.c | 7 +- compat/sha1-chunked.c | 2 +- compat/win32/trace2_win32_process_info.c | 2 + config.c | 3 + connected.c | 2 + convert.c | 2 + convert.h | 2 +- csum-file.c | 9 +- csum-file.h | 2 +- delta-islands.c | 2 + diagnose.c | 2 + diff-lib.c | 7 +- diff.c | 9 +- diff.h | 2 +- diffcore-break.c | 3 + diffcore-rename.c | 7 +- diffcore.h | 2 +- dir.c | 9 +- dir.h | 2 +- entry.c | 2 + environment.c | 3 + fetch-pack.c | 2 + fmt-merge-msg.c | 2 + fsck.c | 5 +- fsmonitor-ipc.c | 2 + git.c | 2 + hash-ll.h | 310 ---------------- hash-lookup.c | 5 +- hash.h | 366 ++++++++++++++++--- help.c | 2 + hex.c | 8 +- hex.h | 28 +- http-backend.c | 2 + http-fetch.c | 8 +- http-push.c | 5 +- http-walker.c | 6 +- http.c | 2 + list-objects-filter-options.c | 2 + list-objects-filter.c | 2 + list-objects.c | 2 + log-tree.c | 2 + loose.c | 2 + loose.h | 2 + ls-refs.c | 2 + mailmap.c | 2 + match-trees.c | 6 +- merge-blobs.c | 2 + merge-ort.c | 2 + merge-ort.h | 2 +- merge-recursive.c | 3 + merge.c | 2 + midx-write.c | 2 + midx.c | 5 +- negotiator/default.c | 2 + negotiator/skipping.c | 2 + notes-cache.c | 2 + notes-merge.c | 8 +- notes-utils.c | 2 + notes.c | 14 +- object-file-convert.c | 6 +- object-file.c | 19 +- object-name.c | 2 + object.c | 2 + object.h | 2 +- oid-array.c | 2 + oidmap.h | 2 +- oidset.c | 8 +- oidset.h | 4 +- oidtree.c | 4 +- oidtree.h | 2 +- oss-fuzz/fuzz-commit-graph.c | 2 + pack-bitmap-write.c | 6 +- pack-bitmap.c | 5 +- pack-check.c | 7 +- pack-revindex.c | 2 + pack-write.c | 5 +- packfile.c | 20 +- packfile.h | 2 + parallel-checkout.c | 8 +- parse-options-cb.c | 2 + path.c | 3 + pathspec.c | 2 + pretty.c | 2 + progress.c | 2 + promisor-remote.c | 2 + protocol-caps.c | 5 +- range-diff.c | 2 + reachable.c | 2 + read-cache-ll.h | 2 +- read-cache.c | 21 +- rebase-interactive.c | 2 + ref-filter.c | 2 + reflog-walk.c | 2 + reflog.c | 2 + refs.c | 8 +- refs.h | 8 +- refs/files-backend.c | 8 +- refs/packed-backend.c | 8 +- refs/ref-cache.h | 2 +- refs/reftable-backend.c | 39 +- refspec.c | 2 + reftable/dump.c | 2 +- reftable/reftable-record.h | 2 +- reftable/system.h | 2 +- remote-curl.c | 2 + remote.c | 10 +- remote.h | 2 +- replace-object.c | 2 +- repository.c | 8 + repository.h | 9 +- rerere.c | 2 + reset.c | 2 + reset.h | 2 +- resolve-undo.c | 5 +- resolve-undo.h | 2 +- revision.c | 2 + run-command.c | 2 + scalar.c | 2 + send-pack.c | 2 + sequencer.c | 8 +- serve.c | 4 +- server-info.c | 2 + setup.c | 2 + shallow.c | 2 + split-index.c | 4 +- split-index.h | 2 +- streaming.c | 3 + submodule-config.c | 4 +- submodule.c | 8 +- t/helper/test-bitmap.c | 2 + t/helper/test-bloom.c | 2 + t/helper/test-cache-tree.c | 2 + t/helper/test-dump-cache-tree.c | 2 + t/helper/test-dump-fsmonitor.c | 2 + t/helper/test-dump-split-index.c | 2 + t/helper/test-dump-untracked-cache.c | 2 + t/helper/test-find-pack.c | 2 + t/helper/test-fsmonitor-client.c | 2 + t/helper/test-hash-speed.c | 2 +- t/helper/test-lazy-init-name-hash.c | 2 + t/helper/test-match-trees.c | 2 + t/helper/test-oid-array.c | 4 + t/helper/test-oidmap.c | 2 + t/helper/test-pack-mtimes.c | 2 + t/helper/test-partial-clone.c | 2 +- t/helper/test-proc-receive.c | 9 +- t/helper/test-reach.c | 2 + t/helper/test-read-cache.c | 2 + t/helper/test-read-graph.c | 2 + t/helper/test-read-midx.c | 2 + t/helper/test-ref-store.c | 2 + t/helper/test-repository.c | 2 + t/helper/test-revision-walking.c | 2 + t/helper/test-scrap-cache-tree.c | 2 + t/helper/test-sha1.c | 2 +- t/helper/test-sha256.c | 2 +- t/helper/test-submodule-config.c | 4 +- t/helper/test-submodule-nested-repo-config.c | 2 + t/helper/test-submodule.c | 2 + t/helper/test-trace2.c | 2 + t/helper/test-write-cache.c | 2 + t/t0064-oid-array.sh | 18 + t/t5550-http-fetch-dumb.sh | 6 + t/test-lib-functions.sh | 5 + t/unit-tests/lib-oid.h | 2 +- t/unit-tests/t-example-decorate.c | 2 + tag.c | 2 + tmp-objdir.c | 2 + transport-helper.c | 2 + transport.c | 2 + tree-diff.c | 1 + tree-walk.c | 6 +- tree-walk.h | 2 +- tree.c | 2 + unpack-trees.c | 2 + upload-pack.c | 2 + walker.c | 2 + worktree.c | 2 + wt-status.c | 6 +- xdiff-interface.c | 2 + xdiff-interface.h | 2 +- 227 files changed, 1002 insertions(+), 617 deletions(-) delete mode 100644 hash-ll.h Range-diff against v2: 1: d2154e8c45 = 1: d2154e8c45 hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functions 2: aa468c3d88 ! 2: c481479598 hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()` @@ Commit message As those functions are now independent of `the_repository`, we can move them from "hash.h" to "hash-ll.h". + Note that both in this and subsequent commits in this series we always + just pass `the_repository->hash_algo` as input even if it is obvious + that there is a repository in the context that we should be using the + hash from instead. This is done to be on the safe side and not introduce + any regressions. All callsites should eventually be amended to use a + repo passed via parameters, but this is outside the scope of this patch + series. + Signed-off-by: Patrick Steinhardt <ps@pks.im> ## builtin/index-pack.c ## @@ pack-check.c: static int verify_packfile(struct repository *r, r->hash_algo->final_fn(hash, &ctx); pack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL); - if (!hasheq(hash, pack_sig)) -+ if (!hasheq(hash, pack_sig, r->hash_algo)) ++ if (!hasheq(hash, pack_sig, the_repository->hash_algo)) err = error("%s pack checksum mismatch", p->pack_name); - if (!hasheq(index_base + index_size - r->hash_algo->hexsz, pack_sig)) + if (!hasheq(index_base + index_size - r->hash_algo->hexsz, pack_sig, -+ r->hash_algo)) ++ the_repository->hash_algo)) err = error("%s pack checksum does not match its index", p->pack_name); unuse_pack(w_curs); 3: 403ea4485b ! 3: 226173a92b hash: require hash algorithm in `oidread()` and `oidclr()` @@ commit-graph.c: static struct tree *load_tree_for_commit(struct repository *r, st_mult(GRAPH_DATA_WIDTH, graph_pos - g->num_commits_in_base); - oidread(&oid, commit_data); -+ oidread(&oid, commit_data, r->hash_algo); ++ oidread(&oid, commit_data, the_repository->hash_algo); set_commit_tree(c, lookup_tree(r, &oid)); return c->maybe_tree; @@ commit-graph.c: int write_commit_graph(struct object_directory *odb, struct object_id oid; - oidread(&oid, g->chunk_oid_lookup + st_mult(g->hash_len, i)); + oidread(&oid, g->chunk_oid_lookup + st_mult(g->hash_len, i), -+ r->hash_algo); ++ the_repository->hash_algo); oid_array_append(&ctx->oids, &oid); } } @@ commit-graph.c: static int verify_one_commit_graph(struct repository *r, - oidread(&cur_oid, g->chunk_oid_lookup + st_mult(g->hash_len, i)); + oidread(&cur_oid, g->chunk_oid_lookup + st_mult(g->hash_len, i), -+ r->hash_algo); ++ the_repository->hash_algo); if (i && oidcmp(&prev_oid, &cur_oid) >= 0) graph_report(_("commit-graph has incorrect OID order: %s then %s"), 4: fa263d6b07 = 4: 3947c0c04d global: ensure that object IDs are always padded 5: a7df209bda = 5: 91eb94bc0b hash: convert `oidcmp()` and `oideq()` to compare whole hash 6: 9058837c93 = 6: 9ae29e1912 hash: make `is_null_oid()` independent of `the_repository` 7: d26584dc8f = 7: da48d5fdea hash: require hash algorithm in `is_empty_{blob,tree}_oid()` 8: 4858cca25f = 8: 1cf25bec3e hash: require hash algorithm in `empty_tree_oid_hex()` 9: cb3694ad0e ! 9: 7e023a335f global: introduce `USE_THE_REPOSITORY_VARIABLE` macro @@ remote.c #include "abspath.h" #include "config.h" + ## repository.c ## +@@ + #include "promisor-remote.h" + #include "refs.h" + ++/* ++ * We do not define `USE_THE_REPOSITORY_VARIABLE` in this file because we do ++ * not want to rely on functions that implicitly use `the_repository`. This ++ * means that the `extern` declaration of `the_repository` isn't visible here, ++ * which makes sparse unhappy. We thus declare it here. ++ */ ++extern struct repository *the_repository; ++ + /* The main repository */ + static struct repository the_repo; + struct repository *the_repository = &the_repo; + ## repository.h ## @@ repository.h: struct repository { unsigned different_commondir:1; 10: 4492548209 = 10: 74c88ebc39 refs: avoid include cycle with "repository.h" 11: f3cbc4b9f9 = 11: fe4550ba0c hash-ll: merge with "hash.h" 12: 9178098dd7 = 12: 30babd8a67 http-fetch: don't crash when parsing packfile without a repo 13: 0b4436c32b = 13: fa41a85c7e oidset: pass hash algorithm when parsing file 14: c7abfbc489 = 14: 0b42208e2f protocol-caps: use hash algorithm from passed-in repository 15: 9ae4fdb8f1 = 15: e1f4bffea1 replace-object: use hash algorithm from passed-in repository 16: 3ceb726655 = 16: 5b8f981ea2 compat/fsmonitor: fix socket path in networked SHA256 repos 17: 74e5489bd0 = 17: ad83b17ad0 t/helper: use correct object hash in partial-clone helper 18: 470aea1fc8 = 18: a488363bcb t/helper: fix segfault in "oid-array" command without repository 19: 1f0682fc7d = 19: 5de7a01af5 t/helper: remove dependency on `the_repository` in "proc-receive" 20: 16fb86c2b2 = 20: 436ffc0570 hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE`