diff mbox series

[v4] commit: add a commit.allowEmpty config variable

Message ID 20181113155656.22975-1-tanushreetumane@gmail.com
State New, archived
Headers show
Series [v4] commit: add a commit.allowEmpty config variable | expand

Commit Message

TANUSHREE TUMANE Nov. 13, 2018, 3:56 p.m. UTC
From: tanushree27 <tanushreetumane@gmail.com>

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 <tanushreetumane@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
---
 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(-)

Comments

Johannes Schindelin Nov. 13, 2018, 7:24 p.m. UTC | #1
Hi,

On Tue, 13 Nov 2018, Tanushree Tumane wrote:

> From: tanushree27 <tanushreetumane@gmail.com>
> 
> 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 <tanushreetumane@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> ---
>  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, &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/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
> 
>
Ævar Arnfjörð Bjarmason Nov. 13, 2018, 9:27 p.m. UTC | #2
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 <tanushreetumane@gmail.com>
>>
>> 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.fsf@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 <tanushreetumane@gmail.com>
>> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
>> ---
>>  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, &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;
>> +

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 should at least say "I had such-and-such
feedback suggesting X, but decided to go for Y anyway because...".

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

Let's drop this comment. It's redundant to the test description.

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

So now you've dropped any tests of "git commit" (even though you're
changing commit.c, and just testing revert.c. So again, if that's all we
want isn't this whole thing just a simple bugfix of:

diff --git a/builtin/commit.c b/builtin/commit.c
index 96d336ec3d..1a12cc559e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1546,6 +1546,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 (whence == FROM_CHERRY_PICK)
+               allow_empty = allow_empty_message = 1;
+
        if (dry_run)
                return dry_run_commit(argc, argv, prefix, current_head, &s);
        index_file = prepare_index(argc, argv, prefix, current_head, 0);

Possibly dropping the allow_empty_message part, but it seems reasonable
that whether you're re-picking an empty commit or one with an empty
message cherry-pick should always work.

I see that fails various existing tests, and I'm going to stop digging
now, but that brings me back to the "let's explain this better" part of
the feedback. I.e. if we can't just fix it let's explain why it can't be
made a default because it breaks such-and-such, so we need the config
option.
Junio C Hamano Nov. 14, 2018, 4:16 a.m. UTC | #3
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Agreed. I'm happy to see the test for-loop gone as I noted in
> https://public-inbox.org/git/87d0rm7zeo.fsf@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.
Johannes Schindelin Nov. 14, 2018, 2:04 p.m. UTC | #4
Hi,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Agreed. I'm happy to see the test for-loop gone as I noted in
> > https://public-inbox.org/git/87d0rm7zeo.fsf@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
Johannes Schindelin Nov. 15, 2018, 8:40 a.m. UTC | #5
Hi,

On Wed, 14 Nov 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > Agreed. I'm happy to see the test for-loop gone as I noted in
> > https://public-inbox.org/git/87d0rm7zeo.fsf@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
Jeff King Nov. 15, 2018, 9:41 a.m. UTC | #6
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
Johannes Schindelin Nov. 15, 2018, 4:16 p.m. UTC | #7
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
diff mbox series

Patch

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