Re: [PATCHv3 4/4] builtin/blame: highlight recently changed lines

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 8, 2018, 19:56 UTC
Message-ID: <xmqqfu7g86qs.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20180104224007.214548-5-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 24 quoted lines

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

Show 7 quoted lines

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