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

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

[Shawn O. Pearce]

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.

This is a simple plumbing option that lets a porcelain ask the
plumbing for its capabilities, at which point the porcelain can
work around anything missing, or recommend to the user that they
upgrade their plumbing layer.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 Because 'maint' does not support `git-blame -w` I can't apply your
 recent (and insanely good idea) 'git-gui: use "blame -w -C -C" for
 "where did it come from, originally?"' without something like the
 following, so I know that `git blame -w -C -C` will be accepted
 by the underlying plumbing.  ;-)

 help.c           |   77 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t0000-basic.sh |   16 +++++++++++
 2 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index 1cd33ec..9f24a0f 100644
--- a/help.c
+++ b/help.c
@@ -9,6 +9,11 @@
 #include "common-cmds.h"
 #include <sys/ioctl.h>
 
+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);
+	for (i = 0; i < cnt; i++) {
+		const char *f = supported_features[i];
+		if (!is_feature_name_sane(f))
+			die("feature name \"%s\" is bogus", f);
+		printf("%s\n", f);
+	}
+}
+
+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;
+}
+
+static const char version_usage[] =
+"git-version [(--list-features | --supports-feature=<name>*)]";
+
 int cmd_version(int argc, const char **argv, const char *prefix)
 {
-	printf("git version %s\n", git_version_string);
-	return 0;
+	int i, ret = 0, list = 0, test = 0;
+
+	for (i = 1; i < argc; i++) {
+		const char *arg = argv[i];
+		if (!strcmp(arg, "--list-features"))
+			list = 1;
+		else if (!prefixcmp(arg, "--supports-feature=")) {
+			test = 1;
+			ret |= supports_feature(arg + 19);
+		} else
+			usage(version_usage);
+	}
+
+	if (list && test)
+		die("cannot use both --list-features and --supports-feature");
+	if (list)
+		list_features();
+	if (!list && !test)
+		printf("git version %s\n", git_version_string);
+	return ret;
 }
 
 int cmd_help(int argc, const char **argv, const char *prefix)
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 8bfe832..9fc5824 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -31,6 +31,22 @@ fi
 . ./test-lib.sh
 
 ################################################################
+# git version
+
+test_expect_success \
+	'git version is functional' \
+	'git version'
+test_expect_success \
+	'git version --list-features' \
+	'git version --list-features'
+test_expect_success \
+	'feature "list-features" is supported' \
+	'git version --supports-feature=list-features'
+test_expect_failure \
+	'feature "THISNEVERWILLBEAGITFEATURE" is not supported' \
+	'git version --supports-feature=THISNEVERWILLBEAGITFEATURE'
+
+################################################################
 # git-init has been done in an empty repository.
 # make sure it is empty.
 
-- 
1.5.2.2.1050.g51a8b

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

[Junio C Hamano]

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

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

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

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> 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.

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

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

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

[Junio C Hamano]

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

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

If we were to go this route (which I am not convinced is the
right direction yet---using "git version" to check the version
and say "1.5.3 has blame -w" might be easier to manage, and that
needs to be maintained only by the tools that care about the
particular feature, "blame -w"), we will potentially be talking
about hundreds of features in the long run.  I would rather want
to automate this even further, as keeping the feature list and
features together would become huge maintenance headache.

So, first of all, I would want the list of features in
supported_features[] to come from a separate file, whose sole
purpose is to have that list.  I would even suggest making it an
generated file, which depends on the source files (not
necessarily limited to C files), and built from within the build
procedure.

Whenever you add a new command line option, you would add a
greppable comment near it, something like...

 		else if (!strcmp("-s", arg))
 			output_option |= OUTPUT_NO_AUTHOR;
+		/* FEATURE[[[blame-ignore-whitespace]]] */
+		else if (!strcmp("-w", arg))
+			xdl_opts |= XDF_IGNORE_WHITESPACE;
 		else if (!strcmp("-S", arg) && ++i < argc)
 			revs_file = argv[i];

and have Makefile extract them, sort them and build the meat of
supported_features[] array automatically.

Unless we have something like that, I have to worry about merge
conflicts every time a new topic graduates to 'master'.

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

Ok, that is probably a sane assumption.

> ....  I figured the bsearch would take more
> code than the linear, and probably wasn't worth it in this dark
> little corner of Git.

Huh?  Don't you already have compar() function that bsearch(3)
can take?

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

