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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Nov 11, 2013, 03:57 UTC
Message-ID: <CAMP44s3P7mmBP7=g4ZZpQcqyzeyAcAeH6gNfusbNX8xeQ+0JVQ@mail.gmail.com>
In-Reply-To: <52800CB7.7080905@bbn.com>

On Sun, Nov 10, 2013 at 4:46 PM, Richard Hansen <rhansen@bbn.com> wrote:

Show 48 quoted lines

> On 2013-10-29 04:41, Felipe Contreras wrote:
>> 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.

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.

Show 19 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

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.

Show 28 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.

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