On Sun, Nov 10, 2013 at 4:46 PM, Richard Hansen <rhansen@xxxxxxx> wrote:
> On 2013-10-29 04:41, Felipe Contreras wrote:
>> Richard Hansen wrote:
>>> Signed-off-by: Richard Hansen <rhansen@xxxxxxx>
>>> ---
>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html