bash_completion outside repo

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

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

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

On Fri, Sep 11, 2009 at 9:33 AM, Todd Zullinger [off-list ref] wrote:

james bardin wrote:

quoted

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
[off-list ref] 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.

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

Jeff King [off-list ref] 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.

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

quoted

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

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

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

Jeff King [off-list ref] 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().

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

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

Jeff King wrote:

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

quoted

quoted

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

Todd Zullinger [off-list ref] 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 <redacted>

Thanks, looks good.

Trivially-acked-by: Shawn O. Pearce [off-list ref]

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

-- 
Shawn.

On Sat, Sep 12, 2009 at 20:31, Shawn O. Pearce [off-list ref] wrote:

Todd Zullinger [off-list ref] wrote:

quoted

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

Thanks, looks good.

I have not looked into this, but what about pushurl?

Bert

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