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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Pranit Bauva <pranit.bauva@xxxxxxxxx> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]