Re: [PATCH 1/3] apply: get rid of useless x < 0 comparison on a size_t type

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> According to the C standard size_t is always unsigned, therefore the
> comparison "n1 < 0 || n2 < 0" when n1 and n2 are size_t will always be
> false.
>
> This was raised by clang 2.9 which throws this warning when compiling
> apply.c:
>
>     builtin/apply.c:253:9: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
>             if (n1 < 0 || n2 < 0)
>                 ~~ ^ ~
>     builtin/apply.c:253:19: warning: comparison of unsigned expression < 0 is always false [-Wtautological-compare]
>             if (n1 < 0 || n2 < 0)
>                           ~~ ^ ~
>
> This check was originally added in v1.6.5-rc0~53^2 by Giuseppe Bilotta
> while adding an option to git-apply to ignore whitespace differences.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

I agree that these quantities can never be negative, so I'll apply the
patch as is.

But I have this suspicion that this was a rather sloppy defensive check to
protect this codepath against potential breakage in another codepath (most
likely update_pre_post_images() touched by the same patch) that adjusts
image->line[].len the caller of this function uses to feed these two
parameters. Giuseppe may have been not confident enough that the code
added to that function ensures not to undershoot when it reduces "len", or
something.

Giuseppe, can you explain what is going on?

Thanks

>  builtin/apply.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 84a8a0b..b3b59db 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -250,9 +250,6 @@ static int fuzzy_matchlines(const char *s1, size_t n1,
>  	const char *last2 = s2 + n2 - 1;
>  	int result = 0;
>  
> -	if (n1 < 0 || n2 < 0)
> -		return 0;
> -
>  	/* ignore line endings */
>  	while ((*last1 == '\r') || (*last1 == '\n'))
>  		last1--;