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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Nov 10, 2012, 19:13 UTC
Message-ID: <CAMP44s2WHZh8Jc6PaM-9h9Hkf3Un_xAC=29TNFF1u9sSEYA7gg@mail.gmail.com>
In-Reply-To: <20121110123250.GR12052@goldbirke>

On Sat, Nov 10, 2012 at 1:32 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 28 quoted lines

> 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.

I disagree. If we add a new parameter to __gitcomp(), and we need to add a new parameter to test_gitcomp(), so be it. Yes, we might change the behavior of the other tests, but that's what reviews are for: to make sure we don't alter other behavior by mistake. That's what we do for code, and that what we should do for tests.

But in this particular case nothing would need to change because test_gitcomp() would pass whatever arguments it receives, being four, or five, or twenty. So this is not a concern, maybe some other kind of change, but not this.

Compare this: http://article.gmane.org/gmane.comp.version-control.git/208168

To this: http://article.gmane.org/gmane.comp.version-control.git/207927

If we ever need to make changes to the __gitcomp tests, a small change is better than a big change; IOW: the test code would be more maintainable.

Even more, the end result is much less code: less code = more maintainability.

Show 5 quoted lines

> 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.

Maybe it's easier for you to understand, but certainly not for other people: 'declare -a COMPREPLY'? cur? print_comp? What does that even mean? Chances are most people don't even know what __gitcomp is. Fortunately they don't have to dig too deep to find all those things out, but they can do the same for test_gitcomp().

It might make sense to add some comment on top of test_gitcomp to explain the arguments and the input to make things easier, but that would only make it more readable, not less, than the current situation, because right now you would have to add a similar comment to each and every block of code that calls __gitcomp.

Functions make our life easier.

> 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).

Yes, and that version is much bigger than this: http://article.gmane.org/gmane.comp.version-control.git/208168

Code that allows the later patch is more maintainable.

Show 13 quoted lines

> 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.

What I do is copy the function to test_gitcomp2() and add the debugging there, and only call it from the places where I want the debugging. I don't think this is an issue.

In fact, I think it's an advantage; sometimes that's exactly what you want, to add the debugging for everything.

> 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.

That's policy. We can decide what each test-case contains right here, on the mailing list. There's no need to have verbose code to slightly hint a policy.

Cheers.

-- 
Felipe Contreras