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

[Pranit Bauva]

Hey Eric,
Sorry for the late reply. I was on vacation.

On Mon, May 16, 2016 at 6:05 AM, Eric Sunshine  wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva  wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva  wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano  wrote:
 Pranit Bauva  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).

Just adding to the points mentioned by Christian.

I had initially planned to first improve test coverage and then start
with function conversion as I mentioned in my introductory mail[1]. I
also pointed out that I did some work searching about the tools to
test coverage (kcov as Matthieu) suggested and I found that it is not
that easy to set it up. Then Christian pointed it out (privately) that
I can do this afterwards too before the code is finally merged. And
also I am trying to see the test coverage as and when I am converting
each function.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/292308

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christian Couder]

On Mon, May 16, 2016 at 2:35 AM, Eric Sunshine  wrote:
> On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva  wrote:
>> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva  wrote:
>>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano  wrote:
 Pranit Bauva  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

On Sun, May 8, 2016 at 9:34 AM, Pranit Bauva  wrote:
> On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva  wrote:
>> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano  wrote:
>>> Pranit Bauva  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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

On Sun, May 8, 2016 at 11:53 AM, Pranit Bauva  wrote:
> On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano  wrote:
>> Pranit Bauva  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.
>
> Why I think its better to go the subcommand way:
>  * I can test **C implementation** of the function to verify whether
> it was done in a proper way. By using a "top down" approach, I can
> make the test suite passing by running the code from git-bisect.sh not
> the re-written C code.
>  * I have made a very few minor mistakes in conversion of
> check_term_format() which just messed up my head (I actually spent 3
> days before Christian pointed out that '!' was missing). Imagine
> debugging the complete git-bisect.sh to find a very small error after
> a complete implementation.
>  * Using subcommands, I can easily verify whether I have done it the
> right way or not.
>  * It makes the review process very simple. The reviewers/testers can
> verify that my method is working correctly and well computers can
> detect errors better than humans.
>  * I can convert small functions which can be reviewed easily rather
> than dumping a big series.
>
> What I think is that the bottom up approach will make life easier for
> the me and for reviewer. Yes, it does make the code "unclean" for the
> time being and if it is between releases then all the more painful.
> Well, the latter part can be avoided by keeping it in next.
>
> Please point out if I am mistaken about anything.

There is also a third way in which I can go which is kind of merge of
both the ways:

 * Make two branches, one will contain the top down approach and
another will contain the subcommand approach.

 * I will write convert a function as a subcommand and test it locally
as well as on Travis-Cl by a PR on my git.git repo. The
reviewers/testers can consult the Travis-CI for output of the tests.
This will also reduce the need of locally compiling/testing.

 * I will then just write the function as it is in my "top down"
branch and thus send the patch to the mailing list. These can be kept
in next until it finishes.

 * Then will remove linking of git-bisect.sh and thus finalize.

This will be a bit more intensive not in a coding way but in
maintaining the repo. So I will have more experience of maintaining a
git repo. :)

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

On Sun, May 8, 2016 at 7:55 AM, Junio C Hamano  wrote:
> Pranit Bauva  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
wrote. This is still open for a "philosophical" discussion. I am
assuming 1e1ea69fa4e is how Eric is suggesting.

Why I think its better to go the subcommand way:
 * I can test **C implementation** of the function to verify whether
it was done in a proper way. By using a "top down" approach, I can
make the test suite passing by running the code from git-bisect.sh not
the re-written C code.
 * I have made a very few minor mistakes in conversion of
check_term_format() which just messed up my head (I actually spent 3
days before Christian pointed out that '!' was missing). Imagine
debugging the complete git-bisect.sh to find a very small error after
a complete implementation.
 * Using subcommands, I can easily verify whether I have done it the
right way or not.
 * It makes the review process very simple. The reviewers/testers can
verify that my method is working correctly and well computers can
detect errors better than humans.
 * I can convert small functions which can be reviewed easily rather
than dumping a big series.

What I think is that the bottom up approach will make life easier for
the me and for reviewer. Yes, it does make the code "unclean" for the
time being and if it is between releases then all the more painful.
Well, the latter part can be avoided by keeping it in next.

Please point out if I am mistaken about anything.

