Re: [PATCH] Introduce git version --list-features for porcelain use

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 21, 2007, 05:47 UTC
Message-ID: <7v1wg55065.fsf@assigned-by-dhcp.pobox.com>
In-Reply-To: <20070621045903.GA14047@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

> As a porcelain author I'm finding it difficult to keep track of
> what features I can use in git-gui.  Newer versions of Git have
> newer capabilities but they don't always immediately get newer
> version numbers that I can easily test for.

Two and half comments, and a discussion.

Show 25 quoted lines

> +static const char *supported_features[] = {
> +	"blame-ignore-whitespace",
> +	"list-features",
> +};
> +
>  /* most GUI terminals set COLUMNS (although some don't export it) */
>  static int term_columns(void)
>  {
> @@ -190,10 +195,78 @@ void help_unknown_cmd(const char *cmd)
>  	exit(1);
>  }
>  
> +static int is_feature_name_sane(const char *a)
> +{
> +	if (!*a || *a == '-')
> +		return 0;
> +	for (; *a; a++) {
> +		if (! ((*a >= 'a' && *a <= 'z')
> +		    || (*a >= '0' && *a <= '9')
> +		    || *a == '-'))
> +			return 0;
> +	}
> +	return 1;
> +}
> +

Show 16 quoted lines

> +static int cmp_feature(const void *a_, const void *b_)
> +{
> +	const char *a = *((const char **)a_);
> +	const char *b = *((const char **)b_);
> +	return strcmp(a, b);
> +}
> +
> +static void list_features()
> +{
> +	unsigned cnt = ARRAY_SIZE(supported_features);
> +	unsigned i;
> +
> +	qsort(supported_features, cnt,
> +		sizeof(supported_features[0]), cmp_feature);
> ...
> +}

Unless we are talking about dynamically extensible feature list (eh, dll, anybody?), it might be easier to keep (1) the list sorted, and (2) free of insane feature name in supported_features[] array at the source level. Then you can lose that is_feature_name_sane() function.

Show 11 quoted lines

> +static int supports_feature(const char *the_feature)
> +{
> +	unsigned cnt = ARRAY_SIZE(supported_features);
> +	unsigned i;
> +
> +	for (i = 0; i < cnt; i++) {
> +		if (!strcmp(supported_features[i], the_feature))
> +			return 0;
> +	}
> +	return 1;
> +}

And you can  perform a bsearch here. instead of linear.

> +test_expect_failure \
> +	'feature "THISNEVERWILLBEAGITFEATURE" is not supported' \
> +	'git version --supports-feature=THISNEVERWILLBEAGITFEATURE'

I would expect that THISNEW... will get complaint saying "That is not a valid feature name, as it has uppercase", from a version that has is_feature_name_sane() function.

I suspect that this patch is meant for my 'maint' (and 1.5.2.3). Or is it for my 'master'? What's your plan to handle transition?

For example, if this appears on 1.5.2.3, then supported_features[] should not have blame-ignore-whitespace, unless we are talking about cherry-picking, and I honestly do not think "blame -w" deserves to go to the maintenance only series. On the other hand, --list-features could go to 'maint' under 'future prooofing' category, I guess.

If this is meant to be only for 1.5.3 and later, then you know that "blame -w" is available as well, so the fact you can do "git version --list-features" alone tells you that you can use "blame -w", among other many things, such as "diff -C -C" instead of --find-copies-harder.

Where does the above discussion lead us? It essentially means, in either case, "blame-ignore-whitespace" should not be in that supported_features[] array.