[PATCH] commit: make --only --allow-empty work without paths

[PATCH] commit: make --only --allow-empty work without paths

[Andreas Krey]

--only is implied when paths are present, and required
them unless --amend. But with --allow-empty it should
be allowed as well - it is the only way to create an
empty commit in the presence of staged changes.

Signed-off-by: Andreas Krey <a.krey@gmx.de>
---

I stumbled over this omission trying
to create an empty commit while changes
are staged. (We use such empty commits as
workaround when devs forgot to put issues
into the actual commits. And one had
staged changes at that point.)

Arguably, requiring paths with --only is
pointless anyway because it is implicit
in that case, but I'm happy when it works
like in this patch.

(The interdepence of the tests is a strange thing;
making --run=N somewhat pointless.)

(And I hope that the patch in commit.c is
actually sufficient for this, but have
not found indications to the contrary.)

 Documentation/git-commit.txt | 3 ++-
 builtin/commit.c             | 2 +-
 t/t7501-commit.sh            | 9 +++++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
 	If this option is specified together with `--amend`, then
 	no paths need to be specified, which can be used to amend
 	the last commit without committing changes that have
-	already been staged.
+	already been staged. If used together with `--allow-empty`
+	paths are also not required, and an empty commit will be created.
 
 -u[<mode>]::
 --untracked-files[=<mode>]::
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29..89b66816f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1206,7 +1206,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
-	if (argc == 0 && (also || (only && !amend)))
+	if (argc == 0 && (also || (only && !amend && !allow_empty)))
 		die(_("No paths with --include/--only does not make sense."));
 	if (argc == 0 && only && amend)
 		only_include_assumed = _("Clever... amending the last one with dirty index.");
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a67..0d8d89309 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents' '
 	git diff --exit-code
 '
 
+test_expect_success 'allow-empty --only ignores staged contents' '
+	echo changed-again >file &&
+	git add file &&
+	git commit --allow-empty --only -m "empty" &&
+	git cat-file blob HEAD:file >file.actual &&
+	test_cmp file.expect file.actual &&
+	git diff --exit-code
+'
+
 test_expect_success 'set up editor' '
 	cat >editor <<-\EOF &&
 	#!/bin/sh
-- 
2.11.0.10.g1e1b186

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH] commit: make --only --allow-empty work without paths

[Jeff King]

On Fri, Dec 02, 2016 at 11:15:13PM +0100, Andreas Krey wrote:

> --only is implied when paths are present, and required
> them unless --amend. But with --allow-empty it should
> be allowed as well - it is the only way to create an
> empty commit in the presence of staged changes.

OK. I'm not sure why you would want to create an empty commit in such a
case. But I do agree that this seems like a natural outcome for "--only
--allow-empty". So whether it is particularly useful or not, it seems
like the right thing to do. The patch itself looks good to me.

> Arguably, requiring paths with --only is
> pointless anyway because it is implicit
> in that case, but I'm happy when it works
> like in this patch.

I think the point is just to warn the user that what they've asked for
is by definition a noop (and that's why there's already an exception for
--amend, which _does_ make it do something). The fact that --only is
implicit with paths is mostly historical; at one point it was not the
default. These days it's unnecessary, but retained for backwards
compatibility.

> (The interdepence of the tests is a strange thing;
> making --run=N somewhat pointless.)

Yes, I think --run is a misfeature (I actually had to look it up, as I
had completely forgotten that it was added). It's too hard to know which
tests are required setup for later ones, and often the dependency is
implicit. If a single test script is annoyingly long to run, I'd argue
it should be broken out into its own script (and that will let it run in
parallel when the full suite is run, too). I don't know that t7501
qualifies, though; it runs in about 800ms on my machine.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8976c3d29..89b66816f 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1206,7 +1206,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  
>  	if (also + only + all + interactive > 1)
>  		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
> -	if (argc == 0 && (also || (only && !amend)))
> +	if (argc == 0 && (also || (only && !amend && !allow_empty)))
>  		die(_("No paths with --include/--only does not make sense."));
>  	if (argc == 0 && only && amend)
>  		only_include_assumed = _("Clever... amending the last one with dirty index.");

I think this should be sufficient. Obviously we'll end up with an empty
commit, but allow_empty should cover that case later on.

> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index d84897a67..0d8d89309 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents' '
>  	git diff --exit-code
>  '
>  
> +test_expect_success 'allow-empty --only ignores staged contents' '
> +	echo changed-again >file &&
> +	git add file &&
> +	git commit --allow-empty --only -m "empty" &&
> +	git cat-file blob HEAD:file >file.actual &&
> +	test_cmp file.expect file.actual &&
> +	git diff --exit-code
> +'
> +

Usually we'd put new tests at the end. I guess you wanted this here to
match the "--amend --only" test before it. That kind of sticks this
oddball in the middle of a bunch of --amend tests, but I'm not sure it
would go better anywhere else. So I'm fine with it here.

-Peff

Re: [PATCH] commit: make --only --allow-empty work without paths

