* [PATCH 0/3] Fix support for FreeBSD's /bin/sh @ 2014-04-11 8:28 Kyle J. McKay 2014-04-11 8:28 ` [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD Kyle J. McKay ` (2 more replies) 0 siblings, 3 replies; 21+ messages in thread From: Kyle J. McKay @ 2014-04-11 8:28 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine This patch series words around the FreeBSD /bin/sh problems that cause rebase to fail on FreeBSD as well as test t5560-http-backend-noserver. The rebase issue was first introduced in Git v1.8.4 which started using the "return" statement to return from a "dot" command script where the "dot" command itself was located inside a function. This behaves badly when using FreeBSD's /bin/sh. An attempt was made to fix this in Git v1.8.4.1, but it only addressed part of the problem. Patch 1/3 fixes the problem by eliminating such use of "return", patch 2/3 then reverts the earlier workaround since it is no longer needed and did not fully solve the problem. The t5560 issue was first introduced in Git v1.7.0 which started using a backslash escapes in ${...} expansions. The FreeBSD /bin/sh does not properly support such escapes, but there is a simple change that works everywhere (using [?] instead of \?). There are more details in the individual patches. This patch series is based on maint since these are bug fixes and that's what SubmittingPatches says to do... Kyle J. McKay (3): rebase: avoid non-function use of "return" on FreeBSD Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD" test: fix t5560 on FreeBSD git-rebase--am.sh | 131 +++++++-------- git-rebase--interactive.sh | 341 ++++++++++++++++++++------------------- git-rebase--merge.sh | 87 +++++----- git-rebase.sh | 11 +- t/t5560-http-backend-noserver.sh | 4 +- 5 files changed, 287 insertions(+), 287 deletions(-) The above stat may seem like a lot, but when using diff -w --stat you get this: git-rebase--am.sh | 3 +++ git-rebase--interactive.sh | 3 +++ git-rebase--merge.sh | 3 +++ git-rebase.sh | 11 +---------- t/t5560-http-backend-noserver.sh | 4 ++-- 5 files changed, 12 insertions(+), 12 deletions(-) which more accurately reflects what was actually touched. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 8:28 [PATCH 0/3] Fix support for FreeBSD's /bin/sh Kyle J. McKay @ 2014-04-11 8:28 ` Kyle J. McKay 2014-04-11 8:48 ` Matthieu Moy 2014-04-14 22:51 ` Junio C Hamano 2014-04-11 8:28 ` [PATCH 2/3] Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD" Kyle J. McKay 2014-04-11 8:28 ` [PATCH 3/3] test: fix t5560 on FreeBSD Kyle J. McKay 2 siblings, 2 replies; 21+ messages in thread From: Kyle J. McKay @ 2014-04-11 8:28 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) the git-rebase--*.sh scripts have used a "return" to return from the "dot" command that runs them. While this is allowed by POSIX, the FreeBSD /bin/sh utility behaves poorly under some circumstances when such a "return" is executed. In particular, if the "dot" command is contained within a function, then when a "return" is executed by the script it runs (that is not itself inside a function), control will return from the function that contains the "dot" command skipping any statements that might follow the dot command inside that function. Commit 99855ddf (first appearing in v1.8.4.1) addresses this by making the "dot" command the last line in the function. Unfortunately the FreeBSD /bin/sh may also execute some statements in the script run by the "dot" command that appear after the troublesome "return". The fix in 99855ddf does not address this problem. For example, if you have script1.sh with these contents: #!/bin/sh # script1.sh run_script2() { . "$(dirname -- "$0")/script2.sh" _e=$? echo only this line should show [ $_e -eq 5 ] || echo expected status 5 got $_e return 3 } run_script2 e=$? [ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; } And script2.sh with these contents: # script2.sh if [ 5 -gt 3 ]; then return 5 fi case bad in *) echo always shows esac echo should not get here ! : When running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh' after making it executable), the expected output from a POSIX shell is simply the single line: only this line should show However, when run using FreeBSD's /bin/sh, the following output appears instead: should not get here expected status 3 got 1 Not only did the lines following the "dot" command in the run_script2 function in script1.sh get skipped, but additional lines in script2.sh following the "return" got executed -- but not all of them (e.g. the "echo always shows" line did not run). These issues can be avoided by not using a top-level "return" in script2.sh. If script2.sh is changed to this: # script2.sh fixed main() { if [ 5 -gt 3 ]; then return 5 fi case bad in *) echo always shows esac echo should not get here ! : } main Then it behaves the same when using FreeBSD's /bin/sh as when using other more POSIX compliant /bin/sh implementations. We fix the git-rebase--*.sh scripts in a similar fashion by moving the top-level code that contains "return" statements into its own function and then calling that as the last line in the script. The changes introduced by this commit are best viewed with the --ignore-all-space (-w) diff option. Signed-off-by: Kyle J. McKay <mackyle@gmail.com> --- git-rebase--am.sh | 117 ++++++++-------- git-rebase--interactive.sh | 335 +++++++++++++++++++++++---------------------- git-rebase--merge.sh | 87 ++++++------ 3 files changed, 274 insertions(+), 265 deletions(-) For convenience, here are the diffs using -w: |--- a/git-rebase--am.sh |+++ b/git-rebase--am.sh |@@ -4,6 +4,7 @@ # Copyright (c) 2010 Junio C Hamano. # +git_rebase__am() { case "$action" in continue) git am --resolved --resolvemsg="$resolvemsg" && |@@ -73,3 +74,5 @@ then fi move_to_original_branch +} +git_rebase__am |--- a/git-rebase--interactive.sh |+++ b/git-rebase--interactive.sh |@@ -810,6 +810,7 @@ add_exec_commands () { mv "$1.new" "$1" } +git_rebase__interactive() { case "$action" in continue) # do we have anything to commit? |@@ -1042,3 +1043,5 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" output git checkout $onto || die_abort "could not detach HEAD" git update-ref ORIG_HEAD $orig_head do_rest +} +git_rebase__interactive |--- a/git-rebase--merge.sh |+++ b/git-rebase--merge.sh |@@ -101,6 +101,7 @@ finish_rb_merge () { say All done. } +git_rebase__merge() { case "$action" in continue) read_state |@@ -151,3 +152,5 @@ do done finish_rb_merge +} +git_rebase__merge diff --git a/git-rebase--am.sh b/git-rebase--am.sh index a4f683a5..2d3f6d55 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -4,72 +4,75 @@ # Copyright (c) 2010 Junio C Hamano. # -case "$action" in -continue) - git am --resolved --resolvemsg="$resolvemsg" && - move_to_original_branch - return - ;; -skip) - git am --skip --resolvemsg="$resolvemsg" && - move_to_original_branch - return - ;; -esac - -test -n "$rebase_root" && root_flag=--root - -ret=0 -if test -n "$keep_empty" -then - # we have to do this the hard way. git format-patch completely squashes - # empty commits and even if it didn't the format doesn't really lend - # itself well to recording empty patches. fortunately, cherry-pick - # makes this easy - git cherry-pick --allow-empty "$revisions" - ret=$? -else - rm -f "$GIT_DIR/rebased-patches" +git_rebase__am() { + case "$action" in + continue) + git am --resolved --resolvemsg="$resolvemsg" && + move_to_original_branch + return + ;; + skip) + git am --skip --resolvemsg="$resolvemsg" && + move_to_original_branch + return + ;; + esac - git format-patch -k --stdout --full-index --ignore-if-in-upstream \ - --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \ - $root_flag "$revisions" >"$GIT_DIR/rebased-patches" - ret=$? + test -n "$rebase_root" && root_flag=--root - if test 0 != $ret + ret=0 + if test -n "$keep_empty" then + # we have to do this the hard way. git format-patch completely squashes + # empty commits and even if it didn't the format doesn't really lend + # itself well to recording empty patches. fortunately, cherry-pick + # makes this easy + git cherry-pick --allow-empty "$revisions" + ret=$? + else rm -f "$GIT_DIR/rebased-patches" - case "$head_name" in - refs/heads/*) - git checkout -q "$head_name" - ;; - *) - git checkout -q "$orig_head" - ;; - esac - cat >&2 <<-EOF + git format-patch -k --stdout --full-index --ignore-if-in-upstream \ + --src-prefix=a/ --dst-prefix=b/ --no-renames --no-cover-letter \ + $root_flag "$revisions" >"$GIT_DIR/rebased-patches" + ret=$? - git encountered an error while preparing the patches to replay - these revisions: + if test 0 != $ret + then + rm -f "$GIT_DIR/rebased-patches" + case "$head_name" in + refs/heads/*) + git checkout -q "$head_name" + ;; + *) + git checkout -q "$orig_head" + ;; + esac - $revisions + cat >&2 <<-EOF - As a result, git cannot rebase them. - EOF - return $? - fi + git encountered an error while preparing the patches to replay + these revisions: + + $revisions - git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches" - ret=$? + As a result, git cannot rebase them. + EOF + return $? + fi - rm -f "$GIT_DIR/rebased-patches" -fi + git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches" + ret=$? -if test 0 != $ret -then - test -d "$state_dir" && write_basic_state - return $ret -fi + rm -f "$GIT_DIR/rebased-patches" + fi -move_to_original_branch + if test 0 != $ret + then + test -d "$state_dir" && write_basic_state + return $ret + fi + + move_to_original_branch +} +git_rebase__am diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 43631b47..42164f11 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -810,16 +810,17 @@ add_exec_commands () { mv "$1.new" "$1" } -case "$action" in -continue) - # do we have anything to commit? - if git diff-index --cached --quiet HEAD -- - then - : Nothing to commit -- skip this - else - if ! test -f "$author_script" +git_rebase__interactive() { + case "$action" in + continue) + # do we have anything to commit? + if git diff-index --cached --quiet HEAD -- then - die "You have staged changes in your working tree. If these changes are meant to be + : Nothing to commit -- skip this + else + if ! test -f "$author_script" + then + die "You have staged changes in your working tree. If these changes are meant to be squashed into the previous commit, run: git commit --amend @@ -832,42 +833,42 @@ In both case, once you're done, continue with: git rebase --continue " - fi - . "$author_script" || - die "Error trying to find the author identity to amend commit" - if test -f "$amend" - then - current_head=$(git rev-parse --verify HEAD) - test "$current_head" = $(cat "$amend") || - die "\ + fi + . "$author_script" || + die "Error trying to find the author identity to amend commit" + if test -f "$amend" + then + current_head=$(git rev-parse --verify HEAD) + test "$current_head" = $(cat "$amend") || + die "\ You have uncommitted changes in your working tree. Please, commit them first and then run 'git rebase --continue' again." - do_with_author git commit --amend --no-verify -F "$msg" -e || - die "Could not commit staged changes." - else - do_with_author git commit --no-verify -F "$msg" -e || - die "Could not commit staged changes." + do_with_author git commit --amend --no-verify -F "$msg" -e || + die "Could not commit staged changes." + else + do_with_author git commit --no-verify -F "$msg" -e || + die "Could not commit staged changes." + fi fi - fi - record_in_rewritten "$(cat "$state_dir"/stopped-sha)" + record_in_rewritten "$(cat "$state_dir"/stopped-sha)" - require_clean_work_tree "rebase" - do_rest - return 0 - ;; -skip) - git rerere clear + require_clean_work_tree "rebase" + do_rest + return 0 + ;; + skip) + git rerere clear - do_rest - return 0 - ;; -edit-todo) - git stripspace --strip-comments <"$todo" >"$todo".new - mv -f "$todo".new "$todo" - collapse_todo_ids - append_todo_help - git stripspace --comment-lines >>"$todo" <<\EOF + do_rest + return 0 + ;; + edit-todo) + git stripspace --strip-comments <"$todo" >"$todo".new + mv -f "$todo".new "$todo" + collapse_todo_ids + append_todo_help + git stripspace --comment-lines >>"$todo" <<\EOF You are editing the todo file of an ongoing interactive rebase. To continue rebase after editing, run: @@ -875,170 +876,172 @@ To continue rebase after editing, run: EOF - git_sequence_editor "$todo" || - die "Could not execute editor" - expand_todo_ids + git_sequence_editor "$todo" || + die "Could not execute editor" + expand_todo_ids - exit - ;; -esac + exit + ;; + esac -git var GIT_COMMITTER_IDENT >/dev/null || - die "You need to set your committer info first" + git var GIT_COMMITTER_IDENT >/dev/null || + die "You need to set your committer info first" -comment_for_reflog start + comment_for_reflog start -if test ! -z "$switch_to" -then - GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" - output git checkout "$switch_to" -- || - die "Could not checkout $switch_to" + if test ! -z "$switch_to" + then + GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to" + output git checkout "$switch_to" -- || + die "Could not checkout $switch_to" - comment_for_reflog start -fi + comment_for_reflog start + fi -orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?" -mkdir -p "$state_dir" || die "Could not create temporary $state_dir" + orig_head=$(git rev-parse --verify HEAD) || die "No HEAD?" + mkdir -p "$state_dir" || die "Could not create temporary $state_dir" -: > "$state_dir"/interactive || die "Could not mark as interactive" -write_basic_state -if test t = "$preserve_merges" -then - if test -z "$rebase_root" + : > "$state_dir"/interactive || die "Could not mark as interactive" + write_basic_state + if test t = "$preserve_merges" then - mkdir "$rewritten" && - for c in $(git merge-base --all $orig_head $upstream) - do - echo $onto > "$rewritten"/$c || + if test -z "$rebase_root" + then + mkdir "$rewritten" && + for c in $(git merge-base --all $orig_head $upstream) + do + echo $onto > "$rewritten"/$c || + die "Could not init rewritten commits" + done + else + mkdir "$rewritten" && + echo $onto > "$rewritten"/root || die "Could not init rewritten commits" - done + fi + # No cherry-pick because our first pass is to determine + # parents to rewrite and skipping dropped commits would + # prematurely end our probe + merges_option= else - mkdir "$rewritten" && - echo $onto > "$rewritten"/root || - die "Could not init rewritten commits" + merges_option="--no-merges --cherry-pick" fi - # No cherry-pick because our first pass is to determine - # parents to rewrite and skipping dropped commits would - # prematurely end our probe - merges_option= -else - merges_option="--no-merges --cherry-pick" -fi -shorthead=$(git rev-parse --short $orig_head) -shortonto=$(git rev-parse --short $onto) -if test -z "$rebase_root" - # this is now equivalent to ! -z "$upstream" -then - shortupstream=$(git rev-parse --short $upstream) - revisions=$upstream...$orig_head - shortrevisions=$shortupstream..$shorthead -else - revisions=$onto...$orig_head - shortrevisions=$shorthead -fi -git rev-list $merges_option --pretty=oneline --abbrev-commit \ - --abbrev=7 --reverse --left-right --topo-order \ - $revisions | \ - sed -n "s/^>//p" | -while read -r shortsha1 rest -do - - if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1 + shorthead=$(git rev-parse --short $orig_head) + shortonto=$(git rev-parse --short $onto) + if test -z "$rebase_root" + # this is now equivalent to ! -z "$upstream" then - comment_out="$comment_char " + shortupstream=$(git rev-parse --short $upstream) + revisions=$upstream...$orig_head + shortrevisions=$shortupstream..$shorthead else - comment_out= + revisions=$onto...$orig_head + shortrevisions=$shorthead fi + git rev-list $merges_option --pretty=oneline --abbrev-commit \ + --abbrev=7 --reverse --left-right --topo-order \ + $revisions | \ + sed -n "s/^>//p" | + while read -r shortsha1 rest + do - if test t != "$preserve_merges" - then - printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo" - else - sha1=$(git rev-parse $shortsha1) - if test -z "$rebase_root" + if test -z "$keep_empty" && is_empty_commit $shortsha1 && ! is_merge_commit $shortsha1 then - preserve=t - for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) - do - if test -f "$rewritten"/$p - then - preserve=f - fi - done + comment_out="$comment_char " else - preserve=f - fi - if test f = "$preserve" - then - touch "$rewritten"/$sha1 - printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo" + comment_out= fi - fi -done -# Watch for commits that been dropped by --cherry-pick -if test t = "$preserve_merges" -then - mkdir "$dropped" - # Save all non-cherry-picked changes - git rev-list $revisions --left-right --cherry-pick | \ - sed -n "s/^>//p" > "$state_dir"/not-cherry-picks - # Now all commits and note which ones are missing in - # not-cherry-picks and hence being dropped - git rev-list $revisions | - while read rev - do - if test -f "$rewritten"/$rev -a "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = "" + if test t != "$preserve_merges" then - # Use -f2 because if rev-list is telling us this commit is - # not worthwhile, we don't want to track its multiple heads, - # just the history of its first-parent for others that will - # be rebasing on top of it - git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev - short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev) - sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" - rm "$rewritten"/$rev + printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo" + else + sha1=$(git rev-parse $shortsha1) + if test -z "$rebase_root" + then + preserve=t + for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -s -f2-) + do + if test -f "$rewritten"/$p + then + preserve=f + fi + done + else + preserve=f + fi + if test f = "$preserve" + then + touch "$rewritten"/$sha1 + printf '%s\n' "${comment_out}pick $shortsha1 $rest" >>"$todo" + fi fi done -fi -test -s "$todo" || echo noop >> "$todo" -test -n "$autosquash" && rearrange_squash "$todo" -test -n "$cmd" && add_exec_commands "$todo" + # Watch for commits that been dropped by --cherry-pick + if test t = "$preserve_merges" + then + mkdir "$dropped" + # Save all non-cherry-picked changes + git rev-list $revisions --left-right --cherry-pick | \ + sed -n "s/^>//p" > "$state_dir"/not-cherry-picks + # Now all commits and note which ones are missing in + # not-cherry-picks and hence being dropped + git rev-list $revisions | + while read rev + do + if test -f "$rewritten"/$rev -a "$(sane_grep "$rev" "$state_dir"/not-cherry-picks)" = "" + then + # Use -f2 because if rev-list is telling us this commit is + # not worthwhile, we don't want to track its multiple heads, + # just the history of its first-parent for others that will + # be rebasing on top of it + git rev-list --parents -1 $rev | cut -d' ' -s -f2 > "$dropped"/$rev + short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev) + sane_grep -v "^[a-z][a-z]* $short" <"$todo" > "${todo}2" ; mv "${todo}2" "$todo" + rm "$rewritten"/$rev + fi + done + fi + + test -s "$todo" || echo noop >> "$todo" + test -n "$autosquash" && rearrange_squash "$todo" + test -n "$cmd" && add_exec_commands "$todo" -cat >>"$todo" <<EOF + cat >>"$todo" <<EOF $comment_char Rebase $shortrevisions onto $shortonto EOF -append_todo_help -git stripspace --comment-lines >>"$todo" <<\EOF + append_todo_help + git stripspace --comment-lines >>"$todo" <<\EOF However, if you remove everything, the rebase will be aborted. EOF -if test -z "$keep_empty" -then - printf '%s\n' "$comment_char Note that empty commits are commented out" >>"$todo" -fi + if test -z "$keep_empty" + then + printf '%s\n' "$comment_char Note that empty commits are commented out" >>"$todo" + fi -has_action "$todo" || - die_abort "Nothing to do" + has_action "$todo" || + die_abort "Nothing to do" -cp "$todo" "$todo".backup -git_sequence_editor "$todo" || - die_abort "Could not execute editor" + cp "$todo" "$todo".backup + git_sequence_editor "$todo" || + die_abort "Could not execute editor" -has_action "$todo" || - die_abort "Nothing to do" + has_action "$todo" || + die_abort "Nothing to do" -expand_todo_ids + expand_todo_ids -test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks + test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks -GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" -output git checkout $onto || die_abort "could not detach HEAD" -git update-ref ORIG_HEAD $orig_head -do_rest + GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" + output git checkout $onto || die_abort "could not detach HEAD" + git update-ref ORIG_HEAD $orig_head + do_rest +} +git_rebase__interactive diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index e7d96de9..b5f05bf5 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -101,53 +101,56 @@ finish_rb_merge () { say All done. } -case "$action" in -continue) - read_state - continue_merge - while test "$msgnum" -le "$end" - do - call_merge "$msgnum" +git_rebase__merge() { + case "$action" in + continue) + read_state continue_merge + while test "$msgnum" -le "$end" + do + call_merge "$msgnum" + continue_merge + done + finish_rb_merge + return + ;; + skip) + read_state + git rerere clear + msgnum=$(($msgnum + 1)) + while test "$msgnum" -le "$end" + do + call_merge "$msgnum" + continue_merge + done + finish_rb_merge + return + ;; + esac + + mkdir -p "$state_dir" + echo "$onto_name" > "$state_dir/onto_name" + write_basic_state + + msgnum=0 + for cmt in `git rev-list --reverse --no-merges "$revisions"` + do + msgnum=$(($msgnum + 1)) + echo "$cmt" > "$state_dir/cmt.$msgnum" done - finish_rb_merge - return - ;; -skip) - read_state - git rerere clear - msgnum=$(($msgnum + 1)) + + echo 1 >"$state_dir/msgnum" + echo $msgnum >"$state_dir/end" + + end=$msgnum + msgnum=1 + while test "$msgnum" -le "$end" do call_merge "$msgnum" continue_merge done - finish_rb_merge - return - ;; -esac - -mkdir -p "$state_dir" -echo "$onto_name" > "$state_dir/onto_name" -write_basic_state - -msgnum=0 -for cmt in `git rev-list --reverse --no-merges "$revisions"` -do - msgnum=$(($msgnum + 1)) - echo "$cmt" > "$state_dir/cmt.$msgnum" -done - -echo 1 >"$state_dir/msgnum" -echo $msgnum >"$state_dir/end" - -end=$msgnum -msgnum=1 -while test "$msgnum" -le "$end" -do - call_merge "$msgnum" - continue_merge -done - -finish_rb_merge + finish_rb_merge +} +git_rebase__merge -- tg: (0bc85abb..) t/freebsd-sh-return (depends on: maint) ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 8:28 ` [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD Kyle J. McKay @ 2014-04-11 8:48 ` Matthieu Moy 2014-04-11 14:29 ` Kyle J. McKay 2014-04-14 22:51 ` Junio C Hamano 1 sibling, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2014-04-11 8:48 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > If script2.sh is changed to this: > > # script2.sh fixed > main() { > if [ 5 -gt 3 ]; then > return 5 > fi > case bad in *) > echo always shows > esac > echo should not get here > ! : > } > main Wouldn't it be better to just stop using . within function? The .-ed script could define the complete function, and then the function would be used from the toplevel script. If I understand correctly, your version uses nested functions with file inclusion between both levels of nesting. That might work for the shells you tested, but if the goal is to avoid using tricky features that may trigger bugs on some shells, that seems backward. IOW, why not move the whole run_specific_rebase_internal function to git-rebase--$type? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 8:48 ` Matthieu Moy @ 2014-04-11 14:29 ` Kyle J. McKay 2014-04-11 17:30 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Kyle J. McKay @ 2014-04-11 14:29 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine On Apr 11, 2014, at 01:48, Matthieu Moy wrote: > "Kyle J. McKay" <mackyle@gmail.com> writes: > >> If script2.sh is changed to this: >> >> # script2.sh fixed >> main() { >> if [ 5 -gt 3 ]; then >> return 5 >> fi >> case bad in *) >> echo always shows >> esac >> echo should not get here >> ! : >> } >> main > > Wouldn't it be better to just stop using . within function? > > The .-ed script could define the complete function, and then the > function would be used from the toplevel script. > > If I understand correctly, your version uses nested functions with > file > inclusion between both levels of nesting. That might work for the > shells > you tested, but if the goal is to avoid using tricky features that may > trigger bugs on some shells, that seems backward. There are already nested functions with file inclusion between both levels of nesting in git-rebase--interactive.sh and git-rebase-- merge.sh now, so it's not introducing anything new. > IOW, why not move the whole run_specific_rebase_internal function to > git-rebase--$type? So what change are you proposing exactly? The current code in maint does this: git-rebase.sh: top-level git-rebase.sh: run_specific_rebase() git-rebase.sh: run_specific_rebase_internal() -- contains "dot" git-rebase--$type.sh: top-level -- has "return" To make the kind of change I think you're proposing would be somewhat more invasive than the proposed patch. Each of the git-rebase--$type scripts would have to be modified not to do anything other than define functions when included which would require some code movement to avoid having two "main" functions -- either that or there need to be multiple "dot" includes in git-rebase.sh so the code not in a function only executes when the selected case that uses it is active -- or the entire contents of each git-rebase--$type script gets indented and becomes a function definition, but that would introduce functions defining functions which seems like it would add use of a new tricky feature not previously used. I'm not saying it's a bad idea, it's just somewhat more invasive than simply inserting 3 lines. --Kyle ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 14:29 ` Kyle J. McKay @ 2014-04-11 17:30 ` Matthieu Moy 2014-04-11 23:08 ` Kyle J. McKay 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2014-04-11 17:30 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > There are already nested functions with file inclusion between both > levels of nesting in git-rebase--interactive.sh and git-rebase-- > merge.sh now, so it's not introducing anything new. OK, so it's less serious than I thought. But still, we're introducing a function with 3 levels of nesting, split accross files, in an area where we know that at least one shell is buggy ... >> IOW, why not move the whole run_specific_rebase_internal function to >> git-rebase--$type? > > So what change are you proposing exactly? Something along the lines of this: diff --git a/git-rebase--am.sh b/git-rebase--am.sh index df46f4c..4f7b22d 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -4,6 +4,8 @@ # Copyright (c) 2010 Junio C Hamano. # +run_specific_rebase_infile() { + case "$action" in continue) git am --resolved --resolvemsg="$resolvemsg" \ @@ -75,3 +77,4 @@ then fi move_to_original_branch +} [ Same patch for other git-rebase--*.sh variants] index 76f7f71..1a150bd 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -186,7 +186,7 @@ run_specific_rebase_internal () { # run_specific_rebase_internal has the file inclusion as a # last statement, so POSIX and FreeBSD's return will do the # same thing. - . git-rebase--$type + run_specific_rebase_infile } run_specific_rebase () { @@ -438,6 +438,8 @@ else state_dir="$apply_dir" fi +. git-rebase--$type + if test -z "$rebase_root" then case "$#" in I minimized patch size, so it would obviously need a reidentation, and would require some cleanup so that run_specific_rebase_internal is merged back into run_specific_rebase (a bit like your PATCH 2). I find the result simpler, just using the basic pattern "use '. file' to import a set of functions, and then use these functions". The real patch is a bit more tricky though, because we need to run the ". git-rebase--$type" after computing type properly. A patch passing the tests but requiring cleanup is given below. > To make the kind of change I think you're proposing would be somewhat > more invasive than the proposed patch. Each of the git-rebase--$type > scripts would have to be modified not to do anything other than define > functions That's almost what your patch does already: move everything into a function, and call it. Except, I'd put the function call outside the file inclusion. diff --git a/git-rebase--am.sh b/git-rebase--am.sh index df46f4c..4f7b22d 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -4,6 +4,8 @@ # Copyright (c) 2010 Junio C Hamano. # +run_specific_rebase_infile() { + case "$action" in continue) git am --resolved --resolvemsg="$resolvemsg" \ @@ -75,3 +77,4 @@ then fi move_to_original_branch +} diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 6046778..5dfbf14 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -820,6 +820,7 @@ add_exec_commands () { mv "$1.new" "$1" } +run_specific_rebase_infile() { case "$action" in continue) # do we have anything to commit? @@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" output git checkout $onto || die_abort "could not detach HEAD" git update-ref ORIG_HEAD $orig_head do_rest +} diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index d84f412..907aa46 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -99,6 +99,7 @@ finish_rb_merge () { say All done. } +run_specific_rebase_infile () { case "$action" in continue) read_state @@ -149,3 +150,4 @@ do done finish_rb_merge +} diff --git a/git-rebase.sh b/git-rebase.sh index 76f7f71..63e0e68 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -186,7 +186,7 @@ run_specific_rebase_internal () { # run_specific_rebase_internal has the file inclusion as a # last statement, so POSIX and FreeBSD's return will do the # same thing. - . git-rebase--$type + run_specific_rebase_infile } run_specific_rebase () { @@ -366,6 +366,29 @@ then die "$(gettext "The --edit-todo action can only be used during interactive rebase.")" fi +if test -n "$rebase_root" && test -z "$onto" +then + test -z "$interactive_rebase" && interactive_rebase=implied +fi + +if test -z "$in_progress" +then + if test -n "$interactive_rebase" + then + type=interactive + state_dir="$merge_dir" + elif test -n "$do_merge" + then + type=merge + state_dir="$merge_dir" + else + type=am + state_dir="$apply_dir" + fi +fi + +. git-rebase--$type + case "$action" in continue) # Sanity check @@ -420,24 +443,6 @@ and run me again. I am stopping in case you still have something valuable there.')" fi -if test -n "$rebase_root" && test -z "$onto" -then - test -z "$interactive_rebase" && interactive_rebase=implied -fi - -if test -n "$interactive_rebase" -then - type=interactive - state_dir="$merge_dir" -elif test -n "$do_merge" -then - type=merge - state_dir="$merge_dir" -else - type=am - state_dir="$apply_dir" -fi - if test -z "$rebase_root" then case "$#" in -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 17:30 ` Matthieu Moy @ 2014-04-11 23:08 ` Kyle J. McKay 2014-04-12 17:07 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Kyle J. McKay @ 2014-04-11 23:08 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine On Apr 11, 2014, at 10:30, Matthieu Moy wrote: > "Kyle J. McKay" <mackyle@gmail.com> writes: > >> There are already nested functions with file inclusion between both >> levels of nesting in git-rebase--interactive.sh and git-rebase-- >> merge.sh now, so it's not introducing anything new. > > OK, so it's less serious than I thought. But still, we're > introducing a > function with 3 levels of nesting, split accross files, in an area > where > we know that at least one shell is buggy ... Currently in maint: The current code in maint does this: git-rebase.sh: top-level git-rebase.sh: run_specific_rebase() git-rebase.sh: run_specific_rebase_internal() -- contains "dot" git-rebase--interactive.sh: top-level (using --continue or -- skip) git-rebase--interactive.sh: do_rest git-rebase--interactive.sh: do_next git-rebase--interactive.sh: record_in_rewritten git-rebase--interactive.sh: flush_rewritten_pending So I really do not see the additional level of nesting as an issue since we've already got much more than 3 levels of nesting going on now. If nesting was going to be a problem, something would have broken already. In fact, since the follow on patch removes the run_specific_rebase_internal function what we would have after the originally proposed first two patches is: git-rebase.sh: top-level git-rebase.sh: run_specific_rebase() -- contains "dot" git-rebase--interactive.sh: top-level (using --continue or --skip) git-rebase--interactive.sh: git_rebase__interactive git-rebase--interactive.sh: do_rest git-rebase--interactive.sh: do_next git-rebase--interactive.sh: record_in_rewritten git-rebase--interactive.sh: flush_rewritten_pending Which has exactly the same nesting depth albeit the "dot" has moved up one level. >>> IOW, why not move the whole run_specific_rebase_internal function to >>> git-rebase--$type? >> >> So what change are you proposing exactly? > > Something along the lines of this: > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index 6046778..5dfbf14 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -820,6 +820,7 @@ add_exec_commands () { > mv "$1.new" "$1" > } > > +run_specific_rebase_infile() { > case "$action" in > continue) > # do we have anything to commit? > @@ -1055,3 +1056,4 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: > checkout $onto_name" > output git checkout $onto || die_abort "could not detach HEAD" > git update-ref ORIG_HEAD $orig_head > do_rest > +} > diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh > index d84f412..907aa46 100644 > --- a/git-rebase--merge.sh > +++ b/git-rebase--merge.sh > @@ -99,6 +99,7 @@ finish_rb_merge () { > say All done. > } > > +run_specific_rebase_infile () { > case "$action" in > continue) > read_state > @@ -149,3 +150,4 @@ do > done > > finish_rb_merge > +} The problem with these changes, particularly the git-rebase-- interactive.sh one is that a bunch of code is still run when the file is "dot" included. With the changes to git-rebase.sh, that code will now run regardless of the action and it will run before it would have now. So if any of the variables it sets affect the functions like read_basic_state or finish_rebase (they don't currently appear to), then there's a potential for new bugs. That initial code would not previously have run in the --abort case at all. But, you say the tests pass with those changes, so the changes are probably okay. However, they create a potential situation where some code is added to the top of one of the git-rebase--$type.sh scripts and suddenly git rebase --abort stops working right because that code is being run when it shouldn't or the operation of read_basic_state and/or finish_rebase is adversely affected. Hopefully the rebase tests would catch any such issue right away though. So, in light of the fact that function nesting seems to be a non-issue here, and it seems to me the originally proposed changes have much less potential to introduce breakage either now or in the future, I still prefer them. --Kyle ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 23:08 ` Kyle J. McKay @ 2014-04-12 17:07 ` Matthieu Moy 2014-04-13 2:45 ` Kyle J. McKay 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2014-04-12 17:07 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > On Apr 11, 2014, at 10:30, Matthieu Moy wrote: >> "Kyle J. McKay" <mackyle@gmail.com> writes: >> >>> There are already nested functions with file inclusion between both >>> levels of nesting in git-rebase--interactive.sh and git-rebase-- >>> merge.sh now, so it's not introducing anything new. >> >> OK, so it's less serious than I thought. But still, we're >> introducing a >> function with 3 levels of nesting, split accross files, in an area >> where >> we know that at least one shell is buggy ... > > Currently in maint: > > The current code in maint does this: > > git-rebase.sh: top-level > git-rebase.sh: run_specific_rebase() > git-rebase.sh: run_specific_rebase_internal() -- contains "dot" > git-rebase--interactive.sh: top-level (using --continue or -- > skip) > git-rebase--interactive.sh: do_rest > git-rebase--interactive.sh: do_next You're confusing function calls and function nesting. do_rest calls do_next, but the definition of do_next is not nested within do_rest. When I talk about nested function, I mean f() { g() { ... } } Obviously, having functions call each other is not an issue. That's what functions are meant to be. Now, having run_specific_rebase_internal include a file which defines functions which contain nested functions _is_ something I find weird. It both stresses the shell in a buggy area and makes the code harder to understand. > The problem with these changes, particularly the git-rebase-- > interactive.sh one is that a bunch of code is still run when the file > is "dot" included. Function definitions, and variables assignments. Is it so much of an issue? What's the difference between a function definition or variable assignment within git-rebase--*.sh and within git-rebase.sh? -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-12 17:07 ` Matthieu Moy @ 2014-04-13 2:45 ` Kyle J. McKay 2014-04-14 8:24 ` Matthieu Moy 0 siblings, 1 reply; 21+ messages in thread From: Kyle J. McKay @ 2014-04-13 2:45 UTC (permalink / raw) To: Matthieu Moy; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine On Apr 12, 2014, at 10:07, Matthieu Moy wrote: > "Kyle J. McKay" <mackyle@gmail.com> writes: > >> On Apr 11, 2014, at 10:30, Matthieu Moy wrote: >>> "Kyle J. McKay" <mackyle@gmail.com> writes: >>> >>>> There are already nested functions with file inclusion between both >>>> levels of nesting in git-rebase--interactive.sh and git-rebase-- >>>> merge.sh now, so it's not introducing anything new. >>> >>> OK, so it's less serious than I thought. But still, we're >>> introducing a >>> function with 3 levels of nesting, split accross files, in an area >>> where >>> we know that at least one shell is buggy ... >> >> Currently in maint: >> >> The current code in maint does this: >> >> git-rebase.sh: top-level >> git-rebase.sh: run_specific_rebase() >> git-rebase.sh: run_specific_rebase_internal() -- contains "dot" >> git-rebase--interactive.sh: top-level (using --continue or -- >> skip) >> git-rebase--interactive.sh: do_rest >> git-rebase--interactive.sh: do_next > > You're confusing function calls and function nesting. do_rest calls > do_next, but the definition of do_next is not nested within do_rest. > > When I talk about nested function, I mean > > f() { > g() { > ... > } > } > > Obviously, having functions call each other is not an issue. That's > what > functions are meant to be. > > Now, having run_specific_rebase_internal include a file which defines > functions which contain nested functions _is_ something I find > weird. It > both stresses the shell in a buggy area and makes the code harder to > understand. I meant: "nested functions" = "nested function calls" You meant: "nested functions" = "nested function definitions" Okay. But nested function definitions is not something new to the rebase code. >> The problem with these changes, particularly the git-rebase-- >> interactive.sh one is that a bunch of code is still run when the file >> is "dot" included. > > Function definitions, and variables assignments. Is it so much of an > issue? > > What's the difference between a function definition or variable > assignment within git-rebase--*.sh and within git-rebase.sh? As I said, this is the issue: On Apr 11, 2014, at 16:08, Kyle J. McKay wrote: > The problem with these changes, particularly the git-rebase-- > interactive.sh one is that a bunch of code is still run when the > file is "dot" included. With the changes to git-rebase.sh, that > code will now run regardless of the action and it will run before it > would have now. So if any of the variables it sets affect the > functions like read_basic_state or finish_rebase (they don't > currently appear to), then there's a potential for new bugs. That > initial code would not previously have run in the --abort case at all. Let me rephrase. Patch 1/3 does not change the order in which individual statements are executed in the rebase code. Nor does it change the logic. It simply introduces one additional function callstack level, but the same individual statements are executed in the same order for all control flows. No additional statements other than the introduced callstack level. Nothing's executed in a different order. No control paths execute additional statements they did not before. The changes you propose introduce exactly the same additional function callstack level. Then they proceed to alter the order in which statements are executed. Statements that did not execute in some control paths before are now executed in those control paths. Other statements are moved around to execute earlier/later than they did before. My point is not that this is wrong. It's nice, really, to move the "dot" include to the top level. The point is that's not necessary to fix the problem and moving statements around and adding statements to some control paths increases the risk of introducing an uncaught bug when it's not necessary to fix the problem. I would like to get a fix in so that rebase works out-of-the-box when Git version 1.8.4 or later is built on FreeBSD. So I'm not going to belabor the point anymore. There's a follow-up patch 4/3 attached to the end of this message that makes the changes you have proposed in your earlier email on top of patches 1/3 and 2/3. The log message and all changes are taken from your emails and so this patch is assigned to you and has a 'Needs-Signed-off-by:' line. On Apr 11, 2014, at 10:30, Matthieu Moy wrote: > The real patch is a bit more tricky though, because we need to run the > ". git-rebase--$type" after computing type properly. A patch passing > the > tests but requiring cleanup is given below. Unfortunately, after applying the below patch, some of the rebase tests (3403, 3407, 3418, 3420, 3421) start failing for me on systems where they did not fail previously. Even though some of the previously failing tests on FreeBSD now pass with the patch, 3421 now fails on FreeBSD where it did not before. Here's a test summary of the t34xx*.sh tests: NOTE: at the time of these tests, maint and v1.9.2 were at the same commit. ______________SYSTEM____t34xx*.sh_results________________ maint FreeBSD FAIL: 3403,3404,3407,3409,3410,3412,3418,3419,3420 maint Darwin PASS maint Linux PASS maint+1-3/3 FreeBSD PASS maint+1-3/3 Darwin PASS maint+1-3/3 Linux PASS maint+1-4/3 FreeBSD FAIL[*]: 3403,3407,3418,3420,3421 maint+1-4/3 Darwin FAIL[*]: 3403,3407,3418,3420,3421 maint+1-4/3 Linux FAIL[*]: 3403,3407,3418,3420,3421 [*]: The failing test reports for all three systems are identical for the "maint+1-4/3" run (except for the wallclock secs part): Test Summary Report ------------------- t3403-rebase-skip.sh (Wstat: 256 Tests: 10 Failed: 3) Failed tests: 8-10 Non-zero exit status: 1 t3407-rebase-abort.sh (Wstat: 256 Tests: 11 Failed: 3) Failed tests: 7-9 Non-zero exit status: 1 t3418-rebase-continue.sh (Wstat: 256 Tests: 6 Failed: 2) Failed tests: 5-6 Non-zero exit status: 1 t3420-rebase-autostash.sh (Wstat: 256 Tests: 24 Failed: 11) Failed tests: 14-24 Non-zero exit status: 1 t3421-rebase-topology-linear.sh (Wstat: 256 Tests: 76 Failed: 16) Failed tests: 51-53, 55, 57, 59-63, 65, 67, 73-76 Non-zero exit status: 1 Files=22, Tests=374, 354 wallclock secs ( 0.31 usr 0.13 sys + 79.57 cusr 233.99 csys = 314.00 CPU) Result: FAIL make: *** [prove] Error 1 So I suggest that in the interest of fixing rebase on FreeBSD in an expeditious fashion, patches 1/3 and 2/3 get picked up (see note below) now and that the follow-on patch below, after being enhanced to pass all tests, be submitted separately at some future point. --Kyle P.S. Note to JCH: the below patch requires the previous 1/3 and 2/3 be applied first. As per SubmittingPatches for bug fixes those are based on maint. Because of the whitespace change 1/3 introduces it does not apply cleanly to master, next or pu. :( Please let me know if you would like me to resend the initial series (1 & 2 -- 3 has already been picked up) based on a different branch instead of maint. ---- 8< ---- From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Subject: [PATCH 4/3] rebase: stop using . within function Move the whole run_specific_rebase_internal function to git-rebase--$type. The .-ed script defines the complete function, and then the function is used from the toplevel script. The goal is to avoid using tricky features that may trigger bugs on some shells. The result is simpler, just using the basic pattern: 1. use '. file' to import a set of functions 2. then use these functions Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> --- git-rebase--am.sh | 3 +-- git-rebase--interactive.sh | 3 +-- git-rebase--merge.sh | 3 +-- git-rebase.sh | 40 +++++++++++++++++++++------------------- 4 files changed, 24 insertions(+), 25 deletions(-) diff --git a/git-rebase--am.sh b/git-rebase--am.sh index 2d3f6d55..b48b3e90 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -4,7 +4,7 @@ # Copyright (c) 2010 Junio C Hamano. # -git_rebase__am() { +run_specific_rebase_infile() { case "$action" in continue) git am --resolved --resolvemsg="$resolvemsg" && @@ -75,4 +75,3 @@ git_rebase__am() { move_to_original_branch } -git_rebase__am diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 42164f11..a7670eb0 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -810,7 +810,7 @@ add_exec_commands () { mv "$1.new" "$1" } -git_rebase__interactive() { +run_specific_rebase_infile() { case "$action" in continue) # do we have anything to commit? @@ -1044,4 +1044,3 @@ EOF git update-ref ORIG_HEAD $orig_head do_rest } -git_rebase__interactive diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index b5f05bf5..9550e656 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -101,7 +101,7 @@ finish_rb_merge () { say All done. } -git_rebase__merge() { +run_specific_rebase_infile() { case "$action" in continue) read_state @@ -153,4 +153,3 @@ git_rebase__merge() { finish_rb_merge } -git_rebase__merge diff --git a/git-rebase.sh b/git-rebase.sh index 07e2bd48..9e105626 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -175,7 +175,7 @@ run_specific_rebase () { export GIT_EDITOR autosquash= fi - . git-rebase--$type + run_specific_rebase_infile ret=$? if test $ret -eq 0 then @@ -353,6 +353,26 @@ then die "$(gettext "The --edit-todo action can only be used during interactive rebase.")" fi +if test -n "$rebase_root" && test -z "$onto" +then + test -z "$interactive_rebase" && interactive_rebase=implied +fi + +if test -n "$interactive_rebase" +then + type=interactive + state_dir="$merge_dir" +elif test -n "$do_merge" +then + type=merge + state_dir="$merge_dir" +else + type=am + state_dir="$apply_dir" +fi + +. git-rebase--$type + case "$action" in continue) # Sanity check @@ -407,24 +427,6 @@ and run me again. I am stopping in case you still have something valuable there.')" fi -if test -n "$rebase_root" && test -z "$onto" -then - test -z "$interactive_rebase" && interactive_rebase=implied -fi - -if test -n "$interactive_rebase" -then - type=interactive - state_dir="$merge_dir" -elif test -n "$do_merge" -then - type=merge - state_dir="$merge_dir" -else - type=am - state_dir="$apply_dir" -fi - if test -z "$rebase_root" then case "$#" in -- 1.8.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-13 2:45 ` Kyle J. McKay @ 2014-04-14 8:24 ` Matthieu Moy 2014-04-14 22:28 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Matthieu Moy @ 2014-04-14 8:24 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Junio C Hamano, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > So I suggest that in the interest of fixing rebase on FreeBSD in an > expeditious fashion, patches 1/3 and 2/3 get picked up (see note > below) now and that the follow-on patch below, after being enhanced to > pass all tests, be submitted separately at some future point. Seems a good plan to me. > Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > Subject: [PATCH 4/3] rebase: stop using . within function > > Move the whole run_specific_rebase_internal function to > git-rebase--$type. > > The .-ed script defines the complete function, and then the > function is used from the toplevel script. > > The goal is to avoid using tricky features that may trigger > bugs on some shells. > > The result is simpler, just using the basic pattern: > > 1. use '. file' to import a set of functions > 2. then use these functions > > Needs-Signed-off-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> > --- > git-rebase--am.sh | 3 +-- > git-rebase--interactive.sh | 3 +-- > git-rebase--merge.sh | 3 +-- > git-rebase.sh | 40 +++++++++++++++++++++------------------- > 4 files changed, 24 insertions(+), 25 deletions(-) > > diff --git a/git-rebase--am.sh b/git-rebase--am.sh > index 2d3f6d55..b48b3e90 100644 > --- a/git-rebase--am.sh > +++ b/git-rebase--am.sh > @@ -4,7 +4,7 @@ > # Copyright (c) 2010 Junio C Hamano. > # > > -git_rebase__am() { > +run_specific_rebase_infile() { > case "$action" in > continue) > git am --resolved --resolvemsg="$resolvemsg" && > @@ -75,4 +75,3 @@ git_rebase__am() { > > move_to_original_branch > } > -git_rebase__am > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index 42164f11..a7670eb0 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -810,7 +810,7 @@ add_exec_commands () { > mv "$1.new" "$1" > } > > -git_rebase__interactive() { > +run_specific_rebase_infile() { > case "$action" in > continue) > # do we have anything to commit? > @@ -1044,4 +1044,3 @@ EOF > git update-ref ORIG_HEAD $orig_head > do_rest > } > -git_rebase__interactive > diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh > index b5f05bf5..9550e656 100644 > --- a/git-rebase--merge.sh > +++ b/git-rebase--merge.sh > @@ -101,7 +101,7 @@ finish_rb_merge () { > say All done. > } > > -git_rebase__merge() { > +run_specific_rebase_infile() { > case "$action" in > continue) > read_state > @@ -153,4 +153,3 @@ git_rebase__merge() { > > finish_rb_merge > } > -git_rebase__merge > diff --git a/git-rebase.sh b/git-rebase.sh > index 07e2bd48..9e105626 100755 > --- a/git-rebase.sh > +++ b/git-rebase.sh > @@ -175,7 +175,7 @@ run_specific_rebase () { > export GIT_EDITOR > autosquash= > fi > - . git-rebase--$type > + run_specific_rebase_infile > ret=$? > if test $ret -eq 0 > then > @@ -353,6 +353,26 @@ then > die "$(gettext "The --edit-todo action can only be used during interactive rebase.")" > fi > > +if test -n "$rebase_root" && test -z "$onto" > +then > + test -z "$interactive_rebase" && interactive_rebase=implied > +fi > + > +if test -n "$interactive_rebase" > +then > + type=interactive > + state_dir="$merge_dir" > +elif test -n "$do_merge" > +then > + type=merge > + state_dir="$merge_dir" > +else > + type=am > + state_dir="$apply_dir" > +fi > + > +. git-rebase--$type > + > case "$action" in > continue) > # Sanity check > @@ -407,24 +427,6 @@ and run me again. I am stopping in case you still have something > valuable there.')" > fi > > -if test -n "$rebase_root" && test -z "$onto" > -then > - test -z "$interactive_rebase" && interactive_rebase=implied > -fi > - > -if test -n "$interactive_rebase" > -then > - type=interactive > - state_dir="$merge_dir" > -elif test -n "$do_merge" > -then > - type=merge > - state_dir="$merge_dir" > -else > - type=am > - state_dir="$apply_dir" > -fi > - > if test -z "$rebase_root" > then > case "$#" in > -- > 1.8.5 -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-14 8:24 ` Matthieu Moy @ 2014-04-14 22:28 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2014-04-14 22:28 UTC (permalink / raw) To: Matthieu Moy; +Cc: Kyle J. McKay, git, Ramkumar Ramachandra, Eric Sunshine Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes: > "Kyle J. McKay" <mackyle@gmail.com> writes: > >> So I suggest that in the interest of fixing rebase on FreeBSD in an >> expeditious fashion, patches 1/3 and 2/3 get picked up (see note >> below) now and that the follow-on patch below, after being enhanced to >> pass all tests, be submitted separately at some future point. > > Seems a good plan to me. OK, should I take that an Ack on 1 & 2? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-11 8:28 ` [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD Kyle J. McKay 2014-04-11 8:48 ` Matthieu Moy @ 2014-04-14 22:51 ` Junio C Hamano 2014-04-16 4:32 ` Kyle J. McKay 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2014-04-14 22:51 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > For convenience, here are the diffs using -w: > > |--- a/git-rebase--am.sh > |+++ b/git-rebase--am.sh > |@@ -4,6 +4,7 @@ > # Copyright (c) 2010 Junio C Hamano. > # > > +git_rebase__am() { > case "$action" in > continue) > git am --resolved --resolvemsg="$resolvemsg" && > |@@ -73,3 +74,5 @@ then > fi > > move_to_original_branch > +} > +git_rebase__am I think we would want to see the actual change formatted this way (without needing to pass "-w" to "git show"), as it will make it clear that this artificial extra level of "define the whole thing inside a function and then make a single call to it" is a workaround of specific shell's glitch that we would not have to have in an ideal world ;-) Besides that would make it less likely to cause conflicts with the real changes in flight. Please double check what I queued on 'pu' when I push out today's integration result. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-14 22:51 ` Junio C Hamano @ 2014-04-16 4:32 ` Kyle J. McKay 2014-04-16 16:47 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Kyle J. McKay @ 2014-04-16 4:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine On Apr 14, 2014, at 15:51, Junio C Hamano wrote: > I think we would want to see the actual change formatted this way > (without needing to pass "-w" to "git show"), as it will make it > clear that this artificial extra level of "define the whole thing > inside a function and then make a single call to it" is a workaround > of specific shell's glitch that we would not have to have in an > ideal world ;-) > > Besides that would make it less likely to cause conflicts with the > real changes in flight. Sounds good to me. > Please double check what I queued on 'pu' when I push out today's > integration result. > diff --git a/git-rebase--am.sh b/git-rebase--am.sh > index a4f683a5..2ab514ea 100644 > --- a/git-rebase--am.sh > +++ b/git-rebase--am.sh > @@ -4,6 +4,13 @@ > # Copyright (c) 2010 Junio C Hamano. > # > > +# The whole contents of the file is run by dot-sourcing this file > from > +# inside a shell function, and "return"s we see below are expected to > +# return from that function that dot-sources us. However, FreeBSD > +# /bin/sh misbehaves on such a construct, so we will work it around > by > +# enclosing the whole thing inside an extra layer of a function. > +git_rebase__am () { I think the wording may be just a bit off: > and "return"s we see below are expected to return from that function > that dot-sources us. I thought that was one of the buggy behaviors we are working around? Maybe I'm just reading it wrong... Does this wording seem any clearer? and "return"s we see below are expected not to return from the function that dot-sources us, but rather to the next command after the one in the function that dot-sources us. Otherwise the patch in pu looks fine (all t34*.sh tests pass for me on FreeBSD with pu at 8d8dc6db). Thank you for adding the comments. If I'm the only one getting a wrong meaning from the comments, then no reason to change them. --Kyle ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-16 4:32 ` Kyle J. McKay @ 2014-04-16 16:47 ` Junio C Hamano 2014-04-16 18:11 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2014-04-16 16:47 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > On Apr 14, 2014, at 15:51, Junio C Hamano wrote: >> I think we would want to see the actual change formatted this way >> (without needing to pass "-w" to "git show"), as it will make it >> clear that this artificial extra level of "define the whole thing >> inside a function and then make a single call to it" is a workaround >> of specific shell's glitch that we would not have to have in an >> ideal world ;-) >> >> Besides that would make it less likely to cause conflicts with the >> real changes in flight. > > Sounds good to me. > >> Please double check what I queued on 'pu' when I push out today's >> integration result. > >> diff --git a/git-rebase--am.sh b/git-rebase--am.sh >> index a4f683a5..2ab514ea 100644 >> --- a/git-rebase--am.sh >> +++ b/git-rebase--am.sh >> @@ -4,6 +4,13 @@ >> # Copyright (c) 2010 Junio C Hamano. >> # >> >> +# The whole contents of the file is run by dot-sourcing this file >> from >> +# inside a shell function, and "return"s we see below are expected to >> +# return from that function that dot-sources us. However, FreeBSD >> +# /bin/sh misbehaves on such a construct, so we will work it around >> by >> +# enclosing the whole thing inside an extra layer of a function. >> +git_rebase__am () { > > I think the wording may be just a bit off: > >> and "return"s we see below are expected to return from that function >> that dot-sources us. > > I thought that was one of the buggy behaviors we are working around? Yeah, it is written as if every reader has the knowledge that the extra extra__func () { ... } extra__func around did not originally exist. The description does not read well with the work-around already there. > Maybe I'm just reading it wrong... > > Does this wording seem any clearer? > > and "return"s we see below are expected not to return > from the function that dot-sources us, but rather to > the next command after the one in the function that > dot-sources us. Not really. The comment was not about explaining "return"s. When a reader reads the text with the work-around, it is clear that these "return"s are inside this extra function and there is no possible interpretation other than that they are to return from the extra function. The comment was meant to explain why a seemingly strange "define a function and then immediately call it just once" pattern is used, and "Earlier, these returns were not inside any function when this file is viewed standalone. Because this file is to be dot-sourced within a function, they were to return from that dot-sourcing function --- but some shells do not behave that way, hence this strange construct." was meant to be that explanation, but apparently it failed to convey what I meant to say. > If I'm the only one getting a wrong meaning from the comments, then no > reason to change them. I agree that the description does not read well with the work-around already there. I am not sure what would be a better way to phrase it, though. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-16 16:47 ` Junio C Hamano @ 2014-04-16 18:11 ` Junio C Hamano 2014-04-16 18:23 ` Junio C Hamano 2014-04-17 0:41 ` Kyle J. McKay 0 siblings, 2 replies; 21+ messages in thread From: Junio C Hamano @ 2014-04-16 18:11 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine Junio C Hamano <gitster@pobox.com> writes: > "Kyle J. McKay" <mackyle@gmail.com> writes: > >> If I'm the only one getting a wrong meaning from the comments, then no >> reason to change them. > > I agree that the description does not read well with the work-around > already there. I am not sure what would be a better way to phrase > it, though. Here is a tentative rewrite at the beginning and the end of rebase-am: # The whole contents of the file is run by dot-sourcing this # file from inside a shell function. It used to be that # "return"s we see below were not inside any function, and # expected to return from the function that dot-sourced us. # # However, FreeBSD /bin/sh misbehaves on such a construct and # continues to run the statements that follow such a "return". # As a work-around, we introduce an extra layer of a function # here, and immediately call it after defining it. git_rebase__am () { ... } # ... and then we call the whole thing. git_rebase__am By the way, you have this in your log message: ... the git-rebase--*.sh scripts have used a "return" to return from the "dot" command that runs them. While this is allowed by POSIX,... Is it "this is allowed", or is it "this should be the way and shells that do not do so are buggy"? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-16 18:11 ` Junio C Hamano @ 2014-04-16 18:23 ` Junio C Hamano 2014-04-17 0:41 ` Kyle J. McKay 1 sibling, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2014-04-16 18:23 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine Junio C Hamano <gitster@pobox.com> writes: > By the way, you have this in your log message: > > ... the git-rebase--*.sh scripts have used a "return" to return > from the "dot" command that runs them. While this is allowed by > POSIX,... > > > Is it "this is allowed", or is it "this should be the way and shells > that do not do so are buggy"? Answering myself... The only "unspecified" I see is this: If the shell is not currently executing a function or dot script, the results are unspecified. which clearly does not apply to the version before this patch (we are executing a dot script). And The return utility shall cause the shell to stop executing the current function or dot script. would mean that we are correct to expect that "should not get here" is not reached, as the "return 5" would cause the shell to stop executing the dot script there. So "while this is allowed by POSIX" may be a bit misleading and needs to be reworded, I guess? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-16 18:11 ` Junio C Hamano 2014-04-16 18:23 ` Junio C Hamano @ 2014-04-17 0:41 ` Kyle J. McKay 2014-04-17 17:15 ` Junio C Hamano 1 sibling, 1 reply; 21+ messages in thread From: Kyle J. McKay @ 2014-04-17 0:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine On Apr 16, 2014, at 11:11, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> "Kyle J. McKay" <mackyle@gmail.com> writes: >> >>> If I'm the only one getting a wrong meaning from the comments, >>> then no >>> reason to change them. >> >> I agree that the description does not read well with the work-around >> already there. I am not sure what would be a better way to phrase >> it, though. > > Here is a tentative rewrite at the beginning and the end of rebase-am: > > # The whole contents of the file is run by dot-sourcing this > # file from inside a shell function. It used to be that > # "return"s we see below were not inside any function, and > # expected to return from the function that dot-sourced us. I think it's the "return from the function that dot-sourced us" that gives me the wrong meaning. The "return from" part says to me that function will be returning which it will not unless the dot command was the last command in the function. Either "return to the function that dot-sourced us" or "return from the dot command that dot-sourced us", but using the original wording implies to me that the function that dot-sourced us will return as soon as the dot-sourced script executes the return and that is exactly one of the bugs we're working around. I think just the s/from/to/ would fix it so it does not give me the wrong impression, but that doesn't mean that would not confuse everyone else. ;) > # > # However, FreeBSD /bin/sh misbehaves on such a construct and > # continues to run the statements that follow such a "return". > # As a work-around, we introduce an extra layer of a function > # here, and immediately call it after defining it. > git_rebase__am () { > > ... > > } > # ... and then we call the whole thing. > git_rebase__am On Apr 16, 2014, at 11:23, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > By the way, you have this in your log message: >> >> ... the git-rebase--*.sh scripts have used a "return" to return >> from the "dot" command that runs them. While this is allowed by >> POSIX,... >> >> >> Is it "this is allowed", or is it "this should be the way and shells >> that do not do so are buggy"? > > Answering myself... > > The only "unspecified" I see is this: > > If the shell is not currently executing a function or dot > script, the results are unspecified. > > which clearly does not apply to the version before this patch (we > are executing a dot script). And > > The return utility shall cause the shell to stop executing the > current function or dot script. > > would mean that we are correct to expect that "should not get here" > is not reached, as the "return 5" would cause the shell to stop > executing the dot script there. > > So "while this is allowed by POSIX" may be a bit misleading and > needs to be reworded, I guess? "allowed by POSIX" is not incorrect. ;) But perhaps the log message should say 'While POSIX specifies that a "return" may be used to exit a "dot" sourced script,' there instead to be clearer. Which would make that whole first paragraph become: Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) the git-rebase--*.sh scripts have used a "return" to return from the "dot" command that runs them. While POSIX specifies that a "return" may be used to exit a "dot" sourced script, the FreeBSD /bin/sh utility behaves poorly under some circumstances when such a "return" is executed. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-17 0:41 ` Kyle J. McKay @ 2014-04-17 17:15 ` Junio C Hamano 2014-04-18 0:26 ` Kyle J. McKay 0 siblings, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2014-04-17 17:15 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > Either "return to the function that dot-sourced us" or "return from > the dot command that dot-sourced us", > but using the original wording > implies to me that the function that dot-sourced us will return as > soon as the dot-sourced script executes the return and that is exactly > one of the bugs we're working around. True. "return to the function that dot-sourced us" is what I meant. To be more pedantic, "return to the point in the function that dot-sourced this file". > I think just the s/from/to/ would fix it so it does not give me the > wrong impression, but that doesn't mean that would not confuse > everyone else. ;) Yeah, let's do that. Thanks for carefully reading them. >> So "while this is allowed by POSIX" may be a bit misleading and >> needs to be reworded, I guess? > > "allowed by POSIX" is not incorrect. ;) Calling something that is required as "allowed" is a bit misleading, if not outright dishonest. Allowed implies that you are free not to do so and can still be in compliance, but I do not think that is the case. I'd think it makes it clearer to say that we take "return stopping the dot-sourced file" as a given and FreeBSD does not behave that way. -- >8 -- From: "Kyle J. McKay" <mackyle@gmail.com> Date: Fri, 11 Apr 2014 01:28:17 -0700 Subject: [PATCH] rebase: avoid non-function use of "return" on FreeBSD Since a1549e10, 15d4bf2e and 01a1e646 (first appearing in v1.8.4) the git-rebase--*.sh scripts have used a "return" to stop execution of the dot-sourced file and return to the "dot" command that dot-sourced it. The /bin/sh utility on FreeBSD however behaves poorly under some circumstances when such a "return" is executed. In particular, if the "dot" command is contained within a function, then when a "return" is executed by the script it runs (that is not itself inside a function), control will return from the function that contains the "dot" command skipping any statements that might follow the dot command inside that function. Commit 99855ddf (first appearing in v1.8.4.1) addresses this by making the "dot" command the last line in the function. Unfortunately the FreeBSD /bin/sh may also execute some statements in the script run by the "dot" command that appear after the troublesome "return". The fix in 99855ddf does not address this problem. For example, if you have script1.sh with these contents: run_script2() { . "$(dirname -- "$0")/script2.sh" _e=$? echo only this line should show [ $_e -eq 5 ] || echo expected status 5 got $_e return 3 } run_script2 e=$? [ $e -eq 3 ] || { echo expected status 3 got $e; exit 1; } And script2.sh with these contents: if [ 5 -gt 3 ]; then return 5 fi case bad in *) echo always shows esac echo should not get here ! : When running script1.sh (e.g. '/bin/sh script1.sh' or './script1.sh' after making it executable), the expected output from a POSIX shell is simply the single line: only this line should show However, when run using FreeBSD's /bin/sh, the following output appears instead: should not get here expected status 3 got 1 Not only did the lines following the "dot" command in the run_script2 function in script1.sh get skipped, but additional lines in script2.sh following the "return" got executed -- but not all of them (e.g. the "echo always shows" line did not run). These issues can be avoided by not using a top-level "return" in script2.sh. If script2.sh is changed to this: main() { if [ 5 -gt 3 ]; then return 5 fi case bad in *) echo always shows esac echo should not get here ! : } main Then it behaves the same when using FreeBSD's /bin/sh as when using other more POSIX compliant /bin/sh implementations. We fix the git-rebase--*.sh scripts in a similar fashion by moving the top-level code that contains "return" statements into its own function and then calling that as the last line in the script. Signed-off-by: Kyle J. McKay <mackyle@gmail.com> Acked-by: Matthieu Moy <Matthieu.Moy@imag.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- git-rebase--am.sh | 15 +++++++++++++++ git-rebase--interactive.sh | 15 +++++++++++++++ git-rebase--merge.sh | 15 +++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/git-rebase--am.sh b/git-rebase--am.sh index a4f683a..1cdc139 100644 --- a/git-rebase--am.sh +++ b/git-rebase--am.sh @@ -4,6 +4,17 @@ # Copyright (c) 2010 Junio C Hamano. # +# The whole contents of this file is run by dot-sourcing it from +# inside a shell function. It used to be that "return"s we see +# below were not inside any function, and expected to return +# to the function that dot-sourced us. +# +# However, FreeBSD /bin/sh misbehaves on such a construct and +# continues to run the statements that follow such a "return". +# As a work-around, we introduce an extra layer of a function +# here, and immediately call it after defining it. +git_rebase__am () { + case "$action" in continue) git am --resolved --resolvemsg="$resolvemsg" && @@ -73,3 +84,7 @@ then fi move_to_original_branch + +} +# ... and then we call the whole thing. +git_rebase__am diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 43631b4..9e1dd1e 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -810,6 +810,17 @@ add_exec_commands () { mv "$1.new" "$1" } +# The whole contents of this file is run by dot-sourcing it from +# inside a shell function. It used to be that "return"s we see +# below were not inside any function, and expected to return +# to the function that dot-sourced us. +# +# However, FreeBSD /bin/sh misbehaves on such a construct and +# continues to run the statements that follow such a "return". +# As a work-around, we introduce an extra layer of a function +# here, and immediately call it after defining it. +git_rebase__interactive () { + case "$action" in continue) # do we have anything to commit? @@ -1042,3 +1053,7 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" output git checkout $onto || die_abort "could not detach HEAD" git update-ref ORIG_HEAD $orig_head do_rest + +} +# ... and then we call the whole thing. +git_rebase__interactive diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh index e7d96de..838fbed 100644 --- a/git-rebase--merge.sh +++ b/git-rebase--merge.sh @@ -101,6 +101,17 @@ finish_rb_merge () { say All done. } +# The whole contents of this file is run by dot-sourcing it from +# inside a shell function. It used to be that "return"s we see +# below were not inside any function, and expected to return +# to the function that dot-sourced us. +# +# However, FreeBSD /bin/sh misbehaves on such a construct and +# continues to run the statements that follow such a "return". +# As a work-around, we introduce an extra layer of a function +# here, and immediately call it after defining it. +git_rebase__merge () { + case "$action" in continue) read_state @@ -151,3 +162,7 @@ do done finish_rb_merge + +} +# ... and then we call the whole thing. +git_rebase__merge -- 1.9.2-630-gc3ddd0c ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD 2014-04-17 17:15 ` Junio C Hamano @ 2014-04-18 0:26 ` Kyle J. McKay 0 siblings, 0 replies; 21+ messages in thread From: Kyle J. McKay @ 2014-04-18 0:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine On Apr 17, 2014, at 10:15, Junio C Hamano wrote: >> I think just the s/from/to/ would fix it so it does not give me the >> wrong impression, but that doesn't mean that would not confuse >> everyone else. ;) > > Yeah, let's do that. Thanks for carefully reading them. > I'd think it makes it clearer to say that we take "return stopping > the dot-sourced file" as a given and FreeBSD does not behave that > way. > > -- > 8 -- > From: "Kyle J. McKay" <mackyle@gmail.com> > Date: Fri, 11 Apr 2014 01:28:17 -0700 > Subject: [PATCH] rebase: avoid non-function use of "return" on FreeBSD [...] The new version of the patch looks great, let's use that. Thanks for adjusting it. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/3] Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD" 2014-04-11 8:28 [PATCH 0/3] Fix support for FreeBSD's /bin/sh Kyle J. McKay 2014-04-11 8:28 ` [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD Kyle J. McKay @ 2014-04-11 8:28 ` Kyle J. McKay 2014-04-11 8:28 ` [PATCH 3/3] test: fix t5560 on FreeBSD Kyle J. McKay 2 siblings, 0 replies; 21+ messages in thread From: Kyle J. McKay @ 2014-04-11 8:28 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine This reverts commit 99855ddf4bd319cd06a0524e755ab1c1b7d39f3b. The workaround 99855ddf introduced to deal with problematic "return" statements in scripts run by "dot" commands located inside functions only handles one part of the problem. The issue has now been addressed by not using "return" statements in this way in the git-rebase--*.sh scripts. This workaround is therefore no longer necessary, so clean up the code by reverting it. Conflicts: git-rebase.sh Signed-off-by: Kyle J. McKay <mackyle@gmail.com> --- git-rebase.sh | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 8a3efa29..07e2bd48 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -169,22 +169,13 @@ You can run "git stash pop" or "git stash drop" at any time. rm -rf "$state_dir" } -run_specific_rebase_internal () { +run_specific_rebase () { if [ "$interactive_rebase" = implied ]; then GIT_EDITOR=: export GIT_EDITOR autosquash= fi - # On FreeBSD, the shell's "return" returns from the current - # function, not from the current file inclusion. - # run_specific_rebase_internal has the file inclusion as a - # last statement, so POSIX and FreeBSD's return will do the - # same thing. . git-rebase--$type -} - -run_specific_rebase () { - run_specific_rebase_internal ret=$? if test $ret -eq 0 then -- tg: (1bb252a9..) t/revert-99855ddf (depends on: t/freebsd-sh-return) ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/3] test: fix t5560 on FreeBSD 2014-04-11 8:28 [PATCH 0/3] Fix support for FreeBSD's /bin/sh Kyle J. McKay 2014-04-11 8:28 ` [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD Kyle J. McKay 2014-04-11 8:28 ` [PATCH 2/3] Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD" Kyle J. McKay @ 2014-04-11 8:28 ` Kyle J. McKay 2014-04-11 20:52 ` Junio C Hamano 2 siblings, 1 reply; 21+ messages in thread From: Kyle J. McKay @ 2014-04-11 8:28 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine Since fd0a8c2e (first appearing in v1.7.0), the t/t5560-http-backend-noserver.sh test has used a backslash escape inside a ${} expansion in order to specify a literal '?' character. Unfortunately the FreeBSD /bin/sh does not interpret this correctly. In a POSIX compliant shell, the following: x='one?two?three' echo "${x#*\?}" Would be expected to produce this: two?three When using the FreeBSD /bin/sh instead you get this: one?two?three In fact the FreeBSD /bin/sh treats the backslash as a literal character to match so that this: y='one\two\three' echo "${y#*\?}" Produces this unexpected value: wo\three In this case the backslash is not only treated literally, it also fails to defeat the special meaning of the '?' character. Instead, we can use the [...] construct to defeat the special meaning of the '?' character and match it exactly in a way that works for the FreeBSD /bin/sh as well as other POSIX /bin/sh implementations. Changing the example like so: x='one?two?three' echo "${x#*[?]}" Produces the expected output using the FreeBSD /bin/sh. Therefore, change the use of \? to [?] in order to be compatible with the FreeBSD /bin/sh which allows t/t5560-http-backend-noserver.sh to pass on FreeBSD again. Signed-off-by: Kyle J. McKay <mackyle@gmail.com> --- t/t5560-http-backend-noserver.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t5560-http-backend-noserver.sh b/t/t5560-http-backend-noserver.sh index 9be9ae34..5abd11a5 100755 --- a/t/t5560-http-backend-noserver.sh +++ b/t/t5560-http-backend-noserver.sh @@ -9,8 +9,8 @@ test_have_prereq GREP_STRIPS_CR && export GREP_OPTIONS=-U run_backend() { echo "$2" | - QUERY_STRING="${1#*\?}" \ - PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%\?*}" \ + QUERY_STRING="${1#*[?]}" \ + PATH_TRANSLATED="$HTTPD_DOCUMENT_ROOT_PATH/${1%%[?]*}" \ git http-backend >act.out 2>act.err } -- tg: (532c2992..) t/freebsd-t5560 (depends on: t/revert-99855ddf) ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 3/3] test: fix t5560 on FreeBSD 2014-04-11 8:28 ` [PATCH 3/3] test: fix t5560 on FreeBSD Kyle J. McKay @ 2014-04-11 20:52 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2014-04-11 20:52 UTC (permalink / raw) To: Kyle J. McKay; +Cc: git, Matthieu Moy, Ramkumar Ramachandra, Eric Sunshine "Kyle J. McKay" <mackyle@gmail.com> writes: > Instead, we can use the [...] construct to defeat the special meaning > of the '?' character and match it exactly in a way that works for the > FreeBSD /bin/sh as well as other POSIX /bin/sh implementations. > > Changing the example like so: > > x='one?two?three' > echo "${x#*[?]}" > > Produces the expected output using the FreeBSD /bin/sh. I'll queue this in the meantime, while 1&2/3 are sorted out between you and Matthieu. I will also take "cp -a" patch with "-R -P -p", not just "-R" for the reasons stated in the other thread. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2014-04-18 0:26 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-04-11 8:28 [PATCH 0/3] Fix support for FreeBSD's /bin/sh Kyle J. McKay 2014-04-11 8:28 ` [PATCH 1/3] rebase: avoid non-function use of "return" on FreeBSD Kyle J. McKay 2014-04-11 8:48 ` Matthieu Moy 2014-04-11 14:29 ` Kyle J. McKay 2014-04-11 17:30 ` Matthieu Moy 2014-04-11 23:08 ` Kyle J. McKay 2014-04-12 17:07 ` Matthieu Moy 2014-04-13 2:45 ` Kyle J. McKay 2014-04-14 8:24 ` Matthieu Moy 2014-04-14 22:28 ` Junio C Hamano 2014-04-14 22:51 ` Junio C Hamano 2014-04-16 4:32 ` Kyle J. McKay 2014-04-16 16:47 ` Junio C Hamano 2014-04-16 18:11 ` Junio C Hamano 2014-04-16 18:23 ` Junio C Hamano 2014-04-17 0:41 ` Kyle J. McKay 2014-04-17 17:15 ` Junio C Hamano 2014-04-18 0:26 ` Kyle J. McKay 2014-04-11 8:28 ` [PATCH 2/3] Revert "rebase: fix run_specific_rebase's use of "return" on FreeBSD" Kyle J. McKay 2014-04-11 8:28 ` [PATCH 3/3] test: fix t5560 on FreeBSD Kyle J. McKay 2014-04-11 20:52 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.