Re: bash_completion outside repo

From: Jeff King <peff@peff.net>
Date: Sep 11, 2009, 21:12 UTC
Message-ID: <20090911211209.GA26527@coredump.intra.peff.net>
In-Reply-To: <7viqfpw6tv.fsf@alter.siamese.dyndns.org>

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 16 quoted lines

> 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