Re: [PATCH 2/4] builtin/blame: dim uninteresting metadata

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 6, 2018, 08:26 UTC
Message-ID: <CAPig+cQg3o2iicGhSmwPM8dDjtUFSGow5dwprbRNnq4-m5EmFA@mail.gmail.com>
In-Reply-To: <CAGZ79ka22DPHX5_etFREvdVjfsQPzQG66iFgyfsjCdLnwUcAdA@mail.gmail.com>

On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <sbeller@google.com> wrote:

Show 26 quoted lines

> On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> 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.