[RFC] Implementing git config handling in Git.pm

[RFC] Implementing git config handling in Git.pm

[Frank Lichtenheld]

Hi.

A week ago or so when I presented my GITCVS::config patch I mentioned
that we should better implement most of it in Git.pm. I would like to
do so but get a bit of input first on how to implement it.

Targets:
 1) We should offer to parse the config only once since that is
   a huge performance gain if the caller wants to use several
   values from it.
 2) The parsing should be complete and safe.
 3) If at all possible, we should not have to implement a
   complete parser in Perl, since that is just needless
   code to maintain.

Possible Solutions:
 1) Call git-config.
   Pro: Easy to implement
   Contra: Violates at least target 2. Neither git-config --get nor
   git-config --list offer a complete and safe view on the config
   file. Just try including = in a subsection name (--list) or newlines in
   a value (both) to see what I mean.
 2) Extend git-config to give a machine parsable output and then
    proceed with solution 1
   Pro: Still reasonably easy to implement (?). Would benefit
    other scripts, too.
   Contra: Neither the fastest nor the most flexible
    solution.
 3) Try to use the C code from config.c directly.
   Pro: Probably the fastest solution due to avoiding the
    forks.
   Contra: Probably a bit more complex (any XS experts here?),
    both to implement and to maintain.
 4) Implement an own git config parser in Perl
   Pro: Might be actually easier than 3 and faster than 2
   Contra: See target 3

I would go for solution 2. Any reason to prefer one of the
others (or one I didn't even think of)?

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [RFC] Implementing git config handling in Git.pm

[Petr Baudis]

  Hi,

On Mon, May 21, 2007 at 12:59:54AM CEST, Frank Lichtenheld wrote:
> Possible Solutions:
>  1) Call git-config.
>    Pro: Easy to implement
>    Contra: Violates at least target 2. Neither git-config --get nor
>    git-config --list offer a complete and safe view on the config
>    file. Just try including = in a subsection name (--list) or newlines in
>    a value (both) to see what I mean.
>  2) Extend git-config to give a machine parsable output and then
>     proceed with solution 1
>    Pro: Still reasonably easy to implement (?). Would benefit
>     other scripts, too.
>    Contra: Neither the fastest nor the most flexible
>     solution.

  Yes, this might be fine for you. The argument for 4 (implementing our
own in Perl) is that we would like it to be _real_ fast for gitweb
(especially the summary page needs to look at each repository). But it
would be a question of a benchmark to look really how much would calling
git-config slow us down. So as at least a proof-of-concept and initial
implementation I think this is more than fine, and we can proceed to
implement our own parser only when it's clearly needed.

>  3) Try to use the C code from config.c directly.
>    Pro: Probably the fastest solution due to avoiding the
>     forks.
>    Contra: Probably a bit more complex (any XS experts here?),
>     both to implement and to maintain.

  There was various trouble with XS in the past, and I think the general
feel was that we want to get back to using XS again sometime, but only
when Git will be reasonably libified (to support multiple repositories
at once, etc.).

>  4) Implement an own git config parser in Perl
>    Pro: Might be actually easier than 3 and faster than 2
>    Contra: See target 3

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
Ever try. Ever fail. No matter. // Try again. Fail again. Fail better.
		-- Samuel Beckett

[PATCH] config: Add --quoted option to produce machine-parsable output

[Frank Lichtenheld]

This option will enclose key names in quotes (") if they
contain a subsection and then escape " and \. It will also
escape line breaks in values. Together this should produce
an easily parsable output.

Affects --list and --get-*

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 builtin-config.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 83 insertions(+), 9 deletions(-)

 Will add asciidoc documentation and test cases if people think that this is
 a good idea.

 I'm writing C about once a year, so I really don't mind being told if it's
 crap ;)

diff --git a/builtin-config.c b/builtin-config.c
index b2515f7..454cf4e 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -2,7 +2,7 @@
 #include "cache.h"
 
 static const char git_config_set_usage[] =
-"git-config [ --global | --system ] [ --bool | --int ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list";
+"git-config [ --global | --system ] [ --bool | --int ] [--quoted] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list";
 
 static char *key;
 static regex_t *key_regexp;
@@ -12,14 +12,73 @@ static int use_key_regexp;
 static int do_all;
 static int do_not_match;
 static int seen;
+static int quoted;
 static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;
 
+static char* quote_key(const char *key_)
+{
+	char *pos1, *pos2;
+	char* key_quot;
+
+	pos1 = strchr(key_, '.');
+	pos2 = strrchr(key_, '.');
+	if (pos1 != pos2) { /* has subsection */
+		char* key;
+		key = xmalloc(strlen(key_)*2 + 1);
+		key_quot = key;
+		*key++ = '"';
+		while (*key_) {
+			if (*key_ == '"' || *key_ == '\\')
+				*key++ = '\\';
+			*key++ = *key_++;
+		}
+		*key++ = '"';
+		*key = 0;
+	} else {
+		key_quot = xstrdup(key_);
+	}
+	return key_quot;
+}
+
+static char* quote_value(const char* value_)
+{
+	char *val_quot, *val;
+	val = xmalloc(strlen(value_)*2 + 1);
+	val_quot = val;
+
+	while (*value_) {
+		if (*value_ == '\n') {
+			*val++ = '\\';
+			*val++ = 'n';
+			value_ += 2;
+		} else
+			*val++ = *value_++;
+	}
+	*val = 0;
+
+	return val_quot;
+}
+
 static int show_all_config(const char *key_, const char *value_)
 {
-	if (value_)
-		printf("%s=%s\n", key_, value_);
-	else
-		printf("%s\n", key_);
+	if (!quoted) {
+		if (value_)
+			printf("%s=%s\n", key_, value_);
+		else
+			printf("%s\n", key_);
+	} else {
+		char* key_quot = quote_key(key_);
+
+		if (value_) {
+			char* val_quot = quote_value(value_);
+			printf("%s=%s\n", key_quot, val_quot);
+			free(val_quot);
+		} else
+			printf("%s\n", key_quot);
+
+		free(key_quot);
+	}
+
 	return 0;
 }
 
