# [PATCH 1/3] contrib/subtree: stop explicitly using a bash shell 6 messages from 2013-03-24 to 2013-03-25. Participants: Paul Campbell, Simon Ruderich, Junio C Hamano. Thread: https://gitlist.dev/t/33275 ## Paul Campbell, 2013-03-24 19:37 Subject: [PATCH 0/3] Improve POSIX compatibility and general portablity Message-ID: <1364153863-27437-1-git-send-email-pcampbell@kemitix.net> URL: https://gitlist.dev/e/1364153863-27437-1-git-send-email-pcampbell%40kemitix.net ``` git-subtree is considered to be a Bash only script and not POSIX compatibile. I removed the /bin/bash and tested with dash. All the tests passed. I also followed some of the advice found in https://wiki.ubuntu.com/DashAsBinSh for converting a bash script to be more portable. Paul Campbell (3): contrib/subtree: stop explicitly using a bash shell contrib/subtree: remove use of -a/-o in [ commands contrib/subtree: replace echo options with printf contrib/subtree/git-subtree.sh | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) -- 1.8.2 ``` ## Paul Campbell, 2013-03-24 19:37 Subject: [PATCH 1/3] contrib/subtree: stop explicitly using a bash shell Message-ID: <1364153863-27437-2-git-send-email-pcampbell@kemitix.net> URL: https://gitlist.dev/e/1364153863-27437-2-git-send-email-pcampbell%40kemitix.net In-Reply-To: <1364153863-27437-1-git-send-email-pcampbell@kemitix.net> ``` Don't explicitly use the Bash shell but allow the system to provide a hopefully POSIX compatible shell at /bin/sh. Signed-off-by: Paul Campbell --- Only the system's I was able to test this on (Debian squeeze) /bin/sh is the dash shell. contrib/subtree/git-subtree.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 8a23f58..5701376 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -1,4 +1,4 @@ -#!/bin/bash +#!/bin/sh # # git-subtree.sh: split/join git repositories in subdirectories of this one # -- 1.8.2 ``` ## Paul Campbell, 2013-03-24 19:37 Subject: [PATCH 2/3] contrib/subtree: remove use of -a/-o in [ commands Message-ID: <1364153863-27437-3-git-send-email-pcampbell@kemitix.net> URL: https://gitlist.dev/e/1364153863-27437-3-git-send-email-pcampbell%40kemitix.net In-Reply-To: <1364153863-27437-1-git-send-email-pcampbell@kemitix.net> ``` Use of -a and -o in the [ command can have confusing semantics. Use a separate test invocation for each single test, combining them with && and ||, and use ordinary parentheses for grouping. Signed-off-by: Paul Campbell --- contrib/subtree/git-subtree.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 5701376..884cbfb 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -119,7 +119,7 @@ esac dir="$(dirname "$prefix/.")" -if [ "$command" != "pull" -a "$command" != "add" -a "$command" != "push" ]; then +if ( test "$command" != "pull" ) && ( test "$command" != "add" ) && ( test "$command" != "push" ); then revs=$(git rev-parse $default --revs-only "$@") || exit $? dirs="$(git rev-parse --no-revs --no-flags "$@")" || exit $? if [ -n "$dirs" ]; then @@ -181,9 +181,9 @@ cache_set() { oldrev="$1" newrev="$2" - if [ "$oldrev" != "latest_old" \ - -a "$oldrev" != "latest_new" \ - -a -e "$cachedir/$oldrev" ]; then + if ( test "$oldrev" != "latest_old" ) \ + && ( test "$oldrev" != "latest_new" ) \ + && ( test -e "$cachedir/$oldrev" ); then die "cache for $oldrev already exists!" fi echo "$newrev" >"$cachedir/$oldrev" @@ -273,12 +273,12 @@ find_existing_splits() git-subtree-split:) sub="$b" ;; END) debug " Main is: '$main'" - if [ -z "$main" -a -n "$sub" ]; then + if ( test -z "$main" ) && ( test -n "$sub" ); then # squash commits refer to a subtree debug " Squash: $sq from $sub" cache_set "$sq" "$sub" fi - if [ -n "$main" -a -n "$sub" ]; then + if ( test -n "$main" ) && (test -n "$sub" ); then debug " Prior: $main -> $sub" cache_set $main $sub cache_set $sub $sub @@ -541,7 +541,7 @@ cmd_add_commit() tree=$(git write-tree) || exit $? headrev=$(git rev-parse HEAD) || exit $? - if [ -n "$headrev" -a "$headrev" != "$rev" ]; then + if ( test -n "$headrev" ) && ( test "$headrev" != "$rev" ); then headp="-p $headrev" else headp= -- 1.8.2 ``` ## Paul Campbell, 2013-03-24 19:37 Subject: [PATCH 3/3] contrib/subtree: replace echo options with printf Message-ID: <1364153863-27437-4-git-send-email-pcampbell@kemitix.net> URL: https://gitlist.dev/e/1364153863-27437-4-git-send-email-pcampbell%40kemitix.net In-Reply-To: <1364153863-27437-1-git-send-email-pcampbell@kemitix.net> ``` Options to echo are not portable. In particular, the echo -e option is implemented by some shells, including bash, to expand escape sequences. Use the printf command instead, which is portable and much more reliable. Only instances of echo and say (a wrapper for echo) where they are used with options have been replaced with printf. say_progress() is added to mirror the behaviour of say() in respecting the -q/--quiet option. Signed-off-by: Paul Campbell --- This is a better version of the previously submitted patch (http://article.gmane.org/gmane.comp.version-control.git/218103) which added another option to echo. contrib/subtree/git-subtree.sh | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 884cbfb..35caf12 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -61,6 +61,13 @@ say() fi } +say_progress() +{ + if [ -z "$quiet" ]; then + printf "%s\r" "$@" >&2 + fi +} + assert() { if "$@"; then @@ -311,7 +318,7 @@ copy_commit() GIT_COMMITTER_NAME \ GIT_COMMITTER_EMAIL \ GIT_COMMITTER_DATE - (echo -n "$annotate"; cat ) | + (printf "$annotate"; cat ) | git commit-tree "$2" $3 # reads the rest of stdin ) || die "Can't copy commit $1" } @@ -592,7 +599,7 @@ cmd_split() eval "$grl" | while read rev parents; do revcount=$(($revcount + 1)) - say -n "$revcount/$revmax ($createcount) " + say_progress "$revcount/$revmax ($createcount)" debug "Processing commit: $rev" exists=$(cache_get $rev) if [ -n "$exists" ]; then -- 1.8.2 ``` ## Simon Ruderich, 2013-03-24 21:17 Subject: Re: [PATCH 2/3] contrib/subtree: remove use of -a/-o in [ commands Message-ID: <20130324211713.GA6155@ruderich.org> URL: https://gitlist.dev/e/20130324211713.GA6155%40ruderich.org In-Reply-To: <1364153863-27437-3-git-send-email-pcampbell@kemitix.net> ``` From: Paul Campbell Use of -a and -o in the [ command can have confusing semantics. Use a separate test invocation for each single test, combining them with && and ||. Signed-off-by: Paul Campbell Signed-off-by: Simon Ruderich --- On Sun, Mar 24, 2013 at 07:37:42PM +0000, Paul Campbell wrote: > Use a separate test invocation for each single test, combining them with > && and ||, and use ordinary parentheses for grouping. Hello Paul, Parentheses are only necessary if both && and || are used to enforce precedence; the shell can split the commands without needing the parentheses. In these cases they can all be removed. Regards Simon contrib/subtree/git-subtree.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 5701376..d02e6c5 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -119,7 +119,7 @@ esac dir="$(dirname "$prefix/.")" -if [ "$command" != "pull" -a "$command" != "add" -a "$command" != "push" ]; then +if test "$command" != "pull" && test "$command" != "add" && test "$command" != "push"; then revs=$(git rev-parse $default --revs-only "$@") || exit $? dirs="$(git rev-parse --no-revs --no-flags "$@")" || exit $? if [ -n "$dirs" ]; then @@ -181,9 +181,9 @@ cache_set() { oldrev="$1" newrev="$2" - if [ "$oldrev" != "latest_old" \ - -a "$oldrev" != "latest_new" \ - -a -e "$cachedir/$oldrev" ]; then + if test "$oldrev" != "latest_old" \ + && test "$oldrev" != "latest_new" \ + && test -e "$cachedir/$oldrev"; then die "cache for $oldrev already exists!" fi echo "$newrev" >"$cachedir/$oldrev" @@ -273,12 +273,12 @@ find_existing_splits() git-subtree-split:) sub="$b" ;; END) debug " Main is: '$main'" - if [ -z "$main" -a -n "$sub" ]; then + if test -z "$main" && test -n "$sub"; then # squash commits refer to a subtree debug " Squash: $sq from $sub" cache_set "$sq" "$sub" fi - if [ -n "$main" -a -n "$sub" ]; then + if test -n "$main" && test -n "$sub"; then debug " Prior: $main -> $sub" cache_set $main $sub cache_set $sub $sub @@ -541,7 +541,7 @@ cmd_add_commit() tree=$(git write-tree) || exit $? headrev=$(git rev-parse HEAD) || exit $? - if [ -n "$headrev" -a "$headrev" != "$rev" ]; then + if test -n "$headrev" && test "$headrev" != "$rev"; then headp="-p $headrev" else headp= -- 1.8.2 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ``` ## Junio C Hamano, 2013-03-25 17:57 Subject: Re: [PATCH 1/3] contrib/subtree: stop explicitly using a bash shell Message-ID: <7vy5dbxszy.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vy5dbxszy.fsf%40alter.siamese.dyndns.org In-Reply-To: <1364153863-27437-2-git-send-email-pcampbell@kemitix.net> ``` Paul Campbell writes: > Don't explicitly use the Bash shell but allow the system to provide a > hopefully POSIX compatible shell at /bin/sh. > > Signed-off-by: Paul Campbell > --- > > Only the system's I was able to test this on (Debian squeeze) /bin/sh is > the dash shell. > > contrib/subtree/git-subtree.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh > index 8a23f58..5701376 100755 > --- a/contrib/subtree/git-subtree.sh > +++ b/contrib/subtree/git-subtree.sh > @@ -1,4 +1,4 @@ > -#!/bin/bash > +#!/bin/sh > # > # git-subtree.sh: split/join git repositories in subdirectories of this one > # Interesting. I'll leave the final "yeah, this is safe" declaration to David and Avery, but I've always assumed without checking that this script relied on bash-isms like local variable semantics, arrays, regexp/substring variable substitutions, etc. With a quick scan, however, I do not seem to find anythning glaringly unportable. ```