Stefan Beller <sbeller@xxxxxxxxxx> 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.
> @@ -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.
> ...
> } 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.
> - 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);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html