The explaination why "futureproofing" is unnecessary for 'maint'
is good, but that does not necessarily mean --list-featues would
need to list _all_ the features in the version that supports
that.  "blame -w" is in 'master' already, for example.  Is
"have-mergetool" a feature?  Yes, it is.  But you already know
the command exists if "git version" knows --list-features.

> 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?

Assuming if this --list-features is a viable approach in the
long run, my gut feeling of relative order of things is:

 (1) We prepare rock-solid runtime with an _EMPTY_ feature array
     in 'master'.

 (2) We add automated feature array management in 'master', as
     outlined above.

 (3) Merge the above two to 'next'.

 (4) Update all existing topics, including the ones that are
     already in 'next', with the feature mark-up in comment
     (i.e. "FEATURE[[[feature-name]]]").

After (3) happens, any new topics that introduce a new feature
should be accompanied with the feature mark-up.

When an update appears on 'master' _after_ (1) and (2) happens
_but_ Porcelain writers like yourself do not realize that the
feature would be _very_ helpful for their Porcelains, and would
want to be able to tell if it exists via --list-features, long
after we start to support the feature, we might retroactively
have to add the feature mark-up.  There will be gap between the
commit that actually introduce and make the feature usable, and
the commit that teaches "git version" to talk about it in such a
case.  But I think that should be Ok.

I do not think we want to keep track of every little "features"
if no Porcelain cares about it.

Here is a list of potential "feature names", from the latest
"What's cooking" message.

 log-unlimited-message
 log-follow
 oneline-first-paragraph
 svn-merge-local
 work-tree-environ
 filter-branch

[PATCH] Make list of features auto-managed.

[Junio C Hamano]

This updates the build procedure so that a mark-up in the form
of:

	SP "FEATURE<" feature-name ">" SP

is picked up from the sources, in order to help keeping
supported_features[] array automatically in sync.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

  Junio C Hamano <gitster@pobox.com> writes:

  > So, first of all, I would want the list of features in
  > supported_features[] to come from a separate file, whose sole
  > purpose is to have that list.  I would even suggest making it an
  > generated file, which depends on the source files (not
  > necessarily limited to C files), and built from within the build
  > procedure.
  > ...
  > Assuming if this --list-features is a viable approach in the
  > long run, my gut feeling of relative order of things is:
  >
  >  (1) We prepare rock-solid runtime with an _EMPTY_ feature array
  >      in 'master'.
  >
  >  (2) We add automated feature array management in 'master', as
  >      outlined above.
  >
  >  (3) Merge the above two to 'next'.
  >
  >  (4) Update all existing topics, including the ones that are
  >      already in 'next', with the feature mark-up in comment
  >      (i.e. "FEATURE[[[feature-name]]]").
  >
  > After (3) happens, any new topics that introduce a new feature
  > should be accompanied with the feature mark-up.

  So here is a PoC for step (2).  It loses more lines than it
  adds, primarily because we can lose is_feature_name_sane(); it
  is done at the build time now.

 .gitignore           |    1 +
 Makefile             |   12 +++++++++++-
 builtin-blame.c      |    1 +
 generate-features.sh |    9 +++++++++
 help.c               |   48 +++++++++++++-----------------------------------
 5 files changed, 35 insertions(+), 36 deletions(-)
 create mode 100755 generate-features.sh

diff --git a/.gitignore b/.gitignore
index e8b060c..64ee08b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -169,3 +169,4 @@ config.status
 config.mak.autogen
 config.mak.append
 configure
+features.h
diff --git a/Makefile b/Makefile
index c09dfaf..d8d1ddc 100644
--- a/Makefile
+++ b/Makefile
@@ -262,6 +262,13 @@ BUILT_INS = \
 	git-fsck-objects$X git-cherry-pick$X \
 	$(patsubst builtin-%.o,git-%$X,$(BUILTIN_OBJS))
 
+# what are the source files
+SOURCE_FILES = $(sort \
+	$(SCRIPT_SH) $(SCRIPT_PERL) $(SCRIPT_PYTHON) \
+	$(patsubst git-%$X,%.c,$(PROGRAMS)) \
+	$(LIB_H) \
+	$(patsubst %.o,%.c,$(LIB_OBJS) $(BUILTIN_OBJS)))
+
 # what 'all' will build and 'install' will install, in gitexecdir
 ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
 
