git/pack-bitmap.c, branch v2.48.2

Merge branch 'tb/bitmap-fix-pack-reuse'

2024-12-23T17:32:22Z

Code to reuse objects based on bitmap contents have been tightened to avoid race condition even when multiple packs are involved. * tb/bitmap-fix-pack-reuse: pack-bitmap.c: ensure pack validity for all reuse packs

Merge branch 'ps/build-sign-compare'

2024-12-23T17:32:11Z

Start working to make the codebase buildable with -Wsign-compare. * ps/build-sign-compare: t/helper: don't depend on implicit wraparound scalar: address -Wsign-compare warnings builtin/patch-id: fix type of `get_one_patchid()` builtin/blame: fix type of `length` variable when emitting object ID gpg-interface: address -Wsign-comparison warnings daemon: fix type of `max_connections` daemon: fix loops that have mismatching integer types global: trivial conversions to fix `-Wsign-compare` warnings pkt-line: fix -Wsign-compare warning on 32 bit platform csum-file: fix -Wsign-compare warning on 32-bit platform diff.h: fix index used to loop through unsigned integer config.mak.dev: drop `-Wno-sign-compare` global: mark code units that generate warnings with `-Wsign-compare` compat/win32: fix -Wsign-compare warning in "wWinMain()" compat/regex: explicitly ignore "-Wsign-compare" warnings git-compat-util: introduce macros to disable "-Wsign-compare" warnings

pack-bitmap.c: ensure pack validity for all reuse packs

2024-12-18T17:51:09Z

Commit 44f9fd6496 (pack-bitmap.c: check preferred pack validity when opening MIDX bitmap, 2022-05-24) prevents a race condition whereby the preferred pack disappears between opening the MIDX bitmap and attempting verbatim reuse out of its packs. That commit forces open_midx_bitmap_1() to ensure the validity of the MIDX's preferred pack, meaning that we have an open file handle on the *.pack, ensuring that we can reuse bytes out of verbatim later on in the process[^1]. But 44f9fd6496 was not extended to cover multi-pack reuse, meaning that this same race condition exists for non-preferred packs during verbatim reuse. Work around that race in the same way by only marking valid packs as reuse-able. For packs that aren't reusable, skip over them but include the number of objects they have to ensure we allocate a large enough 'reuse' bitmap (e.g. if a pack in the middle of the MIDX disappeared but we still want to reuse later packs). Since we're ensuring the validity of these packs within the verbatim reuse code, we no longer have to special-case the preferred pack and open it within the open_midx_bitmap_1() function. An alternative approach to the one taken here would be to open all MIDX'd packs from within open_midx_bitmap_1(). But that would be both slower and make the bitmaps less useful, since we can still perform some pack reuse among the packs that still exist when the *.bitmap is opened. After applying this patch, we can simulate the new behavior after instrumenting Git like so: diff --git a/packfile.c b/packfile.c index 9560f0a33c..aedce72524 100644 --- a/packfile.c +++ b/packfile.c @@ -557,6 +557,11 @@ static int open_packed_git_1(struct packed_git *p) ; /* nothing */ p->pack_fd = git_open(p->pack_name); + { + const char *delete = getenv("GIT_RACILY_DELETE"); + if (delete && !strcmp(delete, pack_basename(p))) + return -1; + } if (p->pack_fd < 0 || fstat(p->pack_fd, &st)) return -1; pack_open_fds++; and adding the following test: test_expect_success 'disappearing packs' ' git init disappearing-packs && ( cd disappearing-packs && git config pack.allowPackReuse multi && test_commit A && test_commit B && test_commit C && A="$(echo "A" | git pack-objects --revs $packdir/pack-A)" && B="$(echo "A..B" | git pack-objects --revs $packdir/pack-B)" && C="$(echo "B..C" | git pack-objects --revs $packdir/pack-C)" && git multi-pack-index write --bitmap --preferred-pack=pack-A-$A.idx && test_pack_objects_reused_all 9 3 && test_env GIT_RACILY_DELETE=pack-A-$A.pack \ test_pack_objects_reused_all 6 2 && test_env GIT_RACILY_DELETE=pack-B-$B.pack \ test_pack_objects_reused_all 6 2 && test_env GIT_RACILY_DELETE=pack-C-$C.pack \ test_pack_objects_reused_all 6 2 ) ' Note that we could relax the single-pack version of this which was most recently addressed in dc1daacdcc (pack-bitmap: check pack validity when opening bitmap, 2021-07-23), but only partially. Because we still need to know the object count in the pack, we'd still have to open the pack's *.idx, so the savings there are marginal. Note likewise that we add a new "if (!packs_nr)" early return in the pack reuse code to avoid a potentially expensive allocation on the 'reuse' bitmap in the case that no packs are available for reuse. [^1]: Unless we run out of open file handles. If that happens and we are forced to close the only open file handle of a file that has been removed from underneath us, there is nothing we can do. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano

