threads / patch / 12024

patchconfig: deprecate using "" as boolean value false.

Subject: [PATCH] config: deprecate using "" as boolean value false.

## tl;dr

4 messages between Feb 11, 2008 and Feb 11, 2008. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Christian Couder· Feb 11, 2008, 07:22 UTC · lore
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 config.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)
Show changes to config.c +12 −1
diff --git a/config.c b/config.c
index 526a3f4..b5f1a11 100644
--- a/config.c
+++ b/config.c
@@ -300,8 +300,19 @@ int git_config_bool(const char *name, const char *value)
 {
 	if (!value)
 		return 1;
-	if (!*value)
+	if (!*value) {
+		fprintf(stderr,
+			"Warning: using an empty value for boolean config "
+			"variables is deprecated.\n"
+			"An empty value currently means 'false' as a "
+			"boolean, but may very well means 'true' in the "
+			"future!\n"
+			"Please consider using a 'false' value explicitely "
+			"for variable '%s', so that your config is future "
+			"proof. You can do that using:\n"
+			"\tgit config %s false\n", name, name);
 		return 0;
+	}
 	if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
 		return 1;
 	if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))

-- 
1.5.4.20.gc135a-dirty
Junio C Hamano· Feb 11, 2008, 08:21 UTC · re: Christian Couder · lore

Re: [PATCH] config: deprecate using "" as boolean value false.

Why do you call this a "fix", when the only effect of it is breaking people's existing setups? Wouldn't a set of proper fixes to the config parsing callback functions solve all the issues without having any downsides?

I think you are wasting both of our times. You could be doing the real fix instead. Or I could be spending time on it, instead of repeating to you how mistaken this approach is.

So far I've seen and queued only two usable fixes since that [Janitor] request. Maybe I should have ignored all this discussion in this thread and spent my time on doing the Janitorial work. The reason I joined the thread was that I hoped that I could save people like _you_ from wasting _your_ time in the discussion, instead of making better use of it by doing audits and fixes.

Grrr.
Junio C Hamano· Feb 11, 2008, 08:46 UTC · re: Junio C Hamano · lore

Re: [PATCH] config: deprecate using "" as boolean value false.

Junio C Hamano <gitster@pobox.com> writes:
> So far I've seen and queued only two usable fixes since that
> [Janitor] request.  Maybe I should have ignored all this
> discussion in this thread and spent my time on doing the
> Janitorial work.

In the thread I mentioned erroring out when a value we expect is missing instead of silently ignoring such a misconfiguration, and I showed an incorrect example of doing so using die().

The caller of the callback reports the line number and the filename upon error. So we should just return an error and let the caller do the more detailed reporting.

With this patch, a misconfiguration:
	[apply]
        	whitespace
will result in:
	$ git apply
        error: Missing value for 'apply.whitespace'
        fatal: bad config file line 29 in .git/config
which is much nicer.
---
 cache.h         |    1 +
 config.c        |    9 +++++++++
 builtin-apply.c |    2 ++
 3 files changed, 12 insertions(+), 0 deletions(-)
Show changes to 3 files +12 −0

cache.h, config.c, builtin-apply.c

diff --git a/cache.h b/cache.h
index 549f4bb..6abcee4 100644
--- a/cache.h
+++ b/cache.h
@@ -589,6 +589,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value);
+extern int config_error_nonbool(const char *);
 
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
diff --git a/config.c b/config.c
index 498259e..e9a9a6f 100644
--- a/config.c
+++ b/config.c
@@ -1074,3 +1074,12 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 	free(config_filename);
 	return ret;
 }
+
+/*
+ * Call this to report error for your variable that should not
+ * get a boolean value (i.e. "[my] var" means "true").
+ */
+int config_error_nonbool(const char *var)
+{
+	return error("Missing value for '%s'", var);
+}
diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..a11b1bb 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2746,6 +2746,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 static int git_apply_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "apply.whitespace")) {
+		if (!value)
+			return config_error_nonbool(var);
 		apply_default_whitespace = xstrdup(value);
 		return 0;
 	}
Junio C Hamano· Feb 11, 2008, 09:22 UTC · re: Junio C Hamano · lore

