Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

Junio C Hamano [off-list ref] writes:

Do we need to have a check to detect a buggy compiler that takes the
syntax but produces an incorrectly initialized array?  I could add a
test to ensure that HEADER comes out BOLD, etc. (or we may already
have such a test) and then reorder these lines in this patch, if
that is the kind of breakage we anticipate.

So here is a lunch-time hack I did to replace the patch I sent
earlier.  I kind of like the ordering of the elements better than
the original, in that it somehow feels more logical, even though I
merely sorted alphabetically ;-).


 builtin/clean.c              | 19 ++++++++++---------
 t/t7301-clean-interactive.sh | 10 ++++++++++
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 057fc97fe4..e2bb3c69ed 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c

@@ -33,15 +33,6 @@ static const char *msg_skip_git_dir = N_("Skipping repository %s\n");
 static const char *msg_would_skip_git_dir = N_("Would skip repository %s\n");
 static const char *msg_warn_remove_failed = N_("failed to remove %s");
 
-static int clean_use_color = -1;
-static char clean_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RESET,
-	GIT_COLOR_NORMAL,	/* PLAIN */
-	GIT_COLOR_BOLD_BLUE,	/* PROMPT */
-	GIT_COLOR_BOLD,		/* HEADER */
-	GIT_COLOR_BOLD_RED,	/* HELP */
-	GIT_COLOR_BOLD_RED,	/* ERROR */
-};
 enum color_clean {
 	CLEAN_COLOR_RESET = 0,
 	CLEAN_COLOR_PLAIN = 1,

@@ -51,6 +42,16 @@ enum color_clean {
 	CLEAN_COLOR_ERROR = 5
 };
 
+static int clean_use_color = -1;
+static char clean_colors[][COLOR_MAXLEN] = {
+	[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_HEADER] = GIT_COLOR_BOLD,
+	[CLEAN_COLOR_HELP] = GIT_COLOR_BOLD_RED,
+	[CLEAN_COLOR_PLAIN] = GIT_COLOR_NORMAL,
+	[CLEAN_COLOR_PROMPT] = GIT_COLOR_BOLD_BLUE,
+	[CLEAN_COLOR_RESET] = GIT_COLOR_RESET,
+};
+
 #define MENU_OPTS_SINGLETON		01
 #define MENU_OPTS_IMMEDIATE		02
 #define MENU_OPTS_LIST_ONLY		04

diff --git a/t/t7301-clean-interactive.sh b/t/t7301-clean-interactive.sh
index 3ae394e934..556e1850e2 100755
--- a/t/t7301-clean-interactive.sh
+++ b/t/t7301-clean-interactive.sh

@@ -472,4 +472,14 @@ test_expect_success 'git clean -id with prefix and path (ask)' '
 
 '
 
+test_expect_success 'git clean -i paints the header in HEADER color' '
+	>a.out &&
+	echo q |
+	git -c color.ui=always clean -i |
+	test_decode_color |
+	head -n 1 >header &&
+	# not i18ngrep
+	grep "^<BOLD>" header
+'
+
 test_done

This is another test balloon to see if we get complaints from people
whose compilers do not support variables scoped to for loops.

This part of the code base was chosen as it is very old code that does
not change often, such that a potential revert is easy.

Signed-off-by: Stefan Beller <redacted>
---

This is a rather aggressive test ballon, my compiler needed some
good arguments to accept the new world order:

object.c: In function ‘object_array_remove_duplicates’:
object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
  for (unsigned src = 0; src < nr; src++) {
  ^
object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code

Using -std=c99 works for me.

Thanks,
Stefan

 object.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/object.c b/object.c
index f818777412..af26ee2fbc 100644
--- a/object.c
+++ b/object.c

@@ -397,11 +397,11 @@ static int contains_name(struct object_array *array, const char *name)
 
 void object_array_remove_duplicates(struct object_array *array)
 {
-	unsigned nr = array->nr, src;
+	unsigned nr = array->nr;
 	struct object_array_entry *objects = array->objects;
 
 	array->nr = 0;
-	for (src = 0; src < nr; src++) {
+	for (unsigned src = 0; src < nr; src++) {
 		if (!contains_name(array, objects[src].name)) {
 			if (src != array->nr)
 				objects[array->nr] = objects[src];

-- 
2.14.0.rc0.3.g6c2e499285

On 07/19, Stefan Beller wrote:

This is another test balloon to see if we get complaints from people
whose compilers do not support variables scoped to for loops.

This part of the code base was chosen as it is very old code that does
not change often, such that a potential revert is easy.

Signed-off-by: Stefan Beller <redacted>
---

This is a rather aggressive test ballon, my compiler needed some
good arguments to accept the new world order:

object.c: In function ‘object_array_remove_duplicates’:
object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
  for (unsigned src = 0; src < nr; src++) {
  ^
object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code

Using -std=c99 works for me.

This would need a change to the makefile then wouldn't it?

-- 
Brandon Williams

On Wed, Jul 19, 2017 at 11:23:42AM -0700, Brandon Williams wrote:

quoted

object.c: In function ‘object_array_remove_duplicates’:
object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
  for (unsigned src = 0; src < nr; src++) {
  ^
object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code

Using -std=c99 works for me.

This would need a change to the makefile then wouldn't it?

Actually, it complicates things even more, I'd think. We probably can't
just blindly add "-std=c99" to CFLAGS, as not all compilers would
support it (even if they _do_ support this construct).

Interestingly I have no problems compiling it here. I wonder if Stefan's
config.mak is supplying -std=c89 or some other restrictive flag. Or if
his compiler is a different version (though I tried with gcc-6, gcc-4.9,
and clang-3.8).

-Peff

On Mon, Jul 24, 2017 at 10:08 AM, Jeff King [off-list ref] wrote:

On Wed, Jul 19, 2017 at 11:23:42AM -0700, Brandon Williams wrote:

quoted

quoted

object.c: In function ‘object_array_remove_duplicates’:
object.c:404:2: error: ‘for’ loop initial declarations are only allowed in C99 mode
  for (unsigned src = 0; src < nr; src++) {
  ^
object.c:404:2: note: use option -std=c99 or -std=gnu99 to compile your code

Using -std=c99 works for me.

This would need a change to the makefile then wouldn't it?

Actually, it complicates things even more, I'd think. We probably can't
just blindly add "-std=c99" to CFLAGS, as not all compilers would
support it (even if they _do_ support this construct).

Interestingly I have no problems compiling it here. I wonder if Stefan's
config.mak is supplying -std=c89 or some other restrictive flag. Or if
his compiler is a different version (though I tried with gcc-6, gcc-4.9,
and clang-3.8).

Before this patch, I only had
  CFLAGS += -g -O0
in config.mak (as I switched working directories recently), I'll throw in
  DEVELOPER=1

My compiler version is ancient (gcc 4.8.4-2ubuntu1~14.04.3)
apparently (why did I never check in this environment?)

On Mon, Jul 24, 2017 at 10:12:59AM -0700, Stefan Beller wrote:

quoted

Interestingly I have no problems compiling it here. I wonder if Stefan's
config.mak is supplying -std=c89 or some other restrictive flag. Or if
his compiler is a different version (though I tried with gcc-6, gcc-4.9,
and clang-3.8).

Before this patch, I only had
  CFLAGS += -g -O0
in config.mak (as I switched working directories recently), I'll throw in
  DEVELOPER=1

My compiler version is ancient (gcc 4.8.4-2ubuntu1~14.04.3)
apparently (why did I never check in this environment?)

Ah, indeed, it's the compiler version. And I actually screwed up my
gcc-4.9 test. It complains, too. It looks like the default for gcc
bumped from gnu90 to gnu11 in gcc 5.

-Peff