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

From: Pranit Bauva <pranit.bauva@gmail.com>
Date: May 7, 2016, 13:07 UTC
Message-ID: <CAFZEwPOA9c8eQAacnuB1n=juOhN1zsMdzcupS3ijqwYFPRrW4A@mail.gmail.com>
In-Reply-To: <xmqqtwia25qc.fsf@gitster.mtv.corp.google.com>

On Sat, May 7, 2016 at 3:45 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> Pranit Bauva <pranit.bauva@gmail.com> 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