git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Libor Pechacek <lpechacek@suse.cz>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH v3] Documentation fixes in git-config
Date: Thu, 1 Mar 2012 04:08:28 -0500	[thread overview]
Message-ID: <20120301090828.GA16033@sigill.intra.peff.net> (raw)
In-Reply-To: <20120301081941.GA31079@fm.suse.cz>

On Thu, Mar 01, 2012 at 09:19:42AM +0100, Libor Pechacek wrote:

> Variable names must start with an alphabetic character, regexp config key
> matching has its limits.
> [...]
> This patch has fallen through the cracks, therefore I re-send it.  Previous
> discussion about this patch is at http://www.spinics.net/lists/git/msg149593.html.
> The only change I've done since version 2 of this patch is replacing
> apostrophes with backticks in the first hunk.

Wow, it's been a while. :)

Generally it looks OK to me, but I have two comments:

>  Syntax
>  ~~~~~~
> @@ -54,9 +55,10 @@ All the other lines (and the remainder of the line after the section
>  header) are recognized as setting variables, in the form
>  'name = value'.  If there is no equal sign on the line, the entire line
>  is taken as 'name' and the variable is recognized as boolean "true".
> -The variable names are case-insensitive and only alphanumeric
> -characters and `-` are allowed.  There can be more than one value
> -for a given variable; we say then that variable is multivalued.
> +The variable names are case-insensitive, allow only alphanumeric characters
> +and `-`, and must start with an alphabetic character.  There can be more
> +than one value for a given variable; we say then that variable is
> +multivalued.

Not an error you introduced, but should it be "...we say then that _the_
variable is multivalued".

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index aa8303b..a54fee8 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -85,8 +85,13 @@ OPTIONS
>  	is not exactly one.
>  
>  --get-regexp::
> -	Like --get-all, but interprets the name as a regular expression.
> -	Also outputs the key names.
> +	Like --get-all, but interprets the name as a regular expression and
> +	writes out the key names.  Regular expression matching is currently
> +	case-sensitive and done against a canonicalized version of the key
> +	in which section and variable names are lowercased, but subsection
> +	names are not.  Regular expressions are partially lower-cased
> +	before matching (everything before the first dot and after the last
> +	dot), which makes things like "Core.*' work.

I know I ack'ed this last time around, but reading it fresh, I think we
are probably better off to just not mention the down-casing at all. It's
just confusing, and people shouldn't depend on it. They should know that
they are comparing against the canonical name, and should use lowercase
in their regex. I.e., just cut out the last sentence from there.

-Peff

  reply	other threads:[~2012-03-01  9:08 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-01-08 14:46 git-config does not check validity of variable names Libor Pechacek
2011-01-11  5:59 ` Jeff King
2011-01-19 10:01   ` Libor Pechacek
2011-01-19 14:11     ` [PATCH] Sanity-ckeck config " Libor Pechacek
2011-01-20 23:22       ` Jeff King
2011-01-21  0:06         ` Jeff King
2011-01-19 14:14     ` [PATCH] Documentation fixes in git-config Libor Pechacek
2011-01-21  0:27       ` Jeff King
2011-01-21 10:20         ` Libor Pechacek
2011-01-21 10:25           ` [PATCH v2] " Libor Pechacek
2011-01-21 16:25             ` Jeff King
2011-01-23 19:46               ` Libor Pechacek
2012-03-01  8:19             ` [PATCH v3] " Libor Pechacek
2012-03-01  9:08               ` Jeff King [this message]
2012-03-01 10:54                 ` Libor Pechacek
2012-03-01 16:24                 ` Junio C Hamano
2012-03-01 10:59               ` [PATCH v4] " Libor Pechacek
2011-01-21 10:02 ` [PATCH] Sanity-ckeck config variable names Libor Pechacek
2011-01-21 10:23   ` [PATCH v2] " Libor Pechacek
2011-01-21 16:23     ` Jeff King
2011-01-27 14:28       ` [PATCH v3] Sanity-check " Libor Pechacek
2011-01-27 22:45         ` Junio C Hamano
2011-01-28 14:53           ` Libor Pechacek
2011-01-30 19:40             ` [PATCH v4] " Libor Pechacek
2011-02-10 22:49               ` Junio C Hamano
2011-02-11 18:52                 ` Libor Pechacek
2011-01-27 14:52 ` [PATCH] Disallow empty section and " Libor Pechacek
2011-01-30 20:34   ` [PATCH v2] " Libor Pechacek
2011-01-31  7:48     ` Johannes Sixt
2011-01-31  9:17       ` Libor Pechacek
2011-01-31  9:29         ` Johannes Sixt
2011-01-31 13:08           ` [PATCH v3] " Libor Pechacek
2011-01-31 16:48             ` Jens Lehmann
2011-02-01  7:13               ` [PATCH v4] " Libor Pechacek
2011-02-10 22:49                 ` 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=20120301090828.GA16033@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=lpechacek@suse.cz \
    /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).