git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: sunshine@sunshineco.com, git@vger.kernel.org
Subject: Re: [PATCHv3 4/4] builtin/blame: highlight recently changed lines
Date: Mon, 08 Jan 2018 11:56:11 -0800	[thread overview]
Message-ID: <xmqqfu7g86qs.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20180104224007.214548-5-sbeller@google.com> (Stefan Beller's message of "Thu, 4 Jan 2018 14:40:07 -0800")

Stefan Beller <sbeller@google.com> writes:

> +static struct color_field {
> +	timestamp_t hop;
> +	char col[COLOR_MAXLEN];
> +} *colorfield;
> +static int colorfield_nr, colorfield_alloc;
> +
> +static void parse_color_fields(const char *s)
> +{
> +	struct string_list l = STRING_LIST_INIT_DUP;
> +	struct string_list_item *item;
> +	enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
> +
> +	/* Ideally this would be stripped and split at the same time? */
> +	string_list_split(&l, s, ',', -1);
> +	ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
> +
> +	for_each_string_list_item(item, &l) {
> +		switch (next) {
> +		case EXPECT_DATE:
> +			colorfield[colorfield_nr].hop = approxidate(item->string);
> +			next = EXPECT_COLOR;
> +			colorfield_nr++;
> +			ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
> +			break;

This should make sure cf[i].hop is monotonically increasing to avoid
end-user mistakes, I would think (what's 'hop' by the way?).

> +		case EXPECT_COLOR:
> +			if (color_parse(item->string, colorfield[colorfield_nr].col))
> +				die(_("expecting a color: %s"), item->string);

When you have a typo in one of your configuration files, say "[color
"blame"] highlightrecent = 1,week,blue,...", you'd want to see a bit
more than just "expecting a color: week" to help you diagnose and
resolve the issue.  Giving the name of the variable and the file the
wrong definition was found in would be needed, givin that this is
called from the config callback git_blame_config() below.

> +			next = EXPECT_DATE;
> +			break;
> +		}
> +	}
> +
> +	if (next == EXPECT_COLOR)
> +		die (_("must end with a color"));

Same here.

>  		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
>  		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line differently"), OUTPUT_COLOR_FIELDS),
> +		OPT_BIT(0, "heated-lines", &output_option, N_("color lines by age"), OUTPUT_HEATED_LINES),

These options may be useful while having fun experimenting, but my
gut feeling is that these are too fine-grained for end-users to
tweak per invocation basis (which is what command line options are
for).  

But perhaps I am biased (as anybody else), as personally I find
anything beyond step 2/4 uninteresting, and not adding too many of
these options is consistent with that viewpoint ;-)

In any case, thanks for a fun read.


  reply	other threads:[~2018-01-08 19:56 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-10  1:09 [RFC PATCH 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
2017-11-10  1:09 ` [PATCH 1/4] color.h: modernize header Stefan Beller
2017-11-10  1:10 ` [PATCH 2/4] builtin/blame: dim uninteresting metadata Stefan Beller
2017-11-10  1:10 ` [PATCH 3/4] builtin/blame: add option to color metadata fields separately Stefan Beller
2017-11-10  1:10 ` [PATCH 4/4] builtin/blame: highlight recently changed lines Stefan Beller
2018-01-04 22:40 ` [PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
2018-01-04 22:40   ` [PATCHv3 1/4] color.h: document and modernize header Stefan Beller
2018-01-08 19:14     ` Junio C Hamano
2018-01-04 22:40   ` [PATCHv3 2/4] builtin/blame: dim uninteresting metadata Stefan Beller
2018-01-08 19:34     ` Junio C Hamano
2018-02-08 21:19       ` Stefan Beller
2018-01-04 22:40   ` [PATCHv3 3/4] builtin/blame: add option to color metadata fields separately Stefan Beller
2018-01-04 22:40   ` [PATCHv3 4/4] builtin/blame: highlight recently changed lines Stefan Beller
2018-01-08 19:56     ` Junio C Hamano [this message]
2018-01-08 20:09       ` Stefan Beller
2018-02-01 19:29   ` [PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Ævar Arnfjörð Bjarmason
2018-02-01 20:24     ` Stefan Beller

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqfu7g86qs.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.com \
    --cc=sunshine@sunshineco.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).