Minor bug: git config ignores empty sections

Minor bug: git config ignores empty sections

[Eli Barzilay]

Looks like there's a problem with setting a config with an empty
section, making it create a new section.  The result is:

    $ git --version
    git version 2.9.2
    $ git init
    Initialized empty Git repository in /home/eli/t/.git/
    $ t() { git config x.y x; git config --unset x.y; }
    $ t;t;t
    $ grep -c '\[x\]' .git/config
    3
    $ git config x.z x
    $ t;t;t
    $ git config x.z x     # adds another [x], but leaves it populated
    $ t;t;t;t;t;t;t;t
    $ grep -c '\[x\]' .git/config
    4

-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Sun, Aug 14, 2016 at 09:29:27PM -0400, Eli Barzilay wrote:

> Looks like there's a problem with setting a config with an empty
> section, making it create a new section.  The result is:
> 
>     $ git --version
>     git version 2.9.2
>     $ git init
>     Initialized empty Git repository in /home/eli/t/.git/
>     $ t() { git config x.y x; git config --unset x.y; }
>     $ t;t;t
>     $ grep -c '\[x\]' .git/config
>     3
>     $ git config x.z x
>     $ t;t;t
>     $ git config x.z x     # adds another [x], but leaves it populated
>     $ t;t;t;t;t;t;t;t
>     $ grep -c '\[x\]' .git/config
>     4

Yep. This is a very old and well-known bug. IIRC, the problem is that
config updates use the regular callback-parser, so they see only the
config keys themselves. Empty section headers never trigger them.
Fixing it would require rewriting the config-update code.

And implicit in your test is the other bug, which is that deleting the
last key in a section leaves the empty header. I think it's related to
the same issue.

Patches welcome, of course.

-Peff

Re: Minor bug: git config ignores empty sections

[Andreas Schwab]

On Aug 15 2016, Jeff King <peff@peff.net> wrote:

> And implicit in your test is the other bug, which is that deleting the
> last key in a section leaves the empty header. I think it's related to
> the same issue.

Indiscriminately removing empty section headers may break comments that
have been put there on purpose.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Minor bug: git config ignores empty sections

[Junio C Hamano]

Eli Barzilay <eli@barzilay.org> writes:

> Looks like there's a problem with setting a config with an empty
> section, making it create a new section.  The result is:

Thanks; this unfortunately is a well-known issue, that once was even
(supposed to be) a part of GSoC project but hasn't been solved.

The latest mention of the same issue I think is

https://public-inbox.org/git/xmqqeg95aor6.fsf@gitster.mtv.corp.google.com/

which redirects to

    If you are interested, start here:

    http://thread.gmane.org/gmane.comp.version-control.git/219505/focus=219524

but gmane web interface is not working and an equivalent is here:

https://public-inbox.org/git/20130329195155.GA19994@sigill.intra.peff.net/

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:

> On Aug 15 2016, Jeff King <peff@peff.net> wrote:
> 
> > And implicit in your test is the other bug, which is that deleting the
> > last key in a section leaves the empty header. I think it's related to
> > the same issue.
> 
> Indiscriminately removing empty section headers may break comments that
> have been put there on purpose.

I know, but we do not even do so discriminately.

-Peff

Re: Minor bug: git config ignores empty sections

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:
>
>> On Aug 15 2016, Jeff King <peff@peff.net> wrote:
>> 
>> > And implicit in your test is the other bug, which is that deleting the
>> > last key in a section leaves the empty header. I think it's related to
>> > the same issue.
>> 
>> Indiscriminately removing empty section headers may break comments that
>> have been put there on purpose.
>
> I know, but we do not even do so discriminately.

I notice that we have thought about all the issues when we last
discussed it in 2013.  Refining a message from the earlier thread,
as it illustrates tricky cases in which we have to be careful.

If we were to remove the section header at this point, we should
be removing the comment two out of three cases.

 - if it is about section.key, it should go when section.key goes.
 - if it is about section, it should go when section goes.
 - if it is a more generic comment about this configuration file,
   it should stay.

A configuration file may have something like this:

        # Now, let's list all the remotes I interact with.

        # This is where I push all the separate topics.
        [remote "github"]
                # fetch like everybody else without auth
                url = git://github.com/user/git
                # push with auth
                pushURL = github.com:user/git
                # publish my working area as-is
                mirror

        # Traditional "canonical" one
        [remote "korg"]
                url = k.org:/pub/scm/user/git

If I were to retire the "github" account, removing the entire
[remote "github"] section while keeping the comments before that
section would be confusing, which would end up with:

        # Now, let's list all the remotes I interact with.

        # This is where I push all the separate topics.

        # Traditional "canonical" one
        [remote "korg"]
                url = k.org:/pub/scm/user/git

because I do not push all the separate topics to korg.

Removing the section while removing the comments that pertain to the
section is impossible; "Now, let's list all the remotes" should
stay, "This is where I push" should go.

So a comment outside [section "name"] is tricky; it needs some
mechanism (or convention) to tell us if it is about the particular
section, or it is about the location in the configuration file.

