Re: Fixing the warning about warning(""); was: Re: [PATCH] difftool.c: mark a file-local symbol with static

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Wed, 25 Jan 2017, Jeff King wrote:

> On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:
> 
> > > Gross, but at least it's self documenting. :)
> > > 
> > > I guess a less horrible version of that is:
> > > 
> > >   static inline warning_blank_line(void)
> > >   {
> > > 	warning("%s", "");
> > >   }
> > > 
> > > We'd potentially need a matching one for error(), but at last it avoids
> > > macro trickery.
> > 
> > I fail to see how this function, or this definition, makes the code better
> > than simply calling `warning("%s", "");` and be done with it.
> 
> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to ("").

We could switch the DEVELOPER option on by default, when gcc or clang is
used at least. Otherwise the DEVELOPER option (which I like very much)
would not be able to live up to its full potential.

Another thing we should consider: paying more attention to Continuous
Integration. At the moment, it happens quite frequently that `pu` builds
and passes the test suite fine on Linux, but neither on Windows nor on
MacOSX and it takes days to get the regressions fixed.

I vote for this patch:

> -- >8 --
> Subject: [PATCH] difftool: hack around -Wzero-length-format warning
> 
> Building with "gcc -Wall" will complain that the format in:
> 
>   warning("")
> 
> is empty. Which is true, but the warning is over-eager. We
> are calling the function for its side effect of printing
> "warning:", even with an empty string.
> 
> Our DEVELOPER Makefile knob disables the warning, but not
> everybody uses it. Let's silence the warning in the code so
> that nobody reports it or tries to "fix" it.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/difftool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 42ad9e804..b5e85ab07 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  				warning(_("both files modified: '%s' and '%s'."),
>  					wtdir.buf, rdir.buf);
>  				warning(_("working tree file has been left."));
> -				warning("");
> +				warning("%s", "");
>  				err = 1;
>  			} else if (unlink(wtdir.buf) ||
>  				   copy_file(wtdir.buf, rdir.buf, st.st_mode))
> -- 
> 2.11.0.840.gd37c5973a

Ciao,
Dscho