bash_completion outside repo

bash_completion outside repo

[james bardin]

Hi,

I'm making a git rpm for our site, and I'm getting an error with
bash_completion on RHEL/CentOS 5.

When outside a repo, any completion causes git to spit out
  fatal: error processing config file(s)

This is 1.6.4.2 using the contrib bash completion file

Thanks
-jim

Re: bash_completion outside repo

[Michael J Gruber]

james bardin venit, vidit, dixit 10.09.2009 17:13:
> Hi,
> 
> I'm making a git rpm for our site, and I'm getting an error with
> bash_completion on RHEL/CentOS 5.
> 
> When outside a repo, any completion causes git to spit out
>   fatal: error processing config file(s)
> 
> This is 1.6.4.2 using the contrib bash completion file
> 
> Thanks
> -jim

I can't reproduce this with git version 1.6.5.rc0.164.g5f6b0 on Fedora 11.

Which exact steps reproduce it for you, and which relevant settings (PS1
and GIT_PS1_...) do you use? Do you have a system or global .gitconfig?

Michael

Re: bash_completion outside repo

[Todd Zullinger]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

james bardin wrote:
> I'm making a git rpm for our site, and I'm getting an error with
> bash_completion on RHEL/CentOS 5.

Out of curiosity, have you tried rebuilding the Fedora packages from
rawhide?  They should work on RHEL/CentOS 5 just fine (I use them
myself).

> When outside a repo, any completion causes git to spit out
>   fatal: error processing config file(s)
>
> This is 1.6.4.2 using the contrib bash completion file

I can't reproduce this on either Fedora or CentOS.

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A great many people think they are thinking when they are merely
rearranging their prejudices.
    -- William James


[-- Attachment #2: Type: application/pgp-signature, Size: 542 bytes --]

Re: bash_completion outside repo

[james bardin]

On Fri, Sep 11, 2009 at 9:33 AM, Todd Zullinger <tmz@pobox.com> wrote:
> james bardin wrote:
>> I'm making a git rpm for our site, and I'm getting an error with
>> bash_completion on RHEL/CentOS 5.
>
> Out of curiosity, have you tried rebuilding the Fedora packages from
> rawhide?  They should work on RHEL/CentOS 5 just fine (I use them
> myself).
>

I was trying to prune out some of the dependencies to make internal
deployment easier.
I bundled the doc tarballs instead of requiring asciidoc, and git
includes its own perl(Error) if it's not available.



On Fri, Sep 11, 2009 at 9:11 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:

> I can't reproduce this with git version 1.6.5.rc0.164.g5f6b0 on Fedora 11.
>
> Which exact steps reproduce it for you, and which relevant settings (PS1
> and GIT_PS1_...) do you use? Do you have a system or global .gitconfig?
>

I did a make install, and dropped the completion file in
/etc/bash_completion.d/. No other settings changed. I did a quick
check, and it happens with the current 1.6.5 snapshot too, and on a
fedora 10 box I found.

It seems I only get this error if I don't have a global config.
Touching ~/.gitconfig stops the error.

Re: bash_completion outside repo

[Jeff King]

On Fri, Sep 11, 2009 at 10:00:33AM -0400, james bardin wrote:

> I did a make install, and dropped the completion file in
> /etc/bash_completion.d/. No other settings changed. I did a quick
> check, and it happens with the current 1.6.5 snapshot too, and on a
> fedora 10 box I found.
> 
> It seems I only get this error if I don't have a global config.
> Touching ~/.gitconfig stops the error.

Ah, I see. It looks like we use "git config --list" to view several bits
of configuration. However, it is not happy if there is no config file to
list.

However, I'm not sure that "config --list" isn't broken. Inside a repo,
doing "git config --list" shows the repo config and my global config,
and exits with no error.  Outside a repo, it shows my global config, and
exits with no error. But if I _don't_ have global config, it produces an
error. Shouldn't it treat that as simply "no config is available"?

I also question why it is using "git config --list" at all in snippets
like this:

        for i in $(git --git-dir="$d" config --list); do
                case "$i" in
                remote.*.url=*)
                        i="${i#remote.}"
                        echo "${i/.url=*/}"
                        ;;
                esac
        done

