Re: [PATCH v3 0/9] Fix the early config

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 8, 2017, 16:18 UTC
Message-ID: <alpine.DEB.2.20.1703081701100.3767@virtualbox>
In-Reply-To: <20170308073035.pt5ysp6bzpozoqps@sigill.intra.peff.net>

Hi Peff,

On Wed, 8 Mar 2017, Jeff King wrote:

> Another "non-gentle" thing I noticed here while looking at
> another thread: the repository-format version check uses the config
> parser, which will die() in certain circumstances. [...]

Yes, that is part of the reason why I was not eager to add that check to discover_git_directory(). The config code is die()-happy.

This is a much bigger problem, of course, and related to a constant gripe of mine: I *always* get the quoting wrong in my aliases. Always. And when I want to fix it, `git config -e` simply errors out because of an invalid config. Yes, Git, I know, that is the exact reason why I want to edit the config in the first place.

I am certain you will agree that this is a different topic, therefore subject to a separate patch series.

In any case, I am fairly certain that the examples you showed demonstrate that the config has to be rather broken for this patch series to have a negative impact. And it still would report the broken config so that the user is not blocked (she can fix the config and call the paginating command again).

Show 7 quoted lines

> On Tue, Mar 07, 2017 at 03:31:43PM +0100, Johannes Schindelin wrote:
> 
> > And another change: the GIT_DIR_NONE value was handled incorrectly in
> > discover_git_directory().
> 
> This is the "if (setup_git_directory_1() <= 0)" change from the
> interdiff? That's subtle.

Yes, it is subtle.

> The compiler could have noticed if we used a switch statement here. But
> then any new error conditions would have to be added to that switch
> statement.

We could still do that.

Show 5 quoted lines

> > I am slightly disappointed that the these additional problems were not
> > spotted in any review but my own. And I had not even included a Duck.
> 
> Get used to being disappointed, I guess. A non-zero number of bugs will
> slip through when writing code _and_ when reviewing it.

I know that. I know that bugs are prone to come in through code contributions. I don't go for 100%. But I would hope for a better rate than we have right now: we pride ourselves in the OSS community to make bugs shallow. I really would like to believe that we catch bugs rather than discuss formatting (which should be automated) or white-space mangling (which should not even be a problem if we were truly open for contributions).

Maybe I am just a grumpy old guy. But I *hate* seeing how much buggy code we get into Git, despite having a review process that does a very good job of deterring seasoned developers from contributing.

I really wish it were different. I really wish that we did a better job at catching bugs before they enter `master`. I really wish that I could be proud of our code review process.

> > [ceil_offset]
> > Hopefully that clears up the picture?
> 
> Yes, it does. Thanks.

Perfect. Then the time I spent trying to figure all of this out was not spent in vain.

Ciao, Dscho