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

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

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---

Hi Johannes,

If you need to re-roll your 'js/difftool-builtin' branch, could
you please squash this into the relevant patch.

Thanks!

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).

ATB,
Ramsay Jones

 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 3480920..830369c 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -170,7 +170,7 @@ struct path_entry {
 	char path[FLEX_ARRAY];
 };
 
-int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
 {
 	return strcmp(a->path, key ? key : b->path);
 }
-- 
2.9.0

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Hi Ramsay,

On Tue, 29 Nov 2016, Ramsay Jones wrote:

> If you need to re-roll your 'js/difftool-builtin' branch, could
> you please squash this into the relevant patch.

Fixed. Thanks!

> 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";

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?
Dscho

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

[Ramsay Jones]

On 30/11/16 11:07, Johannes Schindelin wrote:
> 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 ...

> 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

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

[Junio C Hamano]

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

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

[Jeff King]

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

> 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

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

[Ramsay Jones]

On 30/11/16 21:25, Jeff King wrote:
> On Wed, Nov 30, 2016 at 12:40:25PM -0800, Junio C Hamano wrote:
> 
>> 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

Ah, thank you! I've been trying to remember this for the last hour or so ...
(I misremembered something about gettext, and went looking in the wrong
direction ;-) ).

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

Yes, I agree, gcc is making an unwarranted assumption.

> 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).

Yes, I remember the discussion now and agree that this could
cause problems.

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

Hmm, well I have been building git for some time with the warning
enabled without problem.

[I can see a few examples of 'status_printf_ln(s, c, "%s", "");' in
git-grep output, so ...]

I am not suggesting any change here, and was just curious why it
seemed to be unnecessary now (I knew it was necessary at one time).

ATB,
Ramsay Jones

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

[Jeff King]

On Wed, Nov 30, 2016 at 10:37:40PM +0000, Ramsay Jones wrote:

> > 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.
> 
> Hmm, well I have been building git for some time with the warning
> enabled without problem.
> 
> [I can see a few examples of 'status_printf_ln(s, c, "%s", "");' in
> git-grep output, so ...]
> 
> I am not suggesting any change here, and was just curious why it
> seemed to be unnecessary now (I knew it was necessary at one time).

I forgot, we ended up reversing course later and silencing them:

  http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/

By the rationale of that conversation, we should be doing:

  warning("%s", "");

here.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I forgot, we ended up reversing course later and silencing them:
>
>   http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
>
> By the rationale of that conversation, we should be doing:
>
>   warning("%s", "");
>
> here.

I forgot too.  Thanks for digging up that thread.

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

[Ramsay Jones]

On 30/11/16 23:46, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> I forgot, we ended up reversing course later and silencing them:
>>
>>   http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
>>
>> By the rationale of that conversation, we should be doing:
>>
>>   warning("%s", "");
>>
>> here.
> 
> I forgot too.  Thanks for digging up that thread.

Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022
as well.

So, by the same rationale, we should remove -Wno-format-zero-length
from DEVELOPER_CFLAGS. yes?

ATB,
Ramsay Jones

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

[Jeff King]

On Thu, Dec 01, 2016 at 01:18:35AM +0000, Ramsay Jones wrote:

> >> I forgot, we ended up reversing course later and silencing them:
> >>
> >>   http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/
> >>
> >> By the rationale of that conversation, we should be doing:
> >>
> >>   warning("%s", "");
> >>
> >> here.
> > 
> > I forgot too.  Thanks for digging up that thread.
> 
> Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022
> as well.
> 
> So, by the same rationale, we should remove -Wno-format-zero-length
> from DEVELOPER_CFLAGS. yes?

I don't have a preference on which direction we go, but yes, right now
we are in an awkward middle ground. We should do one of:

  1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure
     future patches to do not violate it.

  2. Declare warning("") as OK.

I still think the warning is silly, but (1) has value in that it
produces the least surprise and annoyance to various people building
Git.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't have a preference on which direction we go, but yes, right now
> we are in an awkward middle ground. We should do one of:
>
>   1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure
>      future patches to do not violate it.
>
>   2. Declare warning("") as OK.
>
> I still think the warning is silly, but (1) has value in that it
> produces the least surprise and annoyance to various people building
> Git.