instead of just using "git config --get-regexp 'remote\..*\.url'", which
would be slightly more efficient, and also doesn't have this bug. ;)

-Peff

Re: bash_completion outside repo

[Shawn O. Pearce]

Jeff King <peff@peff.net> wrote:
> I also question why it is using "git config --list" at all in snippets
> like this:
> 
>         for i in $(git --git-dir="$d" config --list); do
>                 case "$i" in
>                 remote.*.url=*)
>                         i="${i#remote.}"
>                         echo "${i/.url=*/}"
>                         ;;
>                 esac
>         done
> 
> instead of just using "git config --get-regexp 'remote\..*\.url'", which
> would be slightly more efficient, and also doesn't have this bug. ;)

F'king oversight.  You are right, this should be --get-regexp.
There isn't a reason here, probably other than "I forgot about
--get-regexp when I wrote the original code".

-- 
Shawn.

Re: bash_completion outside repo

[Jeff King]

On Fri, Sep 11, 2009 at 07:36:51AM -0700, Shawn O. Pearce wrote:

> > instead of just using "git config --get-regexp 'remote\..*\.url'", which
> > would be slightly more efficient, and also doesn't have this bug. ;)
> 
> F'king oversight.  You are right, this should be --get-regexp.
> There isn't a reason here, probably other than "I forgot about
> --get-regexp when I wrote the original code".

OK. I will leave a bash-completion patch for you (or somebody else
interested) as I don't use it myself.

Assuming you make such a patch, that will clear up the original issue. I
wonder if we should fix "git config --list". The current semantics seem
a little crazy to me, but it is a scriptable interface. I'm inclined to
call this a bug, though.

-Peff

Re: bash_completion outside repo

[Jeff King]

On Fri, Sep 11, 2009 at 11:09:34AM -0400, Jeff King wrote:

> Assuming you make such a patch, that will clear up the original issue. I
> wonder if we should fix "git config --list". The current semantics seem
> a little crazy to me, but it is a scriptable interface. I'm inclined to
> call this a bug, though.

And here is a patch to fix it.

-- >8 --
Subject: [PATCH] config: treat non-existent config files as empty

The git_config() function signals error by returning -1 in
two instances:

  1. An actual error occurs in opening a config file (parse
     errors cause an immediate die).

  2. Of the three possible config files, none was found.

However, this second case is often not an error at all; it
simply means that the user has no configuration (they are
outside a repo, and they have no ~/.gitconfig file). This
can lead to confusing errors, such as when the bash
completion calls "git config --list" outside of a repo. If
the user has a ~/.gitconfig, the command completes
succesfully; if they do not, it complains to stderr.

This patch allows callers of git_config to distinguish
between the two cases. Error is signaled by -1, and
otherwise the return value is the number of files parsed.
This means that the traditional "git_config(...) < 0" check
for error should work, but callers who want to know whether
we parsed any files or not can still do so.

We need to tweak one use of git_config in builtin-remote
that previously assumed the return value was either '0' or
'-1'.

Signed-off-by: Jeff King <peff@peff.net>
---

This is actually a bit overengineered. Of the hundreds of calls to
git_config, there are exactly _two_ which check the return value. And
neither of them cares whether we parsed files or not; they really only
care if there was an error. So we could simply return 0 as long as there
is no error.

This also makes me wonder, though. Git can do wildly different things
(including hard-to-reverse things) based on config (e.g., just consider
gc.pruneExpire). Yet we call git_config() without ever checking for
errors. In the actual parsing routines, we die() if there is an error.
But if we fail to open the file due to some transient error, we will
silently ignore the situation.

