Re: [PATCH 1/7] tests: subshell indentation stylefix

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 7, 2010, 03:44 UTC
Message-ID: <20100907034417.GL1182@burratino>
In-Reply-To: <20100907014254.GB1182@burratino>

Jonathan Nieder wrote:

> Format the subshells introduced by the previous patch (Several tests:
> cd inside subshell instead of around, 2010-09-06)

Review of said previous patch follows.  Sorry for the confusing diffs.

Show 20 quoted lines

> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -27,12 +27,13 @@ test_expect_success 'update-index and ls-files' '
>  	one) echo pass one ;;
>  	*) echo bad one; exit 1 ;;
>  	esac &&
> -	(cd dir &&
> -	git update-index --add two &&
> -	case "`git ls-files`" in
> -	two) echo pass two ;;
> -	*) echo bad two; exit 1 ;;
> -	esac
> +	(
> +		cd dir &&
> +		git update-index --add two &&
> +		case "`git ls-files`" in
> +		two) echo pass two ;;
> +		*) echo bad two; exit 1 ;;
> +		esac
>  	) &&

Trapping the "exit" in a subshell improves behavior. I wonder why the script does not use "false" or "(exit 1)"; the tests outside the subshell have the same problem...

Show 13 quoted lines

> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -85,9 +85,10 @@ EOF
>  "
>  
>  commit_file sm1 &&
> -head3=$(cd sm1 &&
> -git reset --hard HEAD~2 >/dev/null &&
> -git rev-parse --verify HEAD | cut -c1-7
> +head3=$(
> +	cd sm1 &&
> +	git reset --hard HEAD~2 >/dev/null &&
> +	git rev-parse --verify HEAD | cut -c1-7

Unrelated: the --verify here does not have much effect on the upstream
of a pipeline.  Maybe "git log -1 --abbrev=7 --format=%h", or even
better, "git log -1 --format=%h" (for DEFAULT_ABBREV) would do the
trick.

Show 20 quoted lines

> --- a/t/t9100-git-svn-basic.sh
> +++ b/t/t9100-git-svn-basic.sh
> @@ -22,16 +22,17 @@ esac
>  test_expect_success \
>      'initialize git svn' '
>  	mkdir import &&
> -	(cd import &&
> -	echo foo > foo &&
> -	ln -s foo foo.link
> -	mkdir -p dir/a/b/c/d/e &&
> -	echo "deep dir" > dir/a/b/c/d/e/file &&
> -	mkdir bar &&
> -	echo "zzz" > bar/zzz &&
> -	echo "#!/bin/sh" > exec.sh &&
> -	chmod +x exec.sh &&
> -	svn_cmd import -m "import for git svn" . "$svnrepo" >/dev/null
> +	(
> +		cd import &&
> +		echo foo >foo &&
> +		ln -s foo foo.link

Missing SYMLINKS prerequisite?

Show 19 quoted lines

> --- a/t/t9107-git-svn-migrate.sh
> +++ b/t/t9107-git-svn-migrate.sh
> @@ -6,14 +6,16 @@ test_description='git svn metadata migrations from previous versions'
>  test_expect_success 'setup old-looking metadata' '
>  	cp "$GIT_DIR"/config "$GIT_DIR"/config-old-git-svn &&
>  	mkdir import &&
> -	(cd import &&
> -		for i in trunk branches/a branches/b \
> -		         tags/0.1 tags/0.2 tags/0.3; do
> -			mkdir -p $i && \
> -			echo hello >> $i/README || exit 1
> -		done && \
> +	(
> +		cd import &&
> +		for i in trunk branches/a branches/b tags/0.1 tags/0.2 tags/0.3
> +		do
> +			mkdir -p $i &&
> +			echo hello >>$i/README ||
> +			exit 1

An "exit" to avoid wasting time after a failing setup test might seem appropriate, but I am happier to see it trapped by the subshell.

Show 19 quoted lines

> --- a/t/t9116-git-svn-log.sh
> +++ b/t/t9116-git-svn-log.sh
> @@ -8,14 +8,16 @@ test_description='git svn log tests'
>  
>  test_expect_success 'setup repository and import' '
>  	mkdir import &&
> -	(cd import &&
> -		for i in trunk branches/a branches/b \
> -		         tags/0.1 tags/0.2 tags/0.3; do
> -			mkdir -p $i && \
> -			echo hello >> $i/README || exit 1
> -		done && \
> +	(
> +		cd import &&
> +		for i in trunk branches/a branches/b tags/0.1 tags/0.2 tags/0.3
> +		do
> +			mkdir -p $i &&
> +			echo hello >>$i/README ||
> +			exit 1

Likewise.

Show 16 quoted lines

> --- a/t/t9119-git-svn-info.sh
> +++ b/t/t9119-git-svn-info.sh
> @@ -179,11 +186,13 @@ test_expect_success 'info --url added-directory' '
>  	'
>  
>  test_expect_success 'info added-symlink-file' "
> -	(cd gitwc &&
> +	(
> +		cd gitwc &&
>  		ln -s added-file added-symlink-file &&
>  		git add added-symlink-file
>  	) &&
> -	(cd svnwc &&
> +	(
> +		cd svnwc &&
>  		ln -s added-file added-symlink-file &&

More missing SYMLINKS prerequisites.

To summarize, wherever you change behavior, it is improved.  Thanks.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>