@@ -755,7 +762,10 @@ git$X: git.o $(BUILTIN_OBJS) $(GITLIBS)
 		$(ALL_CFLAGS) -o $@ $(filter %.c,$^) git.o \
 		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
-help.o: common-cmds.h
+features.h: $(SOURCE_FILES) generate-features.sh
+	$(QUIET_GEN)./generate-features.sh $(SOURCE_FILES) > $@+ && mv $@+ $@
+
+help.o: common-cmds.h features.h
 
 git-merge-subtree$X: git-merge-recursive$X
 	$(QUIET_BUILT_IN)rm -f $@ && ln git-merge-recursive$X $@
diff --git a/builtin-blame.c b/builtin-blame.c
index f7e2c13..c84d96a 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2162,6 +2162,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		else if (!strcmp("-s", arg))
 			output_option |= OUTPUT_NO_AUTHOR;
 		else if (!strcmp("-w", arg))
+			/* FEATURE<blame-ignore-whitespace> */
 			xdl_opts |= XDF_IGNORE_WHITESPACE;
 		else if (!strcmp("-S", arg) && ++i < argc)
 			revs_file = argv[i];
diff --git a/generate-features.sh b/generate-features.sh
new file mode 100755
index 0000000..674c5fe
--- /dev/null
+++ b/generate-features.sh
@@ -0,0 +1,9 @@
+#!/bin/sh
+#
+# New features that Porcelains may care about can be marked in the
+# source code with FEATURE<feature name>.  This script is driven
+# by Makefile to pick them out and sort them to prepare features.h
+# file, which is included by help.c.
+
+sed -n -e 's|.* FEATURE<\([a-z0-9][-a-z0-9]*[a-z0-9]\)> .*/|  "\1",|p' "$@" |
+sort -u
diff --git a/help.c b/help.c
index d050cbf..b0887ea 100644
--- a/help.c
+++ b/help.c
@@ -9,9 +9,10 @@
 #include "common-cmds.h"
 #include <sys/ioctl.h>
 
+/* FEATURE<list-features> */
+
 static const char *supported_features[] = {
-	"blame-ignore-whitespace",
-	"list-features",
+#include "features.h"
 };
 
 /* most GUI terminals set COLUMNS (although some don't export it) */
@@ -195,51 +196,28 @@ void help_unknown_cmd(const char *cmd)
 	exit(1);
 }
 
-static int is_feature_name_sane(const char *a)
+static void list_features(void)
 {
-	if (!*a || *a == '-')
-		return 0;
-	for (; *a; a++) {
-		if (! ((*a >= 'a' && *a <= 'z')
-		    || (*a >= '0' && *a <= '9')
-		    || *a == '-'))
-			return 0;
-	}
-	return 1;
+	unsigned cnt = ARRAY_SIZE(supported_features);
+	unsigned i;
+
+	for (i = 0; i < cnt; i++)
+		printf("%s\n", supported_features[i]);
 }
 
 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);
-	for (i = 0; i < cnt; i++) {
-		const char *f = supported_features[i];
-		if (!is_feature_name_sane(f))
-			die("feature name \"%s\" is bogus", f);
-		printf("%s\n", f);
-	}
+	return strcmp(a, b);
 }
 
 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;
+	return !bsearch(&the_feature, supported_features,
+			(size_t) ARRAY_SIZE(supported_features),
+			sizeof(const char *), cmp_feature);
 }
 
 static const char version_usage[] =
-- 
1.5.2.2.1050.g51a8b

Re: [PATCH] Make list of features auto-managed.

[Junio C Hamano]

I am now getting sort of convinced that the list-features is a
slippery slop towards total suckage, unless managed very
carefully.

The "auto manage" patch I am responding to would tempt anybody
to extend the mechanism to do something like the attached patch,
because a one-line comment near the code, like:

	/* FEATURE<oneline-first-paragraph> */

does not look very useful nor pretty by itself in the source
code.  It makes it very tempting to actually describe the
"feature".  People with only half a brain would even advocate
updating supported_features[] to a tuple of (name, explanation),
and have "git version --list-features" to spit both out, or
something like that.  I really do not think we would want to go
there.

A few years down the road, do we really care if we did not do
something long time ago, but the code was updated and added as a
new feature?  That information belongs to the commit log, not to
in-source comments.  This will lead to the same mistake often
made by users of other SCMs, embedding "$Log$" in their sources.


=== sample diff that is not acceptable ===