What is most awkward is that "make", with no customization out of
box, suggests to use -Wall in CFLAGS and triggers the misguided
warning, and DEVELOPER_CFLAGS, partly because it adds -Werror to
turn warnings (including this misguided one) into errors, disables
it with -Wno-format-zero-length.  As a result:

 - Casual builders that follow our suggestion get warnings.

 - Developers do not notice their new code may make the "casual
   builder" experience worse.

That is totally backwards.  That is probably what you meant by "the
least surprise and annoyance"?

I also still think that any_printf_like_function("%s", "") looks
silly.  I know that we've already started moving in that direction
and we stopped at a place we do not want to be in, but perhaps it
was a mistake to move in that direction in the first place.  I am
tempted to say we should do something like the attached, but that
may not fly well, as I suspect that -Wno-format-zero-length may be a
lot more exotic than the -Wall compiler option.  An obvious second
best option would be to drop -Wall from the "everybody" CFLAGS and
move it to DEVELOPER_CFLAGS instead.

diff --git a/Makefile b/Makefile
index a379738db2..137c10e257 100644
--- a/Makefile
+++ b/Makefile
@@ -391,10 +391,9 @@ GIT-VERSION-FILE: FORCE
 
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -Wno-format-zero-length
 DEVELOPER_CFLAGS = -Werror \
 	-Wdeclaration-after-statement \
-	-Wno-format-zero-length \
 	-Wold-style-definition \
 	-Woverflow \
 	-Wpointer-arith \

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

[Jeff King]

On Thu, Dec 01, 2016 at 10:20:50AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I don't have a preference on which direction we go, but yes, right now
> > we are in an awkward middle ground. We should do one of:
> >
> >   1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure
> >      future patches to do not violate it.
> >
> >   2. Declare warning("") as OK.
> >
> > I still think the warning is silly, but (1) has value in that it
> > produces the least surprise and annoyance to various people building
> > Git.
> 
> What is most awkward is that "make", with no customization out of
> box, suggests to use -Wall in CFLAGS and triggers the misguided
> warning, and DEVELOPER_CFLAGS, partly because it adds -Werror to
> turn warnings (including this misguided one) into errors, disables
> it with -Wno-format-zero-length.  As a result:
> 
>  - Casual builders that follow our suggestion get warnings.
> 
>  - Developers do not notice their new code may make the "casual
>    builder" experience worse.
> 
> That is totally backwards.  That is probably what you meant by "the
> least surprise and annoyance"?

Yes, exactly. :)

The surprise and annoyance for (1) is that people who get caught up by
the warning while writing new patches say "this warning is stupid, why
don't we disable it". But that is a much smaller number of people to be
annoyed.

> I also still think that any_printf_like_function("%s", "") looks
> silly.  I know that we've already started moving in that direction
> and we stopped at a place we do not want to be in, but perhaps it
> was a mistake to move in that direction in the first place.  I am
> tempted to say we should do something like the attached, but that
> may not fly well, as I suspect that -Wno-format-zero-length may be a
> lot more exotic than the -Wall compiler option.

Yeah, I think portability concerns are what caused us not to go down
that road in the first place. But I think it's also a mistake to put too
much into CFLAGS, because somebody who wants to override one flag has to
repeat the rest. So, e.g., right now I might do:

  make CFLAGS="-O3 -Wall"

to bump the optimization level. But if our codebase relies on
-Wno-format-zero-length being there, then I have to know to put it in
there, too.

You can work around that with an extra make variable, but that also
makes it harder to disable if your compiler doesn't like it.

> An obvious second
> best option would be to drop -Wall from the "everybody" CFLAGS and
> move it to DEVELOPER_CFLAGS instead.

Yeah, though that doesn't help the example above.

As ugly as warning("%s", "") is, I think it may be the thing that annoys
the smallest number of people.

-Peff

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

[David Aguilar]

