Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eyal Soha <shawarmakarma@gmail.com>
Cc: Jeff King <peff@peff.net>, git@vger.kernel.org
Subject: Re: [PATCH 1/3] color.c: Refactor color_output to use enums
Date: Tue, 21 Jan 2020 09:49:11 -0800
Message-ID: <xmqqmuagbsyw.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CANsz78J93XynUJkBvvdD=BfXPQ-Wq17uzqPi3opMAPPKbt2Rwg@mail.gmail.com> (Eyal Soha's message of "Tue, 21 Jan 2020 08:37:33 -0800")

Eyal Soha <shawarmakarma@gmail.com> writes:

> 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.

      reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CANsz78+ugmd62F4Qk+VT7Pi=ZPtMSkZjXOwLNRCFhoS9jrOkeQ@mail.gmail.com>
     [not found] ` <CANsz78K-BiswWPdhd_N25NuApcv7zSb2cw2Y9DSinkpNpuogYw@mail.gmail.com>
2020-01-07 15:36   ` Fwd: Add support for axiterm colors in parse_color Eyal Soha
2020-01-08  9:52     ` Jeff King
2020-01-10  0:20       ` Eyal Soha
2020-01-10 11:15         ` Jeff King
2020-01-10 15:02           ` Eyal Soha
2020-01-15 15:32             ` Eyal Soha
2020-01-10 15:05           ` [PATCH 1/3] color.c: Refactor color_output to use enums Eyal Soha
2020-01-10 15:05             ` [PATCH 2/3] color.c: Support bright aixterm colors Eyal Soha
2020-01-15 22:42               ` Jeff King
2020-01-10 15:05             ` [PATCH 3/3] color.c: Alias RGB colors 8-15 to " Eyal Soha
2020-01-15 22:45               ` Jeff King
2020-01-15 22:33             ` [PATCH 1/3] color.c: Refactor color_output to use enums Jeff King
2020-01-16 18:01             ` Junio C Hamano
2020-01-16 18:23               ` Jeff King
2020-01-16 19:25                 ` Eyal Soha
2020-01-18 14:53                   ` Eyal Soha
2020-01-18 14:53                     ` [PATCH 2/3] color.c: Support bright aixterm colors Eyal Soha
2020-01-18 18:47                       ` Junio C Hamano
2020-01-21 16:52                         ` Eyal Soha
2020-01-21 16:56                           ` [PATCH 1/3] color.c: refactor color_output arguments Eyal Soha
2020-01-21 16:56                             ` [PATCH 2/3] color.c: support bright aixterm colors Eyal Soha
2020-01-23 22:54                               ` Junio C Hamano
2020-01-21 16:56                             ` [PATCH 3/3] color.c: alias RGB colors 8-15 to " Eyal Soha
2020-01-23 22:50                             ` [PATCH 1/3] color.c: refactor color_output arguments Junio C Hamano
2020-02-11 19:36                             ` [PATCH v3 0/3] es/bright-colors (hopefully final) reroll Junio C Hamano
2020-02-11 19:36                               ` [PATCH v3 1/3] color.c: refactor color_output arguments Junio C Hamano
2020-02-11 19:46                                 ` Jeff King
2020-02-11 23:01                                   ` Eyal Soha
2020-02-11 23:06                                     ` Junio C Hamano
2020-02-11 19:36                               ` [PATCH v3 2/3] color.c: support bright aixterm colors Junio C Hamano
2020-02-11 19:36                               ` [PATCH v3 3/3] color.c: alias RGB colors 8-15 to " Junio C Hamano
2020-01-21 17:37                           ` [PATCH 2/3] color.c: Support bright " Junio C Hamano
2020-01-18 14:53                     ` [PATCH 3/3] color.c: Alias RGB colors 8-15 to " Eyal Soha
2020-01-18 18:34                       ` Junio C Hamano
2020-01-18 17:51                     ` [PATCH 1/3] color.c: Refactor color_output to use enums Junio C Hamano
2020-01-21 16:37                       ` Eyal Soha
2020-01-21 17:49                         ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmuagbsyw.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=shawarmakarma@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git