git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: bash_completion outside repo

From
Junio C Hamano <gitster@pobox.com>
Date
Sep 11, 2009, 21:22 UTC
Message-ID
<7vvdjpuqgx.fsf@alter.siamese.dyndns.org>
In-Reply-To
<7viqfpw6tv.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
Show 24 quoted lines
> The last one is not the topic of this patch, but it is quite problematic.
> When you are interested in finding out what value gc.pruneExpire is set,
> you do not care (as long as the configuration file was syntactically
> correct and you did not have to skip any file you were supposed to read
> due to EACCESS) if "branch.autosetupmerge" has an invalid value.
>
> A possible longer term solution would be to:
>
>  - Change the signature of callbacks (e.g. git_default_branch_config()) so
>    that they return void.  They are not allowed to report any semantic
>    errors while parsing.
>
>  - Instead, they use special "INVALID" value and store that when they see
>    a semantically incorrect value.  They may also want to store what the
>    actual string the config file gave them for later reporting, together
>    with the name of and the line number in the config file for diagnostic
>    purposes.
>
>  - The user of the internalized value (i.e. "git grep git_branch_track"
>    shows there are only two, cmd_branch() and cmd_checkout()) must check
>    for the above INVALID value before they use the variable, and die at
>    the point of the use.
>
> I'll send an illustration patch separately.

So here is an illustration to handle _only_ a misspelled branch.autosetupmerge.

If you have this in your .git/config file:
	[branch]
        	autosetupmerge = nevver

you cannot run "git diff" without this patch. But with this patch, only the commands that _care_ about this misconfiguration would notice and report.

The new world order is:
 - The config parsing callback should detect semantic errors, but should
   not return non-zero, to let the parser continue.
   In the _real_ patch, their signatures should be changed to return void.
   I wanted to illustrate more important points in this patch so I didn't
   do that.
 - Instead, when they detect an error in configured values, they make a
   note that the value is bad (e.g. I introduced BRANCH_TRACK_CONFIG_ERROR
   for this), and also can ask record_bad_config() to record this fact for
   possible later reporting purposes.  You also _could_ issue error() to
   make it easier to detect unrelated but bad configuration for users, but
   that is secondary and I didn't do that in this illustration.
 - Then, when the values are actually _used_, the users should take notice
   of the situation (e.g. both cmd_checkout() and cmd_branch() codepaths
   are modified to do this) and ask die_with_bad_config() to report the
   error the parser detected earlier.

This way, commands that do not use the value of branch.autosetupmerge, e.g. "git diff", won't have to abort.

 builtin-branch.c   |    5 +++-
 builtin-checkout.c |    5 +++-
 cache.h            |    7 ++++++
 config.c           |   57 ++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 9f57992..10010a7 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -635,9 +635,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		rename_branch(head, argv[0], rename > 1);
 	else if (rename && (argc == 2))
 		rename_branch(argv[0], argv[1], rename > 1);
-	else if (argc <= 2)
+	else if (argc <= 2) {
+		if (track == BRANCH_TRACK_CONFIG_ERROR)
+			die_with_bad_config("branch.autosetupmerge");
 		create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
 			      force_create, reflog, track);
+	}
 	else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/builtin-checkout.c b/builtin-checkout.c
index d050c37..3bf57a5 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -630,8 +630,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		opts.new_branch = argv0 + 1;
 	}
 