On Thu, Dec 01, 2016 at 01:50:57PM -0500, Jeff King wrote:
> On Thu, Dec 01, 2016 at 10:20:50AM -0800, Junio C Hamano wrote:
> > I also still think that any_printf_like_function("%s", "") looks
> > silly.  I know that we've already started moving in that direction
> > and we stopped at a place we do not want to be in, but perhaps it
> > was a mistake to move in that direction in the first place.  I am
> > tempted to say we should do something like the attached, but that
> > may not fly well, as I suspect that -Wno-format-zero-length may be a
> > lot more exotic than the -Wall compiler option.
> 
> Yeah, I think portability concerns are what caused us not to go down
> that road in the first place.
> makes it harder to disable if your compiler doesn't like it.
> 
> > An obvious second
> > best option would be to drop -Wall from the "everybody" CFLAGS and
> > move it to DEVELOPER_CFLAGS instead.
> 
> Yeah, though that doesn't help the example above.
> 
> As ugly as warning("%s", "") is, I think it may be the thing that annoys
> the smallest number of people.
> 
> -Peff

How about using warning(" ") instead?

For difftool.c specifically, the following is a fine solution,
and doesn't require that we change our warning flags just for
this one file.
-- 
David

--- 8< ---
From 28bdc381202ced35399cfdf4899a019b0000c7a0 Mon Sep 17 00:00:00 2001
From: David Aguilar <davvid@gmail.com>
Date: Sat, 21 Jan 2017 21:21:09 -0800
Subject: [PATCH] difftool: avoid zero-length format string

The purpose of the warning("") line is to add an empty line to
improve the readability of the message.

Use warning(" ") instead to avoid zero-length format string
warnings while retaining the desired behavior.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 builtin/difftool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 42ad9e804a..d9f8ada291 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(" ");
 				err = 1;
 			} else if (unlink(wtdir.buf) ||
 				   copy_file(wtdir.buf, rdir.buf, st.st_mode))
-- 
2.11.0.747.g28bdc38120

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

[Jeff King]

mn Sat, Jan 21, 2017 at 09:26:08PM -0800, David Aguilar wrote:

> > > An obvious second
> > > best option would be to drop -Wall from the "everybody" CFLAGS and
> > > move it to DEVELOPER_CFLAGS instead.
> > 
> > Yeah, though that doesn't help the example above.
> > 
> > As ugly as warning("%s", "") is, I think it may be the thing that annoys
> > the smallest number of people.
> > 
> > -Peff
> 
> How about using warning(" ") instead?
> 
> For difftool.c specifically, the following is a fine solution,
> and doesn't require that we change our warning flags just for
> this one file.

I dunno. As ugly as the "%s" thing is in the source, at least it doesn't
change the output. Not that an extra space is the end of the world, but
it seems like it's letting the problem escape from the source code.

Do people still care about resolving this? -Wno-format-zero-length is in
the DEVELOPER options. It wasn't clear to me if that was sufficient, or
if we're going to get a bunch of reports from people that need to be
directed to the right compiler options.

The problematic code just hit 'next', so I suppose we'll see soon (OTOH,
the real test is when it get shipped as part of a release).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > As ugly as warning("%s", "") is, I think it may be the thing that annoys
>> > the smallest number of people.
>> > 
>> > -Peff
>> 
>> How about using warning(" ") instead?
>> 
>> For difftool.c specifically, the following is a fine solution,
>> and doesn't require that we change our warning flags just for
>> this one file.
>
> I dunno. As ugly as the "%s" thing is in the source, at least it doesn't
> change the output. Not that an extra space is the end of the world, but
> it seems like it's letting the problem escape from the source code.
>
> Do people still care about resolving this? -Wno-format-zero-length is in
> the DEVELOPER options. It wasn't clear to me if that was sufficient, or
> if we're going to get a bunch of reports from people that need to be
> directed to the right compiler options.

I view both as ugly, but probably "%s", "" is lessor of the two
evils.

Perhaps

	#define JUST_SHOW_EMPTY_LINE "%s", ""

		...
		warning(JUST_SHOW_EMPTY_LINE);
                ...

or something silly like that?

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

[Jeff King]

On Tue, Jan 24, 2017 at 01:52:13PM -0800, Junio C Hamano wrote:

