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

From: Jeff King <peff@peff.net>
Date: Mar 29, 2013, 17:00 UTC
Message-ID: <20130329170032.GA3552@sigill.intra.peff.net>
In-Reply-To: <CAD7mMPW=jr6PaAc50h-Wpf42-UPrn0A5KmisqXNXqqLv78AEgg@mail.gmail.com>

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

Show 18 quoted lines

> 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