mbox series

[v2,00/20] Introduce `USE_THE_REPOSITORY_VARIABLE` macro

Message ID cover.1718259125.git.ps@pks.im (mailing list archive)
Headers show
Series Introduce `USE_THE_REPOSITORY_VARIABLE` macro | expand

Message

Patrick Steinhardt June 13, 2024, 6:13 a.m. UTC
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`

Comments

Junio C Hamano June 13, 2024, 6:01 p.m. UTC | #1
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 June 13, 2024, 6:48 p.m. UTC | #2
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"
Ramsay Jones June 13, 2024, 11:14 p.m. UTC | #3
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
Junio C Hamano June 13, 2024, 11:18 p.m. UTC | #4
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.
Ramsay Jones June 13, 2024, 11:55 p.m. UTC | #5
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
Junio C Hamano June 14, 2024, 12:17 a.m. UTC | #6
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.
Patrick Steinhardt June 14, 2024, 5:28 a.m. UTC | #7
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
Junio C Hamano June 14, 2024, 3:54 p.m. UTC | #8
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.