patch, 2 partsbisect--helper: `bisect_log` shell function in C

> Reimplement the `bisect_voc` shell function in C. This is a too small
> function to be called as a subcommand though the working of this
> function has been tested by calling it as a subcommand.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Pranit Bauva <pranit.bauva@gmail.com>

> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>
> ---
> The PR for testing of this function by the subcommand approach can be
> found at:
> https://github.com/pranitbauva1997/git/pull/5
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
>  builtin/bisect--helper.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 87764fe..455f1cb 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -94,6 +94,16 @@ int bisect_log(void)
>         return 0;
>  }
>
> +int bisect_voc(const char *term)
> +{
> +       if (!strcmp(term, "bad"))
> +               printf("bad|new\n");
> +       if (!strcmp(term, "good"))
> +               printf("good|old\n");
> +
> +       return 0;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         enum {
> --
> 2.8.2
>

> Hi Pranit,
>
> On Sat, 14 May 2016, Pranit Bauva wrote:
>
>> Reimplement the `bisect_voc` shell function in C. This is a too small
>> function to be called as a subcommand though the working of this
>> function has been tested by calling it as a subcommand.
>
> This leaves me puzzled as to what this patch is supposed to do. Maybe
> rename this function to have a more intuitive name, and then throw in a
> patch that makes use of the function?

>> +int bisect_voc(const char *term)
>> +{
>> + if (!strcmp(term, "bad"))
>> + printf("bad|new\n");
>> + if (!strcmp(term, "good"))
>> + printf("good|old\n");
>
> If you meant to use this as a helper command, then the implementation is
> right, but you're not doing that.

> 1) First, I'd wait for this "one day" to happen. In general, write code
>    when you need it, don't write it ahead of time. Currently, you have
>    dead and untested code (I know, *you* have tested it, but it's still
>    "untested" as far as git.git is concerned). Dead code may bother
>    people reading the code (one would not understand why it's there),
>    and untested code means it may break later without anyone noticing.
>

>   which gives one more argument for 1): once you have a use-case, you
>   can design the API properly, and not blindly guess that you're going
>   to need printf. Actually, writting these 2 example lines, I also
>   noticed that the parameters could/should be an enum type rather than
>   a string, it makes the code both more efficient and clearer.
>

> On Mon, May 16, 2016 at 12:10 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 14 May 2016, Pranit Bauva wrote:
> >
> >> Reimplement the `bisect_voc` shell function in C. This is a too small
> >> function to be called as a subcommand though the working of this
> >> function has been tested by calling it as a subcommand.
> >
> > This leaves me puzzled as to what this patch is supposed to do. Maybe
> > rename this function to have a more intuitive name, and then throw in a
> > patch that makes use of the function?
> 
> Are you suggesting to first have an introductory patch which will
> rename the function in the shell script and then this patch which will
> convert the "new" shell function to C? I can do this. I have to think
> of a nice name. How does 'good_or_bad" sound?

> +int bisect_voc(const char *term)
> +{
> +	if (!strcmp(term, "bad"))
> +		printf("bad|new\n");
> +	if (!strcmp(term, "good"))
> +		printf("good|old\n");

> Reimplement the `bisect_log` shell function in C and add a
> `--bisect-log` subcommand to `git bisect--helper` to call it from
> git-bisect.sh .
>
> Using `--bisect-log` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other method.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
> +int bisect_log(void)

> +               return error(_("We are not bisecting."));
> +
> +       printf("%s", buf.buf);
> +       strbuf_release(&buf);
> +
> +       return 0;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
> @@ -109,6 +127,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                 if (argc != 2)
>                         die(_("--write-terms requires two arguments"));
>                 return write_terms(argv[0], argv[1]);
> +       case BISECT_LOG:

> On Fri, May 13, 2016 at 4:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> bisect--helper: `bisect_log` shell function in C
>
> Do you need to insert "rewrite" or "reimplement" in the subject?
>
>> Reimplement the `bisect_log` shell function in C and add a
>> `--bisect-log` subcommand to `git bisect--helper` to call it from
>> git-bisect.sh .
>>
>> Using `--bisect-log` subcommand is a temporary measure to port shell
>> function to C so as to use the existing test suite. As more functions
>> are ported, this subcommand will be retired and will be called by some
>> other method.
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>> ---
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> @@ -79,11 +80,26 @@ int write_terms(const char *bad, const char *good)
>> +int bisect_log(void)
>
> s/^/static/

>> +{
>> +       struct strbuf buf = STRBUF_INIT;
>> +
>> +       if (strbuf_read_file(&buf, ".git/BISECT_LOG", 256) < 0)
>
> As mentioned in my review of the "write-terms" rewrite, hardcoding
> ".git/" here is wrong for a variety of reasons. See get_git_dir(),
> get_git_common_dir(), etc. in cache.h instead.

>> +               return error(_("We are not bisecting."));
>> +
>> +       printf("%s", buf.buf);
>> +       strbuf_release(&buf);
>> +
>> +       return 0;
>> +}
>> +
>>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>  {
>> @@ -109,6 +127,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>>                 if (argc != 2)
>>                         die(_("--write-terms requires two arguments"));
>>                 return write_terms(argv[0], argv[1]);
>> +       case BISECT_LOG:
>
> Shouldn't you be die()ing here with an appropriate error message if
> argc is not 0?