{"thread":{"id":"43848","subject":"Minor bug: git config ignores empty sections","startedAt":"2016-08-15T01:29:35Z","lastAt":"2016-08-21T17:10:19Z","messageCount":12,"participants":["Eli Barzilay","Jeff King","Andreas Schwab","Junio C Hamano","Jakub Narębski"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"299311","messageId":"CALO-gutdz5VMgoRmbqEa9UiaTC+L2Sy2n-3AF+zfPr-X8+1U4A@mail.gmail.com","threadId":"43848","inReplyTo":null,"subject":"Minor bug: git config ignores empty sections","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2016-08-15T01:29:27Z","receivedAt":"2016-08-15T01:29:35Z","isPatch":false,"sender":{"key":"eli@barzilay.org","avatar":"https://avatars.githubusercontent.com/u/185905?v=4"},"body":"Looks like there's a problem with setting a config with an empty\nsection, making it create a new section. The result is:\n\n $ git --version\n git version 2.9.2\n $ git init\n Initialized empty Git repository in /home/eli/t/.git/\n $ t() { git config x.y x; git config --unset x.y; }\n $ t;t;t\n $ grep -c '\\[x\\]' .git/config\n 3\n $ git config x.z x\n $ t;t;t\n $ git config x.z x # adds another [x], but leaves it populated\n $ t;t;t;t;t;t;t;t\n $ grep -c '\\[x\\]' .git/config\n 4\n\n-- \n ((x=>x(x))(x=>x(x))) Eli Barzilay:\n http://barzilay.org/ Maze is Life!\n"},{"id":"299328","messageId":"20160815120916.6iobqirqbg76exms@sigill.intra.peff.net","threadId":"43848","inReplyTo":"CALO-gutdz5VMgoRmbqEa9UiaTC+L2Sy2n-3AF+zfPr-X8+1U4A@mail.gmail.com","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-15T12:09:16Z","receivedAt":"2016-08-15T12:09:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 14, 2016 at 09:29:27PM -0400, Eli Barzilay wrote:\n\n> Looks like there's a problem with setting a config with an empty\n> section, making it create a new section. The result is:\n> \n> $ git --version\n> git version 2.9.2\n> $ git init\n> Initialized empty Git repository in /home/eli/t/.git/\n> $ t() { git config x.y x; git config --unset x.y; }\n> $ t;t;t\n> $ grep -c '\\[x\\]' .git/config\n> 3\n> $ git config x.z x\n> $ t;t;t\n> $ git config x.z x # adds another [x], but leaves it populated\n> $ t;t;t;t;t;t;t;t\n> $ grep -c '\\[x\\]' .git/config\n> 4\n\nYep. This is a very old and well-known bug. IIRC, the problem is that\nconfig updates use the regular callback-parser, so they see only the\nconfig keys themselves. Empty section headers never trigger them.\nFixing it would require rewriting the config-update code.\n\nAnd implicit in your test is the other bug, which is that deleting the\nlast key in a section leaves the empty header. I think it's related to\nthe same issue.\n\nPatches welcome, of course.\n\n-Peff\n"},{"id":"299361","messageId":"8737m63phh.fsf@linux-m68k.org","threadId":"43848","inReplyTo":"20160815120916.6iobqirqbg76exms@sigill.intra.peff.net","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-08-15T17:34:50Z","receivedAt":"2016-08-15T17:34:59Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Aug 15 2016, Jeff King wrote:\n\n> And implicit in your test is the other bug, which is that deleting the\n> last key in a section leaves the empty header. I think it's related to\n> the same issue.\n\nIndiscriminately removing empty section headers may break comments that\nhave been put there on purpose.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"299362","messageId":"xmqqvaz2gc24.fsf@gitster.mtv.corp.google.com","threadId":"43848","inReplyTo":"CALO-gutdz5VMgoRmbqEa9UiaTC+L2Sy2n-3AF+zfPr-X8+1U4A@mail.gmail.com","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T17:46:27Z","receivedAt":"2016-08-15T17:46:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eli Barzilay writes:\n\n> Looks like there's a problem with setting a config with an empty\n> section, making it create a new section. The result is:\n\nThanks; this unfortunately is a well-known issue, that once was even\n(supposed to be) a part of GSoC project but hasn't been solved.\n\nThe latest mention of the same issue I think is\n\nhttps://public-inbox.org/git/xmqqeg95aor6.fsf@gitster.mtv.corp.google.com/\n\nwhich redirects to\n\n If you are interested, start here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/219505/focus=219524\n\nbut gmane web interface is not working and an equivalent is here:\n\nhttps://public-inbox.org/git/20130329195155.GA19994@sigill.intra.peff.net/\n\n"},{"id":"299366","messageId":"20160815180905.znnz6evufsne5wy6@sigill.intra.peff.net","threadId":"43848","inReplyTo":"8737m63phh.fsf@linux-m68k.org","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-15T18:09:05Z","receivedAt":"2016-08-15T18:09:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:\n\n> On Aug 15 2016, Jeff King wrote:\n> \n> > And implicit in your test is the other bug, which is that deleting the\n> > last key in a section leaves the empty header. I think it's related to\n> > the same issue.\n> \n> Indiscriminately removing empty section headers may break comments that\n> have been put there on purpose.\n\nI know, but we do not even do so discriminately.\n\n-Peff\n"},{"id":"299369","messageId":"xmqqh9alhoor.fsf@gitster.mtv.corp.google.com","threadId":"43848","inReplyTo":"20160815180905.znnz6evufsne5wy6@sigill.intra.peff.net","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T18:28:20Z","receivedAt":"2016-08-15T18:29:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:\n>\n>> On Aug 15 2016, Jeff King wrote:\n>> \n>> > And implicit in your test is the other bug, which is that deleting the\n>> > last key in a section leaves the empty header. I think it's related to\n>> > the same issue.\n>> \n>> Indiscriminately removing empty section headers may break comments that\n>> have been put there on purpose.\n>\n> I know, but we do not even do so discriminately.\n\nI notice that we have thought about all the issues when we last\ndiscussed it in 2013. Refining a message from the earlier thread,\nas it illustrates tricky cases in which we have to be careful.\n\nIf we were to remove the section header at this point, we should\nbe removing the comment two out of three cases.\n\n - if it is about section.key, it should go when section.key goes.\n - if it is about section, it should go when section goes.\n - if it is a more generic comment about this configuration file,\n it should stay.\n\nA configuration file may have something like this:\n\n # Now, let's list all the remotes I interact with.\n\n # This is where I push all the separate topics.\n [remote \"github\"]\n # fetch like everybody else without auth\n url = git://github.com/user/git\n # push with auth\n pushURL = github.com:user/git\n # publish my working area as-is\n mirror\n\n # Traditional \"canonical\" one\n [remote \"korg\"]\n url = k.org:/pub/scm/user/git\n\nIf I were to retire the \"github\" account, removing the entire\n[remote \"github\"] section while keeping the comments before that\nsection would be confusing, which would end up with:\n\n # Now, let's list all the remotes I interact with.\n\n # This is where I push all the separate topics.\n\n # Traditional \"canonical\" one\n [remote \"korg\"]\n url = k.org:/pub/scm/user/git\n\nbecause I do not push all the separate topics to korg.\n\nRemoving the section while removing the comments that pertain to the\nsection is impossible; \"Now, let's list all the remotes\" should\nstay, \"This is where I push\" should go.\n\nSo a comment outside [section \"name\"] is tricky; it needs some\nmechanism (or convention) to tell us if it is about the particular\nsection, or it is about the location in the configuration file.\n\nRemoving only the keys and keeping the skelton around would give us:\n\n # Now, let's list all the remotes I interact with.\n # This is where I push all the separate topics.\n [remote \"github\"]\n # fetch like everybody else without auth\n # push with auth\n # publish my working area as-is\n\n # Traditional \"canonical\" one\n [remote \"korg\"]\n url = k.org:/pub/scm/user/git\n\nwhich is still confusing, but at least the confusion is not spread\nto adjacent sections. What we could do easily without understanding\nnatural languages is to remove comments inside [section \"name\"], but\nit still depends on a convention that such a per-key comment comes\nbefore the key, not after, i.e.\n\n # This is where I push all the separate topics.\n [remote \"github\"]\n # fetch like everybody else without auth\n ...\n # publish my working area as-is\n mirror\n\t\t# note that mirror may push out outside tags/ and heads/\n\n\t# Traditional \"canonical\" one\n\t[remote \"korg\"]\n\nWe could also take hints from the indentation level, but now we are\ndeep into the \"convention\" territory once we start doing that.\n"},{"id":"299374","messageId":"87y43x3lt3.fsf@linux-m68k.org","threadId":"43848","inReplyTo":"xmqqh9alhoor.fsf@gitster.mtv.corp.google.com","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-08-15T18:54:16Z","receivedAt":"2016-08-15T18:54:27Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Aug 15 2016, Junio C Hamano wrote:\n\n> If we were to remove the section header at this point, we should\n> be removing the comment two out of three cases.\n>\n> - if it is about section.key, it should go when section.key goes.\n> - if it is about section, it should go when section goes.\n> - if it is a more generic comment about this configuration file,\n> it should stay.\n\nThe comment may not be related to the surrounding keys, but just a\ncommented-out config key that you want to keep around for reference.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"299376","messageId":"20160815185500.htgrz3t2wkztg4ww@sigill.intra.peff.net","threadId":"43848","inReplyTo":"xmqqh9alhoor.fsf@gitster.mtv.corp.google.com","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-15T18:55:01Z","receivedAt":"2016-08-15T18:55:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 15, 2016 at 11:28:20AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:\n> >\n> >> On Aug 15 2016, Jeff King wrote:\n> >> \n> >> > And implicit in your test is the other bug, which is that deleting the\n> >> > last key in a section leaves the empty header. I think it's related to\n> >> > the same issue.\n> >> \n> >> Indiscriminately removing empty section headers may break comments that\n> >> have been put there on purpose.\n> >\n> > I know, but we do not even do so discriminately.\n> \n> I notice that we have thought about all the issues when we last\n> discussed it in 2013. Refining a message from the earlier thread,\n> as it illustrates tricky cases in which we have to be careful.\n\nThanks for digging up the threads that I was too lazy to find.\n\nI agree with most everything here, though I would be happy if somebody\neven wrote a patch to handle the \"easy\" cases.\n\n> So a comment outside [section \"name\"] is tricky; it needs some\n> mechanism (or convention) to tell us if it is about the particular\n> section, or it is about the location in the configuration file.\n\nKeep in mind that even \"outside\" is hard, because sections do not\nexplicitly close.\n\nSo in:\n\n [core]\n foo = bar\n\n # here are my remotes\n\n [remote \"github\"]\n url = ...\n\nHow do we know that the comment is \"outside\" and not part of [core]?\n\nWe can perhaps guess so because there are no keys after it in the\nsection, though there are some special cases, like:\n\n [core]\n foo = bar\n # This isn't necessary anymore because...\n # xyzzy = false\n\nor even:\n\n [core]\n foo = bar # needed because of xyzzy\n\nYou can probably make reasonable cases based on heuristics around\nnewlines, but that is even further into \"convention\" territory.\n\n-Peff\n"},{"id":"299397","messageId":"xmqqshu5g3lc.fsf@gitster.mtv.corp.google.com","threadId":"43848","inReplyTo":"20160815185500.htgrz3t2wkztg4ww@sigill.intra.peff.net","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T20:49:19Z","receivedAt":"2016-08-15T20:49:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> So a comment outside [section \"name\"] is tricky; it needs some\n>> mechanism (or convention) to tell us if it is about the particular\n>> section, or it is about the location in the configuration file.\n>\n> Keep in mind that even \"outside\" is hard, because sections do not\n> explicitly close.\n\nYes, that was what I wanted to say. We'd be deep in the\n\"convention\" territory.\n"},{"id":"299429","messageId":"CALO-guvVMFitNdGYEonXZ9rh8g8=L9gZojXVUu7FO2_0ki24EQ@mail.gmail.com","threadId":"43848","inReplyTo":"20160815185500.htgrz3t2wkztg4ww@sigill.intra.peff.net","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Eli Barzilay","fromEmail":"eli@barzilay.org","sentAt":"2016-08-16T04:06:56Z","receivedAt":"2016-08-16T04:07:02Z","isPatch":false,"sender":{"key":"eli@barzilay.org","avatar":"https://avatars.githubusercontent.com/u/185905?v=4"},"body":"On Mon, Aug 15, 2016 at 2:55 PM, Jeff King wrote:\n> On Mon, Aug 15, 2016 at 11:28:20AM -0700, Junio C Hamano wrote:\n>\n>> I notice that we have thought about all the issues when we last\n>> discussed it in 2013. Refining a message from the earlier thread,\n>> as it illustrates tricky cases in which we have to be careful.\n>\n> Thanks for digging up the threads that I was too lazy to find.\n>\n> I agree with most everything here, though I would be happy if somebody\n> even wrote a patch to handle the \"easy\" cases.\n\nSo it sounds like removing an empty header is problematic in most cases,\nbut adding a new variable to an existing empty header should not be...?\n\nI looked at the code, and had a rough sketch that works as follows:\n\n* Make git_parse_source() call the callback in a special way to note\n that a section header is seen (I hacked it by passing a special value,\n a pointer to a global string, as the second argument)\n\n* Add a new store.last_section_offset field\n\n* In store_aux(), if it's getting the special value, and the section\n name matches, save the offset in store.last_section_offset\n\n* In git_config_set_multivar_in_file_gently() right before the \"write\n the first part of the config\" comment, test that\n (store.seen == 1 && copy_begin == 0 && copy_end == contents_sz\n && store.last_section_offset > 0)\n and if so, write the contents up to that point, and set copy_begin;\n if the condition is false, do the same thing it does now\n\n* A bit after that, add \"&& store.last_section_offset == 0\" to the\n condition that decides whether to call store_write_section()\n\nIt looks to me like something like this can work, but it's very hacky\nbecause I wanted to see if it can work quickly, and because I don't know\nif this kind of a solution will be wanted, and because I don't know\nenough about that code in general. Making it be an actual solution\ninvolves a better way to call the callback in a special way (my hack\ndoes the special thing only if `fn==store_aux`, but it shouldn't),\ncalling it after the last variable in a section is seen so it's added\nafter that.\n\nSo, will something like this be acceptable? If so, is there anyone who\nI can ask questions about the code?\n\n-- \n ((x=>x(x))(x=>x(x))) Eli Barzilay:\n http://barzilay.org/ Maze is Life!\n"},{"id":"299448","messageId":"20160816123631.ckbvzustl2j37gdx@sigill.intra.peff.net","threadId":"43848","inReplyTo":"CALO-guvVMFitNdGYEonXZ9rh8g8=L9gZojXVUu7FO2_0ki24EQ@mail.gmail.com","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-16T12:36:31Z","receivedAt":"2016-08-16T12:36:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 16, 2016 at 12:06:56AM -0400, Eli Barzilay wrote:\n\n> So it sounds like removing an empty header is problematic in most cases,\n> but adding a new variable to an existing empty header should not be...?\n\nI think it's less problematic, though there are still some funny cases.\nLike:\n\n [foo]\n # comments for the \"foo\" section\n\n # comments introducing the \"bar\" section\n [bar]\n\nIdeally the new entry goes in between the two comments. But I don't\nthink it would be unreasonable to add it right after \"[foo]\". In this\nexample, using the blank line as a hint might be useful, but that's\ngoing to be a lot harder to coax out of the callback-parser as syntactic\nevent.\n\n> I looked at the code, and had a rough sketch that works as follows:\n> \n> * Make git_parse_source() call the callback in a special way to note\n> that a section header is seen (I hacked it by passing a special value,\n> a pointer to a global string, as the second argument)\n\nWe'd want to make sure that didn't kick in for \"regular\" callers, with\nsome kind of option (or, blech, a global variable that changes the\nbehavior of git_parse_source(), though the config code is already full\nof them).\n\n> * Add a new store.last_section_offset field\n>\n> * In store_aux(), if it's getting the special value, and the section\n> name matches, save the offset in store.last_section_offset\n\nMake sense.\n\n> * In git_config_set_multivar_in_file_gently() right before the \"write\n> the first part of the config\" comment, test that\n> (store.seen == 1 && copy_begin == 0 && copy_end == contents_sz\n> && store.last_section_offset > 0)\n> and if so, write the contents up to that point, and set copy_begin;\n> if the condition is false, do the same thing it does now\n\nThis part I'm not sure about. Naively I'd think you could do with less\nspecial-casing here. If we want to add \"foo.two\" and find:\n\n [foo]\n one = whatever\n\nthen we mark the end of that line as the point we want to copy up to,\nand then add our new \"two = ...\" line.\n\nConceptually if we see just:\n\n [foo]\n\nwe should be able to kick in the same code. So it seems like the logic\nwould all be in the detection and setting of the offset.\n\nBut I am not sure I understand all of the code here, and it's rather\ncomplicated. So there is probably a good reason that wouldn't work.\n\n> It looks to me like something like this can work, but it's very hacky\n> because I wanted to see if it can work quickly, and because I don't know\n> if this kind of a solution will be wanted, and because I don't know\n> enough about that code in general. Making it be an actual solution\n> involves a better way to call the callback in a special way (my hack\n> does the special thing only if `fn==store_aux`, but it shouldn't),\n> calling it after the last variable in a section is seen so it's added\n> after that.\n> \n> So, will something like this be acceptable? If so, is there anyone who\n> I can ask questions about the code?\n\nYeah, I think this general strategy is the smallest change that could\nwork.\n\nWhat I think would be much more sane in general is to parse the whole\nthing into a tree (or even a flat list of events), marking each\nsyntactically as a section header, a key, a comment, a run of\nwhitespace, and so on. And then do manipulations on that tree (e.g.,\nwalk down to the section of interest, add a new key at the end), and\nthen reformat the tree back into a stream of bytes. That lets you\nseparate the policy logic from the parsing, and do high-level operations\nthat might involve multiple passes or backtracking in the tree (e.g.,\nwalk to the section, walk past any existing keys, walk past any comments\nbut _not_ past any blank lines, then add the new key).\n\nThere are other other upsides, too. For example, the current code cannot\nwrite multiple unrelated keys in a single pass.\n\nThe downsides is that it's a complete rewrite of the code. So it's a\n_lot_ more work, and it carries more risk of regression. So please don't\ntake this as \"no, your solution isn't OK; you have to do the rewrite\".\nWe've been living with band-aids on the config code for a decade, so I\nam OK with one more.\n\n-Peff\n"},{"id":"299824","messageId":"968003ae-4f62-ebe9-f7af-53b4e1bcf7fd@gmail.com","threadId":"43848","inReplyTo":"20160816123631.ckbvzustl2j37gdx@sigill.intra.peff.net","subject":"Re: Minor bug: git config ignores empty sections","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-08-21T17:09:54Z","receivedAt":"2016-08-21T17:10:19Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 16.08.2016 o 14:36, Jeff King pisze:\n\n> What I think would be much more sane in general is to parse the whole\n> thing into a tree (or even a flat list of events),\n\nList of events is cheaper on memory (though I don't think there is\nan issue with size of config file), and can be turned into tree\nand DOM easily.\n\n> marking each\n> syntactically as a section header, a key, a comment, a run of\n> whitespace, and so on. And then do manipulations on that tree (e.g.,\n> walk down to the section of interest, add a new key at the end), and\n> then reformat the tree back into a stream of bytes. That lets you\n> separate the policy logic from the parsing, and do high-level operations\n> that might involve multiple passes or backtracking in the tree (e.g.,\n> walk to the section, walk past any existing keys, walk past any comments\n> but _not_ past any blank lines, then add the new key).\n> \n> There are other other upsides, too. For example, the current code cannot\n> write multiple unrelated keys in a single pass.\n> \n> The downsides is that it's a complete rewrite of the code. So it's a\n> _lot_ more work, and it carries more risk of regression. So please don't\n> take this as \"no, your solution isn't OK; you have to do the rewrite\".\n> We've been living with band-aids on the config code for a decade, so I\n> am OK with one more.\n\nSo how 'git config --show-origin --list' works? \n\nAh, I guess it prints as it goes, instead of parsing config file(s)\ninto stream of events, and printing from those.\n\n-- \nJakub Narębski\n\n"}]}