Re: [PATCH v2 01/11] Add and use generic name->id mapping code for color slot parsing

2018-05-29 Thread Stefan Beller 
On Sat, May 26, 2018 at 6:55 AM, Nguyễn Thái Ngọc Duy  wrote:
> Instead of hard coding the name-to-id mapping in C code, keep it in an
> array and use a common function to do the parsing. This reduces code
> and also allows us to list all possible color slots later.
>
> This starts using C99 designated initializers more for convenience
> (the first designated initializers have been introduced in builtin/clean.c
> for some time without complaints)

s/for some time without complaints/
in 512f41cfac5 (clean.c: use designated initializer, 2017-07-14) which
is almost a year without complaint/ maybe?

(just in case a resend is needed; please be precise and mention that
other commit so it's easy to see the reasoning there)

> +
> +int lookup_config(const char **mapping, int nr_mapping, const char *var)
> +{
> +   int i;
> +
> +   for (i = 0; i < nr_mapping; i++) {
> +   const char *name = mapping[i];
> +

if (!name)
break;

maybe? We do we need to check for 'name' in the next
condition? What input do we expect/allow?

> +   if (name && !strcasecmp(var, name))
> +   return i;


> +#define LOOKUP_CONFIG(mapping, var) \
> +   lookup_config(mapping, ARRAY_SIZE(mapping), var)
> +int lookup_config(const char **mapping, int nr_mapping, const char *var);

Can you add a comment here saying what mapping is or should look like?

[PATCH v2 01/11] Add and use generic name->id mapping code for color slot parsing

2018-05-26 Thread Nguyễn Thái Ngọc Duy 
Instead of hard coding the name-to-id mapping in C code, keep it in an
array and use a common function to do the parsing. This reduces code
and also allows us to list all possible color slots later.

This starts using C99 designated initializers more for convenience
(the first designated initializers have been introduced in builtin/clean.c
for some time without complaints)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/branch.c | 28 +
 builtin/clean.c  | 28 +
 builtin/commit.c | 33 ++
 config.c | 13 
 config.h |  4 
 diff.c   | 53 +++-
 log-tree.c   | 35 
 7 files changed, 82 insertions(+), 112 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index f85d3de531..09232576b6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -55,26 +55,18 @@ enum color_branch {
BRANCH_COLOR_UPSTREAM = 5
 };
 
+static const char *color_branch_slots[] = {
+   [BRANCH_COLOR_RESET]= "reset",
+   [BRANCH_COLOR_PLAIN]= "plain",
+   [BRANCH_COLOR_REMOTE]   = "remote",
+   [BRANCH_COLOR_LOCAL]= "local",
+   [BRANCH_COLOR_CURRENT]  = "current",
+   [BRANCH_COLOR_UPSTREAM] = "upstream",
+};
+
 static struct string_list output = STRING_LIST_INIT_DUP;
 static unsigned int colopts;
 
-static int parse_branch_color_slot(const char *slot)
-{
-   if (!strcasecmp(slot, "plain"))
-   return BRANCH_COLOR_PLAIN;
-   if (!strcasecmp(slot, "reset"))
-   return BRANCH_COLOR_RESET;
-   if (!strcasecmp(slot, "remote"))
-   return BRANCH_COLOR_REMOTE;
-   if (!strcasecmp(slot, "local"))
-   return BRANCH_COLOR_LOCAL;
-   if (!strcasecmp(slot, "current"))
-   return BRANCH_COLOR_CURRENT;
-   if (!strcasecmp(slot, "upstream"))
-   return BRANCH_COLOR_UPSTREAM;
-   return -1;
-}
-
 static int git_branch_config(const char *var, const char *value, void *cb)
 {
const char *slot_name;
@@ -86,7 +78,7 @@ static int git_branch_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (skip_prefix(var, "color.branch.", &slot_name)) {
-   int slot = parse_branch_color_slot(slot_name);
+   int slot = LOOKUP_CONFIG(color_branch_slots, slot_name);
if (slot < 0)
return 0;
if (!value)
diff --git a/builtin/clean.c b/builtin/clean.c
index fad533a0a7..0ccd95e693 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -42,6 +42,15 @@ enum color_clean {
CLEAN_COLOR_ERROR = 5
 };
 
+static const char *color_interactive_slots[] = {
+   [CLEAN_COLOR_ERROR]  = "error",
+   [CLEAN_COLOR_HEADER] = "header",
+   [CLEAN_COLOR_HELP]   = "help",
+   [CLEAN_COLOR_PLAIN]  = "plain",
+   [CLEAN_COLOR_PROMPT] = "prompt",
+   [CLEAN_COLOR_RESET]  = "reset",
+};
+
 static int clean_use_color = -1;
 static char clean_colors[][COLOR_MAXLEN] = {
[CLEAN_COLOR_ERROR] = GIT_COLOR_BOLD_RED,
@@ -82,23 +91,6 @@ struct menu_stuff {
void *stuff;
 };
 
-static int parse_clean_color_slot(const char *var)
-{
-   if (!strcasecmp(var, "reset"))
-   return CLEAN_COLOR_RESET;
-   if (!strcasecmp(var, "plain"))
-   return CLEAN_COLOR_PLAIN;
-   if (!strcasecmp(var, "prompt"))
-   return CLEAN_COLOR_PROMPT;
-   if (!strcasecmp(var, "header"))
-   return CLEAN_COLOR_HEADER;
-   if (!strcasecmp(var, "help"))
-   return CLEAN_COLOR_HELP;
-   if (!strcasecmp(var, "error"))
-   return CLEAN_COLOR_ERROR;
-   return -1;
-}
-
 static int git_clean_config(const char *var, const char *value, void *cb)
 {
const char *slot_name;
@@ -113,7 +105,7 @@ static int git_clean_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (skip_prefix(var, "color.interactive.", &slot_name)) {
-   int slot = parse_clean_color_slot(slot_name);
+   int slot = LOOKUP_CONFIG(color_interactive_slots, slot_name);
if (slot < 0)
return 0;
if (!value)
diff --git a/builtin/commit.c b/builtin/commit.c
index a842fea666..2b43a6c48a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -66,6 +66,18 @@ N_("If you wish to skip this commit, use:\n"
 "Then \"git cherry-pick --continue\" will resume cherry-picking\n"
 "the remaining commits.\n");
 
+static const char *color_status_slots[] = {
+   [WT_STATUS_HEADER]= "header",
+   [WT_STATUS_UPDATED]   = "updated",
+   [WT_STATUS_CHANGED]   = "changed",
+   [WT_STATUS_UNTRACKED] = "untracked",
+   [WT_STATUS_NOBRANCH]  = "noBranch",
+   [WT_STATUS_UNMERGED]  = "unmerged",
+   [WT_STATUS_LOCA