Granted, such transient errors are unlikely. The biggest reasons for
failing to open a file are that it doesn't exist, or that we have no
permission to read it, both of which are treated explicitly in
git_config as "silently ok". But I wonder if we should simply be dying
on such an error, and git_config() should just have a void return.

 builtin-remote.c       |    3 ++-
 config.c               |    4 +---
 t/t1300-repo-config.sh |    8 ++++++++
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/builtin-remote.c b/builtin-remote.c
index 0777dd7..3bf1fe8 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -1245,7 +1245,8 @@ static int update(int argc, const char **argv)
 	for (i = 1; i < argc; i++) {
 		int groups_found = 0;
 		remote_group.name = argv[i];
-		result = git_config(get_remote_group, &groups_found);
+		if (git_config(get_remote_group, &groups_found) < 0)
+			result = -1;
 		if (!groups_found && (i != 1 || strcmp(argv[1], "default"))) {
 			struct remote *remote;
 			if (!remote_is_configured(argv[i]))
diff --git a/config.c b/config.c
index e87edea..e429674 100644
--- a/config.c
+++ b/config.c
@@ -709,9 +709,7 @@ int git_config(config_fn_t fn, void *data)
 		found += 1;
 	}
 	free(repo_config);
-	if (found == 0)
-		return -1;
-	return ret;
+	return ret == 0 ? found : ret;
 }
 
 /*
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 83b7294..db987b7 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -289,6 +289,14 @@ test_expect_success 'working --list' \
 	'git config --list > output && cmp output expect'
 
 cat > expect << EOF
+EOF
+
+test_expect_success '--list without repo produces empty output' '
+	git --git-dir=nonexistent config --list >output &&
+	test_cmp expect output
+'
+
+cat > expect << EOF
 beta.noindent sillyValue
 nextsection.nonewline wow2 for me
 EOF
-- 
1.6.5.rc0.174.g29a6d.dirty

Re: bash_completion outside repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This is actually a bit overengineered. Of the hundreds of calls to
> git_config, there are exactly _two_ which check the return value. And
> neither of them cares whether we parsed files or not; they really only
> care if there was an error. So we could simply return 0 as long as there
> is no error.
>
> This also makes me wonder, though. Git can do wildly different things
> (including hard-to-reverse things) based on config (e.g., just consider
> gc.pruneExpire). Yet we call git_config() without ever checking for
> errors. In the actual parsing routines, we die() if there is an error.
> But if we fail to open the file due to some transient error, we will
> silently ignore the situation.
>
> Granted, such transient errors are unlikely. The biggest reasons for
> failing to open a file are that it doesn't exist, or that we have no
> permission to read it, both of which are treated explicitly in
> git_config as "silently ok". But I wonder if we should simply be dying
> on such an error, and git_config() should just have a void return.

Thanks.

ENOENT should be the same as having an empty file, which is the main point
of the patch and at the same time why you feel this is overengineered.  I
agree with you on both counts.

While I also agree that EACCESS and other failures should be treated as
fatal in principle for safety, e.g. when running prune without being able
to read gc.pruneExpire as you mentioned, we would need a tradeoff between
safety and convenience.  When asked to help looking at a complex merge in
a colleages' repository, do you want your "git diff" to refuse to run only
because her .git/config cannot be read by you?  Of course, you _can_ work
this particular one around by various means (e.g. prefixing GIT_CONFIG=...
to force ignoring the file; telling the colleage that she'd better make
her .git/config readable to you if she wants your help), if either one of
the owner of the .git/config file or the party that wants to access the
repository is a non-person such a workaround would be harder to arrange.

Also there was a move going in the opposite direction to allow a config
file that is syntactically broken to be handled without the parser dying,
primarily to help "git config -e".  In the longer term, our direction
shouldn't be adding more die() in the git_config() callchain, but instead
allowing it to report different kind of failures to let the caller act
based on what it does and what the nature of failure is.

For example, "gc" may say "I won't prune because I had to skip some of the
lines in your .git/config because you have syntax errors in them, and I
may have missed the definition of gc.pruneExpire you may wanted to place
on them", while "diff" may ignore that kind of errors.

Having said all that, my preference *for now* would be to ignore "there is
no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors
and die().

There are some other glitches in the current git_config() callchain.

 - No config file anywhere gives an error.  I agree with you that this is
   a bug.

 - Having GIT_CONFIG=/no/such/file in the environment gives an error,
   which is good.

 - config.c::git_parse_file() [*1*] dies when it detects certain file
   format errors itself.  This is not good for "git config -e", as it
   needs to learn core.editor before it can be used to fix such an error.

   This function then calls config.c::get_value() and it dies when
   config.c::get_value() reports any error.

 - config.c::get_value() is called by config.c::git_parse_file() to finish
   parsing out the <name, value> pair, and stores the "value" in a form
   usable in the code, e.g. a variable defined in environment.c.  The
   function returns an error on some file format errors (e.g. a variable
   name is too long, string quoting unterminated) that signals the calling
   config.c::git_parse_file() to die().  These error returns are good (the
   caller however may need to be fixed for "config -e" issue not to die).

   It then calls the parse callbacck routines.  They return error when
   they detect semantic errors (e.g. "branch.autosetupmerge = alwys" is
   not one of the valid values).

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.

[Footnote]

*1* What a horrible name for this function!  It is static so git_ prefix
is unneeded, and if it anticipates it might get someday exported, parse_file
is too generic and should be named git_parse_config_file().

Re: bash_completion outside repo

[Jeff King]

On Fri, Sep 11, 2009 at 01:43:24PM -0700, Junio C Hamano wrote:

> While I also agree that EACCESS and other failures should be treated as
> fatal in principle for safety, e.g. when running prune without being able
> to read gc.pruneExpire as you mentioned, we would need a tradeoff between
> safety and convenience.  When asked to help looking at a complex merge in
> a colleages' repository, do you want your "git diff" to refuse to run only
> because her .git/config cannot be read by you?  Of course, you _can_ work

Sorry, I think I was a bit unclear in the original message. There are
two classes of errors right now:

  1. access(fn, R_OK) != 0 (i.e., ENOENT and EACCESS)

  2. fopen(fn, "r") != 0

In the case of (1), we treat it as an empty file (except in the case
that _all_ files fail (1), in which case we reported an error, which is
really stupid and is what this patch fixes).

In the case of (2), we treat it as an error, but because nobody actually
bothers to check the error code, it is effectively ignored. What I was
thinking is that (2) should be promoted to die(), and leave (1) as-is.

So I think in your example it would fall under (1), and we both agree
that should be allowed.

But:

> Also there was a move going in the opposite direction to allow a config
> file that is syntactically broken to be handled without the parser dying,
> primarily to help "git config -e".  In the longer term, our direction
> shouldn't be adding more die() in the git_config() callchain, but instead
> allowing it to report different kind of failures to let the caller act
> based on what it does and what the nature of failure is.

Yeah, that is the opposite of what I proposed above. But it is a step
towards lib-ifying the config code, which is probably a good thing. The
config code is an utter mess. I am a little hesitant to touch it just
because I don't think there is anything _broken_ in it exactly, but
every time I look at it, I am always shocked by how unnecessarily
complex it is.

I think the "right" thing to do would probably be to have a lib-ified
function to read the config, and then have a wrapper that 99% of the
programs use that just checks the error return and calls die() if there
is a problem. But such a cleanup is likely to introduce new bugs, so I
have let it be (also, because my time is not infinite and there are
other more interesting things to work on ;) ).

> For example, "gc" may say "I won't prune because I had to skip some of the
> lines in your .git/config because you have syntax errors in them, and I
> may have missed the definition of gc.pruneExpire you may wanted to place
> on them", while "diff" may ignore that kind of errors.

Yeah, that makes sense to me, and should be possible with a decent
lib-ified interface.

> Having said all that, my preference *for now* would be to ignore "there is
> no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors
> and die().

OK, then I think we are on the same page.

> There are some other glitches in the current git_config() callchain.
> 
>  - No config file anywhere gives an error.  I agree with you that this is
>    a bug.

Yep, and this patch fixes that.

>  - Having GIT_CONFIG=/no/such/file in the environment gives an error,
>    which is good.

Yep, and and this patch should leave that untouched (I didn't test that
specifically, but I checked that "git config --global" does, and I
assume they use the same code path. Of course, one never knows...).

> 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.

That all makes sense to me. My biggest worry is that we will need to be
checking for "INVALID" values in lots of places before actually using
the value from a variable. IOW, it is nice for code to be able to call
into some library call that respects a config variable without having to
care about doing some magic setup to say "Oh, by the way, I am
interesting in the value of diff.foo". At the same time, it is nice for
the library code to not have to say "We're using diff.foo. Let's make
sure somebody has checked the value before using it."

In other words, I would like not-too-syntactically-painful lazy values.
With no runtime overhead. In C. ;)

-Peff

Re: bash_completion outside repo

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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")) {

Re: bash_completion outside repo

[Jeff King]

On Fri, Sep 11, 2009 at 02:22:06PM -0700, Junio C Hamano wrote:

> 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.

OK, that example makes sense. But I'm a little dubious of how this
scales to something like color.diff.plain. Who is responsible for
checking? Do we do it at the beginning of every program which cares
about diff values? If so, what is the failure mode when we forget (and I
suspect we will, because it is easy for programs to call into unexpected
code that is three layers deep)?

I think something like that needs to "belong" to the diff code itself. I
guess in the case of "diff", we could check all diff-related config at
diff setup time. But what about something used in several places, like
core.quotepath?

-Peff

Re: bash_completion outside repo

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think something like that needs to "belong" to the diff code itself. I
> guess in the case of "diff", we could check all diff-related config at
> diff setup time.

Not necessarily.  You do not want to care about color configuration if you are
doing diff --raw for example.  The one that first uses the color variable
should be able to notice the breakage, no?

> But what about something used in several places, like
> core.quotepath?

Exactly the same way I checked what codepaths needed to fix for the
autosetupmerge stuff.  core.quotepath internally sets quote_path_fully,
and the sole user of quote_path_fully is sq_must_quote() which is only
used by next_quote_pos().  So you can have your check very isolated.

Re: bash_completion outside repo

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I think something like that needs to "belong" to the diff code itself. I
>> guess in the case of "diff", we could check all diff-related config at
>> diff setup time.
>
> Not necessarily.  You do not want to care about color configuration if you are
> doing diff --raw for example.  The one that first uses the color variable
> should be able to notice the breakage, no?
>
>> But what about something used in several places, like
>> core.quotepath?
>
> Exactly the same way I checked what codepaths needed to fix for the
> autosetupmerge stuff.  core.quotepath internally sets quote_path_fully,
> and the sole user of quote_path_fully is sq_must_quote() which is only
> used by next_quote_pos().  So you can have your check very isolated.

Note that I was _not_ defending the approach my illustration took.  I
merely was pointing out that core.quotepath and diff.color are _not_
valid counterexamples to it.

A better counter-proposal would be made something along this line:

	Currently, core.frotz and nitfol.rezrov both map internally to a
	variable xyzzy, so config parser could set xyzzy = INVALID_INFOCOM
        but the user of xyzzy cannot report which configuration variable
	caused the error with your illustration scheme.  Here is my
	attempt to solve this issue.

	...patch follows...

And I am sure we will be able to find such examples that my illustration
patch is not _exactly_ the best approach to solve pretty easily around
remote.*.* and branch.*.* variables.

Re: bash_completion outside repo

[Jeff King]

On Fri, Sep 11, 2009 at 02:57:28PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think something like that needs to "belong" to the diff code itself. I
> > guess in the case of "diff", we could check all diff-related config at
> > diff setup time.
> 
> Not necessarily.  You do not want to care about color configuration if you are
> doing diff --raw for example.  The one that first uses the color variable
> should be able to notice the breakage, no?
> 
> > But what about something used in several places, like
> > core.quotepath?
> 
> Exactly the same way I checked what codepaths needed to fix for the
> autosetupmerge stuff.  core.quotepath internally sets quote_path_fully,
> and the sole user of quote_path_fully is sq_must_quote() which is only
> used by next_quote_pos().  So you can have your check very isolated.

I guess I'm just worried that in doing this for _every_ variable we are
going to run across cases where variables are used in several different
codepaths, and we are going to end up adding a large number of tests for
"is this thing valid". And if we forget one, it's going to cause us to
access some sentinel value that may cause a segfault.

But that is just my gut feeling. I haven't actually looked at doing a
full-scale conversion.

-Peff

[PATCH] completion: Replace config --list with --get-regexp

[Todd Zullinger]

James Bardin noted that the completion spewed warnings when no git
config file is present.  This is likely a bug to be fixed in git config,
but it's also a good excuse to simplify the completion code by using the
--get-regexp option as Jeff King pointed out.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

Jeff King wrote:
> On Fri, Sep 11, 2009 at 07:36:51AM -0700, Shawn O. Pearce wrote:
>
>>> instead of just using "git config --get-regexp 'remote\..*\.url'", which
>>> would be slightly more efficient, and also doesn't have this bug. ;)
>>
>> F'king oversight.  You are right, this should be --get-regexp.
>> There isn't a reason here, probably other than "I forgot about
>> --get-regexp when I wrote the original code".
>
> OK. I will leave a bash-completion patch for you (or somebody else
> interested) as I don't use it myself.

Something like this perhaps?  Perhaps the commit message could use
adjustment to reference your fix for 'config --list'?

 contrib/completion/git-completion.bash |   30 +++++++++---------------------
 1 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bf688e1..d668fc9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -318,13 +318,9 @@ __git_remotes ()
 		echo ${i#$d/remotes/}
 	done
 	[ "$ngoff" ] && shopt -u nullglob
-	for i in $(git --git-dir="$d" config --list); do
-		case "$i" in
-		remote.*.url=*)
-			i="${i#remote.}"
-			echo "${i/.url=*/}"
-			;;
-		esac
+	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
+		i="${i#remote.}"
+		echo "${i/.url*/}"
 	done
 }
 
