git/bisect.c, branch v2.48.2

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

bisect: address Coverity warning about potential double free

2024-11-26T01:22:24Z

Coverity has started to warn about a potential double-free in `find_bisection()`. This warning is triggered because we may modify the list head of the passed-in `commit_list` in case it is an UNINTERESTING commit, but still call `free_commit_list()` on the original variable that points to the now-freed head in case where `do_find_bisection()` returns a `NULL` pointer. As far as I can see, this double free cannot happen in practice, as `do_find_bisection()` only returns a `NULL` pointer when it was passed a `NULL` input. So in order to trigger the double free we would have to call `find_bisection()` with a commit list that only consists of UNINTERESTING commits, but I have not been able to construct a case where that happens. Drop the `else` branch entirely as it seems to be a no-op anyway. Another option might be to instead call `free_commit_list()` on `list`, which is the modified version of `commit_list` and thus wouldn't cause a double free. But as mentioned, I couldn't come up with any case where a passed-in non-NULL list becomes empty, so this shouldn't be necessary. And if it ever does become necessary we'd notice anyway via the leak sanitizer. Interestingly enough we did not have a single test exercising this branch: all tests pass just fine even when replacing it with a call to `BUG()`. Add a test that exercises it. Reported-by: Jeff King Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

bisect: fix various cases where we leak commit list items

2024-11-20T23:23:42Z

There are various cases where we leak commit list items because we evict items from the list, but don't free them. Plug those. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

bisect: fix leaking commit list items in `check_merge_base()`

2024-11-20T23:23:41Z

While we free the result commit list at the end of `check_merge_base()`, we forget to free any items that we have already iterated over. Fix this by using a separate variable to iterate through them. This leak is exposed by t6030, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

bisect: fix multiple leaks in `bisect_next_all()`

2024-11-20T23:23:41Z

There are multiple leaks in `bisect_next_all()`. For one we don't free the `tried` commit list. Second, one of the branches uses a direct return instead of jumping to the cleanup code. Fix these by freeing the commit list and converting the return to a goto. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

bisect: fix leaking `current_bad_oid`

2024-11-20T23:23:41Z

When reading bisect refs we read the reference mapping to the "bad" term into the global `current_bad_oid` variable. This is an allocated string, but because it is global we never have to free it. This changes though when `register_ref()` is being called multiple times, at which point we'll overwrite the previous pointer and thus make it unreachable. Fix this issue by freeing the previous value. This leak is exposed by t6030, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

bisect: fix leaking string in `handle_bad_merge_base()`

2024-11-20T23:23:40Z

When handling a bad merge base we print an error, which includes the set of good revisions joined by spaces. This string is allocated, but never freed. Fix this memory leak. Note that the local `bad_hex` varible also looks like a string that we should free. But in fact, `oid_to_hex()` returns an address to a static variable even though it is declared to return a non-constant string. The function signature is thus quite misleading and really should be fixed, but doing so is outside of the scope of this patch series. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

bisect: fix leaking good/bad terms when reading multipe times

2024-11-20T23:23:40Z

Even though `read_bisect_terms()` is declared as assigning string constants, it in fact assigns allocated strings to the `read_bad` and `read_good` out parameters. The only callers of this function assign the result to global variables and thus don't have to free them in order to be leak-free. But that changes when executing the function multiple times because we'd then overwrite the previous value and thus make it unreachable. Fix the function signature and free the previous values. This leak is exposed by t0630, but plugging it does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano

Merge branch 'ps/pack-refs-auto-heuristics'

2024-09-12T18:47:23Z

"git pack-refs --auto" for the files backend was too aggressive, which has been a bit tamed. * ps/pack-refs-auto-heuristics: refs/files: use heuristic to decide whether to repack with `--auto` t0601: merge tests for auto-packing of refs wrapper: introduce `log2u()`

wrapper: introduce `log2u()`

2024-09-04T15:03:24Z

We have an implementation of a function that computes the log2 for an integer. While we could instead use log2(3P), that involves floating point numbers and is thus needlessly complex and inefficient. We're about to add a second caller that wants to compute log2 for a `size_t`. Let's thus move the function into "wrapper.h" such that it becomes generally available. While at it, tweak the implementation a bit: - The parameter is converted from `int` to `uintmax_t`. This conversion is safe to do in "bisect.c" because we already check that the argument is positive. - The return value is an `unsigned`. It cannot ever be negative, so it is pointless for it to be a signed integer. - Loop until `!n` instead of requiring that `n > 1` and then subtract 1 from the result and add a special case for `!sz`. This helps compilers to generate more efficient code. Compilers recognize the pattern of this function and optimize accordingly. On GCC 14.2 x86_64: log2u(unsigned long): test rdi, rdi je .L3 bsr rax, rdi ret .L3: mov eax, -1 ret Clang 18.1 does not yet recognize the pattern, but starts to do so on Clang trunk x86_64. The code isn't quite as efficient as the one generated by GCC, but still manages to optimize away the loop: log2u(unsigned long): test rdi, rdi je .LBB0_1 shr rdi bsr rcx, rdi mov eax, 127 cmovne rax, rcx xor eax, -64 add eax, 65 ret .LBB0_1: mov eax, -1 ret The pattern is also recognized on other platforms like ARM64 GCC 14.2.0, where we end up using `clz`: log2u(unsigned long): clz x2, x0 cmp x0, 0 mov w1, 63 sub w0, w1, w2 csinv w0, w0, wzr, ne ret Note that we have a similar function `fastlog2()` in the reftable code. As that codebase is separate from the Git codebase we do not adapt it to use the new function. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano