[PATCH 1/6] Fix some "printf format" warnings.

[PATCH 1/6] Fix some "printf format" warnings.

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

It's interesting to note that all of the warnings are caused by mismatching
parameter expression involving a call to ntohl().
[OK, it wasn't that interesting ;-)]

  builtin-ls-files.c       |    2 +-
  builtin-unpack-objects.c |    2 +-
  fetch-pack.c             |    2 +-
  index-pack.c             |    2 +-
  merge-index.c            |    2 +-
  read-cache.c             |    2 +-
  receive-pack.c           |    2 +-
  sha1_file.c              |    4 ++--
  unpack-trees.c           |    2 +-
  9 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin-ls-files.c b/builtin-ls-files.c
index ac89eb2..5c990a5 100644
--- a/builtin-ls-files.c
+++ b/builtin-ls-files.c
@@ -193,7 +193,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
  		putchar(line_terminator);
  	}
  	else {
-		printf("%s%06o %s %d\t",
+		printf("%s%06lo %s %d\t",
  		       tag,
  		       ntohl(ce->ce_mode),
  		       abbrev ? find_unique_abbrev(ce->sha1,abbrev)
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index d351e02..d31d995 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -333,7 +333,7 @@ static void unpack_all(void)
  	if (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)
  		die("bad pack file");
  	if (!pack_version_ok(hdr->hdr_version))
-		die("unknown pack file version %d", ntohl(hdr->hdr_version));
+		die("unknown pack file version %ld", ntohl(hdr->hdr_version));
  	fprintf(stderr, "Unpacking %d objects\n", nr_objects);

  	obj_list = xmalloc(nr_objects * sizeof(*obj_list));
diff --git a/fetch-pack.c b/fetch-pack.c
index c787106..9d86ec3 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -510,7 +510,7 @@ static int get_pack(int xd[2])

  		if (read_pack_header(fd[0], &header))
  			die("protocol error: bad pack header");
-		snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%u,%u",
+		snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%lu,%lu",
  			 ntohl(header.hdr_version), ntohl(header.hdr_entries));
  		if (ntohl(header.hdr_entries) < unpack_limit)
  			do_keep = 0;
diff --git a/index-pack.c b/index-pack.c
index 72e0962..01104cb 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -167,7 +167,7 @@ static void parse_pack_header(void)
  	if (hdr->hdr_signature != htonl(PACK_SIGNATURE))
  		die("pack signature mismatch");
  	if (!pack_version_ok(hdr->hdr_version))
-		die("pack version %d unsupported", ntohl(hdr->hdr_version));
+		die("pack version %ld unsupported", ntohl(hdr->hdr_version));

  	nr_objects = ntohl(hdr->hdr_entries);
  	use(sizeof(struct pack_header));
diff --git a/merge-index.c b/merge-index.c
index a9983dd..2d3cac3 100644
--- a/merge-index.c
+++ b/merge-index.c
@@ -60,7 +60,7 @@ static int merge_entry(int pos, const char *path)
  			break;
  		found++;
  		strcpy(hexbuf[stage], sha1_to_hex(ce->sha1));
-		sprintf(ownbuf[stage], "%o", ntohl(ce->ce_mode) & (~S_IFMT));
+		sprintf(ownbuf[stage], "%lo", ntohl(ce->ce_mode) & (~S_IFMT));
  		arguments[stage] = hexbuf[stage];
  		arguments[stage + 4] = ownbuf[stage];
  	} while (++pos < active_nr);
diff --git a/read-cache.c b/read-cache.c
index c54a611..6d46dcd 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -128,7 +128,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)
  		changed |= !S_ISLNK(st->st_mode) ? TYPE_CHANGED : 0;
  		break;
  	default:
-		die("internal error: ce_mode is %o", ntohl(ce->ce_mode));
+		die("internal error: ce_mode is %lo", ntohl(ce->ce_mode));
  	}
  	if (ce->ce_mtime.sec != htonl(st->st_mtime))
  		changed |= MTIME_CHANGED;
diff --git a/receive-pack.c b/receive-pack.c
index 7311c82..9e83371 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -282,7 +282,7 @@ static const char *unpack(void)
  	hdr_err = parse_pack_header(&hdr);
  	if (hdr_err)
  		return hdr_err;