@@ -38,16 +97,26 @@ static int show_config(const char* key_, const char* value_)
 			  regexec(regexp, (value_?value_:""), 0, NULL, 0)))
 		return 0;
 
-	if (show_keys)
-		printf("%s ", key_);
+	if (show_keys) {
+		if (quoted) {
+			char* key = quote_key(key_);
+			printf("%s ", key);
+			free(key);
+		} else
+			printf("%s ", key_);
+	}
 	if (seen && !do_all)
 		dup_error = 1;
 	if (type == T_INT)
 		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
 	else if (type == T_BOOL)
 		vptr = git_config_bool(key_, value_) ? "true" : "false";
-	else
-		vptr = value_?value_:"";
+	else {
+		if (quoted)
+			vptr = value_?quote_value(value_):"";
+		else
+			vptr = value_?value_:"";
+	}
 	seen++;
 	if (dup_error) {
 		error("More than one value for the key %s: %s",
@@ -56,6 +125,9 @@ static int show_config(const char* key_, const char* value_)
 	else
 		printf("%s\n", vptr);
 
+	if (quoted && value_)
+		free((char *)vptr);
+
 	return 0;
 }
 
@@ -141,6 +213,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			type = T_INT;
 		else if (!strcmp(argv[1], "--bool"))
 			type = T_BOOL;
+		else if (!strcmp(argv[1], "--quoted"))
+			quoted = 1;
 		else if (!strcmp(argv[1], "--list") || !strcmp(argv[1], "-l"))
 			return git_config(show_all_config);
 		else if (!strcmp(argv[1], "--global")) {
-- 
1.5.2-rc3.GIT

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> This option will enclose key names in quotes (") if they
> contain a subsection and then escape " and \. It will also
> escape line breaks in values. Together this should produce
> an easily parsable output.
>
> Affects --list and --get-*
>
> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
> ---
>  builtin-config.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 83 insertions(+), 9 deletions(-)
>
>  Will add asciidoc documentation and test cases if people think that this is
>  a good idea.
>
>  I'm writing C about once a year, so I really don't mind being told if it's
>  crap ;)

We probably would want to make this compatible with the quoting
rules various fo "host" language have.  quote.c has host
language support to implement {perl,python,tcl}_quote_print()
for single string values or keys, so we should extend that idea.

In your application, what you are trying to do is to show a
"hash" (key => value) in a notation that is friendly to the host
language.

Git.pm could simply do:

	my $eval = `git config --perl --get-regexp 'gitcvs\..*'`;
	my $cfg = eval "$eval";

if you code your "perl" notation to produce:

	+{
		'gitcvs.ext.enabled' => 'false',
                'gitcvs.logfile' => '/var/log/gitcvs.log',
	}

in order to read things in.

Hmm?

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Johannes Schindelin]

Hi,

On Mon, 21 May 2007, Junio C Hamano wrote:

> Frank Lichtenheld <frank@lichtenheld.de> writes:
> 
> > This option will enclose key names in quotes (") if they
> > contain a subsection and then escape " and \. It will also
> > escape line breaks in values. Together this should produce
> > an easily parsable output.
> >
> > Affects --list and --get-*
> >
> > Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
> > ---
> >  builtin-config.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 files changed, 83 insertions(+), 9 deletions(-)
> >
> > Will add asciidoc documentation and test cases if people think that 
> > this is a good idea.
> >
> > I'm writing C about once a year, so I really don't mind being told if 
> > it's crap ;)
> 
> We probably would want to make this compatible with the quoting rules 
> various fo "host" language have.  quote.c has host language support to 
> implement {perl,python,tcl}_quote_print() for single string values or 
> keys, so we should extend that idea.
> 
> In your application, what you are trying to do is to show a "hash" (key 
> => value) in a notation that is friendly to the host language.
> 
> Git.pm could simply do:
> 
> 	my $eval = `git config --perl --get-regexp 'gitcvs\..*'`;
> 	my $cfg = eval "$eval";
> 
> if you code your "perl" notation to produce:
> 
> 	+{
> 		'gitcvs.ext.enabled' => 'false',
>                 'gitcvs.logfile' => '/var/log/gitcvs.log',
> 	}
> 
> in order to read things in.
> 
> Hmm?

IOW, something like 
http://article.gmane.org/gmane.comp.version-control.git/36922

Hmm?

Ciao,
Dscho

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> Git.pm could simply do:
>> 
>> 	my $eval = `git config --perl --get-regexp 'gitcvs\..*'`;
>> 	my $cfg = eval "$eval";
>> 
>> if you code your "perl" notation to produce:
>> 
>> 	+{
>> 		'gitcvs.ext.enabled' => 'false',
>>                 'gitcvs.logfile' => '/var/log/gitcvs.log',
>> 	}
>> 
>> in order to read things in.
>> 
>> Hmm?
>
> IOW, something like 
> http://article.gmane.org/gmane.comp.version-control.git/36922

Indeed (perhaps except fixing minor details like not hijacking
the variable name).  Care to resubmit with docs and tests?

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Johannes Schindelin]

Hi,

On Mon, 21 May 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> Git.pm could simply do:
> >> 
> >> 	my $eval = `git config --perl --get-regexp 'gitcvs\..*'`;
> >> 	my $cfg = eval "$eval";
> >> 
> >> if you code your "perl" notation to produce:
> >> 
> >> 	+{
> >> 		'gitcvs.ext.enabled' => 'false',
> >>                 'gitcvs.logfile' => '/var/log/gitcvs.log',
> >> 	}
> >> 
> >> in order to read things in.
> >> 
> >> Hmm?
> >
> > IOW, something like 
> > http://article.gmane.org/gmane.comp.version-control.git/36922
> 
> Indeed (perhaps except fixing minor details like not hijacking
> the variable name).  Care to resubmit with docs and tests?

Well, my version was not good, as Eric pointed out.

Do you want me to clean up Eric's version (according to my own comments, 
that is)?

