RE: [PATCH v4 12/10] git-remote-testgit: support the new 'force' option

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Oct 29, 2013, 08:41 UTC
Message-ID: <526f74cf8307f_37cdfb1e7c31@nysa.notmuch>
In-Reply-To: <1382908589-7000-1-git-send-email-rhansen@bbn.com>

Richard Hansen wrote:

Show 41 quoted lines

> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  git-remote-testgit.sh | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
> index 6d2f282..80546c1 100755
> --- a/git-remote-testgit.sh
> +++ b/git-remote-testgit.sh
> @@ -6,6 +6,7 @@ url=$2
>  
>  dir="$GIT_DIR/testgit/$alias"
>  prefix="refs/testgit/$alias"
> +forcearg=
>  
>  default_refspec="refs/heads/*:${prefix}/heads/*"
>  
> @@ -39,6 +40,7 @@ do
>  		fi
>  		test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
>  		test -n "$GIT_REMOTE_TESTGIT_NO_PRIVATE_UPDATE" && echo "no-private-update"
> +		echo 'option'
>  		echo
>  		;;
>  	list)
> @@ -93,6 +95,7 @@ do
>  		before=$(git for-each-ref --format=' %(refname) %(objectname) ')
>  
>  		git fast-import \
> +			${forcearg} \
>  			${testgitmarks:+"--import-marks=$testgitmarks"} \
>  			${testgitmarks:+"--export-marks=$testgitmarks"} \
>  			--quiet
> @@ -115,6 +118,21 @@ do
>  
>  		echo
>  		;;
> +	option\ *)
> +		read cmd opt val <<EOF
> +${line}
> +EOF

We can do <<-EOF to align this properly.

Also, I don't see why all the variables are ${foo} instead of $foo.

> +		case ${opt} in
> +		    force)

I think the convention is to align these:

case $opt in force)

Show 5 quoted lines

> +			case ${val} in
> +			    true) forcearg=--force; echo 'ok';;
> +			    false) forcearg=; echo 'ok';;
> +			    *) printf %s\\n "error '${val}'\
> + is not a valid value for option ${opt}";;

I think this is packing a lot of stuff and it's not that readable.

Moreover, this is not for production purposes, it's for testing purposes and a guideline, I think this suffices.

	option\ *)
		read cmd opt val <<-EOF
		$line
		EOF
		case $opt in
		force)
			test $val = "true" && force="true" || force=
			echo "ok"
			;;
		*)
			echo "unsupported"
			;;
		esac
		;;

But this is definetly good to have, will merge.

-- 
Felipe Contreras