[00/20] packfile: avoid using the 'the_repository' global variable

Message

karthik nayak Oct. 21, 2024, 9:57 a.m. UTC

The `packfile.c` file uses the global variable 'the_repository' extensively
throughout the code. Let's remove all usecases of this, by modifying the
required functions to accept a 'struct repository' instead. This is to clean up
usage of global state.

The first 18 patches are mostly passing a `struct repository` to each of the
functions within `packfile.c` from other files. The last two patches move some
global config variables and make them local. I'm not too well versed with this
section of the code, so would be nice to get some eyes here.

Karthik Nayak (20):
  packfile: pass down repository to `odb_pack_name`
  packfile: pass down repository to `unuse_one_window`
  packfile: pass down repository to `close_one_pack`
  packfile: pass down repository to `add_packed_git`
  packfile: pass down repository to `unpack_object_header`
  packfile: pass down repository to `get_delta_base`
  packfile: use provided repository in `packed_object_info`
  packfile: pass down repository to `unpack_compressed_entry`
  packfile: pass down repository to `nth_packed_object_id`
  packfile: pass down repository to `find_pack_entry_one`
  packfile: pass down repository to `fill_pack_entry`
  packfile: pass down repository to `has_object[_kept]_pack`
  packfile: pass down repository to `for_each_packed_object`
  packfile: pass down repository to `is_promisor_object`
  object-store: pass down repository to `each_packed_object_fn`
  packfile: pass down repository to `open_pack_index`
  packfile: stop using 'the_hash_algo'
  packfile: pass down repository to `nth_packed_object_offset`
  config: make `delta_base_cache_limit` a non-global variable
  config: make `packed_git_(limit|window_size)` non-global variables

 builtin/cat-file.c          |  17 +-
 builtin/count-objects.c     |   4 +-
 builtin/fast-import.c       |  18 +-
 builtin/fsck.c              |  30 +--
 builtin/gc.c                |   5 +-
 builtin/index-pack.c        |  22 ++-
 builtin/pack-objects.c      |  67 ++++---
 builtin/pack-redundant.c    |   4 +-
 builtin/repack.c            |   9 +-
 builtin/rev-list.c          |   2 +-
 commit-graph.c              |  15 +-
 config.c                    |  22 ---
 connected.c                 |   7 +-
 diff.c                      |   3 +-
 environment.c               |   3 -
 environment.h               |   1 -
 fsck.c                      |   2 +-
 http-push.c                 |   5 +-
 http-walker.c               |   2 +-
 http.c                      |  15 +-
 list-objects.c              |   7 +-
 midx-write.c                |  16 +-
 midx.c                      |   8 +-
 object-name.c               |  16 +-
 object-store-ll.h           |  10 +-
 pack-bitmap.c               |  23 ++-
 pack-check.c                |  17 +-
 pack-mtimes.c               |   4 +-
 pack-mtimes.h               |   3 +-
 pack-objects.h              |   3 +-
 pack-revindex.c             |   7 +-
 pack-write.c                |   1 +
 pack.h                      |   1 +
 packfile.c                  | 376 +++++++++++++++++++++---------------
 packfile.h                  |  63 +++---
 promisor-remote.c           |   2 +-
 prune-packed.c              |   2 +-
 reachable.c                 |  14 +-
 revision.c                  |  15 +-
 streaming.c                 |   6 +-
 t/helper/test-find-pack.c   |   2 +-
 t/helper/test-pack-mtimes.c |   4 +-
 tag.c                       |   2 +-
 43 files changed, 482 insertions(+), 373 deletions(-)

Comments

Taylor Blau Oct. 21, 2024, 9:03 p.m. UTC | #1

On Mon, Oct 21, 2024 at 11:57:43AM +0200, Karthik Nayak wrote:
> The `packfile.c` file uses the global variable 'the_repository' extensively
> throughout the code. Let's remove all usecases of this, by modifying the
> required functions to accept a 'struct repository' instead. This is to clean up
> usage of global state.
>
> The first 18 patches are mostly passing a `struct repository` to each of the
> functions within `packfile.c` from other files. The last two patches move some
> global config variables and make them local. I'm not too well versed with this
> section of the code, so would be nice to get some eyes here.

I agree with the goal of this series, but I worry that as written it
will be quite disruptive to other topics on the list.

The standard way to avoid this disruption is to, for e.g. the first
change, do the following:

  - Introduce a new function repo_odb_pack_name() that takes in a
    'struct repository *', and rewrite odb_pack_name() in terms of it
    (passing 'the_repository' in as the argument).

  - Write a Coccinelle rule to replace all calls to odb_pack_name()
    with calls to repo_odb_pack_name().

  - Submit those patches without adjusting any non-obvious callers or
    ones that are not contained to a single compilation unit that you
    are already touching.

  - Wait until a new development cycle has begun, run spatch on the new
    rule to replace all other calls. Then optionally rename
    repo_odb_pack_name() to odb_pack_name().

I think Patrick (CC'd) has done one of these transitions recently, so
I'll defer to him in case I got any of the details wrong.

In the meantime, I'm going to hold this one out of seen as it may be
disruptive in the current state.

Thanks,
Taylor