> > I dunno. As ugly as the "%s" thing is in the source, at least it doesn't
> > change the output. Not that an extra space is the end of the world, but
> > it seems like it's letting the problem escape from the source code.
> >
> > Do people still care about resolving this? -Wno-format-zero-length is in
> > the DEVELOPER options. It wasn't clear to me if that was sufficient, or
> > if we're going to get a bunch of reports from people that need to be
> > directed to the right compiler options.
> 
> I view both as ugly, but probably "%s", "" is lessor of the two
> evils.
> 
> Perhaps
> 
> 	#define JUST_SHOW_EMPTY_LINE "%s", ""
> 
> 		...
> 		warning(JUST_SHOW_EMPTY_LINE);
>                 ...
> 
> or something silly like that?

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.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Tue, 24 Jan 2017, Jeff King wrote:

> On Tue, Jan 24, 2017 at 01:52:13PM -0800, Junio C Hamano wrote:
> 
> > > I dunno. As ugly as the "%s" thing is in the source, at least it
> > > doesn't change the output. Not that an extra space is the end of the
> > > world, but it seems like it's letting the problem escape from the
> > > source code.
> > >
> > > Do people still care about resolving this? -Wno-format-zero-length
> > > is in the DEVELOPER options. It wasn't clear to me if that was
> > > sufficient, or if we're going to get a bunch of reports from people
> > > that need to be directed to the right compiler options.
> > 
> > I view both as ugly, but probably "%s", "" is lessor of the two evils.
> > 
> > Perhaps
> > 
> > 	#define JUST_SHOW_EMPTY_LINE "%s", ""
> > 
> > 		...
> > 		warning(JUST_SHOW_EMPTY_LINE);
> >                 ...
> > 
> > or something silly like that?
> 
> 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.

Ciao,
Johannes

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

[Jeff King]

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 also
write a comment. But I am happy if we simply catch it in review (or
preferably the person is clueful enough to read the output of git-blame
and see why it is that way in the first place).

So maybe:

-- >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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The only advantage is that it is self-documenting, so somebody does not
> come through later and convert ("%s", "") back to (""). We could also
> write a comment. But I am happy if we simply catch it in review (or
> preferably the person is clueful enough to read the output of git-blame
> and see why it is that way in the first place).

And the last sentence unfortunatly does not reflect reality.  

I would prefer something self-documenting, like your wrapper with a
comment.  Then somebody who is looking at the implementation of
warning_blank_line() will not get tempted to turn "%s", "" into ""
because of the comment.  And somebody who is looking at the callsite
of warning_blank_line() will think twice before suggesting to turn
it into warning("").

That does not make it unnecessary to review; we still need to catch
those who wants to add new calls to warning("") without even knowing
the presence of warning_blank_line(), if the original codepath being
touched does not have any call to it.

> So maybe:

In any case, the patch is a minimum effort band-aid that lets us
punt on the whole issue for now, so I'll queue it as-is.

Thanks.


> -- >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))

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

[Jeff King]

On Wed, Jan 25, 2017 at 01:28:27PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The only advantage is that it is self-documenting, so somebody does not
> > come through later and convert ("%s", "") back to (""). We could also
> > write a comment. But I am happy if we simply catch it in review (or
> > preferably the person is clueful enough to read the output of git-blame
> > and see why it is that way in the first place).
> 
> And the last sentence unfortunatly does not reflect reality.  
> 
> I would prefer something self-documenting, like your wrapper with a
> comment.  Then somebody who is looking at the implementation of
> warning_blank_line() will not get tempted to turn "%s", "" into ""
> because of the comment.  And somebody who is looking at the callsite
> of warning_blank_line() will think twice before suggesting to turn
> it into warning("").

I am OK with it either way. I was mostly responding to Dscho's
complaint, and I would just like to get this resolved so we never have
to revisit it again. :)

> In any case, the patch is a minimum effort band-aid that lets us
> punt on the whole issue for now, so I'll queue it as-is.

Here's one other option that I came across. Pragmas feel gross, but I
think it will behave as we want, and it doesn't require cooperation from
the callsites at all.

