Re: [PATCH] difftool.c: mark a file-local symbol with static

From: Jeff King <peff@peff.net>
Date: Nov 30, 2016, 21:25 UTC
Message-ID: <20161130212510.ihcmvig7jq44p3nx@sigill.intra.peff.net>
In-Reply-To: <xmqqtwaod7ly.fsf@gitster.mtv.corp.google.com>

On Wed, Nov 30, 2016 at 12:40:25PM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
> > [I have fixed my config.mak file now, so I don't see the warning
> > anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or
> > not, is a separate matter.]
> 
> I suspect that 658df95a4a ("add DEVELOPER makefile knob to check for
> acknowledged warnings", 2016-02-25) took it from me (namely, Make
> script in my 'todo' branch).  In turn, I added it to my set of flags
> in order to squelch this exact warning, so...

For anybody interested in the history, we started using this when status_printf() got the format attribute. Relevant patch and discussion:

  http://public-inbox.org/git/20130710002328.GC19423@sigill.intra.peff.net/T/#u

We went with disabling the warning because it really is wrong. It makes an assumption that calling a format function with an empty string doesn't do anything, but that's only true of the stock printf functions. Our custom functions _do_ have a side effect in this case.

The other options are to have a special function for "print a warning (or status) line with no content". Or to teach those functions to handle newlines specially. We've often discussed that you should be able to do:

  warning("foo\nbar");

and have it print:

  warning: foo
  warning: bar

That's useful in itself, and would probably make cases like this easier to handle, too. But it's a pretty big change. Another option would be to just teach formatting functions to handle a single "\n" as a synonym for the empty string (or even detect trailing newlines and avoid appending our own in that case). That would mean you could do:

  warning("\n");

to print a blank line. That's arguably more obvious about the intent to a reader (I say arguably because the new behavior _is_ subtle if you happen to know that warning() usually appends a newline).

Anyway. Those are all options, but I don't think there is any problem with sticking with warning("") for now. It is not the first part of the code that tickles the format-zero-length warning.

-Peff