Re: [PATCH v4 12/10] git-remote-testgit: support the new 'force' option
Richard Hansen rhan...@bbn.com writes: 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, What you see does not matter in the context of this project ;-) This is what we have in Documentation/CodingGuidelines: For shell scripts specifically (not exhaustive): - Case arms are indented at the same depth as case and esac lines. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 12/10] git-remote-testgit: support the new 'force' option
On 2013-11-11 13:28, Junio C Hamano wrote: Richard Hansen rhan...@bbn.com writes: 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, What you see does not matter in the context of this project ;-) This is what we have in Documentation/CodingGuidelines: For shell scripts specifically (not exhaustive): - Case arms are indented at the same depth as case and esac lines. Doh! I missed that. Thanks for pointing it out. Thanks, Richard -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
Signed-off-by: Richard Hansen rhan...@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.
+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
+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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 12/10] git-remote-testgit: support the new 'force' option
On Sun, Nov 10, 2013 at 4:46 PM, Richard Hansen rhan...@bbn.com wrote:
On 2013-10-29 04:41, Felipe Contreras wrote:
Richard Hansen wrote:
Signed-off-by: Richard Hansen rhan...@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.
That looks very weird to me, plus -EOF is often used already in git tests.
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 --
Sure, but with the price of less readibility. If consistency was the
priority, we would be doing the follwoing in C:
if (foo) {
# single line
}
Since the if might contain multiple lines, but we don't do that,
because readibility is more important than consistency. So sometimes
it's with braces, sometimes without.
+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
The style in C has the cases at the same level, so I think it makes
sense to do the same in shell, but I'm not sure if that's followed
already.
+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.
Good, the final code style can be decided later on, and perhaps update
Documentation/CodingGuidelines, but it's good the rest is more or less
settled.
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 12/10] git-remote-testgit: support the new 'force' option
Richard Hansen wrote:
Signed-off-by: Richard Hansen rhan...@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)
+ 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 12/10] git-remote-testgit: support the new 'force' option
Signed-off-by: Richard Hansen rhan...@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
+ case ${opt} in
+ force)
+ 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};;
+ esac;;
+ *) echo unsupported;;
+ esac
+ ;;
'')
exit
;;
--
1.8.4.1.614.ga09cf56
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