[Andreas Krey]

On Fri, 02 Dec 2016 23:32:55 +0000, Jeff King wrote:
> On Fri, Dec 02, 2016 at 11:15:13PM +0100, Andreas Krey wrote:
> 
> > --only is implied when paths are present, and required
> > them unless --amend. But with --allow-empty it should
> > be allowed as well - it is the only way to create an
> > empty commit in the presence of staged changes.
> 
> OK. I'm not sure why you would want to create an empty commit in such a
> case.

User: Ok tool, make me a pullreq.

Tool: But you haven't mentioned any issue
      in your commit messages. Which are they?

User: Ok, that would be A-123.

Tool: git commit --allow-empty -m 'FIX: A-123'

Originally we checked that the status output was
empty, and later added an option for 'yes, I know
that there are uncommitted changes; I don't want
them included'.

And then someone had staged changes, which lead me here,
because there is no way now to create an empty commit
(just for the commit message) in that situation.
Amending the previous commit wouldn't fly with us
because of a local ban on non-fast-forward pushes.

...
> > (The interdepence of the tests is a strange thing;
> > making --run=N somewhat pointless.)
> 
> Yes, I think --run is a misfeature (I actually had to look it up, as I
...
> implicit. If a single test script is annoyingly long to run, I'd argue

It wasn't about runtime but about output. I would have
liked to see only the output of my still-failing test;
a 'stop after test X' would be helpful there.

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH] commit: make --only --allow-empty work without paths

[Jeff King]

On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:

> > OK. I'm not sure why you would want to create an empty commit in such a
> > case.
> 
> User: Ok tool, make me a pullreq.
> 
> Tool: But you haven't mentioned any issue
>       in your commit messages. Which are they?
> 
> User: Ok, that would be A-123.
> 
> Tool: git commit --allow-empty -m 'FIX: A-123'

OK. I think "tool" is slightly funny here, but I get that is part of the
real world works. Thanks for illustrating.

