Re: git silently ignores include directive with single quotes

From: Jeff King <peff@peff.net>
Date: Sep 11, 2018, 20:57 UTC
Message-ID: <20180911205709.GA25828@sigill.intra.peff.net>
In-Reply-To: <xmqqk1nrojpq.fsf@gitster-ct.c.googlers.com>

On Tue, Sep 11, 2018 at 01:36:01PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I think that's syntactically invalid. At any rate, there are clearly
> > three options for setting a bit:
> >
> >   1. In the section header (+include, or Ævar's includeIf suggestion).
> >
> >   2. In another key (which looks pretty clean, but does introduce
> >      ordering constraints).
> >
> >   3. In the key name (maybePath or similar).
> >
> > I don't have a huge preference between them.
> 
> What's the longer term goal for the endgame?  Is it acceptable that
> include.path will stay to be "optional include" for compatibility
> with users' existing configuration files, and include.requiredpath
> or similar gets introduced to allow people who want to get warned?
> Or do we want the usual multi-step deprecation dance where the first
> phase introduces include.maybepath and include.path starts warning
> against missing one, encouraging it to be rewritten to maybepath?

I don't see much point in introducing include.requiredPath. It might be useful for people who want to be extra-careful with their includes, but it would not really help users who simply made a spelling error and didn't know how to debug it.

So switching the default for include.path and providing an escape hatch seems like the more useful path (if we indeed want to do one of these; adding better debugging like GIT_TRACE_CONFIG is yet another option).

As far as deprecation, it depends on what the new behavior is. If it is simply that include.path will generate a warning on a missing file, I don't think there is any point in a multi-step dance. The endgame is a warning, which is no different than the deprecated-stage behavior. :)

If the endgame is to die(), then I'd agree that there should be a warning in the middle.

Between all those things I mentioned (or simply leaving it as-is), I really don't have a strong feeling. I hoped people who did would generate a patch to give something concrete to review.

Show 5 quoted lines

> I have mild preference against #2, as I suspect that the ordering
> constraints makes it harder to understand to end users.  Between #1
> and #3, there wouldn't be much difference, whether the endgame is
> "add a stricter variant that is opt in" or "migrate to a stricter
> default".

The thing that #2 buys you is that multiple such bits could be combined. If we imagine that later there is another choice to make in interpreting include.path with two options, "foo" and "bar", then we would be stuck with:

  include.maybeFooPath
  include.maybeBarPath
  include.fooPath
  include.barPath

and of course it only gets worse with a third one. Whereas with independent options, you can do:

  [include]
  warnOnMissing = false
  otherPreference = bar
  path = ...

I dunno. The combinatorics might not be too bad if we document the required order, and actually code the parsing side like:

  if (!skip_prefix(key, "maybe", &key))
	warn_on_missing = 0;
  if (!skip_prefix(key, "foo", &key))
        other_pref = "foo";
  ...and so on

It's kind of hacky, but it does encode the bits into the name. As long as they remain bits, and not, say, arbitrary strings.

-Peff