[v3,0/8] Change midx.c and midx-write.c to not use global variables

Message

Karthik Nayak Nov. 27, 2024, 4:28 p.m. UTC

Similar to the earlier patch series on cleaning up packfile.c and
removing usage of global variables [1], we change the midx.c and
midx-write.c files to no longer use global variables.

This is done by the following:
  - Usage of repository variable already available in existing structs.
  - Passing down repository variable from other subsystems.
  - Modifying all subcommands to obtain repository variable from the
  command in `builtins/` and passing down the variable from there.

The biggest change is in the first commit, wherein we modify all
subcommands to add the repository variable. Since the subcommand
definition are not often changed, it shouldn't cause too many conflicts
with in flight topics.

Since the `packfile.c` cleanup is still in flight, this series is based
on top of master: b31fb630c0 (Merge https://github.com/j6t/git-gui,
2024-11-11) with those patches merged in.

Since v3 this series also depends on
'kn/pass-repo-to-builtin-sub-sub-commands', which was split out from
this series.

[1]: https://lore.kernel.org/git/[e-mail]/

Signed-off-by: Karthik Nayak <[e-mail]>
---
Changes in v3:
- Split out the first commit into a separate series [1].
- Improved some of the commit messages to be more descriptive.
- Merged the 8th and 9th commits together, since they were similar.
- v2: https://lore.kernel.org/r/20241119-374-refactor-midx-c-[e-mail]

Changes in v2:
- Split commit modifying static functions in `midx-write.c` to multiple
commits, which makes it easier to review.
- Remove usage of `the_repository` in `test-parse-options.c` and use
NULL instead.
- Fix the commit messages to be imperative.
- Fix error in commit sign off.
- v1: https://lore.kernel.org/r/20241115-374-refactor-midx-c-[e-mail]

[1]: https://lore.kernel.org/git/xmqq34jdyey3.fsf@gitster.g/T/#t

---
Karthik Nayak (8):
      midx-write: pass down repository to static functions
      midx-write: use `revs->repo` inside `read_refs_snapshot`
      write-midx: add repository field to `write_midx_context`
      midx-write: pass down repository to `write_midx_file[_only]`
      midx: cleanup internal usage of `the_repository` and `the_hash_algo`
      midx: pass `repository` to `load_multi_pack_index`
      midx: pass down `hash_algo` to functions using global variables
      midx: inline the `MIDX_MIN_SIZE` definition

 builtin/multi-pack-index.c |   6 +--
 builtin/repack.c           |   2 +-
 midx-write.c               | 130 +++++++++++++++++++++++----------------------
 midx.c                     |  88 ++++++++++++++++--------------
 midx.h                     |  24 +++++----
 pack-bitmap.c              |   6 +--
 pack-revindex.c            |   2 +-
 t/helper/test-read-midx.c  |   8 +--
 8 files changed, 140 insertions(+), 126 deletions(-)
---

Range-diff versus v2:

 1:  6c00e25c86 <  -:  ---------- packfile: add repository to struct `packed_git`
 2:  70fc8a79af <  -:  ---------- packfile: use `repository` from `packed_git` directly
 3:  167a1f3a11 <  -:  ---------- packfile: pass `repository` to static function in the file
 4:  b7cfe78217 <  -:  ---------- packfile: pass down repository to `odb_pack_name`
 5:  5566f5554c <  -:  ---------- packfile: pass down repository to `has_object[_kept]_pack`
 6:  1b26e45a9b <  -:  ---------- packfile: pass down repository to `for_each_packed_object`
 7:  1bdc34f4d8 <  -:  ---------- config: make `delta_base_cache_limit` a non-global variable
 8:  7b6baa89ac <  -:  ---------- config: make `packed_git_(limit|window_size)` non-global variables
 9:  a3667d87ec <  -:  ---------- midx: add repository to `multi_pack_index` struct
