Re: What's cooking in git.git (Nov 2012, #02; Fri, 9)

["SZEDER Gábor" <szeder@ira.uka.de>]

Hi,

On Fri, Nov 09, 2012 at 07:33:31PM -0500, Jeff King wrote:
> On Sat, Nov 10, 2012 at 12:21:48AM +0100, Felipe Contreras wrote:
> > > * fc/completion-test-simplification (2012-10-29) 2 commits
> > >  - completion: simplify __gitcomp test helper
> > >  - completion: refactor __gitcomp related tests
> > >
> > >  Clean up completion tests.
> > >
> > >  There were some comments on the list.
> > >
> > >  Expecting a re-roll.
> > 
> > The second patch I can re-roll, but the first patch needs some
> > external input. My preference is that tests should also be simple and
> > maintainable, SZEDER's preference is that tests are better being
> > explicit and verbose (even if harder to maintain) to minimize possible
> > issues in the tests.
> 
> I think it is better to keep the tests simple and maintainable.

Maintainable?  There is nothing to maintain here.  Users' completion
scripts depend on __gitcomp(), so its behavior shouldn't be changed.
It can only be extended by a fifth parameter or by quoting words when
necessary, but these future changes must not alter the current
behavior checked by these tests, therefore even then these tests must
be left intact.

Simple?  Currently you only need to look at __gitcomp() and the test
itself to understand what's going on.  With this series you'll also
need to look at test_gitcomp(), figure out what its parameters are
supposed to mean, and possibly get puzzled on the way why __gitcomp()
is now seemingly called with only one parameter.

So, I don't see much benefit in this series (except the part to use
print_comp instead of "change IFS && echo", but that's already done in
this patch:
http://article.gmane.org/gmane.comp.version-control.git/207927).

OTOH, this series has some serious drawbacks.

It makes debugging more difficult.  While working on the quoting
issues I managed to break completion tests many-many times lately.  In
normal tests I could add a few debugging instructions to the failed
test to find out where the breakage lies, without affecting other
tests.  However, if the failed test uses the test_completion() helper,
then I have to add debugging instructions to test_completion() itself,
too.  This is bad, because many tests use this helper function and are
therefore affected by the debugging instructions, producing truckloads
of output making it difficult to dig out the relevant parts, or, worse
yet, causing breakages in other tests.  With this series the same
difficulties will come to __gitcomp() tests, too.

It can also encourage writing bad tests, similar to those that managed
to cram many test_completion() lines into a single tests, giving me
headaches to figure out what went wrong this time.


Best,
Gábor