[PATCH] submodule-config: Untangle logic in parse_config

[PATCH] submodule-config: Untangle logic in parse_config

[Stefan Beller]

This improves readability of the parse_config logic by making it more concise.

CC: Eric Sunshine <sunshine@sunshineco.com>
CC: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Eric commented on this during the long series for parallelizing 
 fetching submodules. I realized this can be added as an independant patch
 as it doesn't interfere with the series at all, but rather was needed for the
 next series I planned to put on top (parallelize git submodule). I need to
 rethink the next series anyway, so I extracted this patch as a useful
 readability improvement. 
 
 submodule-config.c | 74 +++++++++++++++++++++---------------------------------
 1 file changed, 29 insertions(+), 45 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 393de53..afe0ea8 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -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);
 
 	if (!strcmp(item.buf, "path")) {
-		struct strbuf path = STRBUF_INIT;
-		if (!value) {
+		if (!value)
 			ret = config_error_nonbool(var);
-			goto release_return;
-		}
-		if (!me->overwrite && submodule->path != NULL) {
+		else if (!me->overwrite && submodule->path != NULL)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"path");
-			goto release_return;
+		else {
+			if (submodule->path)
+				cache_remove_path(me->cache, submodule);
+			free((void *) submodule->path);
+			submodule->path = xstrdup(value);
+			cache_put_path(me->cache, submodule);
 		}
-
-		if (submodule->path)
-			cache_remove_path(me->cache, submodule);
-		free((void *) submodule->path);
-		strbuf_addstr(&path, value);
-		submodule->path = strbuf_detach(&path, NULL);
-		cache_put_path(me->cache, submodule);
 	} else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
 		/* when parsing worktree configurations we can die early */
 		int die_on_error = is_null_sha1(me->gitmodules_sha1);
 		if (!me->overwrite &&
-		    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {
+		    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"fetchrecursesubmodules");
-			goto release_return;
-		}
-
-		submodule->fetch_recurse = parse_fetch_recurse(var, value,
+		else
+			submodule->fetch_recurse = parse_fetch_recurse(
+								var, value,
 								die_on_error);
 	} 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;
-		}
-		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:
 	strbuf_release(&name);
 	strbuf_release(&item);
 
-- 
2.5.0.274.g94829aa.dirty

Re: [PATCH] submodule-config: Untangle logic in parse_config

[Junio C Hamano]

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.

> @@ -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);

[PATCHv2] submodule-config: Shorten logic in parse_config

[Stefan Beller]

This makes the parsing more concise by removing the forward goto as well
as unifying the structure of parsing the {ignore, url, path} options.
By unifying we introduce subtle changes in the error cases. We notice
non-boolean variables before noticing duplicate variables now.

CC: Eric Sunshine <sunshine@sunshineco.com>
CC: Heiko Voigt <hvoigt@hvoigt.net>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 69 ++++++++++++++++++++----------------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 393de53..96f1a0b 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -261,74 +261,57 @@ static int parse_config(const char *var, const char *value, void *data)
 			name.buf);
 
 	if (!strcmp(item.buf, "path")) {
-		struct strbuf path = STRBUF_INIT;
-		if (!value) {
+		if (!value)
 			ret = config_error_nonbool(var);
-			goto release_return;
-		}
-		if (!me->overwrite && submodule->path != NULL) {
+		else if (!me->overwrite && submodule->path != NULL)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"path");
-			goto release_return;
+		else {
+			if (submodule->path)
+				cache_remove_path(me->cache, submodule);
+			free((void *) submodule->path);
+			submodule->path = xstrdup(value);
+			cache_put_path(me->cache, submodule);
 		}
-
-		if (submodule->path)
-			cache_remove_path(me->cache, submodule);
-		free((void *) submodule->path);
-		strbuf_addstr(&path, value);
-		submodule->path = strbuf_detach(&path, NULL);
-		cache_put_path(me->cache, submodule);
 	} else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
 		/* when parsing worktree configurations we can die early */
 		int die_on_error = is_null_sha1(me->gitmodules_sha1);
 		if (!me->overwrite &&
-		    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE) {
+		    submodule->fetch_recurse != RECURSE_SUBMODULES_NONE)
 			warn_multiple_config(me->commit_sha1, submodule->name,
 					"fetchrecursesubmodules");
-			goto release_return;
-		}
-
-		submodule->fetch_recurse = parse_fetch_recurse(var, value,
+		else
+			submodule->fetch_recurse = parse_fetch_recurse(
+								var, value,
 								die_on_error);
 	} 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;
-		}
-		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:
 	strbuf_release(&name);
 	strbuf_release(&item);
 
-- 
2.5.0.267.g8d6e698.dirty