[RFC PATCH 0/6] Compile cleanly in pedantic mode

[RFC PATCH 0/6] Compile cleanly in pedantic mode

[Beat Bolli]

While developing 6aaded550 ("builtin/config: work around an unsized
array forward declaration", 2018-07-05), I have compiled Git with
CFLAGS="-std=c99 -pedantic".

This is an RFC patch series that fixes a few compiler warnings when
compiling with these options, always assuming that this is a worthwile
goal.

Note that all warnings were produced by -pedantic; the C99 standard
option by itself didn't cause any of them.

The warnings were:

1) Char arrays initialized from a parenthesized string.

        Suppressed by defining USE_PARENS_AROUND_GETTEXT_N to 0
        globally. This was done just to keep the amount of warnings
        manageable; this series leaves that knob alone. The advantage of
        not mistakenly concatenating two translated strings is greater.

2) connect.h, refs/refs-internal.h: Forward reference to an enum.

        Added two #includes that define the enums. This was already
        (inconclusively) talked about in [0].

3) convert.c: Invalid escape sequence "\e".

        Replaced with "\033".

4) seqencer.c: Empty statements at top level.

        Removed the extra semicolons.

5) string-list.c: Forbidden to cast from void * to a function pointer and
   vice versa.

        Encapsulated the function pointer in a context struct. This is
        controversial because it has a performance impact, namely one
        additional pointer dereference per string comparison. An
        alternative might be to use multiple casts via intptr_t. But
        I'm not sure if this is worth the trouble.

6) utf8.c: overflow of char values.

        Use unsigned char for the BOM constants.

This series has patches for 2) to 6).

Regards,
Beat

[0] https://public-inbox.org/git/53ab8626-f862-a732-b369-abeab69a468f@ramsayjones.plus.com/T/

[RFC PATCH 1/6] connect.h: avoid forward declaration of an enum

[Beat Bolli]

Include protocol.h to define enum protocol_version.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 connect.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/connect.h b/connect.h
index 0e69c6709c..c86f862f2f 100644
--- a/connect.h
+++ b/connect.h
@@ -1,6 +1,8 @@
 #ifndef CONNECT_H
 #define CONNECT_H
 
+#include "protocol.h"   /* for enum protocol_version */
+
 #define CONNECT_VERBOSE       (1u << 0)
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
-- 
2.15.0.rc1.299.gda03b47c3

[RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

[Beat Bolli]

Include iterator.h to define enum iterator_selection.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 refs/refs-internal.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd..a78b5cb803 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -1,6 +1,8 @@
 #ifndef REFS_REFS_INTERNAL_H
 #define REFS_REFS_INTERNAL_H
 
+#include "iterator.h"   /* for enum iterator_selection */
+
 /*
  * Data structures and functions for the internal use of the refs
  * module. Code outside of the refs module should use only the public
-- 
2.15.0.rc1.299.gda03b47c3

[RFC PATCH 3/6] convert.c: replace "\e" escapes with "\033".

[Beat Bolli]

The "\e" escape is not defined in ISO C.

While on this line, add a missing space after the comma.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 convert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index 64d0d30e08..edebb946f5 100644
--- a/convert.c
+++ b/convert.c
@@ -334,7 +334,7 @@ static void trace_encoding(const char *context, const char *path,
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(
-			&trace,"| \e[2m%2i:\e[0m %2x \e[2m%c\e[0m%c",
+			&trace, "| \033[2m%2i:\033[0m %2x \033[2m%c\033[0m%c",
 			i,
 			(unsigned char) buf[i],
 			(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
-- 
2.15.0.rc1.299.gda03b47c3

[RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

[Beat Bolli]

The marco GIT_PATH_FUNC expands to a complete statement including the
semicolon. Remove two extra trailing semicolons.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..66e7073995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
  * The file to keep track of how many commands were already processed (e.g.
  * for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
 /*
  * The file to keep track of how many commands are to be processed in total
  * (e.g. for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
 /*
  * The commit message that is planned to be used for any changes that
  * need to be committed following a user interaction.
-- 
2.15.0.rc1.299.gda03b47c3

[RFC PATCH 5/6] string-list.c: avoid conversion from void * to function pointer

[Beat Bolli]

ISO C forbids the conversion of void pointers to function pointers.
Introduce a context struct that encapsulates the function pointer.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 string-list.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/string-list.c b/string-list.c
index a0cf0cfe88..771c455098 100644
--- a/string-list.c
+++ b/string-list.c
@@ -224,18 +224,28 @@ struct string_list_item *string_list_append(struct string_list *list,
 			list->strdup_strings ? xstrdup(string) : (char *)string);
 }
 
+/*
+ * Encapsulate the compare function pointer because ISO C99 forbids
+ * casting from void * to a function pointer and vice versa.
+ */
+struct string_list_sort_ctx
+{
+	compare_strings_fn cmp;
+};
+
 static int cmp_items(const void *a, const void *b, void *ctx)
 {
-	compare_strings_fn cmp = ctx;
+	struct string_list_sort_ctx *sort_ctx = ctx;
 	const struct string_list_item *one = a;
 	const struct string_list_item *two = b;
-	return cmp(one->string, two->string);
+	return sort_ctx->cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-	QSORT_S(list->items, list->nr, cmp_items,
-		list->cmp ? list->cmp : strcmp);
+	struct string_list_sort_ctx sort_ctx = {list->cmp ? list->cmp : strcmp};
+
+	QSORT_S(list->items, list->nr, cmp_items, &sort_ctx);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.15.0.rc1.299.gda03b47c3

[RFC PATCH 6/6] utf8.c: avoid char overflow

[Beat Bolli]

In ISO C, char constants must be in the range -128..127. Change the BOM
constants to unsigned char to avoid overflow.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 utf8.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/utf8.c b/utf8.c
index d55e20c641..833ce00617 100644
--- a/utf8.c
+++ b/utf8.c
@@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
 #endif
 
 static int has_bom_prefix(const char *data, size_t len,
-			  const char *bom, size_t bom_len)
+			  const unsigned char *bom, size_t bom_len)
 {
 	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
 }
 
-static const char utf16_be_bom[] = {0xFE, 0xFF};
-static const char utf16_le_bom[] = {0xFF, 0xFE};
-static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
+static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
+static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
+static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
 
 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {
-- 
2.15.0.rc1.299.gda03b47c3

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

[Eric Sunshine]

On Sun, Jul 8, 2018 at 10:44 AM Beat Bolli <dev+git@drbeat.li> wrote:
> The marco GIT_PATH_FUNC expands to a complete statement including the
> semicolon. Remove two extra trailing semicolons.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

While you're at it, perhaps it would be a good idea to fix the example
in path.h which teaches the "wrong" way:

/*
 * You can define a static memoized git path like:
 *
 *    static GIT_PATH_FUNC(git_path_foo, "FOO");
 *
 * or use one of the global ones below.
 */

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

[Philip Oakley]

From: "Eric Sunshine" <sunshine@sunshineco.com>
To: "Beat Bolli" <dev+git@drbeat.li>
> On Sun, Jul 8, 2018 at 10:44 AM Beat Bolli <dev+git@drbeat.li> wrote:
>> The marco GIT_PATH_FUNC expands to a complete statement including the

s/marco/macro/

>> semicolon. Remove two extra trailing semicolons.
>>
>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>> ---
>>  sequencer.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> While you're at it, perhaps it would be a good idea to fix the example
> in path.h which teaches the "wrong" way:
> 
> /*
> * You can define a static memoized git path like:
> *
> *    static GIT_PATH_FUNC(git_path_foo, "FOO");
> *
> * or use one of the global ones below.
> */
>

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

[ig]

Am 08.07.2018 23:17, schrieb Philip Oakley:
> From: "Eric Sunshine" <sunshine@sunshineco.com>
> To: "Beat Bolli" <dev+git@drbeat.li>
>> On Sun, Jul 8, 2018 at 10:44 AM Beat Bolli <dev+git@drbeat.li> wrote:
>>> The marco GIT_PATH_FUNC expands to a complete statement including the
> 
> s/marco/macro/

ACK. In addition, the whole sentence is wrong: GIT_PATH_FUNC defines a
function, not a statement. Will be fixes in a reroll.

>>> semicolon. Remove two extra trailing semicolons.
>>> 
>>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>>> ---
>>>  sequencer.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> While you're at it, perhaps it would be a good idea to fix the example
>> in path.h which teaches the "wrong" way:
>> 
>> /*
>> * You can define a static memoized git path like:
>> *
>> *    static GIT_PATH_FUNC(git_path_foo, "FOO");
>> *
>> * or use one of the global ones below.
>> */

Thanks,
Beat

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Johannes Schindelin]

Hi Beat,

On Sun, 8 Jul 2018, Beat Bolli wrote:

> In ISO C, char constants must be in the range -128..127. Change the BOM
> constants to unsigned char to avoid overflow.
> 
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  utf8.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/utf8.c b/utf8.c
> index d55e20c641..833ce00617 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
>  #endif
>  
>  static int has_bom_prefix(const char *data, size_t len,
> -			  const char *bom, size_t bom_len)
> +			  const unsigned char *bom, size_t bom_len)
>  {
>  	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
>  }
>  
> -static const char utf16_be_bom[] = {0xFE, 0xFF};
> -static const char utf16_le_bom[] = {0xFF, 0xFE};
> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};

