[PATCH] upload-pack: fix some sparse warnings

Ramsay Jones· Jan 5, 2018, 20:30 UTC · lore

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).

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?

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?

Thanks!

ATB, Ramsay Jones

 upload-pack.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Show changes to upload-pack.c +2 −1

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· Jan 8, 2018, 18:51 UTC · re: Ramsay Jones · lore

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

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

patchupload-pack: fix some sparse warnings