Re: [PATCH] tests: Introduce test_seq

[Jeff King <peff@peff.net>]

On Sat, Aug 04, 2012 at 06:38:08PM +0200, Johannes Sixt wrote:

> 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