{"thread":{"id":"12246","subject":"[PATCH] A test for commit --amend allowing changing of a very empty commit message","startedAt":"2008-02-21T19:54:38Z","lastAt":"2008-02-21T22:01:44Z","messageCount":8,"participants":["Alex Riesen","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"69504","messageId":"20080221195438.GA6973@steel.home","threadId":"12246","inReplyTo":null,"subject":"[PATCH] A test for commit --amend allowing changing of a very empty commit message","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-02-21T19:54:38Z","receivedAt":"2008-02-21T19:54:38Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Signed-off-by: Alex Riesen \n---\n\nWell, it fails... In current master\n\n t/t7501-commit.sh | 10 ++++++++++\n 1 files changed, 10 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex 361886c..f81bf7b 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -345,4 +345,14 @@ test_expect_success 'amend using the message from a commit named with tag' '\n \n '\n \n+test_expect_success 'allow amend of an empty message' '\n+\n+\tgit reset --hard &&\n+\tsha=$(:|git commit-tree HEAD^{tree} -p HEAD) &&\n+\tgit reset --hard $sha &&\n+\tgit commit --amend -mChanged &&\n+\tgit cat-file commit HEAD|tail -n1|grep ^Changed$\n+\n+'\n+\n test_done\n-- \n1.5.4.2.204.g6d0ab\n"},{"id":"69508","messageId":"7vwsoyt5xt.fsf@gitster.siamese.dyndns.org","threadId":"12246","inReplyTo":"20080221195438.GA6973@steel.home","subject":"Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-21T20:23:10Z","receivedAt":"2008-02-21T20:23:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> Signed-off-by: Alex Riesen \n> ---\n>\n> Well, it fails... In current master\n\nThen please mark it with test_expect_failure.\n\n> +test_expect_success 'allow amend of an empty message' '\n> +\n> +\tgit reset --hard &&\n> +\tsha=$(:|git commit-tree HEAD^{tree} -p HEAD) &&\n\nYou do not have to pipe; Alex Riesen writes:\n> >\n> > Well, it fails... In current master\n> \n> Then please mark it with test_expect_failure.\n\nThat's because I expect it to be fixed (in the next mail)\n\n> > +test_expect_success 'allow amend of an empty message' '\n> > +\n> > +\tgit reset --hard &&\n> > +\tsha=$(:|git commit-tree HEAD^{tree} -p HEAD) &&\n> \n> You do not have to pipe; \n---\nAlex Riesen, Thu, Feb 21, 2008 21:35:06 +0100:\n> Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100:\n> > Alex Riesen writes:\n> > >\n> > > Well, it fails... In current master\n> > \n> > Then please mark it with test_expect_failure.\n> \n> That's because I expect it to be fixed (in the next mail)\n> \n\nIn this one. I am a bit unsure regarding the change (it looks a little\ntoo strong), even though it passes the test suite.\n\n builtin-commit.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 065e1f7..ab3fd7b 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -520,6 +520,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix)\n \n \t\tif (get_sha1(parent, sha1))\n \t\t\tcommitable = !!active_nr;\n+\t\telse if (amend)\n+\t\t\tcommitable = 1;\n \t\telse {\n \t\t\tinit_revisions(&rev, \"\");\n \t\t\trev.abbrev = 0;\n-- \n1.5.4.2.204.g6d0ab\n"},{"id":"69513","messageId":"7vmyput4dn.fsf@gitster.siamese.dyndns.org","threadId":"12246","inReplyTo":"20080221203802.GB20143@steel.home","subject":"Re: [PATCH] Fix amending of a commit with an empty message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-21T20:56:52Z","receivedAt":"2008-02-21T20:56:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n> In this one. I am a bit unsure regarding the change (it looks a little\n> too strong), even though it passes the test suite.\n\nI think that is because your new test is faulty.\n\nInstead of ':' I complained about, try having \"echo foo\" there.\nI think it will still fail.\n\nSo your test failure is not about \"a commit with an empty message\"\nat all.\n\nLook at the previous test and how we allow an empty commit (not\n\"with empty message\", but \"with the same tree as its sole\nparent\") to be made and amended.\n\nHaving said that, here is a quiz. When the user says \"git\ncommit --amend\", what are the valid reasons not to allow it,\nother than:\n\n - the index is unmerged.\n - the branch is yet to be born.\n"},{"id":"69514","messageId":"alpine.LSU.1.00.0802212056320.17164@racer.site","threadId":"12246","inReplyTo":"20080221203506.GA20143@steel.home","subject":"Re: [PATCH] A test for commit --amend allowing changing of a very empty commit message","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-02-21T20:57:15Z","receivedAt":"2008-02-21T20:57:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 21 Feb 2008, Alex Riesen wrote:\n\n> Junio C Hamano, Thu, Feb 21, 2008 21:23:10 +0100:\n> > Alex Riesen writes:\n> > >\n> > > Well, it fails... In current master\n> > \n> > Then please mark it with test_expect_failure.\n> \n> That's because I expect it to be fixed (in the next mail)\n\nAFAICT the preferred procedure is to add a patch demonstrating the \nfailure, with test_expect_failure, and another patch fixing it, replacing \nthe _failure with _success.\n\nCiao,\nDscho\n"},{"id":"69518","messageId":"20080221213204.GC20143@steel.home","threadId":"12246","inReplyTo":"7vmyput4dn.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Fix amending of a commit with an empty message","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-02-21T21:32:04Z","receivedAt":"2008-02-21T21:32:04Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Junio C Hamano, Thu, Feb 21, 2008 21:56:52 +0100:\n> Alex Riesen writes:\n> \n> > In this one. I am a bit unsure regarding the change (it looks a little\n> > too strong), even though it passes the test suite.\n> \n> I think that is because your new test is faulty.\n> \n> Instead of ':' I complained about, try having \"echo foo\" there.\n> I think it will still fail.\n\nOh, indeed. The commit to amend does not introduce any changes.\n\n> Having said that, here is a quiz. When the user says \"git\n> commit --amend\", what are the valid reasons not to allow it,\n> other than:\n> \n> - the index is unmerged.\n> - the branch is yet to be born.\n\nThere are no tree changes in the commit to be amended.\nAnd if there is a strong wish to do just the amendment,\nthere is always --allow-empty.\n\nOk, it must have been the amount of err... tee I just had.\n"},{"id":"69525","messageId":"7v8x1et1dj.fsf@gitster.siamese.dyndns.org","threadId":"12246","inReplyTo":"20080221213204.GC20143@steel.home","subject":"Re: [PATCH] Fix amending of a commit with an empty message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-21T22:01:44Z","receivedAt":"2008-02-21T22:01:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Riesen writes:\n\n>> Having said that, here is a quiz. When the user says \"git\n>> commit --amend\", what are the valid reasons not to allow it,\n>> other than:\n>> \n>> - the index is unmerged.\n>> - the branch is yet to be born.\n>\n> There are no tree changes in the commit to be amended.\n> And if there is a strong wish to do just the amendment,\n> there is always --allow-empty.\n\nIf the original commit had some changes, but the final tree,\nafter hacking, experimenting and fixing, ended up in the same\nshape as the parent's, the user might rather want to \"reset\nHEAD^\" away the commit than amending now-empty commit. Erroring\nout when --allow-empty is not given (like we currently do) would\nbe sensible in such a case.\n\nHowever, if the original commit was made with --allow-empty, it\nmight have been done for a reason, and I was not sure if forcing\nthe user to say --allow-empty while amending (like we currently\ndo) was such a good idea. The user previously may have created\nthat empty (in content changes) commit to piss in the snow, or\nperhaps the empty commit is due to foreign SCM import, and the\nuser may be trying to improve the log message that is attached\nto it. The user might even be an automated script in the case\nof foreign SCM import, but admittedly, it could choose to always\nsay --allow-empty.\n\nThat was the motivation behind that quiz.\n"}]}