{"thread":{"id":"36088","subject":"[PATCH v7 03/11] trailer: read and process config information","startedAt":"2014-03-06T22:14:00Z","lastAt":"2014-03-07T22:16:27Z","messageCount":4,"participants":["Christian Couder","Junio C Hamano","David Kastrup"],"isPatch":true,"patchVersion":7,"patchTotal":11},"messages":[{"id":"236240","messageId":"20140306221409.29648.10706.chriscool@tuxfamily.org","threadId":"36088","inReplyTo":null,"subject":"[PATCH v7 03/11] trailer: read and process config information","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2014-03-06T22:14:00Z","receivedAt":"2014-03-06T22:14:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This patch implements reading the configuration\nto get trailer information, and then processing\nit and storing it in a doubly linked list.\n\nThe config information is stored in the list\nwhose first item is pointed to by:\n\nstatic struct trailer_item *first_conf_item;\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n trailer.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 134 insertions(+)\n\ndiff --git a/trailer.c b/trailer.c\nindex 52108c2..7a6d35d 100644\n--- a/trailer.c\n+++ b/trailer.c\n@@ -25,6 +25,8 @@ struct trailer_item {\n \tstruct conf_info conf;\n };\n \n+static struct trailer_item *first_conf_item;\n+\n static int same_token(struct trailer_item *a, struct trailer_item *b, int alnum_len)\n {\n \treturn !strncasecmp(a->token, b->token, alnum_len);\n@@ -245,3 +247,135 @@ static void process_trailers_lists(struct trailer_item **in_tok_first,\n \t\tapply_arg_if_missing(in_tok_first, in_tok_last, arg_tok);\n \t}\n }\n+\n+static int set_where(struct conf_info *item, const char *value)\n+{\n+\tif (!strcasecmp(\"after\", value))\n+\t\titem->where = WHERE_AFTER;\n+\telse if (!strcasecmp(\"before\", value))\n+\t\titem->where = WHERE_BEFORE;\n+\telse\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int set_if_exists(struct conf_info *item, const char *value)\n+{\n+\tif (!strcasecmp(\"addIfDifferent\", value))\n+\t\titem->if_exists = EXISTS_ADD_IF_DIFFERENT;\n+\telse if (!strcasecmp(\"addIfDifferentNeighbor\", value))\n+\t\titem->if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;\n+\telse if (!strcasecmp(\"add\", value))\n+\t\titem->if_exists = EXISTS_ADD;\n+\telse if (!strcasecmp(\"overwrite\", value))\n+\t\titem->if_exists = EXISTS_OVERWRITE;\n+\telse if (!strcasecmp(\"doNothing\", value))\n+\t\titem->if_exists = EXISTS_DO_NOTHING;\n+\telse\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int set_if_missing(struct conf_info *item, const char *value)\n+{\n+\tif (!strcasecmp(\"doNothing\", value))\n+\t\titem->if_missing = MISSING_DO_NOTHING;\n+\telse if (!strcasecmp(\"add\", value))\n+\t\titem->if_missing = MISSING_ADD;\n+\telse\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,\n+\t\t\t TRAILER_IF_EXISTS, TRAILER_IF_MISSING };\n+\n+static int set_name_and_type(const char *conf_key, const char *suffix,\n+\t\t\t     enum trailer_info_type type,\n+\t\t\t     char **pname, enum trailer_info_type *ptype)\n+{\n+\tint ret = ends_with(conf_key, suffix);\n+\tif (ret) {\n+\t\t*pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));\n+\t\t*ptype = type;\n+\t}\n+\treturn ret;\n+}\n+\n+static struct trailer_item *get_conf_item(const char *name)\n+{\n+\tstruct trailer_item *item;\n+\tstruct trailer_item *previous;\n+\n+\t/* Look up item with same name */\n+\tfor (previous = NULL, item = first_conf_item;\n+\t     item;\n+\t     previous = item, item = item->next) {\n+\t\tif (!strcasecmp(item->conf.name, name))\n+\t\t\treturn item;\n+\t}\n+\n+\t/* Item does not already exists, create it */\n+\titem = xcalloc(sizeof(struct trailer_item), 1);\n+\titem->conf.name = xstrdup(name);\n+\n+\tif (!previous)\n+\t\tfirst_conf_item = item;\n+\telse {\n+\t\tprevious->next = item;\n+\t\titem->previous = previous;\n+\t}\n+\n+\treturn item;\n+}\n+\n+static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n+{\n+\tif (starts_with(conf_key, \"trailer.\")) {\n+\t\tconst char *orig_conf_key = conf_key;\n+\t\tstruct trailer_item *item;\n+\t\tstruct conf_info *conf;\n+\t\tchar *name;\n+\t\tenum trailer_info_type type;\n+\n+\t\tconf_key += 8;\n+\t\tif (!set_name_and_type(conf_key, \".key\", TRAILER_KEY, &name, &type) &&\n+\t\t    !set_name_and_type(conf_key, \".command\", TRAILER_COMMAND, &name, &type) &&\n+\t\t    !set_name_and_type(conf_key, \".where\", TRAILER_WHERE, &name, &type) &&\n+\t\t    !set_name_and_type(conf_key, \".ifexists\", TRAILER_IF_EXISTS, &name, &type) &&\n+\t\t    !set_name_and_type(conf_key, \".ifmissing\", TRAILER_IF_MISSING, &name, &type))\n+\t\t\treturn 0;\n+\n+\t\titem = get_conf_item(name);\n+\t\tconf = &item->conf;\n+\t\tfree(name);\n+\n+\t\tswitch (type) {\n+\t\tcase TRAILER_KEY:\n+\t\t\tif (conf->key)\n+\t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n+\t\t\tconf->key = xstrdup(value);\n+\t\t\tbreak;\n+\t\tcase TRAILER_COMMAND:\n+\t\t\tif (conf->command)\n+\t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n+\t\t\tconf->command = xstrdup(value);\n+\t\t\tbreak;\n+\t\tcase TRAILER_WHERE:\n+\t\t\tif (set_where(conf, value))\n+\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n+\t\t\tbreak;\n+\t\tcase TRAILER_IF_EXISTS:\n+\t\t\tif (set_if_exists(conf, value))\n+\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n+\t\t\tbreak;\n+\t\tcase TRAILER_IF_MISSING:\n+\t\t\tif (set_if_missing(conf, value))\n+\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tdie(\"internal bug in trailer.c\");\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n-- \n1.8.5.2.204.gcfe299d.dirty\n"},{"id":"236275","messageId":"xmqqmwh1u2gf.fsf@gitster.dls.corp.google.com","threadId":"36088","inReplyTo":"20140306221409.29648.10706.chriscool@tuxfamily.org","subject":"Re: [PATCH v7 03/11] trailer: read and process config information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-07T21:24:32Z","receivedAt":"2014-03-07T21:24:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> This patch implements reading the configuration\n> to get trailer information, and then processing\n> it and storing it in a doubly linked list.\n\n\"Read and process the ...\", perhaps?\n\n> The config information is stored in the list\n> whose first item is pointed to by:\n>\n> static struct trailer_item *first_conf_item;\n\nIf feels somewhat strange if a doubly linked list has only the first\npointer without the last pointer, unless the previous field of the\nfirst one by convention points at the last one, forming a cycle\n(which I think is a reasonable thing to do, as a desire for a quick\naccess to the top and the bottom is often a reason to use doubly\nlinked list---I didn't check if you implement it that way, though).\n\n> +static int set_where(struct conf_info *item, const char *value)\n> +{\n> +\tif (!strcasecmp(\"after\", value))\n> +\t\titem->where = WHERE_AFTER;\n> +\telse if (!strcasecmp(\"before\", value))\n> +\t\titem->where = WHERE_BEFORE;\n> +\telse\n> +\t\treturn 1;\n\nPlease follow the usual convention of returning a negative value\nupon error, unless there is a compelling reason not to do so.\n\nDo we really want to do \"strcasecmp\" here?  Are some values case\nsensitive and we only allow \"after\", \"AfTer\", \"AFTER\", etc. as case\ninsensitive keywords?  If all values are expected to be case\ninsensitive, wouldn't it be better to downcase in the caller (just\nlike we do in the config parser) and use strcmp() against lower case\nconstants?\n\nAll of the comments applies equally to other functions.\n\n> +enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_WHERE,\n> +\t\t\t TRAILER_IF_EXISTS, TRAILER_IF_MISSING };\n> +\n> +static int set_name_and_type(const char *conf_key, const char *suffix,\n> +\t\t\t     enum trailer_info_type type,\n> +\t\t\t     char **pname, enum trailer_info_type *ptype)\n> +{\n> +\tint ret = ends_with(conf_key, suffix);\n> +\tif (ret) {\n> +\t\t*pname = xstrndup(conf_key, strlen(conf_key) - strlen(suffix));\n> +\t\t*ptype = type;\n> +\t}\n> +\treturn ret;\n> +}\n\nThis implementation, together with the caller that makes many calls\nto it, looks overly inefficient (both runtime- and reviewtime-wise),\ndoesn't it?\n\nHow about having the caller upfront find the last dot before\nchecking the name-and-type to avoid calling ends_with() so many\ntimes unnecessarily?\n\nAlso, wouldn't it be better to make the caller more table-driven,\ni.e.\n\n\tstatic struct {\n        \tconst char *name;\n                enum trailer_kinfo_type type;\n\t} trailer_config_items[] = {\n\t\t{ \"key\", TRAILER_KEY },\n                ...\n        };\n\nin the caller's scope, and then\n\n\tconst char *trailer_item, *variable_name;\n\n        trailer_item = skip_prefix(conf_key, \"trailer.\");\n        if (!trailer_item)\n\t\treturn 0;\n\n\tvariable_name = strrchr(trailer_item, '.');\n        if (!variable_name) {\n\t\t... trailer_item is a two-level variable name.\n        \t... Handle it in whatever way.\n\t\treturn 0;\n        }\n\n        variable_name++;\n        for (i = 0; i < ARRAY_SIZE(trailer_config_items); i++) {\n\t\tif (strcmp(trailer_config_items[i].name, variable_name))\n\t\t\tcontinue;\n\t\tname = xstrdup(...);\n                type = trailer_config_items[i].type;\n\t\tgoto found;\n\t}\n\n        /* Unknown trailer.<item>.variable_name */\n        return 0;\n\nfound:\n\t... do whatever depending on the type ...\n\nor something?\n\n> +static struct trailer_item *get_conf_item(const char *name)\n> +{\n> +\tstruct trailer_item *item;\n> +\tstruct trailer_item *previous;\n> +\n> +\t/* Look up item with same name */\n> +\tfor (previous = NULL, item = first_conf_item;\n> +\t     item;\n> +\t     previous = item, item = item->next) {\n> +\t\tif (!strcasecmp(item->conf.name, name))\n> +\t\t\treturn item;\n> +\t}\n> +\n> +\t/* Item does not already exists, create it */\n> +\titem = xcalloc(sizeof(struct trailer_item), 1);\n> +\titem->conf.name = xstrdup(name);\n> +\n> +\tif (!previous)\n> +\t\tfirst_conf_item = item;\n> +\telse {\n> +\t\tprevious->next = item;\n> +\t\titem->previous = previous;\n> +\t}\n> +\n> +\treturn item;\n> +}\n> +\n> +static int git_trailer_config(const char *conf_key, const char *value, void *cb)\n> +{\n> +\tif (starts_with(conf_key, \"trailer.\")) {\n> +\t\tconst char *orig_conf_key = conf_key;\n> +\t\tstruct trailer_item *item;\n> +\t\tstruct conf_info *conf;\n> +\t\tchar *name;\n> +\t\tenum trailer_info_type type;\n> +\n> +\t\tconf_key += 8;\n> +\t\tif (!set_name_and_type(conf_key, \".key\", TRAILER_KEY, &name, &type) &&\n> +\t\t    !set_name_and_type(conf_key, \".command\", TRAILER_COMMAND, &name, &type) &&\n> +\t\t    !set_name_and_type(conf_key, \".where\", TRAILER_WHERE, &name, &type) &&\n> +\t\t    !set_name_and_type(conf_key, \".ifexists\", TRAILER_IF_EXISTS, &name, &type) &&\n> +\t\t    !set_name_and_type(conf_key, \".ifmissing\", TRAILER_IF_MISSING, &name, &type))\n> +\t\t\treturn 0;\n> +\n> +\t\titem = get_conf_item(name);\n> +\t\tconf = &item->conf;\n> +\t\tfree(name);\n> +\n> +\t\tswitch (type) {\n> +\t\tcase TRAILER_KEY:\n> +\t\t\tif (conf->key)\n> +\t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n> +\t\t\tconf->key = xstrdup(value);\n> +\t\t\tbreak;\n> +\t\tcase TRAILER_COMMAND:\n> +\t\t\tif (conf->command)\n> +\t\t\t\twarning(_(\"more than one %s\"), orig_conf_key);\n> +\t\t\tconf->command = xstrdup(value);\n> +\t\t\tbreak;\n> +\t\tcase TRAILER_WHERE:\n> +\t\t\tif (set_where(conf, value))\n> +\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n> +\t\t\tbreak;\n> +\t\tcase TRAILER_IF_EXISTS:\n> +\t\t\tif (set_if_exists(conf, value))\n> +\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n> +\t\t\tbreak;\n> +\t\tcase TRAILER_IF_MISSING:\n> +\t\t\tif (set_if_missing(conf, value))\n> +\t\t\t\twarning(_(\"unknown value '%s' for key '%s'\"), value, orig_conf_key);\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tdie(\"internal bug in trailer.c\");\n> +\t\t}\n> +\t}\n> +\treturn 0;\n> +}\n"},{"id":"236278","messageId":"87ob1hbrfp.fsf@fencepost.gnu.org","threadId":"36088","inReplyTo":"xmqqmwh1u2gf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v7 03/11] trailer: read and process config information","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-07T21:59:54Z","receivedAt":"2014-03-07T21:59:54Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n>\n>> This patch implements reading the configuration\n>> to get trailer information, and then processing\n>> it and storing it in a doubly linked list.\n>\n> \"Read and process the ...\", perhaps?\n>\n>> The config information is stored in the list\n>> whose first item is pointed to by:\n>>\n>> static struct trailer_item *first_conf_item;\n>\n> If feels somewhat strange if a doubly linked list has only the first\n> pointer without the last pointer, unless the previous field of the\n> first one by convention points at the last one, forming a cycle\n> (which I think is a reasonable thing to do, as a desire for a quick\n> access to the top and the bottom is often a reason to use doubly\n> linked list---I didn't check if you implement it that way, though).\n\nCan't say I agree here.  Basically all my doubly-linked lists are not\nfor traversing data forwards and backwards but for making it possible to\ndelete list members that have not been reached by list traversal but\nrather by orthogonal data access methods.  Consequently, my back links\nusually don't point to the previous list member (which would require\nspecial-casing the first element) but rather to its referring\nforward-pointing link (which for the first list element means a pointer\nto the list head).\n\nHaving a \"last\" pointer is an orthogonal concept: you need it if you\nwant to append to a list's end without actually looking at its members.\nThe presence of such a last pointer actually makes it quite more ugly to\ndelete the last member of a doubly-linked list reached by some other\nmeans as you then need to have some way of adjusting the tail pointer\naccordingly.\n\n-- \nDavid Kastrup\n"},{"id":"236279","messageId":"xmqq4n39u01w.fsf@gitster.dls.corp.google.com","threadId":"36088","inReplyTo":"87ob1hbrfp.fsf@fencepost.gnu.org","subject":"Re: [PATCH v7 03/11] trailer: read and process config information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-07T22:16:27Z","receivedAt":"2014-03-07T22:16:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Kastrup <dak@gnu.org> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Christian Couder <chriscool@tuxfamily.org> writes:\n>>\n>>> The config information is stored in the list\n>>> whose first item is pointed to by:\n>>>\n>>> static struct trailer_item *first_conf_item;\n>>\n>> If feels somewhat strange ...\n>\n> Can't say I agree here.  Basically all my doubly-linked lists are not\n> for traversing data forwards and backwards ...\n> Having a \"last\" pointer is an orthogonal concept ...\n\nYeah, that is where \"somewhat strange\", not \"wrong\", comes from ;-)\n"}]}