Re: Unresolved issues #2

Linus Torvalds [off-list ref] writes:

Finally, I think "git repo-config" is buggy. Try with this .config file:
...
So we'd really be screwing with porcelain if we made them use this ;)

Thanks Linus and Sean for bringing this up and fixing it.

I have a vague feeling that this may not be the last breakage of
the repo-config command.  My first reaction to the repo-config
code was "eek".  It tries to reuse as much the existing material
as possible -- I understand it was done that way in order to
preserve the comments and blank lines from the original config
file intact, but it just felt very error prone (demonstrated by
cases like this and the other one Sean brought up) and generally
wrong.

It might make sense to rewrite it to parse and read the existing
configuration as a whole, do necessary manupulations on the
parsed internal representation in-core, and write the result out
from scratch.  That would fix another of my pet peeve: after an
invocation of repo-config to remove the last variable in a
section, it leaves an empty section header in.

        $ git repo-config foo.bar true
        $ cat .git/config 
        [core]
                repositoryformatversion = 0
                filemode = true
        [foo]
                bar = true
        $ git repo-config foo1.baz false
        $ git repo-config --unset foo.bar
        $ cat .git/config 
        [core]
                repositoryformatversion = 0
                filemode = true
        [foo]
        [foo1]
                baz = false

Hi,

On Sat, 6 May 2006, Junio C Hamano wrote:

Linus Torvalds [off-list ref] writes:

quoted

Finally, I think "git repo-config" is buggy. Try with this .config file:
...
So we'd really be screwing with porcelain if we made them use this ;)

Thanks Linus and Sean for bringing this up and fixing it.

I have a vague feeling that this may not be the last breakage of
the repo-config command.  My first reaction to the repo-config
code was "eek".  It tries to reuse as much the existing material
as possible -- I understand it was done that way in order to
preserve the comments and blank lines from the original config
file intact, but it just felt very error prone (demonstrated by
cases like this and the other one Sean brought up) and generally
wrong.

It was done because the very syntax of the config suggests it be a 
user-editable file. I do not want to mess with the comments more than 
necessary.

It might make sense to rewrite it to parse and read the existing
configuration as a whole, do necessary manupulations on the
parsed internal representation in-core, and write the result out
from scratch.  That would fix another of my pet peeve: after an
invocation of repo-config to remove the last variable in a
section, it leaves an empty section header in.

Does it really hurt? I think not.

Anyway, I'll look into this.

Ciao,
Dscho

On Sat, 6 May 2006, Johannes Schindelin wrote:

It was done because the very syntax of the config suggests it be a 
user-editable file.

Yeah, I personally much prefer user-friendly config files. Any format that 
thinks that "easy parsing" is more important than "visually obvious" is 
bad. So I obviously think that XML is a horrid piece of cr*p (has anybody 
ever noticed I have strong opinions?) and totally unreadable.

I think "git repo-config" is doing a reasonable job of editing a file that 
is really designed to be user-friendly. That said, the code _is_ a bit 
scary.

It might be worthwhile to re-write config.c to read the config file into 
memory and work on it in-memory instead of doing the funky mixed usage 
(using fgetc/ftell to read it, but then switching over to mmap when 
rewriting it).

IOW, maybe that "static FILE *config_file" should be changed to something 
more like "static const char *config_buffer; unsigned int len;" instead, 
and at least make both the reading and writing use the same buffer rather 
than mixing stdio and mmap..

		Linus

Junio C Hamano wrote:

It might make sense to rewrite it to parse and read the existing
configuration as a whole, do necessary manupulations on the
parsed internal representation in-core, and write the result out
from scratch.

Or perhaps do git repo-config read and change config file in two passes:
read and build some kind of index (beginning of section, end of
section/last variable in section, number of elements in section), then in
second pass add some information if needed.

-- 
Jakub Narebski
Warsaw, Poland