Re: [PATCH v4] config: add conditional include

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Duy,

On Thu, 14 Jul 2016, Duy Nguyen wrote:

> On Thu, Jul 14, 2016 at 5:53 PM, Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> wrote:
> > Hi Duy,
> >
> > On Thu, 14 Jul 2016, Nguyễn Thái Ngọc Duy wrote:
> >
> >> Helped-by: Jeff King <peff@xxxxxxxx>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> >
> > 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

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

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

> >> +     ; 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

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]