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

From: Patrick Steinhardt <[email-masked]>
Date: Fri, 14 Jun 2024 08:49:40 +0200

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