On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <[email protected]> wrote: > On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <[email protected]> > wrote: >> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <[email protected]> wrote: >>> +static inline void colors_unset(const char **use_color, const char >>> **reset_color) >>> +{ >>> + *use_color = ""; >>> + *reset_color = ""; >>> +} >>> + >>> +static inline void colors_set(const char **use_color, const char >>> **reset_color) >>> +{ >>> + *use_color = repeated_meta_color; >>> + *reset_color = GIT_COLOR_RESET; >>> +} >> >> I'm not convinced that this colors_unset() / colors_set() / >> setup_line_color() abstraction is buying much. With this abstraction, >> I found the code more difficult to reason about than if the colors >> were just set/unset manually in the code which needs the colors. I >> *could* perhaps imagine setup_line_color() existing as a separate >> function since it is slightly more complex than the other two, but as >> it has only a single caller through all patches, even that may not be >> sufficient to warrant its existence. > > Have you viewed this patch in context of the following patch? > Originally I was convinced an abstraction was not needed, but > as the next patch shows, a helper per field seems handy.
I did take the other patch into consideration when making the observation, and I still found the code more difficult to reason about than if these minor bits of code had merely been inlined into the callers. I neglected to mention previously that part of the problem may be that these function names do not do a good job of conveying what is being done, thus I repeatedly had to consult the function implementations while reading callers in order to understand what was going on.
