Re: [PATCH] submodule-config: Untangle logic in parse_config
Stefan Beller <sbeller@google.com> writes:
> This improves readability of the parse_config logic by making it more concise.
This does make it shorter, but "improve readability" is highly subjective and "untangle logic" is content-free without explaining what aspect of the logic is being untangled into what other shape.
Show 9 quoted lines
> @@ -257,78 +257,62 @@ static int parse_config(const char *var, const char *value, void *data)
> if (!name_and_item_from_var(var, &name, &item))
> return 0;
>
> - submodule = lookup_or_create_by_name(me->cache, me->gitmodules_sha1,
> - name.buf);
> + submodule = lookup_or_create_by_name(me->cache,
> + me->gitmodules_sha1,
> + name.buf);
Just a formatting change, which does make it easier to follow, but does not untangle the logic.
Show 15 quoted lines
> ...
> } else if (!strcmp(item.buf, "ignore")) {
> - struct strbuf ignore = STRBUF_INIT;
> - if (!me->overwrite && submodule->ignore != NULL) {
> + if (!value)
> + ret = config_error_nonbool(var);
> + else if (!me->overwrite && submodule->ignore != NULL)
> warn_multiple_config(me->commit_sha1, submodule->name,
> "ignore");
> - goto release_return;
> - }
> - if (!value) {
> - ret = config_error_nonbool(var);
> - goto release_return;
> - }This is not a faithful conversion, in that we used to complain and abort when seeing multiple values with or without value but now we complain about malformed boolean first. I do not think the difference matters, but it is worth noting in the log, as it is a clean-up that makes the order of checks consistent between ignore and url, if I am reading the patch correctly.
Show 39 quoted lines
> - if (strcmp(value, "untracked") && strcmp(value, "dirty") &&
> - strcmp(value, "all") && strcmp(value, "none")) {
> + else if (strcmp(value, "untracked") &&
> + strcmp(value, "dirty") &&
> + strcmp(value, "all") &&
> + strcmp(value, "none"))
> warning("Invalid parameter '%s' for config option "
> "'submodule.%s.ignore'", value, var);
> - goto release_return;
> + else {
> + free((void *) submodule->ignore);
> + submodule->ignore = xstrdup(value);
> }
> -
> - free((void *) submodule->ignore);
> - strbuf_addstr(&ignore, value);
> - submodule->ignore = strbuf_detach(&ignore, NULL);
> } else if (!strcmp(item.buf, "url")) {
> - struct strbuf url = STRBUF_INIT;
> if (!value) {
> ret = config_error_nonbool(var);
> - goto release_return;
> - }
> - if (!me->overwrite && submodule->url != NULL) {
> + } else if (!me->overwrite && submodule->url != NULL) {
> warn_multiple_config(me->commit_sha1, submodule->name,
> "url");
> - goto release_return;
> + } else {
> + free((void *) submodule->url);
> + submodule->url = xstrdup(value);
> }
> -
> - free((void *) submodule->url);
> - strbuf_addstr(&url, value);
> - submodule->url = strbuf_detach(&url, NULL);
> }
>
> -release_return:So overall, I think there is not much "untangled", but its primary effect is that a forward "goto" to the clean-up is removed by making each component in if/else if/... cascade more independently complete.
Generally, a large piece of code is _easier_ to read with forward "goto"s that jump to the shared clean-up code, as they serve as visual cues that tell the reader "you can stop reading here and ignore the remainder of this if/else if/... cascade". But this function is no too large and removing them does not make the result harder to read, so I am not opposed to this change. If each individual component in if/else if/... cascade grows too large in the future, it can easily become its own helper function.
So the patch looks OK to me, except for the "hmm, the order of checks are made uniform without being documented?" comment above.
Thanks.
> strbuf_release(&name);
> strbuf_release(&item);
Stefan Beller