10:  91a35c4876 <  -:  ---------- builtin: pass repository to sub commands
11:  a916fe708f !  1:  d7c1056ebd midx-write: pass down repository to static functions
    @@ Commit message
         access to this struct, pass down the required information from
         non-static functions `write_midx_file` and `write_midx_file_only`.
     
    +    This requires that the function `hash_to_hex` is also replaced with
    +    `hash_to_hex_algop` since the former internally accesses the
    +    `the_hash_algo` global variable.
    +
         This ensures that the usage of global variables is limited to these
    -    non-static functions, which will be cleaned up in a follow up commits.
    +    non-static functions, which will be cleaned up in a follow up commit.
     
         Signed-off-by: Karthik Nayak <[e-mail]>
     
12:  a2fa59eca5 !  2:  11cd6c6bec midx-write: use `revs->repo` inside `read_refs_snapshot`
    @@ Metadata
      ## Commit message ##
         midx-write: use `revs->repo` inside `read_refs_snapshot`
     
    -    The `read_refs_snapshot` uses the `parse_oid_hex` function which
    -    internally uses global variables. Let's instead use
    -    `parse_oid_hex_algop` and provide the hash algo via `revs->repo`.
    +    The function `read_refs_snapshot()` uses `parse_oid_hex()`, which relies
    +    on the global `the_hash_algo` variable. Let's instead use
    +    `parse_oid_hex_algop()` and provide the hash algo via `revs->repo`.
     
    -    Also, while here, fix a missing newline after the functions definition.
    +    Also, while here, fix a missing newline after the function's definition.
     
         Signed-off-by: Karthik Nayak <[e-mail]>
     