diff --git a/commit.c b/commit.c
index dbb28b5..4b713b6 100644
--- a/commit.c
+++ b/commit.c
@@ -1016,6 +1016,12 @@ static void pp_header(enum cmit_fmt fmt,
 	}
 }
 
+/*
+ * FEATURE<oneline-first-paragraph>: we used to treat the first line of the
+ * commit messages special, but messages imported from foreign culture often
+ * have more than one line in the first "paragraph".  We now treat the
+ * first paragraph as special for the purposes of --pretty=oneline.
+ */
 static void pp_title_line(enum cmit_fmt fmt,
 			  const char **msg_p,
 			  unsigned long *len_p,
@@ -1140,6 +1146,11 @@ static void pp_remainder(enum cmit_fmt fmt,
 	}
 }
 
+/*
+ * FEATURE<log-unlimited-message>: we used to have a hard limit
+ * of 16kB message, but that is sometimes not enough for huge commits
+ * imported from foreign culture.  We now dynamically grow the buffer.
+ */
 unsigned long pretty_print_commit(enum cmit_fmt fmt,
 				  const struct commit *commit,
 				  unsigned long len,

Re: [PATCH] Make list of features auto-managed.

[Nicolas Pitre]

On Thu, 21 Jun 2007, Junio C Hamano wrote:

> I am now getting sort of convinced that the list-features is a
> slippery slop towards total suckage, unless managed very
> carefully.
> 
> The "auto manage" patch I am responding to would tempt anybody
> to extend the mechanism to do something like the attached patch,
> because a one-line comment near the code, like:
> 
> 	/* FEATURE<oneline-first-paragraph> */
> 
> does not look very useful nor pretty by itself in the source
> code.  It makes it very tempting to actually describe the
> "feature".  People with only half a brain would even advocate
> updating supported_features[] to a tuple of (name, explanation),
> and have "git version --list-features" to spit both out, or
> something like that.  I really do not think we would want to go
> there.
> 
> A few years down the road, do we really care if we did not do
> something long time ago, but the code was updated and added as a
> new feature?  That information belongs to the commit log, not to
> in-source comments.  This will lead to the same mistake often
> made by users of other SCMs, embedding "$Log$" in their sources.

Well, just in case my opinion matters...

I don't like this feature list idea at all.

Not only is it going to grow indefinitely with no possibility for 
garbage collection because programs will depends on particular features 
to be listed there, the simple fact that deciding if a particular 
improvement should be listed is a subjective matter which will make the 
list grow faster than it absolutely should.

Given that this appears to be needed by git-guy, and since git-guy is 
shipped with main git already, it is a bit odd to have to support code 
to work with older git versions.  Why isn't git-guy simply remaining in 
synch with the git version it is shipped with?  This way cruft won't 
accumulate forever.

And if the tool and git core really have to be decoupled, then the best 
way to test for a specific feature is actually to get the git 
version.  No silly issues with a feature list to maintain, and no issue 
with a particular feature that might not have been added to the list.  

When you need git behavior X and you know that it appeared in version Y 
then you only need to test for git_version >= Y.  Determining that 
particular Y is much easier after the facts using the commit log than 
trying to anticipate what item should be added to a feature list for 
future usage.  In fact the same argument as for not explicitly recording 
renames in commit objects should apply here.


Nicolas

Re: [PATCH] Make list of features auto-managed.

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> Well, just in case my opinion matters...

Well, you already do ;-)

> I don't like this feature list idea at all.

... and thanks for bringing a bit of sanity to this thread.

> When you need git behavior X and you know that it appeared in version Y 
> then you only need to test for git_version >= Y.  Determining that 
> particular Y is much easier after the facts using the commit log than 
> trying to anticipate what item should be added to a feature list for 
> future usage.  In fact the same argument as for not explicitly recording 
> renames in commit objects should apply here.

Very well put; that is exactly what I wanted to say in two
messages ago.

Re: [PATCH] Make list of features auto-managed.

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Nicolas Pitre <nico@cam.org> writes:
> > I don't like this feature list idea at all.
> 
> ... and thanks for bringing a bit of sanity to this thread.

Indeed.  I've read Junio's counter proposal (the grep'ed FEATURE<?>
macro) and I'm now convinced you are both right, this feature list
thing is just going to grow out of control or nobody is going to
mark new features.  Either way its useless for its intended purpose.
 
