Re: [PATCH 2/5] implement submodule config cache for lookup of submodule names

Heiko Voigt [off-list ref] writes:

...
+static int is_cache_init = 0;

Please don't initialise variables in the .bss to zero by hand.

+ ...
+	warning("%s:.gitmodules, multiple configurations found for "
+			"submodule.%s.%s. Skipping second one!",
+			commit_string, name, option);
+}
+ ...
+		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;
+		}

These two look inconsistent in different ways.  I think we typically
quote the names like so:

	warning("I have trouble with variable '%s' somehow", var);

On Thu, Jun 12, 2014 at 02:58:20PM -0700, Junio C Hamano wrote:

Heiko Voigt [off-list ref] writes:

quoted

...
+static int is_cache_init = 0;

Please don't initialise variables in the .bss to zero by hand.

Ok will remove that.

quoted

+ ...
+	warning("%s:.gitmodules, multiple configurations found for "
+			"submodule.%s.%s. Skipping second one!",
+			commit_string, name, option);
+}
+ ...
+		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;
+		}

These two look inconsistent in different ways.  I think we typically
quote the names like so:

	warning("I have trouble with variable '%s' somehow", var);

Ok will change the quotation for the variable and parameter names.

Here is the fixup[1] I will queue in my branch.

Cheers Heiko

[1] ---8<----
Subject: [PATCH] fixup! implement submodule config cache for lookup of
 submodule names

---
 submodule-config.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 437fbdb..f330ccc 100644
--- a/submodule-config.c
+++ b/submodule-config.c

@@ -26,7 +26,7 @@ struct submodule_entry {
 };
 
 static struct submodule_cache cache;
-static int is_cache_init = 0;
+static int is_cache_init;
 
 static int config_path_cmp(const struct submodule_entry *a,
 			   const struct submodule_entry *b,

@@ -230,7 +230,7 @@ static void warn_multiple_config(const unsigned char *commit_sha1,
 	if (commit_sha1)
 		commit_string = sha1_to_hex(commit_sha1);
 	warning("%s:.gitmodules, multiple configurations found for "
-			"submodule.%s.%s. Skipping second one!",
+			"'submodule.%s.%s'. Skipping second one!",
 			commit_string, name, option);
 }
 

@@ -298,8 +298,8 @@ static int parse_config(const char *var, const char *value, void *data)
 		}
 		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);
+			warning("Invalid parameter '%s' for config option "
+					"'submodule.%s.ignore'", value, var);
 			goto release_return;
 		}
 

-- 
2.0.0