On Mon, Aug 04, 2025 at 10:17:20AM +0200, Patrick Steinhardt wrote:
> The "commit-graph.c" file has a bunch of sign comparison warnings:
>
> - There are a bunch of variables that are declared as signed integers
> even though they are used to count entities, like for example
> `num_commit_graphs_before` and `num_commit_graphs_after`.
I have similar thoughts as in the previous patch about this spot, too.
> - There are several cases where we use signed loop variables to
> iterate through an unsigned entity count.
Here I am more sympathetic to using a size_t to count the number of
allocated elements in some array/buffer.
> - The bloom settings hash version is being assigned `-1` even though
> it's an unsigned value. This is used to indicate an unspecified
> value and relies on 1's complement.
OK. I think that comparing "-1" to an unsigned value is meant equivalent
to saying "are all 32 of these bits set"? But making it explicit seems
reasonable, since it's not immediately clear from reading this function
that 'hash_version' is unsigned.
> @@ -622,7 +621,7 @@ int open_commit_graph_chain(const char *chain_file,
> close(*fd);
> return 0;
> }
> - if (st->st_size < the_hash_algo->hexsz) {
> + if (st->st_size < (ssize_t) the_hash_algo->hexsz) {
I understand why the compiler is telling you to make hexsz a signed
quantity, but I am not sure that the cast here is aiding the reader, nor
am I sure that it is making the code safer.
Thanks,
Taylor
