{"thread":{"id":"40539","subject":"[PATCH] submodule-config: Untangle logic in parse_config","startedAt":"2015-10-12T17:58:58Z","lastAt":"2015-10-13T00:02:47Z","messageCount":3,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"271464","messageId":"1444672738-2944-1-git-send-email-sbeller@google.com","threadId":"40539","inReplyTo":null,"subject":"[PATCH] submodule-config: Untangle logic in parse_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-12T17:58:58Z","receivedAt":"2015-10-12T17:58:58Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This improves readability of the parse_config logic by making it more concise.\n\nCC: Eric Sunshine <sunshine@sunshineco.com>\nCC: Heiko Voigt <hvoigt@hvoigt.net>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Eric commented on this during the long series for parallelizing \n fetching submodules. I realized this can be added as an independant patch\n as it doesn't interfere with the series at all, but rather was needed for the\n next series I planned to put on top (parallelize git submodule). I need to\n rethink the next series anyway, so I extracted this patch as a useful\n readability improvement. \n \n submodule-config.c | 74 +++++++++++++++++++++---------------------------------\n 1 file changed, 29 insertions(+), 45 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 393de53..afe0ea8 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)\n \tif (!name_and_item_from_var(var, &name, &item))\n \t\treturn 0;\n \n-\tsubmodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,\n-\t\t\tname.buf);\n+\tsubmodule = lookup_or_create_by_name(me->cache,\n+\t\t\t\t\t     me->gitmodules_sha1,\n+\t\t\t\t\t     name.buf);\n \n \tif (!strcmp(item.buf, \"path\")) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tif (!value) {\n+\t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->path != NULL) {\n+\t\telse if (!me->overwrite && submodule->path != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tif (submodule->path)\n+\t\t\t\tcache_remove_path(me->cache, submodule);\n+\t\t\tfree((void *) submodule->path);\n+\t\t\tsubmodule->path = xstrdup(value);\n+\t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\n-\t\tif (submodule->path)\n-\t\t\tcache_remove_path(me->cache, submodule);\n-\t\tfree((void *) submodule->path);\n-\t\tstrbuf_addstr(&path, value);\n-\t\tsubmodule->path = strbuf_detach(&path, NULL);\n-\t\tcache_put_path(me->cache, submodule);\n \t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n-\t\t    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"fetchrecursesubmodules\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\n-\t\tsubmodule->fetch_recurse = parse_fetch_recurse(var, value,\n+\t\telse\n+\t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n+\t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n \t} else if (!strcmp(item.buf, \"ignore\")) {\n-\t\tstruct strbuf ignore = STRBUF_INIT;\n-\t\tif (!me->overwrite && submodule->ignore != NULL) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->ignore != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!value) {\n-\t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n-\t\t    strcmp(value, \"all\") && strcmp(value, \"none\")) {\n+\t\telse if (strcmp(value, \"untracked\") &&\n+\t\t\t strcmp(value, \"dirty\") &&\n+\t\t\t strcmp(value, \"all\") &&\n+\t\t\t strcmp(value, \"none\"))\n \t\t\twarning(\"Invalid parameter '%s' for config option \"\n \t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tfree((void *) submodule->ignore);\n+\t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->ignore);\n-\t\tstrbuf_addstr(&ignore, value);\n-\t\tsubmodule->ignore = strbuf_detach(&ignore, NULL);\n \t} else if (!strcmp(item.buf, \"url\")) {\n-\t\tstruct strbuf url = STRBUF_INIT;\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url != NULL) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n-\t\t\tgoto release_return;\n+\t\t} else {\n+\t\t\tfree((void *) submodule->url);\n+\t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->url);\n-\t\tstrbuf_addstr(&url, value);\n-\t\tsubmodule->url = strbuf_detach(&url, NULL);\n \t}\n \n-release_return:\n \tstrbuf_release(&name);\n \tstrbuf_release(&item);\n \n-- \n2.5.0.274.g94829aa.dirty\n"},{"id":"271479","messageId":"xmqqy4f7lwkf.fsf@gitster.mtv.corp.google.com","threadId":"40539","inReplyTo":"1444672738-2944-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] submodule-config: Untangle logic in parse_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-12T19:24:16Z","receivedAt":"2015-10-12T19:24:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This improves readability of the parse_config logic by making it more concise.\n\nThis does make it shorter, but \"improve readability\" is highly\nsubjective and \"untangle logic\" is content-free without explaining\nwhat aspect of the logic is being untangled into what other shape.\n\n> @@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)\n>  \tif (!name_and_item_from_var(var, &name, &item))\n>  \t\treturn 0;\n>  \n> -\tsubmodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,\n> -\t\t\tname.buf);\n> +\tsubmodule = lookup_or_create_by_name(me->cache,\n> +\t\t\t\t\t     me->gitmodules_sha1,\n> +\t\t\t\t\t     name.buf);\n\nJust a formatting change, which does make it easier to follow, but\ndoes not untangle the logic.\n\n> ...\n>  \t} else if (!strcmp(item.buf, \"ignore\")) {\n> -\t\tstruct strbuf ignore = STRBUF_INIT;\n> -\t\tif (!me->overwrite && submodule->ignore != NULL) {\n> +\t\tif (!value)\n> +\t\t\tret = config_error_nonbool(var);\n> +\t\telse if (!me->overwrite && submodule->ignore != NULL)\n>  \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n>  \t\t\t\t\t\"ignore\");\n> -\t\t\tgoto release_return;\n> -\t\t}\n> -\t\tif (!value) {\n> -\t\t\tret = config_error_nonbool(var);\n> -\t\t\tgoto release_return;\n> -\t\t}\n\nThis is not a faithful conversion, in that we used to complain and\nabort when seeing multiple values with or without value but now we\ncomplain about malformed boolean first.  I do not think the\ndifference matters, but it is worth noting in the log, as it is a\nclean-up that makes the order of checks consistent between ignore\nand url, if I am reading the patch correctly.\n\n> -\t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n> -\t\t    strcmp(value, \"all\") && strcmp(value, \"none\")) {\n> +\t\telse if (strcmp(value, \"untracked\") &&\n> +\t\t\t strcmp(value, \"dirty\") &&\n> +\t\t\t strcmp(value, \"all\") &&\n> +\t\t\t strcmp(value, \"none\"))\n>  \t\t\twarning(\"Invalid parameter '%s' for config option \"\n>  \t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n> -\t\t\tgoto release_return;\n> +\t\telse {\n> +\t\t\tfree((void *) submodule->ignore);\n> +\t\t\tsubmodule->ignore = xstrdup(value);\n>  \t\t}\n> -\n> -\t\tfree((void *) submodule->ignore);\n> -\t\tstrbuf_addstr(&ignore, value);\n> -\t\tsubmodule->ignore = strbuf_detach(&ignore, NULL);\n>  \t} else if (!strcmp(item.buf, \"url\")) {\n> -\t\tstruct strbuf url = STRBUF_INIT;\n>  \t\tif (!value) {\n>  \t\t\tret = config_error_nonbool(var);\n> -\t\t\tgoto release_return;\n> -\t\t}\n> -\t\tif (!me->overwrite && submodule->url != NULL) {\n> +\t\t} else if (!me->overwrite && submodule->url != NULL) {\n>  \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n>  \t\t\t\t\t\"url\");\n> -\t\t\tgoto release_return;\n> +\t\t} else {\n> +\t\t\tfree((void *) submodule->url);\n> +\t\t\tsubmodule->url = xstrdup(value);\n>  \t\t}\n> -\n> -\t\tfree((void *) submodule->url);\n> -\t\tstrbuf_addstr(&url, value);\n> -\t\tsubmodule->url = strbuf_detach(&url, NULL);\n>  \t}\n>  \n> -release_return:\n\nSo overall, I think there is not much \"untangled\", but its primary\neffect is that a forward \"goto\" to the clean-up is removed by making\neach component in if/else if/... cascade more independently complete.\n\nGenerally, a large piece of code is _easier_ to read with forward\n\"goto\"s that jump to the shared clean-up code, as they serve as\nvisual cues that tell the reader \"you can stop reading here and\nignore the remainder of this if/else if/... cascade\".  But this\nfunction is no too large and removing them does not make the\nresult harder to read, so I am not opposed to this change.  If each\nindividual component in if/else if/... cascade grows too large in\nthe future, it can easily become its own helper function.\n\nSo the patch looks OK to me, except for the \"hmm, the order of\nchecks are made uniform without being documented?\" comment above.\n\nThanks.\n\n\n>  \tstrbuf_release(&name);\n>  \tstrbuf_release(&item);\n"},{"id":"271548","messageId":"1444694567-17511-1-git-send-email-sbeller@google.com","threadId":"40539","inReplyTo":"xmqqy4f7lwkf.fsf@gitster.mtv.corp.google.com","subject":"[PATCHv2] submodule-config: Shorten logic in parse_config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-10-13T00:02:47Z","receivedAt":"2015-10-13T00:02:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes the parsing more concise by removing the forward goto as well\nas unifying the structure of parsing the {ignore, url, path} options.\nBy unifying we introduce subtle changes in the error cases. We notice\nnon-boolean variables before noticing duplicate variables now.\n\nCC: Eric Sunshine <sunshine@sunshineco.com>\nCC: Heiko Voigt <hvoigt@hvoigt.net>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule-config.c | 69 ++++++++++++++++++++----------------------------------\n 1 file changed, 26 insertions(+), 43 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 393de53..96f1a0b 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -261,74 +261,57 @@ static int parse_config(const char *var, const char *value, void *data)\n \t\t\tname.buf);\n \n \tif (!strcmp(item.buf, \"path\")) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\t\tif (!value) {\n+\t\tif (!value)\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->path != NULL) {\n+\t\telse if (!me->overwrite && submodule->path != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"path\");\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tif (submodule->path)\n+\t\t\t\tcache_remove_path(me->cache, submodule);\n+\t\t\tfree((void *) submodule->path);\n+\t\t\tsubmodule->path = xstrdup(value);\n+\t\t\tcache_put_path(me->cache, submodule);\n \t\t}\n-\n-\t\tif (submodule->path)\n-\t\t\tcache_remove_path(me->cache, submodule);\n-\t\tfree((void *) submodule->path);\n-\t\tstrbuf_addstr(&path, value);\n-\t\tsubmodule->path = strbuf_detach(&path, NULL);\n-\t\tcache_put_path(me->cache, submodule);\n \t} else if (!strcmp(item.buf, \"fetchrecursesubmodules\")) {\n \t\t/* when parsing worktree configurations we can die early */\n \t\tint die_on_error = is_null_sha1(me->gitmodules_sha1);\n \t\tif (!me->overwrite &&\n-\t\t    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {\n+\t\t    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"fetchrecursesubmodules\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\n-\t\tsubmodule->fetch_recurse = parse_fetch_recurse(var, value,\n+\t\telse\n+\t\t\tsubmodule->fetch_recurse = parse_fetch_recurse(\n+\t\t\t\t\t\t\t\tvar, value,\n \t\t\t\t\t\t\t\tdie_on_error);\n \t} else if (!strcmp(item.buf, \"ignore\")) {\n-\t\tstruct strbuf ignore = STRBUF_INIT;\n-\t\tif (!me->overwrite && submodule->ignore != NULL) {\n+\t\tif (!value)\n+\t\t\tret = config_error_nonbool(var);\n+\t\telse if (!me->overwrite && submodule->ignore != NULL)\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"ignore\");\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!value) {\n-\t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n-\t\t    strcmp(value, \"all\") && strcmp(value, \"none\")) {\n+\t\telse if (strcmp(value, \"untracked\") &&\n+\t\t\t strcmp(value, \"dirty\") &&\n+\t\t\t strcmp(value, \"all\") &&\n+\t\t\t strcmp(value, \"none\"))\n \t\t\twarning(\"Invalid parameter '%s' for config option \"\n \t\t\t\t\t\"'submodule.%s.ignore'\", value, var);\n-\t\t\tgoto release_return;\n+\t\telse {\n+\t\t\tfree((void *) submodule->ignore);\n+\t\t\tsubmodule->ignore = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->ignore);\n-\t\tstrbuf_addstr(&ignore, value);\n-\t\tsubmodule->ignore = strbuf_detach(&ignore, NULL);\n \t} else if (!strcmp(item.buf, \"url\")) {\n-\t\tstruct strbuf url = STRBUF_INIT;\n \t\tif (!value) {\n \t\t\tret = config_error_nonbool(var);\n-\t\t\tgoto release_return;\n-\t\t}\n-\t\tif (!me->overwrite && submodule->url != NULL) {\n+\t\t} else if (!me->overwrite && submodule->url != NULL) {\n \t\t\twarn_multiple_config(me->commit_sha1, submodule->name,\n \t\t\t\t\t\"url\");\n-\t\t\tgoto release_return;\n+\t\t} else {\n+\t\t\tfree((void *) submodule->url);\n+\t\t\tsubmodule->url = xstrdup(value);\n \t\t}\n-\n-\t\tfree((void *) submodule->url);\n-\t\tstrbuf_addstr(&url, value);\n-\t\tsubmodule->url = strbuf_detach(&url, NULL);\n \t}\n \n-release_return:\n \tstrbuf_release(&name);\n \tstrbuf_release(&item);\n \n-- \n2.5.0.267.g8d6e698.dirty\n"}]}