Re: [PATCH 2/2] bisect--helper: `bisect_voc` shell function in C

From: Pranit Bauva <pranit.bauva@gmail.com>
Date: May 22, 2016, 19:49 UTC
Message-ID: <CAFZEwPN0fydOiKZ4ZShivuNDpU=VAKe=wwimPSi-Yae0JA4Ukw@mail.gmail.com>
In-Reply-To: <CAFZEwPMFBmHUaX+Y8Fpd4BnJiB8N_XBOX30hRsSvb3tm8-MX5w@mail.gmail.com>

Hey Matthieu, Sorry for the late reply. Somehow your email didn't receive my mailbox. I got to see this mail when I was going through the gmane archives.

Matthieu Moy <Matthieu.Moy <at> grenoble-inp.fr> writes:
  Pranit Bauva <pranit.bauva <at> gmail.com> writes:

Show 9 quoted lines

>> +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.

> If you write the function because one day you'll be calling it from C,
> then:

Show 7 quoted lines

> 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.
>

I think this function can wait then. I will include this patch when its really required. I wanted to convert this function ASAP because it was a really tiny one and an easy one.

> 2) Second, you'd need to return the string, not print it. You'll
>    typically use it like this:

>     printf(_("You need to give me at least one %s and one %s"),
>            bisect_voc(BISECT_BAD), bisect_voc(BISECT_GOOD));

Show 6 quoted lines

>   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.
>

Okay I get it. It would be much more efficient to return a enum because its difficult to parse text output into a C program. I hadn't looked further into the function. Thanks for pointing it out early!

I will wait before re-rolling this patch and will do it when I convert bisect_state().

Regards, Pranit Bauva