Re: [PATCH] t1300: document some aesthetic failures of the config editor

Jeff King [off-list ref] writes:

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).

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".

+
+	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
+'

This side I would agree it is unconditionally a good thing to do.

On Fri, Mar 29, 2013 at 11:51:51AM -0700, Junio C Hamano wrote:

quoted

+	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 <redacted>
---
 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

Hi Peff,

On Fri, 29 Mar 2013, Jeff King wrote:

This would have been good on its own. This documents what a user may
expect, and it is a reasonable expectation.

However, Junio, what you suggested in addition *and squashed in before
merging to `master`*, is not a reasonable expectation. If you are asking
*code* to determine that in a config like this, the second line is a
comment belonging to the section, and the first line is not, that is
totally unreasonable:

        # some generic comment on the configuration file itself
        # a comment specific to this "section" section.
        [section]
        # some intervening lines
        # that should also be dropped

        key = value
        # please be careful when you update the above variable


Worse: it does *not* demonstrate a known breakage, at least not precisely,
as what you ask here *is not technically possible*. Not even with NLP, at
least if you drive for 100%. It's just not.

And your example is not even complete, as this is a totally valid config:

	[core]
		; These settings affect many Git operations; be careful
		; what you change here

		key = value

Obviously, your example gives the impression that `git config --unset
core.key` shoud *delete* that comment (that obviously is intended to
document the section, not the `key` value).

And this is bad, really bad. And this comment does not make it better:

	I think we may not attain that ideal without some natural language
	processing of the comments. But hey, no reason not to shoot for the
	stars. :)

There *is* a reason, a very good reason *not* to shoot for the stars.

Think about it. The `test_expect_failure` function is intended to
demonstrate bugs, and once those bugs are fixed, the _failure should be
turned into _success. And if somebody looks for work, they can look for
test_expect_failure and find tons of micro-projects.

What you did there was to change some valid demonstration of a bug that
can be fixed to something that cannot be fixed. So if an occasional lurker
comes along, sees what you expect to be fixed, they would have said
"Whoa!" and you lost a contribution.

Let's avoid such a "shoot for the stars [... and get nothing, not even an
incremental improvement in return...]" in the future.

On a positive note: I just finished work on a set of patches addressing
this:
https://github.com/git/git/compare/master...dscho:empty-config-section (I
plan on submitting this tomorrow)

Ciao,
Dscho

P.S.: While I am already raising awareness about unintended consequences,
let me also add this: that "cute" feature that unambiguous abbreviations
of options are allowed bit me royally today. Try this: `git config
--remove section.key`. And then be surprised that it does not work, even
if you have that entry. The reason? The option `--remove` is a unique
abbreviation of `--remove-section`, so even if I clearly meant `--unset`,
that feature (that every Git user I tell about it is very surprised to
hear about, so it is not like it is helping a lot of users) has the
unintended consequence of being completely wrong. It would have been
better to tell me that there is no `--remove` option.

On Wed, Mar 28, 2018 at 06:33:55PM +0200, Johannes Schindelin wrote:

On Fri, 29 Mar 2013, Jeff King wrote:

quoted

Subject: [PATCH] t1300: document some aesthetic failures of the config editor

This is an old one. :) I had to go look up the old thread to refresh
myself.

[...]
Obviously, your example gives the impression that `git config --unset
core.key` shoud *delete* that comment (that obviously is intended to
document the section, not the `key` value).

And this is bad, really bad. And this comment does not make it better:

	I think we may not attain that ideal without some natural language
	processing of the comments. But hey, no reason not to shoot for the
	stars. :)

There *is* a reason, a very good reason *not* to shoot for the stars.

I think you are reading more into my comment than was intended. No, I
don't think we plan to implement a sufficiently advanced AI to cover all
these cases. But as I said in the thread:

  It makes sense 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.

So it was always intended for this test to give a general sense of the
problem, from which somebody interested could dig further and work on
it.

Probably the commit message could have made this more clear (or even an
in-code comment).

Think about it. The `test_expect_failure` function is intended to
demonstrate bugs, and once those bugs are fixed, the _failure should be
turned into _success. And if somebody looks for work, they can look for
test_expect_failure and find tons of micro-projects.

What you did there was to change some valid demonstration of a bug that
can be fixed to something that cannot be fixed. So if an occasional lurker
comes along, sees what you expect to be fixed, they would have said
"Whoa!" and you lost a contribution.

Hypothetically, you may be right. But don't all bugs have some element
of this? People can find an expect_failure as a starting point, but
they'll have to dig into the background and history of the bug if they
want to know the subtleties. This one is just more subtle than some
others.

On a positive note: I just finished work on a set of patches addressing
this:
https://github.com/git/git/compare/master...dscho:empty-config-section (I
plan on submitting this tomorrow)

Great. If your series throws away my test and replaces it with something
more attainable (preferably with expect_success ;) ), I think that is
certainly a positive change.

-Peff