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

From: Richard Hansen <rhansen@bbn.com>
Date: Nov 10, 2013, 22:46 UTC
Message-ID: <52800CB7.7080905@bbn.com>
In-Reply-To: <526f74cf8307f_37cdfb1e7c31@nysa.notmuch>

On 2013-10-29 04:41, Felipe Contreras wrote:

Show 44 quoted lines

> Richard Hansen wrote:
>> 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.

Good point. I personally avoid tabs whenever possible, and <<- only works with tabs, so I'm in the habit of doing <<EOF.

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

I'm in the habit of doing ${foo} because I like the consistency -- sometimes you need them to disambiguate, and sometimes you need special expansions like ${foo##bar} or ${foo:-bar}.

In this case it's actually less consistent to do ${foo} because the rest of the file doesn't use {} when not needed, so I agree with your change.

Show 8 quoted lines

> 
>> +		case ${opt} in
>> +		    force)
> 
> I think the convention is to align these:
> 
> case $opt in
> force)

The existing case statement in this file indents the patterns the same amount as the case statement, so this should be aligned to match.

In general I rarely see the case patterns indented at the same level as
the case statement, possibly because Emacs shell-mode indents the
patterns more than the case statement (by default).  The POSIX spec
contains a mix of styles:
  * the normative text documenting the format of a 'case' construct
    indents the patterns more than the 'case' statement
  * two of the four non-normative examples indent the patterns
    more than the 'case' statements; the other two do not

Show 27 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
> 		;;

Works for me.

> 
> But this is definetly good to have, will merge.

Thanks, Richard