Ciao,
Dscho

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Jan Hudec]

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

On Mon, May 21, 2007 at 19:46:59 +0200, Frank Lichtenheld wrote:
> This option will enclose key names in quotes (") if they
> contain a subsection and then escape " and \. It will also
> escape line breaks in values. Together this should produce
> an easily parsable output.

That will lead to either eval (which runs perl parser and probably won't win
anything) or regexps (which is not big win over parsing the .ini directly
with them) on the perl side. IMHO only thing that would actually be faster is
NUL-separated entries.

Either:
    KEY <NUL> VALUE <NUL>

or:
    KEY <TAB> VALUE <NUL>

I am not sure whether there can be multi-valued entries. If so, than there
are three options:

1. Simply repeated key/value pairs:
   KEY <NUL> VALUE1 <NUL> KEY <NUL> VALUE2 <NUL>.
   KEY <TAB> VALUE1 <NUL> KEY <TAB> VALUE2 <NUL> resp.

2. Key/count/values:
   KEY <NUL> 1 <NUL> VALUE <NUL>
   KEY <NUL> 2 <NUL> VALUE1 <NUL> VALUE2 <NUL>
   (there's probably no benefit for the tab-nul format, because the first
   value must be terminated with NUL)

3. Empty-entry terminated:
   KEY <NUL> VALUE <NUL> <NUL>
   KEY <NUL> VALUE1 <NUL> VALUE2 <NUL> <NUL>
   (again no point in terminating the KEY with tab)

The advantage of such format is, that it can be parsed with:
    local $/ = "\0";
    while(<INPUT>) {
	$hash{$_} = <INPUT>;
    }
and slight variations for the other variants. It should be similarly easy
from python and C. Shell won't like it, though.

Note: In both bash and zsh, read -d '' line reads NUL-terminated "lines".
However, dash does not have -d option to read :-(.

-- 
						 Jan 'Bulb' Hudec <bulb@ucw.cz>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Frank Lichtenheld]

On Mon, May 21, 2007 at 09:54:23PM +0200, Jan Hudec wrote:
> On Mon, May 21, 2007 at 19:46:59 +0200, Frank Lichtenheld wrote:
> > This option will enclose key names in quotes (") if they
> > contain a subsection and then escape " and \. It will also
> > escape line breaks in values. Together this should produce
> > an easily parsable output.
> 
> That will lead to either eval (which runs perl parser and probably won't win
> anything) or regexps (which is not big win over parsing the .ini directly
> with them) on the perl side. IMHO only thing that would actually be faster is
> NUL-separated entries.

> Either:
>     KEY <NUL> VALUE <NUL>
> 
> or:
>     KEY <TAB> VALUE <NUL>

Both subsection names and values can contain <TAB> characters, so the
latter isn't possible.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH] config: Add --quoted option to produce machine-parsable output

[Jakub Narebski]

Frank Lichtenheld wrote:

> On Mon, May 21, 2007 at 09:54:23PM +0200, Jan Hudec wrote:
>> On Mon, May 21, 2007 at 19:46:59 +0200, Frank Lichtenheld wrote:
>> > This option will enclose key names in quotes (") if they
>> > contain a subsection and then escape " and \. It will also
>> > escape line breaks in values. Together this should produce
>> > an easily parsable output.
>> 
>> That will lead to either eval (which runs perl parser and probably won't win
>> anything) or regexps (which is not big win over parsing the .ini directly
>> with them) on the perl side. IMHO only thing that would actually be faster is
>> NUL-separated entries.
> 
>> Either:
>>     KEY <NUL> VALUE <NUL>
>> 
>> or:
>>     KEY <TAB> VALUE <NUL>
> 
> Both subsection names and values can contain <TAB> characters, so the
> latter isn't possible.

But neither subsection names (even [section "subsection"] style) not key
names cannot contain newline <LF>. I.e.

        KEY <LF> VALUE <NUL>

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

[PATCH/RFC] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

Use \n as delimiter between key and value and \0 as
delimiter after each key/value pair. This should be
easily parsable output.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 builtin-config.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

 Note the FIXME. Does anyone remember the reason why --get-regexp
 and --list use different output format?

 On Tue, May 22, 2007 at 12:37:57AM +0200, Jakub Narebski wrote:
 > Frank Lichtenheld wrote:
 > > On Mon, May 21, 2007 at 09:54:23PM +0200, Jan Hudec wrote:
 > >>     KEY <TAB> VALUE <NUL>
 > > Both subsection names and values can contain <TAB> characters, so the
 > > latter isn't possible.
 > But neither subsection names (even [section "subsection"] style) not key
 > names cannot contain newline <LF>. I.e.
 >         KEY <LF> VALUE <NUL>

diff --git a/builtin-config.c b/builtin-config.c
index b2515f7..bed2722 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -2,7 +2,7 @@
 #include "cache.h"
 
 static const char git_config_set_usage[] =
-"git-config [ --global | --system ] [ --bool | --int ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list";
+"git-config [ --global | --system ] [ --bool | --int ] [ -z | --null ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list";
 
 static char *key;
 static regex_t *key_regexp;
@@ -12,14 +12,16 @@ static int use_key_regexp;
 static int do_all;
 static int do_not_match;
 static int seen;
+static char delim = '=';
+static char term = '\n';
 static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;
 
 static int show_all_config(const char *key_, const char *value_)
 {
 	if (value_)
-		printf("%s=%s\n", key_, value_);
+		printf("%s%c%s%c", key_, delim, value_, term);
 	else
-		printf("%s\n", key_);
+		printf("%s%c", key_, term);
 	return 0;
 }
 
@@ -39,6 +41,7 @@ static int show_config(const char* key_, const char* value_)
 		return 0;
 
 	if (show_keys)
+		/* FIXME: not useful with --null */
 		printf("%s ", key_);
 	if (seen && !do_all)
 		dup_error = 1;
@@ -54,7 +57,7 @@ static int show_config(const char* key_, const char* value_)
 				key_, vptr);
 	}
 	else
-		printf("%s\n", vptr);
+		printf("%s%c", vptr, term);
 
 	return 0;
 }