An alternative approach that might be easier to read (and avoids the
confusion arising from our use of (signed) chars for strings pretty much
everywhere):

#define FE ((char)0xfe)
#define FF ((char)0xff)

...

Ciao,
Dscho

Re: [RFC PATCH 0/6] Compile cleanly in pedantic mode

[Johannes Schindelin]

Hi Beat,

On Sun, 8 Jul 2018, Beat Bolli wrote:

> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".
> 
> This is an RFC patch series that fixes a few compiler warnings when
> compiling with these options, always assuming that this is a worthwile
> goal.

Thank you, I think it is safe to remove the RFC label: this is really good
to have, in order to make it as portable C as possible.

Ciao,
Dscho

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Beat Bolli]

Hi Dscho

Am 09.07.2018 15:14, schrieb Johannes Schindelin:
> Hi Beat,
> 
> On Sun, 8 Jul 2018, Beat Bolli wrote:
> 
>> In ISO C, char constants must be in the range -128..127. Change the 
>> BOM
>> constants to unsigned char to avoid overflow.
>> 
>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>> ---
>>  utf8.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/utf8.c b/utf8.c
>> index d55e20c641..833ce00617 100644
>> --- a/utf8.c
>> +++ b/utf8.c
>> @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int 
>> insz,
>>  #endif
>> 
>>  static int has_bom_prefix(const char *data, size_t len,
>> -			  const char *bom, size_t bom_len)
>> +			  const unsigned char *bom, size_t bom_len)
>>  {
>>  	return data && bom && (len >= bom_len) && !memcmp(data, bom, 
>> bom_len);
>>  }
>> 
>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> 
> An alternative approach that might be easier to read (and avoids the
> confusion arising from our use of (signed) chars for strings pretty 
> much
> everywhere):
> 
> #define FE ((char)0xfe)
> #define FF ((char)0xff)
> 
> ...

I have tried this first (without the macros, though), and thought it 
looked
really ugly. That's why I chose this solution. The usage is pretty local 
and
close to function has_bom_prefix().

Would an explaining comment help?

Beat

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Beat Bolli]

Am 09.07.2018 16:48, schrieb Beat Bolli:
> Hi Dscho
> 
> Am 09.07.2018 15:14, schrieb Johannes Schindelin:
>> Hi Beat,
>> 
>> On Sun, 8 Jul 2018, Beat Bolli wrote:
>> 
>>> In ISO C, char constants must be in the range -128..127. Change the 
>>> BOM
>>> constants to unsigned char to avoid overflow.
>>> 
>>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>>> ---
>>>  utf8.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/utf8.c b/utf8.c
>>> index d55e20c641..833ce00617 100644
>>> --- a/utf8.c
>>> +++ b/utf8.c
>>> @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int 
>>> insz,
>>>  #endif
>>> 
>>>  static int has_bom_prefix(const char *data, size_t len,
>>> -			  const char *bom, size_t bom_len)
>>> +			  const unsigned char *bom, size_t bom_len)
>>>  {
>>>  	return data && bom && (len >= bom_len) && !memcmp(data, bom, 
>>> bom_len);
>>>  }
>>> 
>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 
>>> 0xFF};
>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 
>>> 0x00};
>> 
>> An alternative approach that might be easier to read (and avoids the
>> confusion arising from our use of (signed) chars for strings pretty 
>> much
>> everywhere):
>> 
>> #define FE ((char)0xfe)
>> #define FF ((char)0xff)
>> 
>> ...
> 
> I have tried this first (without the macros, though), and thought it 
> looked
> really ugly. That's why I chose this solution. The usage is pretty 
> local and
> close to function has_bom_prefix().
> 
> Would an explaining comment help?

I have found an even simpler solution. Use proper char literals.

I will put this into v2.

Regards,
Beat


diff --git a/utf8.c b/utf8.c
index d55e20c641..982217eec9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -566,10 +566,10 @@ static int has_bom_prefix(const char *data, size_t 
len,
         return data && bom && (len >= bom_len) && !memcmp(data, bom, 
bom_len);
  }

