threads / patch / 33275

patch, 3 partscontrib/subtree: stop explicitly using a bash shell

Subject: [PATCH 1/3] contrib/subtree: stop explicitly using a bash shell

## tl;dr

6 messages between Mar 24, 2013 and Mar 25, 2013. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Paul Campbell· Mar 24, 2013, 19:37 UTC · lore

[PATCH 0/3] Improve POSIX compatibility and general portablity

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· Mar 24, 2013, 19:37 UTC · re: Paul Campbell · lore

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 <pcampbell@kemitix.net>
---

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(-)

Show changes to contrib/subtree/git-subtree.sh +1 −1

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

Junio C Hamano· Mar 25, 2013, 17:57 UTC · re: Paul Campbell · lore

Re: [PATCH 1/3] contrib/subtree: stop explicitly using a bash shell

Paul Campbell <pcampbell@kemitix.net> writes:

Show 22 quoted lines

> 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 <pcampbell@kemitix.net>
> ---
>
> 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.

Paul Campbell· Mar 24, 2013, 19:37 UTC · re: Paul Campbell · lore

[PATCH 2/3] contrib/subtree: remove use of -a/-o in [ commands

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 <pcampbell@kemitix.net>
---
 contrib/subtree/git-subtree.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Show changes to contrib/subtree/git-subtree.sh +7 −7

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

Simon Ruderich· Mar 24, 2013, 21:17 UTC · re: Paul Campbell · lore

Re: [PATCH 2/3] contrib/subtree: remove use of -a/-o in [ commands

From: Paul Campbell <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 ||.

Signed-off-by: Paul Campbell <pcampbell@kemitix.net>
Signed-off-by: Simon Ruderich <simon@ruderich.org>
---

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(-)

Show changes to contrib/subtree/git-subtree.sh +7 −7

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

Paul Campbell· Mar 24, 2013, 19:37 UTC · re: Paul Campbell · lore

[PATCH 3/3] contrib/subtree: replace echo options with printf

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 <pcampbell@kemitix.net>
---

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(-)

Show changes to contrib/subtree/git-subtree.sh +9 −2

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