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

[Eric Sunshine <sunshine@sunshineco.com>]

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 ()

> +       while read -r option; do

Style: drop semi-colon and place 'do' on its own line

> +               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 ...").