Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > > > From @chucklu: > > > > > my user case is like this : > > > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > > are merge commits. Then I will get lots of popup like: > > > > > >The previous cherry-pick is now empty, possibly due to conflict > > >resolution. > > >If you wish to commit it anyway, use: > > > > > >git commit --allow-empty > > > > > >If you wish to skip this commit, use: > > > > > >git reset > > > > > >Then "git cherry-pick --continue" will resume cherry-picking > > >the remaining commits. > > > > My quick interpretation of this is that the user actually needs a way to > > skip silently commits which are now empty. > > If it's always intended to be used with cherry-pick, shouldn't > cherry-pick learn a --keep-empty (like rebase has)? That would avoid > even stopping for this case in the first place. I'd go for the other way round: --skip-empty. However, given the very unhappy turn in that Git for Windows ticket (somebody asks for a feature, then just sits back, and does not even confirm that the analysis covers their use case, let alone participates in this discussion), I am personally not really interested in driving this one any further. Tanushree proved that they know how to contribute to the Git mailing list, as a pre-requisite for the Outreachy project, and that is the positive outcome of this thread as far as I am concerned. I am pretty happy about that, too. Ciao, Dscho
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
On Thu, Nov 15, 2018 at 09:40:38AM +0100, Johannes Schindelin wrote: > From @chucklu: > > > my user case is like this : > > > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > > are merge commits. Then I will get lots of popup like: > > > >The previous cherry-pick is now empty, possibly due to conflict > >resolution. > >If you wish to commit it anyway, use: > > > >git commit --allow-empty > > > >If you wish to skip this commit, use: > > > >git reset > > > >Then "git cherry-pick --continue" will resume cherry-picking > >the remaining commits. > > My quick interpretation of this is that the user actually needs a way to > skip silently commits which are now empty. If it's always intended to be used with cherry-pick, shouldn't cherry-pick learn a --keep-empty (like rebase has)? That would avoid even stopping for this case in the first place. -Peff
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi, On Wed, 14 Nov 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Agreed. I'm happy to see the test for-loop gone as I noted in > > https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as > > noted in that v3 feedback the whole "why would anyone want this?" > > explanation is still missing, and this still smells like a workaround > > for a bug we should be fixing elsewhere in the sequencing code. > > Thanks. I share the same impression that this is sweeping a bug > under a wrong rug. I asked for clarification at https://github.com/git-for-windows/git/issues/1854 and in my best imitation of Lt Tawney Madison, I report back: >From @chucklu: > my user case is like this : > > When I want to cherr-pick commits from A to G (ABCDEFG), image C and E > are merge commits. Then I will get lots of popup like: > >The previous cherry-pick is now empty, possibly due to conflict >resolution. >If you wish to commit it anyway, use: > >git commit --allow-empty > >If you wish to skip this commit, use: > >git reset > >Then "git cherry-pick --continue" will resume cherry-picking >the remaining commits. My quick interpretation of this is that the user actually needs a way to skip silently commits which are now empty. Ciao, Dscho
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi, On Wed, 14 Nov 2018, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > Agreed. I'm happy to see the test for-loop gone as I noted in > > https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as > > noted in that v3 feedback the whole "why would anyone want this?" > > explanation is still missing, and this still smells like a workaround > > for a bug we should be fixing elsewhere in the sequencing code. > > Thanks. I share the same impression that this is sweeping a bug > under a wrong rug. I agree that the scenario is under-explained. Of course, I have to say that this is not Tanushree's problem; They only copied what is in https://github.com/git-for-windows/git/issues/1854 and @chucklu did not grace us with an explanation, either. Based on historical context, I would wager a bet that the scenario is that some commits that may or may not have been in a different SCM originally and that may or may not have been empty and/or squashed in `master` need to be cherry-picked. But I agree that this should be clarified. I prodded the original wish-haver. Ciao, Dscho
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Ævar Arnfjörð Bjarmason writes: > Agreed. I'm happy to see the test for-loop gone as I noted in > https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as > noted in that v3 feedback the whole "why would anyone want this?" > explanation is still missing, and this still smells like a workaround > for a bug we should be fixing elsewhere in the sequencing code. Thanks. I share the same impression that this is sweeping a bug under a wrong rug.
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
On Tue, Nov 13 2018, Johannes Schindelin wrote: [Comments on the v4 patch also inline, found it easier to reply just to this one] > On Tue, 13 Nov 2018, Tanushree Tumane wrote: > >> From: tanushree27 >> >> when we cherrypick an existing commit it doesn't change anything and >> therefore it fails prompting to reset (skip commit) or commit using >> --allow-empty attribute and then continue. > > This is a nice paragraph, but it might make sense to connect it to the > commit's oneline somehow. I, for one, was surprised to see the oneline > talk about `git commit` and the commit message about `git cherry-pick`. > > I could imagine that an introductory paragraph, talking about why one > might want to commit empty commits, might be the best lead into the > subject, and the paragraph about `cherry-pick` could follow (and be > introduced by saying something along the lines that this config setting > has more reach than just `git commit`; it also affects `git cherry-pick`)? Agreed. I'm happy to see the test for-loop gone as I noted in https://public-inbox.org/git/87d0rm7zeo@evledraar.gmail.com/ but as noted in that v3 feedback the whole "why would anyone want this?" explanation is still missing, and this still smells like a workaround for a bug we should be fixing elsewhere in the sequencing code. [The rest of this for Tanushree] >> >> Add commit.allowEmpty configuration variable as a convenience to skip >> this process. >> >> 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 >> Signed-off-by: Tanushree Tumane >> --- >> Documentation/config.txt | 5 + >> Documentation/git-commit.txt | 3 ++- >> builtin/commit.c | 8 >> t/t3500-cherry.sh| 10 ++ >> 4 files changed, 25 insertions(+), 1 deletion(-) >> >> 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, _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; >> + I had two comments on this hunk in my v3 feedback. It's fine to go for something different (although I still think you should change it), but for others following along you
Re: [PATCH v4] commit: add a commit.allowEmpty config variable
Hi, On Tue, 13 Nov 2018, Tanushree Tumane wrote: > From: tanushree27 > > when we cherrypick an existing commit it doesn't change anything and > therefore it fails prompting to reset (skip commit) or commit using > --allow-empty attribute and then continue. This is a nice paragraph, but it might make sense to connect it to the commit's oneline somehow. I, for one, was surprised to see the oneline talk about `git commit` and the commit message about `git cherry-pick`. I could imagine that an introductory paragraph, talking about why one might want to commit empty commits, might be the best lead into the subject, and the paragraph about `cherry-pick` could follow (and be introduced by saying something along the lines that this config setting has more reach than just `git commit`; it also affects `git cherry-pick`)? Ciao, Johannes > > Add commit.allowEmpty configuration variable as a convenience to skip > this process. > > 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 > Signed-off-by: Tanushree Tumane > --- > Documentation/config.txt | 5 + > Documentation/git-commit.txt | 3 ++- > builtin/commit.c | 8 > t/t3500-cherry.sh| 10 ++ > 4 files changed, 25 insertions(+), 1 deletion(-) > > 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, _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, ); > index_file = prepare_index(argc, argv, prefix, current_head, 0); > diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh > index f038f34b7c..11504e2d9f 100755 > --- a/t/t3500-cherry.sh > +++ b/t/t3500-cherry.sh > @@ -55,4 +55,14 @@ test_expect_success \ > expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*" > ' > > + > +# Tests for commit.allowEmpty config > + > +test_expect_success 'cherry-pick existing commit with commit.allowEmpty' ' > +test_tick && > + test_commit "first" && > + test_commit "second" && > + git -c commit.allowEmpty=true cherry-pick HEAD~1 > +' > + >
[PATCH v4] commit: add a commit.allowEmpty config variable
From: tanushree27 when we cherrypick an existing commit it doesn't change anything and therefore it fails prompting to reset (skip commit) or commit using --allow-empty attribute and then continue. Add commit.allowEmpty configuration variable as a convenience to skip this process. 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 Signed-off-by: Tanushree Tumane --- Documentation/config.txt | 5 + Documentation/git-commit.txt | 3 ++- builtin/commit.c | 8 t/t3500-cherry.sh| 10 ++ 4 files changed, 25 insertions(+), 1 deletion(-) 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, _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, ); index_file = prepare_index(argc, argv, prefix, current_head, 0); diff --git a/t/t3500-cherry.sh b/t/t3500-cherry.sh index f038f34b7c..11504e2d9f 100755 --- a/t/t3500-cherry.sh +++ b/t/t3500-cherry.sh @@ -55,4 +55,14 @@ test_expect_success \ expr "$(echo $(git cherry master my-topic-branch) )" : "+ [^ ]* - .*" ' + +# Tests for commit.allowEmpty config + +test_expect_success 'cherry-pick existing commit with commit.allowEmpty' ' +test_tick && + test_commit "first" && + test_commit "second" && + git -c commit.allowEmpty=true cherry-pick HEAD~1 +' + test_done -- 2.19.1.windows.1.495.g7e9d1c442b.dirty