{"thread":{"id":"30990","subject":"\"git commit --amend --only --\" nevertheless commits staged changes","startedAt":"2012-07-10T10:41:13Z","lastAt":"2012-07-10T20:40:29Z","messageCount":5,"participants":["Marc Strapetz","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"194864","messageId":"4FFC06C9.5050405@syntevo.com","threadId":"30990","inReplyTo":null,"subject":"\"git commit --amend --only --\" nevertheless commits staged changes","fromName":"Marc Strapetz","fromEmail":"marc.strapetz@syntevo.com","sentAt":"2012-07-10T10:41:13Z","receivedAt":"2012-07-10T10:41:13Z","isPatch":false,"sender":{"key":"marc.strapetz@syntevo.com","avatar":"https://avatars.githubusercontent.com/u/3380730?v=4"},"body":"When using \"git commit --amend --only --message <message> --\", I'd\nexpect to have just the commit message of my last commit changed,\naccording to the man page:\n\n\"--only Make a commit only from the paths specified on the command line,\ndisregarding any contents that have been staged so far. [...] If this\noption is specified together with --amend, then no paths need to be\nspecified, which can be used to amend the last commit without committing\nchanges that have already been staged.\"\n\nHowever, all staged changes are committed as well. So looks like either\nthe man page or Git is wrong here!?\n\nTested with 1.7.10.msysgit.1.\n\n-Marc\n"},{"id":"194879","messageId":"20120710201125.GA23798@sigill.intra.peff.net","threadId":"30990","inReplyTo":"4FFC06C9.5050405@syntevo.com","subject":"Re: \"git commit --amend --only --\" nevertheless commits staged changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-10T20:11:25Z","receivedAt":"2012-07-10T20:11:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2012 at 12:41:13PM +0200, Marc Strapetz wrote:\n\n> When using \"git commit --amend --only --message <message> --\", I'd\n> expect to have just the commit message of my last commit changed,\n> according to the man page:\n> \n> \"--only Make a commit only from the paths specified on the command line,\n> disregarding any contents that have been staged so far. [...] If this\n> option is specified together with --amend, then no paths need to be\n> specified, which can be used to amend the last commit without committing\n> changes that have already been staged.\"\n> \n> However, all staged changes are committed as well. So looks like either\n> the man page or Git is wrong here!?\n\nI think git has a bug. As far as I can tell, this has never worked as\nthe documentation advertised. We originally forbid the use of \"--only\"\nwithout paths as nonsensical. This was loosened by 6a74642 (git-commit\n--amend: two fixes., 2006-04-20) to let \"--amend --only --\", but I don't\nthink it even worked then.\n\nUsing this test:\n\n  git init repo &&\n  cd repo &&\n  echo \"foo one\" >foo &&\n  echo \"bar one\" >bar &&\n  git add . &&\n  git commit -m one &&\n  echo \"foo two\" >foo &&\n  echo \"bar two\" >bar &&\n  git add foo &&\n  GIT_EDITOR=true git commit --amend -o &&\n  git cat-file -p HEAD:foo &&\n  git cat-file -p HEAD:bar\n\nI always get:\n\n  foo two\n  bar one\n\ni.e., we accidentally amend the commit with the staged contents in the\nindex. I get the same results for 6a74642 and on. If you switch the\ncommit to \"-o bar\", it does work properly (you get the updated \"bar\",\nbut the staged \"foo\" in the index is ignored).\n\n-Peff\n"},{"id":"194880","messageId":"7vwr2bgyqf.fsf@alter.siamese.dyndns.org","threadId":"30990","inReplyTo":"4FFC06C9.5050405@syntevo.com","subject":"Re: \"git commit --amend --only --\" nevertheless commits staged changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-10T20:14:32Z","receivedAt":"2012-07-10T20:14:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marc Strapetz <marc.strapetz@syntevo.com> writes:\n\n> When using \"git commit --amend --only --message <message> --\", I'd\n> expect to have just the commit message of my last commit changed,\n> according to the man page:\n>\n> \"--only Make a commit only from the paths specified on the command line,\n> disregarding any contents that have been staged so far. [...] If this\n> option is specified together with --amend, then no paths need to be\n> specified, which can be used to amend the last commit without committing\n> changes that have already been staged.\"\n>\n> However, all staged changes are committed as well. So looks like either\n> the man page or Git is wrong here!?\n\nI do not think the combination with --amend, --only and no paths\never worked.  We rejected such a combination before 6a74642c5, which\nmerely made us to accept the combination but I do not think the\ncommit did anything to re-read the tree from the HEAD being amended\nto the index.\n\nSomething like this, but I haven't thought about what other things\nit may break.\n\n builtin/commit.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f43eaaf..59ef5e1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -184,6 +184,9 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \tint i;\n \tchar *m;\n \n+\tif (!pattern)\n+\t\treturn 0;\n+\n \tfor (i = 0; pattern[i]; i++)\n \t\t;\n \tm = xcalloc(1, i);\n@@ -345,7 +348,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t * and create commit from the_index.\n \t * We still need to refresh the index here.\n \t */\n-\tif (!pathspec || !*pathspec) {\n+\tif (!(only && amend) && (!pathspec || !*pathspec)) {\n \t\tfd = hold_locked_index(&index_lock, 1);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tif (active_cache_changed) {\n"},{"id":"194881","messageId":"20120710203052.GB23798@sigill.intra.peff.net","threadId":"30990","inReplyTo":"7vwr2bgyqf.fsf@alter.siamese.dyndns.org","subject":"Re: \"git commit --amend --only --\" nevertheless commits staged changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-10T20:30:52Z","receivedAt":"2012-07-10T20:30:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2012 at 01:14:32PM -0700, Junio C Hamano wrote:\n\n> I do not think the combination with --amend, --only and no paths\n> ever worked.  We rejected such a combination before 6a74642c5, which\n> merely made us to accept the combination but I do not think the\n> commit did anything to re-read the tree from the HEAD being amended\n> to the index.\n> \n> Something like this, but I haven't thought about what other things\n> it may break.\n\nOur emails just crossed. I came to the exact same conclusion, and just\nwrote almost the exact same patch.\n\n> -\tif (!pathspec || !*pathspec) {\n> +\tif (!(only && amend) && (!pathspec || !*pathspec)) {\n\nIt is sufficient to check only \"only\" in the first part of your\nconditional, as we disallow empty pathspecs with \"-o\". And even if we\ndidn't disallow it, this would do the right thing by trying to create a\npartial commit with no changes (which would fail, of course, but is the\nonly sane thing for the prepare_index function to do; certainly doing an\nas-is commit is simply wrong).\n\n-Peff\n"},{"id":"194882","messageId":"20120710204028.GC23798@sigill.intra.peff.net","threadId":"30990","inReplyTo":"20120710203052.GB23798@sigill.intra.peff.net","subject":"Re: \"git commit --amend --only --\" nevertheless commits staged changes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-07-10T20:40:29Z","receivedAt":"2012-07-10T20:40:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 10, 2012 at 04:30:52PM -0400, Jeff King wrote:\n\n> On Tue, Jul 10, 2012 at 01:14:32PM -0700, Junio C Hamano wrote:\n> \n> > I do not think the combination with --amend, --only and no paths\n> > ever worked.  We rejected such a combination before 6a74642c5, which\n> > merely made us to accept the combination but I do not think the\n> > commit did anything to re-read the tree from the HEAD being amended\n> > to the index.\n> > \n> > Something like this, but I haven't thought about what other things\n> > it may break.\n> \n> Our emails just crossed. I came to the exact same conclusion, and just\n> wrote almost the exact same patch.\n\nHere it is with a test and commit message. I believe this fix could also\nmake:\n\n  git commit --allow-empty --only --\n\nwork if we removed the \"--only does not make sense without paths\" check.\nBut I seriously doubt that anybody cares, given that \"--only\" is the\ndefault (i.e., just omitting it already does what you want there,\nwhether you have pathspecs or not).\n\n-- >8 --\nSubject: [PATCH] commit: fix \"--amend --only\" with no pathspec\n\nWhen we do not have any pathspec, we typically disallow an\nexplicit \"--only\", because it makes no sense (your commit\nwould, by definition, be empty). But since 6a74642\n(git-commit --amend: two fixes., 2006-04-20), we have\nallowed \"--amend --only\" with the intent that it would amend\nthe commit, ignoring any contents staged in the index.\n\nHowever, while that commit allowed the combination, we never\nactually implemented the logic to make it work. The current\ncode notices that we have no pathspec and assumes we want to\ndo an as-is commit (i.e., the \"--only\" is ignored).\n\nInstead, we must make sure to follow the partial-commit\ncode-path. We also need to tweak the list_paths function to\nhandle a NULL pathspec.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c  |  5 ++++-\n t/t7501-commit.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex f43eaaf..3c3385c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -184,6 +184,9 @@ static int list_paths(struct string_list *list, const char *with_tree,\n \tint i;\n \tchar *m;\n \n+\tif (!pattern)\n+\t\treturn 0;\n+\n \tfor (i = 0; pattern[i]; i++)\n \t\t;\n \tm = xcalloc(1, i);\n@@ -345,7 +348,7 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t * and create commit from the_index.\n \t * We still need to refresh the index here.\n \t */\n-\tif (!pathspec || !*pathspec) {\n+\tif (!only && (!pathspec || !*pathspec)) {\n \t\tfd = hold_locked_index(&index_lock, 1);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tif (active_cache_changed) {\ndiff --git a/t/t7501-commit.sh b/t/t7501-commit.sh\nindex b20ca0e..9f8d423 100755\n--- a/t/t7501-commit.sh\n+++ b/t/t7501-commit.sh\n@@ -108,6 +108,16 @@ test_expect_success 'amend commit' '\n \tEDITOR=./editor git commit --amend\n '\n \n+test_expect_success 'amend --only ignores staged contents' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tcp file file.expect &&\n+\techo changed >file &&\n+\tgit add file &&\n+\tgit commit --no-edit --amend --only &&\n+\tgit cat-file blob HEAD:file >file.actual &&\n+\ttest_cmp file.expect file.actual\n+'\n+\n test_expect_success 'set up editor' '\n \tcat >editor <<-\\EOF &&\n \t#!/bin/sh\n-- \n1.7.11.35.gbaf554e.dirty\n"}]}