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

[Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>]

Pranit Bauva <pranit.bauva@gmail.com> writes:

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

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.

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));

   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.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/