"Shawn O. Pearce" [off-list ref] 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.
quoted hunk
+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;
+}
+
+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.
+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.
Junio C Hamano [off-list ref] wrote:
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.
Yes, this is true. But I'm being overly defensive here. If the
list does get out of order we force it back in --list-features just
to be consistent in output, and t0000 will fail if any item in the
list is insane.
If you want to be less defensive, OK, it would also reduce the
patch size a bit, but may allow someone to add an insane item to
the list.
quoted
+static int supports_feature(const char *the_feature)
And you can perform a bsearch here. instead of linear.
Sure. But I'm actually expecting more for Porcelain that cares
to run `git version --list-features` and store that output into
its own internal table, then consult that table rather than the
--supports-feature option. I figured the bsearch would take more
code than the linear, and probably wasn't worth it in this dark
little corner of Git.
quoted
+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.
Eh, probably true. I guess I should make that lowercase so it is
actually a sane name, just one so unlikely that we will never use it.
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?
Eh, sorry, I should have mentioned that. I meant for you to apply
this to your master, where `git blame -w` is already present.
Currently with next I get:
$ git version --list-features
git version 1.5.2.2.1050.g51a8b
So what I was planning on doing in git-gui was running that, and if
I just get one line with `git version` on it (which is an insane
feature name btw) then I know --list-features is not supported,
and neither is any other feature that I might be looking for from
the --list-features command.
On the other hand if --list-features is actually supported instead
I'll get back at least a line with "list-features" on it, and I
won't get a line with "git version" on it (as that is an insane
feature name).
I guess it is good that our cmd_version() routine never actually
checked to see if its argv[] matched what it expects to receive
(nothing up until now). :)
Now I'm fine with you applying this back into a 1.5.2.x maint
release, but because of the cmd_version() behavior I don't think
that is really required. Nor am I looking for you to cherry-pick
back the `git blame -w` feature.
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.
Yes, that is true.
Where does the above discussion lead us? It essentially means,
in either case, "blame-ignore-whitespace" should not be in that
supported_features[] array.
Hmm. So assuming that is true, under what rule do you propose we
add new features to that array in the future? And what name(s)
do we give to them?
For a recent hypothetical example, assume this patch was already
applied in your master by the time Linus submitted his useful hack
`git log --follow`. How would we signal in the supported_features[]
that log would understand --follow for a single file pathname?
--
Shawn.