Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()

[Christian Couder <christian.couder@gmail.com>]

On Mon, May 16, 2016 at 2:35 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>>> I completely missed your point and you want me to go the Eric Sunshine's way?
>>>>
>>>> I am neutral.
>>>>
>>>> When I read your response to Eric's "top down" suggestion, I didn't
>>>> quite get much more than "I started going this way, and I do not
>>>> want to change to the other direction.", which was what I had the
>>>> most trouble with.  If your justification for the approach to start
>>>> from building a tiny bottom layer that will need to be dismantled
>>>> soon and repeat that (which sounds somewhat wasteful) were more
>>>> convincing, I may have felt differently.
>>>
>>> Sorry if it seemed that "I have done quite some work and I don't want
>>> to scrape it off and redo everything". This isn't a case for me. I
>>> think of this as just a small part in the process of learning and my
>>> efforts would be completely wasted as I can still reuse the methods I
>>
>> efforts would **not** be completely wasted
>>
>>> wrote. This is still open for a "philosophical" discussion. I am
>>> assuming 1e1ea69fa4e is how Eric is suggesting.
>
> Speaking of 1e1ea69 (pull: implement skeletal builtin pull,
> 2015-06-14), one of the (numerous) things Paul Tan did which impressed
> me was to formally measure test suite coverage of the commands he was
> converting to C, and then improve coverage where it was lacking. That
> approach increases confidence in the conversion far more than fallible
> human reviews do.
>
> Setting aside the top-down vs. bottom-up discussion, as a reviewer
> (and user) I'd be far more interested in seeing you spend a good
> initial chunk of your project emulating Paul's approach to measuring
> and improving test coverage (though I don't know how your GSoC mentors
> feel about that).

If we could test each `git bisect-helper` subcommand that has been
converted to C separately, then I think it would be quite easy and a
good idea. And maybe Paul's approach is really great and easy to use,
but I haven't followed his GSoC, so I cannot tell.

As we take the approach of testing the whole thing, which is made of a
number of different source files (shell code in git-bisect.sh, C code
in `git bisect-helper` subcommands and in bisect.{c,h}), I am worried
that Pranit would spend too much time initially not only to measure
test coverage but especially to find ways to improve it.

The good thing with converting code incrementally to C is that one
does not need to understand everything first. While if one wants to
increase test coverage initially, it might be needed to understand
many things.

What I started asking though is to take advantage of bugs that are
found, and were not covered by the test suite, to improve test
coverage.

Also starting to convert functions right away can make the student
feel productive early and in turn make everyone else happy to see some
progress.