Merge branch 'kn/midx-wo-the-repository'

2024-12-13T15:33:44Z

Yet another "pass the repository through the callchain" topic. * kn/midx-wo-the-repository: midx: inline the `MIDX_MIN_SIZE` definition midx: pass down `hash_algo` to functions using global variables midx: pass `repository` to `load_multi_pack_index` midx: cleanup internal usage of `the_repository` and `the_hash_algo` midx-write: pass down repository to `write_midx_file[_only]` write-midx: add repository field to `write_midx_context` midx-write: use `revs->repo` inside `read_refs_snapshot` midx-write: pass down repository to static functions packfile.c: remove unnecessary prepare_packed_git() call midx: add repository to `multi_pack_index` struct config: make `packed_git_(limit|window_size)` non-global variables config: make `delta_base_cache_limit` a non-global variable packfile: pass down repository to `for_each_packed_object` packfile: pass down repository to `has_object[_kept]_pack` packfile: pass down repository to `odb_pack_name` packfile: pass `repository` to static function in the file packfile: use `repository` from `packed_git` directly packfile: add repository to struct `packed_git`

global: mark code units that generate warnings with `-Wsign-compare`

2024-12-06T11:20:02Z

Mark code units that generate warnings with `-Wsign-compare`. This allows for a structured approach to get rid of all such warnings over time in a way that can be easily measured. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

midx: pass down `hash_algo` to functions using global variables

2024-12-04T01:32:21Z

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 Signed-off-by: Junio C Hamano

Merge branch 'tb/boundary-traversal-fix'

2024-12-04T01:14:44Z

A trivial "correctness" fix that does not yet matter in practice. * tb/boundary-traversal-fix: pack-bitmap.c: typofix in `find_boundary_objects()`

midx: add repository to `multi_pack_index` struct

2024-12-03T23:21:55Z

The `multi_pack_index` struct represents the MIDX for a repository. Here, we add a pointer to the repository in this struct, allowing direct use of the repository variable without relying on the global `the_repository` struct. With this addition, we can determine the repository associated with a `bitmap_index` struct. A `bitmap_index` points to either a `packed_git` or a `multi_pack_index`, both of which have direct repository references. To support this, we introduce a static helper function, `bitmap_repo`, in `pack-bitmap.c`, which retrieves a repository given a `bitmap_index`. With this, we clear up all usages of `the_repository` within `pack-bitmap.c` and also remove the `USE_THE_REPOSITORY_VARIABLE` definition. Bringing us another step closer to remove all global variable usage. Although this change also opens up the potential to clean up `midx.c`, doing so would require additional refactoring to pass the repository struct to functions where the MIDX struct is created: a task better suited for future patches. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano

packfile: pass down repository to `has_object[_kept]_pack`

2024-12-03T23:21:54Z

The functions `has_object[_kept]_pack` currently rely on the global variable `the_repository`. To eliminate global variable usage in `packfile.c`, we should progressively shift the dependency on the_repository to higher layers. Let's remove its usage from these functions and any related ones. Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano

pack-bitmap.c: typofix in `find_boundary_objects()`

2024-11-21T23:57:18Z

In the boundary-based bitmap traversal, we use the given 'rev_info' structure to first do a commit-only walk in order to determine the boundary between interesting and uninteresting objects. That walk only looks at commit objects, regardless of the state of revs->blob_objects, revs->tree_objects, and so on. In order to do this, we store the state of these variables in temporary fields before setting them back to zero, performing the traversal, and then setting them back. But there is a typo here that dates back to b0afdce5da (pack-bitmap.c: use commit boundary during bitmap traversal, 2023-05-08), where we incorrectly store the value of the "tags" field as "revs->blob_objects". This could lead to problems later on if, say, the caller wants tag objects but *not* blob objects. In the pre-image behavior, we'd set revs->tag_objects back to the old value of revs->blob_objects, thus emitting fewer objects than expected back to the caller. Fix that by correctly assigning the value of 'revs->tag_objects' to the 'tmp_tags' field. Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano