[PATCH] A test for commit --amend allowing changing of a very empty commit message

[PATCH] A test for commit --amend allowing changing of a very empty commit message

[Alex Riesen]

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Well, it fails... In current master

 t/t7501-commit.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 361886c..f81bf7b 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -345,4 +345,14 @@ test_expect_success 'amend using the message from a commit named with tag' '
 
 '
 
+test_expect_success 'allow amend of an empty message' '
+
+	git reset --hard &&
+	sha=$(:|git commit-tree HEAD^{tree} -p HEAD) &&
+	git reset --hard $sha &&
+	git commit --amend -mChanged &&
+	git cat-file commit HEAD|tail -n1|grep ^Changed$
+
+'
+
 test_done
-- 
1.5.4.2.204.g6d0ab

Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>
> Well, it fails... In current master

Then please mark it with test_expect_failure.

> +test_expect_success 'allow amend of an empty message' '
> +
> +	git reset --hard &&
> +	sha=$(:|git commit-tree HEAD^{tree} -p HEAD) &&

You do not have to pipe; </dev/null would do ;-).

Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message

[Alex Riesen]

Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> >
> > Well, it fails... In current master
> 
> Then please mark it with test_expect_failure.

That's because I expect it to be fixed (in the next mail)

> > +test_expect_success 'allow amend of an empty message' '
> > +
> > +	git reset --hard &&
> > +	sha=$(:|git commit-tree HEAD^{tree} -p HEAD) &&
> 
> You do not have to pipe; </dev/null would do ;-).

Sadly: no. There are very challenged systems. Like, for instance, ones
having no idea of what "/dev/null" is. Windows/Cygwin/ActiveState Perl
in a bloody stupid corporate combination.

I'm trying to minimize differences between my tree for my system at
work and the main tree. It is honestly the only purpose for this
construct.

[PATCH] Fix amending of a commit with an empty message

[Alex Riesen]

This change probably does even more than that: it allows amending of
anything, even of a commit which does not change anything.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Alex Riesen, Thu, Feb 21, 2008 21:35:06 +0100:
> Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> > >
> > > Well, it fails... In current master
> > 
> > Then please mark it with test_expect_failure.
> 
> That's because I expect it to be fixed (in the next mail)
> 

In this one. I am a bit unsure regarding the change (it looks a little
too strong), even though it passes the test suite.

 builtin-commit.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 065e1f7..ab3fd7b 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -520,6 +520,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
 
 		if (get_sha1(parent, sha1))
 			commitable = !!active_nr;
+		else if (amend)
+			commitable = 1;
 		else {
 			init_revisions(&rev, "");
 			rev.abbrev = 0;
-- 
1.5.4.2.204.g6d0ab

Re: [PATCH] Fix amending of a commit with an empty message

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> In this one. I am a bit unsure regarding the change (it looks a little
> too strong), even though it passes the test suite.

I think that is because your new test is faulty.

Instead of ':' I complained about, try having "echo foo" there.
I think it will still fail.

So your test failure is not about "a commit with an empty message"
at all.

Look at the previous test and how we allow an empty commit (not
"with empty message", but "with the same tree as its sole
parent") to be made and amended.

Having said that, here is a quiz.  When the user says "git
commit --amend", what are the valid reasons not to allow it,
other than:

 - the index is unmerged.
 - the branch is yet to be born.

Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message

[Johannes Schindelin]

Hi,

On Thu, 21 Feb 2008, Alex Riesen wrote:

> Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100:
> > Alex Riesen <raa.lkml@gmail.com> writes:
> > >
> > > Well, it fails... In current master
> > 
> > Then please mark it with test_expect_failure.
> 
> That's because I expect it to be fixed (in the next mail)

AFAICT the preferred procedure is to add a patch demonstrating the 
failure, with test_expect_failure, and another patch fixing it, replacing 
the _failure with _success.

Ciao,
Dscho

Re: [PATCH] Fix amending of a commit with an empty message

[Alex Riesen]

Junio C Hamano, Thu, Feb 21, 2008 21:56:52 +0100:
> Alex Riesen <raa.lkml@gmail.com> writes:
> 
> > In this one. I am a bit unsure regarding the change (it looks a little
> > too strong), even though it passes the test suite.
> 
> I think that is because your new test is faulty.
> 
> Instead of ':' I complained about, try having "echo foo" there.
> I think it will still fail.

Oh, indeed. The commit to amend does not introduce any changes.

> Having said that, here is a quiz.  When the user says "git
> commit --amend", what are the valid reasons not to allow it,
> other than:
> 
>  - the index is unmerged.
>  - the branch is yet to be born.

There are no tree changes in the commit to be amended.
And if there is a strong wish to do just the amendment,
there is always --allow-empty.

Ok, it must have been the amount of err... tee I just had.

Re: [PATCH] Fix amending of a commit with an empty message

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

>> Having said that, here is a quiz.  When the user says "git
>> commit --amend", what are the valid reasons not to allow it,
>> other than:
>> 
>>  - the index is unmerged.
>>  - the branch is yet to be born.
>
> There are no tree changes in the commit to be amended.
> And if there is a strong wish to do just the amendment,
> there is always --allow-empty.

If the original commit had some changes, but the final tree,
after hacking, experimenting and fixing, ended up in the same
shape as the parent's, the user might rather want to "reset
HEAD^" away the commit than amending now-empty commit.  Erroring
out when --allow-empty is not given (like we currently do) would
be sensible in such a case.

However, if the original commit was made with --allow-empty, it
might have been done for a reason, and I was not sure if forcing
the user to say --allow-empty while amending (like we currently
do) was such a good idea.  The user previously may have created
that empty (in content changes) commit to piss in the snow, or
perhaps the empty commit is due to foreign SCM import, and the
user may be trying to improve the log message that is attached
to it.  The user might even be an automated script in the case
of foreign SCM import, but admittedly, it could choose to always
say --allow-empty.

That was the motivation behind that quiz.