Re: git silently ignores include directive with single quotes

From: Jeff King <peff@peff.net>
Date: Sep 9, 2018, 02:29 UTC
Message-ID: <20180909022931.GA13485@sigill.intra.peff.net>
In-Reply-To: <87musr7h7q.fsf@evledraar.gmail.com>

On Sun, Sep 09, 2018 at 12:32:57AM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 18 quoted lines

> > You could even do:
> >
> >   [include]
> >   warnOnMissing = false
> >   path = one
> >   warnOnMissing = true
> >   path = two
> >
> > to treat two includes differently (though I'm not sure why you would
> > want to).
> 
> I think this is introducing a brand new caveat into our *.ini syntax,
> i.e. that we're sensitive to the order in which we're parsing
> *different* keys.
> 
> I.e. we already had the concept that some keys override existing sets
> (e.g. user.name), but not that a x.y=foo controls the behavior of a
> subsequent a.b=bar, or the other way around.

This already exists. For example:

  echo '[foo]bar = inc' >config.inc
  echo '[foo]bar = main' >config.main
  echo '[include]path = config.inc' >>config.main
  git config -f config.main --includes foo.bar

Arriving at that answer requires expanding the include's contents at the exact same spot in the file.

As far as I know this is the only case, though, so include.path really is special. And this would be expanding that specialness to other things in include.*. But I'm not sure that is really that big a deal. That warnOnMissing isn't meant to be just a normal key. It's an order-sensitive directive for further parsing, just like include.path is. Either the parser understands includes (and has to handle these) or it doesn't.

So I'm not worried about any burden on the parsing side, but...

Show 7 quoted lines

> This also makes programmatic (via "git config") editing of the config
> hard, we'd need to introduce something like:
> 
>     git config -f ~/.gitconfig a.b bar
>     git config -f ~/.gitconfig --before a.b x.y foo
> 
> To set a.b=bar before x.y=foo, or --after or whatever.

Yes, I don't think "git config include.warnOnMissing true" would be very useful, because it would generally be added after any includes you have, and therefore not affect them.

I think this is generally an issue with include.path, too. My assumption has been that anybody at the level of using includes is probably going to be hand-editing anyway.

But if include.* is special on the parsing side, I don't know that it is that bad to make it special on the writing side, too. I.e., to recognize "git config include.warnOnMissing true" and always add it at the head of any existing include block.

It certainly _feels_ hacky, but I think it would behave sensibly and predictably. And it would just work, as opposed to requiring something like "--before", which would be quite a subtle gotcha for somebody to forget to use.

> Yeah, we could do that, and it wouldn't break the model described above,
> We can make that work, but this would be nasty. E.g. are we going to
> treat EACCES and ENOENT the same way in this construct?

I don't have a strong opinion (after all, I already wrote the behavior I thought was reasonable long ago ;) ). So I think it would be up to somebody to propose. We do already report and die on EACCES (and basically any other error except ENOENT). So if we did treat them both as a warning, that would be a weakening for EACCES.

-Peff