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

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

Richard Hansen wrote:

quoted

Signed-off-by: Richard Hansen <redacted>
---
 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.

quoted

+		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

quoted

+			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