Re: [PATCH v5 0/2] bisect--helper: rewrite of check-term-format()
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()
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()
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()
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()
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()
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()
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()
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