> > When you need git behavior X and you know that it appeared in version Y 
> > then you only need to test for git_version >= Y.  Determining that 
> > particular Y is much easier after the facts using the commit log than 
> > trying to anticipate what item should be added to a feature list for 
> > future usage.  In fact the same argument as for not explicitly recording 
> > renames in commit objects should apply here.

Here's the problem though: `git-blame -w` will be supported
in Git 1.5.3 and later, we all know this.  But Git doesn't.
Ask git-describe what version `master` and `next` are; its
v1.5.2.2-249 and v1.5.2.2-1050.

So tell me, how can git-gui know that Git 1.5.2.2.249 is OK, and
1.5.3 is OK, but 1.5.2.3 isn't?  Actually its 1.5.2.1.160 that is
OK (b82871b introduced the -w option).  Sure Junio won't release
a 1.5.2.1.160 as an actual tagged release (160 patch releases to
1.5.2.1 is nuts).  But what about in the future if a cool feature
3 commits past 1.5.3 appears?  Wouldn't that look like 1.5.3.3,
and isn't that a possibly valid version number?

Besides, I can't say 1.5.2.3 is >= 1.5.2.2.249, because in git.git
it isn't.  Only 1.5.3 will be >= 1.5.2.2.249.


Nico mentioned that git-gui ships with git.git, and therefore should
just rely on exactly whatever that git.git supports at the time of
the merge.  I think that is only partially valid.  git-gui is also
an independent project with a repository and history that exists
outside of git.git.  Users can (and should be able to) mix and
match the version of git-gui with the version of plumbing, to the
maximum extent possible.  I'd like to at least gracefully fail by
disabling an option, or suggesting the user upgrade their plumbing,
if an option isn't supported.