[PATCH] teach config parsers to diagnose missing values

I really hate to do this.

I announced an opportunity for new people to start hacking, and I really wanted to stay out of their way. It was a relatively easy way for people to get their hands dirty.

But then I somehow was trapped in a discussion that suggested to introduce a needless regression that breaks people's existing repositories, and had to argue against the insanity, because the suggestion would have discouraged new people from actually looking at the code and futzing with it.

Somehow I ended up doing this myself, which meant that the easy entrance opportunity was robbed from them. I am really unhappy about this, not about the resulting code, but about the process.

Anyway, here it is. I think I've caught all of them (except the one in builtin-gc that Miklos Vajna earlier sent in, and the one in archive-tar.c that I demonstrated in my original message).

---
 builtin-apply.c       |    2 ++
 builtin-branch.c      |    9 ++++++---
 builtin-clean.c       |    1 +
 builtin-commit.c      |    2 ++
 builtin-config.c      |    2 ++
 builtin-log.c         |    2 +-
 builtin-reflog.c      |   16 +++++++++++-----
 builtin-show-branch.c |    2 ++
 builtin-tag.c         |    2 +-
 cache.h               |    1 +
 config.c              |    9 +++++++++
 connect.c             |    2 ++
 convert.c             |    4 ++--
 diff.c                |   14 ++++++++++++--
 git.c                 |    2 ++
 help.c                |    2 ++
 http.c                |   25 +++++++++++++++----------
 merge-recursive.c     |   11 ++++++-----
 remote.c              |    9 ++++++---
 setup.c               |    2 ++
 wt-status.c           |    2 ++
 21 files changed, 89 insertions(+), 32 deletions(-)
Show changes to 21 files +89 −32

builtin-apply.c, builtin-branch.c, builtin-clean.c, builtin-commit.c, builtin-config.c, builtin-log.c, builtin-reflog.c, builtin-show-branch.c, builtin-tag.c, cache.h, config.c, connect.c, convert.c, diff.c, git.c, help.c, http.c, merge-recursive.c, remote.c, setup.c, wt-status.c

diff --git a/builtin-apply.c b/builtin-apply.c
index 15432b6..a11b1bb 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2746,6 +2746,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
 static int git_apply_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "apply.whitespace")) {
+		if (!value)
+			return config_error_nonbool(var);
 		apply_default_whitespace = xstrdup(value);
 		return 0;
 	}
diff --git a/builtin-branch.c b/builtin-branch.c
index 089cae5..e414c88 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -70,12 +70,15 @@ static int git_branch_config(const char *var, const char *value)
 	}
 	if (!prefixcmp(var, "color.branch.")) {
 		int slot = parse_branch_color_slot(var, 13);
+		if (!value)
+			return config_error_nonbool(var);
 		color_parse(value, var, branch_colors[slot]);
 		return 0;
 	}
-	if (!strcmp(var, "branch.autosetupmerge"))
-			branch_track = git_config_bool(var, value);
-
+	if (!strcmp(var, "branch.autosetupmerge")) {
+		branch_track = git_config_bool(var, value);
+		return 0;
+	}
 	return git_default_config(var, value);
 }
 
