From: Eric Sunshine Date: Wed, 18 May 2016 17:43:29 GMT Subject: Re: [PATCH v2 2/5] t1500: test_rev_parse: facilitate future test enhancements Message-ID: In-Reply-To: On Wed, May 18, 2016 at 1:32 PM, Eric Sunshine wrote: > On Wed, May 18, 2016 at 12:38 PM, SZEDER Gábor wrote: >> Quoting Eric Sunshine : >>> + 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.