{"thread":{"id":"11025","subject":"[PATCH] Replace misleading message during interactive rebasing","startedAt":"2007-11-26T21:18:11Z","lastAt":"2007-11-28T12:53:59Z","messageCount":11,"participants":["Wincent Colaiuta","Johannes Sixt","Jeff King","Jakub Narebski","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"60988","messageId":"1196111891-18518-1-git-send-email-win@wincent.com","threadId":"11025","inReplyTo":null,"subject":"[PATCH] Replace misleading message during interactive rebasing","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-26T21:18:11Z","receivedAt":"2007-11-26T21:18:11Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"git-rebase--interactive uses git-cherry-pick under the covers to reorder\ncommits, which in turn means that in the event of a conflict a message\nwill be shown advising the user to commit the results and use the -c\nswitch to retain authorship after fixing the conflict.\n\nThe message is misleading because what the user really needs to do is\nrun \"git rebase --continue\"; the committing is handled by git-rebase\nand the authorship of the commit message is retained automatically.\n\nWe solve this problem by using an environment variable to communicate\nto git-cherry-pick that rebasing is underway and replace the misleading\nerror message with a more helpful one.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n\nThis applies on top of \"master\".\n\nThe question of whether the environment variable should have a leading\nunderscore came up on the mailing list. I don't really care at all either\nway, I'd just like to see the misleading message go away. I'll leave it\nup to others to decide.\n\nAnother thing to decide is whether the help text should be more than\njust \"run 'git rebase --continue'\", but should mention\n\"git rebase --abort\" as well. Junio, please feel free to modify the\npatch if you think it would be appropriate.\n\n builtin-revert.c           |    8 +++++---\n git-rebase--interactive.sh |    2 ++\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex a0586f9..5a57574 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -229,7 +229,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \tunsigned char head[20];\n \tstruct commit *base, *next, *parent;\n \tint i;\n-\tchar *oneline, *reencoded_message = NULL;\n+\tchar *oneline, *reencoded_message = NULL, *help_message;\n \tconst char *message, *encoding;\n \tconst char *defmsg = xstrdup(git_path(\"MERGE_MSG\"));\n \n@@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const char **argv)\n \t\t}\n \t\tif (close(msg_fd) || commit_lock_file(&msg_file) < 0)\n \t\t\tdie (\"Error wrapping up %s\", defmsg);\n+\t\thelp_message = getenv(\"_GIT_CHERRY_PICK_HELP\");\n \t\tfprintf(stderr, \"Automatic %s failed.  \"\n \t\t\t\"After resolving the conflicts,\\n\"\n \t\t\t\"mark the corrected paths with 'git add <paths>' \"\n-\t\t\t\"and commit the result.\\n\", me);\n-\t\tif (action == CHERRY_PICK) {\n+\t\t\t\"and %s.\\n\", me,\n+\t\t\thelp_message ? help_message : \"commit the result\");\n+\t\tif (action == CHERRY_PICK && !help_message) {\n \t\t\tfprintf(stderr, \"When commiting, use the option \"\n \t\t\t\t\"'-c %s' to retain authorship and message.\\n\",\n \t\t\t\tfind_unique_abbrev(commit->object.sha1,\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex bf44b6a..e5f9810 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -117,6 +117,7 @@ pick_one () {\n \t\tsha1=$(git rev-parse --short $sha1)\n \t\toutput warn Fast forward to $sha1\n \telse\n+\t\texport _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n \t\toutput git cherry-pick \"$@\"\n \tfi\n }\n@@ -187,6 +188,7 @@ pick_one_preserving_merges () {\n \t\t\tfi\n \t\t\t;;\n \t\t*)\n+\t\t\texport _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n \t\t\toutput git cherry-pick \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n \t\t\t;;\n-- \n1.5.3.6.952.g84ef\n"},{"id":"61033","messageId":"474BD5CA.7050407@viscovery.net","threadId":"11025","inReplyTo":"1196111891-18518-1-git-send-email-win@wincent.com","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-27T08:31:06Z","receivedAt":"2007-11-27T08:31:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Wincent Colaiuta schrieb:\n> +\t\texport _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n\nIsn't this a bashism? Should be:\n\n\t\t_GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n\t\texport _GIT_CHERRY_PICK_HELP\n\n(and the second instance of this as well, of course)\n\n-- Hannes\n"},{"id":"61036","messageId":"13E55438-D62B-4C92-892F-43773902E734@wincent.com","threadId":"11025","inReplyTo":"474BD5CA.7050407@viscovery.net","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-27T08:49:04Z","receivedAt":"2007-11-27T08:49:04Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 27/11/2007, a las 9:31, Johannes Sixt escribió:\n\n> Wincent Colaiuta schrieb:\n>> +\t\texport _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n>\n> Isn't this a bashism? Should be:\n>\n> \t\t_GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n> \t\texport _GIT_CHERRY_PICK_HELP\n>\n> (and the second instance of this as well, of course)\n\nI wondered that myself before submitting the patch, but then saw that  \nthe same pattern was used at other places as well:\n\ngit-clone.sh:\n\nW=$(cd \"$GIT_WORK_TREE\" && pwd) && export GIT_WORK_TREE=\"$W\"\n\ngit-filter-branch.sh:\n\nexport GIT_INDEX_FILE=\"$(pwd)/../index\"\nexport GIT_COMMIT=$commit\nexport GIT_COMMIT=\"$sha1\"\n\ngit-quiltimport.sh:\n\nexport GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' \"$tmp_info\")\nexport GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' \"$tmp_info\")\nexport GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' \"$tmp_info\")\nexport SUBJECT=$(sed -ne 's/Subject: //p' \"$tmp_info\")\n\nSo if this is a problem, those sites will need to be changed as well.\n\nCheers,\nWincent\n"},{"id":"61038","messageId":"20071127094639.GC3571@sigill.intra.peff.net","threadId":"11025","inReplyTo":"474BD5CA.7050407@viscovery.net","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-27T09:46:40Z","receivedAt":"2007-11-27T09:46:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 27, 2007 at 09:31:06AM +0100, Johannes Sixt wrote:\n\n> Isn't this a bashism? Should be:\n>\n> \t\t_GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n> \t\texport _GIT_CHERRY_PICK_HELP\n>\n> (and the second instance of this as well, of course)\n\nNo, it's in POSIX:\n\nhttp://www.opengroup.org/onlinepubs/009695399/utilities/export.html\n\nHowever, I'm sure you will be shocked to learn that /bin/sh on Solaris\ndoesn't understand it:\n\n$ export foo=bar\nfoo=bar: is not an identifier\n\n-Peff\n"},{"id":"61040","messageId":"figp6f$onj$1@ger.gmane.org","threadId":"11025","inReplyTo":"20071127094639.GC3571@sigill.intra.peff.net","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2007-11-27T09:49:03Z","receivedAt":"2007-11-27T09:49:03Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King wrote:\n\n> On Tue, Nov 27, 2007 at 09:31:06AM +0100, Johannes Sixt wrote:\n> \n>> Isn't this a bashism? Should be:\n>>\n>>              _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n>>              export _GIT_CHERRY_PICK_HELP\n>>\n>> (and the second instance of this as well, of course)\n> \n> No, it's in POSIX:\n> \n> http://www.opengroup.org/onlinepubs/009695399/utilities/export.html\n> \n> However, I'm sure you will be shocked to learn that /bin/sh on Solaris\n> doesn't understand it:\n> \n> $ export foo=bar\n> foo=bar: is not an identifier\n\nIf I remember correctly /bin/sh on Solaris cannot be used because\nof other issues (like $(...) and such).\n\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n"},{"id":"61039","messageId":"20071127095230.GA4337@sigill.intra.peff.net","threadId":"11025","inReplyTo":"figp6f$onj$1@ger.gmane.org","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-27T09:52:30Z","receivedAt":"2007-11-27T09:52:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 27, 2007 at 10:49:03AM +0100, Jakub Narebski wrote:\n\n> > However, I'm sure you will be shocked to learn that /bin/sh on Solaris\n> > doesn't understand it:\n> > \n> > $ export foo=bar\n> > foo=bar: is not an identifier\n> \n> If I remember correctly /bin/sh on Solaris cannot be used because\n> of other issues (like $(...) and such).\n\nYes, my response was not \"we can't use this\" (because clearly we have\nbeen for some time) but rather \"look how crappy the Solaris shell is.\nMaybe you were thinking of it?\"\n\nIOW, the construct is fine to the best of my knowledge.\n\n-Peff\n"},{"id":"61041","messageId":"20071127101520.GA5208@sigill.intra.peff.net","threadId":"11025","inReplyTo":"figp6f$onj$1@ger.gmane.org","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-11-27T10:15:20Z","receivedAt":"2007-11-27T10:15:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 27, 2007 at 10:49:03AM +0100, Jakub Narebski wrote:\n\n> If I remember correctly /bin/sh on Solaris cannot be used because\n> of other issues (like $(...) and such).\n\nBTW, your new mail/news gateway solution seems to send two messages. I\ngot one that was addressed _only_ to me, and then this one came to the\nlist (and was addressed only to the list). That means that I had to\nmanually re-add the list when responding to the first one (or just\nassume that you responded to me off-list). Personally, I think this is\nworse than the previous \"send only to the list\" behavior because senders\nare inevitably going to fail to re-add the list (but then, I was never\nas bothered by others not to be cc'd in the first place).\n\n-Peff\n"},{"id":"61165","messageId":"7vve7n9eaa.fsf@gitster.siamese.dyndns.org","threadId":"11025","inReplyTo":"474BD5CA.7050407@viscovery.net","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-28T04:37:01Z","receivedAt":"2007-11-28T04:37:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Wincent Colaiuta schrieb:\n>> +\t\texport _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n>\n> Isn't this a bashism?\n\nBeing an old-fashioned shell programmer myself, \"export VAR=VAL\" does\nraise my eyebrows.  It is in POSIX but are there shells that we do\nsupport (dash, bash, ksh -- /bin/sh on Solaris does not count) that\nwant their exports old-fashioned way?\n"},{"id":"61190","messageId":"47D3817B-AA0D-4949-99ED-C36611604D5E@wincent.com","threadId":"11025","inReplyTo":"7vve7n9eaa.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2007-11-28T08:26:55Z","receivedAt":"2007-11-28T08:26:55Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 28/11/2007, a las 5:37, Junio C Hamano escribió:\n\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n>\n>> Wincent Colaiuta schrieb:\n>>> +\t\texport _GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n>>\n>> Isn't this a bashism?\n>\n> Being an old-fashioned shell programmer myself, \"export VAR=VAL\" does\n> raise my eyebrows.  It is in POSIX but are there shells that we do\n> support (dash, bash, ksh -- /bin/sh on Solaris does not count) that\n> want their exports old-fashioned way?\n\n\nAs I noted earlier in thread, the idiom is used elsewhere in Git  \nalready. Below is a list of the instances I found where shell scripts  \nuse the idiom. There are also other instances (documentation, test  \nscripts, even a tcl file) where the idiom is used. As far as its  \nsuitability I only have directory experience with Bash, and it's  \nalways worked in all versions that I've tried in the 2.x and 3.x  \nrange. The other shells, I don't know.\n\nIn any case, below I'll paste in a revised patch that doesn't use the  \nexport FOO=... idiom. I can also submit another patch changing the  \nother usages of the idiom elsewhere in the tree, but I'd need guidance  \non how far you'd want to go (only the Git commands? also the tests?  \ndon't worry about the docs? etc).\n\nEl 27/11/2007, a las 9:49, Wincent Colaiuta escribió:\n\n> I wondered that myself before submitting the patch, but then saw  \n> that the same pattern was used at other places as well:\n>\n> git-clone.sh:\n>\n> W=$(cd \"$GIT_WORK_TREE\" && pwd) && export GIT_WORK_TREE=\"$W\"\n>\n> git-filter-branch.sh:\n>\n> export GIT_INDEX_FILE=\"$(pwd)/../index\"\n> export GIT_COMMIT=$commit\n> export GIT_COMMIT=\"$sha1\"\n>\n> git-quiltimport.sh:\n>\n> export GIT_AUTHOR_NAME=$(sed -ne 's/Author: //p' \"$tmp_info\")\n> export GIT_AUTHOR_EMAIL=$(sed -ne 's/Email: //p' \"$tmp_info\")\n> export GIT_AUTHOR_DATE=$(sed -ne 's/Date: //p' \"$tmp_info\")\n> export SUBJECT=$(sed -ne 's/Subject: //p' \"$tmp_info\")\n>\n> So if this is a problem, those sites will need to be changed as well.\n>\n> Cheers,\n> Wincent\n\nHere's the revised patch:\n--------8<---------\n From cd11e1355011796fe16d0eb7116fae4070f2a30d Mon Sep 17 00:00:00 2001\nFrom: Wincent Colaiuta <win@wincent.com>\nDate: Mon, 26 Nov 2007 13:35:46 +0100\nSubject: [PATCH] Replace misleading message during interactive rebasing\n\ngit-rebase--interactive uses git-cherry-pick under the covers to reorder\ncommits, which in turn means that in the event of a conflict a message\nwill be shown advising the user to commit the results and use the -c\nswitch to retain authorship after fixing the conflict.\n\nThe message is misleading because what the user really needs to do is\nrun \"git rebase --continue\"; the committing is handled by git-rebase\nand the authorship of the commit message is retained automatically.\n\nWe solve this problem by using an environment variable to communicate\nto git-cherry-pick that rebasing is underway and replace the misleading\nerror message with a more helpful one.\n\nSigned-off-by: Wincent Colaiuta <win@wincent.com>\n---\n  builtin-revert.c           |    8 +++++---\n  git-rebase--interactive.sh |    4 ++++\n  2 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-revert.c b/builtin-revert.c\nindex a0586f9..5a57574 100644\n--- a/builtin-revert.c\n+++ b/builtin-revert.c\n@@ -229,7 +229,7 @@ static int revert_or_cherry_pick(int argc, const  \nchar **argv)\n  \tunsigned char head[20];\n  \tstruct commit *base, *next, *parent;\n  \tint i;\n-\tchar *oneline, *reencoded_message = NULL;\n+\tchar *oneline, *reencoded_message = NULL, *help_message;\n  \tconst char *message, *encoding;\n  \tconst char *defmsg = xstrdup(git_path(\"MERGE_MSG\"));\n\n@@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const  \nchar **argv)\n  \t\t}\n  \t\tif (close(msg_fd) || commit_lock_file(&msg_file) < 0)\n  \t\t\tdie (\"Error wrapping up %s\", defmsg);\n+\t\thelp_message = getenv(\"_GIT_CHERRY_PICK_HELP\");\n  \t\tfprintf(stderr, \"Automatic %s failed.  \"\n  \t\t\t\"After resolving the conflicts,\\n\"\n  \t\t\t\"mark the corrected paths with 'git add <paths>' \"\n-\t\t\t\"and commit the result.\\n\", me);\n-\t\tif (action == CHERRY_PICK) {\n+\t\t\t\"and %s.\\n\", me,\n+\t\t\thelp_message ? help_message : \"commit the result\");\n+\t\tif (action == CHERRY_PICK && !help_message) {\n  \t\t\tfprintf(stderr, \"When commiting, use the option \"\n  \t\t\t\t\"'-c %s' to retain authorship and message.\\n\",\n  \t\t\t\tfind_unique_abbrev(commit->object.sha1,\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex bf44b6a..3552c89 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -117,6 +117,8 @@ pick_one () {\n  \t\tsha1=$(git rev-parse --short $sha1)\n  \t\toutput warn Fast forward to $sha1\n  \telse\n+\t\t_GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n+\t\texport _GIT_CHERRY_PICK_HELP &&\n  \t\toutput git cherry-pick \"$@\"\n  \tfi\n  }\n@@ -187,6 +189,8 @@ pick_one_preserving_merges () {\n  \t\t\tfi\n  \t\t\t;;\n  \t\t*)\n+\t\t\t_GIT_CHERRY_PICK_HELP=\"run 'git rebase --continue'\"\n+\t\t\texport _GIT_CHERRY_PICK_HELP &&\n  \t\t\toutput git cherry-pick \"$@\" ||\n  \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n  \t\t\t;;\n-- \n1.5.3.6.953.gdffc\n"},{"id":"61171","messageId":"Pine.LNX.4.64.0711281237250.27959@racer.site","threadId":"11025","inReplyTo":"47D3817B-AA0D-4949-99ED-C36611604D5E@wincent.com","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-28T12:41:09Z","receivedAt":"2007-11-28T12:41:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 28 Nov 2007, Wincent Colaiuta wrote:\n\n> @@ -352,11 +352,13 @@ static int revert_or_cherry_pick(int argc, const char\n> **argv)\n> \t\t}\n> \t\tif (close(msg_fd) || commit_lock_file(&msg_file) < 0)\n> \t\t\tdie (\"Error wrapping up %s\", defmsg);\n> +\t\thelp_message = getenv(\"_GIT_CHERRY_PICK_HELP\");\n> \t\tfprintf(stderr, \"Automatic %s failed.  \"\n> \t\t\t\"After resolving the conflicts,\\n\"\n> \t\t\t\"mark the corrected paths with 'git add <paths>' \"\n> -\t\t\t\"and commit the result.\\n\", me);\n> -\t\tif (action == CHERRY_PICK) {\n> +\t\t\t\"and %s.\\n\", me,\n> +\t\t\thelp_message ? help_message : \"commit the result\");\n> +\t\tif (action == CHERRY_PICK && !help_message) {\n> \t\t\tfprintf(stderr, \"When commiting, use the option \"\n> \t\t\t\t\"'-c %s' to retain authorship and message.\\n\",\n> \t\t\t\tfind_unique_abbrev(commit->object.sha1,\n\nWhat about Junio's remark that _GIT_CHERRY_PICK_HELP should rather replace \nthe _complete_ message?\n\nYou could still provide the \"me\" and unique_abbrev parameters, so that the \nfirst %s in _GIT_CHERRY_PICK_HELP would be replaced by the operation, and \nthe second by the sha1.  Hmm?\n\nCiao,\nDscho\n"},{"id":"61174","messageId":"474D64E7.9060603@viscovery.net","threadId":"11025","inReplyTo":"Pine.LNX.4.64.0711281237250.27959@racer.site","subject":"Re: [PATCH] Replace misleading message during interactive rebasing","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-28T12:53:59Z","receivedAt":"2007-11-28T12:53:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> You could still provide the \"me\" and unique_abbrev parameters, so that the \n> first %s in _GIT_CHERRY_PICK_HELP would be replaced by the operation, and \n> the second by the sha1.  Hmm?\n\nGaah! First rule to safe programming: Don't use user input as format \nstrings. getenv(_GIT_CHERRY_PICK_HELP) *is* user input. In this case, the \ncaller knows exactly what the put into the help string, so it should do so.\n\n-- Hannes\n"}]}