On Tue, Jan 26, 2016 at 4:28 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak <karthik.188@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> 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 ()
>
Will change.
>> + while read -r option; do
>
> Style: drop semi-colon and place 'do' on its own line
Will change.
>
>> + 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.
>
"align:$option" seems right, will change.
>> + 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
> }
>
After reading this and Junio's detailed description stating otherwise.
It's safe to
go with something similar to what Junio suggested :
for option in ...
do
test_expect_success "align:$option" '
git for-each-ref --format="...$option..." &&
...
'
done
>> + test_cmp expect actual
>> + '
>> + done;
>
> Style: drop the semi-colon
>
Will change.
> 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.
>
Thanks.
> 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 ...").
Maybe an independent patch post this series. I'll keep a note to myself.
--
Regards,
Karthik Nayak
--
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