| Message ID | 20181103151205.29122-1-tanushreetumane@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] commit: add a commit.allowEmpty config variable | expand |
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.
Æ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.
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
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(-)