On Thu, Jan 4, 2018 at 10:10 PM, Elijah Newren <newren@xxxxxxxxx> wrote: > On Wed, Jan 3, 2018 at 5:52 PM, SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote: > >>> + test $(git rev-parse :0:y/b) = $(git rev-parse O:z/b) && >> >> There is a test helper for that :) >> >> test_cmp_rev :0:y/b O:z/b >> >> Note, that this is not only a matter of useful output on failure, but >> also that of correctness and robustness. > > > Cool, good to know. Is there any reason test_cmp_rev is not > documented in t/README? Because of mere oversight, perhaps? (both for this and for the 'verbose' helper) I forgot that the test helpers are documented in 't/README' (well, apparently only some of them), I usually go straight to 't/test-lib-functions.sh' to find test helpers or to learn what they are doing. > I already changed these yesterday, as part of trying to avoid the use > of plain 'test' as you suggested (I was just waiting another day or so > for more feedback before resubmitting the series). Since I tended to > have several of these rev-parse comparisons in a single test, I simply > combined them: > git rev-parse >actual > :0:y/b :1:x/c :2:x/c :3:x/c > git rev-parse >expect > O:z/b O:x/c A:x/c B:x/c > test_cmp expect actual > > That does result in fewer rev-parse invocations, which is probably > good, but the test_cmp_rev does seem slightly more readable. Hmmm... Yeah, the significantly more 'git rev-parse' invocations are the reason why I didn't recommend 'test_cmd_rev' in those cases. Folks running the test suite on Windows probably won't be very happy about such a change. Though I had a bit of a "Huh?!" moment when seeing those combined 'git rev-parse' pairs for the first time, I don't think converting them to a list of 'test_cmp_rev' invocations would make it notably easier to read. Have you considered vertically aligning the corresponding rev arguments in those 'git rev-parse' pairs? Furthermore, on second look, while 'test_cmp_rev' does produce output on failure, it's not really useful for humans, being only a diff of two files with a single object name in each. As it is, I don't think it's any more useful than the output of those combined 'git rev-parse'-'test_cmp' combos would be, so no gain there, either. OTOH, we could easily enhance 'test_cmp_rev' to include the given revs in its error message... I leave it up to you.
