Re: [PATCH v2 2/3] trailers: export action enums and corresponding lookup functions

[Junio C Hamano]

Christian Couder  writes:

> On Thu, Jul 13, 2017 at 12:21 AM, Paolo Bonzini  wrote:
>
>> diff --git a/trailer.h b/trailer.h
>> index e90ba1270..f306bf059 100644
>> --- a/trailer.h
>> +++ b/trailer.h
>> @@ -1,11 +1,33 @@
>>  #ifndef TRAILER_H
>>  #define TRAILER_H
>>
>> +enum action_where {
>> +   WHERE_END,
>> +   WHERE_AFTER,
>> +   WHERE_BEFORE,
>> +   WHERE_START
>> +};
>> +enum action_if_exists {
>> +   EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
>> +   EXISTS_ADD_IF_DIFFERENT,
>> +   EXISTS_ADD,
>> +   EXISTS_REPLACE,
>> +   EXISTS_DO_NOTHING
>> +};
>> +enum action_if_missing {
>> +   MISSING_ADD,
>> +   MISSING_DO_NOTHING
>> +};
>
> As these enums are now in trailer.h, maybe more specific names like
> "trailer_action_where" instead of "action_where" would be better.
>
>>  struct trailer_opts {
>> int in_place;
>> int trim_empty;
>>  };
>>
>> +int set_where(enum action_where *item, const char *value);
>> +int set_if_exists(enum action_if_exists *item, const char *value);
>> +int set_if_missing(enum action_if_missing *item, const char *value);
>
> "trailer_" should perhaps be added at the beginning of the names of
> the above functions too.

All sensible suggestions.  Thanks.

Re: [PATCH v2 2/3] trailers: export action enums and corresponding lookup functions

[Christian Couder]

On Thu, Jul 13, 2017 at 12:21 AM, Paolo Bonzini  wrote:

> diff --git a/trailer.h b/trailer.h
> index e90ba1270..f306bf059 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -1,11 +1,33 @@
>  #ifndef TRAILER_H
>  #define TRAILER_H
>
> +enum action_where {
> +   WHERE_END,
> +   WHERE_AFTER,
> +   WHERE_BEFORE,
> +   WHERE_START
> +};
> +enum action_if_exists {
> +   EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
> +   EXISTS_ADD_IF_DIFFERENT,
> +   EXISTS_ADD,
> +   EXISTS_REPLACE,
> +   EXISTS_DO_NOTHING
> +};
> +enum action_if_missing {
> +   MISSING_ADD,
> +   MISSING_DO_NOTHING
> +};

As these enums are now in trailer.h, maybe more specific names like
"trailer_action_where" instead of "action_where" would be better.

>  struct trailer_opts {
> int in_place;
> int trim_empty;
>  };
>
> +int set_where(enum action_where *item, const char *value);
> +int set_if_exists(enum action_if_exists *item, const char *value);
> +int set_if_missing(enum action_if_missing *item, const char *value);

"trailer_" should perhaps be added at the beginning of the names of
the above functions too.

[PATCH v2 2/3] trailers: export action enums and corresponding lookup functions

[Paolo Bonzini]

From: Paolo Bonzini 

Separate the mechanical changes out of the next patch.  The functions
are changed to take a pointer to enum, because struct conf_info is not
going to be public.

Set the default values explicitly in default_conf_info, since they are
not anymore close to default_conf_info and it's not obvious which
constant has value 0.  With the next patch, in fact, the values will
not be zero anymore!

Signed-off-by: Paolo Bonzini 
---
v1->v2: avoid use of designated initializers

 trailer.c | 48 +++-
 trailer.h | 22 ++
 2 files changed, 45 insertions(+), 25 deletions(-)

diff --git a/trailer.c b/trailer.c
index a3eb42818..a371c7b45 100644
--- a/trailer.c
+++ b/trailer.c
@@ -10,11 +10,6 @@
  * Copyright (c) 2013, 2014 Christian Couder 
  */
 
-enum action_where { WHERE_END, WHERE_AFTER, WHERE_BEFORE, WHERE_START };
-enum action_if_exists { EXISTS_ADD_IF_DIFFERENT_NEIGHBOR, 
EXISTS_ADD_IF_DIFFERENT,
-   EXISTS_ADD, EXISTS_REPLACE, EXISTS_DO_NOTHING };
-enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
-
 struct conf_info {
char *name;
char *key;
@@ -374,44 +369,44 @@ static void process_trailers_lists(struct list_head *head,
}
 }
 
-static int set_where(struct conf_info *item, const char *value)
+int set_where(enum action_where *item, const char *value)
 {
if (!strcasecmp("after", value))
-   item->where = WHERE_AFTER;
+   *item = WHERE_AFTER;
else if (!strcasecmp("before", value))
-   item->where = WHERE_BEFORE;
+   *item = WHERE_BEFORE;
else if (!strcasecmp("end", value))
-   item->where = WHERE_END;
+   *item = WHERE_END;
else if (!strcasecmp("start", value))
-   item->where = WHERE_START;
+   *item = WHERE_START;
else
return -1;
return 0;
 }
 
-static int set_if_exists(struct conf_info *item, const char *value)
+int set_if_exists(enum action_if_exists *item, const char *value)
 {
if (!strcasecmp("addIfDifferent", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT;
+   *item = EXISTS_ADD_IF_DIFFERENT;
else if (!strcasecmp("addIfDifferentNeighbor", value))
-   item->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+   *item = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
else if (!strcasecmp("add", value))
-   item->if_exists = EXISTS_ADD;
+   *item = EXISTS_ADD;
else if (!strcasecmp("replace", value))
-   item->if_exists = EXISTS_REPLACE;
+   *item = EXISTS_REPLACE;
else if (!strcasecmp("doNothing", value))
-   item->if_exists = EXISTS_DO_NOTHING;
+   *item = EXISTS_DO_NOTHING;
else
return -1;
return 0;
 }
 
-static int set_if_missing(struct conf_info *item, const char *value)
+int set_if_missing(enum action_if_missing *item, const char *value)
 {
if (!strcasecmp("doNothing", value))
-   item->if_missing = MISSING_DO_NOTHING;
+   *item = MISSING_DO_NOTHING;
else if (!strcasecmp("add", value))
-   item->if_missing = MISSING_ADD;
+   *item = MISSING_ADD;
else
return -1;
return 0;
@@ -471,15 +466,15 @@ static int git_trailer_default_config(const char 
*conf_key, const char *value, v
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
if (!strcmp(trailer_item, "where")) {
-   if (set_where(_conf_info, value) < 0)
+   if (set_where(_conf_info.where, value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifexists")) {
-   if (set_if_exists(_conf_info, value) < 0)
+   if (set_if_exists(_conf_info.if_exists, value) 
< 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifmissing")) {
-   if (set_if_missing(_conf_info, value) < 0)
+   if (set_if_missing(_conf_info.if_missing, 
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "separators")) {
@@ -533,15 +528,15 @@ static int git_trailer_config(const char *conf_key, const 
char *value, void *cb)
conf->command = xstrdup(value);
break;
case TRAILER_WHERE:
-   if (set_where(conf, value))
+   if