git/negotiator, branch jch

use commit_stack instead of prio_queue in LIFO mode

2026-03-18T17:39:56Z

A prio_queue with a NULL compare function acts as a stack -- the last element in is the first one out (LIFO). Use an actual commit_stack instead where possible, as it documents the behavior better, provides type safety and saves some memory because prio_queue stores an additional tie-breaking counter per element. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano

refs: introduce wrapper struct for `each_ref_fn`

2025-11-04T15:32:24Z

The `each_ref_fn` callback function type is used across our code base for several different functions that iterate through reference. There's a bunch of callbacks implementing this type, which makes any changes to the callback signature extremely noisy. An example of the required churn is e8207717f1 (refs: add referent to each_ref_fn, 2024-08-09): adding a single argument required us to change 48 files. It was already proposed back then [1] that we might want to introduce a wrapper structure to alleviate the pain going forward. While this of course requires the same kind of global refactoring as just introducing a new parameter, it at least allows us to more change the callback type afterwards by just extending the wrapper structure. One counterargument to this refactoring is that it makes the structure more opaque. While it is obvious which callsites need to be fixed up when we change the function type, it's not obvious anymore once we use a structure. That being said, we only have a handful of sites that actually need to populate this wrapper structure: our ref backends, "refs/iterator.c" as well as very few sites that invoke the iterator callback functions directly. Introduce this wrapper structure so that we can adapt the iterator interfaces more readily. [1]: Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

prio-queue: use size_t rather than int for size

2024-12-20T15:21:45Z

The alloc and nr fields of a prio-queue tell us how much memory is allocated and used in the array. So the natural type for them is size_t, which prevents overflow on 64-bit systems where "int" is still 32 bits. This is unlikely to happen in practice, as we typically use it for storing commits, and having 2^31 of those is rather a lot. But it's good to keep our generic data structures as flexible as possible. And as we start to enforce -Wsign-compare, it means that callers need to use "int", too, and the problem proliferates. Let's fix it at the source. The changes here can be put into a few groups: 1. Changing the alloc/nr fields in the struct to size_t. This requires swapping out int for size_t in negotiator/skipping.c, as well as in prio_queue_get(), because those all iterate over the array. Building with -Wsign-compare complains about these. 2. Other code that assigns or passes around indexes into the array (e.g., the swap() and compare() functions) won't trigger -Wsign-compare because we are simply truncating the values. These are caught by -Wconversion, but I've adjusted them here to future-proof us. 3. In prio_queue_reverse() we compute "queue->nr - 1" without checking if anything is in the queue, which underflows now that nr is unsigned. We can fix that by returning early when the queue is empty (there is nothing to reverse). 4. The insertion_ctr variable is currently unsigned, but can likewise grow (it is actually worse, because adding and removing an element many times will keep increasing the counter, even though "nr" does not). I've bumped that to size_t here, as well. But -Wconversion notes that computing the "cmp" result by subtracting the counters and assigning to "int" is a potential problem. And that's true even before this patch, since we use an unsigned counter (imagine comparing "2^32-1" and "0", which should be a high positive value, but instead is "-1" as a signed int). Since we only care about the sign (and not the magnitude) of the result, we could fix this by swapping out the subtraction for a ternary comparison. Probably the performance impact would be negligible, since we just called into a custom compare function and branched on its result anyway. But it's easy enough to do a branchless version by subtracting the comparison results. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

Merge branch 'ps/leakfixes-part-6'

2024-09-20T18:16:30Z

More leakfixes. * ps/leakfixes-part-6: (22 commits) builtin/repack: fix leaking keep-pack list merge-ort: fix two leaks when handling directory rename modifications match-trees: fix leaking prefixes in `shift_tree()` builtin/fmt-merge-msg: fix leaking buffers builtin/grep: fix leaking object context builtin/pack-objects: plug leaking list of keep-packs builtin/repack: fix leaking line buffer when packing promisors negotiator/skipping: fix leaking commit entries shallow: fix leaking members of `struct shallow_info` shallow: free grafts when unregistering them object: clear grafts when clearing parsed object pool gpg-interface: fix misdesigned signing key interfaces send-pack: fix leaking push cert nonce remote: fix leak in reachability check of a remote-tracking ref remote: fix leaking tracking refs builtin/submodule--helper: fix leaking refs on push-check submodule: fix leaking fetch task data upload-pack: fix leaking child process data on reachability checks builtin/push: fix leaking refspec query result send-pack: fix leaking common object IDs ...

drop trailing newline from warning/error/die messages

2024-09-05T16:07:12Z

Our error reporting routines append a trailing newline, and the strings we pass to them should not include them (otherwise we get an extra blank line after the message). These cases were all found by looking at the results of: git grep -P '[^_](error|error_errno|warning|die|die_errno)\(.*\\n"[,)]' '*.c' Note that we _do_ sometimes include a newline in the middle of such messages, to create multiline output (hence our grep matching "," or ")" after we see the newline, so we know we're at the end of the string). It's possible that one or more of these cases could intentionally be including a blank line at the end, but having looked at them all manually, I think these are all just mistakes. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano

negotiator/skipping: fix leaking commit entries

2024-09-05T15:49:12Z

When releasing the skipping negotiator we free its priority queue, but not the contained entries. Fix this to plug a memory leak. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

refs: add referent to each_ref_fn

2024-08-09T15:47:34Z

Add a parameter to each_ref_fn so that callers to the ref APIs that use this function as a callback can have acess to the unresolved value of a symbolic ref. Signed-off-by: John Cai Signed-off-by: Junio C Hamano

global: introduce `USE_THE_REPOSITORY_VARIABLE` macro

2024-06-14T17:26:33Z

Use of the `the_repository` variable is deprecated nowadays, and we slowly but steadily convert the codebase to not use it anymore. Instead, callers should be passing down the repository to work on via parameters. It is hard though to prove that a given code unit does not use this variable anymore. The most trivial case, merely demonstrating that there is no direct use of `the_repository`, is already a bit of a pain during code reviews as the reviewer needs to manually verify claims made by the patch author. The bigger problem though is that we have many interfaces that implicitly rely on `the_repository`. Introduce a new `USE_THE_REPOSITORY_VARIABLE` macro that allows code units to opt into usage of `the_repository`. The intent of this macro is to demonstrate that a certain code unit does not use this variable anymore, and to keep it from new dependencies on it in future changes, be it explicit or implicit For now, the macro only guards `the_repository` itself as well as `the_hash_algo`. There are many more known interfaces where we have an implicit dependency on `the_repository`, but those are not guarded at the current point in time. Over time though, we should start to add guards as required (or even better, just remove them). Define the macro as required in our code units. As expected, most of our code still relies on the global variable. Nearly all of our builtins rely on the variable as there is no way yet to pass `the_repository` to their entry point. For now, declare the macro in "biultin.h" to keep the required changes at least a little bit more contained. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

cocci: apply rules to rewrite callers of "refs" interfaces

2024-05-07T17:06:59Z

Apply the rules that rewrite callers of "refs" interfaces to explicitly pass `struct ref_store`. The resulting patch has been applied with the `--whitespace=fix` option. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

treewide: remove unnecessary includes in source files

2023-12-26T20:04:31Z

Each of these were checked with gcc -E -I. ${SOURCE_FILE} | grep ${HEADER_FILE} to ensure that removing the direct inclusion of the header actually resulted in that header no longer being included at all (i.e. that no other header pulled it in transitively). ...except for a few cases where we verified that although the header was brought in transitively, nothing from it was directly used in that source file. These cases were: * builtin/credential-cache.c * builtin/pull.c * builtin/send-pack.c Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano