Re: [PATCH 1/3] color.c: Refactor color_output to use enums

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 21, 2020, 17:49 UTC
Message-ID: <xmqqmuagbsyw.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CANsz78J93XynUJkBvvdD=BfXPQ-Wq17uzqPi3opMAPPKbt2Rwg@mail.gmail.com>

Eyal Soha <shawarmakarma@gmail.com> writes:

Show 23 quoted lines

> On Sat, Jan 18, 2020 at 9:51 AM Junio C Hamano <gitster@pobox.com> wrote:
>> Please downcase Refactor; that way this change would not
>> meaninglessly stand out in the "git shortlog --no-merges" output.
>
> Sure, no problem.
>
>> The blank before your sign-off means all the times spent gets
>> discarded, which is not exactly encouraging to the reviewers.
>
> So I should make a better description for the patch?  Sure!  What
> should I put?  It's kind of hard to get a good description that
> describes the refactoring without digging into the reasoning behind
> it, which is in the follow-up patch.  What kind of description should
> I give?  How about like this:
>
>     color.c: refactor color_output arguments
>
>     color_output() now uses a more descriptive "background" argument
>     instead of "type".
>
>     Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
>
> Suits?

Quite a lot is missing from these two lines what I mentioned as examples in the part you omitted from your quote, I think.

 - what design decision was made?  e.g. how .value is expressed
   differently from the code before this patch, e.g. how "fore/back"
   information is passed from the caller differently between the
   code before and after this patch, etc.

 - why these design choices are good ones?  e.g. making .value 30-37
   range instead of 0-7 range and pass 0/10 as offset from the base
   foreground value when the caller wants to give background color
   allows us to do X better than the original arrangement?

Perhaps there are some other things we discussed in the review thread that may be worth resurrecting, but at least I recall I had trouble understanding why you chose to do things the way the patch did for the above two points.

After all, anything that reviewers needed help in their first reading with your explanation to understand is a good candidate [*1*] that needs clarification to help future readers of the "git show" output of the commit resulting from your final version of the patch.

Thanks.

[Footnote]

*1* There of course are cases where a simple explanation results in
    a reviewer who was initially confused to say "Ah, I misread a
    word, but your original is good after I re-read it carefully",
    so not everything a reviewer gets confused necessarily deserves
    mention in the final version of the log message.  But these are
    good starting points to anticipate confusion by future readers.