@@ -605,13 +601,9 @@ __git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
 __git_aliases ()
 {
 	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --list); do
-		case "$i" in
-		alias.*)
-			i="${i#alias.}"
-			echo "${i/=*/}"
-			;;
-		esac
+	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "alias\..*" 2>/dev/null); do
+		i="${i#alias.}"
+		echo "${i/ */}"
 	done
 }
 
@@ -1769,13 +1761,9 @@ _git_remote ()
 		;;
 	update)
 		local i c='' IFS=$'\n'
-		for i in $(git --git-dir="$(__gitdir)" config --list); do
-			case "$i" in
-			remotes.*)
-				i="${i#remotes.}"
-				c="$c ${i/=*/}"
-				;;
-			esac
+		for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
+			i="${i#remotes.}"
+			c="$c ${i/ */}"
 		done
 		__gitcomp "$c"
 		;;
-- 
1.6.4.2

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
That men do not learn very much from the lessons of history is the
most important of all the lessons of history.
    -- Aldous Huxley Collected Essays, 1959

Re: [PATCH] completion: Replace config --list with --get-regexp

[Shawn O. Pearce]

Todd Zullinger <tmz@pobox.com> wrote:
> James Bardin noted that the completion spewed warnings when no git
> config file is present.  This is likely a bug to be fixed in git config,
> but it's also a good excuse to simplify the completion code by using the
> --get-regexp option as Jeff King pointed out.
> 
> Signed-off-by: Todd Zullinger <tmz@pobox.com>

