On Fri, Mar 29, 2013 at 11:51:51AM -0700, Junio C Hamano wrote:
> > + cat >expect <<-\EOF &&
> > + # some intervening lines
> > + # that should be saved
> > + EOF
>
> I do not know if I agree with this expectation.
>
> Most likely these comments are about the section, and possibly even
> are specific to section.key, not applicable to the section in
> general). If we _were_ to remove the section header at this point,
> we should be removing the comment two out of three cases (if it is
> about section.key, it should go when section.key goes; if it is
> about section, it should go when section goes; if it is a more
> generic comment about this configuration file, it should stay).
I agree that probably makes more sense (I actually wrote the test before
responding to Thomas, and then got bogged down in the code change and
forgot to update it when I decided to give up).
> A better approach may be to only insist on the "when adding, reuse
> an empty section header" side of the coin. Then we do not have to
> worry about "we keep cruft that talks about some section but what
> the comment says is illegible now the crucial bit of information,
> section name the comment talks about, is gone".
I think they are two separate problems. They happen to combine to
produce the behavior that Phil reported, but I would still expect
"--unset" not to leave cruft. It makes sense to document to me to
document both via tests; even if we end up tweaking the expected
behavior when the fix is actually implemented, the presence of the test
still serves as a reminder of the issue.
Here it is with the updated expectation. I don't care _that_ much, so if
you feel strongly and want to drop the first test, feel free.
-- >8 --
Subject: [PATCH] t1300: document some aesthetic failures of the config editor
The config-editing code used by "git config var value" is
built around the regular config callback parser, whose only
triggerable item is an actual key. As a result, it does not
know anything about section headers, which can result in
unnecessarily ugly output:
1. When we delete the last key in a section, we should be
able to delete the section header.
2. When we add a key into a section, we should be able to
reuse the same section header, even if that section did
not have any keys in it already.
Unfortunately, fixing these is not trivial with the current
code. It would involve the config parser recording and
passing back information on each item it finds, including
headers, keys, and even comments (or even better, generating
an actual in-memory parse-tree).
Since these behaviors do not cause any functional problems
(i.e., the resulting config parses as expected, it is just
uglier than one would like), fixing them can wait until
somebody feels like substantially refactoring the parsing
code. In the meantime, let's document them as known issues
with some tests.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
t/t1300-repo-config.sh | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3c96fda..213e5a8 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1087,4 +1087,34 @@ test_expect_success 'barf on incomplete string' '
grep " line 3 " error
'
+# good section hygiene
+test_expect_failure 'unsetting the last key in a section removes header' '
+ cat >.git/config <<-\EOF &&
+ [section]
+ # some intervening lines
+ # that should also be dropped
+
+ key = value
+ EOF
+
+ >expect &&
+
+ git config --unset section.key &&
+ test_cmp expect .git/config
+'
+
+test_expect_failure 'adding a key into an empty section reuses header' '
+ cat >.git/config <<-\EOF &&
+ [section]
+ EOF
+
+ q_to_tab >expect <<-\EOF &&
+ [section]
+ Qkey = value
+ EOF
+
+ git config section.key value
+ test_cmp expect .git/config
+'
+
test_done
--
1.8.2.rc3.27.g5c55d5c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html