Re: [PATCH v2 2/5] t1500: test_rev_parse: facilitate future test enhancements
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- May 18, 2016, 17:43 UTC
- Message-ID
- <CAPig+cQtt1TcHdRgZcB_fJ7_SEbZLjbKP04Eq=eRCCkU6PnQ1Q@mail.gmail.com>
- In-Reply-To
- <CAPig+cTv8UNg+0taNF4B2wytn0d-jGboCntxBy8TPgY+1qW85w@mail.gmail.com>
On Wed, May 18, 2016 at 1:32 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 34 quoted lines
> 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.