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

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Nov 30, 2016, 19:57 UTC
Message-ID: <29abc89b-9ca5-930f-8e90-ca446ac2b96a@ramsayjones.plus.com>
In-Reply-To: <alpine.DEB.2.20.1611301204020.117539@virtualbox>

On 30/11/16 11:07, Johannes Schindelin wrote:

Show 19 quoted lines

> Hi Ramsay,
> 
> On Tue, 29 Nov 2016, Ramsay Jones wrote:
>> Also, due to a problem in my config.mak file on Linux (a commented
>> out line that had a line continuation '\', grrrrr!), gcc issued a
>> warning, thus:
>>
>>   builtin/difftool.c: In function ‘run_dir_diff’:
>>   builtin/difftool.c:568:13: warning: zero-length gnu_printf format string [-Wformat-zero-length]
>>        warning("");
>>                ^
>> I am not sure why -Wno-format-zero-length is set in DEVELOPER_CFLAGS,
>> but do you really need to space the output with an an 'empty'
>> "warning:" line? (Just curious).
> 
> That `warning("");` comes from a straight-forward port of this line (see
> https://github.com/git/git/blob/v2.11.0/git-difftool.perl#L425):
> 
> 	$errmsg .= "warning:\n";

Ah, OK, so it is being used to 'space out' the (possibly multiple) 'Both files modified' warning(s) followed by a 2-line warning summary. Hmm, I am not sure the 'Working tree file has been left' (after each pair of files) part of the message is adding much ...

Show 8 quoted lines

> I could see two possible ways out:
> 
> - warning("%s", ""); (ugly!)
> 
> - do away with the "prefix every line with warning:" convention and simply
>   have a multi-line `warning(_("...\n...\n"), ...)`
> 
> What do you think?

I think, for now, it is probably best to do nothing. ;-)

Until you have replaced the 'legacy' difftool, it would be best not to alter the output from the tool, so that the tests can be used unaltered on both. This could be addressed (if necessary) after you complete your series.

[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.]

ATB, Ramsay Jones