Regards,
Pranit Bauva
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

On Sat, May 7, 2016 at 3:45 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> A very interesting fact to note:
>> I made a mistake by dropping of the first argument 'term' in the function
>> one_of() and compiled and the test suite passed fully (which was pointed
>> out by Christian). This shows that the test coverage is incomplete.
>> I am
>> currently writing tests to improve on that coverage. It will be included
>> separately.
>>
>> Link to v4: http://thread.gmane.org/gmane.comp.version-control.git/293488
>>
>> Changes wrt v4:
>>  * Stick with 'subcommand' in the commit message.
>>  * Rename enum as 'subcommand' to make it singular.
>
> I've already said what needs to be said on this.
>
>>  * s/bug/BUG/g
>>  * Drop _() in the default of switch case
>>  * Use some suggestions for commit message no. 2 and also include why I am
>>using subcommand.
>
> I am not particularly impressed by the rationale, though.
>
> As a starter project to show that you learned how to add a new mode
> to bisect--helper, check-term-format may be a small enough function
> that is easy to dip the toe into the codebase, so in that sense it
> may be an appropriate first step, but otherwise it is not all that
> interesting, especially when we _know_ that it will be discarded.

I do understand that check-term-format was a suggestion just for me to
"Get a taste" for the project.
I have now also converted write_terms() function. I have made a PR[1]
which needs a few things to be addressed.
Just a summary of what the PR does:
 * Converts the shell function write() terms to C
 * Adds a subcommand for write_terms()
 * Remove the subcommand for check_term_format()
 * Call the method check_term_format() from inside write_terms().

Do you want me to include both of the functions check_term_format()
and write_terms() in the same commit or a different commit.

OR,
I completely missed your point and you want me to go the Eric Sunshine's way?

[1]: https://github.com/pranitbauva1997/git/pull/3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  writes:

> A very interesting fact to note:
> I made a mistake by dropping of the first argument 'term' in the function
> one_of() and compiled and the test suite passed fully (which was pointed
> out by Christian). This shows that the test coverage is incomplete. 
> I am
> currently writing tests to improve on that coverage. It will be included
> separately.
>
> Link to v4: http://thread.gmane.org/gmane.comp.version-control.git/293488
>
> Changes wrt v4:
>  * Stick with 'subcommand' in the commit message.
>  * Rename enum as 'subcommand' to make it singular.

I've already said what needs to be said on this.

>  * s/bug/BUG/g
>  * Drop _() in the default of switch case
>  * Use some suggestions for commit message no. 2 and also include why I am
>using subcommand.

I am not particularly impressed by the rationale, though.

As a starter project to show that you learned how to add a new mode
to bisect--helper, check-term-format may be a small enough function
that is easy to dip the toe into the codebase, so in that sense it
may be an appropriate first step, but otherwise it is not all that
interesting, especially when we _know_ that it will be discarded.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

A very interesting fact to note:
I made a mistake by dropping of the first argument 'term' in the function
one_of() and compiled and the test suite passed fully (which was pointed
out by Christian). This shows that the test coverage is incomplete. I am
currently writing tests to improve on that coverage. It will be included
separately.

Link to v4: http://thread.gmane.org/gmane.comp.version-control.git/293488

Changes wrt v4:
 * Stick with 'subcommand' in the commit message.
 * Rename enum as 'subcommand' to make it singular.
 * s/bug/BUG/g
 * Drop _() in the default of switch case
 * Use some suggestions for commit message no. 2 and also include why I am
   using subcommand.
 * Drop of the latter part in the comment of one_of()
 * Remove flag from check_term_format()
 * Use return error() instead of die()
 * Drop '\n' from the localization strings
 * Include localization for a missed string
 * A comment in function check_term_format() is re-wrapped to fit in 80 columns
 * Use 'term' instead of 'ref'
 * Reword the string in the default of switch case
 * I have kept the commit message same as previous for commit no. 1 except for
   addressing the minor nits.

Pranit Bauva (2):
  bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  bisect: rewrite `check_term_format` shell function in C

 builtin/bisect--helper.c | 76 
 git-bisect.sh| 31 ++--
 2 files changed, 72 insertions(+), 35 deletions(-)

-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html