Re: [PATCH v4] config: add conditional include

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 16, 2016, 13:30 UTC
Message-ID: <alpine.DEB.2.20.1607161507250.28832@virtualbox>
In-Reply-To: <CACsJy8AjVX1Say0srEq+ezGg=CzmbjBAt4PnuikXiqdnVC4G6g@mail.gmail.com>

Hi Duy,

On Thu, 14 Jul 2016, Duy Nguyen wrote:

Show 12 quoted lines

> On Thu, Jul 14, 2016 at 5:53 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Duy,
> >
> > On Thu, 14 Jul 2016, Nguyễn Thái Ngọc Duy wrote:
> >
> >> Helped-by: Jeff King <peff@peff.com>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> >
> > This commit message is quite a bit short on details.
> 
> Because it's fully documented in config.txt.

Surely there is more information left for the commit message, such as: what motivated the patch, what alternative solutions were considered, was any thought given to how this might break down, etc

Show 14 quoted lines

> > I still fail to see what use case this would benefit,
> 
> It allows you to group repos together. The first mail that started all
> this [1] is one of the use cases: you may want to use one identity in
> a group of repos, and another identity in some other. I want some
> more, to use a private key one some repos and another private key on
> some other. Some more settings may be shareable this way, like coding
> style-related (trailing spaces or not...)
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/273811
> 
> > and I already start to suspect that the change would open a can of worms that might not be desired.
> 
> You can choose not to use it, I guess.

Sadly, as the maintainer I am unable to share in that luxury of yours.

Show 5 quoted lines

> From what I see it's nothing super tricky. The config hierarchy we have
> now is: system -> per-user -> repo. This could change this to: system ->
> per-user -> per-directory -> repo. You could create a different
> hierarchy, but with git-config now showing where a config var comes
> from, it's not hard to troubleshoot.

The more indirection you allow, the harder it gets. And that problem is exacerbated when allowing conditional includes.

Show 9 quoted lines

> >> +     ; include if $GIT_DIR is /path/to/foo/.git
> >> +     [include "gitdir:/path/to/foo/.git"]
> >> +             path = /path/to/foo.inc
> >
> > I find this way to specify a conditional unintuitive. Reading
> > "gitdir:/path/to/foo/.git" suggests to me that the Git dir is *re-set*,
> > not that this is a condition.
> 
> Well.. to me it's no different than [remote "foo"] to apply stuff to "foo".

Except that "include" is an imperative and "remote" is not.

Quite frankly, this conditional business scares me. If you introduce it for [include], users will want it for every config setting. And the current syntax is just not up to, say, making user.name conditional on anything.

As an alternative solution to your problem, you could of course avoid all conditional includes. Simply by adding the include.path settings explicitly to the configs that require them. Now, that would make reasoning and trouble-shooting simple, wouldn't it?

And the most beautiful aspect of it: no patch needed.

Ciao, Dscho