[v3] commit: add a commit.allowEmpty config variable
diff mbox series

Message ID 20181103151205.29122-1-tanushreetumane@gmail.com
State New
Headers show
Series
  • [v3] commit: add a commit.allowEmpty config variable
Related show

Commit Message

TANUSHREE TUMANE Nov. 3, 2018, 3:12 p.m. UTC
Add commit.allowEmpty configuration variable as a convenience for those
who always prefer --allow-empty.

Add tests to check the behavior introduced by this commit.

This closes https://github.com/git-for-windows/git/issues/1854

Signed-off-by: tanushree27 <tanushreetumane@gmail.com>
---
 Documentation/config.txt     |  5 +++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             |  8 ++++++++
 t/t7500-commit.sh            | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+), 1 deletion(-)

Comments

Ævar Arnfjörð Bjarmason Nov. 3, 2018, 7:07 p.m. UTC | #1
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.
Junio C Hamano Nov. 5, 2018, 12:30 a.m. UTC | #2
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

The command line "--allowAnything" in general is meant to be an
escape hatch for unusual situations, and if a workflow requires
constant use of that escape hatch, there is something wrong either
in the workflow or in the tool used in the workflow, and it is what
we should first see if we can fix, I would think, before making it
easy to constantly use the escape hatch.

I didn't look at the external reference you looked at but it sounds
like your review comment is taking the topic in the right direction.

Thanks for digging for the backstory.

Patch
diff mbox series

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c0727b7866..f3828518a5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1467,6 +1467,11 @@  commit.verbose::
 	A boolean or int to specify the level of verbose with `git commit`.
 	See linkgit:git-commit[1].
 
+commit.allowEmpty::
+	A boolean to specify whether empty commits are allowed with `git
+	commit`. See linkgit:git-commit[1]. 
+	Defaults to false.
+
 credential.helper::
 	Specify an external helper to be called when a username or
 	password credential is needed; the helper may consult external
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f970a43422..5d3bbf017a 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -176,7 +176,8 @@  The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`.
 	Usually recording a commit that has the exact same tree as its
 	sole parent commit is a mistake, and the command prevents you
 	from making such a commit.  This option bypasses the safety, and
-	is primarily for use by foreign SCM interface scripts.
+	is primarily for use by foreign SCM interface scripts. See
+	`commit.allowEmpty` in linkgit:git-config[1].
 
 --allow-empty-message::
        Like --allow-empty this command is primarily for use by foreign
diff --git a/builtin/commit.c b/builtin/commit.c
index 67fa949204..4516309ac2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -101,6 +101,7 @@  static int all, also, interactive, patch_interactive, only, amend, signoff;
 static int edit_flag = -1; /* unspecified */
 static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
 static int config_commit_verbose = -1; /* unspecified */
+static int config_commit_allow_empty = -1; /* unspecified */
 static int no_post_rewrite, allow_empty_message;
 static char *untracked_files_arg, *force_date, *ignore_submodule_arg, *ignored_arg;
 static char *sign_commit;
@@ -1435,6 +1436,10 @@  static int git_commit_config(const char *k, const char *v, void *cb)
 		config_commit_verbose = git_config_bool_or_int(k, v, &is_bool);
 		return 0;
 	}
+	if (!strcmp(k, "commit.allowempty")) {
+		config_commit_allow_empty = git_config_bool(k, v);
+		return 0;
+	}
 
 	status = git_gpg_config(k, v, NULL);
 	if (status)
@@ -1556,6 +1561,9 @@  int cmd_commit(int argc, const char **argv, const char *prefix)
 	if (verbose == -1)
 		verbose = (config_commit_verbose < 0) ? 0 : config_commit_verbose;
 
+	if (config_commit_allow_empty >= 0)  /* if allowEmpty is allowed in config*/
+		allow_empty = config_commit_allow_empty;
+	
 	if (dry_run)
 		return dry_run_commit(argc, argv, prefix, current_head, &s);
 	index_file = prepare_index(argc, argv, prefix, current_head, 0);
diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh
index 170b4810e0..25a7facd53 100755
--- a/t/t7500-commit.sh
+++ b/t/t7500-commit.sh
@@ -359,4 +359,36 @@  test_expect_success 'new line found before status message in commit template' '
 	test_i18ncmp expected-template editor-input
 '
 
+# 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
+
 test_done