Re: [PATCH] upload-pack: fix some sparse warnings

From: Brandon Williams <bmwill@google.com>
Date: Jan 8, 2018, 18:51 UTC
Message-ID: <20180108185103.GA156560@google.com>
In-Reply-To: <720013d4-a493-e0b6-7a38-9ebbbaa198ce@ramsayjones.plus.com>

On 01/05, Ramsay Jones wrote:

Show 11 quoted lines

> 
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
> 
> Hi Brandon,
> 
> If you need to re-roll your 'bw/protocol-v2' branch, could you please
> squash this (or something like it) into the relevant patches. The first
> hunk would go in commit 6ec1105192, "upload-pack: convert to a builtin",
> 2018-01-02), whereas the second hunk would go to commit b3f3749a24,
> "upload-pack: factor out processing lines", 2018-01-02).

Thanks for finding these, I'll make sure I include them in the relevant commits.

Show 18 quoted lines

> 
> The sparse warnings were:
> 
>        SP upload-pack.c
>    upload-pack.c:783:43: error: incompatible types for operation (<=)
>    upload-pack.c:783:43:    left side has type int *depth
>    upload-pack.c:783:43:    right side has type int
>    upload-pack.c:783:43: error: incorrect type in conditional
>    upload-pack.c:783:43:    got bad type
>    upload-pack.c:1389:5: warning: symbol 'cmd_upload_pack' was not declared. Should it be static?
> 
> [Note that the line numbers are off-by-one.]
> 
> I note, in passing, that strtol() returns a 'long int' but *depth is
> an 'int' (yes, the original code was like that too ;-) ).
> 
> Should cmd_upload_pack(), along with the #include "builtin.h", be moved
> to builtin/upload-pack.c?

Junio mentioned something similar when I sent out the WIP series, I must have forgotten to do that before sending this out. I'll make that change :)

> 
> Also, I note that packet_read_with_status(), introduced in commit 4570421c3,
> is not called outside of pkt-line.c; does this symbol need to be extern?

I thought it might, but you're right it doesn't look like it needs to. I'll change that so its not exported.

Show 31 quoted lines

> 
> Thanks!
> 
> ATB,
> Ramsay Jones
> 
> 
>  upload-pack.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/upload-pack.c b/upload-pack.c
> index 8002f1f20..6271245e2 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -1,4 +1,5 @@
>  #include "cache.h"
> +#include "builtin.h"
>  #include "config.h"
>  #include "refs.h"
>  #include "pkt-line.h"
> @@ -780,7 +781,7 @@ static int process_deepen(const char *line, int *depth)
>  	if (skip_prefix(line, "deepen ", &arg)) {
>  		char *end = NULL;
>  		*depth = strtol(arg, &end, 0);
> -		if (!end || *end || depth <= 0)
> +		if (!end || *end || *depth <= 0)
>  			die("Invalid deepen: %s", line);
>  		return 1;
>  	}
> -- 
> 2.15.0

-- 
Brandon Williams