On Thu, Feb 8, 2018 at 3:43 PM, Jeff King <peff@xxxxxxxx> wrote: > On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote: >> +/* >> + * Resolve the constants as returned by git_config_colorbool() >> + * (specifically "auto") to a boolean answer. >> + */ >> +extern int want_color(int var); > > This explanation left me even more confused about what should go in > "var", and I think I'm the one who wrote the function. ;) Agreed, this still fails to (directly) answer the question I asked in [1] about what 'var' is. > I think the point is that "var" is a quad-state variable (yes, no, auto, > or "unknown") and we are converting to a boolean. This would probably be > a lot more clear if GIT_COLOR_* were all enum values and not #defines, > and this function took the matching enum type. > > So I think that's what you were trying to name with "constants as > returned by...", but it definitely took me some thinking to parse it. Rather than talking about plural "constants" (which makes it more confusing), it would likely be helpful for it to say (explicitly) that the caller passes in the result of git_config_colorbool() as 'var'. Or something like that. >> +/* >> + * Output the formatted string in the specified color (and then reset to normal >> + * color so subsequent output is uncolored). Omits the color encapsulation if >> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting >> + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf >> + * instead, up to its first NUL character. >> + */ > > It probably doesn't matter much in practice, but the color_print_strbuf > behavior sounds like a bug. Shouldn't it print the whole strbuf, even if > it has an embedded NUL? I (parenthetically) suggested[1] the same about fixing the bug/misbehavior, though doing so is outside the scope of this particular patch. [1]: https://public-inbox.org/git/CAPig+cQVGsQk3tj43V6f3rFTD8smDxqWvug_u4__EWxOQG90xA@xxxxxxxxxxxxxx/
