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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2013, 18:51 UTC
Message-ID: <7vobe2nins.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130329175058.GA13506@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 3c96fda..d62facb 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1087,4 +1087,36 @@ 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 be saved
> +	key = value
> +	EOF
> +
> +	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).

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

Show 18 quoted lines

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