-- >8 --
Subject: [PATCH] disable -Wzero-length-format via #pragma

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.

We disable this warning already with the DEVELOPER Makefile
knob. But we can't unconditionally add -Wno-format-zero-length
to the normal CFLAGS variable, because not all compilers will
understand it. So we may get reports about the warning from
non-developer users who compile with our default of -Wall.

Instead, we can disable the warning using a gcc-specific
#pragma. This should be ignored by non-gcc compilers, and do
what we want for gcc.

I tested also with clang, which often implements gcc
compatible extensions like this. Clang does not generate the
warning in the first place, but also does not complain about
our pragma.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 325950426..a6558930d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -413,6 +413,8 @@ extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+#pragma GCC diagnostic ignored "-Wformat-zero-length"
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
-- 
2.11.0.840.gd37c5973a

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

[Johannes Sixt]

Am 25.01.2017 um 23:01 schrieb Jeff King:
> +#pragma GCC diagnostic ignored "-Wformat-zero-length"

Last time I used #pragma GCC in a cross-platform project, it triggered 
an "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't 
know if the C compiler would also warn.) It would have to be spelled 
like this:

#pragma warning(disable: 4068)   /* MSVC: unknown pragma */
#pragma GCC diagnostic ignored "-Wformat-zero-length"

Dscho mentioned that he's compiling with MSVC. It would do him a favor.

-- Hannes

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

[Johannes Schindelin]

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

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

[Johannes Schindelin]

Hi Hannes,

On Thu, 26 Jan 2017, Johannes Sixt wrote:

> Am 25.01.2017 um 23:01 schrieb Jeff King:
> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Last time I used #pragma GCC in a cross-platform project, it triggered
> an "unknown pragma" warning for MSVC.

It is starting to become a little funny how many ways we can discuss the
resolution of the GCC compiler warning.

And it starts to show: we try to solve the thing in so many ways, just to
avoid the obviously most-trivial patch to change warning(""); to
warning("%s", "") (the change to warning(" "); would change behavior, but
I would be fine with that, too).

I am not really interested in any of these complicated workarounds. If you
gentle people decide they are better in Git's source code, go ahead. I do
not have to like what you are doing, I just have to work with it.

> (It was the C++ compiler, I don't know if the C compiler would also
> warn.) It would have to be spelled like this:
> 
> #pragma warning(disable: 4068)   /* MSVC: unknown pragma */
> #pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Dscho mentioned that he's compiling with MSVC. It would do him a favor.

I am compiling with MSVC, and the idea is to tap into that large number of
Windows developers who Git traditionally has had a really bad time
attracting. From that perspective, I would say it would not only do me a
favor, but anybody who builds Git for Windows using Visual Studio.

But we also have to consider whether it would do anybody a "dis-favor".
#pragma statements are by definition highly dependent on the compiler. I
have no idea whether there are developers out there building Git with
C compilers other than GCC, clang or MSVC (as I did back in the days on
IRIX and HP/UX), but there is quite the potential for problems here [*1*].

To keep Git's source code truly portable, the #pragma would have to be
guarded by a GCC-specific #ifdef ... #endif.

Ciao,
Dscho

Footnote *1*: This is just another instance where a discussion on the Git
mailing list reminds me of
http://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves, as it tries
to avoid an obvious solution by trying to come up with a different
solution that in turn requires additional solutions to additional problems
caused by the alternative solution.

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

[Jeff King]

On Thu, Jan 26, 2017 at 07:39:55AM +0100, Johannes Sixt wrote:

> Am 25.01.2017 um 23:01 schrieb Jeff King:
> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Last time I used #pragma GCC in a cross-platform project, it triggered an
> "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't know if
> the C compiler would also warn.) It would have to be spelled like this:
> 
> #pragma warning(disable: 4068)   /* MSVC: unknown pragma */
> #pragma GCC diagnostic ignored "-Wformat-zero-length"
> 
> Dscho mentioned that he's compiling with MSVC. It would do him a favor.

Bleh. The point of #pragma is to ignore ones you don't know about.