-static const char utf16_be_bom[] = {0xFE, 0xFF};
-static const char utf16_le_bom[] = {0xFF, 0xFE};
-static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const char utf16_be_bom[] = {'\xFE', '\xFF'};
+static const char utf16_le_bom[] = {'\xFF', '\xFE'};
+static const char utf32_be_bom[] = {'\0', '\0', '\xFE', '\xFF'};
+static const char utf32_le_bom[] = {'\xFF', '\xFE', '\0', '\0'};

  int has_prohibited_utf_bom(const char *enc, const char *data, size_t 
len)
  {

Re: [RFC PATCH 0/6] Compile cleanly in pedantic mode

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".

Nice.  I also pretty recently realized that I stopped building with
the pedantic option by accident, probably when DEVELOPER build knob
was added to the main Makefile, and have been disturbed by these
failures.

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>
>> An alternative approach that might be easier to read (and avoids the
>> confusion arising from our use of (signed) chars for strings pretty
>> much
>> everywhere):
>>
>> #define FE ((char)0xfe)
>> #define FF ((char)0xff)
>>
>> ...
>
> I have tried this first (without the macros, though), and thought
> it looked really ugly. That's why I chose this solution. The usage
> is pretty local and close to function has_bom_prefix().

I found that what you posted was already OK, as has_bom_prefix()
appears only locally in this file and that is the only thing that
cares about these foo_bom[] constants.  Casting the elements in
these arrays to (char) type is also fine and not all that ugly,
I think, and between the two (but without the macro) I have no
strong preference.  I wonder if writing them as '\376' and '\377'
as old timers would helps the compiler, though.

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Beat Bolli]

On 09.07.18 18:33, Junio C Hamano wrote:
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>
>>> An alternative approach that might be easier to read (and avoids the
>>> confusion arising from our use of (signed) chars for strings pretty
>>> much
>>> everywhere):
>>>
>>> #define FE ((char)0xfe)
>>> #define FF ((char)0xff)
>>>
>>> ...
>>
>> I have tried this first (without the macros, though), and thought
>> it looked really ugly. That's why I chose this solution. The usage
>> is pretty local and close to function has_bom_prefix().
> 
> I found that what you posted was already OK, as has_bom_prefix()
> appears only locally in this file and that is the only thing that
> cares about these foo_bom[] constants.  Casting the elements in
> these arrays to (char) type is also fine and not all that ugly,
> I think, and between the two (but without the macro) I have no
> strong preference.  I wonder if writing them as '\376' and '\377'
> as old timers would helps the compiler, though.
> 

Yes, it does, as I found out in
https://public-inbox.org/git/e3df2644b59b170e26b2a7c0d3978331@drbeat.li/

But I prefer hex; it's closer to the usual definition of the BOM bytes.

Beat

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Beat Bolli <dev+git@drbeat.li> writes:
>
>>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>
>>> An alternative approach that might be easier to read (and avoids the
>>> confusion arising from our use of (signed) chars for strings pretty
>>> much
>>> everywhere):
>>>
>>> #define FE ((char)0xfe)
>>> #define FF ((char)0xff)
>>>
>>> ...
>>
>> I have tried this first (without the macros, though), and thought
>> it looked really ugly. That's why I chose this solution. The usage
>> is pretty local and close to function has_bom_prefix().
>
> I found that what you posted was already OK, as has_bom_prefix()
> appears only locally in this file and that is the only thing that
> cares about these foo_bom[] constants.  Casting the elements in
> these arrays to (char) type is also fine and not all that ugly,
> I think, and between the two (but without the macro) I have no
> strong preference.  I wonder if writing them as '\376' and '\377'
> as old timers would helps the compiler, though.

Heh, I should have read the remainder of my mailbox to learn that
you came to the same conclusion in response to Dscho's comment
before saying the above.  Hex is OK as all our compilers would be
capable of gloking '\xff' (there is a rather unusual '\x20' already
in builtin/clone.c).

Thanks.

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

[Jeff King]

On Sun, Jul 08, 2018 at 04:43:38PM +0200, Beat Bolli wrote:

> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index dd834314bd..a78b5cb803 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -1,6 +1,8 @@
>  #ifndef REFS_REFS_INTERNAL_H
>  #define REFS_REFS_INTERNAL_H
>  
> +#include "iterator.h"   /* for enum iterator_selection */

IMHO this kind of comment does more harm than good, because it is so
prone to going stale (nobody is going to bother updating it when they
add new dependencies on iterator.h). Anybody who is interested in the
original reason can use "git blame" to dig up your commit message. And
anybody who is thinking about deleting that line would need to dig into
whether anything had been added in the meantime that also requires the
include.

So at best it's redundant, and at worst it's slightly misleading. :)

Not worth a re-roll by itself, but it looked like you had a few other
bits in the other patches to address.

Other than this minor quibble, the whole series looks good to me, modulo
the existing review.

-Peff

[PATCH 0/6] Compile cleanly in pedantic mode

[Beat Bolli]

While developing 6aaded550 ("builtin/config: work around an unsized
array forward declaration", 2018-07-05), I have compiled Git with
CFLAGS="-std=c99 -pedantic".

This series fixes a few compiler warnings when compiling with these
options.

Note that all warnings were produced by -pedantic; the C99 standard
option by itself didn't cause any of them.

The warnings were:

1) Char arrays initialized from a parenthesized string.

        Suppressed by defining USE_PARENS_AROUND_GETTEXT_N to 0
        globally. This was done just to keep the amount of warnings
        manageable; this series leaves that knob alone. The advantage of
        not mistakenly concatenating two translated strings is greater.