Thanks, looks good.

Trivially-acked-by: Shawn O. Pearce <spearce@spearce.org>

>  contrib/completion/git-completion.bash |   30 +++++++++---------------------
>  1 files changed, 9 insertions(+), 21 deletions(-)

-- 
Shawn.

Re: [PATCH] completion: Replace config --list with --get-regexp

[Bert Wesarg]

On Sat, Sep 12, 2009 at 20:31, Shawn O. Pearce <spearce@spearce.org> wrote:
> Todd Zullinger <tmz@pobox.com> wrote:
>> James Bardin noted that the completion spewed warnings when no git
>> config file is present.  This is likely a bug to be fixed in git config,
>> but it's also a good excuse to simplify the completion code by using the
>> --get-regexp option as Jeff King pointed out.
>>
>> Signed-off-by: Todd Zullinger <tmz@pobox.com>
>
> Thanks, looks good.
I have not looked into this, but what about pushurl?

Bert

Re: [PATCH] completion: Replace config --list with --get-regexp

[Todd Zullinger]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

Bert Wesarg wrote:
> I have not looked into this, but what about pushurl?

Is it reasonable to expect someone to have remote.<name>.pushurl and
not have remote.<name>.url set?  If not, then we should be fine, as
all the old code and this new code do is extract <name>.

OTOH, if there are some cases where setting pushurl and not url make
sense, extending the regex to catch pushurl as well is a simple matter
of changing 'remote\..*\.url' to 'remote\..*\.(push)?url' and, I
believe, using 'echo "${i/.*url*/}"' to strip off everything after the
remote <name>.

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fleas can be taught nearly anything that a Congressman can.
    -- Mark Twain


[-- Attachment #2: Type: application/pgp-signature, Size: 542 bytes --]

Re: [PATCH] completion: Replace config --list with --get-regexp

[Junio C Hamano]

Thanks, everybody.  Will apply.