Re: [PATCH v2 11/12] builtin/gc: fix signedness issues in ODB-related functionality

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> There are a couple of signedness issues in ODB-related functionality.
> These are not a problem because we disable -Wsign-compare in this file,
> but once we move these functions into "odb/source-files.c" they will
> result in warnings.
>
> Fix those issues:
>
>   - In `too_many_loose_objects()` we receive a signed limit, but compare
>     it with the unsigned actual number of loose objects. This is fixed
>     by bailing out immediately when the limit is smaller than or equal
>     to zero, which we also do similarly in other places. The warning is
>     then squelched via a cast.
>
>   - In `find_base_packs()` we compare the signed size of the pack
>     against the unsigned limit. As the pack size is always going to be a
>     positive file size it's safe to cast it to an unsigned value.
>
>   - In `odb_optimize()` we compare the unsigned `keep_pack.nr` value
>     against the signed `gc_auto_pack_limit`. We only reach this code
>     when `too_many_packs()` returns true-ish, and that can only happen
>     when `gc_auto_pack_limit > 0`. Consequently, we can fix the warning
>     by casting the limit to an unsigned value.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/gc.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)

Yuck.  The -Wsign-compare strikes again X-<.

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 3207182488..8cf3781313 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -430,19 +430,21 @@ static int rerere_gc_condition(struct gc_config *cfg UNUSED)
>  
>  static int too_many_loose_objects(struct odb_source_files *files, int limit)
>  {
> -	/*
> -	 * This is weird, but stems from legacy behaviour: the GC auto
> -	 * threshold was always essentially interpreted as if it was rounded up
> -	 * to the next multiple 256 of, so we retain this behaviour for now.
> -	 */
> -	int auto_threshold = DIV_ROUND_UP(limit, 256) * 256;
>  	unsigned long loose_count;
>  
> +	if (limit <= 0)
> +		return 0;
> +
>  	if (odb_source_count_objects(&files->loose->base, ODB_COUNT_OBJECTS_APPROXIMATE,
>  				     &loose_count) < 0)
>  		return 0;
>  
> -	return loose_count > auto_threshold;
> +	/*
> +	 * This is weird, but stems from legacy behaviour: the GC auto
> +	 * threshold was always essentially interpreted as if it was rounded up
> +	 * to the next multiple 256 of, so we retain this behaviour for now.
> +	 */
> +	return loose_count > (DIV_ROUND_UP(((unsigned long) limit), 256) * 256);
>  }

OK.  It is trivially correct (if rounding up is correct, that is).

> @@ -456,7 +458,7 @@ static struct packed_git *find_base_packs(struct odb_source_files *files,
>  		if (e->pack->is_cruft)
>  			continue;
>  		if (limit) {
> -			if (e->pack->pack_size >= limit)
> +			if ((uintmax_t) e->pack->pack_size >= limit)

Here, just like in too_many_loose_objects(), 'limit' is of type
'unsigned long'.  While it makes sense to convert both sides of
the comparison to an unsigned type, casting only the left side
to a type that differs from the right side puzzles me.

Presumably, the other side is of type 'off_t', which is signed,
explaining the desire to cast it to an unsigned type.  But I am
not sure what happens if 'off_t' is wider than 'unsigned long'.

Otherwise looking good.