# [PATCH] A test for commit --amend allowing changing of a very empty commit message 8 messages from 2008-02-21 to 2008-02-21. Participants: Alex Riesen, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/12246 ## Alex Riesen, 2008-02-21 19:54 Subject: [PATCH] A test for commit --amend allowing changing of a very empty commit message Message-ID: <20080221195438.GA6973@steel.home> URL: https://gitlist.dev/e/20080221195438.GA6973%40steel.home ``` Signed-off-by: Alex Riesen --- 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 ``` ## Junio C Hamano, 2008-02-21 20:23 Subject: Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message Message-ID: <7vwsoyt5xt.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwsoyt5xt.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080221195438.GA6973@steel.home> ``` Alex Riesen writes: > Signed-off-by: Alex Riesen > --- > > 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; URL: https://gitlist.dev/e/20080221203506.GA20143%40steel.home In-Reply-To: <7vwsoyt5xt.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100: > Alex Riesen 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; URL: https://gitlist.dev/e/20080221203802.GB20143%40steel.home In-Reply-To: <20080221203506.GA20143@steel.home> ``` 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 --- Alex Riesen, Thu, Feb 21, 2008 21:35:06 +0100: > Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100: > > Alex Riesen 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 ``` ## Junio C Hamano, 2008-02-21 20:56 Subject: Re: [PATCH] Fix amending of a commit with an empty message Message-ID: <7vmyput4dn.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmyput4dn.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080221203802.GB20143@steel.home> ``` Alex Riesen 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. ``` ## Johannes Schindelin, 2008-02-21 20:57 Subject: Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802212056320.17164%40racer.site In-Reply-To: <20080221203506.GA20143@steel.home> ``` Hi, On Thu, 21 Feb 2008, Alex Riesen wrote: > Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100: > > Alex Riesen 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 ``` ## Alex Riesen, 2008-02-21 21:32 Subject: Re: [PATCH] Fix amending of a commit with an empty message Message-ID: <20080221213204.GC20143@steel.home> URL: https://gitlist.dev/e/20080221213204.GC20143%40steel.home In-Reply-To: <7vmyput4dn.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano, Thu, Feb 21, 2008 21:56:52 +0100: > Alex Riesen 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. ``` ## Junio C Hamano, 2008-02-21 22:01 Subject: Re: [PATCH] Fix amending of a commit with an empty message Message-ID: <7v8x1et1dj.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x1et1dj.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080221213204.GC20143@steel.home> ``` Alex Riesen 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. ```