[PATCH v4 12/10] git-remote-testgit: support the new 'force' option
Signed-off-by: Richard Hansen
---
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
RE: [PATCH v4 12/10] git-remote-testgit: support the new 'force' option
Richard Hansen wrote:
> Signed-off-by: Richard Hansen
> ---
> 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 < +${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
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
>> ---
>> 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 <> +${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 <
> 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 wrote:
> On 2013-10-29 04:41, Felipe Contreras wrote:
>> Richard Hansen wrote:
>>> Signed-off-by: Richard Hansen
>>> ---
>>> 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 <>> +${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 <> 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 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 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