@@ -155,6 +158,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp(argv[1], "--system"))
 			setenv("GIT_CONFIG", ETC_GITCONFIG, 1);
+		else if (!strcmp(argv[1], "--null") || !strcmp(argv[1], "-z")) {
+			term = '\0';
+			delim = '\n';
+		}
 		else if (!strcmp(argv[1], "--rename-section")) {
 			int ret;
 			if (argc != 4)
-- 
1.5.2.1

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Johannes Schindelin]

Hi,

On Mon, 18 Jun 2007, Frank Lichtenheld wrote:

>  Note the FIXME. Does anyone remember the reason why --get-regexp
>  and --list use different output format?

AFAIK --list was meant as a replacement to git-var --list. Thus, it had to 
behave exactly the same.

As for the FIXME: If you have a config like this:

	[core]
		Some = where
		over
		the = core.rainbow

git-config -z would output something like this:

	core.some\0where\0core.over\0core.the\0core.rainbow\0

Right?

As you can see, it is quite hard for a parser to find out what is key, and 
what is value. That FIXME is _exactly_ about this dilemma.

IIRC I stated once that -z should output a value of "true" for these 
cases, since they only make sense as booleans. But AFAIR nothing 
conclusive came out of that thread.

Ciao,
Dscho

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Jakub Narebski]

Johannes Schindelin wrote:

> As for the FIXME: [...]

Please read original message carefully:

>>  Note the FIXME. Does anyone remember the reason why --get-regexp
>>  and --list use different output format?

I think the FIXME is for --get-regexp. And was added by the patch.

> If you have a config like this: 
> 
>         [core]
>                 Some = where
>                 over
>                 the = core.rainbow
> 
> git-config -z would output something like this:
> 
>         core.some\0where\0core.over\0core.the\0core.rainbow\0
> 
> Right?

False. Delim is different from term. You would get

	core.some\nwhere\0core.over\0core.the\ncore.rainbow\0

> As you can see, it is quite hard for a parser to find out what is
> key, and what is value.

	local $/ = "\0";

	while (my $line = <$fd>) {
		chomp $line;
		my ($key, $value) = split(/\n/, $line, 2);

		push @{$config{$key}}, $value if defined $key;
	}

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

On Tue, Jun 19, 2007 at 01:55:24AM +0100, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 18 Jun 2007, Frank Lichtenheld wrote:
> 
> >  Note the FIXME. Does anyone remember the reason why --get-regexp
> >  and --list use different output format?
> 
> AFAIK --list was meant as a replacement to git-var --list. Thus, it had to 
> behave exactly the same.
> 
> As for the FIXME: If you have a config like this:
> 
> 	[core]
> 		Some = where
> 		over
> 		the = core.rainbow
> 
> git-config -z would output something like this:
> 
> 	core.some\0where\0core.over\0core.the\0core.rainbow\0
> 
> Right?

