{"thread":{"id":"36389","subject":"[PATCH 0/3] Fix support for FreeBSD's /bin/sh","startedAt":"2014-04-11T08:28:16Z","lastAt":"2014-04-18T00:26:00Z","messageCount":21,"participants":["Kyle J. McKay","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"238672","messageId":"438458da797bcab97449bfa931a9d1d@74d39fa044aa309eaea14b9f57fe79c","threadId":"36389","inReplyTo":null,"subject":"[PATCH 0/3] Fix support for FreeBSD's /bin/sh","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T08:28:16Z","receivedAt":"2014-04-11T08:28:16Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"This patch series words around the FreeBSD /bin/sh problems that cause rebase\nto fail on FreeBSD as well as test t5560-http-backend-noserver.\n\nThe rebase issue was first introduced in Git v1.8.4 which started using the\n\"return\" statement to return from a \"dot\" command script where the \"dot\"\ncommand itself was located inside a function. This behaves badly when using\nFreeBSD's /bin/sh. An attempt was made to fix this in Git v1.8.4.1, but it\nonly addressed part of the problem.\n\nPatch 1/3 fixes the problem by eliminating such use of \"return\", patch 2/3\nthen reverts the earlier workaround since it is no longer needed and did\nnot fully solve the problem.\n\nThe t5560 issue was first introduced in Git v1.7.0 which started using a\nbackslash escapes in ${...} expansions. The FreeBSD /bin/sh does not\nproperly support such escapes, but there is a simple change that works\neverywhere (using [?] instead of \\?).\n\nThere are more details in the individual patches.\n\nThis patch series is based on maint since these are bug fixes and that's\nwhat SubmittingPatches says to do...\n\nKyle J. McKay (3):\n rebase: avoid non-function use of \"return\" on FreeBSD\n Revert \"rebase: fix run_specific_rebase's use of \"return\" on FreeBSD\"\n test: fix t5560 on FreeBSD\n\n git-rebase--am.sh | 131 +++++++--------\n git-rebase--interactive.sh | 341 ++++++++++++++++++++-------------------\n git-rebase--merge.sh | 87 +++++-----\n git-rebase.sh | 11 +-\n t/t5560-http-backend-noserver.sh | 4 +-\n 5 files changed, 287 insertions(+), 287 deletions(-)\n\nThe above stat may seem like a lot, but when using diff -w --stat you get this:\n\n git-rebase--am.sh | 3 +++\n git-rebase--interactive.sh | 3 +++\n git-rebase--merge.sh | 3 +++\n git-rebase.sh | 11 +----------\n t/t5560-http-backend-noserver.sh | 4 ++--\n 5 files changed, 12 insertions(+), 12 deletions(-)\n\nwhich more accurately reflects what was actually touched.\n"},{"id":"238673","messageId":"0779303f7d2257a618b5bed00260a8a@74d39fa044aa309eaea14b9f57fe79c","threadId":"36389","inReplyTo":"438458da797bcab97449bfa931a9d1d@74d39fa044aa309eaea14b9f57fe79c","subject":"[PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T08:28:17Z","receivedAt":"2014-04-11T08:28:17Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) the\ngit-rebase--*.sh scripts have used a \"return\" to return from the \"dot\"\ncommand that runs them. While this is allowed by POSIX, the FreeBSD\n/bin/sh utility behaves poorly under some circumstances when such a\n\"return\" is executed.\n\nIn particular, if the \"dot\" command is contained within a function,\nthen when a \"return\" is executed by the script it runs (that is not\nitself inside a function), control will return from the function\nthat contains the \"dot\" command skipping any statements that might\nfollow the dot command inside that function. Commit 99855ddf (first\nappearing in v1.8.4.1) addresses this by making the \"dot\" command\nthe last line in the function. Unfortunately the FreeBSD /bin/sh\nmay also execute some statements in the script run by the \"dot\"\ncommand that appear after the troublesome \"return\". The fix in\n99855ddf does not address this problem.\n\nFor example, if you have script1.sh with these contents:\n\n#!/bin/sh\n# script1.sh\nrun_script2() {\n . \"$(dirname -- \"$0\")/script2.sh\"\n _e=$?\n echo only this line should show\n [ $_e -eq 5 ] || echo expected status 5 got $_e\n return 3\n}\nrun_script2\ne=$?\n[ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; }\n\nAnd script2.sh with these contents:\n\n# script2.sh\nif [ 5 -gt 3 ]; then\n return 5\nfi\ncase bad in *)\n echo always shows\nesac\necho should not get here\n! :\n\nWhen running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh'\nafter making it executable), the expected output from a POSIX shell\nis simply the single line:\n\nonly this line should show\n\nHowever, when run using FreeBSD's /bin/sh, the following output\nappears instead:\n\nshould not get here\nexpected status 3 got 1\n\nNot only did the lines following the \"dot\" command in the run_script2\nfunction in script1.sh get skipped, but additional lines in script2.sh\nfollowing the \"return\" got executed -- but not all of them (e.g. the\n\"echo always shows\" line did not run).\n\nThese issues can be avoided by not using a top-level \"return\" in\nscript2.sh. If script2.sh is changed to this:\n\n# script2.sh fixed\nmain() {\n if [ 5 -gt 3 ]; then\n return 5\n fi\n case bad in *)\n echo always shows\n esac\n echo should not get here\n ! :\n}\nmain\n\nThen it behaves the same when using FreeBSD's /bin/sh as when using\nother more POSIX compliant /bin/sh implementations.\n\nWe fix the git-rebase--*.sh scripts in a similar fashion by moving\nthe top-level code that contains \"return\" statements into its own\nfunction and then calling that as the last line in the script.\n\nThe changes introduced by this commit are best viewed with the\n--ignore-all-space (-w) diff option.\n\nSigned-off-by: Kyle J. McKay \n\n---\n git-rebase--am.sh | 117 ++++++++--------\n git-rebase--interactive.sh | 335 +++++++++++++++++++++++----------------------\n git-rebase--merge.sh | 87 ++++++------\n 3 files changed, 274 insertions(+), 265 deletions(-)\n\n\nFor convenience, here are the diffs using -w:\n\n|--- a/git-rebase--am.sh\n|+++ b/git-rebase--am.sh\n|@@ -4,6 +4,7 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n+git_rebase__am() {\n \tcase \"$action\" in\n \tcontinue)\n \t\tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n|@@ -73,3 +74,5 @@ then\n \tfi\n \n \tmove_to_original_branch\n+}\n+git_rebase__am\n\n|--- a/git-rebase--interactive.sh\n|+++ b/git-rebase--interactive.sh\n|@@ -810,6 +810,7 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n+git_rebase__interactive() {\n \tcase \"$action\" in\n \tcontinue)\n \t\t# do we have anything to commit?\n|@@ -1042,3 +1043,5 @@ GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n \toutput git checkout $onto || die_abort \"could not detach HEAD\"\n \tgit update-ref ORIG_HEAD $orig_head\n \tdo_rest\n+}\n+git_rebase__interactive\n\n|--- a/git-rebase--merge.sh\n|+++ b/git-rebase--merge.sh\n|@@ -101,6 +101,7 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n+git_rebase__merge() {\n \tcase \"$action\" in\n \tcontinue)\n \t\tread_state\n|@@ -151,3 +152,5 @@ do\n \tdone\n \n \tfinish_rb_merge\n+}\n+git_rebase__merge\n\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex a4f683a5..2d3f6d55 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,72 +4,75 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n-case \"$action\" in\n-continue)\n-\tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n-\tmove_to_original_branch\n-\treturn\n-\t;;\n-skip)\n-\tgit am --skip --resolvemsg=\"$resolvemsg\" &&\n-\tmove_to_original_branch\n-\treturn\n-\t;;\n-esac\n-\n-test -n \"$rebase_root\" && root_flag=--root\n-\n-ret=0\n-if test -n \"$keep_empty\"\n-then\n-\t# we have to do this the hard way. git format-patch completely squashes\n-\t# empty commits and even if it didn't the format doesn't really lend\n-\t# itself well to recording empty patches. fortunately, cherry-pick\n-\t# makes this easy\n-\tgit cherry-pick --allow-empty \"$revisions\"\n-\tret=$?\n-else\n-\trm -f \"$GIT_DIR/rebased-patches\"\n+git_rebase__am() {\n+\tcase \"$action\" in\n+\tcontinue)\n+\t\tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n+\t\tmove_to_original_branch\n+\t\treturn\n+\t\t;;\n+\tskip)\n+\t\tgit am --skip --resolvemsg=\"$resolvemsg\" &&\n+\t\tmove_to_original_branch\n+\t\treturn\n+\t\t;;\n+\tesac\n \n-\tgit format-patch -k --stdout --full-index --ignore-if-in-upstream \\\n-\t\t--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n-\t\t$root_flag \"$revisions\" >\"$GIT_DIR/rebased-patches\"\n-\tret=$?\n+\ttest -n \"$rebase_root\" && root_flag=--root\n \n-\tif test 0 != $ret\n+\tret=0\n+\tif test -n \"$keep_empty\"\n \tthen\n+\t\t# we have to do this the hard way. git format-patch completely squashes\n+\t\t# empty commits and even if it didn't the format doesn't really lend\n+\t\t# itself well to recording empty patches. fortunately, cherry-pick\n+\t\t# makes this easy\n+\t\tgit cherry-pick --allow-empty \"$revisions\"\n+\t\tret=$?\n+\telse\n \t\trm -f \"$GIT_DIR/rebased-patches\"\n-\t\tcase \"$head_name\" in\n-\t\trefs/heads/*)\n-\t\t\tgit checkout -q \"$head_name\"\n-\t\t\t;;\n-\t\t*)\n-\t\t\tgit checkout -q \"$orig_head\"\n-\t\t\t;;\n-\t\tesac\n \n-\t\tcat >&2 <<-EOF\n+\t\tgit format-patch -k --stdout --full-index --ignore-if-in-upstream \\\n+\t\t\t--src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \\\n+\t\t\t$root_flag \"$revisions\" >\"$GIT_DIR/rebased-patches\"\n+\t\tret=$?\n \n-\t\tgit encountered an error while preparing the patches to replay\n-\t\tthese revisions:\n+\t\tif test 0 != $ret\n+\t\tthen\n+\t\t\trm -f \"$GIT_DIR/rebased-patches\"\n+\t\t\tcase \"$head_name\" in\n+\t\t\trefs/heads/*)\n+\t\t\t\tgit checkout -q \"$head_name\"\n+\t\t\t\t;;\n+\t\t\t*)\n+\t\t\t\tgit checkout -q \"$orig_head\"\n+\t\t\t\t;;\n+\t\t\tesac\n \n-\t\t $revisions\n+\t\t\tcat >&2 <<-EOF\n \n-\t\tAs a result, git cannot rebase them.\n-\t\tEOF\n-\t\treturn $?\n-\tfi\n+\t\t\tgit encountered an error while preparing the patches to replay\n+\t\t\tthese revisions:\n+\n+\t\t\t $revisions\n \n-\tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" <\"$GIT_DIR/rebased-patches\"\n-\tret=$?\n+\t\t\tAs a result, git cannot rebase them.\n+\t\t\tEOF\n+\t\t\treturn $?\n+\t\tfi\n \n-\trm -f \"$GIT_DIR/rebased-patches\"\n-fi\n+\t\tgit am $git_am_opt --rebasing --resolvemsg=\"$resolvemsg\" <\"$GIT_DIR/rebased-patches\"\n+\t\tret=$?\n \n-if test 0 != $ret\n-then\n-\ttest -d \"$state_dir\" && write_basic_state\n-\treturn $ret\n-fi\n+\t\trm -f \"$GIT_DIR/rebased-patches\"\n+\tfi\n \n-move_to_original_branch\n+\tif test 0 != $ret\n+\tthen\n+\t\ttest -d \"$state_dir\" && write_basic_state\n+\t\treturn $ret\n+\tfi\n+\n+\tmove_to_original_branch\n+}\n+git_rebase__am\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 43631b47..42164f11 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -810,16 +810,17 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n-case \"$action\" in\n-continue)\n-\t# do we have anything to commit?\n-\tif git diff-index --cached --quiet HEAD --\n-\tthen\n-\t\t: Nothing to commit -- skip this\n-\telse\n-\t\tif ! test -f \"$author_script\"\n+git_rebase__interactive() {\n+\tcase \"$action\" in\n+\tcontinue)\n+\t\t# do we have anything to commit?\n+\t\tif git diff-index --cached --quiet HEAD --\n \t\tthen\n-\t\t\tdie \"You have staged changes in your working tree. If these changes are meant to be\n+\t\t\t: Nothing to commit -- skip this\n+\t\telse\n+\t\t\tif ! test -f \"$author_script\"\n+\t\t\tthen\n+\t\t\t\tdie \"You have staged changes in your working tree. If these changes are meant to be\n squashed into the previous commit, run:\n \n git commit --amend\n@@ -832,42 +833,42 @@ In both case, once you're done, continue with:\n \n git rebase --continue\n \"\n-\t\tfi\n-\t\t. \"$author_script\" ||\n-\t\t\tdie \"Error trying to find the author identity to amend commit\"\n-\t\tif test -f \"$amend\"\n-\t\tthen\n-\t\t\tcurrent_head=$(git rev-parse --verify HEAD)\n-\t\t\ttest \"$current_head\" = $(cat \"$amend\") ||\n-\t\t\tdie \"\\\n+\t\t\tfi\n+\t\t\t. \"$author_script\" ||\n+\t\t\t\tdie \"Error trying to find the author identity to amend commit\"\n+\t\t\tif test -f \"$amend\"\n+\t\t\tthen\n+\t\t\t\tcurrent_head=$(git rev-parse --verify HEAD)\n+\t\t\t\ttest \"$current_head\" = $(cat \"$amend\") ||\n+\t\t\t\tdie \"\\\n You have uncommitted changes in your working tree. Please, commit them\n first and then run 'git rebase --continue' again.\"\n-\t\t\tdo_with_author git commit --amend --no-verify -F \"$msg\" -e ||\n-\t\t\t\tdie \"Could not commit staged changes.\"\n-\t\telse\n-\t\t\tdo_with_author git commit --no-verify -F \"$msg\" -e ||\n-\t\t\t\tdie \"Could not commit staged changes.\"\n+\t\t\t\tdo_with_author git commit --amend --no-verify -F \"$msg\" -e ||\n+\t\t\t\t\tdie \"Could not commit staged changes.\"\n+\t\t\telse\n+\t\t\t\tdo_with_author git commit --no-verify -F \"$msg\" -e ||\n+\t\t\t\t\tdie \"Could not commit staged changes.\"\n+\t\t\tfi\n \t\tfi\n-\tfi\n \n-\trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n+\t\trecord_in_rewritten \"$(cat \"$state_dir\"/stopped-sha)\"\n \n-\trequire_clean_work_tree \"rebase\"\n-\tdo_rest\n-\treturn 0\n-\t;;\n-skip)\n-\tgit rerere clear\n+\t\trequire_clean_work_tree \"rebase\"\n+\t\tdo_rest\n+\t\treturn 0\n+\t\t;;\n+\tskip)\n+\t\tgit rerere clear\n \n-\tdo_rest\n-\treturn 0\n-\t;;\n-edit-todo)\n-\tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n-\tmv -f \"$todo\".new \"$todo\"\n-\tcollapse_todo_ids\n-\tappend_todo_help\n-\tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n+\t\tdo_rest\n+\t\treturn 0\n+\t\t;;\n+\tedit-todo)\n+\t\tgit stripspace --strip-comments <\"$todo\" >\"$todo\".new\n+\t\tmv -f \"$todo\".new \"$todo\"\n+\t\tcollapse_todo_ids\n+\t\tappend_todo_help\n+\t\tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n \n You are editing the todo file of an ongoing interactive rebase.\n To continue rebase after editing, run:\n@@ -875,170 +876,172 @@ To continue rebase after editing, run:\n \n EOF\n \n-\tgit_sequence_editor \"$todo\" ||\n-\t\tdie \"Could not execute editor\"\n-\texpand_todo_ids\n+\t\tgit_sequence_editor \"$todo\" ||\n+\t\t\tdie \"Could not execute editor\"\n+\t\texpand_todo_ids\n \n-\texit\n-\t;;\n-esac\n+\t\texit\n+\t\t;;\n+\tesac\n \n-git var GIT_COMMITTER_IDENT >/dev/null ||\n-\tdie \"You need to set your committer info first\"\n+\tgit var GIT_COMMITTER_IDENT >/dev/null ||\n+\t\tdie \"You need to set your committer info first\"\n \n-comment_for_reflog start\n+\tcomment_for_reflog start\n \n-if test ! -z \"$switch_to\"\n-then\n-\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n-\toutput git checkout \"$switch_to\" -- ||\n-\tdie \"Could not checkout $switch_to\"\n+\tif test ! -z \"$switch_to\"\n+\tthen\n+\t\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $switch_to\"\n+\t\toutput git checkout \"$switch_to\" -- ||\n+\t\tdie \"Could not checkout $switch_to\"\n \n-\tcomment_for_reflog start\n-fi\n+\t\tcomment_for_reflog start\n+\tfi\n \n-orig_head=$(git rev-parse --verify HEAD) || die \"No HEAD?\"\n-mkdir -p \"$state_dir\" || die \"Could not create temporary $state_dir\"\n+\torig_head=$(git rev-parse --verify HEAD) || die \"No HEAD?\"\n+\tmkdir -p \"$state_dir\" || die \"Could not create temporary $state_dir\"\n \n-: > \"$state_dir\"/interactive || die \"Could not mark as interactive\"\n-write_basic_state\n-if test t = \"$preserve_merges\"\n-then\n-\tif test -z \"$rebase_root\"\n+\t: > \"$state_dir\"/interactive || die \"Could not mark as interactive\"\n+\twrite_basic_state\n+\tif test t = \"$preserve_merges\"\n \tthen\n-\t\tmkdir \"$rewritten\" &&\n-\t\tfor c in $(git merge-base --all $orig_head $upstream)\n-\t\tdo\n-\t\t\techo $onto > \"$rewritten\"/$c ||\n+\t\tif test -z \"$rebase_root\"\n+\t\tthen\n+\t\t\tmkdir \"$rewritten\" &&\n+\t\t\tfor c in $(git merge-base --all $orig_head $upstream)\n+\t\t\tdo\n+\t\t\t\techo $onto > \"$rewritten\"/$c ||\n+\t\t\t\t\tdie \"Could not init rewritten commits\"\n+\t\t\tdone\n+\t\telse\n+\t\t\tmkdir \"$rewritten\" &&\n+\t\t\techo $onto > \"$rewritten\"/root ||\n \t\t\t\tdie \"Could not init rewritten commits\"\n-\t\tdone\n+\t\tfi\n+\t\t# No cherry-pick because our first pass is to determine\n+\t\t# parents to rewrite and skipping dropped commits would\n+\t\t# prematurely end our probe\n+\t\tmerges_option=\n \telse\n-\t\tmkdir \"$rewritten\" &&\n-\t\techo $onto > \"$rewritten\"/root ||\n-\t\t\tdie \"Could not init rewritten commits\"\n+\t\tmerges_option=\"--no-merges --cherry-pick\"\n \tfi\n-\t# No cherry-pick because our first pass is to determine\n-\t# parents to rewrite and skipping dropped commits would\n-\t# prematurely end our probe\n-\tmerges_option=\n-else\n-\tmerges_option=\"--no-merges --cherry-pick\"\n-fi\n \n-shorthead=$(git rev-parse --short $orig_head)\n-shortonto=$(git rev-parse --short $onto)\n-if test -z \"$rebase_root\"\n-\t# this is now equivalent to ! -z \"$upstream\"\n-then\n-\tshortupstream=$(git rev-parse --short $upstream)\n-\trevisions=$upstream...$orig_head\n-\tshortrevisions=$shortupstream..$shorthead\n-else\n-\trevisions=$onto...$orig_head\n-\tshortrevisions=$shorthead\n-fi\n-git rev-list $merges_option --pretty=oneline --abbrev-commit \\\n-\t--abbrev=7 --reverse --left-right --topo-order \\\n-\t$revisions | \\\n-\tsed -n \"s/^>//p\" |\n-while read -r shortsha1 rest\n-do\n-\n-\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1\n+\tshorthead=$(git rev-parse --short $orig_head)\n+\tshortonto=$(git rev-parse --short $onto)\n+\tif test -z \"$rebase_root\"\n+\t\t# this is now equivalent to ! -z \"$upstream\"\n \tthen\n-\t\tcomment_out=\"$comment_char \"\n+\t\tshortupstream=$(git rev-parse --short $upstream)\n+\t\trevisions=$upstream...$orig_head\n+\t\tshortrevisions=$shortupstream..$shorthead\n \telse\n-\t\tcomment_out=\n+\t\trevisions=$onto...$orig_head\n+\t\tshortrevisions=$shorthead\n \tfi\n+\tgit rev-list $merges_option --pretty=oneline --abbrev-commit \\\n+\t\t--abbrev=7 --reverse --left-right --topo-order \\\n+\t\t$revisions | \\\n+\t\tsed -n \"s/^>//p\" |\n+\twhile read -r shortsha1 rest\n+\tdo\n \n-\tif test t != \"$preserve_merges\"\n-\tthen\n-\t\tprintf '%s\\n' \"${comment_out}pick $shortsha1 $rest\" >>\"$todo\"\n-\telse\n-\t\tsha1=$(git rev-parse $shortsha1)\n-\t\tif test -z \"$rebase_root\"\n+\t\tif test -z \"$keep_empty\" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1\n \t\tthen\n-\t\t\tpreserve=t\n-\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n-\t\t\tdo\n-\t\t\t\tif test -f \"$rewritten\"/$p\n-\t\t\t\tthen\n-\t\t\t\t\tpreserve=f\n-\t\t\t\tfi\n-\t\t\tdone\n+\t\t\tcomment_out=\"$comment_char \"\n \t\telse\n-\t\t\tpreserve=f\n-\t\tfi\n-\t\tif test f = \"$preserve\"\n-\t\tthen\n-\t\t\ttouch \"$rewritten\"/$sha1\n-\t\t\tprintf '%s\\n' \"${comment_out}pick $shortsha1 $rest\" >>\"$todo\"\n+\t\t\tcomment_out=\n \t\tfi\n-\tfi\n-done\n \n-# Watch for commits that been dropped by --cherry-pick\n-if test t = \"$preserve_merges\"\n-then\n-\tmkdir \"$dropped\"\n-\t# Save all non-cherry-picked changes\n-\tgit rev-list $revisions --left-right --cherry-pick | \\\n-\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n-\t# Now all commits and note which ones are missing in\n-\t# not-cherry-picks and hence being dropped\n-\tgit rev-list $revisions |\n-\twhile read rev\n-\tdo\n-\t\tif test -f \"$rewritten\"/$rev -a \"$(sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks)\" = \"\"\n+\t\tif test t != \"$preserve_merges\"\n \t\tthen\n-\t\t\t# Use -f2 because if rev-list is telling us this commit is\n-\t\t\t# not worthwhile, we don't want to track its multiple heads,\n-\t\t\t# just the history of its first-parent for others that will\n-\t\t\t# be rebasing on top of it\n-\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n-\t\t\tshort=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)\n-\t\t\tsane_grep -v \"^[a-z][a-z]* $short\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n-\t\t\trm \"$rewritten\"/$rev\n+\t\t\tprintf '%s\\n' \"${comment_out}pick $shortsha1 $rest\" >>\"$todo\"\n+\t\telse\n+\t\t\tsha1=$(git rev-parse $shortsha1)\n+\t\t\tif test -z \"$rebase_root\"\n+\t\t\tthen\n+\t\t\t\tpreserve=t\n+\t\t\t\tfor p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-)\n+\t\t\t\tdo\n+\t\t\t\t\tif test -f \"$rewritten\"/$p\n+\t\t\t\t\tthen\n+\t\t\t\t\t\tpreserve=f\n+\t\t\t\t\tfi\n+\t\t\t\tdone\n+\t\t\telse\n+\t\t\t\tpreserve=f\n+\t\t\tfi\n+\t\t\tif test f = \"$preserve\"\n+\t\t\tthen\n+\t\t\t\ttouch \"$rewritten\"/$sha1\n+\t\t\t\tprintf '%s\\n' \"${comment_out}pick $shortsha1 $rest\" >>\"$todo\"\n+\t\t\tfi\n \t\tfi\n \tdone\n-fi\n \n-test -s \"$todo\" || echo noop >> \"$todo\"\n-test -n \"$autosquash\" && rearrange_squash \"$todo\"\n-test -n \"$cmd\" && add_exec_commands \"$todo\"\n+\t# Watch for commits that been dropped by --cherry-pick\n+\tif test t = \"$preserve_merges\"\n+\tthen\n+\t\tmkdir \"$dropped\"\n+\t\t# Save all non-cherry-picked changes\n+\t\tgit rev-list $revisions --left-right --cherry-pick | \\\n+\t\t\tsed -n \"s/^>//p\" > \"$state_dir\"/not-cherry-picks\n+\t\t# Now all commits and note which ones are missing in\n+\t\t# not-cherry-picks and hence being dropped\n+\t\tgit rev-list $revisions |\n+\t\twhile read rev\n+\t\tdo\n+\t\t\tif test -f \"$rewritten\"/$rev -a \"$(sane_grep \"$rev\" \"$state_dir\"/not-cherry-picks)\" = \"\"\n+\t\t\tthen\n+\t\t\t\t# Use -f2 because if rev-list is telling us this commit is\n+\t\t\t\t# not worthwhile, we don't want to track its multiple heads,\n+\t\t\t\t# just the history of its first-parent for others that will\n+\t\t\t\t# be rebasing on top of it\n+\t\t\t\tgit rev-list --parents -1 $rev | cut -d' ' -s -f2 > \"$dropped\"/$rev\n+\t\t\t\tshort=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)\n+\t\t\t\tsane_grep -v \"^[a-z][a-z]* $short\" <\"$todo\" > \"${todo}2\" ; mv \"${todo}2\" \"$todo\"\n+\t\t\t\trm \"$rewritten\"/$rev\n+\t\t\tfi\n+\t\tdone\n+\tfi\n+\n+\ttest -s \"$todo\" || echo noop >> \"$todo\"\n+\ttest -n \"$autosquash\" && rearrange_squash \"$todo\"\n+\ttest -n \"$cmd\" && add_exec_commands \"$todo\"\n \n-cat >>\"$todo\" <>\"$todo\" <>\"$todo\" <<\\EOF\n+\tappend_todo_help\n+\tgit stripspace --comment-lines >>\"$todo\" <<\\EOF\n \n However, if you remove everything, the rebase will be aborted.\n \n EOF\n \n-if test -z \"$keep_empty\"\n-then\n-\tprintf '%s\\n' \"$comment_char Note that empty commits are commented out\" >>\"$todo\"\n-fi\n+\tif test -z \"$keep_empty\"\n+\tthen\n+\t\tprintf '%s\\n' \"$comment_char Note that empty commits are commented out\" >>\"$todo\"\n+\tfi\n \n \n-has_action \"$todo\" ||\n-\tdie_abort \"Nothing to do\"\n+\thas_action \"$todo\" ||\n+\t\tdie_abort \"Nothing to do\"\n \n-cp \"$todo\" \"$todo\".backup\n-git_sequence_editor \"$todo\" ||\n-\tdie_abort \"Could not execute editor\"\n+\tcp \"$todo\" \"$todo\".backup\n+\tgit_sequence_editor \"$todo\" ||\n+\t\tdie_abort \"Could not execute editor\"\n \n-has_action \"$todo\" ||\n-\tdie_abort \"Nothing to do\"\n+\thas_action \"$todo\" ||\n+\t\tdie_abort \"Nothing to do\"\n \n-expand_todo_ids\n+\texpand_todo_ids\n \n-test -d \"$rewritten\" || test -n \"$force_rebase\" || skip_unnecessary_picks\n+\ttest -d \"$rewritten\" || test -n \"$force_rebase\" || skip_unnecessary_picks\n \n-GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n-output git checkout $onto || die_abort \"could not detach HEAD\"\n-git update-ref ORIG_HEAD $orig_head\n-do_rest\n+\tGIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n+\toutput git checkout $onto || die_abort \"could not detach HEAD\"\n+\tgit update-ref ORIG_HEAD $orig_head\n+\tdo_rest\n+}\n+git_rebase__interactive\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex e7d96de9..b5f05bf5 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -101,53 +101,56 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n-case \"$action\" in\n-continue)\n-\tread_state\n-\tcontinue_merge\n-\twhile test \"$msgnum\" -le \"$end\"\n-\tdo\n-\t\tcall_merge \"$msgnum\"\n+git_rebase__merge() {\n+\tcase \"$action\" in\n+\tcontinue)\n+\t\tread_state\n \t\tcontinue_merge\n+\t\twhile test \"$msgnum\" -le \"$end\"\n+\t\tdo\n+\t\t\tcall_merge \"$msgnum\"\n+\t\t\tcontinue_merge\n+\t\tdone\n+\t\tfinish_rb_merge\n+\t\treturn\n+\t\t;;\n+\tskip)\n+\t\tread_state\n+\t\tgit rerere clear\n+\t\tmsgnum=$(($msgnum + 1))\n+\t\twhile test \"$msgnum\" -le \"$end\"\n+\t\tdo\n+\t\t\tcall_merge \"$msgnum\"\n+\t\t\tcontinue_merge\n+\t\tdone\n+\t\tfinish_rb_merge\n+\t\treturn\n+\t\t;;\n+\tesac\n+\n+\tmkdir -p \"$state_dir\"\n+\techo \"$onto_name\" > \"$state_dir/onto_name\"\n+\twrite_basic_state\n+\n+\tmsgnum=0\n+\tfor cmt in `git rev-list --reverse --no-merges \"$revisions\"`\n+\tdo\n+\t\tmsgnum=$(($msgnum + 1))\n+\t\techo \"$cmt\" > \"$state_dir/cmt.$msgnum\"\n \tdone\n-\tfinish_rb_merge\n-\treturn\n-\t;;\n-skip)\n-\tread_state\n-\tgit rerere clear\n-\tmsgnum=$(($msgnum + 1))\n+\n+\techo 1 >\"$state_dir/msgnum\"\n+\techo $msgnum >\"$state_dir/end\"\n+\n+\tend=$msgnum\n+\tmsgnum=1\n+\n \twhile test \"$msgnum\" -le \"$end\"\n \tdo\n \t\tcall_merge \"$msgnum\"\n \t\tcontinue_merge\n \tdone\n-\tfinish_rb_merge\n-\treturn\n-\t;;\n-esac\n-\n-mkdir -p \"$state_dir\"\n-echo \"$onto_name\" > \"$state_dir/onto_name\"\n-write_basic_state\n-\n-msgnum=0\n-for cmt in `git rev-list --reverse --no-merges \"$revisions\"`\n-do\n-\tmsgnum=$(($msgnum + 1))\n-\techo \"$cmt\" > \"$state_dir/cmt.$msgnum\"\n-done\n-\n-echo 1 >\"$state_dir/msgnum\"\n-echo $msgnum >\"$state_dir/end\"\n-\n-end=$msgnum\n-msgnum=1\n \n-while test \"$msgnum\" -le \"$end\"\n-do\n-\tcall_merge \"$msgnum\"\n-\tcontinue_merge\n-done\n-\n-finish_rb_merge\n+\tfinish_rb_merge\n+}\n+git_rebase__merge\n-- \ntg: (0bc85abb..) t/freebsd-sh-return (depends on: maint)\n"},{"id":"238675","messageId":"1cc6896f60ac6495f949d9921e659f2@74d39fa044aa309eaea14b9f57fe79c","threadId":"36389","inReplyTo":"438458da797bcab97449bfa931a9d1d@74d39fa044aa309eaea14b9f57fe79c","subject":"[PATCH 2/3] Revert \"rebase: fix run_specific_rebase's use of \"return\" on FreeBSD\"","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T08:28:18Z","receivedAt":"2014-04-11T08:28:18Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"This reverts commit 99855ddf4bd319cd06a0524e755ab1c1b7d39f3b.\n\nThe workaround 99855ddf introduced to deal with problematic\n\"return\" statements in scripts run by \"dot\" commands located\ninside functions only handles one part of the problem. The\nissue has now been addressed by not using \"return\" statements\nin this way in the git-rebase--*.sh scripts.\n\nThis workaround is therefore no longer necessary, so clean\nup the code by reverting it.\n\nConflicts:\n git-rebase.sh\n\nSigned-off-by: Kyle J. McKay \n\n---\n git-rebase.sh | 11 +----------\n 1 file changed, 1 insertion(+), 10 deletions(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 8a3efa29..07e2bd48 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -169,22 +169,13 @@ You can run \"git stash pop\" or \"git stash drop\" at any time.\n \trm -rf \"$state_dir\"\n }\n \n-run_specific_rebase_internal () {\n+run_specific_rebase () {\n \tif [ \"$interactive_rebase\" = implied ]; then\n \t\tGIT_EDITOR=:\n \t\texport GIT_EDITOR\n \t\tautosquash=\n \tfi\n-\t# On FreeBSD, the shell's \"return\" returns from the current\n-\t# function, not from the current file inclusion.\n-\t# run_specific_rebase_internal has the file inclusion as a\n-\t# last statement, so POSIX and FreeBSD's return will do the\n-\t# same thing.\n \t. git-rebase--$type\n-}\n-\n-run_specific_rebase () {\n-\trun_specific_rebase_internal\n \tret=$?\n \tif test $ret -eq 0\n \tthen\n-- \ntg: (1bb252a9..) t/revert-99855ddf (depends on: t/freebsd-sh-return)\n"},{"id":"238674","messageId":"0101ef71fafab90325bb799cb8ad80e@74d39fa044aa309eaea14b9f57fe79c","threadId":"36389","inReplyTo":"438458da797bcab97449bfa931a9d1d@74d39fa044aa309eaea14b9f57fe79c","subject":"[PATCH 3/3] test: fix t5560 on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T08:28:19Z","receivedAt":"2014-04-11T08:28:19Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since fd0a8c2e (first appearing in v1.7.0), the\nt/t5560-http-backend-noserver.sh test has used a backslash escape\ninside a ${} expansion in order to specify a literal '?' character.\n\nUnfortunately the FreeBSD /bin/sh does not interpret this correctly.\n\nIn a POSIX compliant shell, the following:\n\nx='one?two?three'\necho \"${x#*\\?}\"\n\nWould be expected to produce this:\n\ntwo?three\n\nWhen using the FreeBSD /bin/sh instead you get this:\n\none?two?three\n\nIn fact the FreeBSD /bin/sh treats the backslash as a literal\ncharacter to match so that this:\n\ny='one\\two\\three'\necho \"${y#*\\?}\"\n\nProduces this unexpected value:\n\nwo\\three\n\nIn this case the backslash is not only treated literally, it also\nfails to defeat the special meaning of the '?' character.\n\nInstead, we can use the [...] construct to defeat the special meaning\nof the '?' character and match it exactly in a way that works for the\nFreeBSD /bin/sh as well as other POSIX /bin/sh implementations.\n\nChanging the example like so:\n\nx='one?two?three'\necho \"${x#*[?]}\"\n\nProduces the expected output using the FreeBSD /bin/sh.\n\nTherefore, change the use of \\? to [?] in order to be compatible with\nthe FreeBSD /bin/sh which allows t/t5560-http-backend-noserver.sh to\npass on FreeBSD again.\n\nSigned-off-by: Kyle J. McKay \n\n---\n t/t5560-http-backend-noserver.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh\nindex 9be9ae34..5abd11a5 100755\n--- a/t/t5560-http-backend-noserver.sh\n+++ b/t/t5560-http-backend-noserver.sh\n@@ -9,8 +9,8 @@ test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U\n \n run_backend() {\n \techo \"$2\" |\n-\tQUERY_STRING=\"${1#*\\?}\" \\\n-\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%\\?*}\" \\\n+\tQUERY_STRING=\"${1#*[?]}\" \\\n+\tPATH_TRANSLATED=\"$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}\" \\\n \tgit http-backend >act.out 2>act.err\n }\n \n-- \ntg: (532c2992..) t/freebsd-t5560 (depends on: t/revert-99855ddf)\n"},{"id":"238676","messageId":"vpqsipkmeum.fsf@anie.imag.fr","threadId":"36389","inReplyTo":"0779303f7d2257a618b5bed00260a8a@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-04-11T08:48:17Z","receivedAt":"2014-04-11T08:48:17Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Kyle J. McKay\" writes:\n\n> If script2.sh is changed to this:\n>\n> # script2.sh fixed\n> main() {\n> if [ 5 -gt 3 ]; then\n> return 5\n> fi\n> case bad in *)\n> echo always shows\n> esac\n> echo should not get here\n> ! :\n> }\n> main\n\nWouldn't it be better to just stop using . within function?\n\nThe .-ed script could define the complete function, and then the\nfunction would be used from the toplevel script.\n\nIf I understand correctly, your version uses nested functions with file\ninclusion between both levels of nesting. That might work for the shells\nyou tested, but if the goal is to avoid using tricky features that may\ntrigger bugs on some shells, that seems backward.\n\nIOW, why not move the whole run_specific_rebase_internal function to\ngit-rebase--$type?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"238697","messageId":"D2BF8546-87B4-431A-B86D-6EFAF205279C@gmail.com","threadId":"36389","inReplyTo":"vpqsipkmeum.fsf@anie.imag.fr","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T14:29:15Z","receivedAt":"2014-04-11T14:29:15Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 11, 2014, at 01:48, Matthieu Moy wrote:\n> \"Kyle J. McKay\" writes:\n>\n>> If script2.sh is changed to this:\n>>\n>> # script2.sh fixed\n>> main() {\n>> if [ 5 -gt 3 ]; then\n>> return 5\n>> fi\n>> case bad in *)\n>> echo always shows\n>> esac\n>> echo should not get here\n>> ! :\n>> }\n>> main\n>\n> Wouldn't it be better to just stop using . within function?\n>\n> The .-ed script could define the complete function, and then the\n> function would be used from the toplevel script.\n>\n> If I understand correctly, your version uses nested functions with \n> file\n> inclusion between both levels of nesting. That might work for the \n> shells\n> you tested, but if the goal is to avoid using tricky features that may\n> trigger bugs on some shells, that seems backward.\n\nThere are already nested functions with file inclusion between both \nlevels of nesting in git-rebase--interactive.sh and git-rebase-- \nmerge.sh now, so it's not introducing anything new.\n\n> IOW, why not move the whole run_specific_rebase_internal function to\n> git-rebase--$type?\n\nSo what change are you proposing exactly?\n\nThe current code in maint does this:\n\ngit-rebase.sh: top-level\n git-rebase.sh: run_specific_rebase()\n git-rebase.sh: run_specific_rebase_internal() -- contains \"dot\"\n git-rebase--$type.sh: top-level -- has \"return\"\n\nTo make the kind of change I think you're proposing would be somewhat \nmore invasive than the proposed patch. Each of the git-rebase--$type \nscripts would have to be modified not to do anything other than define \nfunctions when included which would require some code movement to \navoid having two \"main\" functions -- either that or there need to be \nmultiple \"dot\" includes in git-rebase.sh so the code not in a function \nonly executes when the selected case that uses it is active -- or the \nentire contents of each git-rebase--$type script gets indented and \nbecomes a function definition, but that would introduce functions \ndefining functions which seems like it would add use of a new tricky \nfeature not previously used.\n\nI'm not saying it's a bad idea, it's just somewhat more invasive than \nsimply inserting 3 lines.\n\n--Kyle\n"},{"id":"238716","messageId":"vpq4n1zg4f9.fsf@anie.imag.fr","threadId":"36389","inReplyTo":"D2BF8546-87B4-431A-B86D-6EFAF205279C@gmail.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-04-11T17:30:02Z","receivedAt":"2014-04-11T17:30:02Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Kyle J. McKay\" writes:\n\n> There are already nested functions with file inclusion between both\n> levels of nesting in git-rebase--interactive.sh and git-rebase--\n> merge.sh now, so it's not introducing anything new.\n\nOK, so it's less serious than I thought. But still, we're introducing a\nfunction with 3 levels of nesting, split accross files, in an area where\nwe know that at least one shell is buggy ...\n\n>> IOW, why not move the whole run_specific_rebase_internal function to\n>> git-rebase--$type?\n>\n> So what change are you proposing exactly?\n\nSomething along the lines of this:\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex df46f4c..4f7b22d 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,6 +4,8 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n+run_specific_rebase_infile() {\n+\n case \"$action\" in\n continue)\n \tgit am --resolved --resolvemsg=\"$resolvemsg\" \\\n@@ -75,3 +77,4 @@ then\n fi\n \n move_to_original_branch\n+}\n[ Same patch for other git-rebase--*.sh variants]\nindex 76f7f71..1a150bd 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -186,7 +186,7 @@ run_specific_rebase_internal () {\n \t# run_specific_rebase_internal has the file inclusion as a\n \t# last statement, so POSIX and FreeBSD's return will do the\n \t# same thing.\n-\t. git-rebase--$type\n+\trun_specific_rebase_infile\n }\n \n run_specific_rebase () {\n@@ -438,6 +438,8 @@ else\n \tstate_dir=\"$apply_dir\"\n fi\n \n+. git-rebase--$type\n+\n if test -z \"$rebase_root\"\n then\n \tcase \"$#\" in\n\nI minimized patch size, so it would obviously need a reidentation, and\nwould require some cleanup so that run_specific_rebase_internal is\nmerged back into run_specific_rebase (a bit like your PATCH 2).\n\nI find the result simpler, just using the basic pattern \"use '. file' to\nimport a set of functions, and then use these functions\".\n\nThe real patch is a bit more tricky though, because we need to run the\n\". git-rebase--$type\" after computing type properly. A patch passing the\ntests but requiring cleanup is given below.\n\n> To make the kind of change I think you're proposing would be somewhat\n> more invasive than the proposed patch. Each of the git-rebase--$type\n> scripts would have to be modified not to do anything other than define\n> functions\n\nThat's almost what your patch does already: move everything into a\nfunction, and call it. Except, I'd put the function call outside the\nfile inclusion.\n\n\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex df46f4c..4f7b22d 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,6 +4,8 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n+run_specific_rebase_infile() {\n+\n case \"$action\" in\n continue)\n \tgit am --resolved --resolvemsg=\"$resolvemsg\" \\\n@@ -75,3 +77,4 @@ then\n fi\n \n move_to_original_branch\n+}\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 6046778..5dfbf14 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -820,6 +820,7 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n+run_specific_rebase_infile() {\n case \"$action\" in\n continue)\n \t# do we have anything to commit?\n@@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n output git checkout $onto || die_abort \"could not detach HEAD\"\n git update-ref ORIG_HEAD $orig_head\n do_rest\n+}\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex d84f412..907aa46 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -99,6 +99,7 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n+run_specific_rebase_infile () {\n case \"$action\" in\n continue)\n \tread_state\n@@ -149,3 +150,4 @@ do\n done\n \n finish_rb_merge\n+}\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 76f7f71..63e0e68 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -186,7 +186,7 @@ run_specific_rebase_internal () {\n \t# run_specific_rebase_internal has the file inclusion as a\n \t# last statement, so POSIX and FreeBSD's return will do the\n \t# same thing.\n-\t. git-rebase--$type\n+\trun_specific_rebase_infile\n }\n \n run_specific_rebase () {\n@@ -366,6 +366,29 @@ then\n \tdie \"$(gettext \"The --edit-todo action can only be used during interactive rebase.\")\"\n fi\n \n+if test -n \"$rebase_root\" && test -z \"$onto\"\n+then\n+\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n+fi\n+\n+if test -z \"$in_progress\"\n+then\n+\tif test -n \"$interactive_rebase\"\n+\tthen\n+\t\ttype=interactive\n+\t\tstate_dir=\"$merge_dir\"\n+\telif test -n \"$do_merge\"\n+\tthen\n+\t\ttype=merge\n+\t\tstate_dir=\"$merge_dir\"\n+\telse\n+\t\ttype=am\n+\t\tstate_dir=\"$apply_dir\"\n+\tfi\n+fi\n+\n+. git-rebase--$type\n+\n case \"$action\" in\n continue)\n \t# Sanity check\n@@ -420,24 +443,6 @@ and run me again. I am stopping in case you still have something\n valuable there.')\"\n fi\n \n-if test -n \"$rebase_root\" && test -z \"$onto\"\n-then\n-\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n-fi\n-\n-if test -n \"$interactive_rebase\"\n-then\n-\ttype=interactive\n-\tstate_dir=\"$merge_dir\"\n-elif test -n \"$do_merge\"\n-then\n-\ttype=merge\n-\tstate_dir=\"$merge_dir\"\n-else\n-\ttype=am\n-\tstate_dir=\"$apply_dir\"\n-fi\n-\n if test -z \"$rebase_root\"\n then\n \tcase \"$#\" in\n\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"238750","messageId":"xmqqioqfr3ld.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"0101ef71fafab90325bb799cb8ad80e@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH 3/3] test: fix t5560 on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-11T20:52:30Z","receivedAt":"2014-04-11T20:52:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> Instead, we can use the [...] construct to defeat the special meaning\n> of the '?' character and match it exactly in a way that works for the\n> FreeBSD /bin/sh as well as other POSIX /bin/sh implementations.\n>\n> Changing the example like so:\n>\n> x='one?two?three'\n> echo \"${x#*[?]}\"\n>\n> Produces the expected output using the FreeBSD /bin/sh.\n\nI'll queue this in the meantime, while 1&2/3 are sorted out between\nyou and Matthieu.\n\nI will also take \"cp -a\" patch with \"-R -P -p\", not just \"-R\" for\nthe reasons stated in the other thread.\n\nThanks.\n"},{"id":"238756","messageId":"D3916156-0F84-4DD2-833F-64DF675325D3@gmail.com","threadId":"36389","inReplyTo":"vpq4n1zg4f9.fsf@anie.imag.fr","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-11T23:08:53Z","receivedAt":"2014-04-11T23:08:53Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 11, 2014, at 10:30, Matthieu Moy wrote:\n> \"Kyle J. McKay\" writes:\n>\n>> There are already nested functions with file inclusion between both\n>> levels of nesting in git-rebase--interactive.sh and git-rebase--\n>> merge.sh now, so it's not introducing anything new.\n>\n> OK, so it's less serious than I thought. But still, we're \n> introducing a\n> function with 3 levels of nesting, split accross files, in an area \n> where\n> we know that at least one shell is buggy ...\n\nCurrently in maint:\n\nThe current code in maint does this:\n\ngit-rebase.sh: top-level\n git-rebase.sh: run_specific_rebase()\n git-rebase.sh: run_specific_rebase_internal() -- contains \"dot\"\n git-rebase--interactive.sh: top-level (using --continue or -- \nskip)\n git-rebase--interactive.sh: do_rest\n git-rebase--interactive.sh: do_next\n git-rebase--interactive.sh: record_in_rewritten\n git-rebase--interactive.sh: flush_rewritten_pending\n\nSo I really do not see the additional level of nesting as an issue \nsince we've already got much more than 3 levels of nesting going on \nnow. If nesting was going to be a problem, something would have \nbroken already. In fact, since the follow on patch removes the \nrun_specific_rebase_internal function what we would have after the \noriginally proposed first two patches is:\n\ngit-rebase.sh: top-level\n git-rebase.sh: run_specific_rebase() -- contains \"dot\"\n git-rebase--interactive.sh: top-level (using --continue or --skip)\n git-rebase--interactive.sh: git_rebase__interactive\n git-rebase--interactive.sh: do_rest\n git-rebase--interactive.sh: do_next\n git-rebase--interactive.sh: record_in_rewritten\n git-rebase--interactive.sh: flush_rewritten_pending\n\nWhich has exactly the same nesting depth albeit the \"dot\" has moved up \none level.\n\n>>> IOW, why not move the whole run_specific_rebase_internal function to\n>>> git-rebase--$type?\n>>\n>> So what change are you proposing exactly?\n>\n> Something along the lines of this:\n\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 6046778..5dfbf14 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -820,6 +820,7 @@ add_exec_commands () {\n> \tmv \"$1.new\" \"$1\"\n> }\n>\n> +run_specific_rebase_infile() {\n> case \"$action\" in\n> continue)\n> \t# do we have anything to commit?\n> @@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: \n> checkout $onto_name\"\n> output git checkout $onto || die_abort \"could not detach HEAD\"\n> git update-ref ORIG_HEAD $orig_head\n> do_rest\n> +}\n> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\n> index d84f412..907aa46 100644\n> --- a/git-rebase--merge.sh\n> +++ b/git-rebase--merge.sh\n> @@ -99,6 +99,7 @@ finish_rb_merge () {\n> \tsay All done.\n> }\n>\n> +run_specific_rebase_infile () {\n> case \"$action\" in\n> continue)\n> \tread_state\n> @@ -149,3 +150,4 @@ do\n> done\n>\n> finish_rb_merge\n> +}\n\n\nThe problem with these changes, particularly the git-rebase-- \ninteractive.sh one is that a bunch of code is still run when the file \nis \"dot\" included. With the changes to git-rebase.sh, that code will \nnow run regardless of the action and it will run before it would have \nnow. So if any of the variables it sets affect the functions like \nread_basic_state or finish_rebase (they don't currently appear to), \nthen there's a potential for new bugs. That initial code would not \npreviously have run in the --abort case at all.\n\nBut, you say the tests pass with those changes, so the changes are \nprobably okay. However, they create a potential situation where some \ncode is added to the top of one of the git-rebase--$type.sh scripts \nand suddenly git rebase --abort stops working right because that code \nis being run when it shouldn't or the operation of read_basic_state \nand/or finish_rebase is adversely affected. Hopefully the rebase \ntests would catch any such issue right away though.\n\nSo, in light of the fact that function nesting seems to be a non-issue \nhere, and it seems to me the originally proposed changes have much \nless potential to introduce breakage either now or in the future, I \nstill prefer them.\n\n--Kyle\n"},{"id":"238776","messageId":"vpq38hi8oj3.fsf@anie.imag.fr","threadId":"36389","inReplyTo":"D3916156-0F84-4DD2-833F-64DF675325D3@gmail.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-04-12T17:07:28Z","receivedAt":"2014-04-12T17:07:28Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Kyle J. McKay\" writes:\n\n> On Apr 11, 2014, at 10:30, Matthieu Moy wrote:\n>> \"Kyle J. McKay\" writes:\n>>\n>>> There are already nested functions with file inclusion between both\n>>> levels of nesting in git-rebase--interactive.sh and git-rebase--\n>>> merge.sh now, so it's not introducing anything new.\n>>\n>> OK, so it's less serious than I thought. But still, we're\n>> introducing a\n>> function with 3 levels of nesting, split accross files, in an area\n>> where\n>> we know that at least one shell is buggy ...\n>\n> Currently in maint:\n>\n> The current code in maint does this:\n>\n> git-rebase.sh: top-level\n> git-rebase.sh: run_specific_rebase()\n> git-rebase.sh: run_specific_rebase_internal() -- contains \"dot\"\n> git-rebase--interactive.sh: top-level (using --continue or -- \n> skip)\n> git-rebase--interactive.sh: do_rest\n> git-rebase--interactive.sh: do_next\n\nYou're confusing function calls and function nesting. do_rest calls\ndo_next, but the definition of do_next is not nested within do_rest.\n\nWhen I talk about nested function, I mean\n\nf() {\n\tg() {\n\t\t...\n\t}\n}\n\nObviously, having functions call each other is not an issue. That's what\nfunctions are meant to be.\n\nNow, having run_specific_rebase_internal include a file which defines\nfunctions which contain nested functions _is_ something I find weird. It\nboth stresses the shell in a buggy area and makes the code harder to\nunderstand.\n\n> The problem with these changes, particularly the git-rebase-- \n> interactive.sh one is that a bunch of code is still run when the file\n> is \"dot\" included.\n\nFunction definitions, and variables assignments. Is it so much of an\nissue?\n\nWhat's the difference between a function definition or variable\nassignment within git-rebase--*.sh and within git-rebase.sh?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"238806","messageId":"32c0335e91b9658a9cca007f6851280@74d39fa044aa309eaea14b9f57fe79c","threadId":"36389","inReplyTo":"vpq38hi8oj3.fsf@anie.imag.fr","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-13T02:45:12Z","receivedAt":"2014-04-13T02:45:12Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 12, 2014, at 10:07, Matthieu Moy wrote:\n> \"Kyle J. McKay\" writes:\n>\n>> On Apr 11, 2014, at 10:30, Matthieu Moy wrote:\n>>> \"Kyle J. McKay\" writes:\n>>>\n>>>> There are already nested functions with file inclusion between both\n>>>> levels of nesting in git-rebase--interactive.sh and git-rebase--\n>>>> merge.sh now, so it's not introducing anything new.\n>>>\n>>> OK, so it's less serious than I thought. But still, we're\n>>> introducing a\n>>> function with 3 levels of nesting, split accross files, in an area\n>>> where\n>>> we know that at least one shell is buggy ...\n>>\n>> Currently in maint:\n>>\n>> The current code in maint does this:\n>>\n>> git-rebase.sh: top-level\n>> git-rebase.sh: run_specific_rebase()\n>> git-rebase.sh: run_specific_rebase_internal() -- contains \"dot\"\n>> git-rebase--interactive.sh: top-level (using --continue or --\n>> skip)\n>> git-rebase--interactive.sh: do_rest\n>> git-rebase--interactive.sh: do_next\n>\n> You're confusing function calls and function nesting. do_rest calls\n> do_next, but the definition of do_next is not nested within do_rest.\n>\n> When I talk about nested function, I mean\n>\n> f() {\n> \tg() {\n> \t\t...\n> \t}\n> }\n>\n> Obviously, having functions call each other is not an issue. That's \n> what\n> functions are meant to be.\n>\n> Now, having run_specific_rebase_internal include a file which defines\n> functions which contain nested functions _is_ something I find \n> weird. It\n> both stresses the shell in a buggy area and makes the code harder to\n> understand.\n\nI meant: \"nested functions\" = \"nested function calls\"\nYou meant: \"nested functions\" = \"nested function definitions\"\n\nOkay. But nested function definitions is not something new to the\nrebase code.\n\n>> The problem with these changes, particularly the git-rebase--\n>> interactive.sh one is that a bunch of code is still run when the file\n>> is \"dot\" included.\n>\n> Function definitions, and variables assignments. Is it so much of an\n> issue?\n>\n> What's the difference between a function definition or variable\n> assignment within git-rebase--*.sh and within git-rebase.sh?\n\nAs I said, this is the issue:\n\nOn Apr 11, 2014, at 16:08, Kyle J. McKay wrote:\n\n> The problem with these changes, particularly the git-rebase-- \n> interactive.sh one is that a bunch of code is still run when the \n> file is \"dot\" included. With the changes to git-rebase.sh, that \n> code will now run regardless of the action and it will run before it \n> would have now. So if any of the variables it sets affect the \n> functions like read_basic_state or finish_rebase (they don't \n> currently appear to), then there's a potential for new bugs. That \n> initial code would not previously have run in the --abort case at all.\n\nLet me rephrase.\n\nPatch 1/3 does not change the order in which individual statements are \nexecuted in the rebase code. Nor does it change the logic. It simply \nintroduces one additional function callstack level, but the same \nindividual statements are executed in the same order for all control \nflows. No additional statements other than the introduced callstack \nlevel. Nothing's executed in a different order. No control paths \nexecute additional statements they did not before.\n\nThe changes you propose introduce exactly the same additional function \ncallstack level. Then they proceed to alter the order in which \nstatements are executed. Statements that did not execute in some \ncontrol paths before are now executed in those control paths. Other \nstatements are moved around to execute earlier/later than they did \nbefore.\n\nMy point is not that this is wrong. It's nice, really, to move the \n\"dot\" include to the top level. The point is that's not necessary to \nfix the problem and moving statements around and adding statements to \nsome control paths increases the risk of introducing an uncaught bug \nwhen it's not necessary to fix the problem.\n\nI would like to get a fix in so that rebase works out-of-the-box when \nGit version 1.8.4 or later is built on FreeBSD.\n\nSo I'm not going to belabor the point anymore.\n\nThere's a follow-up patch 4/3 attached to the end of this message that \nmakes the changes you have proposed in your earlier email on top of \npatches 1/3 and 2/3. The log message and all changes are taken from \nyour emails and so this patch is assigned to you and has a\n'Needs-Signed-off-by:' line.\n\nOn Apr 11, 2014, at 10:30, Matthieu Moy wrote:\n> The real patch is a bit more tricky though, because we need to run the\n> \". git-rebase--$type\" after computing type properly. A patch passing \n> the\n> tests but requiring cleanup is given below.\n\nUnfortunately, after applying the below patch, some of the rebase \ntests (3403, 3407, 3418, 3420, 3421) start failing for me on systems \nwhere they did not fail previously. Even though some of the previously \nfailing tests on FreeBSD now pass with the patch, 3421 now fails on \nFreeBSD where it did not before.\n\nHere's a test summary of the t34xx*.sh tests:\n\nNOTE: at the time of these tests, maint and v1.9.2 were at the same \ncommit.\n\n______________SYSTEM____t34xx*.sh_results________________\n\nmaint FreeBSD FAIL: 3403,3404,3407,3409,3410,3412,3418,3419,3420\nmaint Darwin PASS\nmaint Linux PASS\n\nmaint+1-3/3 FreeBSD PASS\nmaint+1-3/3 Darwin PASS\nmaint+1-3/3 Linux PASS\n\nmaint+1-4/3 FreeBSD FAIL[*]: 3403,3407,3418,3420,3421\nmaint+1-4/3 Darwin FAIL[*]: 3403,3407,3418,3420,3421\nmaint+1-4/3 Linux FAIL[*]: 3403,3407,3418,3420,3421\n\n[*]: The failing test reports for all three systems are identical for \nthe \"maint+1-4/3\" run (except for the wallclock secs part):\n\nTest Summary Report\n-------------------\nt3403-rebase-skip.sh (Wstat: 256 Tests: 10 Failed: 3)\n Failed tests: 8-10\n Non-zero exit status: 1\nt3407-rebase-abort.sh (Wstat: 256 Tests: 11 Failed: 3)\n Failed tests: 7-9\n Non-zero exit status: 1\nt3418-rebase-continue.sh (Wstat: 256 Tests: 6 Failed: 2)\n Failed tests: 5-6\n Non-zero exit status: 1\nt3420-rebase-autostash.sh (Wstat: 256 Tests: 24 Failed: 11)\n Failed tests: 14-24\n Non-zero exit status: 1\nt3421-rebase-topology-linear.sh (Wstat: 256 Tests: 76 Failed: 16)\n Failed tests: 51-53, 55, 57, 59-63, 65, 67, 73-76\n Non-zero exit status: 1\nFiles=22, Tests=374, 354 wallclock secs ( 0.31 usr 0.13 sys + 79.57 cusr 233.99 csys = 314.00 CPU)\nResult: FAIL\nmake: *** [prove] Error 1\n\n\nSo I suggest that in the interest of fixing rebase on FreeBSD in an \nexpeditious fashion, patches 1/3 and 2/3 get picked up (see note \nbelow) now and that the follow-on patch below, after being enhanced to \npass all tests, be submitted separately at some future point.\n\n--Kyle\n\nP.S. Note to JCH: the below patch requires the previous 1/3 and 2/3 \nbe applied first. As per SubmittingPatches for bug fixes those are \nbased on maint. Because of the whitespace change 1/3 introduces it \ndoes not apply cleanly to master, next or pu. :( Please let me know \nif you would like me to resend the initial series (1 & 2 -- 3 has \nalready been picked up) based on a different branch instead of maint.\n\n---- 8< ----\nFrom: Matthieu Moy \nSubject: [PATCH 4/3] rebase: stop using . within function\n\nMove the whole run_specific_rebase_internal function to\ngit-rebase--$type.\n\nThe .-ed script defines the complete function, and then the\nfunction is used from the toplevel script.\n\nThe goal is to avoid using tricky features that may trigger\nbugs on some shells.\n\nThe result is simpler, just using the basic pattern:\n\n 1. use '. file' to import a set of functions\n 2. then use these functions\n\nNeeds-Signed-off-by: Matthieu Moy \n---\n git-rebase--am.sh | 3 +--\n git-rebase--interactive.sh | 3 +--\n git-rebase--merge.sh | 3 +--\n git-rebase.sh | 40 +++++++++++++++++++++-------------------\n 4 files changed, 24 insertions(+), 25 deletions(-)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex 2d3f6d55..b48b3e90 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,7 +4,7 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n-git_rebase__am() {\n+run_specific_rebase_infile() {\n \tcase \"$action\" in\n \tcontinue)\n \t\tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n@@ -75,4 +75,3 @@ git_rebase__am() {\n \n \tmove_to_original_branch\n }\n-git_rebase__am\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 42164f11..a7670eb0 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -810,7 +810,7 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n-git_rebase__interactive() {\n+run_specific_rebase_infile() {\n \tcase \"$action\" in\n \tcontinue)\n \t\t# do we have anything to commit?\n@@ -1044,4 +1044,3 @@ EOF\n \tgit update-ref ORIG_HEAD $orig_head\n \tdo_rest\n }\n-git_rebase__interactive\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex b5f05bf5..9550e656 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -101,7 +101,7 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n-git_rebase__merge() {\n+run_specific_rebase_infile() {\n \tcase \"$action\" in\n \tcontinue)\n \t\tread_state\n@@ -153,4 +153,3 @@ git_rebase__merge() {\n \n \tfinish_rb_merge\n }\n-git_rebase__merge\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 07e2bd48..9e105626 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -175,7 +175,7 @@ run_specific_rebase () {\n \t\texport GIT_EDITOR\n \t\tautosquash=\n \tfi\n-\t. git-rebase--$type\n+\trun_specific_rebase_infile\n \tret=$?\n \tif test $ret -eq 0\n \tthen\n@@ -353,6 +353,26 @@ then\n \tdie \"$(gettext \"The --edit-todo action can only be used during interactive rebase.\")\"\n fi\n \n+if test -n \"$rebase_root\" && test -z \"$onto\"\n+then\n+\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n+fi\n+\n+if test -n \"$interactive_rebase\"\n+then\n+\ttype=interactive\n+\tstate_dir=\"$merge_dir\"\n+elif test -n \"$do_merge\"\n+then\n+\ttype=merge\n+\tstate_dir=\"$merge_dir\"\n+else\n+\ttype=am\n+\tstate_dir=\"$apply_dir\"\n+fi\n+\n+. git-rebase--$type\n+\n case \"$action\" in\n continue)\n \t# Sanity check\n@@ -407,24 +427,6 @@ and run me again. I am stopping in case you still have something\n valuable there.')\"\n fi\n \n-if test -n \"$rebase_root\" && test -z \"$onto\"\n-then\n-\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n-fi\n-\n-if test -n \"$interactive_rebase\"\n-then\n-\ttype=interactive\n-\tstate_dir=\"$merge_dir\"\n-elif test -n \"$do_merge\"\n-then\n-\ttype=merge\n-\tstate_dir=\"$merge_dir\"\n-else\n-\ttype=am\n-\tstate_dir=\"$apply_dir\"\n-fi\n-\n if test -z \"$rebase_root\"\n then\n \tcase \"$#\" in\n-- \n1.8.5\n"},{"id":"238819","messageId":"vpq8ur85neh.fsf@anie.imag.fr","threadId":"36389","inReplyTo":"32c0335e91b9658a9cca007f6851280@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-04-14T08:24:38Z","receivedAt":"2014-04-14T08:24:38Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"\"Kyle J. McKay\" writes:\n\n> So I suggest that in the interest of fixing rebase on FreeBSD in an \n> expeditious fashion, patches 1/3 and 2/3 get picked up (see note \n> below) now and that the follow-on patch below, after being enhanced to \n> pass all tests, be submitted separately at some future point.\n\nSeems a good plan to me.\n\n> Needs-Signed-off-by: Matthieu Moy \n\nSigned-off-by: Matthieu Moy \n\n> From: Matthieu Moy \n> Subject: [PATCH 4/3] rebase: stop using . within function\n> \n> Move the whole run_specific_rebase_internal function to\n> git-rebase--$type.\n> \n> The .-ed script defines the complete function, and then the\n> function is used from the toplevel script.\n> \n> The goal is to avoid using tricky features that may trigger\n> bugs on some shells.\n> \n> The result is simpler, just using the basic pattern:\n> \n> 1. use '. file' to import a set of functions\n> 2. then use these functions\n> \n> Needs-Signed-off-by: Matthieu Moy \n> ---\n> git-rebase--am.sh | 3 +--\n> git-rebase--interactive.sh | 3 +--\n> git-rebase--merge.sh | 3 +--\n> git-rebase.sh | 40 +++++++++++++++++++++-------------------\n> 4 files changed, 24 insertions(+), 25 deletions(-)\n> \n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index 2d3f6d55..b48b3e90 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -4,7 +4,7 @@\n> # Copyright (c) 2010 Junio C Hamano.\n> #\n> \n> -git_rebase__am() {\n> +run_specific_rebase_infile() {\n> \tcase \"$action\" in\n> \tcontinue)\n> \t\tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n> @@ -75,4 +75,3 @@ git_rebase__am() {\n> \n> \tmove_to_original_branch\n> }\n> -git_rebase__am\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 42164f11..a7670eb0 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -810,7 +810,7 @@ add_exec_commands () {\n> \tmv \"$1.new\" \"$1\"\n> }\n> \n> -git_rebase__interactive() {\n> +run_specific_rebase_infile() {\n> \tcase \"$action\" in\n> \tcontinue)\n> \t\t# do we have anything to commit?\n> @@ -1044,4 +1044,3 @@ EOF\n> \tgit update-ref ORIG_HEAD $orig_head\n> \tdo_rest\n> }\n> -git_rebase__interactive\n> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\n> index b5f05bf5..9550e656 100644\n> --- a/git-rebase--merge.sh\n> +++ b/git-rebase--merge.sh\n> @@ -101,7 +101,7 @@ finish_rb_merge () {\n> \tsay All done.\n> }\n> \n> -git_rebase__merge() {\n> +run_specific_rebase_infile() {\n> \tcase \"$action\" in\n> \tcontinue)\n> \t\tread_state\n> @@ -153,4 +153,3 @@ git_rebase__merge() {\n> \n> \tfinish_rb_merge\n> }\n> -git_rebase__merge\n> diff --git a/git-rebase.sh b/git-rebase.sh\n> index 07e2bd48..9e105626 100755\n> --- a/git-rebase.sh\n> +++ b/git-rebase.sh\n> @@ -175,7 +175,7 @@ run_specific_rebase () {\n> \t\texport GIT_EDITOR\n> \t\tautosquash=\n> \tfi\n> -\t. git-rebase--$type\n> +\trun_specific_rebase_infile\n> \tret=$?\n> \tif test $ret -eq 0\n> \tthen\n> @@ -353,6 +353,26 @@ then\n> \tdie \"$(gettext \"The --edit-todo action can only be used during interactive rebase.\")\"\n> fi\n> \n> +if test -n \"$rebase_root\" && test -z \"$onto\"\n> +then\n> +\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n> +fi\n> +\n> +if test -n \"$interactive_rebase\"\n> +then\n> +\ttype=interactive\n> +\tstate_dir=\"$merge_dir\"\n> +elif test -n \"$do_merge\"\n> +then\n> +\ttype=merge\n> +\tstate_dir=\"$merge_dir\"\n> +else\n> +\ttype=am\n> +\tstate_dir=\"$apply_dir\"\n> +fi\n> +\n> +. git-rebase--$type\n> +\n> case \"$action\" in\n> continue)\n> \t# Sanity check\n> @@ -407,24 +427,6 @@ and run me again. I am stopping in case you still have something\n> valuable there.')\"\n> fi\n> \n> -if test -n \"$rebase_root\" && test -z \"$onto\"\n> -then\n> -\ttest -z \"$interactive_rebase\" && interactive_rebase=implied\n> -fi\n> -\n> -if test -n \"$interactive_rebase\"\n> -then\n> -\ttype=interactive\n> -\tstate_dir=\"$merge_dir\"\n> -elif test -n \"$do_merge\"\n> -then\n> -\ttype=merge\n> -\tstate_dir=\"$merge_dir\"\n> -else\n> -\ttype=am\n> -\tstate_dir=\"$apply_dir\"\n> -fi\n> -\n> if test -z \"$rebase_root\"\n> then\n> \tcase \"$#\" in\n> -- \n> 1.8.5\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"238872","messageId":"xmqqtx9vlf4s.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"vpq8ur85neh.fsf@anie.imag.fr","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T22:28:51Z","receivedAt":"2014-04-14T22:28:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> \"Kyle J. McKay\" writes:\n>\n>> So I suggest that in the interest of fixing rebase on FreeBSD in an \n>> expeditious fashion, patches 1/3 and 2/3 get picked up (see note \n>> below) now and that the follow-on patch below, after being enhanced to \n>> pass all tests, be submitted separately at some future point.\n>\n> Seems a good plan to me.\n\nOK, should I take that an Ack on 1 & 2?\n"},{"id":"238873","messageId":"xmqqob03le3v.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"0779303f7d2257a618b5bed00260a8a@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-14T22:51:00Z","receivedAt":"2014-04-14T22:51:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> For convenience, here are the diffs using -w:\n>\n> |--- a/git-rebase--am.sh\n> |+++ b/git-rebase--am.sh\n> |@@ -4,6 +4,7 @@\n> # Copyright (c) 2010 Junio C Hamano.\n> #\n> \n> +git_rebase__am() {\n> \tcase \"$action\" in\n> \tcontinue)\n> \t\tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n> |@@ -73,3 +74,5 @@ then\n> \tfi\n> \n> \tmove_to_original_branch\n> +}\n> +git_rebase__am\n\nI think we would want to see the actual change formatted this way\n(without needing to pass \"-w\" to \"git show\"), as it will make it\nclear that this artificial extra level of \"define the whole thing\ninside a function and then make a single call to it\" is a workaround\nof specific shell's glitch that we would not have to have in an\nideal world ;-)\n\nBesides that would make it less likely to cause conflicts with the\nreal changes in flight.\n\nPlease double check what I queued on 'pu' when I push out today's\nintegration result.\n\nThanks.\n"},{"id":"238915","messageId":"FAD02895-24B2-46C3-ABEF-E9CE17926FF9@gmail.com","threadId":"36389","inReplyTo":"xmqqob03le3v.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-16T04:32:40Z","receivedAt":"2014-04-16T04:32:40Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 14, 2014, at 15:51, Junio C Hamano wrote:\n> I think we would want to see the actual change formatted this way\n> (without needing to pass \"-w\" to \"git show\"), as it will make it\n> clear that this artificial extra level of \"define the whole thing\n> inside a function and then make a single call to it\" is a workaround\n> of specific shell's glitch that we would not have to have in an\n> ideal world ;-)\n>\n> Besides that would make it less likely to cause conflicts with the\n> real changes in flight.\n\nSounds good to me.\n\n> Please double check what I queued on 'pu' when I push out today's\n> integration result.\n\n> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n> index a4f683a5..2ab514ea 100644\n> --- a/git-rebase--am.sh\n> +++ b/git-rebase--am.sh\n> @@ -4,6 +4,13 @@\n> # Copyright (c) 2010 Junio C Hamano.\n> #\n>\n> +# The whole contents of the file is run by dot-sourcing this file \n> from\n> +# inside a shell function, and \"return\"s we see below are expected to\n> +# return from that function that dot-sources us. However, FreeBSD\n> +# /bin/sh misbehaves on such a construct, so we will work it around \n> by\n> +# enclosing the whole thing inside an extra layer of a function.\n> +git_rebase__am () {\n\nI think the wording may be just a bit off:\n\n> and \"return\"s we see below are expected to return from that function\n> that dot-sources us.\n\nI thought that was one of the buggy behaviors we are working around?\n\nMaybe I'm just reading it wrong...\n\nDoes this wording seem any clearer?\n\n\tand \"return\"s we see below are expected not to return\n\tfrom the function that dot-sources us, but rather to\n\tthe next command after the one in the function that\n\tdot-sources us.\n\nOtherwise the patch in pu looks fine (all t34*.sh tests pass for me on \nFreeBSD with pu at 8d8dc6db).\n\nThank you for adding the comments.\n\nIf I'm the only one getting a wrong meaning from the comments, then no \nreason to change them.\n\n--Kyle\n"},{"id":"238941","messageId":"xmqqsipdi5lw.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"FAD02895-24B2-46C3-ABEF-E9CE17926FF9@gmail.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T16:47:23Z","receivedAt":"2014-04-16T16:47:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> On Apr 14, 2014, at 15:51, Junio C Hamano wrote:\n>> I think we would want to see the actual change formatted this way\n>> (without needing to pass \"-w\" to \"git show\"), as it will make it\n>> clear that this artificial extra level of \"define the whole thing\n>> inside a function and then make a single call to it\" is a workaround\n>> of specific shell's glitch that we would not have to have in an\n>> ideal world ;-)\n>>\n>> Besides that would make it less likely to cause conflicts with the\n>> real changes in flight.\n>\n> Sounds good to me.\n>\n>> Please double check what I queued on 'pu' when I push out today's\n>> integration result.\n>\n>> diff --git a/git-rebase--am.sh b/git-rebase--am.sh\n>> index a4f683a5..2ab514ea 100644\n>> --- a/git-rebase--am.sh\n>> +++ b/git-rebase--am.sh\n>> @@ -4,6 +4,13 @@\n>> # Copyright (c) 2010 Junio C Hamano.\n>> #\n>>\n>> +# The whole contents of the file is run by dot-sourcing this file\n>> from\n>> +# inside a shell function, and \"return\"s we see below are expected to\n>> +# return from that function that dot-sources us. However, FreeBSD\n>> +# /bin/sh misbehaves on such a construct, so we will work it around\n>> by\n>> +# enclosing the whole thing inside an extra layer of a function.\n>> +git_rebase__am () {\n>\n> I think the wording may be just a bit off:\n>\n>> and \"return\"s we see below are expected to return from that function\n>> that dot-sources us.\n>\n> I thought that was one of the buggy behaviors we are working around?\n\nYeah, it is written as if every reader has the knowledge that the\nextra\n\n\textra__func () {\n ...\n }\n extra__func\n\naround did not originally exist. The description does not read well\nwith the work-around already there.\n\n> Maybe I'm just reading it wrong...\n>\n> Does this wording seem any clearer?\n>\n> \tand \"return\"s we see below are expected not to return\n> \tfrom the function that dot-sources us, but rather to\n> \tthe next command after the one in the function that\n> \tdot-sources us.\n\nNot really. The comment was not about explaining \"return\"s. When a\nreader reads the text with the work-around, it is clear that these\n\"return\"s are inside this extra function and there is no possible\ninterpretation other than that they are to return from the extra\nfunction.\n\nThe comment was meant to explain why a seemingly strange \"define a\nfunction and then immediately call it just once\" pattern is used,\nand \"Earlier, these returns were not inside any function when this\nfile is viewed standalone. Because this file is to be dot-sourced\nwithin a function, they were to return from that dot-sourcing\nfunction --- but some shells do not behave that way, hence this\nstrange construct.\" was meant to be that explanation, but apparently\nit failed to convey what I meant to say.\n\n> If I'm the only one getting a wrong meaning from the comments, then no\n> reason to change them.\n\nI agree that the description does not read well with the work-around\nalready there. I am not sure what would be a better way to phrase\nit, though.\n"},{"id":"238969","messageId":"xmqqtx9tgn5l.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"xmqqsipdi5lw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T18:11:18Z","receivedAt":"2014-04-16T18:11:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Kyle J. McKay\" writes:\n>\n>> If I'm the only one getting a wrong meaning from the comments, then no\n>> reason to change them.\n>\n> I agree that the description does not read well with the work-around\n> already there. I am not sure what would be a better way to phrase\n> it, though.\n\nHere is a tentative rewrite at the beginning and the end of rebase-am:\n\n # The whole contents of the file is run by dot-sourcing this\n # file from inside a shell function. It used to be that\n # \"return\"s we see below were not inside any function, and\n # expected to return from the function that dot-sourced us.\n #\n # However, FreeBSD /bin/sh misbehaves on such a construct and\n # continues to run the statements that follow such a \"return\".\n # As a work-around, we introduce an extra layer of a function\n # here, and immediately call it after defining it.\n git_rebase__am () {\n\n ...\n\n }\n # ... and then we call the whole thing.\n git_rebase__am\n\n\nBy the way, you have this in your log message:\n\n ... the git-rebase--*.sh scripts have used a \"return\" to return\n from the \"dot\" command that runs them. While this is allowed by\n POSIX,...\n\n\nIs it \"this is allowed\", or is it \"this should be the way and shells\nthat do not do so are buggy\"?\n"},{"id":"238970","messageId":"xmqqppkhgmll.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"xmqqtx9tgn5l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-16T18:23:18Z","receivedAt":"2014-04-16T18:23:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> By the way, you have this in your log message:\n>\n> ... the git-rebase--*.sh scripts have used a \"return\" to return\n> from the \"dot\" command that runs them. While this is allowed by\n> POSIX,...\n>\n>\n> Is it \"this is allowed\", or is it \"this should be the way and shells\n> that do not do so are buggy\"?\n\nAnswering myself...\n\nThe only \"unspecified\" I see is this:\n\n If the shell is not currently executing a function or dot\n script, the results are unspecified.\n\nwhich clearly does not apply to the version before this patch (we\nare executing a dot script). And\n\n The return utility shall cause the shell to stop executing the\n current function or dot script.\n\nwould mean that we are correct to expect that \"should not get here\"\nis not reached, as the \"return 5\" would cause the shell to stop\nexecuting the dot script there.\n\nSo \"while this is allowed by POSIX\" may be a bit misleading and\nneeds to be reworded, I guess?\n"},{"id":"238993","messageId":"F88F585D-B146-4CD2-A7E9-D3CB2C5B25A4@gmail.com","threadId":"36389","inReplyTo":"xmqqtx9tgn5l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-17T00:41:51Z","receivedAt":"2014-04-17T00:41:51Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 16, 2014, at 11:11, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> \"Kyle J. McKay\" writes:\n>>\n>>> If I'm the only one getting a wrong meaning from the comments, \n>>> then no\n>>> reason to change them.\n>>\n>> I agree that the description does not read well with the work-around\n>> already there. I am not sure what would be a better way to phrase\n>> it, though.\n>\n> Here is a tentative rewrite at the beginning and the end of rebase-am:\n>\n> # The whole contents of the file is run by dot-sourcing this\n> # file from inside a shell function. It used to be that\n> # \"return\"s we see below were not inside any function, and\n> # expected to return from the function that dot-sourced us.\n\nI think it's the \"return from the function that dot-sourced us\" that \ngives me the wrong meaning. The \"return from\" part says to me that \nfunction will be returning which it will not unless the dot command \nwas the last command in the function.\n\nEither \"return to the function that dot-sourced us\" or \"return from \nthe dot command that dot-sourced us\", but using the original wording \nimplies to me that the function that dot-sourced us will return as \nsoon as the dot-sourced script executes the return and that is exactly \none of the bugs we're working around.\n\nI think just the s/from/to/ would fix it so it does not give me the \nwrong impression, but that doesn't mean that would not confuse \neveryone else. ;)\n\n> #\n> # However, FreeBSD /bin/sh misbehaves on such a construct and\n> # continues to run the statements that follow such a \"return\".\n> # As a work-around, we introduce an extra layer of a function\n> # here, and immediately call it after defining it.\n> git_rebase__am () {\n>\n> ...\n>\n> }\n> # ... and then we call the whole thing.\n> git_rebase__am\n\nOn Apr 16, 2014, at 11:23, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> By the way, you have this in your log message:\n>>\n>> ... the git-rebase--*.sh scripts have used a \"return\" to return\n>> from the \"dot\" command that runs them. While this is allowed by\n>> POSIX,...\n>>\n>>\n>> Is it \"this is allowed\", or is it \"this should be the way and shells\n>> that do not do so are buggy\"?\n>\n> Answering myself...\n>\n> The only \"unspecified\" I see is this:\n>\n> If the shell is not currently executing a function or dot\n> script, the results are unspecified.\n>\n> which clearly does not apply to the version before this patch (we\n> are executing a dot script). And\n>\n> The return utility shall cause the shell to stop executing the\n> current function or dot script.\n>\n> would mean that we are correct to expect that \"should not get here\"\n> is not reached, as the \"return 5\" would cause the shell to stop\n> executing the dot script there.\n>\n> So \"while this is allowed by POSIX\" may be a bit misleading and\n> needs to be reworded, I guess?\n\n\"allowed by POSIX\" is not incorrect. ;)\n\nBut perhaps the log message should say 'While POSIX specifies that a \n\"return\" may be used to exit a \"dot\" sourced script,' there instead to \nbe clearer. Which would make that whole first paragraph become:\n\n Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) \nthe\n git-rebase--*.sh scripts have used a \"return\" to return from the \n\"dot\"\n command that runs them. While POSIX specifies that a \"return\" \nmay be\n used to exit a \"dot\" sourced script, the FreeBSD /bin/sh utility\n behaves poorly under some circumstances when such a \"return\" is \nexecuted.\n"},{"id":"239023","messageId":"xmqq4n1rg9nc.fsf@gitster.dls.corp.google.com","threadId":"36389","inReplyTo":"F88F585D-B146-4CD2-A7E9-D3CB2C5B25A4@gmail.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-17T17:15:19Z","receivedAt":"2014-04-17T17:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" writes:\n\n> Either \"return to the function that dot-sourced us\" or \"return from\n> the dot command that dot-sourced us\",\n\n> but using the original wording\n> implies to me that the function that dot-sourced us will return as\n> soon as the dot-sourced script executes the return and that is exactly\n> one of the bugs we're working around.\n\nTrue. \"return to the function that dot-sourced us\" is what I meant.\nTo be more pedantic, \"return to the point in the function that\ndot-sourced this file\".\n\n> I think just the s/from/to/ would fix it so it does not give me the\n> wrong impression, but that doesn't mean that would not confuse\n> everyone else. ;)\n\nYeah, let's do that. Thanks for carefully reading them.\n\n>> So \"while this is allowed by POSIX\" may be a bit misleading and\n>> needs to be reworded, I guess?\n>\n> \"allowed by POSIX\" is not incorrect. ;)\n\nCalling something that is required as \"allowed\" is a bit misleading,\nif not outright dishonest. Allowed implies that you are free not to\ndo so and can still be in compliance, but I do not think that is the\ncase.\n\nI'd think it makes it clearer to say that we take \"return stopping\nthe dot-sourced file\" as a given and FreeBSD does not behave that\nway.\n\n-- >8 --\nFrom: \"Kyle J. McKay\" \nDate: Fri, 11 Apr 2014 01:28:17 -0700\nSubject: [PATCH] rebase: avoid non-function use of \"return\" on FreeBSD\n\nSince a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4)\nthe git-rebase--*.sh scripts have used a \"return\" to stop execution\nof the dot-sourced file and return to the \"dot\" command that\ndot-sourced it. The /bin/sh utility on FreeBSD however behaves\npoorly under some circumstances when such a \"return\" is executed.\n\nIn particular, if the \"dot\" command is contained within a function,\nthen when a \"return\" is executed by the script it runs (that is not\nitself inside a function), control will return from the function\nthat contains the \"dot\" command skipping any statements that might\nfollow the dot command inside that function. Commit 99855ddf (first\nappearing in v1.8.4.1) addresses this by making the \"dot\" command\nthe last line in the function.\n\nUnfortunately the FreeBSD /bin/sh may also execute some statements\nin the script run by the \"dot\" command that appear after the\ntroublesome \"return\". The fix in 99855ddf does not address this\nproblem.\n\nFor example, if you have script1.sh with these contents:\n\nrun_script2() {\n . \"$(dirname -- \"$0\")/script2.sh\"\n _e=$?\n echo only this line should show\n [ $_e -eq 5 ] || echo expected status 5 got $_e\n return 3\n}\nrun_script2\ne=$?\n[ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; }\n\nAnd script2.sh with these contents:\n\nif [ 5 -gt 3 ]; then\n return 5\nfi\ncase bad in *)\n echo always shows\nesac\necho should not get here\n! :\n\nWhen running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh'\nafter making it executable), the expected output from a POSIX shell\nis simply the single line:\n\nonly this line should show\n\nHowever, when run using FreeBSD's /bin/sh, the following output\nappears instead:\n\nshould not get here\nexpected status 3 got 1\n\nNot only did the lines following the \"dot\" command in the run_script2\nfunction in script1.sh get skipped, but additional lines in script2.sh\nfollowing the \"return\" got executed -- but not all of them (e.g. the\n\"echo always shows\" line did not run).\n\nThese issues can be avoided by not using a top-level \"return\" in\nscript2.sh. If script2.sh is changed to this:\n\nmain() {\n if [ 5 -gt 3 ]; then\n return 5\n fi\n case bad in *)\n echo always shows\n esac\n echo should not get here\n ! :\n}\nmain\n\nThen it behaves the same when using FreeBSD's /bin/sh as when using\nother more POSIX compliant /bin/sh implementations.\n\nWe fix the git-rebase--*.sh scripts in a similar fashion by moving\nthe top-level code that contains \"return\" statements into its own\nfunction and then calling that as the last line in the script.\n\nSigned-off-by: Kyle J. McKay \nAcked-by: Matthieu Moy \nSigned-off-by: Junio C Hamano \n---\n git-rebase--am.sh | 15 +++++++++++++++\n git-rebase--interactive.sh | 15 +++++++++++++++\n git-rebase--merge.sh | 15 +++++++++++++++\n 3 files changed, 45 insertions(+)\n\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex a4f683a..1cdc139 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -4,6 +4,17 @@\n # Copyright (c) 2010 Junio C Hamano.\n #\n \n+# The whole contents of this file is run by dot-sourcing it from\n+# inside a shell function. It used to be that \"return\"s we see\n+# below were not inside any function, and expected to return\n+# to the function that dot-sourced us.\n+#\n+# However, FreeBSD /bin/sh misbehaves on such a construct and\n+# continues to run the statements that follow such a \"return\".\n+# As a work-around, we introduce an extra layer of a function\n+# here, and immediately call it after defining it.\n+git_rebase__am () {\n+\n case \"$action\" in\n continue)\n \tgit am --resolved --resolvemsg=\"$resolvemsg\" &&\n@@ -73,3 +84,7 @@ then\n fi\n \n move_to_original_branch\n+\n+}\n+# ... and then we call the whole thing.\n+git_rebase__am\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 43631b4..9e1dd1e 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -810,6 +810,17 @@ add_exec_commands () {\n \tmv \"$1.new\" \"$1\"\n }\n \n+# The whole contents of this file is run by dot-sourcing it from\n+# inside a shell function. It used to be that \"return\"s we see\n+# below were not inside any function, and expected to return\n+# to the function that dot-sourced us.\n+#\n+# However, FreeBSD /bin/sh misbehaves on such a construct and\n+# continues to run the statements that follow such a \"return\".\n+# As a work-around, we introduce an extra layer of a function\n+# here, and immediately call it after defining it.\n+git_rebase__interactive () {\n+\n case \"$action\" in\n continue)\n \t# do we have anything to commit?\n@@ -1042,3 +1053,7 @@ GIT_REFLOG_ACTION=\"$GIT_REFLOG_ACTION: checkout $onto_name\"\n output git checkout $onto || die_abort \"could not detach HEAD\"\n git update-ref ORIG_HEAD $orig_head\n do_rest\n+\n+}\n+# ... and then we call the whole thing.\n+git_rebase__interactive\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex e7d96de..838fbed 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -101,6 +101,17 @@ finish_rb_merge () {\n \tsay All done.\n }\n \n+# The whole contents of this file is run by dot-sourcing it from\n+# inside a shell function. It used to be that \"return\"s we see\n+# below were not inside any function, and expected to return\n+# to the function that dot-sourced us.\n+#\n+# However, FreeBSD /bin/sh misbehaves on such a construct and\n+# continues to run the statements that follow such a \"return\".\n+# As a work-around, we introduce an extra layer of a function\n+# here, and immediately call it after defining it.\n+git_rebase__merge () {\n+\n case \"$action\" in\n continue)\n \tread_state\n@@ -151,3 +162,7 @@ do\n done\n \n finish_rb_merge\n+\n+}\n+# ... and then we call the whole thing.\n+git_rebase__merge\n-- \n1.9.2-630-gc3ddd0c\n"},{"id":"239058","messageId":"03D68D70-7E30-428C-A13A-79BF477F0CBF@gmail.com","threadId":"36389","inReplyTo":"xmqq4n1rg9nc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/3] rebase: avoid non-function use of \"return\" on FreeBSD","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2014-04-18T00:26:00Z","receivedAt":"2014-04-18T00:26:00Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Apr 17, 2014, at 10:15, Junio C Hamano wrote:\n\n>> I think just the s/from/to/ would fix it so it does not give me the\n>> wrong impression, but that doesn't mean that would not confuse\n>> everyone else. ;)\n>\n> Yeah, let's do that. Thanks for carefully reading them.\n\n> I'd think it makes it clearer to say that we take \"return stopping\n> the dot-sourced file\" as a given and FreeBSD does not behave that\n> way.\n>\n> -- > 8 --\n> From: \"Kyle J. McKay\" \n> Date: Fri, 11 Apr 2014 01:28:17 -0700\n> Subject: [PATCH] rebase: avoid non-function use of \"return\" on FreeBSD\n[...]\n\nThe new version of the patch looks great, let's use that. Thanks for \nadjusting it.\n"}]}