Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> Introduce optional prefixes "width=" and "position=" for the align atom
>> so that the atom can be used as "%(align:width=<width>,position=<position>)".
>>
>> Add Documetation and tests for the same.
>>
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
>> ---
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index fe4796c..0c4417f 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -133,6 +133,47 @@ test_expect_success 'right alignment' '
>> +cat >expect <<-\EOF
>> +|       refname is refs/heads/master       |refs/heads/master
>> +|        refname is refs/heads/side        |refs/heads/side
>> +|         refname is refs/odd/spot         |refs/odd/spot
>> +|     refname is refs/tags/double-tag      |refs/tags/double-tag
>> +|        refname is refs/tags/four         |refs/tags/four
>> +|         refname is refs/tags/one         |refs/tags/one
>> +|     refname is refs/tags/signed-tag      |refs/tags/signed-tag
>> +|        refname is refs/tags/three        |refs/tags/three
>> +|         refname is refs/tags/two         |refs/tags/two
>> +EOF
>> +
>> +test_align_permutations() {
>
> Style: in shell scripts, add space before ()
>

>
>> +               test_expect_success 'align permutations' '
>
> This title is not as illuminating as it could be. It would be better
> to indicate which permutation is being tested. For instance:
>
>     test_expect_success "align:$option" ...
>
> Note the double-quotes. Or:
>
>     test_expect_success "align permutation: $option" ...
>
> or something.
>

>> +               git for-each-ref --format="|%(align:$option)refname is %(refname)%(end)|%(refname)" >actual &&
>
> This is not wrong, per se, but referencing $option inside the
> non-interpolating single-quote context of the test body makes it a bit
> harder to understand than it need be. As it is, at the time that the
> test body actually gets executed, $option still evaluates to the
> desired permutation value so it works. However, it would show intent
> more clearly and be more robust to use a double-quote context to
> interpolate $option into the git-for-each-ref invocation directly
> rather than allowing the test body to pick up the value at execution
> time.
>
> Fixing this means using double- rather than single-quotes for the test
> body, which means you'd also want to flip the  double-quotes wrapping
> the --format= argument over to single-quotes. Also, for style
> consistency, indent the test body. The end result should be something
> like this:
>
>     test_align_permutations () {
>         while ...
>         do
>             test_expect_success "align:$option" "
>                 git for-each-ref --format='...$option...' >actual &&
>                 ...
>             "
>         done
>     }
>

>> +               test_cmp expect actual
>> +               '
>> +       done;
>
> Style: drop the semi-colon
>

> More below...
>
>> +}
>> +
>> +test_align_permutations <<-\EOF
>> +       middle,42
>> +       42,middle
>> +       position=middle,42
>> +       42,position=middle
>> +       middle,width=42
>> +       width=42,middle
>> +       position=middle,width=42
>> +       width=42,position=middle
>> +EOF
>> +
>> +# Last one wins (silently) when multiple arguments of the same type are given
>> +
>> +test_align_permutations <<-\EOF
>> +       32,width=42,middle
>> +       width=30,42,middle
>> +       width=42,position=right,middle
>> +       42,right,position=middle
>> +EOF
>
> Overall, this version is much nicer now that the tests are
> table-driven rather than each permutation being copied/pasted.
>

> This is a tangent, but it's disappointing that this entire test script
> is skipped if GPG is not installed, especially since none of these
> tests seem to care at all about signed tags. By requiring GPG
> (unnecessarily), these tests likely are not getting run as widely as
> they ought to. Consequently, it probably would be a good idea to drop
> the GPG requirement from the top of the file and have the "setup" test
> create lightweight tags ("git tag ...") rather than signed ones ("git
> tag -s ...").