git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Thomas Rast <trast@student.ethz.ch>,
	Phil Haack <haacked@gmail.com>
Subject: Re: [PATCH] t1300: document some aesthetic failures of the config editor
Date: Fri, 29 Mar 2013 15:51:55 -0400	[thread overview]
Message-ID: <20130329195155.GA19994@sigill.intra.peff.net> (raw)
In-Reply-To: <7vobe2nins.fsf@alter.siamese.dyndns.org>

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@peff.net>
---
 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

  reply	other threads:[~2013-03-29 19:52 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-03-29 16:29 Minor bug in git branch --set-upstream-to adding superfluous branch section to config Phil Haack
2013-03-29 17:00 ` Jeff King
2013-03-29 17:20   ` Thomas Rast
2013-03-29 17:23     ` Jeff King
2013-03-29 17:50       ` [PATCH] t1300: document some aesthetic failures of the config editor Jeff King
2013-03-29 18:51         ` Junio C Hamano
2013-03-29 19:51           ` Jeff King [this message]
2013-03-29 20:35             ` Junio C Hamano
2013-03-30  0:21               ` Jeff King
2018-03-28 16:33             ` Johannes Schindelin
2018-03-28 17:59               ` Jeff King
2013-03-29 20:00           ` Junio C Hamano
2013-03-29 17:27 ` Minor bug in git branch --set-upstream-to adding superfluous branch section to config Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130329195155.GA19994@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=haacked@gmail.com \
    --cc=trast@student.ethz.ch \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).