No. At least not with my patch. As you noted that would be
incredibly stupid and worthless. Instead we output something like

core.some\nwhere\0core.over\0core.the\ncore.rainbow\0

So you just can split on \0 and then split on \n. If there is
no \n between two \0, you have a key without value.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Johannes Schindelin]

Hi,

On Tue, 19 Jun 2007, Frank Lichtenheld wrote:

> Instead we output something like
> 
> core.some\nwhere\0core.over\0core.the\ncore.rainbow\0

Ah, I missed that. Maybe because I expected "--null" not to be 
"--newline-and-null" ;-)

Ciao,
Dscho

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> As for the FIXME: If you have a config like this:
>
> 	[core]
> 		Some = where
> 		over
> 		the = core.rainbow
>
> git-config -z would output something like this:
>
> 	core.some\0where\0core.over\0core.the\0core.rainbow\0
>
> Right?
>
> As you can see, it is quite hard for a parser to find out what is key, and 
> what is value. That FIXME is _exactly_ about this dilemma.
>
> IIRC I stated once that -z should output a value of "true" for these 
> cases, since they only make sense as booleans. But AFAIR nothing 
> conclusive came out of that thread.

I do not remember the thread, but I think that may make sense.
"over = 1", "over = true" etc. cannot be canonicalized to "true"
without knowing core.over is boolean, but core.over by itself
without any assignment cannot be anything but a boolean.

Another possibility, though, is to say:

	core.some\0where\0core.over\0\0core.the\0core.rainbow\0

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

On Mon, Jun 18, 2007 at 06:37:35PM -0700, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Another possibility, though, is to say:
> 
> 	core.some\0where\0core.over\0\0core.the\0core.rainbow\0

How do you denote empty values then?

[section]
	key=
	key

this are two very different statements atm (e.g. the one is false and
the other one is true).

I still think using two different delimiters is the simplest choice.

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Johannes Schindelin]

Hi,

On Tue, 19 Jun 2007, Frank Lichtenheld wrote:

> On Mon, Jun 18, 2007 at 06:37:35PM -0700, Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > Another possibility, though, is to say:
> > 
> > 	core.some\0where\0core.over\0\0core.the\0core.rainbow\0
> 
> How do you denote empty values then?
> 
> [section]
> 	key=
> 	key
> 
> this are two very different statements atm (e.g. the one is false and
> the other one is true).
> 
> I still think using two different delimiters is the simplest choice.

Okay, good point. But of course, you have to use a delimiter for the key 
name that cannot be part of the keyname. You picked '\n'. The original was 
'='. Both work.

Ciao,
Dscho

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Tue, 19 Jun 2007, Frank Lichtenheld wrote:
>
>> On Mon, Jun 18, 2007 at 06:37:35PM -0700, Junio C Hamano wrote:
>> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > Another possibility, though, is to say:
>> > 
>> > 	core.some\0where\0core.over\0\0core.the\0core.rainbow\0
>> 
>> How do you denote empty values then?
>> 
>> [section]
>> 	key=
>> 	key
>> 
>> this are two very different statements atm (e.g. the one is false and
>> the other one is true).
>> 
>> I still think using two different delimiters is the simplest choice.
>
> Okay, good point. But of course, you have to use a delimiter for the key 
> name that cannot be part of the keyname. You picked '\n'. The original was 
> '='. Both work.

In the interest of simplicity, it would appear reasonable to use just
= and not introduce an additional delimiter.  This is similar to how
environments are handled and passed in Unix (though not necessarily
relevant).

-- 
David Kastrup

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Jakub Narebski]

Johannes Schindelin wrote:
> On Tue, 19 Jun 2007, Frank Lichtenheld wrote:
>> On Mon, Jun 18, 2007 at 06:37:35PM -0700, Junio C Hamano wrote:

>>> Another possibility, though, is to say:
>>> 
>>> 	core.some\0where\0core.over\0\0core.the\0core.rainbow\0
>> 
>> How do you denote empty values then?
>> 
>> [section]
>> 	key=
>> 	key
>> 
>> this are two very different statements atm (e.g. the one is false and
>> the other one is true).
>> 
>> I still think using two different delimiters is the simplest choice.
> 
> Okay, good point. But of course, you have to use a delimiter for the key 
> name that cannot be part of the keyname. You picked '\n'. The original was 
> '='. Both work.

If I remember correctly (and what I checked to be true), while '=' cannot
be part of keyname nor section name, it can be part of subsection name,
therefore it can be part of fuly qualified key name.

The '\n' can _not_ be part of subsection name, therefore it can not be
part of fully qualified key name.

  $ cat > conftest <<EOF
  [section "sub=section"]
       truekey=true
       emptykey=
       novalkey
  EOF

  $ GIT_CONFIG=conftest git-config -l
  section.sub=section.truekey=true
  section.sub=section.emptykey=
  section.sub=section.novalkey

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

On Tue, Jun 19, 2007 at 12:09:18PM +0100, Johannes Schindelin wrote:
> Okay, good point. But of course, you have to use a delimiter for the key 
> name that cannot be part of the keyname. You picked '\n'. The original was 
> '='. Both work.