-	snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%u,%u",
+	snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%lu,%lu",
  			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));

  	if (ntohl(hdr.hdr_entries) < unpack_limit) {
diff --git a/sha1_file.c b/sha1_file.c
index 8ad7fad..80abd10 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -592,13 +592,13 @@ static int open_packed_git_1(struct packed_git *p)
  	if (hdr.hdr_signature != htonl(PACK_SIGNATURE))
  		return error("file %s is not a GIT packfile", p->pack_name);
  	if (!pack_version_ok(hdr.hdr_version))
-		return error("packfile %s is version %u and not supported"
+		return error("packfile %s is version %lu and not supported"
  			" (try upgrading GIT to a newer version)",
  			p->pack_name, ntohl(hdr.hdr_version));

  	/* Verify the pack matches its index. */
  	if (num_packed_objects(p) != ntohl(hdr.hdr_entries))
-		return error("packfile %s claims to have %u objects"
+		return error("packfile %s claims to have %lu objects"
  			" while index size indicates %u objects",
  			p->pack_name, ntohl(hdr.hdr_entries),
  			num_packed_objects(p));
diff --git a/unpack-trees.c b/unpack-trees.c
index 2e2232c..0d1d8e7 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -539,7 +539,7 @@ static void show_stage_entry(FILE *o,
  	if (!ce)
  		fprintf(o, "%s (missing)\n", label);
  	else
-		fprintf(o, "%s%06o %s %d\t%s\n",
+		fprintf(o, "%s%06lo %s %d\t%s\n",
  			label,
  			ntohl(ce->ce_mode),
  			sha1_to_hex(ce->sha1),
-- 
1.5.0

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>
> It's interesting to note that all of the warnings are caused by mismatching
> parameter expression involving a call to ntohl().
> [OK, it wasn't that interesting ;-)]
> diff --git a/builtin-ls-files.c b/builtin-ls-files.c
> index ac89eb2..5c990a5 100644
> --- a/builtin-ls-files.c
> +++ b/builtin-ls-files.c
> @@ -193,7 +193,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
>  		putchar(line_terminator);
>  	}
>  	else {
> -		printf("%s%06o %s %d\t",
> +		printf("%s%06lo %s %d\t",
>  		       tag,
>  		       ntohl(ce->ce_mode),
>  		       abbrev ? find_unique_abbrev(ce->sha1,abbrev)

I think the issue is ntohl() returns uint32_t, and this did not
surface as an issue so far only because that type happens to be
defined as 'unsigned int' on many systems.  Changing %o to %lo
is shifting the breakage to other systems, isn't it?

I think we should do this instead:

	printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Ramsay Jones]

Hi Junio,

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>> diff --git a/builtin-ls-files.c b/builtin-ls-files.c
>> index ac89eb2..5c990a5 100644
>> --- a/builtin-ls-files.c
>> +++ b/builtin-ls-files.c
>> @@ -193,7 +193,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
>>  		putchar(line_terminator);
>>  	}
>>  	else {
>> -		printf("%s%06o %s %d\t",
>> +		printf("%s%06lo %s %d\t",
>>  		       tag,
>>  		       ntohl(ce->ce_mode),
>>  		       abbrev ? find_unique_abbrev(ce->sha1,abbrev)
> 
> I think the issue is ntohl() returns uint32_t, and this did not
> surface as an issue so far only because that type happens to be
> defined as 'unsigned int' on many systems.  Changing %o to %lo
> is shifting the breakage to other systems, isn't it?
> 
> I think we should do this instead:
> 
> 	printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...
> 
> 

Oops, yes you are right.
(cygwin typedef's uint32_t as unsigned long.)

However, I would hate to add all those casts! Casts are not always evil, but
should be avoided if possible. Having said that, I don't see another solution ...
Hmmm, I dunno, your call ;-)

All the best,

Ramsay Jones

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Simon 'corecode' Schubert]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

Ramsay Jones wrote:
>>> -        printf("%s%06o %s %d\t",
>>> +        printf("%s%06lo %s %d\t",
>>>                 tag,
>>>                 ntohl(ce->ce_mode), 
>> I think we should do this instead:
>>
>>     printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...
> Oops, yes you are right.
> (cygwin typedef's uint32_t as unsigned long.)
> 
> However, I would hate to add all those casts! Casts are not always
> evil, but should be avoided if possible. Having said that, I don't
> see another solution ...

shouldn't it be something like this?

printf("%s%06"PRIo32" %s %d\t", tag, ntohl(ce->ce_mode), ...)

that's the correct and allegedly portable way I guess.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Junio C Hamano]

Simon 'corecode' Schubert <corecode@fs.ei.tum.de> writes:

> Ramsay Jones wrote:
>>>> -        printf("%s%06o %s %d\t",
>>>> +        printf("%s%06lo %s %d\t",
>>>>                 tag,
>>>>                 ntohl(ce->ce_mode),
>>> I think we should do this instead:
>>>
>>>     printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...
>>
>> Oops, yes you are right.
>> (cygwin typedef's uint32_t as unsigned long.)
>>
>> However, I would hate to add all those casts! Casts are not always
>> evil, but should be avoided if possible. Having said that, I don't
>> see another solution ...
>
> shouldn't it be something like this?
>
> printf("%s%06"PRIo32" %s %d\t", tag, ntohl(ce->ce_mode), ...)
>
> that's the correct and allegedly portable way I guess.

Yes, except that that is only portable across platforms with
inttypes.h, and we would need a compatibility definition in
git-compat-util.h next to PRIuMAX definition we already have.

But I wonder if this is really worth it.  The thing is,

    printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...

is perfectly readable for even old timers about git, as long as
they know traditional C and what ntohl() is.  And ce->ce_mode
even fits in 16-bit, so while we are _not_ supporting platforms
whose unsigned int is 16-bit, the above cast is not losing any
useful precision either.

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Ramsay Jones]

Simon 'corecode' Schubert wrote:
> Ramsay Jones wrote:
>>>> -        printf("%s%06o %s %d\t",
>>>> +        printf("%s%06lo %s %d\t",
>>>>                 tag,
>>>>                 ntohl(ce->ce_mode), 
>>> I think we should do this instead:
>>>
>>>     printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...
>> Oops, yes you are right.
>> (cygwin typedef's uint32_t as unsigned long.)
>>
>> However, I would hate to add all those casts! Casts are not always
>> evil, but should be avoided if possible. Having said that, I don't
>> see another solution ...
> 
> shouldn't it be something like this?
> 
> printf("%s%06"PRIo32" %s %d\t", tag, ntohl(ce->ce_mode), ...)
> 
> that's the correct and allegedly portable way I guess.
> 
> cheers
>  simon
> 

Yes, that would work, but again I was trying not to depend on a
C99 header file (namely <inttypes.h>).

I suppose I should just assume that git now requires these C99
headers!

Junio, what do you think about Simon's solution?

ATB,

Ramsay Jones

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Ramsay Jones]

Junio C Hamano wrote:
> Simon 'corecode' Schubert <corecode@fs.ei.tum.de> writes:
> 
>> Ramsay Jones wrote:
>>>>> -        printf("%s%06o %s %d\t",
>>>>> +        printf("%s%06lo %s %d\t",
>>>>>                 tag,
>>>>>                 ntohl(ce->ce_mode),
>>>> I think we should do this instead:
>>>>
>>>>     printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...
>>> Oops, yes you are right.
>>> (cygwin typedef's uint32_t as unsigned long.)
>>>
>>> However, I would hate to add all those casts! Casts are not always
>>> evil, but should be avoided if possible. Having said that, I don't
>>> see another solution ...
>> shouldn't it be something like this?
>>
>> printf("%s%06"PRIo32" %s %d\t", tag, ntohl(ce->ce_mode), ...)
>>
>> that's the correct and allegedly portable way I guess.
> 
> Yes, except that that is only portable across platforms with
> inttypes.h, and we would need a compatibility definition in
> git-compat-util.h next to PRIuMAX definition we already have.
> 

Hmmm, I only have the 1.5.0 code, and so I don't know what the
current code looks like; so if this is rubbish, please ignore...

In version 1.4.4, the only files to depend on <stdint.h> were
{arm,ppc}/sha1.[ch]. If this was a problem on any given arm/ppc
platform, you could configure the software to use the OpenSSL
or Mozilla versions instead.

In 1.5.0, however, git-compat-util.h _unconditionally_ includes
<inttypes.h>, which in turn includes <stdint.h>. Now the files
which depend on definitions in <stdint.h> includes:
    {arm,ppc}/sha1.[ch]
    cache.h
    pack.h
    builtin-pack-objects.c
    fast-import.c
    sha1_file.c
(the symbols being: uint32_t, uint64_t, uint16_t, uintmax_t)
Version 1.5.0 does not seem to depend on any symbols in the
<inttypes.h> header file...

So, at least for 1.5.0, those header files are already
required.

I don't know if that is desired ;-)

ATB,

Ramsay Jones

Re: [PATCH 1/6] Fix some "printf format" warnings.

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

>>> printf("%s%06"PRIo32" %s %d\t", tag, ntohl(ce->ce_mode), ...)
>>>
>>> that's the correct and allegedly portable way I guess.

I do not think this is really worth it.

    printf("%s%06o %s %d\t", tag, (unsigned) ntohl(ce->ce_mode), ...

is perfectly readable for even old timers about git, as long as
they know traditional C and what ntohl() is.  And ce->ce_mode
even fits in 16-bit, so while we are _not_ supporting platforms
whose unsigned int is 16-bit, the above cast is not losing any
useful precision either.

> (the symbols being: uint32_t, uint64_t, uint16_t, uintmax_t)

I think we would want these exact-sized integer types, so if
they are missing the platform ports may want to supply their own
substitute.