Re: [PATCH v3] commit: add a commit.allowEmpty config variable

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Nov 03 2018, tanushree27 wrote:

> +commit.allowEmpty::
> +	A boolean to specify whether empty commits are allowed with `git
> +	commit`. See linkgit:git-commit[1].
> +	Defaults to false.
> +

Good.

> +	if (config_commit_allow_empty >= 0)  /* if allowEmpty is allowed in config*/
> +		allow_empty = config_commit_allow_empty;
> +

This works, but != -1 is our usual idiom for this as you initialize it
to -1. I think that comment can also go then, since it's clear what's
going on.

> +# Tests for commit.allowEmpty config
> +
> +test_expect_success "no commit.allowEmpty and no --allow-empty" "
> +	test_must_fail git commit -m 'test'
> +"
> +
> +test_expect_success "no commit.allowEmpty and --allow-empty" "
> +	git commit --allow-empty -m 'test'
> +"
> +
> +for i in true 1
> +do
> +	test_expect_success "commit.allowEmpty=$i and no --allow-empty" "
> +		git -c commit.allowEmpty=$i commit -m 'test'
> +	"
> +
> +	test_expect_success "commit.allowEmpty=$i and --allow-empty" "
> +		git -c commit.allowEmpty=$i commit --allow-empty -m 'test'
> +	"
> +done
> +
> +for i in false 0
> +do
> +	test_expect_success "commit.allowEmpty=$i and no --allow-empty" "
> +		test_must_fail git -C commit.allowEmpty=$i commit -m 'test'
> +	"
> +
> +	test_expect_success "commit.allowEmpty=$i and --allow-empty" "
> +		test_must_fail git -c commit.allowEmpty=$i commit --allow-empty -m 'test'
> +	"
> +done

Testing both 1 and "true" can be dropped here. Things that use
git_config_bool() can just assume it works, we test it more exhaustively
elsewhere.

Your patch has whitespace errors. Try with "git show --check" or apply
it with git-am, it also doesn't apply cleanly on the latest master.

But on this patch in general: I don't mind making this configurable, but
neither your commit message nor these tests make it clear what the
actual motivation is, which can be seen on the upstream GitHub bug
report.

I.e. you seemingly have no interest in using "git commit" to produce
empty commits, but are just trying to cherry-pick something and it's
failing because it (presumably, or am I missing something) cherry picks
an existing commit content ends up not changing anything.

I.e. you'd like to make the logic 37f7a85793 ("Teach commit about
CHERRY_PICK_HEAD", 2011-02-19) added a message for the default.

So let's talk about that use case, and for those of us less familiar
with this explain why it is that this needs to still be optional at
all. I.e. aren't we just exposing an implementation detail here where
cherry-pick uses the commit machinery? Should we maybe just always pass
--allow-empty on cherry-pick, if not why not?

I can think of some reasons, but the above is a hint that both this
patch + the current documentation which talks about "foreign SCM
scripts" have drifted very far from what this is actually being used
for, so let's update that.