No, they actually don't.

Example:

$ cat .git/config
[foo "bar=baz"]
	key = value
[foo]
	key = key=value
$ git config -l
foo.bar=baz.key=value
foo.key=key=value

So how do I parse that?

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Johannes Schindelin]

Hi,

On Tue, 19 Jun 2007, Frank Lichtenheld wrote:

> On Tue, Jun 19, 2007 at 12:09:18PM +0100, Johannes Schindelin wrote:
> > Okay, good point. But of course, you have to use a delimiter for the key 
> > name that cannot be part of the keyname. You picked '\n'. The original was 
> > '='. Both work.
> 
> No, they actually don't.

Right, I completely forgot that we actually allow all kinds of special 
characters, in order to be able to say "branch.<branchname>.merge" for all 
kinds of branchnames.

Incidentally, I think I found a bug:

[foo "bar\nbaz"]
	key = value

gives

foo.barbaz.key=value

Ciao,
Dscho

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

On Tue, Jun 19, 2007 at 04:57:21PM +0100, Johannes Schindelin wrote:
> [foo "bar\nbaz"]
> 	key = value
> gives
> 
> foo.barbaz.key=value

for me this gives

foo.barnbaz.key=value

which is the intended behaviour AFAICT
(you mean the actual string '\' 'n' here, right? '\n' gives a syntax
error, also the intended behaviour)

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Johannes Schindelin]

Hi,

On Tue, 19 Jun 2007, Frank Lichtenheld wrote:

> On Tue, Jun 19, 2007 at 04:57:21PM +0100, Johannes Schindelin wrote:
> > [foo "bar\nbaz"]
> > 	key = value
> > gives
> > 
> > foo.barbaz.key=value
> 
> for me this gives
> 
> foo.barnbaz.key=value

Yes, of course I had to have a typo in my message. *sigh*

The point is, that I would not expect a "\" to be _ignored_. Either 
interpreted, or throwing an error, but just ignored?

Ciao,
Dscho

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Jakub Narebski]

Johannes Schindelin wrote:
> On Tue, 19 Jun 2007, Frank Lichtenheld wrote:
> > On Tue, Jun 19, 2007 at 04:57:21PM +0100, Johannes Schindelin wrote:

> > > [foo "bar\nbaz"]
> > > 	key = value
> > > gives
> > > 
> > > foo.barbaz.key=value
> > 
> > for me this gives
> > 
> > foo.barnbaz.key=value
> 
> Yes, of course I had to have a typo in my message. *sigh*
> 
> The point is, that I would not expect a "\" to be _ignored_. Either 
> interpreted, or throwing an error, but just ignored?

It is interpreted. Perhaps not what you thought it is interpreted, but 
it is interpreted. '\x' => 'x' for 'x' which are not in some limited 
set. ;-)))

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Jakub Narebski]

On Mon, 18 Jun 2007, Frank Lichtenheld wrote:
> Use \n as delimiter between key and value and \0 as
> delimiter after each key/value pair. This should be
> easily parsable output.
> 
> Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
> ---
>  builtin-config.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)

No documentation. But this is an RFC.
 
>  Note the FIXME. Does anyone remember the reason why --get-regexp
>  and --list use different output format?

I don't know, but at least two scripts use --get-regexp, namely
git-remote and git-submodule. So we would have to be careful about
changing that.
 

I would be enough to add the following to your patch:

> @@ -12,14 +12,16 @@ static int use_key_regexp;
>  static int do_all;
>  static int do_not_match;
>  static int seen;
> +static char delim = '=';
> +static char term = '\n';
  +static char key_delim = ' ';
>  static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;
[...]
> @@ -39,6 +41,7 @@ static int show_config(const char* key_, const char* value_)
>  		return 0;
>  
>  	if (show_keys)
> +		/* FIXME: not useful with --null */
  - 		printf("%s ", key_);
  + 		printf("%s%c", key_, key_delim);
