git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Duy Nguyen <pclouds@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Sebastian Schuberth <sschuberth@gmail.com>,
	Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Subject: Re: [PATCH v4] config: add conditional include
Date: Sat, 16 Jul 2016 15:30:45 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.20.1607161507250.28832@virtualbox> (raw)
In-Reply-To: <CACsJy8AjVX1Say0srEq+ezGg=CzmbjBAt4PnuikXiqdnVC4G6g@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2926 bytes --]

Hi Duy,

On Thu, 14 Jul 2016, Duy Nguyen wrote:

> 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

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

  reply	other threads:[~2016-07-16 13:31 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-06-26  7:06 [PATCH] config: add conditional include Nguyễn Thái Ngọc Duy
2016-06-26 18:27 ` Jeff King
2016-06-27 16:14   ` Duy Nguyen
2016-06-27 16:20     ` Jeff King
2016-06-27 16:32       ` Duy Nguyen
2016-06-27 16:35         ` Jeff King
2016-06-28 17:26 ` [PATCH v2 0/2] Config " Nguyễn Thái Ngọc Duy
2016-06-28 17:26   ` [PATCH v2 1/2] add skip_prefix_mem helper Nguyễn Thái Ngọc Duy
2016-06-28 17:26   ` [PATCH v2 2/2] config: add conditional include Nguyễn Thái Ngọc Duy
2016-06-28 20:49     ` Jeff King
2016-06-29  4:06       ` Duy Nguyen
2016-06-28 23:11     ` Eric Sunshine
2016-07-12 16:42     ` [PATCH v3] " Nguyễn Thái Ngọc Duy
2016-07-13  7:21       ` Matthieu Moy
2016-07-13  7:26         ` Jeff King
2016-07-13 12:48           ` Matthieu Moy
2016-07-13 15:57         ` Duy Nguyen
2016-07-14 15:33       ` [PATCH v4] " Nguyễn Thái Ngọc Duy
2016-07-14 15:53         ` Johannes Schindelin
2016-07-14 16:13           ` Duy Nguyen
2016-07-16 13:30             ` Johannes Schindelin [this message]
2016-07-16 14:48               ` Duy Nguyen
2016-07-16 15:08               ` Jeff King
2016-07-16 16:36                 ` Johannes Schindelin
2016-07-16 16:47                   ` Jeff King
2016-07-17  8:15                     ` Johannes Schindelin
2016-07-20 13:31                       ` Jeff King
2016-07-20 22:07                         ` Junio C Hamano
2016-07-20 16:39                     ` Jakub Narębski
2016-08-13  8:40         ` Duy Nguyen
2016-08-19 13:54           ` Jeff King
2016-08-20 21:08             ` Jakub Narębski
2016-08-22 12:43               ` Duy Nguyen
2016-08-22 12:59                 ` Matthieu Moy
2016-08-22 13:09                   ` Duy Nguyen
2016-08-22 13:22                     ` Matthieu Moy
2016-08-22 13:32                       ` Duy Nguyen
2016-08-23 13:42                         ` Johannes Schindelin
2016-08-24  9:37                           ` Duy Nguyen
2016-08-24 12:44                             ` Jakub Narębski
2016-08-24 14:17                               ` Jeff King
2016-06-28 20:28   ` [PATCH v2 0/2] Config " Jeff King
2016-06-28 20:51     ` Matthieu Moy
2016-06-28 21:03       ` Jeff King
2016-06-29  4:09     ` Duy Nguyen
2016-06-28 22:11   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.20.1607161507250.28832@virtualbox \
    --to=johannes.schindelin@gmx.de \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=sschuberth@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).