It would be easy to wrap it in an #ifdef for __GNUC__ (there is already
a similar pragma with similar wrapping in the code base).

Anyway. I do not want to make life harder for anyone. I think there are
several options floating around now, so I will let Junio decide which
one he wants to pick up.

-Peff

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

[Jeff King]

On Thu, Jan 26, 2017 at 12:37:46PM +0100, Johannes Schindelin wrote:

> > Am 25.01.2017 um 23:01 schrieb Jeff King:
> > > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
> > 
> > Last time I used #pragma GCC in a cross-platform project, it triggered
> > an "unknown pragma" warning for MSVC.
> 
> It is starting to become a little funny how many ways we can discuss the
> resolution of the GCC compiler warning.
> 
> And it starts to show: we try to solve the thing in so many ways, just to
> avoid the obviously most-trivial patch to change warning(""); to
> warning("%s", "") (the change to warning(" "); would change behavior, but
> I would be fine with that, too).

The point is that the trivial patch fixes _this_ case, but does not
prevent the discussion from happening again later. They are two separate
problems. I am OK not solving the latter one and relying on review (as
I've already said), but the solutions do not do the same thing.

-Peff

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

[Jeff King]

On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:

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

I'm not sure that is a good idea. The options include -Werror, which is
a good thing for developers to respect. But people using older versions
of compilers, or on systems with slightly different header files, may
see extraneous warnings. It's good to fix those warnings, but it is a
big inconvenience to regular users who just want to build and use git.

You could split the DEVELOPER options into two groups, though, and only
enable when (after verifying that it is indeed gcc/clang in use). But
now who is coming up with complicated fixes for the warning("") issue?
:)

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Jan 2017, Jeff King wrote:

> On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:
> 
> > 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.
> 
> I'm not sure that is a good idea. The options include -Werror, which is
> a good thing for developers to respect. But people using older versions
> of compilers, or on systems with slightly different header files, may
> see extraneous warnings. It's good to fix those warnings, but it is a
> big inconvenience to regular users who just want to build and use git.

Yeah, you cannot have the cake and eat it, too: on the one side, we want
Git contributors to see problems early (preferably before even submitting
the patch) and at the same time, we want users who compile their Git
themselves to have no trouble doing so.

> You could split the DEVELOPER options into two groups, though, and only
> enable when (after verifying that it is indeed gcc/clang in use). But
> now who is coming up with complicated fixes for the warning("") issue?
> :)

That is yet another instance of the complicator's glove; I would rather
avoid that.

To me, the obvious solution is to pay more attention to Continuous
Integration, in particular on fixing problems right after they are
reported.

Ciao,
Johannes

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jan 26, 2017 at 07:39:55AM +0100, Johannes Sixt wrote:
>
>> Am 25.01.2017 um 23:01 schrieb Jeff King:
>> > +#pragma GCC diagnostic ignored "-Wformat-zero-length"
>> 
>> Last time I used #pragma GCC in a cross-platform project, it triggered an
>> "unknown pragma" warning for MSVC. (It was the C++ compiler, I don't know if
>> the C compiler would also warn.) It would have to be spelled like this:
>> 
>> #pragma warning(disable: 4068)   /* MSVC: unknown pragma */
>> #pragma GCC diagnostic ignored "-Wformat-zero-length"
>> 
>> Dscho mentioned that he's compiling with MSVC. It would do him a favor.
>
> Bleh. The point of #pragma is to ignore ones you don't know about.

Yes.  Let's not go there; somebody else's compiler will complain
about "#pragma warning(disable: 4068)" that it does not understand.

> Anyway. I do not want to make life harder for anyone. I think there are
> several options floating around now, so I will let Junio decide which
> one he wants to pick up.

Well, I'll keep the "do nothing other than squelching this instance"
to solve one of the two problems for now.  

The other "can we make it harder to make the same issue and reduce
the need to discuss this again on the list?" can be an independent
follow-up patch, and I do have a preference (the "less horrible
version, that is static inline warning_blank_line(void)" you gave us
in <20170124230500.h3fasbvutjkkke5h@sigill.intra.peff.net>), but I
do not think we are in a hurry.

Thanks.