Re: [PATCH] ls-files: use correct format string

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Apr 11, 2019, 21:28 UTC
Message-ID: <20190411212830.GF32487@hank.intra.tgummerer.com>
In-Reply-To: <20190411041823.GA17699@sigill.intra.peff.net>

On 04/11, Jeff King wrote:

Show 15 quoted lines

> On Sun, Apr 07, 2019 at 07:47:51PM +0100, Thomas Gummerer wrote:
> 
> > struct stat_data and struct cache_time both use unsigned ints for all
> > their members.  However the format string for 'git ls-files --debug'
> > currently uses %d for formatting these numbers.  This means that we
> > potentially print these values incorrectly if they are greater than
> > INT_MAX.
> > 
> > This has been the case since the --debug option was introduced in 'git
> > ls-files' in 8497421715 ("ls-files: learn a debugging dump format",
> > 2010-07-31).
> 
> I didn't see any comment on this, but it seems like it must be obviously
> correct, since as you note we do define those fields as unsigned. I'm
> really surprised that -Wformat doesn't catch this, though. I wonder why.

Good point. A bit of digging led me to -Wformat-signedness, which should catch this. This turns up a lot of errors in our codebase. I didn't go through to see how many of them are actual errors, and how many are false-positives though.

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65446 describes how the option can lead to false positives, e.g.

    printf ("%u\n", unsigned_short);

might turn up an error. From a quick test this seems to work correctly with gcc 8.2.1 that I have on my machine though, so the issue might be fixed in newer gcc version, even though that bug report is still marked as new.

Maybe it's worth going through the warnings at some point to see if it would be possible to turn -Wformat-signedness on.