-	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
+	if (opts.track == BRANCH_TRACK_UNSPECIFIED) {
+		if (git_branch_track == BRANCH_TRACK_CONFIG_ERROR)
+			die_with_bad_config("branch.autosetupmerge");
 		opts.track = git_branch_track;
+	}
 	if (conflict_style) {
 		opts.merge = 1; /* implied */
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
diff --git a/cache.h b/cache.h
index 5fad24c..f7ff502 100644
--- a/cache.h
+++ b/cache.h
@@ -533,6 +533,7 @@ enum safe_crlf {
 extern enum safe_crlf safe_crlf;
 
 enum branch_track {
+	BRANCH_TRACK_CONFIG_ERROR = -2,
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,
 	BRANCH_TRACK_REMOTE,
@@ -890,6 +891,9 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne
 /* Dumb servers support */
 extern int update_server_info(int);
 
+/*
+ * configuration file management
+ */
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
@@ -910,6 +914,9 @@ extern int git_config_global(void);
 extern int config_error_nonbool(const char *);
 extern const char *config_exclusive_filename;
 
+extern void record_bad_config(const char *name, const char *value);
+extern void die_with_bad_config(const char *name);
+
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
 extern char git_default_name[MAX_GITNAME];
diff --git a/config.c b/config.c
index e87edea..5b6a929 100644
--- a/config.c
+++ b/config.c
@@ -306,6 +306,41 @@ static void die_bad_config(const char *name)
 	die("bad config value for '%s'", name);
 }
 
+static struct bad_config_note {
+	struct bad_config_note *next;
+	const char *name;
+	const char *value;
+	int linenr;
+	const char *filename;
+} *bad_config_note;
+
+void record_bad_config(const char *name, const char *value)
+{
+	struct bad_config_note *note = xcalloc(1, sizeof(*note));
+	note->next = bad_config_note;
+	bad_config_note = note;
+	note->name = xstrdup(name);
+	note->value = xstrdup(value);
+	note->linenr = config_linenr;
+	note->filename = config_file_name ? xstrdup(config_file_name) : NULL;
+}
+
+void die_with_bad_config(const char *name)
+{
+	struct bad_config_note *note;
+	for (note = bad_config_note; note; note = note->next)
+		if (!strcmp(name, note->name)) {
+			char whence[PATH_MAX + 200];
+			if (note->filename)
+				sprintf(whence, " in %s", note->filename);
+			else
+				whence[0] = '\0';
+			die("bad config value '%s' for '%s'%s, line %d",
+			    note->value, note->name, whence, note->linenr);
+		}
+	die("program error: bad config value for %s not recorded", name);
+}
+
 int git_config_int(const char *name, const char *value)
 {
 	long ret = 0;
@@ -324,6 +359,8 @@ unsigned long git_config_ulong(const char *name, const char *value)
 
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
+	long ret;
+
 	*is_bool = 1;
 	if (!value)
 		return 1;
@@ -333,14 +370,24 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 		return 1;
 	if (!strcasecmp(value, "false") || !strcasecmp(value, "no") || !strcasecmp(value, "off"))
 		return 0;
+
 	*is_bool = 0;
-	return git_config_int(name, value);
+
+	ret = 0;
+	if (!git_parse_long(value, &ret)) {
+		record_bad_config(name, value);
+		*is_bool = -1;
+	}
+	return ret;
 }
 
 int git_config_bool(const char *name, const char *value)
 {
-	int discard;
-	return !!git_config_bool_or_int(name, value, &discard);
+	int is_bool, val;
+	val = git_config_bool_or_int(name, value, &is_bool);
+	if (is_bool < 0)
+		return is_bool;
+	return !!val;
 }
 
 int git_config_string(const char **dest, const char *var, const char *value)
@@ -546,11 +593,13 @@ static int git_default_i18n_config(const char *var, const char *value)
 static int git_default_branch_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "branch.autosetupmerge")) {
+		int val;
 		if (value && !strcasecmp(value, "always")) {
 			git_branch_track = BRANCH_TRACK_ALWAYS;
 			return 0;
 		}
-		git_branch_track = git_config_bool(var, value);
+		val = git_config_bool(var, value);
+		git_branch_track = (val < 0) ? BRANCH_TRACK_CONFIG_ERROR : val;
 		return 0;
 	}
 	if (!strcmp(var, "branch.autosetuprebase")) {
Previous: Jeff KingNext: Jeff King
Message 11 of 20 in “bash_completion outside repo”
  1. james bardinSep 10, 2009
  2. Michael J GruberSep 11, 2009
  3. Todd ZullingerSep 11, 2009
  4. james bardinSep 11, 2009
  5. Jeff KingSep 11, 2009
  6. Shawn O. PearceSep 11, 2009
  7. Jeff KingSep 11, 2009
  8. Jeff KingSep 11, 2009
  9. Junio C HamanoSep 11, 2009
  10. Jeff KingSep 11, 2009
  11. Junio C HamanoSep 11, 2009
  12. Jeff KingSep 11, 2009
  13. Junio C HamanoSep 11, 2009
  14. Junio C HamanoSep 11, 2009
  15. Jeff KingSep 11, 2009
  16. completion: Replace config --list with --get-regexpTodd Zullinger, Sep 11, 2009
  17. Shawn O. PearceSep 12, 2009
  18. Bert WesargSep 13, 2009
  19. Todd ZullingerSep 13, 2009
  20. Junio C HamanoSep 13, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.