diff --git a/builtin-clean.c b/builtin-clean.c
index eb853a3..99739a6 100644
--- a/builtin-clean.c
+++ b/builtin-clean.c
@@ -20,6 +20,7 @@ static const char *const builtin_clean_usage[] = {
 
 static int git_clean_config(const char *var, const char *value)
 {
+	/* This is value=NULL safe */
 	if (!strcmp(var, "clean.requireforce"))
 		force = !git_config_bool(var, value);
 	return git_default_config(var, value);
diff --git a/builtin-commit.c b/builtin-commit.c
index c787bed..06e8a34 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -743,6 +743,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 int git_commit_config(const char *k, const char *v)
 {
 	if (!strcmp(k, "commit.template")) {
+		if (!v)
+			return config_error_nonbool(v);
 		template_file = xstrdup(v);
 		return 0;
 	}
diff --git a/builtin-config.c b/builtin-config.c
index e4a12e3..077d8ef 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -168,6 +168,8 @@ static char parsed_color[COLOR_MAXLEN];
 static int git_get_color_config(const char *var, const char *value)
 {
 	if (!strcmp(var, get_color_slot)) {
+		if (!value)
+			config_error_nonbool(var);
 		color_parse(value, var, parsed_color);
 		get_color_found = 1;
 	}
diff --git a/builtin-log.c b/builtin-log.c
index dcc9f81..9458428 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -219,7 +219,7 @@ static int git_log_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "format.subjectprefix")) {
 		if (!value)
-			die("format.subjectprefix without value");
+			config_error_nonbool(var);
 		fmt_patch_subject_prefix = xstrdup(value);
 		return 0;
 	}
diff --git a/builtin-reflog.c b/builtin-reflog.c
index e6834dd..4836ec9 100644
--- a/builtin-reflog.c
+++ b/builtin-reflog.c
@@ -307,13 +307,19 @@ static int collect_reflog(const char *ref, const unsigned char *sha1, int unused
 
 static int reflog_expire_config(const char *var, const char *value)
 {
-	if (!strcmp(var, "gc.reflogexpire"))
+	if (!strcmp(var, "gc.reflogexpire")) {
+		if (!value)
+			config_error_nonbool(var);
 		default_reflog_expire = approxidate(value);
-	else if (!strcmp(var, "gc.reflogexpireunreachable"))
+		return 0;
+	}
+	if (!strcmp(var, "gc.reflogexpireunreachable")) {
+		if (!value)
+			config_error_nonbool(var);
 		default_reflog_expire_unreachable = approxidate(value);
-	else
-		return git_default_config(var, value);
-	return 0;
+		return 0;
+	}
+	return git_default_config(var, value);
 }
 
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 6dc835d..019abd3 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -536,6 +536,8 @@ static void append_one_rev(const char *av)
 static int git_show_branch_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "showbranch.default")) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (default_alloc <= default_num + 1) {
 			default_alloc = default_alloc * 3 / 2 + 20;
 			default_arg = xrealloc(default_arg, sizeof *default_arg * default_alloc);
diff --git a/builtin-tag.c b/builtin-tag.c
index 03e7015..4a4a88c 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -258,7 +258,7 @@ static int git_tag_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "user.signingkey")) {
 		if (!value)
-			die("user.signingkey without value");
+			return config_error_nonbool(value);
 		set_signingkey(value);
 		return 0;
 	}
diff --git a/cache.h b/cache.h
index 549f4bb..6abcee4 100644
--- a/cache.h
+++ b/cache.h
@@ -589,6 +589,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value);
+extern int config_error_nonbool(const char *);
 
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
diff --git a/config.c b/config.c
index 498259e..e9a9a6f 100644
--- a/config.c
+++ b/config.c
@@ -1074,3 +1074,12 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 	free(config_filename);
 	return ret;
 }
+
+/*
+ * Call this to report error for your variable that should not
+ * get a boolean value (i.e. "[my] var" means "true").
+ */
+int config_error_nonbool(const char *var)
+{
+	return error("Missing value for '%s'", var);
+}
diff --git a/connect.c b/connect.c
index 3aefd4a..71597d4 100644
--- a/connect.c
+++ b/connect.c
@@ -370,6 +370,8 @@ static int git_proxy_command_options(const char *var, const char *value)
 
 		if (git_proxy_command)
 			return 0;
