Re: [PATCH v7 03/11] trailer: read and process config information
Christian Couder chrisc...@tuxfamily.org writes:
This patch implements reading the configuration
to get trailer information, and then processing
it and storing it in a doubly linked list.
Read and process the ..., perhaps?
The config information is stored in the list
whose first item is pointed to by:
static struct trailer_item *first_conf_item;
If feels somewhat strange if a doubly linked list has only the first
pointer without the last pointer, unless the previous field of the
first one by convention points at the last one, forming a cycle
(which I think is a reasonable thing to do, as a desire for a quick
access to the top and the bottom is often a reason to use doubly
linked list---I didn't check if you implement it that way, though).
+static int set_where(struct conf_info *item, const char *value)
+{
+ if (!strcasecmp(after, value))
+ item-where = WHERE_AFTER;
+ else if (!strcasecmp(before, value))
+ item-where = WHERE_BEFORE;
+ else
+ return 1;
Please follow the usual convention of returning a negative value
upon error, unless there is a compelling reason not to do so.
Do we really want to do strcasecmp here? Are some values case
sensitive and we only allow after, AfTer, AFTER, etc. as case
insensitive keywords? If all values are expected to be case
insensitive, wouldn't it be better to downcase in the caller (just
like we do in the config parser) and use strcmp() against lower case
constants?
All of the comments applies equally to other functions.
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+ TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static int set_name_and_type(const char *conf_key, const char *suffix,
+ enum trailer_info_type type,
+ char **pname, enum trailer_info_type *ptype)
+{
+ int ret = ends_with(conf_key, suffix);
+ if (ret) {
+ *pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));
+ *ptype = type;
+ }
+ return ret;
+}
This implementation, together with the caller that makes many calls
to it, looks overly inefficient (both runtime- and reviewtime-wise),
doesn't it?
How about having the caller upfront find the last dot before
checking the name-and-type to avoid calling ends_with() so many
times unnecessarily?
Also, wouldn't it be better to make the caller more table-driven,
i.e.
static struct {
const char *name;
enum trailer_kinfo_type type;
} trailer_config_items[] = {
{ key, TRAILER_KEY },
...
};
in the caller's scope, and then
const char *trailer_item, *variable_name;
trailer_item = skip_prefix(conf_key, trailer.);
if (!trailer_item)
return 0;
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
... trailer_item is a two-level variable name.
... Handle it in whatever way.
return 0;
}
variable_name++;
for (i = 0; i ARRAY_SIZE(trailer_config_items); i++) {
if (strcmp(trailer_config_items[i].name, variable_name))
continue;
name = xstrdup(...);
type = trailer_config_items[i].type;
goto found;
}
/* Unknown trailer.item.variable_name */
return 0;
found:
... do whatever depending on the type ...
or something?
+static struct trailer_item *get_conf_item(const char *name)
+{
+ struct trailer_item *item;
+ struct trailer_item *previous;
+
+ /* Look up item with same name */
+ for (previous = NULL, item = first_conf_item;
+ item;
+ previous = item, item = item-next) {
+ if (!strcasecmp(item-conf.name, name))
+ return item;
+ }
+
+ /* Item does not already exists, create it */
+ item = xcalloc(sizeof(struct trailer_item), 1);
+ item-conf.name = xstrdup(name);
+
+ if (!previous)
+ first_conf_item = item;
+ else {
+ previous-next = item;
+ item-previous = previous;
+ }
+
+ return item;
+}
+
+static int git_trailer_config(const char *conf_key, const char *value, void
*cb)
+{
+ if (starts_with(conf_key, trailer.)) {
+ const char *orig_conf_key = conf_key;
+ struct trailer_item *item;
+ struct conf_info *conf;
+ char *name;
+ enum trailer_info_type type;
+
+ conf_key += 8;
+ if (!set_name_and_type(conf_key, .key, TRAILER_KEY, name,
type)
+ !set_name_and_type(conf_key, .command, TRAILER_COMMAND,
name, type)
+ !set_name_and_type(conf_key, .where, TRAILER_WHERE,
name, type)
+
Re: [PATCH v7 03/11] trailer: read and process config information
Junio C Hamano gits...@pobox.com writes: Christian Couder chrisc...@tuxfamily.org writes: This patch implements reading the configuration to get trailer information, and then processing it and storing it in a doubly linked list. Read and process the ..., perhaps? The config information is stored in the list whose first item is pointed to by: static struct trailer_item *first_conf_item; If feels somewhat strange if a doubly linked list has only the first pointer without the last pointer, unless the previous field of the first one by convention points at the last one, forming a cycle (which I think is a reasonable thing to do, as a desire for a quick access to the top and the bottom is often a reason to use doubly linked list---I didn't check if you implement it that way, though). Can't say I agree here. Basically all my doubly-linked lists are not for traversing data forwards and backwards but for making it possible to delete list members that have not been reached by list traversal but rather by orthogonal data access methods. Consequently, my back links usually don't point to the previous list member (which would require special-casing the first element) but rather to its referring forward-pointing link (which for the first list element means a pointer to the list head). Having a last pointer is an orthogonal concept: you need it if you want to append to a list's end without actually looking at its members. The presence of such a last pointer actually makes it quite more ugly to delete the last member of a doubly-linked list reached by some other means as you then need to have some way of adjusting the tail pointer accordingly. -- David Kastrup -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 03/11] trailer: read and process config information
David Kastrup d...@gnu.org writes: Junio C Hamano gits...@pobox.com writes: Christian Couder chrisc...@tuxfamily.org writes: The config information is stored in the list whose first item is pointed to by: static struct trailer_item *first_conf_item; If feels somewhat strange ... Can't say I agree here. Basically all my doubly-linked lists are not for traversing data forwards and backwards ... Having a last pointer is an orthogonal concept ... Yeah, that is where somewhat strange, not wrong, comes from ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v7 03/11] trailer: read and process config information
This patch implements reading the configuration
to get trailer information, and then processing
it and storing it in a doubly linked list.
The config information is stored in the list
whose first item is pointed to by:
static struct trailer_item *first_conf_item;
Signed-off-by: Christian Couder chrisc...@tuxfamily.org
---
trailer.c | 134 ++
1 file changed, 134 insertions(+)
diff --git a/trailer.c b/trailer.c
index 52108c2..7a6d35d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -25,6 +25,8 @@ struct trailer_item {
struct conf_info conf;
};
+static struct trailer_item *first_conf_item;
+
static int same_token(struct trailer_item *a, struct trailer_item *b, int
alnum_len)
{
return !strncasecmp(a-token, b-token, alnum_len);
@@ -245,3 +247,135 @@ static void process_trailers_lists(struct trailer_item
**in_tok_first,
apply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);
}
}
+
+static int set_where(struct conf_info *item, const char *value)
+{
+ if (!strcasecmp(after, value))
+ item-where = WHERE_AFTER;
+ else if (!strcasecmp(before, value))
+ item-where = WHERE_BEFORE;
+ else
+ return 1;
+ return 0;
+}
+
+static int set_if_exists(struct conf_info *item, const char *value)
+{
+ if (!strcasecmp(addIfDifferent, value))
+ item-if_exists = EXISTS_ADD_IF_DIFFERENT;
+ else if (!strcasecmp(addIfDifferentNeighbor, value))
+ item-if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+ else if (!strcasecmp(add, value))
+ item-if_exists = EXISTS_ADD;
+ else if (!strcasecmp(overwrite, value))
+ item-if_exists = EXISTS_OVERWRITE;
+ else if (!strcasecmp(doNothing, value))
+ item-if_exists = EXISTS_DO_NOTHING;
+ else
+ return 1;
+ return 0;
+}
+
+static int set_if_missing(struct conf_info *item, const char *value)
+{
+ if (!strcasecmp(doNothing, value))
+ item-if_missing = MISSING_DO_NOTHING;
+ else if (!strcasecmp(add, value))
+ item-if_missing = MISSING_ADD;
+ else
+ return 1;
+ return 0;
+}
+
+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,
+TRAILER_IF_EXISTS, TRAILER_IF_MISSING };
+
+static int set_name_and_type(const char *conf_key, const char *suffix,
+enum trailer_info_type type,
+char **pname, enum trailer_info_type *ptype)
+{
+ int ret = ends_with(conf_key, suffix);
+ if (ret) {
+ *pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));
+ *ptype = type;
+ }
+ return ret;
+}
+
+static struct trailer_item *get_conf_item(const char *name)
+{
+ struct trailer_item *item;
+ struct trailer_item *previous;
+
+ /* Look up item with same name */
+ for (previous = NULL, item = first_conf_item;
+item;
+previous = item, item = item-next) {
+ if (!strcasecmp(item-conf.name, name))
+ return item;
+ }
+
+ /* Item does not already exists, create it */
+ item = xcalloc(sizeof(struct trailer_item), 1);
+ item-conf.name = xstrdup(name);
+
+ if (!previous)
+ first_conf_item = item;
+ else {
+ previous-next = item;
+ item-previous = previous;
+ }
+
+ return item;
+}
+
+static int git_trailer_config(const char *conf_key, const char *value, void
*cb)
+{
+ if (starts_with(conf_key, trailer.)) {
+ const char *orig_conf_key = conf_key;
+ struct trailer_item *item;
+ struct conf_info *conf;
+ char *name;
+ enum trailer_info_type type;
+
+ conf_key += 8;
+ if (!set_name_and_type(conf_key, .key, TRAILER_KEY, name,
type)
+ !set_name_and_type(conf_key, .command, TRAILER_COMMAND,
name, type)
+ !set_name_and_type(conf_key, .where, TRAILER_WHERE,
name, type)
+ !set_name_and_type(conf_key, .ifexists,
TRAILER_IF_EXISTS, name, type)
+ !set_name_and_type(conf_key, .ifmissing,
TRAILER_IF_MISSING, name, type))
+ return 0;
+
+ item = get_conf_item(name);
+ conf = item-conf;
+ free(name);
+
+ switch (type) {
+ case TRAILER_KEY:
+ if (conf-key)
+ warning(_(more than one %s), orig_conf_key);
+ conf-key = xstrdup(value);
+ break;
+ case TRAILER_COMMAND:
+ if (conf-command)
+ warning(_(more than one %s), orig_conf_key);
+
