Subject: [PATCH v2 00/20] Introduce `USE_THE_REPOSITORY_VARIABLE` macro

From: Patrick Steinhardt <[email-masked]>
Date: Thu, 13 Jun 2024 08:13:21 +0200

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 <[email-masked]>
     
      ## 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 <[email-masked]>
     
      ## 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`
-- 
2.45.2.457.g8d94cfb545.dirty