On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <sbeller@xxxxxxxxxx> wrote:
> On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@xxxxxxxxxx> 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.
