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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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

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