git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	sschuberth@gmail.com, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Subject: Re: [PATCH v6 1/1] config: add conditional include
Date: Sun, 26 Feb 2017 01:12:20 -0500	[thread overview]
Message-ID: <20170226061220.nizc4xeucx6ks6vq@sigill.intra.peff.net> (raw)
In-Reply-To: <20170224131425.32409-4-pclouds@gmail.com>

On Fri, Feb 24, 2017 at 08:14:25PM +0700, Nguyễn Thái Ngọc Duy wrote:

> +static int include_condition_is_true(const char *cond, size_t cond_len)
> +{
> +	/* no condition (i.e., "include.path") is always true */
> +	if (!cond)
> +		return 1;
> +
> +	if (skip_prefix_mem(cond, cond_len, "gitdir:", &cond, &cond_len))
> +		return include_by_gitdir(cond, cond_len, 0);
> +	else if (skip_prefix_mem(cond, cond_len, "gitdir/i:", &cond, &cond_len))
> +		return include_by_gitdir(cond, cond_len, 1);
> +
> +	error(_("unrecognized include condition: %.*s"), (int)cond_len, cond);
> +	/* unknown conditionals are always false */
> +	return 0;
> +}

This "error()" is a nice warning to people who have misspelled the
condition (or are surprised that an old version does not respect their
condition). But it seems out of place with the rest of the compatibility
strategy, which is to quietly ignore include types we don't understand.

For example, if your patch ships in v2.13 and we add the "foo:"
condition in v2.14, then with config like:

  [includeif "foo:bar"]
  path = whatever

you get:

  v2.12: silently ignore
  v2.13: loudly ignore
  v2.14: works

which seems odd.

If we do keep it, it should probably be warning(). I would expect
"error:" to indicate that some requested operation could not be
completed, but this is just "by the way, I ignored this garbage".

-Peff

PS I know we try to avoid dashes in the section names, but "includeIf"
   and "includeif" just look really ugly to me. Maybe it is just me,
   though.

  reply	other threads:[~2017-02-26  6:22 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-23 12:23 [PATCH v5 0/1] Conditional config include Nguyễn Thái Ngọc Duy
2017-02-23 12:23 ` [PATCH v5 1/1] config: add conditional include Nguyễn Thái Ngọc Duy
2017-02-23 19:59   ` Junio C Hamano
2017-02-24  9:37     ` Duy Nguyen
2017-02-24 17:46       ` Junio C Hamano
2017-02-26  6:07         ` Jeff King
2017-02-27 18:42           ` Junio C Hamano
2017-03-06 22:44   ` Stefan Beller
2017-03-07  8:47     ` Jeff King
2017-03-07 18:39       ` Stefan Beller
2017-02-24 13:14 ` [PATCH v6 0/1] Conditional config include Nguyễn Thái Ngọc Duy
2017-02-24 13:14   ` [PATCH v6 1/1] config: add conditional include Nguyễn Thái Ngọc Duy
2017-02-24 18:35     ` Junio C Hamano
2017-02-24 19:48     ` Ramsay Jones
2017-02-24 22:08     ` Philip Oakley
2017-02-26  3:02       ` Duy Nguyen
2017-02-26 12:27         ` Philip Oakley
2017-02-24 13:14   ` [PATCH v6 0/1] Conditional config include Nguyễn Thái Ngọc Duy
2017-02-24 13:18     ` Duy Nguyen
2017-02-24 13:14   ` [PATCH v6 1/1] config: add conditional include Nguyễn Thái Ngọc Duy
2017-02-26  6:12     ` Jeff King [this message]
2017-03-01 11:26   ` [PATCH v7 0/3] Conditional config include Nguyễn Thái Ngọc Duy
2017-03-01 11:26     ` [PATCH v7 1/3] config.txt: clarify multiple key values in include.path Nguyễn Thái Ngọc Duy
2017-03-01 17:40       ` Junio C Hamano
2017-03-01 11:26     ` [PATCH v7 2/3] config.txt: reflow the second include.path paragraph Nguyễn Thái Ngọc Duy
2017-03-01 11:26     ` [PATCH v7 3/3] config: add conditional include Nguyễn Thái Ngọc Duy
2017-03-01 17:16       ` Ramsay Jones
2017-03-01 17:47       ` Junio C Hamano
2017-03-03  6:30         ` Jeff King
2017-03-03 19:05           ` Junio C Hamano
2017-03-03 22:24             ` Jeff King
2017-03-01 17:52     ` [PATCH v7 0/3] Conditional config include Junio C Hamano
2017-03-03  6:33     ` Jeff King
2017-03-03  9:52       ` Duy Nguyen
2017-03-03 19:13         ` Junio C Hamano
2017-03-03 21:08         ` Junio C Hamano
2017-03-03 18:24       ` Junio C Hamano
2017-03-03 22:22         ` Jeff King
2017-03-03 23:22           ` 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=20170226061220.nizc4xeucx6ks6vq@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --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).