threads / patch / 12246

patchA test for commit --amend allowing changing of a very empty commit message

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

## tl;dr

8 messages between Feb 21, 2008 and Feb 21, 2008. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Alex Riesen· Feb 21, 2008, 19:54 UTC · lore
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(-)
Show changes to t/t7501-commit.sh +10 −0
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
Junio C Hamano· Feb 21, 2008, 20:23 UTC · re: Alex Riesen · lore

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

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 ;-).
Alex Riesen· Feb 21, 2008, 20:35 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100:
Show 5 quoted lines
> 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)
Show 6 quoted lines
> > +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.

Alex Riesen· Feb 21, 2008, 20:38 UTC · re: Alex Riesen · lore

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

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:
Show 9 quoted lines
> 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(-)
Show changes to builtin-commit.c +2 −0
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
Junio C Hamano· Feb 21, 2008, 20:56 UTC · re: Alex Riesen · lore

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

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.
Alex Riesen· Feb 21, 2008, 21:32 UTC · re: Junio C Hamano · lore

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

Junio C Hamano, Thu, Feb 21, 2008 21:56:52 +0100:
Show 9 quoted lines
> 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.
Show 6 quoted lines
> 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.
Junio C Hamano· Feb 21, 2008, 22:01 UTC · re: Alex Riesen · lore

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

Alex Riesen <raa.lkml@gmail.com> writes:
Show 10 quoted lines
>> 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.
Johannes Schindelin· Feb 21, 2008, 20:57 UTC · re: Alex Riesen · lore

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

Hi,
On Thu, 21 Feb 2008, Alex Riesen wrote:
Show 8 quoted lines
> 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

← back to recent threads