> > Yes, I think --run is a misfeature (I actually had to look it up, as I
> ...
> > implicit. If a single test script is annoyingly long to run, I'd argue
> 
> It wasn't about runtime but about output. I would have
> liked to see only the output of my still-failing test;
> a 'stop after test X' would be helpful there.

You can do --verbose-only=<n>, but if the test is failing, I typically
use "-v -i". That makes everything verbose, and then stops at the
failing test, so you can see the output easily.

-Peff

Re: [PATCH] commit: make --only --allow-empty work without paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:
>
>> > OK. I'm not sure why you would want to create an empty commit in such a
>> > case.
>> 
>> User: Ok tool, make me a pullreq.
>> 
>> Tool: But you haven't mentioned any issue
>>       in your commit messages. Which are they?
>> 
>> User: Ok, that would be A-123.
>> 
>> Tool: git commit --allow-empty -m 'FIX: A-123'
>
> OK. I think "tool" is slightly funny here, but I get that is part of the
> real world works. Thanks for illustrating.

I am not sure if I understand.  Why isn't the FIX: thing added to
the commit being pulled by amending it?  Would the convention be for
the responder of a pull-request to fetch and drop the tip commit?

Re: [PATCH] commit: make --only --allow-empty work without paths

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Dec 02, 2016 at 11:15:13PM +0100, Andreas Krey wrote:
>
>> --only is implied when paths are present, and required
>> them unless --amend. But with --allow-empty it should
>> be allowed as well - it is the only way to create an
>> empty commit in the presence of staged changes.
>
> OK. I'm not sure why you would want to create an empty commit in such a
> case. But I do agree that this seems like a natural outcome for "--only
> --allow-empty". So whether it is particularly useful or not, it seems
> like the right thing to do. The patch itself looks good to me.

Slightly related topic.  

>> -	if (argc == 0 && (also || (only && !amend)))
>> +	if (argc == 0 && (also || (only && !amend && !allow_empty)))
>>  		die(_("No paths with --include/--only does not make sense."));
>>  	if (argc == 0 && only && amend)
>>  		only_include_assumed = _("Clever... amending the last one with dirty index.");
>

We allow "-o --amend" without no pathspec because that is how you
would reword without changing the tree object in the tip commit, and
we reward the user who figured out such an esoteric use with a
message "Clever...".  I do not think if people who say "I want to
create an empty commit but I already have added changes to the
index" deserve the same "Clever..." praise, so I will not suggest
adding another message above.

More seriously, I suspect that the message outlived its usefulness.
If we wanted to make the "use --amend -o without pathspec if you
want to reword the tip one without touching its tree" easier to
discover, the place to do so is in the documentation, not a message
that is given as a reward to those who already discovered it.

Re: [PATCH] commit: make --only --allow-empty work without paths

[Andreas Krey]

On Mon, 05 Dec 2016 12:36:19 +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> > On Sat, Dec 03, 2016 at 07:59:49AM +0100, Andreas Krey wrote:
...
> >> Tool: git commit --allow-empty -m 'FIX: A-123'
> >
> > OK. I think "tool" is slightly funny here, but I get that is part of the
> > real world works. Thanks for illustrating.
> 
> I am not sure if I understand.  Why isn't the FIX: thing added to
> the commit being pulled by amending it?

Because we don't allow push -f on our blessed repo (bitbucket).
(Oops, answer to wrong question. But the integrators don't want
to meddle with dev's commits, either.)

This has multiple reasons:

- The percentage of people who can and would be willing
  to do rebase -i is small. (Not that they are likely to
  increase under this policy.)

- Our build tool record builds by commit id, and when
  you rebase (even if only for commit message edits)
  you lose your (simple) build history.

> Would the convention be for
> the responder of a pull-request to fetch and drop the tip commit?

No, they need to keep it as there is automation hinging on the FIX line.

I would much prefer people to do rebases/amends instead of this crutch,
but that's not for now.

Hmm, it just occurred to me that we might allow force pushes for specific
users to keep the foot-shooting ratio low.

- Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

[PATCH v2] commit: make --only --allow-empty work without paths

[Andreas Krey]

--only is implied when paths are present, and required
them unless --amend. But with --allow-empty it should
be allowed as well - it is the only way to create an
empty commit in the presence of staged changes.

Also remove the post-fact cleverness indication;
it's in the man page anyway.

Signed-off-by: Andreas Krey <a.krey@gmx.de>
---

Ok, I've removed the clever message, as Junio suggested.
I don't know what else to do to make it acceptable. :-)
We're going to deploy it internally anyway, but I think
it belongs in git.git as well (aka 'Can I has "will queue"?').

 Documentation/git-commit.txt | 3 ++-
 builtin/commit.c             | 4 +---
 t/t7501-commit.sh            | 9 +++++++++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
 	If this option is specified together with `--amend`, then
 	no paths need to be specified, which can be used to amend
 	the last commit without committing changes that have
-	already been staged.
+	already been staged. If used together with `--allow-empty`
+	paths are also not required, and an empty commit will be created.
 
 -u[<mode>]::
 --untracked-files[=<mode>]::
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29..276c74034 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1206,10 +1206,8 @@ static int parse_and_validate_options(int argc, const char *argv[],
 
 	if (also + only + all + interactive > 1)
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
-	if (argc == 0 && (also || (only && !amend)))
+	if (argc == 0 && (also || (only && !amend && !allow_empty)))
 		die(_("No paths with --include/--only does not make sense."));
-	if (argc == 0 && only && amend)
-		only_include_assumed = _("Clever... amending the last one with dirty index.");
 	if (argc > 0 && !also && !only)
 		only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a67..0d8d89309 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -155,6 +155,15 @@ test_expect_success 'amend --only ignores staged contents' '
 	git diff --exit-code
 '
 
+test_expect_success 'allow-empty --only ignores staged contents' '
+	echo changed-again >file &&
+	git add file &&
+	git commit --allow-empty --only -m "empty" &&
+	git cat-file blob HEAD:file >file.actual &&
+	test_cmp file.expect file.actual &&
+	git diff --exit-code
+'
+
 test_expect_success 'set up editor' '
 	cat >editor <<-\EOF &&
 	#!/bin/sh
-- 
2.11.0.10.g1e1b186.dirty

Re: [PATCH v2] commit: make --only --allow-empty work without paths

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> Ok, I've removed the clever message, as Junio suggested.
> I don't know what else to do to make it acceptable. :-)
> We're going to deploy it internally anyway, but I think
> it belongs in git.git as well (aka 'Can I has "will queue"?').

Oh, sorry for being unclear.  Before I started saying "Slightly
related topic.", after quoting "The patch itself looks good to me."
by Peff, I meant to say "Yeah, this looks good; thanks.", but
apparently I forgot.

Removal of "Clever" is a separate issue and it may make sense to do
so, but it deserves its own commit with its own justification.

Sorry for making you send an extra round; let's queue the original,
and if you still are interested, have the "Clever" removal as its
own patch.

Thanks.

[PATCH] commit: remove 'Clever' message for --only --amend

[Andreas Krey]

That behavior is now documented, and we don't
need a reward afterwards.

Signed-off-by: Andreas Krey <a.krey@gmx.de>
---

> Sorry for making you send an extra round; let's queue the original,
> and if you still are interested, have the "Clever" removal as its
> own patch.

Here you go.

 builtin/commit.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 89b66816f..276c74034 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1208,8 +1208,6 @@ static int parse_and_validate_options(int argc, const char *argv[],
 		die(_("Only one of --include/--only/--all/--interactive/--patch can be used."));
 	if (argc == 0 && (also || (only && !amend && !allow_empty)))
 		die(_("No paths with --include/--only does not make sense."));
-	if (argc == 0 && only && amend)
-		only_include_assumed = _("Clever... amending the last one with dirty index.");
 	if (argc > 0 && !also && !only)
 		only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
-- 
2.11.0.10.g1e1b186.dirty