[v5,5/8] t/helper/test-config: unify exit labels

Commit Message

Matheus Tavares Sept. 2, 2020, 6:17 a.m. UTC

test-config's main function has three different exit labels, all of
which have to perform the same cleanup code before returning. Unify the
labels in preparation for the next patch which will increase the cleanup
section.

Signed-off-by: Matheus Tavares <[e-mail]>
---
 t/helper/test-config.c | 51 +++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 31 deletions(-)

Comments

Eric Sunshine Sept. 2, 2020, 7:30 a.m. UTC | #1

On Wed, Sep 2, 2020 at 2:18 AM Matheus Tavares
<[e-mail]> wrote:
> test-config's main function has three different exit labels, all of
> which have to perform the same cleanup code before returning. Unify the
> labels in preparation for the next patch which will increase the cleanup
> section.
>
> Signed-off-by: Matheus Tavares <[e-mail]>
> ---
> diff --git a/t/helper/test-config.c b/t/helper/test-config.c
> @@ -69,16 +69,19 @@ static int early_config_cb(const char *var, const char *value, void *vdata)
> +#define TC_VALUE_NOT_FOUND 1
> +#define TC_CONFIG_FILE_ERROR 2
> +
>  int cmd__config(int argc, const char **argv)
>  {
> +       int i, val, ret = 0;
>
>         if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
>                 read_early_config(early_config_cb, (void *)argv[2]);
> -               return 0;
> +               return ret;
>         }

This one change feels both fragile and increases cognitive load, thus
does not seem desirable. It feels fragile because someone could come
along and insert code above this conditional which assigns some value
other than 0 to 'ret' (not realizing that this conditional wants to
return 0), thus breaking it. It increases cognitive load because it
forces the reader to scan all the code leading up to this point to
determine what value this conditional really wants to return.

Nevertheless, this is a minor objection, not necessarily worth a re-roll.

> @@ -94,10 +97,9 @@ int cmd__config(int argc, const char **argv)
>                         printf("Value not found for \"%s\"\n", argv[2]);
> -                       goto exit1;
> +                       ret = TC_VALUE_NOT_FOUND;

This sort of change, on the other hand, does not increase cognitive
load because it's quite obvious what return value this conditional
wants to return (because it's assigning it explicitly).

Patch

diff --git a/t/helper/test-config.c b/t/helper/test-config.c
index 26d9c2ac4c..8fe43e9775 100644
--- a/t/helper/test-config.c
+++ b/t/helper/test-config.c
@@ -69,16 +69,19 @@  static int early_config_cb(const char *var, const char *value, void *vdata)
 	return 0;
 }
 
+#define TC_VALUE_NOT_FOUND 1
+#define TC_CONFIG_FILE_ERROR 2
+
 int cmd__config(int argc, const char **argv)
 {
-	int i, val;
+	int i, val, ret = 0;
 	const char *v;
 	const struct string_list *strptr;
 	struct config_set cs;
 
 	if (argc == 3 && !strcmp(argv[1], "read_early_config")) {
 		read_early_config(early_config_cb, (void *)argv[2]);
-		return 0;
+		return ret;
 	}
 
 	setup_git_directory();
@@ -94,10 +97,9 @@  int cmd__config(int argc, const char **argv)
 				printf("(NULL)\n");
 			else
 				printf("%s\n", v);
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_value_multi")) {
 		strptr = git_config_get_value_multi(argv[2]);
@@ -109,41 +111,38 @@  int cmd__config(int argc, const char **argv)
 				else
 					printf("%s\n", v);
 			}
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_int")) {
 		if (!git_config_get_int(argv[2], &val)) {
 			printf("%d\n", val);
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_bool")) {
 		if (!git_config_get_bool(argv[2], &val)) {
 			printf("%d\n", val);
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (argc == 3 && !strcmp(argv[1], "get_string")) {
 		if (!git_config_get_string_tmp(argv[2], &v)) {
 			printf("%s\n", v);
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (argc >= 3 && !strcmp(argv[1], "configset_get_value")) {
 		for (i = 3; i < argc; i++) {
 			int err;
 			if ((err = git_configset_add_file(&cs, argv[i]))) {
 				fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]);
-				goto exit2;
+				ret = TC_CONFIG_FILE_ERROR;
+				goto out;
 			}
 		}
 		if (!git_configset_get_value(&cs, argv[2], &v)) {
@@ -151,17 +150,17 @@  int cmd__config(int argc, const char **argv)
 				printf("(NULL)\n");
 			else
 				printf("%s\n", v);
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (argc >= 3 && !strcmp(argv[1], "configset_get_value_multi")) {
 		for (i = 3; i < argc; i++) {
 			int err;
 			if ((err = git_configset_add_file(&cs, argv[i]))) {
 				fprintf(stderr, "Error (%d) reading configuration file %s.\n", err, argv[i]);
-				goto exit2;
+				ret = TC_CONFIG_FILE_ERROR;
+				goto out;
 			}
 		}
 		strptr = git_configset_get_value_multi(&cs, argv[2]);
@@ -173,27 +172,17 @@  int cmd__config(int argc, const char **argv)
 				else
 					printf("%s\n", v);
 			}
-			goto exit0;
 		} else {
 			printf("Value not found for \"%s\"\n", argv[2]);
-			goto exit1;
+			ret = TC_VALUE_NOT_FOUND;
 		}
 	} else if (!strcmp(argv[1], "iterate")) {
 		git_config(iterate_cb, NULL);
-		goto exit0;
+	} else {
+		die("%s: Please check the syntax and the function name", argv[0]);
 	}
 
-	die("%s: Please check the syntax and the function name", argv[0]);
-
-exit0:
-	git_configset_clear(&cs);
-	return 0;
-
-exit1:
-	git_configset_clear(&cs);
-	return 1;
-
-exit2:
+out:
 	git_configset_clear(&cs);
-	return 2;
+	return ret;
 }