On Sat, Jan 6, 2018 at 12:26 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> 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.
I asked the question before rewriting and resending, and now I agree
that we do not want to have these small helpers.
Thanks,
Stefan
