Re: [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities

Johan Herland [off-list ref] writes:

+const char *server_supports(const char *feature)
 {
-	return server_capabilities &&
-		strstr(server_capabilities, feature) != NULL;
+	if (server_capabilities)
+		return strstr(server_capabilities, feature);
+	return NULL;
 }

I've been meaning to fix this part, but currently the feature set is given
as space separated list " featurea featureb featurec" and we check with a
token without any space around, e.g. "if (server_supports("no-done"))",
which is quite broken.

We should tighten this strstr() to make sure we are not matching in the
middle of a string, and the need to do so is even greater now that you are
going to introduce "foo=<value>" and the value could even be strings in
the future.

How about implementing rules like these:

 - feature must appear at the beginning of server_capabilities, or the
   byte immediately before the matched location in server_capabilities
   must be a SP; and

 - if "feature" does not end with an equal sign, it does not expect a
   value. The byte after the matched location in server_capabilities must
   be either the end of string or a SP. A feature that expects a value is
   checked with 'server_supports("feature=")' and the matched location in
   server_capabilities can be followed by anything (i.e. if at the end of
   string or a SP, it gets an empty string as the value, and otherwise it
   will get the stretch of bytes after the '=' up to the next SP).

Given the server_capabilities string "foo=ab bar=froboz boz=nitfol",
I would like to see these happen:

  server_supports("foo=") matches "foo=ab";

  server_supports("ab") does not match anything;

  server_supports("bar") does not match anything;

  server_supports("boz") matches boz=nitfol, without failing at
                         the end of bar=froboz that comes earlier.

On Monday 16 May 2011, Junio C Hamano wrote:

Johan Herland [off-list ref] writes:

quoted

+const char *server_supports(const char *feature)

 {

-	return server_capabilities &&
-		strstr(server_capabilities, feature) != NULL;
+	if (server_capabilities)
+		return strstr(server_capabilities, feature);
+	return NULL;

 }

I've been meaning to fix this part, but currently the feature set is
given as space separated list " featurea featureb featurec" and we check
with a token without any space around, e.g. "if
(server_supports("no-done"))", which is quite broken.

We should tighten this strstr() to make sure we are not matching in the
middle of a string, and the need to do so is even greater now that you
are going to introduce "foo=<value>" and the value could even be strings
in the future.

How about implementing rules like these:

[...]

Agreed. I'll take a stab at this in the re-roll.


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Heya,

On Sun, May 15, 2011 at 23:50, Junio C Hamano [off-list ref] wrote:

We should tighten this strstr() to make sure we are not matching in the
middle of a string, and the need to do so is even greater now that you are
going to introduce "foo=<value>" and the value could even be strings in
the future.

If we are writing this down somewhere, should we also dictate how
spaces should be escaped to be "forward-compatible"?

-- 
Cheers,

Sverre Rabbelier