>  	if (seen && !do_all)
>  		dup_error = 1;
[...]
> @@ -155,6 +158,10 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		}
>  		else if (!strcmp(argv[1], "--system"))
>  			setenv("GIT_CONFIG", ETC_GITCONFIG, 1);
> +		else if (!strcmp(argv[1], "--null") || !strcmp(argv[1], "-z")) {
> +			term = '\0';
> +			delim = '\n';
  +			key_delim = '\n';
> +		}
>  		else if (!strcmp(argv[1], "--rename-section")) {
>  			int ret;
>  			if (argc != 4)


By the way, I have tried to use git-config --null to redo config
file parsing in gitweb, so one git-config call would be needed for
all the config. I have noticed that --bool option description does
not describe the observed behavior fully. For example it returns
'true' not only for '1', but for any integer != 0, including 0xdeadbeef.

By the way, the error message when key value _cannot_ be converted to
the boolean is somewhat misleading:

  $ GIT_CONFIG=conftest git config --bool bool.key7
  fatal: bad config value for 'bool.key7' in conftest

-- 
Jakub Narebski
Poland

Re: [PATCH/RFC] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

On Fri, Jun 22, 2007 at 01:56:00AM +0200, Jakub Narebski wrote:
> On Mon, 18 Jun 2007, Frank Lichtenheld wrote:
> >  Note the FIXME. Does anyone remember the reason why --get-regexp
> >  and --list use different output format?
> 
> I don't know, but at least two scripts use --get-regexp, namely
> git-remote and git-submodule. So we would have to be careful about
> changing that.
>  
> 
> I would be enough to add the following to your patch:

Yeah, I found this a but ugly though so I left it out of the first
patch in case someone had a better idea. Will include that in the
second version (which will also include documentation).

> By the way, I have tried to use git-config --null to redo config
> file parsing in gitweb, so one git-config call would be needed for
> all the config. I have noticed that --bool option description does
> not describe the observed behavior fully. For example it returns
> 'true' not only for '1', but for any integer != 0, including 0xdeadbeef.

Yeah, the description of --bool is very incomplete. Note that empty
values are false, and keys without values are true. I think this all
are valid choices, but they are indeed not documented.

Gruesse.
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

[PATCH 1/3] config: Complete documentation of --get-regexp

[Frank Lichtenheld]

The asciidoc documentation of the --get-regexp option was
incomplete. Add some missing pieces:
 - List the option in SYNOPSIS
 - Mention that key names are printed

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 Documentation/git-config.txt |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index f2c6717..bb6dbb0 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -14,6 +14,7 @@ SYNOPSIS
 'git-config' [--system | --global] --replace-all name [value [value_regex]]
 'git-config' [--system | --global] [type] --get name [value_regex]
 'git-config' [--system | --global] [type] --get-all name [value_regex]
+'git-config' [--system | --global] [type] --get-regexp name_regex [value_regex]
 'git-config' [--system | --global] --unset name [value_regex]
 'git-config' [--system | --global] --unset-all name [value_regex]
 'git-config' [--system | --global] --rename-section old_name new_name
@@ -73,6 +74,7 @@ OPTIONS
 
 --get-regexp::
 	Like --get-all, but interprets the name as a regular expression.
+	Also outputs the key names.
 
 --global::
 	For writing options: write to global ~/.gitconfig file rather than
-- 
1.5.2.1

[PATCH 2/3] config: Change output of --get-regexp for valueless keys

[Frank Lichtenheld]

Print no space after the name of a key without value.
Otherwise keys without values are printed exactly the
same as keys with empty values.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 builtin-config.c       |    8 ++++++--
 t/t1300-repo-config.sh |    6 ++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

 I hope that nobody depends on this specific behaviour.
 Backwards compatibilty would be a pain here, since the --null
 patch would get really complicated

diff --git a/builtin-config.c b/builtin-config.c
index b2515f7..dbc2339 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -38,8 +38,12 @@ static int show_config(const char* key_, const char* value_)
 			  regexec(regexp, (value_?value_:""), 0, NULL, 0)))
 		return 0;
 
-	if (show_keys)
-		printf("%s ", key_);
+	if (show_keys) {
+		if (value_)
+			printf("%s ", key_);
+		else
+			printf("%s", key_);
+	}
 	if (seen && !do_all)
 		dup_error = 1;
 	if (type == T_INT)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 7731fa7..8b5e9fc 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -283,6 +283,12 @@ EOF
 test_expect_success 'get variable with no value' \
 	'git-config --get novalue.variable ^$'
 
+echo novalue.variable > expect
+
+test_expect_success 'get-regexp variable with no value' \
+	'git-config --get-regexp novalue > output &&
+	 cmp output expect' 
+
 git-config > output 2>&1
 
 test_expect_success 'no arguments, but no crash' \
-- 
1.5.2.1

Re: [PATCH 2/3] config: Change output of --get-regexp for valueless keys

[Junio C Hamano]

Frank Lichtenheld <frank@lichtenheld.de> writes:

> Print no space after the name of a key without value.
> Otherwise keys without values are printed exactly the
> same as keys with empty values.

I think this can be defended as a bugfix even though it is a
change in the behaviour.  Thanks.

[PATCH 3/3] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

Use \n as delimiter between key and value and \0 as
delimiter after each key/value pair. This should be
easily parsable output.

Signed-off-by: Frank Lichtenheld <frank@lichtenheld.de>
---
 Documentation/git-config.txt |   18 +++++++++++++-----
 builtin-config.c             |   20 ++++++++++++++------
 t/t1300-repo-config.sh       |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 11 deletions(-)

 This time with documentation and test cases.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index bb6dbb0..8c09b88 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,17 +9,17 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git-config' [--system | --global] name [value [value_regex]]
+'git-config' [--system | --global] [-z|--null] name [value [value_regex]]
 'git-config' [--system | --global] --add name value
 'git-config' [--system | --global] --replace-all name [value [value_regex]]
-'git-config' [--system | --global] [type] --get name [value_regex]
-'git-config' [--system | --global] [type] --get-all name [value_regex]
-'git-config' [--system | --global] [type] --get-regexp name_regex [value_regex]
+'git-config' [--system | --global] [type] [-z|--null] --get name [value_regex]
+'git-config' [--system | --global] [type] [-z|--null] --get-all name [value_regex]
+'git-config' [--system | --global] [type] [-z|--null] --get-regexp name_regex [value_regex]
 'git-config' [--system | --global] --unset name [value_regex]
 'git-config' [--system | --global] --unset-all name [value_regex]
 'git-config' [--system | --global] --rename-section old_name new_name
 'git-config' [--system | --global] --remove-section name
