{"thread":{"id":"14156","subject":"git rebase interactive: usability issue","startedAt":"2008-06-25T23:32:08Z","lastAt":"2008-06-26T13:33:58Z","messageCount":7,"participants":["Dmitry Potapov","Johannes Schindelin","Junio C Hamano","Avery Pennarun","Theodore Tso"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"81211","messageId":"20080625233208.GE5737@dpotapov.dyndns.org","threadId":"14156","inReplyTo":null,"subject":"git rebase interactive: usability issue","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-06-25T23:32:08Z","receivedAt":"2008-06-25T23:32:08Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Hello All,\n\nToday I got another user complaining that git rebase interactive\nsometimes squashing changes without being told to do that. Studying the\nreflog revealed what I expected to see: the user started the process of\nediting of chain of patches started by \"git rebase -i\", and then used\n\"git commit --amend\" to correct some of them, but at some point the\nprocess of rebasing was stopped due to a conflict caused by some previous\nchanges. The user after resolving this conflict run \"git commit --amend\"\nas he did before, without realising that this time it will squash the\ncurrent patch with the previous one.\n\nThough the user realized his mistake after my explanation of how git\nrebase works, I still believe it is a serious usability issue, because\nthe same command: \"git commit --amend\" produces drastically different\nresults depends on whether the rebase process stopped due to conflict\nor on the \"edit\" mark. Moreover, the commit message of second patch is\ngetting lost as result of using \"git commit --amend\" in the former case.\n\nPersonally, I have avoided this issue because normally I don't use git\ncommit --amend during rebasing unless I have to correct the commit\nmessage. Instead, I just edit files and then do \"git add\" on them and\nthen run \"git rebase --continue\". But latest versions of git suggest\nyou use \"git commit --amend\" during interactive rebasing and following\nthis advice, it is easy to fall into this trap.\n\nThe following patch disables \"git commit --amend\" during rebase when\nthe process was stopped due to conflict.\n\n-- >8 --\nFrom: Dmitry Potapov \nDate: Wed, 25 Jun 2008 23:23:22 +0400\nSubject: [PATCH] don't allow 'commit --amend' during rebase conflict resolution\n\nRunning 'commit --amend' during git rebase is almost certainly a mistake,\nwhich causes that two consequent patches are squashed together. Moreover,\nthe commit message of the second commit is silently lost. It is almost\ncertainly not what the user expects. In that very unlikely case when you\nreally want to combine two patches during rebase conflict resolution,\nyou can do that using \"git reset --soft HEAD^\" followed by \"git commit\".\n\nSigned-off-by: Dmitry Potapov \n---\n builtin-commit.c | 13 ++++++++++++-\n 1 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex e3ad38b..d03696f 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -925,6 +925,17 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \targc = parse_and_validate_options(argc, argv, builtin_commit_usage);\n \n+\tstrbuf_init(&sb, 0);\n+\tif (amend)\n+\t{\n+\t\tstrbuf_addf(&sb, \"%s/.dotest-merge\", get_git_dir());\n+\t\tif (!access(sb.buf, F_OK)) {\n+\t\t\tstrbuf_addstr(&sb,\"/amend\");\n+\t\t\tif (access(sb.buf, F_OK))\n+\t\t\t\tdie(\"amend not committed yet patch?\");\n+\t\t}\n+\t}\n+\n \tindex_file = prepare_index(argc, argv, prefix);\n \n \t/* Set up everything for writing the commit object. This includes\n@@ -937,7 +948,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t/*\n \t * The commit object\n \t */\n-\tstrbuf_init(&sb, 0);\n+\tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"tree %s\\n\",\n \t\t sha1_to_hex(active_cache_tree->sha1));\n \n-- \n1.5.6\n"},{"id":"81224","messageId":"alpine.DEB.1.00.0806260416410.4503@eeepc-johanness","threadId":"14156","inReplyTo":"20080625233208.GE5737@dpotapov.dyndns.org","subject":"Re: git rebase interactive: usability issue","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-26T02:17:55Z","receivedAt":"2008-06-26T02:17:55Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 26 Jun 2008, Dmitry Potapov wrote:\n\n> -- >8 --\n> From: Dmitry Potapov \n> Date: Wed, 25 Jun 2008 23:23:22 +0400\n> Subject: [PATCH] don't allow 'commit --amend' during rebase conflict resolution\n\nThis is a very funny way to interpret the SubmittingPatches document. \nJust stating a fact here.\n\n> Running 'commit --amend' during git rebase is almost certainly a \n> mistake, which causes that two consequent patches are squashed together. \n> Moreover, the commit message of the second commit is silently lost. It \n> is almost certainly not what the user expects. In that very unlikely \n> case when you really want to combine two patches during rebase conflict \n> resolution, you can do that using \"git reset --soft HEAD^\" followed by \n> \"git commit\".\n\nNACK.\n\nYou just broke the 'edit' command.\n\nCiao,\nDscho\n"},{"id":"81226","messageId":"7vbq1ovpl7.fsf@gitster.siamese.dyndns.org","threadId":"14156","inReplyTo":"alpine.DEB.1.00.0806260416410.4503@eeepc-johanness","subject":"Re: git rebase interactive: usability issue","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-26T03:32:04Z","receivedAt":"2008-06-26T03:32:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> NACK.\n>\n> You just broke the 'edit' command.\n\nReally? I thought it would be Ok for \"edit\" command.\n\nThe patch checks the presense of /amend and complains only if it does not\nexist, while you create /amend when you respond to \"edit\" insn.\n\nI was relunctant about the patch not because of \"edit\", but because I am\nnot convinced that it will _never_ make sense to be able to amend while\nthe sequence stops with a conflict (as the patch does not give us any way\nto override this rather heavy-handed denial to continue).\n\nI also was hoping that with enough hooks git-commit already calls, this\ncould have been experimented and implemented with hooks without touching C\nlayer at least initially, while people can convince themselves that the\napproach is sane (i.e. it _never_ makes sense to do amend upon conflict).\n"},{"id":"81228","messageId":"32541b130806252059l30431fcbld6196bedb32ab397@mail.gmail.com","threadId":"14156","inReplyTo":"7vbq1ovpl7.fsf@gitster.siamese.dyndns.org","subject":"Re: git rebase interactive: usability issue","fromName":"Avery Pennarun","fromEmail":"apenwarr@gmail.com","sentAt":"2008-06-26T03:59:01Z","receivedAt":"2008-06-26T03:59:01Z","isPatch":false,"sender":{"key":"apenwarr@gmail.com","avatar":"https://avatars.githubusercontent.com/u/20592?v=4"},"body":"On 6/25/08, Junio C Hamano wrote:\n> I was relunctant about the patch not because of \"edit\", but because I am\n> not convinced that it will _never_ make sense to be able to amend while\n> the sequence stops with a conflict (as the patch does not give us any way\n> to override this rather heavy-handed denial to continue).\n\nPerhaps the problem is more that people are encouraged to --amend so\noften that they end up doing it by accident.\n\nWhat if 'edit' worked more like 'squash', in that it produced the new\ntree, but didn't commit it yet? Then you can reset things, commit\nthem, or rebase --continue (which commits automatically if needed)\njust like wish 'squash'.\n\nI think --continue used to not commit automatically, so I can see why\nedit used to commit for you, but maybe that behaviour is not needed\nanymore.\n\nRight now the asymmetry of having to use --amend with 'edit' but not\nwith 'squash' is what leads me to make mistakes sometimes.\n\nHave fun,\n\nAvery\n"},{"id":"81273","messageId":"20080626113549.GF5737@dpotapov.dyndns.org","threadId":"14156","inReplyTo":"7vbq1ovpl7.fsf@gitster.siamese.dyndns.org","subject":"Re: git rebase interactive: usability issue","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-06-26T11:35:50Z","receivedAt":"2008-06-26T11:35:50Z","isPatch":false,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Hi Junio,\n\nPlease, do not apply my previous patch. I just realized that it will\nbreak the following sequence of commands when you are stopped on a\nconflicting commit during rebase:\n\n$ edit file\n$ git add file\n$ git commit\n$ git commit --amend\n\nI don't see a good solution right now. Perhaps, the better approach\nwill be to remove the suggestion of using \"git commit --amend\" and\ninstead to recommend to use \"git add\" to add your changes and then\nrun \"git rebase --continue\". This works regardless whether you stop\non the \"edit\" mark or conflict. The only problem with that is what\nif the user actually wanted to edit the commit message. Currently,\nsaying just \"git rebase --continue\" without adding anything will\nnot allow you to edit the commit message.\n\nAfter studying git-rebase script, I noticed that it always commit\nwith the --no-verify option. It makes sense for those commits that\nwere just \"pick\" but IMHO those commits that were edited by users\nprobably should be commited in the normal way, so the pre-commit\nhook can ensure that your changes are okay.\n\n\nDmitry\n"},{"id":"81282","messageId":"20080626121303.GF8610@mit.edu","threadId":"14156","inReplyTo":"20080625233208.GE5737@dpotapov.dyndns.org","subject":"Re: git rebase interactive: usability issue","fromName":"Theodore Tso","fromEmail":"tytso@mit.edu","sentAt":"2008-06-26T12:13:03Z","receivedAt":"2008-06-26T12:13:03Z","isPatch":false,"sender":{"key":"tytso@mit.edu","avatar":"https://avatars.githubusercontent.com/u/51416?v=4"},"body":"On Thu, Jun 26, 2008 at 03:32:08AM +0400, Dmitry Potapov wrote:\n> Though the user realized his mistake after my explanation of how git\n> rebase works, I still believe it is a serious usability issue, because\n> the same command: \"git commit --amend\" produces drastically different\n> results depends on whether the rebase process stopped due to conflict\n> or on the \"edit\" mark. Moreover, the commit message of second patch is\n> getting lost as result of using \"git commit --amend\" in the former case.\n\nThis is true whether it a conflict is getting addressed during a\ngit-rebase or a git-merge. The observation I would make is that git\nhas stopped a rebase or a merge with a conflict that the user needs to\nfix up, a \"git commit --amend\" is almost always the wrong thing. So I\ncould imagine a safety where after git discovers a merge conflict, it\nsets a flag (probably a file in the .git directory) which causes \"git\ncommit --amend\" stop withan error message \"this probably wasn't what\nyou wanted\", and telling the user to use a --force command if this is\nwhat they wanted. This flag would be cleared on the next \"git commit\"\nor \"git reset\".\n\nIn fact, we do this already for git-merge. Why not just do the same\nthing in the middle of a merge conflict with git-rebase?\n\n \t \t \t \t\t- Ted\n"},{"id":"81288","messageId":"20080626133358.GG5737@dpotapov.dyndns.org","threadId":"14156","inReplyTo":"20080626121303.GF8610@mit.edu","subject":"[PATCH v2] don't allow 'commit --amend' during rebase conflict resolution","fromName":"Dmitry Potapov","fromEmail":"dpotapov@gmail.com","sentAt":"2008-06-26T13:33:58Z","receivedAt":"2008-06-26T13:33:58Z","isPatch":true,"sender":{"key":"dpotapov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6568595?v=4"},"body":"Running 'commit --amend' during git rebase is almost certainly a mistake,\nwhich causes that two consequent patches are squashed together. Moreover,\nthe commit message of the second commit is silently lost. It is almost\ncertainly not what the user expects. In that very unlikely case when you\nreally want to combine two patches during rebase conflict resolution,\nyou can do that using \"git reset --soft HEAD^\" followed by \"git commit\".\n---\n\nOn Thu, Jun 26, 2008 at 08:13:03AM -0400, Theodore Tso wrote:\n> \n> In fact, we do this already for git-merge. Why not just do the same\n> thing in the middle of a merge conflict with git-rebase?\n\nThank you for suggestion. I have corrected my patch to so the same as\nwe do in the case of git-merge conflict. MERGE_MSG is already removed\non successful commit, so the patch is very simple now.\n\n builtin-commit.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex e3ad38b..6d1d955 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -725,6 +725,10 @@ static int parse_and_validate_options(int argc, const char *argv[],\n \t\tdie(\"You have nothing to amend.\");\n \tif (amend && in_merge)\n \t\tdie(\"You are in the middle of a merge -- cannot amend.\");\n+\t/* no MERGE_HEAD but MERGE_MSG means a conflict during rebase */\n+\tif (amend && !access(git_path(\"MERGE_MSG\"), F_OK))\n+\t\tdie(\"You are in the middle of a rebase conflict -- \"\n+\t\t\t\"cannot amend.\");\n \n \tif (use_message)\n \t\tf++;\n-- \n1.5.6.60.gbc566\n"}]}