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

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2011, 06:50 UTC
Message-ID: <7vaaenm957.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1305495440-30836-7-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

Show 8 quoted lines

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