| Message ID | cover.1718259125.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce `USE_THE_REPOSITORY_VARIABLE` macro | expand |
Patrick Steinhardt <ps@pks.im> writes: > this is the second version of my patch series that introduce a new > `USE_THE_REPOSITORY_VARIABLE` macro. If undefined, then declarations > like `the_repository`, `the_hash_algo` and a subset of functions that > implicitly depend on either of these are hidden away. ;-)
Junio C Hamano <gitster@pobox.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> this is the second version of my patch series that introduce a new >> `USE_THE_REPOSITORY_VARIABLE` macro. If undefined, then declarations >> like `the_repository`, `the_hash_algo` and a subset of functions that >> implicitly depend on either of these are hidden away. > > ;-) Two things. (1) This stupid change was needed to please "make sparse", or we'd get this: repository.c:21:19: error: symbol 'the_repository' was not declared. Should it be static? gmake: *** [Makefile:3259: repository.sp] Error 1 repository.c | 2 ++ 1 file changed, 2 insertions(+) diff --git c/repository.c w/repository.c index 95d10cc4a0..22ef85b0b3 100644 --- c/repository.c +++ w/repository.c @@ -18,6 +18,8 @@ /* The main repository */ static struct repository the_repo; + +extern struct repository *the_repository; struct repository *the_repository = &the_repo; /* (2) Aside from a few trivial and expected textual conflicts to be resolved, there were a few new files added that needed merge-fixes, with which I have this topic at the tip of the 'seen' branch. index-info.c | 2 ++ pseudo-merge.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/index-info.c b/index-info.c index 5d61e61e28..791380f910 100644 --- a/index-info.c +++ b/index-info.c @@ -1,3 +1,5 @@ +#define USE_THE_REPOSITORY_VARIABLE + #include "git-compat-util.h" #include "index-info.h" #include "hash.h" diff --git a/pseudo-merge.c b/pseudo-merge.c index e3e0393f11..f0fde13c47 100644 --- a/pseudo-merge.c +++ b/pseudo-merge.c @@ -1,3 +1,5 @@ +#define USE_THE_REPOSITORY_VARIABLE + #include "git-compat-util.h" #include "pseudo-merge.h" #include "date.h"
On 13/06/2024 19:48, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Patrick Steinhardt <ps@pks.im> writes: >> >>> this is the second version of my patch series that introduce a new >>> `USE_THE_REPOSITORY_VARIABLE` macro. If undefined, then declarations >>> like `the_repository`, `the_hash_algo` and a subset of functions that >>> implicitly depend on either of these are hidden away. >> >> ;-) > > Two things. > > (1) This stupid change was needed to please "make sparse", or we'd > get this: > > repository.c:21:19: error: symbol 'the_repository' was not declared. Should it be static? > gmake: *** [Makefile:3259: repository.sp] Error 1 > > repository.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git c/repository.c w/repository.c > index 95d10cc4a0..22ef85b0b3 100644 > --- c/repository.c > +++ w/repository.c > @@ -18,6 +18,8 @@ > > /* The main repository */ > static struct repository the_repo; > + > +extern struct repository *the_repository; > struct repository *the_repository = &the_repo; > Hmm, odd; isn't the declaration of 'the_repository' from the "repository.h" header file visible at this point? puzzled. ATB, Ramsay Jones
On Thu, Jun 13, 2024 at 4:15 PM Ramsay Jones <ramsay@ramsayjones.plus.com> wrote: > > Hmm, odd; isn't the declaration of 'the_repository' from > the "repository.h" header file visible at this point? No. The declaration is guarded with USE_THE_REPOSITORY_VARIABLE CPP macro in the header, and repository.c does not define it.
On 14/06/2024 00:18, Junio C Hamano wrote: > On Thu, Jun 13, 2024 at 4:15 PM Ramsay Jones > <ramsay@ramsayjones.plus.com> wrote: >> >> Hmm, odd; isn't the declaration of 'the_repository' from >> the "repository.h" header file visible at this point? > > No. The declaration is guarded with USE_THE_REPOSITORY_VARIABLE CPP macro > in the header, and repository.c does not define it. > Ah, OK. I haven't been following too closely and didn't notice that the declaration in the header file was now conditional. :( But that does beg the question - why is repository.c not defining the USE_THE_REPOSITORY_VARIABLE? ATB, Ramsay Jones
Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > On 14/06/2024 00:18, Junio C Hamano wrote: >> On Thu, Jun 13, 2024 at 4:15 PM Ramsay Jones >> <ramsay@ramsayjones.plus.com> wrote: >>> >>> Hmm, odd; isn't the declaration of 'the_repository' from >>> the "repository.h" header file visible at this point? >> >> No. The declaration is guarded with USE_THE_REPOSITORY_VARIABLE CPP macro >> in the header, and repository.c does not define it. >> > > Ah, OK. I haven't been following too closely and didn't > notice that the declaration in the header file was now > conditional. :( > > But that does beg the question - why is repository.c not > defining the USE_THE_REPOSITORY_VARIABLE? I think the goal of the series is to eventually get to the point where nobody uses the_repository variable. If repository.c, which consists of a set of service routines that work on a repository instance, defined it, showing willingness to implicitly rely on the_repository through things like get_oid_hex() (which would rely on the_repository->hash_algo), that would go the opposite direction, so everything, other than the definition of the_repository variable itself that allows other files that still do rely implicitly on the variable to link with it, in repository.c would actively want to refuse to use services only available to those who define USE_THE_* macro. It is a similar pattern we took when we weaned ourselves off of the_index compatibility macros. The read-cache.c was the central thing that defined the in-core index services, and it was the first thing that lost the_index compatibility macros like read_cache(), add_file_to_cache(), etc. that implicitly worked on the singleton the_index instance, while everybody else still relied on the singleton and services that implicitly worked on the singleton.
On Thu, Jun 13, 2024 at 05:17:08PM -0700, Junio C Hamano wrote: > Ramsay Jones <ramsay@ramsayjones.plus.com> writes: > > > On 14/06/2024 00:18, Junio C Hamano wrote: > >> On Thu, Jun 13, 2024 at 4:15 PM Ramsay Jones > >> <ramsay@ramsayjones.plus.com> wrote: > >>> > >>> Hmm, odd; isn't the declaration of 'the_repository' from > >>> the "repository.h" header file visible at this point? > >> > >> No. The declaration is guarded with USE_THE_REPOSITORY_VARIABLE CPP macro > >> in the header, and repository.c does not define it. > >> > > > > Ah, OK. I haven't been following too closely and didn't > > notice that the declaration in the header file was now > > conditional. :( > > > > But that does beg the question - why is repository.c not > > defining the USE_THE_REPOSITORY_VARIABLE? > > I think the goal of the series is to eventually get to the point > where nobody uses the_repository variable. If repository.c, which > consists of a set of service routines that work on a repository > instance, defined it, showing willingness to implicitly rely on > the_repository through things like get_oid_hex() (which would rely > on the_repository->hash_algo), that would go the opposite direction, > so everything, other than the definition of the_repository variable > itself that allows other files that still do rely implicitly on the > variable to link with it, in repository.c would actively want to > refuse to use services only available to those who define USE_THE_* > macro. Exactly, that's why it doesn't declare `USE_THE_REPOSITORY_VARIABLE`. The macro doesn't only guard use of `the_repository`, but does also guards other functions that implicitly relies on it, and we do not want to use these in "repository.c". So even though the added `extern` declaration is somewhat ugly, I think it is preferable over defining the macro. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> > But that does beg the question - why is repository.c not >> > defining the USE_THE_REPOSITORY_VARIABLE? >> >> I think the goal of the series is to eventually get to the point >> where nobody uses the_repository variable. If repository.c, which >> consists of a set of service routines that work on a repository >> instance, defined it, showing willingness to implicitly rely on >> the_repository through things like get_oid_hex() (which would rely >> on the_repository->hash_algo), that would go the opposite direction, >> so everything, other than the definition of the_repository variable >> itself that allows other files that still do rely implicitly on the >> variable to link with it, in repository.c would actively want to >> refuse to use services only available to those who define USE_THE_* >> macro. > > Exactly, that's why it doesn't declare `USE_THE_REPOSITORY_VARIABLE`. > The macro doesn't only guard use of `the_repository`, but does also > guards other functions that implicitly relies on it, and we do not want > to use these in "repository.c". So even though the added `extern` > declaration is somewhat ugly, I think it is preferable over defining the > macro. Slightly off-topic, but in retrospect, the approach of marking "this file is done" taken by the very initial version of the "no more implicit access to the_index" series may have been easier to grok easier to grok for reviewers casually looking at it from the sideline, than marking "this file still needs work". When we gradually deprecated the convenience API that implicitly access the_index instance, we marked with NO_THE_INDEX_COMPATIBILITY_MACROS the files that no longer require it (in other words, we are done with the file wrt the transition). We later flipped the polarity and gave USE_THE_INDEX_COMPATIBILITY_MACROS to those that are not marked with NO_THE_INDEX_COMPATIBILITY_MACROS as we progressed and the source files that still used the convenience API has become minorities. Anyway, I think now people understood what this series aims to achieve and how it does so, hopefully. Thanks.
Hi, this is the second version of my patch series that introduce a new `USE_THE_REPOSITORY_VARIABLE` macro. If undefined, then declarations like `the_repository`, `the_hash_algo` and a subset of functions that implicitly depend on either of these are hidden away. This is a step towards fully deprecating and getting rid of this global state. Changes compared to v1: - Pull in gt/unit-test-oidtree at ed54840872 (t/: migrate helper/test-oidtree.c to unit-tests/t-oidtree.c, 2024-06-08) as a new dependency. I mostly did it because it makes commit 19 redundant, so I've it. - As the base commit had to be rebuilt anyway I rebased on top of d63586cb31 (The thirteenth batch, 2024-06-12). - Rewrote the commit message of patch 16 to explain that this is actually a bug that is getting fixed. 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.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 +- 226 files changed, 994 insertions(+), 617 deletions(-) delete mode 100644 hash-ll.h Range-diff against v1: 1: f839013744 = 1: d2154e8c45 hash: drop (mostly) unused `is_empty_{blob,tree}_sha1()` functions 2: 687ad9fc02 = 2: aa468c3d88 hash: require hash algorithm in `hasheq()`, `hashcmp()` and `hashclr()` 3: 346ca76a7c = 3: 403ea4485b hash: require hash algorithm in `oidread()` and `oidclr()` 4: 3ff28f313b = 4: fa263d6b07 global: ensure that object IDs are always padded 5: e2a0f2125d = 5: a7df209bda hash: convert `oidcmp()` and `oideq()` to compare whole hash 6: 3b6ce3b26c = 6: 9058837c93 hash: make `is_null_oid()` independent of `the_repository` 7: 82a391acac = 7: d26584dc8f hash: require hash algorithm in `is_empty_{blob,tree}_oid()` 8: 3f091dd94a = 8: 4858cca25f hash: require hash algorithm in `empty_tree_oid_hex()` 9: 479bebdf53 ! 9: cb3694ad0e global: introduce `USE_THE_REPOSITORY_VARIABLE` macro @@ t/helper/test-dump-untracked-cache.c #include "dir.h" #include "hex.h" - ## t/helper/test-example-decorate.c ## -@@ -+#define USE_THE_REPOSITORY_VARIABLE -+ - #include "test-tool.h" - #include "git-compat-util.h" - #include "object.h" - ## t/helper/test-find-pack.c ## @@ +#define USE_THE_REPOSITORY_VARIABLE @@ t/helper/test-write-cache.c #include "lockfile.h" #include "read-cache-ll.h" + ## t/unit-tests/t-example-decorate.c ## +@@ ++#define USE_THE_REPOSITORY_VARIABLE ++ + #include "test-lib.h" + #include "object.h" + #include "decorate.h" + ## tag.c ## @@ +#define USE_THE_REPOSITORY_VARIABLE 10: 7e718c967a = 10: 4492548209 refs: avoid include cycle with "repository.h" 11: fb7544181a ! 11: f3cbc4b9f9 hash-ll: merge with "hash.h" @@ t/helper/test-sha256.c int cmd__sha256(int ac, const char **av) { + ## t/unit-tests/lib-oid.h ## +@@ + #ifndef LIB_OID_H + #define LIB_OID_H + +-#include "hash-ll.h" ++#include "hash.h" + + /* + * Convert arbitrary hex string to object_id. + ## tree-diff.c ## @@ #include "tree.h" 12: b47fa99f3d = 12: 9178098dd7 http-fetch: don't crash when parsing packfile without a repo 13: 95d9b5fa3e = 13: 0b4436c32b oidset: pass hash algorithm when parsing file 14: c877d48f7d = 14: c7abfbc489 protocol-caps: use hash algorithm from passed-in repository 15: ca5f4056fb = 15: 9ae4fdb8f1 replace-object: use hash algorithm from passed-in repository 16: d4e87f9d6b ! 16: 3ceb726655 compat/fsmonitor: remove dependency on `the_repository` in Darwin IPC @@ Metadata Author: Patrick Steinhardt <ps@pks.im> ## Commit message ## - compat/fsmonitor: remove dependency on `the_repository` in Darwin IPC + compat/fsmonitor: fix socket path in networked SHA256 repos The IPC socket used by the fsmonitor on Darwin is usually contained in the Git repository itself. When the repository is hosted on a networked @@ Commit message directory or the socket directory. In that case, we derive the path by hashing the repository path. - The hashing implicitly depends on `the_repository` though via - `hash_to_hex()`. For one, this is wrong because we really should be - using the passed-in repository. But arguably, it is not sensible to - derive the path hash from the repository's object hash in the first - place -- they don't have anything to do with each other, and a - repository that is hosted in the same path should always derive the same - IPC socket path. + But while we always use SHA1 to hash the repository path, we then end up + using `hash_to_hex()` to append the computed hash to the socket path. + This is wrong because `hash_to_hex()` uses the hash algorithm configured + in `the_repository`, which may not be SHA1. The consequence is that we + may append uninitialized bytes to the path when operating in a SHA256 + repository. - Fix this by unconditionally using SHA1 to derive the path. + Fix this bug by using `hash_to_hex_algop()` with SHA1. Signed-off-by: Patrick Steinhardt <ps@pks.im> ## compat/fsmonitor/fsm-ipc-darwin.c ## +@@ compat/fsmonitor/fsm-ipc-darwin.c: const char *fsmonitor_ipc__get_path(struct repository *r) + git_SHA_CTX sha1ctx; + char *sock_dir = NULL; + struct strbuf ipc_file = STRBUF_INIT; +- unsigned char hash[GIT_MAX_RAWSZ]; ++ unsigned char hash[GIT_SHA1_RAWSZ]; + + if (!r) + BUG("No repository passed into fsmonitor_ipc__get_path"); @@ compat/fsmonitor/fsm-ipc-darwin.c: const char *fsmonitor_ipc__get_path(struct repository *r) /* Create the socket file in either socketDir or $HOME */ if (sock_dir && *sock_dir) { 17: 5310883469 = 17: 74e5489bd0 t/helper: use correct object hash in partial-clone helper 18: 2774b8500f = 18: 470aea1fc8 t/helper: fix segfault in "oid-array" command without repository 19: 339d668da8 < -: ---------- t/helper: remove dependency on `the_repository` in "oidtree" 20: 97fa3051fa = 19: 1f0682fc7d t/helper: remove dependency on `the_repository` in "proc-receive" 21: 92c7f542d2 = 20: 16fb86c2b2 hex: guard declarations with `USE_THE_REPOSITORY_VARIABLE`