Unlike how we gracefully fail with a useful error message say
when an early 1.4 release that doesn't support offset deltas is
given a packfile with an OFS_DELTA in it (corrupt pack, recently
rediscussed on list).  Or when a 1.5.1 client tries to checkout
a tree that uses the new subproject mode in 1.5.2 (missing blob,
recently discussed on #git).

Maybe I should ask the StGIT folks how they deal with this, or if
they just don't worry about it.  I'm suspecting its the latter...


Hmm.

-- 
Shawn.

Re: [PATCH] Make list of features auto-managed.

[Junio C Hamano]

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

> Here's the problem though: `git-blame -w` will be supported
> in Git 1.5.3 and later, we all know this.  But Git doesn't.
> Ask git-describe what version `master` and `next` are; its
> v1.5.2.2-249 and v1.5.2.2-1050.
>
> So tell me, how can git-gui know that Git 1.5.2.2.249 is OK, and
> 1.5.3 is OK, but 1.5.2.3 isn't?

You don't.  Although 1.5.2.2.249 may have it, you do not enable
it for that version.

That is what "1.5.3 or later" implies.  And in 6 weeks that
little distinction does not matter.

Pretty simple.

Re: [PATCH] Make list of features auto-managed.

[Nicolas Pitre]

On Thu, 21 Jun 2007, Shawn O. Pearce wrote:

> > Nicolas Pitre <nico@cam.org> writes:
> > > When you need git behavior X and you know that it appeared in version Y 
> > > then you only need to test for git_version >= Y.  Determining that 
> > > particular Y is much easier after the facts using the commit log than 
> > > trying to anticipate what item should be added to a feature list for 
> > > future usage.  In fact the same argument as for not explicitly recording 
> > > renames in commit objects should apply here.
> 
> Here's the problem though: `git-blame -w` will be supported
> in Git 1.5.3 and later, we all know this.  But Git doesn't.
> Ask git-describe what version `master` and `next` are; its
> v1.5.2.2-249 and v1.5.2.2-1050.
> 
> So tell me, how can git-gui know that Git 1.5.2.2.249 is OK, and
> 1.5.3 is OK, but 1.5.2.3 isn't?  Actually its 1.5.2.1.160 that is
> OK (b82871b introduced the -w option).  Sure Junio won't release
> a 1.5.2.1.160 as an actual tagged release (160 patch releases to
> 1.5.2.1 is nuts).  But what about in the future if a cool feature
> 3 commits past 1.5.3 appears?  Wouldn't that look like 1.5.3.3,
> and isn't that a possibly valid version number?
> 
> Besides, I can't say 1.5.2.3 is >= 1.5.2.2.249, because in git.git
> it isn't.  Only 1.5.3 will be >= 1.5.2.2.249.

First of all, I mentioned at the time that using a . for separator 
between the tagged version and the number of commits since then in the 
git-describe output was a bad idea.  You just made the perfect 
demonstration of that.  If it was just me I'd change that . for a + or a 
: like the original patch did.

Now to your issue.  I think that if you must rely upon a particular 
version then it's best if it is a released version.  Intermediate 
versions as provided by git-describe aren't reliable enough to provide 
a proper number to test against, especially in the presence of 
concurrent branches.  The 1.6.0+38 (let me use a saner output) from the 
master branch is totally different from 1.6.0+38 from the 'next' branch.  
So only released versions should be used to compare against.

Now you say that you don't want to wait for the release to happen before 
using this cool new feature.  Well, I'd reply that life is tough.  
Either you 
trick Junio into making a release sooner because the feature is just too 
valuable to wait.  Or you try the feature (git-blame -w) and hope for 
the best.  Certainly in that case you can predict the behavior of 
older git-blame versions if you pass it -w which they don't understand?

> Nico mentioned that git-gui ships with git.git, and therefore should
> just rely on exactly whatever that git.git supports at the time of
> the merge.  I think that is only partially valid.  git-gui is also
> an independent project with a repository and history that exists
> outside of git.git.  Users can (and should be able to) mix and
> match the version of git-gui with the version of plumbing, to the
> maximum extent possible.  

Fair enough.

> I'd like to at least gracefully fail by
> disabling an option, or suggesting the user upgrade their plumbing,
> if an option isn't supported.

Well you should be able to just try the option and detect it when it 
isn't supported.

> Unlike how we gracefully fail with a useful error message say
> when an early 1.4 release that doesn't support offset deltas is
> given a packfile with an OFS_DELTA in it (corrupt pack, recently
> rediscussed on list).  Or when a 1.5.1 client tries to checkout
> a tree that uses the new subproject mode in 1.5.2 (missing blob,
> recently discussed on #git).

Those are different as you have an older version that couldn't 
anticipate the future.  In your case you can "anticipate the past".


Nicolas

Re: [PATCH] Make list of features auto-managed.

[Shawn O. Pearce]

Nicolas Pitre <nico@cam.org> wrote:
> First of all, I mentioned at the time that using a . for separator 
> between the tagged version and the number of commits since then in the 
> git-describe output was a bad idea.  You just made the perfect 
> demonstration of that.  If it was just me I'd change that . for a + or a 
> : like the original patch did.

I preferred + as well, but we wound up with - in the final patch,
and GIT-VERSION-GEN replaces the - with a . because that makes RPM
happier or something:

  VN=$(echo "$VN" | sed -e 's/-/./g');

> Now you say that you don't want to wait for the release to happen before 
> using this cool new feature.  Well, I'd reply that life is tough.  

In comparsion to other things we all must deal with in life, this
is a cakewalk.  ;-) But yes, your point is well made.

> Either you 
> trick Junio into making a release sooner because the feature is just too 
> valuable to wait.  Or you try the feature (git-blame -w) and hope for 
> the best.  Certainly in that case you can predict the behavior of 
> older git-blame versions if you pass it -w which they don't understand?

I think that's where we are.  I can develop the feature, trick
git-gui into enabling it, but most end-users won't be able to use
it until Junio makes a 1.5.3-rc* or 1.5.3 final.  Tough for them.
Tricking Junio is very hard, he isn't easily tricked.  A good
quality for a mantainer to have.

> > I'd like to at least gracefully fail by
> > disabling an option, or suggesting the user upgrade their plumbing,
> > if an option isn't supported.
> 
> Well you should be able to just try the option and detect it when it 
> isn't supported.

Sure.

Except sometimes we have been lax about option checking and don't
always fail (though we usually do, but haven't always).  And on
Windows doing a fork+exec to poll an option is expensive.  Worse,
some options are destructive.  For example `update-ref -d`.

Should I run `update-ref -d refs/heads/master` to see if the
-d option is supported by update-ref, so that I know if I should
create a particular UI widget or not?  No, of course not.  I should
use a special temporary name, like GITGUI_TEST_FEATURE.  But now
I have to create and delete a temporary item just to decide if a
UI feature should be enabled.  And if the feature doesn't exist,
then I have to cleanup the temporary item "by hand".

Annoying.  Like this email thread I'm sure has become.
 
> > Unlike how we gracefully fail with a useful error message say
> > when an early 1.4 release that doesn't support offset deltas is
> > given a packfile with an OFS_DELTA in it (corrupt pack, recently
> > rediscussed on list).  Or when a 1.5.1 client tries to checkout
> > a tree that uses the new subproject mode in 1.5.2 (missing blob,
> > recently discussed on #git).
> 
> Those are different as you have an older version that couldn't 
> anticipate the future.  In your case you can "anticipate the past".

Yes, however my point here is that I think we have historically
been bad about making our software reasonably future-proof.

The .pack file has a version field, with value 2.  REF_DELTA is not
supported by those binaries that predated its introduction.  They are
unable to properly unpack, index or read a packfile using REF_DELTA.
Why did the .pack version stay at 2 if REF_DELTA is used in the file?

The packed-refs file was introduced without a version header
(whoops).  Later Junio had to wedge things into there in order to
get a version header of sorts so we could have the tag deref values
cached in the packed-refs.

We weren't strict enough in checking file modes in the tree parser
(yes, that was actually a real bug) but basically we didn't think
ahead to what an older binary should do when a future binary used a
file mode we didn't understand.  For example, the user setuid bit.
Or the new dirlink thing, which isn't even a valid mode in UNIX.

I think we are getting a little better at it, but in general we
have a really poor track record in this area.

-- 
Shawn.

Re: [PATCH] Make list of features auto-managed.

[Nicolas Pitre]

On Fri, 22 Jun 2007, Shawn O. Pearce wrote:

> Yes, however my point here is that I think we have historically
> been bad about making our software reasonably future-proof.
> 
> The .pack file has a version field, with value 2.  REF_DELTA is not
> supported by those binaries that predated its introduction.  They are
> unable to properly unpack, index or read a packfile using REF_DELTA.
> Why did the .pack version stay at 2 if REF_DELTA is used in the file?

Because:

1) you wouldn't be able to properly unpack, index or read a pack file 
   using those deltas even with a different pack version _anyway_, and

2) the pack encoding itself has not changed at all.

And #2 is real.  The introduction of an additional object type didn't 
turn the pack content into something that old git versions could 
(mis)interpret and be fooled by.

On the other hand, changes leading to pack version 3 really required a 
pack version bump as those changes could really be misinterpreted by 
older git versions.

The fact that old git versions call the pack corrupt when it contains 
unknown objects is a problem with the error message not the pack 
version.  We even had a bad error message for unknown pack versions too 
before you improved it.


Nicolas

Re: [PATCH] Make list of features auto-managed.

[Josef Weidendorfer]

On Friday 22 June 2007, Shawn O. Pearce wrote:
> > Now you say that you don't want to wait for the release to happen before 
> > using this cool new feature.  Well, I'd reply that life is tough.  
> 
> In comparsion to other things we all must deal with in life, this
> is a cakewalk.  ;-) But yes, your point is well made.

