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:

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:

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 can't reproduce this on either Fedora or CentOS.

On Fri, Sep 11, 2009 at 9:33 AM, Todd Zullinger <tmz@pobox.com> wrote:

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

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 <peff@peff.net> wrote:

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

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

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:

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(-)

Jeff King <peff@peff.net> writes:

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:

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:

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

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

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

Yep, and this patch fixes that.

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

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

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

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(-)

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

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

Jeff King <peff@peff.net> writes:

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?

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.

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

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.

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

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

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:

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(-)

Todd Zullinger <tmz@pobox.com> wrote:

Thanks, looks good.

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

On Sat, Sep 12, 2009 at 20:31, Shawn O. Pearce <spearce@spearce.org> wrote:

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

Bert

Bert Wesarg wrote:

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

Thanks, everybody.  Will apply.