-'git-config' [--system | --global] -l | --list
+'git-config' [--system | --global] [-z|--null] -l | --list
 
 DESCRIPTION
 -----------
@@ -118,6 +118,14 @@ See also <<FILES>>.
 	in the config file will cause the value to be multiplied
 	by 1024, 1048576, or 1073741824 prior to output.
 
+-z, --null::
+	For all options that output values and/or keys, always
+	end values with with the null character (instead of a
+	newline). Use newline instead as a delimiter between
+	key and value. This allows for secure parsing of the
+	output without getting confused e.g. by values that
+	contain line breaks. 
+
 
 [[FILES]]
 FILES
diff --git a/builtin-config.c b/builtin-config.c
index dbc2339..9d52ba8 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -2,7 +2,7 @@
 #include "cache.h"
 
 static const char git_config_set_usage[] =
-"git-config [ --global | --system ] [ --bool | --int ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list";
+"git-config [ --global | --system ] [ --bool | --int ] [ -z | --null ] [--get | --get-all | --get-regexp | --replace-all | --add | --unset | --unset-all] name [value [value_regex]] | --rename-section old_name new_name | --remove-section name | --list";
 
 static char *key;
 static regex_t *key_regexp;
@@ -12,14 +12,17 @@ static int use_key_regexp;
 static int do_all;
 static int do_not_match;
 static int seen;
+static char delim = '=';
+static char key_delim = ' ';
+static char term = '\n';
 static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;
 
 static int show_all_config(const char *key_, const char *value_)
 {
 	if (value_)
-		printf("%s=%s\n", key_, value_);
+		printf("%s%c%s%c", key_, delim, value_, term);
 	else
-		printf("%s\n", key_);
+		printf("%s%c", key_, term);
 	return 0;
 }
 
@@ -40,9 +43,9 @@ static int show_config(const char* key_, const char* value_)
 
 	if (show_keys) {
 		if (value_)
-			printf("%s ", key_);
+			printf("%s%c", key_, key_delim);
 		else
-			printf("%s", key_);
+			printf("%s",key_);
 	}
 	if (seen && !do_all)
 		dup_error = 1;
@@ -58,7 +61,7 @@ static int show_config(const char* key_, const char* value_)
 				key_, vptr);
 	}
 	else
-		printf("%s\n", vptr);
+		printf("%s%c", vptr, term);
 
 	return 0;
 }
@@ -159,6 +162,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		}
 		else if (!strcmp(argv[1], "--system"))
 			setenv("GIT_CONFIG", ETC_GITCONFIG, 1);
+		else if (!strcmp(argv[1], "--null") || !strcmp(argv[1], "-z")) {
+			term = '\0';
+			delim = '\n';
+			key_delim = '\n';
+		}
 		else if (!strcmp(argv[1], "--rename-section")) {
 			int ret;
 			if (argc != 4)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8b5e9fc..99d84cc 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -519,4 +519,36 @@ git config --list > result
 
 test_expect_success 'value continued on next line' 'cmp result expect'
 
+cat > .git/config <<\EOF
+[section "sub=section"]
+	val1 = foo=bar
+	val2 = foo\nbar
+	val3 = \n\n
+	val4 =
+	val5
+EOF
+
+cat > expect <<\EOF
+Key: section.sub=section.val1
+Value: foo=bar
+Key: section.sub=section.val2
+Value: foo
+bar
+Key: section.sub=section.val3
+Value: 
+
+
+Key: section.sub=section.val4
+Value: 
+Key: section.sub=section.val5
+EOF
+
+git config --null --list | perl -0ne 'chop;($key,$value)=split(/\n/,$_,2);print "Key: $key\n";print "Value: $value\n" if defined($value)' > result
+
+test_expect_success '--null --list' 'cmp result expect'
+
+git config --null --get-regexp 'val[0-9]' | perl -0ne 'chop;($key,$value)=split(/\n/,$_,2);print "Key: $key\n";print "Value: $value\n" if defined($value)' > result
+
+test_expect_success '--null --get-regexp' 'cmp result expect'
+
 test_done
-- 
1.5.2.1

Re: [PATCH 3/3] config: Add --null/-z option for null-delimted output

[Jakub Narebski]

Frank Lichtenheld wrote:

>                 else
> -                       printf("%s", key_);
> +                       printf("%s",key_);
>         }

That is a mistake, I think?

-- 
Jakub Narebski
Poland

Re: [PATCH 3/3] config: Add --null/-z option for null-delimted output

[Frank Lichtenheld]

On Tue, Jun 26, 2007 at 01:29:46AM +0200, Jakub Narebski wrote:
> Frank Lichtenheld wrote:
> >                 else
> > -                       printf("%s", key_);
> > +                       printf("%s",key_);
> >         }
> 
> That is a mistake, I think?

Indeed. I remember I saw and fixed that before sending. Must've
been a halluzination ;)

Gruesse,
-- 
Frank Lichtenheld <frank@lichtenheld.de>
www: http://www.djpig.de/

Re: [PATCH 3/3] config: Add --null/-z option for null-delimted output

[Junio C Hamano]

Thanks for the sharp eyes.