From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI, RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id C2D8E1F885 for ; Fri, 10 Jan 2020 15:02:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728239AbgAJPC6 (ORCPT ); Fri, 10 Jan 2020 10:02:58 -0500 Received: from mail-wm1-f68.google.com ([209.85.128.68]:51728 "EHLO mail-wm1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727892AbgAJPC5 (ORCPT ); Fri, 10 Jan 2020 10:02:57 -0500 Received: by mail-wm1-f68.google.com with SMTP id d73so2345732wmd.1 for ; Fri, 10 Jan 2020 07:02:56 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uszohy38i+KvRLgodXF6V3wE6ioCd1hyPpoa/yQrqHA=; b=VHw7ZNFrvo4gbfGSl7j/OtyI/pbwyyhwrRBUnG3mb3PnS3voj/UDTPfdNhtxxM3AEn BkDa4joyandS5OESUEktONIAmB9+dz2mX0SnOMyEEzcglG9EeWv2cOulPPT+4zXvNWCw +yr4zKS8H8IBfDMCZL6I+MDWgi4Y6q2hdAdToRQle5jqWMsUuHvf5qESbvumJ7hEkpiY HF/XdyU1cXZo1fE5ytjz64ioJRkczTZj9eXTr2m+wSG8WWKnE8ZDsu/QdgbCT05Tx0be UYu7UM4Pnt6mUyon452WTPn1450Js3qH976LXch1OSJRQNSatRe3pDcZ0K1cboziR/rB 2LIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uszohy38i+KvRLgodXF6V3wE6ioCd1hyPpoa/yQrqHA=; b=C1PUoupuAaPf1NLKaRmRyHbh8ad/tuNsFkrPrOVllPPDu8fBbeYnvubDrhY28q7bK0 YYQ+2PafonAbNbZVtDLI7XniBbqrrMBhBpW3Cp1HlElAYC9zzDlQKh/YyyVTWfoBsBGY +sBSHwTw+/Q2A5P6iDIz4o4PeUX36aEoXtVYbE8JeyhjKSWRtm2MGpDEJ6pCPGcVJUjK g3LVIt6kCVJ/JZfnms2AisBIYl12xIV57botVQDl2hBwT7aE53a3HpdPVVo+dC2DKS+l Uv/9V4gh/lBZIqTsq25R10ZjQHgUjbYDy7iWI93BmRsnJnhUnTHhpc6Aefuk81r+B+1Y Pq4w== X-Gm-Message-State: APjAAAUMXpcYkqf02Y5Pa5cIKW+zIrpwBW89MHZJNpGsInDY33QSc0Kj xOSagX7qNGXShtOQ1mth8GizEJj4hVGwcfTYLk4= X-Google-Smtp-Source: APXvYqxIBLxF/zyQioeDFKNRPgPpfCELgW79jBHRv/NacXRNd3E+wlSQBtB0tCsThvXaTQeqpjLihSwXYOm5+kLP2eY= X-Received: by 2002:a7b:c08d:: with SMTP id r13mr5156994wmh.104.1578668575262; Fri, 10 Jan 2020 07:02:55 -0800 (PST) MIME-Version: 1.0 References: <20200108095229.GC87523@coredump.intra.peff.net> <20200110111516.GA474613@coredump.intra.peff.net> In-Reply-To: <20200110111516.GA474613@coredump.intra.peff.net> From: Eyal Soha Date: Fri, 10 Jan 2020 10:02:43 -0500 Message-ID: Subject: Re: Fwd: Add support for axiterm colors in parse_color To: Jeff King Cc: git@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Thanks. I think that 3 patches are in order. The first will refactor and add the enums. The second will add the aixterm colors. The last will alias RGB 8-15 to aixterm colors. What's the correct way to email those patches? I will try with git-send-email. BTW, is "git help send-email" supposed to work? Eyal On Fri, Jan 10, 2020 at 6:15 AM Jeff King wrote: > > On Thu, Jan 09, 2020 at 07:20:09PM -0500, Eyal Soha wrote: > > > > That said, I'm not entirely opposed to having more human-readable > > > aliases. I'm not sure if it's worth using the 16-color version (e.g., > > > "ESC[91m" versus the 256-color version "ESC[38;5;9m"). It's possible it > > > would be compatible with more terminals, and it is slightly fewer bytes. > > > > My motivation for this patch was to fix the github workflow log output > > that doesn't support 8bit colors properly. Only the "ANSI" colors > > 0-15 worked. None of the 8-bit colors worked except for 30-37, 40-47, > > 90-97, and 100-107, which matched the ANSI colors. That is a very > > broken color scheme! But that's how it displayed. > > That makes sense. I'm not too surprised to see a terminal that supports > one but not the other. > > But I wonder if there are ones that go the other way around: supporting > 256-color mode but not ANSI 90-97, etc. I doubt it, but I think it would > be nice to split that change out into a separate commit in case we do > run into problems. > > > Done. Here's a new patch! > > Thanks. The content here is looking pretty good (though I have a few > comments below). > > Can you also format it as described in Documentation/SubmittingPatches > and re-send? In particular, it needs a regular commit message and your > signoff. > > > +enum { > > + COLOR_BACKGROUND_OFFSET = 10, > > + COLOR_FOREGROUND_ANSI = 30, > > + COLOR_FOREGROUND_RGB = 38, > > + COLOR_FOREGROUND_256 = 38, > > + COLOR_FOREGROUND_BRIGHT_ANSI = 90, > > +}; > > The split in this enum mostly makes sense to me. It changes the meaning > of "value" for COLOR_ANSI, but this is all local to color.c, so your > changes to output_color() are all that's needed. > > It might be easier to follow the patch if switching to this enum came in > a separate preparatory patch. > > > @@ -92,7 +100,13 @@ static int parse_color(struct color *out, const > > char *name, int len) > > for (i = 0; i < ARRAY_SIZE(color_names); i++) { > > if (match_word(name, len, color_names[i])) { > > out->type = COLOR_ANSI; > > - out->value = i; > > + out->value = i + COLOR_FOREGROUND_ANSI; > > + return 0; > > + } > > + if (*name == '+' && > > + match_word(name+1, len-1, color_names[i])) { > > + out->type = COLOR_ANSI; > > + out->value = i + COLOR_FOREGROUND_BRIGHT_ANSI; > > return 0; > > } > > It would be slightly simpler and more efficient handle the "+" outside > the loop, like: > > int offset = COLOR_FOREGROUND_ANSI; > if (*name == '+') { > offset = COLOR_FOREGROUND_BRIGHT_ANSI; > name++; > len--; > } > > and then in the loop just set "out->value = i + offset". > > You'd probably want to hoist this out to a separate function since > "name" needs to be unchanged in the later part of the function. > > I dunno. It's almost certainly an unmeasurable micro-optimization, but > somehow the flow of it seems simpler to me. > > I also wondered if we'd want English aliases like "brightred" that some > other systems use. It would be easy to add that to the check I showed > above without having to repeat as much. > > > @@ -109,10 +123,15 @@ static int parse_color(struct color *out, const > > char *name, int len) > > else if (val < 0) { > > out->type = COLOR_NORMAL; > > return 0; > > - /* Rewrite low numbers as more-portable standard colors. */ > > + /* Rewrite 0-7 as more-portable standard colors. */ > > } else if (val < 8) { > > out->type = COLOR_ANSI; > > - out->value = val; > > + out->value = val + COLOR_FOREGROUND_ANSI; > > + return 0; > > + /* Rewrite 8-15 as more-portable aixterm colors. */ > > + } else if (val < 16) { > > + out->type = COLOR_ANSI; > > + out->value = val - 8 + COLOR_FOREGROUND_BRIGHT_ANSI; > > And I think this 8-15 handling might want to be a separate commit on > top, just because it's possible it might regress some cases (though > again, I do doubt it). > > > case COLOR_256: > > - out += xsnprintf(out, len, "%c8;5;%d", type, c->value); > > + out += xsnprintf(out, len, "%d;5;%d", COLOR_FOREGROUND_256 + offset, > > + c->value); > > Looks like some unwanted tab/space conversion (here and elsewhere). > > > +test_expect_success 'axiterm bright fg color' ' > > + color "+red" "[91m" > > s/axi/aix/ (here and below). > > > +test_expect_success '8-15 are aliases for aixterm color names' ' > > + color "12 13" "[94;105m" > > Makes sense. > > -Peff