13:  bc6ff987c5 =  3:  d74bf1fa51 write-midx: add repository field to `write_midx_context`
14:  27affa11fb =  4:  630bffe661 midx-write: pass down repository to `write_midx_file[_only]`
15:  02484793f6 =  5:  32d001bdd6 midx: cleanup internal usage of `the_repository` and `the_hash_algo`
16:  790215da01 =  6:  392a96de6a midx: pass `repository` to `load_multi_pack_index`
17:  8eac01e7ac !  7:  35bdc6d6b1 midx: pass down `hash_algo` to `get_midx_filename[_ext]`
    @@ Metadata
     Author: Karthik Nayak <[e-mail]>
     
      ## Commit message ##
    -    midx: pass down `hash_algo` to `get_midx_filename[_ext]`
    +    midx: pass down `hash_algo` to functions using global variables
     
    -    The function `get_midx_filename_ext` uses `hash_to_hex` which internally
    -    uses the global variable `the_repository`. To remove this dependency,
    -    pass down the `hash_algo` to both `get_midx_filename` and
    -    `get_midx_filename_ext`. This adds `the_repository` variable usage to
    -    `midx-write.c`, which will be resolved in a future commit.
    +    The functions `get_split_midx_filename_ext()`, `get_midx_filename()` and
    +    `get_midx_filename_ext()` use `hash_to_hex()` which internally uses the
    +    `the_hash_algo` global variable.
    +
    +    Remove this dependency on global variables by passing down the
    +    `hash_algo` through to the functions mentioned and instead calling
    +    `hash_to_hex_algop()` along with the obtained `hash_algo`.
     
         Signed-off-by: Karthik Nayak <[e-mail]>
     
    @@ midx-write.c: static int link_midx_to_chain(struct multi_pack_index *m)
      
     -		get_midx_filename_ext(&from, m->object_dir, hash,
     -				      midx_exts[i].non_split);
    +-		get_split_midx_filename_ext(&to, m->object_dir, hash,
     +		get_midx_filename_ext(m->repo->hash_algo, &from, m->object_dir,
     +				      hash, midx_exts[i].non_split);
    - 		get_split_midx_filename_ext(&to, m->object_dir, hash,
    ++		get_split_midx_filename_ext(m->repo->hash_algo, &to,
    ++					    m->object_dir, hash,
      					    midx_exts[i].split);
      
    + 		if (link(from.buf, to.buf) < 0 && errno != ENOENT) {
     @@ midx-write.c: static int link_midx_to_chain(struct multi_pack_index *m)
      	return ret;
      }
    @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *o
      	if (safe_create_leading_directories(midx_name.buf))
      		die_errno(_("unable to create leading directories of %s"),
      			  midx_name.buf);
    +@@ midx-write.c: static int write_midx_internal(struct repository *r, const char *object_dir,
    + 		if (link_midx_to_chain(ctx.base_midx) < 0)
    + 			return -1;
    + 
    +-		get_split_midx_filename_ext(&final_midx_name, object_dir,
    +-					    midx_hash, MIDX_EXT_MIDX);
    ++		get_split_midx_filename_ext(r->hash_algo, &final_midx_name,
    ++					    object_dir, midx_hash, MIDX_EXT_MIDX);
    + 
    + 		if (rename_tempfile(&incr, final_midx_name.buf) < 0) {
    + 			error_errno(_("unable to rename new multi-pack-index layer"));
     @@ midx-write.c: static int write_midx_internal(struct repository *r, const char *object_dir,
      	if (commit_lock_file(&lk) < 0)
      		die_errno(_("could not write multi-pack-index"));
    @@ midx.c: const unsigned char *get_midx_checksum(struct multi_pack_index *m)
      }
      
      static int midx_read_oid_fanout(const unsigned char *chunk_start,
    +@@ midx.c: void get_midx_chain_filename(struct strbuf *buf, const char *object_dir)
    + 	strbuf_addstr(buf, "/multi-pack-index-chain");
    + }
    + 
    +-void get_split_midx_filename_ext(struct strbuf *buf, const char *object_dir,
    ++void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo,
    ++				 struct strbuf *buf, const char *object_dir,
    + 				 const unsigned char *hash, const char *ext)
    + {
    + 	get_midx_chain_dirname(buf, object_dir);
    +-	strbuf_addf(buf, "/multi-pack-index-%s.%s", hash_to_hex(hash), ext);
    ++	strbuf_addf(buf, "/multi-pack-index-%s.%s",
    ++		    hash_to_hex_algop(hash, hash_algo), ext);
    + }
    + 
    + static int open_multi_pack_index_chain(const struct git_hash_algo *hash_algo,
    +@@ midx.c: static struct multi_pack_index *load_midx_chain_fd_st(struct repository *r,
    + 		valid = 0;
    + 
    + 		strbuf_reset(&buf);
    +-		get_split_midx_filename_ext(&buf, object_dir, layer.hash,
    +-					    MIDX_EXT_MIDX);
    ++		get_split_midx_filename_ext(r->hash_algo, &buf, object_dir,
    ++					    layer.hash, MIDX_EXT_MIDX);
    + 		m = load_multi_pack_index_one(r, object_dir, buf.buf, local);
    + 
    + 		if (m) {
     @@ midx.c: struct multi_pack_index *load_multi_pack_index(struct repository *r,
      	struct strbuf midx_name = STRBUF_INIT;
      	struct multi_pack_index *m;
    @@ midx.h: struct multi_pack_index {
      			   const unsigned char *hash, const char *ext);
      void get_midx_chain_dirname(struct strbuf *buf, const char *object_dir);
      void get_midx_chain_filename(struct strbuf *buf, const char *object_dir);
    +-void get_split_midx_filename_ext(struct strbuf *buf, const char *object_dir,
    ++void get_split_midx_filename_ext(const struct git_hash_algo *hash_algo,
    ++				 struct strbuf *buf, const char *object_dir,
    + 				 const unsigned char *hash, const char *ext);
    + 
    + struct multi_pack_index *load_multi_pack_index(struct repository *r,
     
      ## pack-bitmap.c ##
     @@ pack-bitmap.c: static int load_bitmap_entries_v1(struct bitmap_index *index)
18:  3357d47ec0 <  -:  ---------- midx: pass down `hash_algo` to `get_split_midx_filename_ext`
19:  6a1ae9d11c !  8:  128f4f08b0 midx: inline the `MIDX_MIN_SIZE` definition
    @@ Commit message
         midx: inline the `MIDX_MIN_SIZE` definition
     
         The `MIDX_MIN_SIZE` definition is used to check the midx_size in
    -    `local_multi_pack_index_one`. This definitions relies on the
    +    `local_multi_pack_index_one`. This definition relies on the
         `the_hash_algo` global variable. Inline this and remove the global
         variable usage.
     


--- 

base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
change-id: 20241111-374-refactor-midx-c-and-midx-write-c-to-not-depend-on-global-state-a88498c2590f

Thanks
- Karthik