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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 25, 2016, 22:58 UTC
Message-ID: <CAPig+cT56AiO-3GNzia7UsGZFM5zu5zsEC31XPuq-by1p4+sbw@mail.gmail.com>
In-Reply-To: <1451980994-26865-13-git-send-email-Karthik.188@gmail.com>

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@gmail.com> wrote:

Show 26 quoted lines

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

Show 21 quoted lines

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