Removing only the keys and keeping the skelton around would give us:

        # Now, let's list all the remotes I interact with.
        # This is where I push all the separate topics.
        [remote "github"]
                # fetch like everybody else without auth
                # push with auth
                # publish my working area as-is

        # Traditional "canonical" one
        [remote "korg"]
                url = k.org:/pub/scm/user/git

which is still confusing, but at least the confusion is not spread
to adjacent sections.  What we could do easily without understanding
natural languages is to remove comments inside [section "name"], but
it still depends on a convention that such a per-key comment comes
before the key, not after, i.e.

        # This is where I push all the separate topics.
        [remote "github"]
                # fetch like everybody else without auth
                ...
                # publish my working area as-is
                mirror
		# note that mirror may push out outside tags/ and heads/

	# Traditional "canonical" one
	[remote "korg"]

We could also take hints from the indentation level, but now we are
deep into the "convention" territory once we start doing that.

Re: Minor bug: git config ignores empty sections

[Andreas Schwab]

On Aug 15 2016, Junio C Hamano <gitster@pobox.com> wrote:

> If we were to remove the section header at this point, we should
> be removing the comment two out of three cases.
>
>  - if it is about section.key, it should go when section.key goes.
>  - if it is about section, it should go when section goes.
>  - if it is a more generic comment about this configuration file,
>    it should stay.

The comment may not be related to the surrounding keys, but just a
commented-out config key that you want to keep around for reference.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Mon, Aug 15, 2016 at 11:28:20AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:
> >
> >> On Aug 15 2016, Jeff King <peff@peff.net> wrote:
> >> 
> >> > And implicit in your test is the other bug, which is that deleting the
> >> > last key in a section leaves the empty header. I think it's related to
> >> > the same issue.
> >> 
> >> Indiscriminately removing empty section headers may break comments that
> >> have been put there on purpose.
> >
> > I know, but we do not even do so discriminately.
> 
> I notice that we have thought about all the issues when we last
> discussed it in 2013.  Refining a message from the earlier thread,
> as it illustrates tricky cases in which we have to be careful.

Thanks for digging up the threads that I was too lazy to find.

I agree with most everything here, though I would be happy if somebody
even wrote a patch to handle the "easy" cases.

> So a comment outside [section "name"] is tricky; it needs some
> mechanism (or convention) to tell us if it is about the particular
> section, or it is about the location in the configuration file.

Keep in mind that even "outside" is hard, because sections do not
explicitly close.

So in:

  [core]
  foo = bar

  # here are my remotes

  [remote "github"]
  url = ...

How do we know that the comment is "outside" and not part of [core]?

We can perhaps guess so because there are no keys after it in the
section, though there are some special cases, like:

  [core]
  foo = bar
  # This isn't necessary anymore because...
  # xyzzy = false

or even:

  [core]
  foo = bar # needed because of xyzzy

You can probably make reasonable cases based on heuristics around
newlines, but that is even further into "convention" territory.

-Peff

Re: Minor bug: git config ignores empty sections

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> So a comment outside [section "name"] is tricky; it needs some
>> mechanism (or convention) to tell us if it is about the particular
>> section, or it is about the location in the configuration file.
>
> Keep in mind that even "outside" is hard, because sections do not
> explicitly close.

Yes, that was what I wanted to say.  We'd be deep in the
"convention" territory.

Re: Minor bug: git config ignores empty sections

[Eli Barzilay]

On Mon, Aug 15, 2016 at 2:55 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Aug 15, 2016 at 11:28:20AM -0700, Junio C Hamano wrote:
>
>> I notice that we have thought about all the issues when we last
>> discussed it in 2013.  Refining a message from the earlier thread,
>> as it illustrates tricky cases in which we have to be careful.
>
> Thanks for digging up the threads that I was too lazy to find.
>
> I agree with most everything here, though I would be happy if somebody
> even wrote a patch to handle the "easy" cases.

So it sounds like removing an empty header is problematic in most cases,
but adding a new variable to an existing empty header should not be...?

I looked at the code, and had a rough sketch that works as follows:

* Make git_parse_source() call the callback in a special way to note
  that a section header is seen (I hacked it by passing a special value,
  a pointer to a global string, as the second argument)

* Add a new store.last_section_offset field

* In store_aux(), if it's getting the special value, and the section
  name matches, save the offset in store.last_section_offset

* In git_config_set_multivar_in_file_gently() right before the "write
  the first part of the config" comment, test that
      (store.seen == 1 && copy_begin == 0 && copy_end == contents_sz
       && store.last_section_offset > 0)
  and if so, write the contents up to that point, and set copy_begin;
  if the condition is false, do the same thing it does now

* A bit after that, add "&& store.last_section_offset == 0" to the
  condition that decides whether to call store_write_section()