Hmm... Perhaps I am missing something.

IMHO this discussion is about how to cope with different versions of two
programs depending on each other, but being released in a loosely coupled way
(this is a problem Xorg now has with their independent releases of Xorg modules).

However, git-gui is released together with git (also in the foreseeable
future, even if we use submodules some day).
And therefore, a git user always will get installations of matching versions
of git and git-gui; this is even true for developers who run "make install"
in git source.

So IMHO there is no version check needed at all in this case. Anybody who
is installing git in strange partial ways should expect to be screwed.

Josef

Re: [PATCH] Make list of features auto-managed.

[Catalin Marinas]

"Shawn O. Pearce" <spearce@spearce.org> wrote:
> Here's the problem though: `git-blame -w` will be supported
> in Git 1.5.3 and later, we all know this.  But Git doesn't.
> Ask git-describe what version `master` and `next` are; its
> v1.5.2.2-249 and v1.5.2.2-1050.
[...]
> Maybe I should ask the StGIT folks how they deal with this, or if
> they just don't worry about it.  I'm suspecting its the latter...

We don't worry much about it. We actually try to be compatible with
some older versions and not use much of the new features in GIT (StGIT
uses a small subset anyway - see stgit/git.py).

-- 
Catalin

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

[Johannes Schindelin]

Hi,

On Thu, 21 Jun 2007, Junio C Hamano wrote:

> [...] using "git version" to check the version and say "1.5.3 has blame 
> -w" > might be easier to manage [ than list-features maintenance ...]

We do something similar already with curl features. So this sounds 
sensible to me. Shawn?

Ciao,
Dscho