On Mon, Aug 13, 2012 at 05:11:10PM -0400, Jeff King wrote:

> On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:
> 
> > >> +        if ((agent_feature = server_feature("agent", &agent_len)) != 
> > >> NULL &&
> > >> +            5 < agent_len && agent_feature[5] == '=') {
> > >>                  agent_supported = 1;
> > >> +                if (args.verbose) {
> > >> +                        fprintf(stderr, "Server version is %.*s\n",
> > >> +                                agent_len - 6, agent_feature + 6);
> > >> +                }
> > >> +        }
> > >
> > > Yeah, this is exactly the kind of ugliness I was trying to avoid with my
> > > allocating wrapper. Still, there is only one call site, so I do not care
> > > overly much (and I as I've already said, I'm lukewarm on the final two
> > > patches, anyway).
> > 
> > Actually, the above is vastly superiour compared to the allocating
> > kind.  Be honest and think about it.  If the caller wants to
> > allocate, it could, and it does not even have to count.  If the
> > caller does not want to allocate, it does not have to pay the price.
> 
> My point is that the run-time allocation price is quite small, but the
> readability cost of that ugly conditional with the magic "5" is
> non-trivial. But they are apples and oranges, so it is hard to compare
> their amounts directly.

So if we want to avoid the allocation, then this is how I would do it:
by returning the feature's _value_ and not the whole key. Since we know
that the beginning part must obviously match what we fed it anyway, it
is not that interesting.

-- >8 --
Subject: [PATCH] parse_feature_request: make it easier to see feature values

We already take care to parse key/value capabilities like
"foo=bar", but the code does not provide a good way of
actually finding out what is on the right-hand side of the
"=".

A server using "parse_feature_request" could accomplish this
with some extra parsing. You must skip past the "key"
portion manually, check for "=" versus NUL or space, and
then find the length by searching for the next space (or
NUL).  But clients can't even do that, since the
"server_supports" interface does not even return the
pointer.

Instead, let's have our parser share more information by
providing a pointer to the value and its length. The
"parse_feature_value" function returns a pointer to the
feature's value portion, along with the length of the value.
If the feature is missing, NULL is returned. If it does not
have an "=", then a zero-length value is returned.

Similarly, "server_feature_value" behaves in the same way,
but always checks the static server_feature_list variable.

We can then implement "server_supports" in terms of
"server_feature_value". We cannot implement the original
"parse_feature_request" in terms of our new function,
because it returned a pointer to the beginning of the
feature. However, no callers actually cared about the value
of the returned pointer, so we can simplify it to a boolean
just as we do for "server_supports".

Signed-off-by: Jeff King <p...@peff.net>
---
 cache.h   |  4 +++-
 connect.c | 45 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 67f28b4..95daa69 100644
--- a/cache.h
+++ b/cache.h
@@ -1038,7 +1038,9 @@ struct extra_have_objects {
 };
 extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int 
flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
-extern const char *parse_feature_request(const char *features, const char 
*feature);
+extern int parse_feature_request(const char *features, const char *feature);
+extern const char *server_feature_value(const char *feature, int *len_ret);
+extern const char *parse_feature_value(const char *feature_list, const char 
*feature, int *len_ret);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char 
*idx_path);
 
diff --git a/connect.c b/connect.c
index 55a85ad..49e56ba 100644
--- a/connect.c
+++ b/connect.c
@@ -115,12 +115,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
        return list;
 }
 
-int server_supports(const char *feature)
-{
-       return !!parse_feature_request(server_capabilities, feature);
-}
-
-const char *parse_feature_request(const char *feature_list, const char 
*feature)
+const char *parse_feature_value(const char *feature_list, const char *feature, 
int *lenp)
 {
        int len;
 
@@ -132,14 +127,46 @@ const char *parse_feature_request(const char 
*feature_list, const char *feature)
                const char *found = strstr(feature_list, feature);
                if (!found)
                        return NULL;
-               if ((feature_list == found || isspace(found[-1])) &&
-                   (!found[len] || isspace(found[len]) || found[len] == '='))
-                       return found;
+               if (feature_list == found || isspace(found[-1])) {
+                       const char *value = found + len;
+                       /* feature with no value (e.g., "thin-pack") */
+                       if (!*value || isspace(*value)) {
+                               if (lenp)
+                                       *lenp = 0;
+                               return value;
+                       }
+                       /* feature with a value (e.g., "agent=git/1.2.3") */
+                       else if (*value == '=') {
+                               value++;
+                               if (lenp)
+                                       *lenp = strcspn(value, " \t\n");
+                               return value;
+                       }
+                       /*
+                        * otherwise we matched a substring of another feature;
+                        * keep looking
+                        */
+               }
                feature_list = found + 1;
        }
        return NULL;
 }
 
+int parse_feature_request(const char *feature_list, const char *feature)
+{
+       return !!parse_feature_value(feature_list, feature, NULL);
+}
+
+const char *server_feature_value(const char *feature, int *len)
+{
+       return parse_feature_value(server_capabilities, feature, len);
+}
+
+int server_supports(const char *feature)
+{
+       return !!server_feature_value(feature, NULL);
+}
+
 enum protocol {
        PROTO_LOCAL = 1,
        PROTO_SSH,
-- 
1.7.12.rc2.11.gf0a1e27

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to