From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff King Subject: Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config Date: Fri, 29 Mar 2013 13:00:32 -0400 Message-ID: <20130329170032.GA3552@sigill.intra.peff.net> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: git@vger.kernel.org To: Phil Haack X-From: git-owner@vger.kernel.org Fri Mar 29 18:01:11 2013 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1ULcfs-0001Qc-OD for gcvg-git-2@plane.gmane.org; Fri, 29 Mar 2013 18:01:09 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755939Ab3C2RAg (ORCPT ); Fri, 29 Mar 2013 13:00:36 -0400 Received: from 75-15-5-89.uvs.iplsin.sbcglobal.net ([75.15.5.89]:48430 "EHLO peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755676Ab3C2RAe (ORCPT ); Fri, 29 Mar 2013 13:00:34 -0400 Received: (qmail 7091 invoked by uid 107); 29 Mar 2013 17:02:21 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) (smtp-auth username relayok, mechanism cram-md5) by peff.net (qpsmtpd/0.84) with ESMTPA; Fri, 29 Mar 2013 13:02:21 -0400 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Fri, 29 Mar 2013 13:00:32 -0400 Content-Disposition: inline In-Reply-To: Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: 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