2) connect.h, refs/refs-internal.h: Forward reference to an enum.

        Added two #includes that define the enums. This was already
        (inconclusively) talked about in [0].

3) convert.c: Invalid escape sequence "\e".

        Replaced with "\033".

4) sequencer.c: Empty statements at top level.

        Removed the extra semicolons.

5) string-list.c: Forbidden to cast from void * to a function pointer and
   vice versa.

        Encapsulated the function pointer in a context struct. This is
        controversial because it has a performance impact, namely one
        additional pointer dereference per string comparison. An
        alternative might be to use multiple casts via intptr_t. But
        I'm not sure if this is worth the trouble.

6) utf8.c: overflow of char values.

        Used proper char literals for the BOM constants.

This series has patches for 2) to 6).

Regards,
Beat

[0] https://public-inbox.org/git/53ab8626-f862-a732-b369-abeab69a468f@ramsayjones.plus.com/T/


Beat Bolli (6):
  connect.h: avoid forward declaration of an enum
  refs/refs-internal.h: avoid forward declaration of an enum
  convert.c: replace "\e" escapes with "\033".
  sequencer.c: avoid empty statements at top level
  string-list.c: avoid conversion from void * to function pointer
  utf8.c: avoid char overflow

 connect.h            |  2 ++
 convert.c            |  2 +-
 path.h               |  2 +-
 refs/refs-internal.h |  2 ++
 sequencer.c          |  4 ++--
 string-list.c        | 18 ++++++++++++++----
 utf8.c               |  8 ++++----
 7 files changed, 26 insertions(+), 12 deletions(-)


Interdiff from the RFC series:

diff --git a/path.h b/path.h
index 1ccd0373c9..fc9d3487a0 100644
--- a/path.h
+++ b/path.h
@@ -147,7 +147,7 @@ extern void report_linked_checkout_garbage(void);
 /*
  * You can define a static memoized git path like:
  *
- *    static GIT_PATH_FUNC(git_path_foo, "FOO");
+ *    static GIT_PATH_FUNC(git_path_foo, "FOO")
  *
  * or use one of the global ones below.
  */
diff --git a/utf8.c b/utf8.c
index 833ce00617..982217eec9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
 #endif

 static int has_bom_prefix(const char *data, size_t len,
-                         const unsigned char *bom, size_t bom_len)
+                         const char *bom, size_t bom_len)
 {
        return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
 }