+		if (!value)
+			return config_error_nonbool(var);
 		/* [core]
 		 * ;# matches www.kernel.org as well
 		 * gitproxy = netcatter-1 for kernel.org
diff --git a/convert.c b/convert.c
index 80f114b..552707e 100644
--- a/convert.c
+++ b/convert.c
@@ -326,14 +326,14 @@ static int read_convert_config(const char *var, const char *value)
 
 	if (!strcmp("smudge", ep)) {
 		if (!value)
-			return error("%s: lacks value", var);
+			return config_error_nonbool(var);
 		drv->smudge = strdup(value);
 		return 0;
 	}
 
 	if (!strcmp("clean", ep)) {
 		if (!value)
-			return error("%s: lacks value", var);
+			return config_error_nonbool(var);
 		drv->clean = strdup(value);
 		return 0;
 	}
diff --git a/diff.c b/diff.c
index 5b8afdc..4d2e23a 100644
--- a/diff.c
+++ b/diff.c
@@ -158,6 +158,8 @@ int git_diff_ui_config(const char *var, const char *value)
 		return 0;
 	}
 	if (!strcmp(var, "diff.external")) {
+		if (!value)
+			return config_error_nonbool(var);
 		external_diff_cmd_cfg = xstrdup(value);
 		return 0;
 	}
@@ -165,8 +167,11 @@ int git_diff_ui_config(const char *var, const char *value)
 		const char *ep = strrchr(var, '.');
 
 		if (ep != var + 4) {
-			if (!strcmp(ep, ".command"))
+			if (!strcmp(ep, ".command")) {
+				if (!value)
+					return config_error_nonbool(var);
 				return parse_lldiff_command(var, ep, value);
+			}
 		}
 	}
 
@@ -177,6 +182,8 @@ int git_diff_basic_config(const char *var, const char *value)
 {
 	if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
 		int slot = parse_diff_color_slot(var, 11);
+		if (!value)
+			return config_error_nonbool(var);
 		color_parse(value, var, diff_colors[slot]);
 		return 0;
 	}
@@ -184,8 +191,11 @@ int git_diff_basic_config(const char *var, const char *value)
 	if (!prefixcmp(var, "diff.")) {
 		const char *ep = strrchr(var, '.');
 		if (ep != var + 4) {
-			if (!strcmp(ep, ".funcname"))
+			if (!strcmp(ep, ".funcname")) {
+				if (!value)
+					return config_error_nonbool(var);
 				return parse_funcname_pattern(var, ep, value);
+			}
 		}
 	}
 
diff --git a/git.c b/git.c
index 15fec89..0cb8688 100644
--- a/git.c
+++ b/git.c
@@ -93,6 +93,8 @@ static char *alias_string;
 static int git_alias_config(const char *var, const char *value)
 {
 	if (!prefixcmp(var, "alias.") && !strcmp(var + 6, alias_command)) {
+		if (!value)
+			return config_error_nonbool(var);
 		alias_string = xstrdup(value);
 	}
 	return 0;
diff --git a/help.c b/help.c
index 1302a61..95e7640 100644
--- a/help.c
+++ b/help.c
@@ -40,6 +40,8 @@ static void parse_help_format(const char *format)
 static int git_help_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "help.format")) {
+		if (!value)
+			return config_error_nonbool(var);
 		help_default_format = xstrdup(value);
 		return 0;
 	}
diff --git a/http.c b/http.c
index d2c11ae..5925d07 100644
--- a/http.c
+++ b/http.c
@@ -101,16 +101,18 @@ static int http_options(const char *var, const char *value)
 
 	if (!strcmp("http.sslcert", var)) {
 		if (ssl_cert == NULL) {
-			ssl_cert = xmalloc(strlen(value)+1);
-			strcpy(ssl_cert, value);
+			if (!value)
+				return config_error_nonbool(var);
+			ssl_cert = xstrdup(value);
 		}
 		return 0;
 	}
 #if LIBCURL_VERSION_NUM >= 0x070902
 	if (!strcmp("http.sslkey", var)) {
 		if (ssl_key == NULL) {
-			ssl_key = xmalloc(strlen(value)+1);
-			strcpy(ssl_key, value);
+			if (!value)
+				return config_error_nonbool(var);
+			ssl_key = xstrdup(value);
 		}
 		return 0;
 	}
@@ -118,16 +120,18 @@ static int http_options(const char *var, const char *value)
 #if LIBCURL_VERSION_NUM >= 0x070908
 	if (!strcmp("http.sslcapath", var)) {
 		if (ssl_capath == NULL) {
-			ssl_capath = xmalloc(strlen(value)+1);
-			strcpy(ssl_capath, value);
+			if (!value)
+				return config_error_nonbool(var);
+			ssl_capath = xstrdup(value);
 		}
 		return 0;
 	}
 #endif
 	if (!strcmp("http.sslcainfo", var)) {
 		if (ssl_cainfo == NULL) {
-			ssl_cainfo = xmalloc(strlen(value)+1);
-			strcpy(ssl_cainfo, value);
+			if (!value)
+				return config_error_nonbool(var);
+			ssl_cainfo = xstrdup(value);
 		}
 		return 0;
 	}
@@ -157,8 +161,9 @@ static int http_options(const char *var, const char *value)
 	}
 	if (!strcmp("http.proxy", var)) {
 		if (curl_http_proxy == NULL) {
-			curl_http_proxy = xmalloc(strlen(value)+1);
-			strcpy(curl_http_proxy, value);
+			if (!value)
+				return config_error_nonbool(var);
+			curl_http_proxy = xstrdup(value);
 		}
 		return 0;
 	}
diff --git a/merge-recursive.c b/merge-recursive.c
index c292a77..34e3167 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -844,8 +844,9 @@ static int read_merge_config(const char *var, const char *value)
 	int namelen;
 
 	if (!strcmp(var, "merge.default")) {
-		if (value)
-			default_ll_merge = strdup(value);
+		if (!value)
+			return config_error_nonbool(var);
+		default_ll_merge = strdup(value);
 		return 0;
 	}
 
@@ -878,14 +879,14 @@ static int read_merge_config(const char *var, const char *value)
 
 	if (!strcmp("name", ep)) {
 		if (!value)
-			return error("%s: lacks value", var);
+			return config_error_nonbool(var);
 		fn->description = strdup(value);
 		return 0;
 	}
 
 	if (!strcmp("driver", ep)) {
 		if (!value)
-			return error("%s: lacks value", var);
+			return config_error_nonbool(var);
 		/*
 		 * merge.<name>.driver specifies the command line:
 		 *
@@ -908,7 +909,7 @@ static int read_merge_config(const char *var, const char *value)
 
 	if (!strcmp("recursive", ep)) {
 		if (!value)
-			return error("%s: lacks value", var);
+			return config_error_nonbool(var);
 		fn->recursive = strdup(value);
 		return 0;
 	}
diff --git a/remote.c b/remote.c
index 0e00680..20abbc0 100644
--- a/remote.c
+++ b/remote.c
@@ -222,15 +222,18 @@ static int handle_config(const char *key, const char *value)
 		subkey = strrchr(name, '.');
 		if (!subkey)
 			return 0;
-		if (!value)
-			return 0;
 		branch = make_branch(name, subkey - name);
 		if (!strcmp(subkey, ".remote")) {
+			if (!value)
+				return config_error_nonbool(key);
 			branch->remote_name = xstrdup(value);
 			if (branch == current_branch)
 				default_remote_name = branch->remote_name;
-		} else if (!strcmp(subkey, ".merge"))
+		} else if (!strcmp(subkey, ".merge")) {
+			if (!value)
+				return config_error_nonbool(key);
 			add_merge(branch, xstrdup(value));
+		}
 		return 0;
 	}
 	if (prefixcmp(key,  "remote."))
diff --git a/setup.c b/setup.c
index adede16..4509598 100644
--- a/setup.c
+++ b/setup.c
@@ -372,6 +372,8 @@ int check_repository_format_version(const char *var, const char *value)
 		if (is_bare_repository_cfg == 1)
 			inside_work_tree = -1;
 	} else if (strcmp(var, "core.worktree") == 0) {
+		if (!value)
+			return config_error_nonbool(var);
 		if (git_work_tree_cfg)
 			free(git_work_tree_cfg);
 		git_work_tree_cfg = xstrdup(value);
diff --git a/wt-status.c b/wt-status.c
index c0c2472..bfd1b0f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -402,6 +402,8 @@ int git_status_config(const char *k, const char *v)
 	}
 	if (!prefixcmp(k, "status.color.") || !prefixcmp(k, "color.status.")) {
 		int slot = parse_status_slot(k, 13);
+		if (!v)
+			return config_error_nonbool(k);
 		color_parse(v, k, wt_status_colors[slot]);
 		return 0;
 	}

← back to recent threads