Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config

[Jeff King <peff@peff.net>]

On Fri, Mar 29, 2013 at 09:29:14AM -0700, Phil Haack wrote:

> If you run the following set of commands:
> 
>     git checkout -b some-branch
>     git push origin some-branch
>     git branch --set-upstream-to origin/some-branch
>     git branch --unset-upstream some-branch
>     git branch --set-upstream-to origin/some-branch
> 
> You get the following result in the .git\config file
> 
>     [branch "some-branch"]
>     [branch "some-branch"]
>         remote = origin
>         merge = refs/heads/some-branch
> 
> My expectation is that the very last call to: git branch --set-upstream-to
> would not add a new config section, but would instead insert this information
> into the existing [branch "some-branch"].

Yes, this is a known issue[1] in the config-editing code. There are
actually two separate problems. Try this:

  $ git config -f foo section.one 1
  $ cat foo
  [section]
          one = 1

Looking good so far...

  $ git config -f foo --unset section.one
  $ cat foo
  [section]

Oops, it would have been nice to clean up that now-empty section. Now
let's re-add...

  $ git config -f foo section.two 2
  $ cat foo
  [section]
  [section]
          two = 2

Oops, it would have been nice to use the existing section. What happens
if we add again...

  $ git config -f foo section.three 3
  $ cat foo
  [section]
  [section]
          two = 2
          three = 3

Now that one worked. I think what happens is that the config editor runs
through the files linearly, munging whatever lines necessary for the
requested operation, and leaving everything else untouched (as it must,
to leave comments and whitespace intact). But it does not keep a
look-behind buffer to realize that a section name is now obsolete (which
we don't know until we get to the next section, or to EOF). In the worst
case, this buffer can grow arbitrarily large, like:

  [foo]
  # the above section is now empty
  # but we have to read through all of
  # these comments to actually
  # realize it
  [bar]

In practice it should not be a big deal (and I do not think it is an
interesting attack vector for somebody trying to run you out of memory).

That brings up another point, which is that comments may get misplaced
by deletion. But that's the case for any modification we do to the file,
since we cannot understand the semantics of comments that say things
like "the line below does X".

So that's the first bug. The second one is, I suspect, just laziness. We
notice that section.two is set, and put section.three after it. But we
do not make the same trigger for just "section". That's probably much
easier to fix.

> In any case, from what I understand the current behavior is technically valid
> and I haven't encountered any adverse effects. It was just something I noticed
> while testing.

Yes, the config files generated by these operations parse as you would
expect them to. I think it's purely an aesthetic concern.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/208113