From: Junio C Hamano <email@example.com> To: Eyal Soha <firstname.lastname@example.org> Cc: Jeff King <email@example.com>, firstname.lastname@example.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: <email@example.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 <firstname.lastname@example.org> writes: > On Sat, Jan 18, 2020 at 9:51 AM Junio C Hamano <email@example.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 <firstname.lastname@example.org> > > 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.
prev parent 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.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 \ firstname.lastname@example.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