-static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
-static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
-static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const char utf16_be_bom[] = {'\xFE', '\xFF'};
+static const char utf16_le_bom[] = {'\xFF', '\xFE'};
+static const char utf32_be_bom[] = {'\0', '\0', '\xFE', '\xFF'};
+static const char utf32_le_bom[] = {'\xFF', '\xFE', '\0', '\0'};

 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {

-- 
2.18.0.203.gfac676dfb9

[PATCH 1/6] connect.h: avoid forward declaration of an enum

[Beat Bolli]

Include protocol.h to define enum protocol_version.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 connect.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/connect.h b/connect.h
index 0e69c6709c..c86f862f2f 100644
--- a/connect.h
+++ b/connect.h
@@ -1,6 +1,8 @@
 #ifndef CONNECT_H
 #define CONNECT_H
 
+#include "protocol.h"   /* for enum protocol_version */
+
 #define CONNECT_VERBOSE       (1u << 0)
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
-- 
2.18.0.203.gfac676dfb9

[PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

[Beat Bolli]

Include iterator.h to define enum iterator_selection.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 refs/refs-internal.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd..a78b5cb803 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -1,6 +1,8 @@
 #ifndef REFS_REFS_INTERNAL_H
 #define REFS_REFS_INTERNAL_H
 
+#include "iterator.h"   /* for enum iterator_selection */
+
 /*
  * Data structures and functions for the internal use of the refs
  * module. Code outside of the refs module should use only the public
-- 
2.18.0.203.gfac676dfb9

[PATCH 3/6] convert.c: replace "\e" escapes with "\033".

[Beat Bolli]

The "\e" escape is not defined in ISO C.

While on this line, add a missing space after the comma.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 convert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index 64d0d30e08..edebb946f5 100644
--- a/convert.c
+++ b/convert.c
@@ -334,7 +334,7 @@ static void trace_encoding(const char *context, const char *path,
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(
-			&trace,"| \e[2m%2i:\e[0m %2x \e[2m%c\e[0m%c",
+			&trace, "| \033[2m%2i:\033[0m %2x \033[2m%c\033[0m%c",
 			i,
 			(unsigned char) buf[i],
 			(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
-- 
2.18.0.203.gfac676dfb9

[PATCH 4/6] sequencer.c: avoid empty statements at top level

[Beat Bolli]

The macro GIT_PATH_FUNC expands to a function definition that ends with
a closing brace. Remove two extra semicolons.

While at it, fix the example in path.h.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 path.h      | 2 +-
 sequencer.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/path.h b/path.h
index 1ccd0373c9..fc9d3487a0 100644
--- a/path.h
+++ b/path.h
@@ -147,7 +147,7 @@ extern void report_linked_checkout_garbage(void);
 /*
  * You can define a static memoized git path like:
  *
- *    static GIT_PATH_FUNC(git_path_foo, "FOO");
+ *    static GIT_PATH_FUNC(git_path_foo, "FOO")
  *
  * or use one of the global ones below.
  */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..66e7073995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
  * The file to keep track of how many commands were already processed (e.g.
  * for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
 /*
  * The file to keep track of how many commands are to be processed in total
  * (e.g. for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
 /*
  * The commit message that is planned to be used for any changes that
  * need to be committed following a user interaction.
-- 
2.18.0.203.gfac676dfb9

[PATCH 5/6] string-list.c: avoid conversion from void * to function pointer

[Beat Bolli]

ISO C forbids the conversion of void pointers to function pointers.
Introduce a context struct that encapsulates the function pointer.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 string-list.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/string-list.c b/string-list.c
index a0cf0cfe88..771c455098 100644
--- a/string-list.c
+++ b/string-list.c
@@ -224,18 +224,28 @@ struct string_list_item *string_list_append(struct string_list *list,
 			list->strdup_strings ? xstrdup(string) : (char *)string);
 }
 
+/*
+ * Encapsulate the compare function pointer because ISO C99 forbids
+ * casting from void * to a function pointer and vice versa.
+ */
+struct string_list_sort_ctx
+{
+	compare_strings_fn cmp;
+};
+
 static int cmp_items(const void *a, const void *b, void *ctx)
 {
-	compare_strings_fn cmp = ctx;
+	struct string_list_sort_ctx *sort_ctx = ctx;
 	const struct string_list_item *one = a;
 	const struct string_list_item *two = b;
-	return cmp(one->string, two->string);
+	return sort_ctx->cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-	QSORT_S(list->items, list->nr, cmp_items,
-		list->cmp ? list->cmp : strcmp);
+	struct string_list_sort_ctx sort_ctx = {list->cmp ? list->cmp : strcmp};
+
+	QSORT_S(list->items, list->nr, cmp_items, &sort_ctx);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.18.0.203.gfac676dfb9

[PATCH 6/6] utf8.c: avoid char overflow

[Beat Bolli]

In ISO C, char constants must be in the range -128..127. Change the BOM
constants to char literals to avoid overflow.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 utf8.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/utf8.c b/utf8.c
index d55e20c641..982217eec9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -566,10 +566,10 @@ static int has_bom_prefix(const char *data, size_t len,
 	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
 }
 
-static const char utf16_be_bom[] = {0xFE, 0xFF};
-static const char utf16_le_bom[] = {0xFF, 0xFE};
-static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const char utf16_be_bom[] = {'\xFE', '\xFF'};
+static const char utf16_le_bom[] = {'\xFF', '\xFE'};
+static const char utf32_be_bom[] = {'\0', '\0', '\xFE', '\xFF'};
+static const char utf32_le_bom[] = {'\xFF', '\xFE', '\0', '\0'};
 
 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {
-- 
2.18.0.203.gfac676dfb9

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

[Beat Bolli]

On 09.07.18 20:46, Jeff King wrote:
> On Sun, Jul 08, 2018 at 04:43:38PM +0200, Beat Bolli wrote:
> 
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index dd834314bd..a78b5cb803 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -1,6 +1,8 @@
>>  #ifndef REFS_REFS_INTERNAL_H
>>  #define REFS_REFS_INTERNAL_H
>>  
>> +#include "iterator.h"   /* for enum iterator_selection */
> 
> IMHO this kind of comment does more harm than good, because it is so
> prone to going stale (nobody is going to bother updating it when they
> add new dependencies on iterator.h). Anybody who is interested in the
> original reason can use "git blame" to dig up your commit message. And
> anybody who is thinking about deleting that line would need to dig into
> whether anything had been added in the meantime that also requires the
> include.
> 
> So at best it's redundant, and at worst it's slightly misleading. :)
> 
> Not worth a re-roll by itself, but it looked like you had a few other
> bits in the other patches to address.
> 
> Other than this minor quibble, the whole series looks good to me, modulo
> the existing review.
> 
> -Peff
> 

Ooosp, I've just sent the non-RFC reroll without this change.

Junio, would you squash this into [1/6] and [2/6], please (if you agree,
of course :-)

Beat

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

[Johannes Schindelin]

Hi Beat,

On Mon, 9 Jul 2018, Beat Bolli wrote:

> Am 09.07.2018 15:14, schrieb Johannes Schindelin:
> > 
> > On Sun, 8 Jul 2018, Beat Bolli wrote:
> > 
> > > In ISO C, char constants must be in the range -128..127. Change the BOM
> > > constants to unsigned char to avoid overflow.
> > > 
> > > Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> > > ---
> > >  utf8.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/utf8.c b/utf8.c
> > > index d55e20c641..833ce00617 100644
> > > --- a/utf8.c
> > > +++ b/utf8.c
> > > @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
> > >  #endif
> > > 
> > > static int has_bom_prefix(const char *data, size_t len,
> > > -			  const char *bom, size_t bom_len)
> > > +			  const unsigned char *bom, size_t bom_len)
> > >  {
> > > 	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
> > >  }
> > > 
> > > -static const char utf16_be_bom[] = {0xFE, 0xFF};
> > > -static const char utf16_le_bom[] = {0xFF, 0xFE};
> > > -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> > > -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> > > +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
> > > +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
> > > +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> > > +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> > 
> > An alternative approach that might be easier to read (and avoids the
> > confusion arising from our use of (signed) chars for strings pretty much
> > everywhere):
> > 
> > #define FE ((char)0xfe)
> > #define FF ((char)0xff)
> > 
> > ...
> 
> I have tried this first (without the macros, though), and thought it looked
> really ugly.

Yep, I would totally agree that it would be very ugly without the macros.

Which is why I suggested the macros instead, in which case it looks
relatively elegant to my eyes.

Ciao,
Dscho

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Beat Bolli]

On 09.07.18 21:25, Beat Bolli wrote:
> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".
> 
> This series fixes a few compiler warnings when compiling with these
> options.

As a small aside, I have also compiled all of Git with -std=c11 using
gcc 8.1. This didn't turn up any new warnings, so we're looking pretty
future-proof in this regard.

Cheers,
Beat

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> The marco GIT_PATH_FUNC expands to a complete statement including the
> semicolon. Remove two extra trailing semicolons.

Wait a bit.  The observation in the log message and the
implementation of GIT_PATH_FUNC() do not match.

        #define GIT_PATH_FUNC(func, filename) \
                const char *func(void) \
                { \
                        static char *ret; \
                        if (!ret) \
                                ret = git_pathdup(filename); \
                        return ret; \
                }

The code generated does "include semicolon" but that is not why the
caller should place semicolon after the closing parens.  Perhaps
replace "including the semicolon." with something else, like ", and
adding a semicolon after it not only is unnecessary but is wrong."
or soemthing like that?

It is a bit unfortunate that we need to live with a slight uglyness
of the resulting source code, unlike e.g. define_commit_slab() that
can (and must) end with a semicolon, which gives us a more natural
look.  But that is a separate issue.

>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..66e7073995 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
>   * The file to keep track of how many commands were already processed (e.g.
>   * for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
> +static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
>  /*
>   * The file to keep track of how many commands are to be processed in total
>   * (e.g. for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
> +static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
>  /*
>   * The commit message that is planned to be used for any changes that
>   * need to be committed following a user interaction.

Re: [PATCH 4/6] sequencer.c: avoid empty statements at top level

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> The macro GIT_PATH_FUNC expands to a function definition that ends with
> a closing brace. Remove two extra semicolons.

Good.  Thanks.

>
> While at it, fix the example in path.h.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  path.h      | 2 +-
>  sequencer.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/path.h b/path.h
> index 1ccd0373c9..fc9d3487a0 100644
> --- a/path.h
> +++ b/path.h
> @@ -147,7 +147,7 @@ extern void report_linked_checkout_garbage(void);
>  /*
>   * You can define a static memoized git path like:
>   *
> - *    static GIT_PATH_FUNC(git_path_foo, "FOO");
> + *    static GIT_PATH_FUNC(git_path_foo, "FOO")
>   *
>   * or use one of the global ones below.
>   */
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..66e7073995 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
>   * The file to keep track of how many commands were already processed (e.g.
>   * for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
> +static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
>  /*
>   * The file to keep track of how many commands are to be processed in total
>   * (e.g. for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
> +static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
>  /*
>   * The commit message that is planned to be used for any changes that
>   * need to be committed following a user interaction.

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

[Beat Bolli]

On 09.07.18 23:34, Junio C Hamano wrote:
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>> The marco GIT_PATH_FUNC expands to a complete statement including the
>> semicolon. Remove two extra trailing semicolons.
> 
> Wait a bit.  The observation in the log message and the
> implementation of GIT_PATH_FUNC() do not match.
> 
>         #define GIT_PATH_FUNC(func, filename) \
>                 const char *func(void) \
>                 { \
>                         static char *ret; \
>                         if (!ret) \
>                                 ret = git_pathdup(filename); \
>                         return ret; \
>                 }
> 
> The code generated does "include semicolon" but that is not why the
> caller should place semicolon after the closing parens.  Perhaps
> replace "including the semicolon." with something else, like ", and
> adding a semicolon after it not only is unnecessary but is wrong."
> or soemthing like that?

This message is fixed in the non-RFC series that I sent at 19:25 UTC. I
noticed the error after the message from Philip Oakley.

Beat

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".

Nicely done.  

With these 6 patches and the USE_PARENCE_AROUND_GETTEXT_N hack, the
forward decl of the unsized static array you dealt with separately
becomes the only remaining violation in the codebase, which is good.

Will queue.  Thanks.

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Beat Bolli]

On 09.07.18 23:45, Junio C Hamano wrote:
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>> While developing 6aaded550 ("builtin/config: work around an unsized
>> array forward declaration", 2018-07-05), I have compiled Git with
>> CFLAGS="-std=c99 -pedantic".
> 
> Nicely done.  
> 
> With these 6 patches and the USE_PARENCE_AROUND_GETTEXT_N hack, the
> forward decl of the unsized static array you dealt with separately
> becomes the only remaining violation in the codebase, which is good.
> 
> Will queue.  Thanks.

Thanks!

Beat

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

[Jeff King]

On Mon, Jul 09, 2018 at 09:30:12PM +0200, Beat Bolli wrote:

> > Other than this minor quibble, the whole series looks good to me, modulo
> > the existing review.
> 
> Ooosp, I've just sent the non-RFC reroll without this change.
> 
> Junio, would you squash this into [1/6] and [2/6], please (if you agree,
> of course :-)

:) I'd be happy with that squash, but not the end of the world if it
doesn't make it. Thanks for the whole series.

-Peff

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Beat Bolli]

Hi Junio

Am 09.07.2018 23:45, schrieb Junio C Hamano:
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>> While developing 6aaded550 ("builtin/config: work around an unsized
>> array forward declaration", 2018-07-05), I have compiled Git with
>> CFLAGS="-std=c99 -pedantic".
> 
> Nicely done.
> 
> With these 6 patches and the USE_PARENCE_AROUND_GETTEXT_N hack, the
> forward decl of the unsized static array you dealt with separately
> becomes the only remaining violation in the codebase, which is good.
> 
> Will queue.  Thanks.

Should we add a "pedantic" flag to DEVOPTS that would simplify building 
pedantically? It would also have to set USE_PARENS_AROUND_GETTEXT_N so 
as to not overwhelm the developer with too much output.

Beat

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Junio C Hamano]

Beat Bolli <dev+git@drbeat.li> writes:

> Should we add a "pedantic" flag to DEVOPTS that would simplify
> building pedantically? It would also have to set
> USE_PARENS_AROUND_GETTEXT_N so as to not overwhelm the developer with
> too much output.

That may be something worth discussing before doing; I'd prefer to
wait until these 6 patches, plus the unsized static array one you
did spearately, graduates to the 'master' branch.

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Johannes Schindelin]

Hi,

On Wed, 11 Jul 2018, Junio C Hamano wrote:

> Beat Bolli <dev+git@drbeat.li> writes:
> 
> > Should we add a "pedantic" flag to DEVOPTS that would simplify
> > building pedantically? It would also have to set
> > USE_PARENS_AROUND_GETTEXT_N so as to not overwhelm the developer with
> > too much output.
> 
> That may be something worth discussing before doing; I'd prefer to
> wait until these 6 patches, plus the unsized static array one you
> did spearately, graduates to the 'master' branch.

If this change to DEVOPTS was done as 7/7, then this would be very easy to
guarantee (or as 2/2 if you want to graduate this here patch series faster
than the static array forward-declaration fix).

Just an idea,
Dscho

Re: [PATCH 0/6] Compile cleanly in pedantic mode

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> That may be something worth discussing before doing; I'd prefer to
>> wait until these 6 patches, plus the unsized static array one you
>> did spearately, graduates to the 'master' branch.
>
> If this change to DEVOPTS was done as 7/7, then this would be very easy to
> guarantee (or as 2/2 if you want to graduate this here patch series faster
> than the static array forward-declaration fix).

It would guarantee that there is *no* period to discuss if it is too
inconvenient for developers between the time these 6 plus 1 graduate
to the master branch (and willing volunteers have chance to try it
themselves) and the change actuallly is forced upon the developers.

That is why I said I'd prefer to wait.

A separate patch that depends on the 6 plus 1 topic that is kept in
'next' longer than the main series is a possibility, though.