Re: [PATCH v2 2/5] t1500: test_rev_parse: facilitate future test enhancements

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, May 18, 2016 at 1:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 18, 2016 at 12:38 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> Quoting Eric Sunshine <sunshine@sunshineco.com>:
>>> +       for o in is-bare-repository \
>>> +                is-inside-git-dir \
>>> +                is-inside-work-tree \
>>> +                show-prefix \
>>> +                git-dir
>>> +       do
>>> +               test $# -eq 0 && break
>>> +               expect="$1"
>>> +               test_expect_success "$name: $o" '
>>> +                       echo "$expect" >expect &&
>>> +                       git rev-parse --$o >actual &&
>>
>> I think that "--$o" looks really weird, but that's subjective, of course.
>>
>> However, the idea popped up in an other thread[1] that we might want
>> something like 'git rev-parse --absolute-path --git-dir', which wouldn't
>> really work with '--$o'.
>>
>> Even if we don't go that route, perhaps it would be better to list the
>> options to be tested including their doubledash prefix.
>
> As this series is only about modernizing t1500, I'd prefer to keep the
> conversion faithful to the original which titles each test "$name:
> is-bare-repository", "$name: is-inside-git-dir", etc., and the current
> approach does so without additional complexity.
>
> I have no objection to upgrading the for-loop items to include the
> leading dashes or updating the logic to support --absolute-path, but
> such changes are outside the scope of this series and can easily be
> built atop it. Also, due to severe time constraints, I'd rather not
> re-roll only for a superficial and subjective change such as adding
> leading dashes to for-loop items.

On reflection, I agree with your points and will include the change in
the re-roll. Thanks.