{"thread":{"id":"7071","subject":"[PATCH 1/6] Fix some \"printf format\" warnings.","startedAt":"2007-03-03T18:28:22Z","lastAt":"2007-03-14T23:41:52Z","messageCount":8,"participants":["Ramsay Jones","Junio C Hamano","Simon 'corecode' Schubert"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"36196","messageId":"45E9BE46.1020801@ramsay1.demon.co.uk","threadId":"7071","inReplyTo":null,"subject":"[PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2007-03-03T18:28:22Z","receivedAt":"2007-03-03T18:28:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nIt's interesting to note that all of the warnings are caused by mismatching\nparameter expression involving a call to ntohl().\n[OK, it wasn't that interesting ;-)]\n\n  builtin-ls-files.c       |    2 +-\n  builtin-unpack-objects.c |    2 +-\n  fetch-pack.c             |    2 +-\n  index-pack.c             |    2 +-\n  merge-index.c            |    2 +-\n  read-cache.c             |    2 +-\n  receive-pack.c           |    2 +-\n  sha1_file.c              |    4 ++--\n  unpack-trees.c           |    2 +-\n  9 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin-ls-files.c b/builtin-ls-files.c\nindex ac89eb2..5c990a5 100644\n--- a/builtin-ls-files.c\n+++ b/builtin-ls-files.c\n@@ -193,7 +193,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n  \t\tputchar(line_terminator);\n  \t}\n  \telse {\n-\t\tprintf(\"%s%06o %s %d\\t\",\n+\t\tprintf(\"%s%06lo %s %d\\t\",\n  \t\t       tag,\n  \t\t       ntohl(ce->ce_mode),\n  \t\t       abbrev ? find_unique_abbrev(ce->sha1,abbrev)\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex d351e02..d31d995 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -333,7 +333,7 @@ static void unpack_all(void)\n  \tif (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)\n  \t\tdie(\"bad pack file\");\n  \tif (!pack_version_ok(hdr->hdr_version))\n-\t\tdie(\"unknown pack file version %d\", ntohl(hdr->hdr_version));\n+\t\tdie(\"unknown pack file version %ld\", ntohl(hdr->hdr_version));\n  \tfprintf(stderr, \"Unpacking %d objects\\n\", nr_objects);\n\n  \tobj_list = xmalloc(nr_objects * sizeof(*obj_list));\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex c787106..9d86ec3 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -510,7 +510,7 @@ static int get_pack(int xd[2])\n\n  \t\tif (read_pack_header(fd[0], &header))\n  \t\t\tdie(\"protocol error: bad pack header\");\n-\t\tsnprintf(hdr_arg, sizeof(hdr_arg), \"--pack_header=%u,%u\",\n+\t\tsnprintf(hdr_arg, sizeof(hdr_arg), \"--pack_header=%lu,%lu\",\n  \t\t\t ntohl(header.hdr_version), ntohl(header.hdr_entries));\n  \t\tif (ntohl(header.hdr_entries) < unpack_limit)\n  \t\t\tdo_keep = 0;\ndiff --git a/index-pack.c b/index-pack.c\nindex 72e0962..01104cb 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -167,7 +167,7 @@ static void parse_pack_header(void)\n  \tif (hdr->hdr_signature != htonl(PACK_SIGNATURE))\n  \t\tdie(\"pack signature mismatch\");\n  \tif (!pack_version_ok(hdr->hdr_version))\n-\t\tdie(\"pack version %d unsupported\", ntohl(hdr->hdr_version));\n+\t\tdie(\"pack version %ld unsupported\", ntohl(hdr->hdr_version));\n\n  \tnr_objects = ntohl(hdr->hdr_entries);\n  \tuse(sizeof(struct pack_header));\ndiff --git a/merge-index.c b/merge-index.c\nindex a9983dd..2d3cac3 100644\n--- a/merge-index.c\n+++ b/merge-index.c\n@@ -60,7 +60,7 @@ static int merge_entry(int pos, const char *path)\n  \t\t\tbreak;\n  \t\tfound++;\n  \t\tstrcpy(hexbuf[stage], sha1_to_hex(ce->sha1));\n-\t\tsprintf(ownbuf[stage], \"%o\", ntohl(ce->ce_mode) & (~S_IFMT));\n+\t\tsprintf(ownbuf[stage], \"%lo\", ntohl(ce->ce_mode) & (~S_IFMT));\n  \t\targuments[stage] = hexbuf[stage];\n  \t\targuments[stage + 4] = ownbuf[stage];\n  \t} while (++pos < active_nr);\ndiff --git a/read-cache.c b/read-cache.c\nindex c54a611..6d46dcd 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -128,7 +128,7 @@ static int ce_match_stat_basic(struct cache_entry *ce, struct stat *st)\n  \t\tchanged |= !S_ISLNK(st->st_mode) ? TYPE_CHANGED : 0;\n  \t\tbreak;\n  \tdefault:\n-\t\tdie(\"internal error: ce_mode is %o\", ntohl(ce->ce_mode));\n+\t\tdie(\"internal error: ce_mode is %lo\", ntohl(ce->ce_mode));\n  \t}\n  \tif (ce->ce_mtime.sec != htonl(st->st_mtime))\n  \t\tchanged |= MTIME_CHANGED;\ndiff --git a/receive-pack.c b/receive-pack.c\nindex 7311c82..9e83371 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -282,7 +282,7 @@ static const char *unpack(void)\n  \thdr_err = parse_pack_header(&hdr);\n  \tif (hdr_err)\n  \t\treturn hdr_err;\n-\tsnprintf(hdr_arg, sizeof(hdr_arg), \"--pack_header=%u,%u\",\n+\tsnprintf(hdr_arg, sizeof(hdr_arg), \"--pack_header=%lu,%lu\",\n  \t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n\n  \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8ad7fad..80abd10 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -592,13 +592,13 @@ static int open_packed_git_1(struct packed_git *p)\n  \tif (hdr.hdr_signature != htonl(PACK_SIGNATURE))\n  \t\treturn error(\"file %s is not a GIT packfile\", p->pack_name);\n  \tif (!pack_version_ok(hdr.hdr_version))\n-\t\treturn error(\"packfile %s is version %u and not supported\"\n+\t\treturn error(\"packfile %s is version %lu and not supported\"\n  \t\t\t\" (try upgrading GIT to a newer version)\",\n  \t\t\tp->pack_name, ntohl(hdr.hdr_version));\n\n  \t/* Verify the pack matches its index. */\n  \tif (num_packed_objects(p) != ntohl(hdr.hdr_entries))\n-\t\treturn error(\"packfile %s claims to have %u objects\"\n+\t\treturn error(\"packfile %s claims to have %lu objects\"\n  \t\t\t\" while index size indicates %u objects\",\n  \t\t\tp->pack_name, ntohl(hdr.hdr_entries),\n  \t\t\tnum_packed_objects(p));\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2e2232c..0d1d8e7 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -539,7 +539,7 @@ static void show_stage_entry(FILE *o,\n  \tif (!ce)\n  \t\tfprintf(o, \"%s (missing)\\n\", label);\n  \telse\n-\t\tfprintf(o, \"%s%06o %s %d\\t%s\\n\",\n+\t\tfprintf(o, \"%s%06lo %s %d\\t%s\\n\",\n  \t\t\tlabel,\n  \t\t\tntohl(ce->ce_mode),\n  \t\t\tsha1_to_hex(ce->sha1),\n-- \n1.5.0\n"},{"id":"36210","messageId":"7v4pp29eok.fsf@assigned-by-dhcp.cox.net","threadId":"7071","inReplyTo":"45E9BE46.1020801@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-03T21:53:47Z","receivedAt":"2007-03-03T21:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n> ---\n>\n> It's interesting to note that all of the warnings are caused by mismatching\n> parameter expression involving a call to ntohl().\n> [OK, it wasn't that interesting ;-)]\n> diff --git a/builtin-ls-files.c b/builtin-ls-files.c\n> index ac89eb2..5c990a5 100644\n> --- a/builtin-ls-files.c\n> +++ b/builtin-ls-files.c\n> @@ -193,7 +193,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n>  \t\tputchar(line_terminator);\n>  \t}\n>  \telse {\n> -\t\tprintf(\"%s%06o %s %d\\t\",\n> +\t\tprintf(\"%s%06lo %s %d\\t\",\n>  \t\t       tag,\n>  \t\t       ntohl(ce->ce_mode),\n>  \t\t       abbrev ? find_unique_abbrev(ce->sha1,abbrev)\n\nI think the issue is ntohl() returns uint32_t, and this did not\nsurface as an issue so far only because that type happens to be\ndefined as 'unsigned int' on many systems.  Changing %o to %lo\nis shifting the breakage to other systems, isn't it?\n\nI think we should do this instead:\n\n\tprintf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n"},{"id":"36244","messageId":"45EAFD21.6010002@ramsay1.demon.co.uk","threadId":"7071","inReplyTo":"7v4pp29eok.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2007-03-04T17:08:49Z","receivedAt":"2007-03-04T17:08:49Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n>> diff --git a/builtin-ls-files.c b/builtin-ls-files.c\n>> index ac89eb2..5c990a5 100644\n>> --- a/builtin-ls-files.c\n>> +++ b/builtin-ls-files.c\n>> @@ -193,7 +193,7 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)\n>>  \t\tputchar(line_terminator);\n>>  \t}\n>>  \telse {\n>> -\t\tprintf(\"%s%06o %s %d\\t\",\n>> +\t\tprintf(\"%s%06lo %s %d\\t\",\n>>  \t\t       tag,\n>>  \t\t       ntohl(ce->ce_mode),\n>>  \t\t       abbrev ? find_unique_abbrev(ce->sha1,abbrev)\n> \n> I think the issue is ntohl() returns uint32_t, and this did not\n> surface as an issue so far only because that type happens to be\n> defined as 'unsigned int' on many systems.  Changing %o to %lo\n> is shifting the breakage to other systems, isn't it?\n> \n> I think we should do this instead:\n> \n> \tprintf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n> \n> \n\nOops, yes you are right.\n(cygwin typedef's uint32_t as unsigned long.)\n\nHowever, I would hate to add all those casts! Casts are not always evil, but\nshould be avoided if possible. Having said that, I don't see another solution ...\nHmmm, I dunno, your call ;-)\n\nAll the best,\n\nRamsay Jones\n"},{"id":"36908","messageId":"45F55DC5.8060702@fs.ei.tum.de","threadId":"7071","inReplyTo":"45EAFD21.6010002@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Simon 'corecode' Schubert","fromEmail":"corecode@fs.ei.tum.de","sentAt":"2007-03-12T14:03:49Z","receivedAt":"2007-03-12T14:03:49Z","isPatch":true,"sender":{"key":"corecode@fs.ei.tum.de","avatar":"https://gravatar.com/avatar/eff9dbf0cdac0d1e6a6cd7ed0e50763edcb376b493b5253a35ff167918ad79e1?d=mp&s=160"},"body":"Ramsay Jones wrote:\n>>> -        printf(\"%s%06o %s %d\\t\",\n>>> +        printf(\"%s%06lo %s %d\\t\",\n>>>                 tag,\n>>>                 ntohl(ce->ce_mode), \n>> I think we should do this instead:\n>>\n>>     printf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n> Oops, yes you are right.\n> (cygwin typedef's uint32_t as unsigned long.)\n> \n> However, I would hate to add all those casts! Casts are not always\n> evil, but should be avoided if possible. Having said that, I don't\n> see another solution ...\n\nshouldn't it be something like this?\n\nprintf(\"%s%06\"PRIo32\" %s %d\\t\", tag, ntohl(ce->ce_mode), ...)\n\nthat's the correct and allegedly portable way I guess.\n\ncheers\n  simon\n\n-- \nServe - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /\"\\\nWork - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \\ /\nParty Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \\\nDude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \\\n\n"},{"id":"36946","messageId":"7vfy8as129.fsf@assigned-by-dhcp.cox.net","threadId":"7071","inReplyTo":"45F55DC5.8060702@fs.ei.tum.de","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-12T19:40:14Z","receivedAt":"2007-03-12T19:40:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon 'corecode' Schubert <corecode@fs.ei.tum.de> writes:\n\n> Ramsay Jones wrote:\n>>>> -        printf(\"%s%06o %s %d\\t\",\n>>>> +        printf(\"%s%06lo %s %d\\t\",\n>>>>                 tag,\n>>>>                 ntohl(ce->ce_mode),\n>>> I think we should do this instead:\n>>>\n>>>     printf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n>>\n>> Oops, yes you are right.\n>> (cygwin typedef's uint32_t as unsigned long.)\n>>\n>> However, I would hate to add all those casts! Casts are not always\n>> evil, but should be avoided if possible. Having said that, I don't\n>> see another solution ...\n>\n> shouldn't it be something like this?\n>\n> printf(\"%s%06\"PRIo32\" %s %d\\t\", tag, ntohl(ce->ce_mode), ...)\n>\n> that's the correct and allegedly portable way I guess.\n\nYes, except that that is only portable across platforms with\ninttypes.h, and we would need a compatibility definition in\ngit-compat-util.h next to PRIuMAX definition we already have.\n\nBut I wonder if this is really worth it.  The thing is,\n\n    printf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n\nis perfectly readable for even old timers about git, as long as\nthey know traditional C and what ntohl() is.  And ce->ce_mode\neven fits in 16-bit, so while we are _not_ supporting platforms\nwhose unsigned int is 16-bit, the above cast is not losing any\nuseful precision either.\n"},{"id":"37124","messageId":"45F726AE.2080609@ramsay1.demon.co.uk","threadId":"7071","inReplyTo":"45F55DC5.8060702@fs.ei.tum.de","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2007-03-13T22:33:18Z","receivedAt":"2007-03-13T22:33:18Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Simon 'corecode' Schubert wrote:\n> Ramsay Jones wrote:\n>>>> -        printf(\"%s%06o %s %d\\t\",\n>>>> +        printf(\"%s%06lo %s %d\\t\",\n>>>>                 tag,\n>>>>                 ntohl(ce->ce_mode), \n>>> I think we should do this instead:\n>>>\n>>>     printf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n>> Oops, yes you are right.\n>> (cygwin typedef's uint32_t as unsigned long.)\n>>\n>> However, I would hate to add all those casts! Casts are not always\n>> evil, but should be avoided if possible. Having said that, I don't\n>> see another solution ...\n> \n> shouldn't it be something like this?\n> \n> printf(\"%s%06\"PRIo32\" %s %d\\t\", tag, ntohl(ce->ce_mode), ...)\n> \n> that's the correct and allegedly portable way I guess.\n> \n> cheers\n>  simon\n> \n\nYes, that would work, but again I was trying not to depend on a\nC99 header file (namely <inttypes.h>).\n\nI suppose I should just assume that git now requires these C99\nheaders!\n\nJunio, what do you think about Simon's solution?\n\nATB,\n\nRamsay Jones\n"},{"id":"37125","messageId":"45F87DC7.7020503@ramsay1.demon.co.uk","threadId":"7071","inReplyTo":"7vfy8as129.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2007-03-14T22:57:11Z","receivedAt":"2007-03-14T22:57:11Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> Simon 'corecode' Schubert <corecode@fs.ei.tum.de> writes:\n> \n>> Ramsay Jones wrote:\n>>>>> -        printf(\"%s%06o %s %d\\t\",\n>>>>> +        printf(\"%s%06lo %s %d\\t\",\n>>>>>                 tag,\n>>>>>                 ntohl(ce->ce_mode),\n>>>> I think we should do this instead:\n>>>>\n>>>>     printf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n>>> Oops, yes you are right.\n>>> (cygwin typedef's uint32_t as unsigned long.)\n>>>\n>>> However, I would hate to add all those casts! Casts are not always\n>>> evil, but should be avoided if possible. Having said that, I don't\n>>> see another solution ...\n>> shouldn't it be something like this?\n>>\n>> printf(\"%s%06\"PRIo32\" %s %d\\t\", tag, ntohl(ce->ce_mode), ...)\n>>\n>> that's the correct and allegedly portable way I guess.\n> \n> Yes, except that that is only portable across platforms with\n> inttypes.h, and we would need a compatibility definition in\n> git-compat-util.h next to PRIuMAX definition we already have.\n> \n\nHmmm, I only have the 1.5.0 code, and so I don't know what the\ncurrent code looks like; so if this is rubbish, please ignore...\n\nIn version 1.4.4, the only files to depend on <stdint.h> were\n{arm,ppc}/sha1.[ch]. If this was a problem on any given arm/ppc\nplatform, you could configure the software to use the OpenSSL\nor Mozilla versions instead.\n\nIn 1.5.0, however, git-compat-util.h _unconditionally_ includes\n<inttypes.h>, which in turn includes <stdint.h>. Now the files\nwhich depend on definitions in <stdint.h> includes:\n    {arm,ppc}/sha1.[ch]\n    cache.h\n    pack.h\n    builtin-pack-objects.c\n    fast-import.c\n    sha1_file.c\n(the symbols being: uint32_t, uint64_t, uint16_t, uintmax_t)\nVersion 1.5.0 does not seem to depend on any symbols in the\n<inttypes.h> header file...\n\nSo, at least for 1.5.0, those header files are already\nrequired.\n\nI don't know if that is desired ;-)\n\nATB,\n\nRamsay Jones\n"},{"id":"37131","messageId":"7vd53b5r5r.fsf@assigned-by-dhcp.cox.net","threadId":"7071","inReplyTo":"45F87DC7.7020503@ramsay1.demon.co.uk","subject":"Re: [PATCH 1/6] Fix some \"printf format\" warnings.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-03-14T23:41:52Z","receivedAt":"2007-03-14T23:41:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n>>> printf(\"%s%06\"PRIo32\" %s %d\\t\", tag, ntohl(ce->ce_mode), ...)\n>>>\n>>> that's the correct and allegedly portable way I guess.\n\nI do not think this is really worth it.\n\n    printf(\"%s%06o %s %d\\t\", tag, (unsigned) ntohl(ce->ce_mode), ...\n\nis perfectly readable for even old timers about git, as long as\nthey know traditional C and what ntohl() is.  And ce->ce_mode\neven fits in 16-bit, so while we are _not_ supporting platforms\nwhose unsigned int is 16-bit, the above cast is not losing any\nuseful precision either.\n\n> (the symbols being: uint32_t, uint64_t, uint16_t, uintmax_t)\n\nI think we would want these exact-sized integer types, so if\nthey are missing the platform ports may want to supply their own\nsubstitute.\n"}]}