Re: [PATCH] tests: Introduce test_seq
- From
Jeff King <peff@peff.net>
- Date
- Aug 6, 2012, 20:16 UTC
- Message-ID
- <20120806201600.GA11078@sigill.intra.peff.net>
- In-Reply-To
- <501D4FF0.4060109@kdbg.org>
On Sat, Aug 04, 2012 at 06:38:08PM +0200, Johannes Sixt wrote:
Show 22 quoted lines
> And the reason for this is that we always told people "don't use seq"
> and they submitted an updated patch. What would we have to do now? We
> have to tell them "don't use seq, use test_seq". Therefore, the patch
> does not accomplish anything useful, IMO.
>
> The function should really just be named 'seq'.
>
> Or how about this strategy:
>
> seq () {
> unset -f seq
> if ! seq 1 2 >/dev/null 2>&1
> then
> # don't have a working seq; provide it as a function
> seq () {
> insert your definition here
> }
> fi
> seq "$@"
> }
>
> but it is not my favorite.No, falling back just makes that problem worse. Our test_seq is not fully compatible with seq. So anyone who uses an advanced feature of seq (like "seq 0 100 10" or "seq -f %02g 1 10") will have the test work on their system (with seq) and then break on some other random platform. So instead of saying "no, don't use seq, use test_seq", reviewers have to catch it and say "don't use some features of seq, because the fallback doesn't have them".
If you eliminate the fallback, then at least the reviewers do not have to catch it (the tests will never work for the patch writer, since they will always use our feature-less seq replacement). But I find it slightly confusion-inducing to call something that is not seq-compatible "seq".
-Peff