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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2020, 17:51 UTC
Message-ID: <xmqqftgcd55s.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20200118145318.5177-1-shawarmakarma@gmail.com>

Eyal Soha <shawarmakarma@gmail.com> writes:

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

Please downcase Refactor; that way this change would not meaninglessly stand out in the "git shortlog --no-merges" output.

> Signed-off-by: Eyal Soha <shawarmakarma@gmail.com>
> ---
>  color.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)

You got help from multiple reviewers on your earlier round and at least the discussion thread would have made it clear what kind of things would help readers understand what design decision was made (e.g. .value is not 0-7 but 30-37, the caller passes "am I talking about the background color?" boolean, there are others) and why this design was chosen (e.g. .value is not 0-7 because we want to add brighter variant later, there would be others that correspond to the "here are the design decisions I made before coming up with this version").

The reviewing is *not* just about explaining your thought to and convincing your reviewers---it is where reviewers help you to explain what your change wanted to do and why it did so to future readers of "git log".

The blank before your sign-off means all the times spent gets discarded, which is not exactly encouraging to the reviewers.