{"thread":{"id":"33327","subject":"Minor bug in git branch --set-upstream-to adding superfluous branch section to config","startedAt":"2013-03-29T16:29:14Z","lastAt":"2018-03-28T17:59:57Z","messageCount":13,"participants":["Phil Haack","Jeff King","Thomas Rast","Junio C Hamano","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"212625","messageId":"CAD7mMPW=jr6PaAc50h-Wpf42-UPrn0A5KmisqXNXqqLv78AEgg@mail.gmail.com","threadId":"33327","inReplyTo":null,"subject":"Minor bug in git branch --set-upstream-to adding superfluous branch section to config","fromName":"Phil Haack","fromEmail":"haacked@gmail.com","sentAt":"2013-03-29T16:29:14Z","receivedAt":"2013-03-29T16:29:14Z","isPatch":false,"sender":{"key":"haacked@gmail.com","avatar":null},"body":"Hello there! Please /cc me with responses as I'm not on the mailing list.\n\nI'm using git version 1.8.1.msysgit.1 and I ran into a very minor issue. It\ndoesn't actually seem to affect operations, but I thought I'd report it in case\nsomeone felt it was worth cleaning up.\n\nIf you run the following set of commands:\n\n    git checkout -b some-branch\n    git push origin some-branch\n    git branch --set-upstream-to origin/some-branch\n    git branch --unset-upstream some-branch\n    git branch --set-upstream-to origin/some-branch\n\nYou get the following result in the .git\\config file\n\n    [branch \"some-branch\"]\n    [branch \"some-branch\"]\n        remote = origin\n        merge = refs/heads/some-branch\n\nMy expectation is that the very last call to: git branch --set-upstream-to\nwould not add a new config section, but would instead insert this information\ninto the existing [branch \"some-branch\"].\n\nIn any case, from what I understand the current behavior is technically valid\nand I haven't encountered any adverse effects. It was just something I noticed\nwhile testing.\n\nThanks!\n"},{"id":"212627","messageId":"20130329170032.GA3552@sigill.intra.peff.net","threadId":"33327","inReplyTo":"CAD7mMPW=jr6PaAc50h-Wpf42-UPrn0A5KmisqXNXqqLv78AEgg@mail.gmail.com","subject":"Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T17:00:32Z","receivedAt":"2013-03-29T17:00:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 09:29:14AM -0700, Phil Haack wrote:\n\n> If you run the following set of commands:\n> \n>     git checkout -b some-branch\n>     git push origin some-branch\n>     git branch --set-upstream-to origin/some-branch\n>     git branch --unset-upstream some-branch\n>     git branch --set-upstream-to origin/some-branch\n> \n> You get the following result in the .git\\config file\n> \n>     [branch \"some-branch\"]\n>     [branch \"some-branch\"]\n>         remote = origin\n>         merge = refs/heads/some-branch\n> \n> My expectation is that the very last call to: git branch --set-upstream-to\n> would not add a new config section, but would instead insert this information\n> into the existing [branch \"some-branch\"].\n\nYes, this is a known issue[1] in the config-editing code. There are\nactually two separate problems. Try this:\n\n  $ git config -f foo section.one 1\n  $ cat foo\n  [section]\n          one = 1\n\nLooking good so far...\n\n  $ git config -f foo --unset section.one\n  $ cat foo\n  [section]\n\nOops, it would have been nice to clean up that now-empty section. Now\nlet's re-add...\n\n  $ git config -f foo section.two 2\n  $ cat foo\n  [section]\n  [section]\n          two = 2\n\nOops, it would have been nice to use the existing section. What happens\nif we add again...\n\n  $ git config -f foo section.three 3\n  $ cat foo\n  [section]\n  [section]\n          two = 2\n          three = 3\n\nNow that one worked. I think what happens is that the config editor runs\nthrough the files linearly, munging whatever lines necessary for the\nrequested operation, and leaving everything else untouched (as it must,\nto leave comments and whitespace intact). But it does not keep a\nlook-behind buffer to realize that a section name is now obsolete (which\nwe don't know until we get to the next section, or to EOF). In the worst\ncase, this buffer can grow arbitrarily large, like:\n\n  [foo]\n  # the above section is now empty\n  # but we have to read through all of\n  # these comments to actually\n  # realize it\n  [bar]\n\nIn practice it should not be a big deal (and I do not think it is an\ninteresting attack vector for somebody trying to run you out of memory).\n\nThat brings up another point, which is that comments may get misplaced\nby deletion. But that's the case for any modification we do to the file,\nsince we cannot understand the semantics of comments that say things\nlike \"the line below does X\".\n\nSo that's the first bug. The second one is, I suspect, just laziness. We\nnotice that section.two is set, and put section.three after it. But we\ndo not make the same trigger for just \"section\". That's probably much\neasier to fix.\n\n> In any case, from what I understand the current behavior is technically valid\n> and I haven't encountered any adverse effects. It was just something I noticed\n> while testing.\n\nYes, the config files generated by these operations parse as you would\nexpect them to. I think it's purely an aesthetic concern.\n\n-Peff\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/208113\n"},{"id":"212630","messageId":"87ip4ayvfn.fsf@linux-k42r.v.cablecom.net","threadId":"33327","inReplyTo":"20130329170032.GA3552@sigill.intra.peff.net","subject":"Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2013-03-29T17:20:28Z","receivedAt":"2013-03-29T17:20:28Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think what happens is that the config editor runs\n> through the files linearly, munging whatever lines necessary for the\n> requested operation, and leaving everything else untouched (as it must,\n> to leave comments and whitespace intact). But it does not keep a\n> look-behind buffer to realize that a section name is now obsolete (which\n> we don't know until we get to the next section, or to EOF). In the worst\n> case, this buffer can grow arbitrarily large, like:\n>\n>   [foo]\n>   # the above section is now empty\n>   # but we have to read through all of\n>   # these comments to actually\n>   # realize it\n>   [bar]\n\nIf we treat this case as having a bunch of comments that make the\nsection non-empty, then we both avoid needing an arbitrarily large\nlookbehind and deleting the user's precious comments...\n\nI.e. the rule would be that we only delete the section if there is\nnothing but whitespace until the next section header.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"212631","messageId":"20130329172307.GA11099@sigill.intra.peff.net","threadId":"33327","inReplyTo":"87ip4ayvfn.fsf@linux-k42r.v.cablecom.net","subject":"Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T17:23:07Z","receivedAt":"2013-03-29T17:23:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 06:20:28PM +0100, Thomas Rast wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > I think what happens is that the config editor runs\n> > through the files linearly, munging whatever lines necessary for the\n> > requested operation, and leaving everything else untouched (as it must,\n> > to leave comments and whitespace intact). But it does not keep a\n> > look-behind buffer to realize that a section name is now obsolete (which\n> > we don't know until we get to the next section, or to EOF). In the worst\n> > case, this buffer can grow arbitrarily large, like:\n> >\n> >   [foo]\n> >   # the above section is now empty\n> >   # but we have to read through all of\n> >   # these comments to actually\n> >   # realize it\n> >   [bar]\n> \n> If we treat this case as having a bunch of comments that make the\n> section non-empty, then we both avoid needing an arbitrarily large\n> lookbehind and deleting the user's precious comments...\n> \n> I.e. the rule would be that we only delete the section if there is\n> nothing but whitespace until the next section header.\n\nI think that is sane. Technically it does not remove the need for the\nbuffer, unless we are also collapsing whitespace (which I think is\nprobably a sane thing to do, too). I'm looking at a patch now...\n\n-Peff\n"},{"id":"212633","messageId":"7v4nfup14c.fsf@alter.siamese.dyndns.org","threadId":"33327","inReplyTo":"CAD7mMPW=jr6PaAc50h-Wpf42-UPrn0A5KmisqXNXqqLv78AEgg@mail.gmail.com","subject":"Re: Minor bug in git branch --set-upstream-to adding superfluous branch section to config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T17:27:47Z","receivedAt":"2013-03-29T17:27:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phil Haack <haacked@gmail.com> writes:\n\n> Hello there! Please /cc me with responses as I'm not on the mailing list.\n>\n> I'm using git version 1.8.1.msysgit.1 and I ran into a very minor issue. It\n> doesn't actually seem to affect operations, but I thought I'd report it in case\n> someone felt it was worth cleaning up.\n\nI do not think this is limited to \"branch --set-whatever\".\n\n> If you run the following set of commands:\n>\n>     git checkout -b some-branch\n>     git push origin some-branch\n>     git branch --set-upstream-to origin/some-branch\n>     git branch --unset-upstream some-branch\n\nThis step causes the removal of the last variable in a configuration\nsection, which leaves an empty section.  As the code to add a new\nvariable to the configuration, trying to reuse an existing section\nheader, does not pay attention to an empty section, you end up with\nan empty section, followed by a new one.\n\nEither removal of configuration variable should be taught to remove\nthe section header when a section becomes empty, or addition of\nconfiguration variable should be taught to notice an empty section\nheader.\n\n\n\n>     git branch --set-upstream-to origin/some-branch\n>\n> You get the following result in the .git\\config file\n>\n>     [branch \"some-branch\"]\n>     [branch \"some-branch\"]\n>         remote = origin\n>         merge = refs/heads/some-branch\n"},{"id":"212637","messageId":"20130329175058.GA13506@sigill.intra.peff.net","threadId":"33327","inReplyTo":"20130329172307.GA11099@sigill.intra.peff.net","subject":"[PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T17:50:58Z","receivedAt":"2013-03-29T17:50:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config-editing code used by \"git config var value\" is\nbuilt around the regular config callback parser, whose only\ntriggerable item is an actual key. As a result, it does not\nknow anything about section headers, which can result in\nunnecessarily ugly output:\n\n  1. When we delete the last key in a section, we should be\n     able to delete the section header.\n\n  2. When we add a key into a section, we should be able to\n     reuse the same section header, even if that section did\n     not have any keys in it already.\n\nUnfortunately, fixing these is not trivial with the current\ncode. It would involve the config parser recording and\npassing back information on each item it finds, including\nheaders, keys, and even comments (or even better, generating\nan actual in-memory parse-tree).\n\nSince these behaviors do not cause any functional problems\n(i.e., the resulting config parses as expected, it is just\nuglier than one would like), fixing them can wait until\nsomebody feels like substantially refactoring the parsing\ncode. In the meantime, let's document them as known issues\nwith some tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI had hoped this would be a quick fix, but it really isn't. I'm happy\nenough if somebody wants to try to work on it, but I think the only\nclean fix is going to involve a rewrite of the parsing code, so I'm\nwilling to let it go for now.\n\nJunio, note that this conflicts semantically (but not textually) with\nthe tip of jc/apply-ws-fix-tab-in-indent, which refactors q_to_tab into\nqz_to_tab_space.\n\n t/t1300-repo-config.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3c96fda..d62facb 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1087,4 +1087,36 @@ test_expect_success 'barf on incomplete string' '\n \tgrep \" line 3 \" error\n '\n \n+# good section hygiene\n+test_expect_failure 'unsetting the last key in a section removes header' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[section]\n+\t# some intervening lines\n+\t# that should be saved\n+\tkey = value\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\t# some intervening lines\n+\t# that should be saved\n+\tEOF\n+\n+\tgit config --unset section.key &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_failure 'adding a key into an empty section reuses header' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[section]\n+\tEOF\n+\n+\tq_to_tab >expect <<-\\EOF &&\n+\t[section]\n+\tQkey = value\n+\tEOF\n+\n+\tgit config section.key value\n+\ttest_cmp expect .git/config\n+'\n+\n test_done\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"212641","messageId":"7vobe2nins.fsf@alter.siamese.dyndns.org","threadId":"33327","inReplyTo":"20130329175058.GA13506@sigill.intra.peff.net","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T18:51:51Z","receivedAt":"2013-03-29T18:51:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 3c96fda..d62facb 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -1087,4 +1087,36 @@ test_expect_success 'barf on incomplete string' '\n>  \tgrep \" line 3 \" error\n>  '\n>  \n> +# good section hygiene\n> +test_expect_failure 'unsetting the last key in a section removes header' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[section]\n> +\t# some intervening lines\n> +\t# that should be saved\n> +\tkey = value\n> +\tEOF\n> +\n> +\tcat >expect <<-\\EOF &&\n> +\t# some intervening lines\n> +\t# that should be saved\n> +\tEOF\n\nI do not know if I agree with this expectation.\n\nMost likely these comments are about the section, and possibly even\nare specific to section.key, not applicable to the section in\ngeneral).  If we _were_ to remove the section header at this point,\nwe should be removing the comment two out of three cases (if it is\nabout section.key, it should go when section.key goes; if it is\nabout section, it should go when section goes; if it is a more\ngeneric comment about this configuration file, it should stay).\n\nA better approach may be to only insist on the \"when adding, reuse\nan empty section header\" side of the coin.  Then we do not have to\nworry about \"we keep cruft that talks about some section but what\nthe comment says is illegible now the crucial bit of information,\nsection name the comment talks about, is gone\".\n\n> +\n> +\tgit config --unset section.key &&\n> +\ttest_cmp expect .git/config\n> +'\n> +\n> +test_expect_failure 'adding a key into an empty section reuses header' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[section]\n> +\tEOF\n> +\n> +\tq_to_tab >expect <<-\\EOF &&\n> +\t[section]\n> +\tQkey = value\n> +\tEOF\n> +\n> +\tgit config section.key value\n> +\ttest_cmp expect .git/config\n> +'\n\nThis side I would agree it is unconditionally a good thing to do.\n"},{"id":"212644","messageId":"20130329195155.GA19994@sigill.intra.peff.net","threadId":"33327","inReplyTo":"7vobe2nins.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-29T19:51:55Z","receivedAt":"2013-03-29T19:51:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 11:51:51AM -0700, Junio C Hamano wrote:\n\n> > +\tcat >expect <<-\\EOF &&\n> > +\t# some intervening lines\n> > +\t# that should be saved\n> > +\tEOF\n> \n> I do not know if I agree with this expectation.\n> \n> Most likely these comments are about the section, and possibly even\n> are specific to section.key, not applicable to the section in\n> general).  If we _were_ to remove the section header at this point,\n> we should be removing the comment two out of three cases (if it is\n> about section.key, it should go when section.key goes; if it is\n> about section, it should go when section goes; if it is a more\n> generic comment about this configuration file, it should stay).\n\nI agree that probably makes more sense (I actually wrote the test before\nresponding to Thomas, and then got bogged down in the code change and\nforgot to update it when I decided to give up).\n\n> A better approach may be to only insist on the \"when adding, reuse\n> an empty section header\" side of the coin.  Then we do not have to\n> worry about \"we keep cruft that talks about some section but what\n> the comment says is illegible now the crucial bit of information,\n> section name the comment talks about, is gone\".\n\nI think they are two separate problems. They happen to combine to\nproduce the behavior that Phil reported, but I would still expect\n\"--unset\" not to leave cruft. It makes sense to document to me to\ndocument both via tests; even if we end up tweaking the expected\nbehavior when the fix is actually implemented, the presence of the test\nstill serves as a reminder of the issue.\n\nHere it is with the updated expectation. I don't care _that_ much, so if\nyou feel strongly and want to drop the first test, feel free.\n\n-- >8 --\nSubject: [PATCH] t1300: document some aesthetic failures of the config editor\n\nThe config-editing code used by \"git config var value\" is\nbuilt around the regular config callback parser, whose only\ntriggerable item is an actual key. As a result, it does not\nknow anything about section headers, which can result in\nunnecessarily ugly output:\n\n  1. When we delete the last key in a section, we should be\n     able to delete the section header.\n\n  2. When we add a key into a section, we should be able to\n     reuse the same section header, even if that section did\n     not have any keys in it already.\n\nUnfortunately, fixing these is not trivial with the current\ncode. It would involve the config parser recording and\npassing back information on each item it finds, including\nheaders, keys, and even comments (or even better, generating\nan actual in-memory parse-tree).\n\nSince these behaviors do not cause any functional problems\n(i.e., the resulting config parses as expected, it is just\nuglier than one would like), fixing them can wait until\nsomebody feels like substantially refactoring the parsing\ncode. In the meantime, let's document them as known issues\nwith some tests.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t1300-repo-config.sh | 30 ++++++++++++++++++++++++++++++\n 1 file changed, 30 insertions(+)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 3c96fda..213e5a8 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1087,4 +1087,34 @@ test_expect_success 'barf on incomplete string' '\n \tgrep \" line 3 \" error\n '\n \n+# good section hygiene\n+test_expect_failure 'unsetting the last key in a section removes header' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[section]\n+\t# some intervening lines\n+\t# that should also be dropped\n+\n+\tkey = value\n+\tEOF\n+\n+\t>expect &&\n+\n+\tgit config --unset section.key &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_failure 'adding a key into an empty section reuses header' '\n+\tcat >.git/config <<-\\EOF &&\n+\t[section]\n+\tEOF\n+\n+\tq_to_tab >expect <<-\\EOF &&\n+\t[section]\n+\tQkey = value\n+\tEOF\n+\n+\tgit config section.key value\n+\ttest_cmp expect .git/config\n+'\n+\n test_done\n-- \n1.8.2.rc3.27.g5c55d5c\n"},{"id":"212645","messageId":"7vd2uinfhc.fsf@alter.siamese.dyndns.org","threadId":"33327","inReplyTo":"7vobe2nins.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T20:00:31Z","receivedAt":"2013-03-29T20:00:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> ...  If we _were_ to remove the section header at this point,\n> we should be removing the comment two out of three cases (if it is\n> about section.key, it should go when section.key goes; if it is\n> about section, it should go when section goes; if it is a more\n> generic comment about this configuration file, it should stay).\n\nThe last case should end more like this:\n\n\t..., it should stay, but why are you writing a comment that\n\tis not specific to this section _inside_ the section in the\n\tfirst place???\n\nAnother case we have to worry about, if we were to remove an empty\nsection header is this case:\n\n\t# 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\t\t# fetch like everybody else without auth\n\t\turl = git://github.com/user/git\n\t\t# push with auth\n                pushURL = github.com:user/git\n\t\t# publish the state of my primary working area as-is\n                mirror\n\n\t# Traditional \"canonical\" one\n\t[remote \"korg\"]\n        \turl = k.org:/pub/scm/user/git\n\nIf I were to retire \"github\" account, removing the section while\nkeeping the comments would be confusing (I do not push all the\nseparate topics to korg).  Removing the section while removing the\ncomments that pertain to the section is impossible; \"Now, let's list\nall the remotes\" should stay, \"This is where I push\" should go.\n\nRemoving only the keys and keeping the skelton around would give us:\n\n\t# 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\t\t# fetch like everybody else without auth\n\t\t# push with auth\n\t\t# publish the state of my primary working area as-is\n\n\t# Traditional \"canonical\" one\n\t[remote \"korg\"]\n        \turl = k.org:/pub/scm/user/git\n\nwhich is still confusing, but at least the confusion is not spread\nto adjacent sections.\n"},{"id":"212648","messageId":"7vvc8alzat.fsf@alter.siamese.dyndns.org","threadId":"33327","inReplyTo":"20130329195155.GA19994@sigill.intra.peff.net","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T20:35:22Z","receivedAt":"2013-03-29T20:35:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Here it is with the updated expectation. I don't care _that_ much, so if\n> you feel strongly and want to drop the first test, feel free.\n\nAs long as we are adding expect_failure without an immediate fix,\nlet's document the ideal, with this patch on top.\n\n t/t1300-repo-config.sh | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 213e5a8..133f26d 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -1089,16 +1089,20 @@ test_expect_success 'barf on incomplete string' '\n \n # good section hygiene\n test_expect_failure 'unsetting the last key in a section removes header' '\n \tcat >.git/config <<-\\EOF &&\n+\t# some generic comment on the configuration file itself\n+\t# a comment specific to this \"section\" section.\n \t[section]\n \t# some intervening lines\n \t# that should also be dropped\n \n \tkey = value\n \tEOF\n \n-\t>expect &&\n+\tcat >expect <<-\\EOF &&\n+\t# some generic comment on the configuration file itself\n+\tEOF\n \n \tgit config --unset section.key &&\n \ttest_cmp expect .git/config\n '\n"},{"id":"212666","messageId":"20130330002139.GA32353@sigill.intra.peff.net","threadId":"33327","inReplyTo":"7vvc8alzat.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-30T00:21:39Z","receivedAt":"2013-03-30T00:21:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2013 at 01:35:22PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Here it is with the updated expectation. I don't care _that_ much, so if\n> > you feel strongly and want to drop the first test, feel free.\n> \n> As long as we are adding expect_failure without an immediate fix,\n> let's document the ideal, with this patch on top.\n> [...]\n>  test_expect_failure 'unsetting the last key in a section removes header' '\n>  \tcat >.git/config <<-\\EOF &&\n> +\t# some generic comment on the configuration file itself\n> +\t# a comment specific to this \"section\" section.\n>  \t[section]\n>  \t# some intervening lines\n>  \t# that should also be dropped\n>  \n>  \tkey = value\n>  \tEOF\n>  \n> -\t>expect &&\n> +\tcat >expect <<-\\EOF &&\n> +\t# some generic comment on the configuration file itself\n> +\tEOF\n\nI think we may not attain that ideal without some natural language\nprocessing of the comments. But hey, no reason not to shoot for the\nstars. :)\n\n-Peff\n"},{"id":"343223","messageId":"nycvar.QRO.7.76.6.1803281220100.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"33327","inReplyTo":"20130329195155.GA19994@sigill.intra.peff.net","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-28T16:33:55Z","receivedAt":"2018-03-28T16:34:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 29 Mar 2013, Jeff King wrote:\n\n> Subject: [PATCH] t1300: document some aesthetic failures of the config editor\n>\n> [...]\n> \n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 3c96fda..213e5a8 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -1087,4 +1087,34 @@ test_expect_success 'barf on incomplete string' '\n>  \tgrep \" line 3 \" error\n>  '\n>  \n> +# good section hygiene\n> +test_expect_failure 'unsetting the last key in a section removes header' '\n> +\tcat >.git/config <<-\\EOF &&\n> +\t[section]\n> +\t# some intervening lines\n> +\t# that should also be dropped\n> +\n> +\tkey = value\n> +\tEOF\n> +\n> +\t>expect &&\n> +\n> +\tgit config --unset section.key &&\n> +\ttest_cmp expect .git/config\n> +'\n\nThis would have been good on its own. This documents what a user may\nexpect, and it is a reasonable expectation.\n\nHowever, Junio, what you suggested in addition *and squashed in before\nmerging to `master`*, is not a reasonable expectation. If you are asking\n*code* to determine that in a config like this, the second line is a\ncomment belonging to the section, and the first line is not, that is\ntotally unreasonable:\n\n        # some generic comment on the configuration file itself\n        # a comment specific to this \"section\" section.\n        [section]\n        # some intervening lines\n        # that should also be dropped\n\n        key = value\n        # please be careful when you update the above variable\n\n\nWorse: it does *not* demonstrate a known breakage, at least not precisely,\nas what you ask here *is not technically possible*. Not even with NLP, at\nleast if you drive for 100%. It's just not.\n\nAnd your example is not even complete, as this is a totally valid config:\n\n\t[core]\n\t\t; These settings affect many Git operations; be careful\n\t\t; what you change here\n\n\t\tkey = value\n\nObviously, your example gives the impression that `git config --unset\ncore.key` shoud *delete* that comment (that obviously is intended to\ndocument the section, not the `key` value).\n\nAnd this is bad, really bad. And this comment does not make it better:\n\n\tI think we may not attain that ideal without some natural language\n\tprocessing of the comments. But hey, no reason not to shoot for the\n\tstars. :)\n\nThere *is* a reason, a very good reason *not* to shoot for the stars.\n\nThink about it. The `test_expect_failure` function is intended to\ndemonstrate bugs, and once those bugs are fixed, the _failure should be\nturned into _success. And if somebody looks for work, they can look for\ntest_expect_failure and find tons of micro-projects.\n\nWhat you did there was to change some valid demonstration of a bug that\ncan be fixed to something that cannot be fixed. So if an occasional lurker\ncomes along, sees what you expect to be fixed, they would have said\n\"Whoa!\" and you lost a contribution.\n\nLet's avoid such a \"shoot for the stars [... and get nothing, not even an\nincremental improvement in return...]\" in the future.\n\nOn a positive note: I just finished work on a set of patches addressing\nthis:\nhttps://github.com/git/git/compare/master...dscho:empty-config-section (I\nplan on submitting this tomorrow)\n\nCiao,\nDscho\n\nP.S.: While I am already raising awareness about unintended consequences,\nlet me also add this: that \"cute\" feature that unambiguous abbreviations\nof options are allowed bit me royally today. Try this: `git config\n--remove section.key`. And then be surprised that it does not work, even\nif you have that entry. The reason? The option `--remove` is a unique\nabbreviation of `--remove-section`, so even if I clearly meant `--unset`,\nthat feature (that every Git user I tell about it is very surprised to\nhear about, so it is not like it is helping a lot of users) has the\nunintended consequence of being completely wrong. It would have been\nbetter to tell me that there is no `--remove` option.\n"},{"id":"343254","messageId":"20180328175950.GE16274@sigill.intra.peff.net","threadId":"33327","inReplyTo":"nycvar.QRO.7.76.6.1803281220100.77@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH] t1300: document some aesthetic failures of the config editor","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-28T17:59:51Z","receivedAt":"2018-03-28T17:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 28, 2018 at 06:33:55PM +0200, Johannes Schindelin wrote:\n\n> On Fri, 29 Mar 2013, Jeff King wrote:\n> \n> > Subject: [PATCH] t1300: document some aesthetic failures of the config editor\n\nThis is an old one. :) I had to go look up the old thread to refresh\nmyself.\n\n> [...]\n> Obviously, your example gives the impression that `git config --unset\n> core.key` shoud *delete* that comment (that obviously is intended to\n> document the section, not the `key` value).\n> \n> And this is bad, really bad. And this comment does not make it better:\n> \n> \tI think we may not attain that ideal without some natural language\n> \tprocessing of the comments. But hey, no reason not to shoot for the\n> \tstars. :)\n> \n> There *is* a reason, a very good reason *not* to shoot for the stars.\n\nI think you are reading more into my comment than was intended. No, I\ndon't think we plan to implement a sufficiently advanced AI to cover all\nthese cases. But as I said in the thread:\n\n  It makes sense to me to document both via tests; even if we end up\n  tweaking the expected behavior when the fix is actually implemented,\n  the presence of the test still serves as a reminder of the issue.\n\nSo it was always intended for this test to give a general sense of the\nproblem, from which somebody interested could dig further and work on\nit.\n\nProbably the commit message could have made this more clear (or even an\nin-code comment).\n\n> Think about it. The `test_expect_failure` function is intended to\n> demonstrate bugs, and once those bugs are fixed, the _failure should be\n> turned into _success. And if somebody looks for work, they can look for\n> test_expect_failure and find tons of micro-projects.\n> \n> What you did there was to change some valid demonstration of a bug that\n> can be fixed to something that cannot be fixed. So if an occasional lurker\n> comes along, sees what you expect to be fixed, they would have said\n> \"Whoa!\" and you lost a contribution.\n\nHypothetically, you may be right. But don't all bugs have some element\nof this? People can find an expect_failure as a starting point, but\nthey'll have to dig into the background and history of the bug if they\nwant to know the subtleties. This one is just more subtle than some\nothers.\n\n> On a positive note: I just finished work on a set of patches addressing\n> this:\n> https://github.com/git/git/compare/master...dscho:empty-config-section (I\n> plan on submitting this tomorrow)\n\nGreat. If your series throws away my test and replaces it with something\nmore attainable (preferably with expect_success ;) ), I think that is\ncertainly a positive change.\n\n-Peff\n"}]}