It looks to me like something like this can work, but it's very hacky
because I wanted to see if it can work quickly, and because I don't know
if this kind of a solution will be wanted, and because I don't know
enough about that code in general.  Making it be an actual solution
involves a better way to call the callback in a special way (my hack
does the special thing only if `fn==store_aux`, but it shouldn't),
calling it after the last variable in a section is seen so it's added
after that.

So, will something like this be acceptable?  If so, is there anyone who
I can ask questions about the code?

-- 
                   ((x=>x(x))(x=>x(x)))                  Eli Barzilay:
                   http://barzilay.org/                  Maze is Life!

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Tue, Aug 16, 2016 at 12:06:56AM -0400, Eli Barzilay wrote:

> So it sounds like removing an empty header is problematic in most cases,
> but adding a new variable to an existing empty header should not be...?

I think it's less problematic, though there are still some funny cases.
Like:

  [foo]
  # comments for the "foo" section

  # comments introducing the "bar" section
  [bar]

Ideally the new entry goes in between the two comments. But I don't
think it would be unreasonable to add it right after "[foo]". In this
example, using the blank line as a hint might be useful, but that's
going to be a lot harder to coax out of the callback-parser as syntactic
event.

> I looked at the code, and had a rough sketch that works as follows:
> 
> * Make git_parse_source() call the callback in a special way to note
>   that a section header is seen (I hacked it by passing a special value,
>   a pointer to a global string, as the second argument)

We'd want to make sure that didn't kick in for "regular" callers, with
some kind of option (or, blech, a global variable that changes the
behavior of git_parse_source(), though the config code is already full
of them).

> * Add a new store.last_section_offset field
>
> * In store_aux(), if it's getting the special value, and the section
>   name matches, save the offset in store.last_section_offset

Make sense.

> * In git_config_set_multivar_in_file_gently() right before the "write
>   the first part of the config" comment, test that
>       (store.seen == 1 && copy_begin == 0 && copy_end == contents_sz
>        && store.last_section_offset > 0)
>   and if so, write the contents up to that point, and set copy_begin;
>   if the condition is false, do the same thing it does now

This part I'm not sure about. Naively I'd think you could do with less
special-casing here. If we want to add "foo.two" and find:

  [foo]
  one = whatever

then we mark the end of that line as the point we want to copy up to,
and then add our new "two = ..." line.

Conceptually if we see just:

  [foo]

we should be able to kick in the same code. So it seems like the logic
would all be in the detection and setting of the offset.

But I am not sure I understand all of the code here, and it's rather
complicated. So there is probably a good reason that wouldn't work.

> It looks to me like something like this can work, but it's very hacky
> because I wanted to see if it can work quickly, and because I don't know
> if this kind of a solution will be wanted, and because I don't know
> enough about that code in general.  Making it be an actual solution
> involves a better way to call the callback in a special way (my hack
> does the special thing only if `fn==store_aux`, but it shouldn't),
> calling it after the last variable in a section is seen so it's added
> after that.
> 
> So, will something like this be acceptable?  If so, is there anyone who
> I can ask questions about the code?

Yeah, I think this general strategy is the smallest change that could
work.

What I think would be much more sane in general is to parse the whole
thing into a tree (or even a flat list of events), marking each
syntactically as a section header, a key, a comment, a run of
whitespace, and so on. And then do manipulations on that tree (e.g.,
walk down to the section of interest, add a new key at the end), and
then reformat the tree back into a stream of bytes. That lets you
separate the policy logic from the parsing, and do high-level operations
that might involve multiple passes or backtracking in the tree (e.g.,
walk to the section, walk past any existing keys, walk past any comments
but _not_ past any blank lines, then add the new key).

There are other other upsides, too. For example, the current code cannot
write multiple unrelated keys in a single pass.

The downsides is that it's a complete rewrite of the code. So it's a
_lot_ more work, and it carries more risk of regression. So please don't
take this as "no, your solution isn't OK; you have to do the rewrite".
We've been living with band-aids on the config code for a decade, so I
am OK with one more.

-Peff

Re: Minor bug: git config ignores empty sections

[Jakub Narębski]

W dniu 16.08.2016 o 14:36, Jeff King pisze:

> What I think would be much more sane in general is to parse the whole
> thing into a tree (or even a flat list of events),

List of events is cheaper on memory (though I don't think there is
an issue with size of config file), and can be turned into tree
and DOM easily.

>                                                    marking each
> syntactically as a section header, a key, a comment, a run of
> whitespace, and so on. And then do manipulations on that tree (e.g.,
> walk down to the section of interest, add a new key at the end), and
> then reformat the tree back into a stream of bytes. That lets you
> separate the policy logic from the parsing, and do high-level operations
> that might involve multiple passes or backtracking in the tree (e.g.,
> walk to the section, walk past any existing keys, walk past any comments
> but _not_ past any blank lines, then add the new key).
> 
> There are other other upsides, too. For example, the current code cannot
> write multiple unrelated keys in a single pass.
> 
> The downsides is that it's a complete rewrite of the code. So it's a
> _lot_ more work, and it carries more risk of regression. So please don't
> take this as "no, your solution isn't OK; you have to do the rewrite".
> We've been living with band-aids on the config code for a decade, so I
> am OK with one more.

So how 'git config --show-origin --list' works?  

Ah, I guess it prints as it goes, instead of parsing config file(s)
into stream of events, and printing from those.

-- 
Jakub Narębski