Re: [PATCH 3/4] connect: learn to parse capabilities with values
Junio C Hamano gits...@pobox.com writes:
I forgot to mention it, but the above was done also to make it
possible but not mandatory to pay extra allocation penalty. The
caller can choose to parse the string into an int, for example,
without extra allocation. Only the ones that want a string value
and keep a copy around do have to do xmemdupz().
Anyway, do you think this is even worth doing at this point? I'm
lukewarm on the final two patches due to the existence of
GIT_TRACE_PACKET, which is much more likely to be useful.
In the longer term, I think giving callers access to the parameter
value given to a capability is necessary. If we had this facility
in the old days, we wouldn't have done side-band-64k but spelled it
as side-band=64k.
For the agent=foo, certainly we don't need it.
Here are the first of two patches to replace your 3 and 4 without
extra allocations, primarily for further discussion.
-- 8 --
Subject: [PATCH 3/4] connect: expose the parameter to a capability
We already take care to parse a capability like foo=bar properly,
but the code does not provide a good way of actually finding out
what is on the right-hand side of the =.
Based on the patch by Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
---
cache.h | 1 +
connect.c | 27 ---
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index 06413e1..d239cee 100644
--- a/cache.h
+++ b/cache.h
@@ -1030,6 +1030,7 @@ 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 *server_feature(const char *feature, int *len_ret);
extern const char *parse_feature_request(const char *features, const char
*feature);
extern struct packed_git *parse_pack_index(unsigned char *sha1, const char
*idx_path);
diff --git a/connect.c b/connect.c
index 912cdde..42640bc 100644
--- a/connect.c
+++ b/connect.c
@@ -99,12 +99,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)
+static const char *parse_feature_request_1(const char *feature_list, const
char *feature, int *lenp)
{
int len;
@@ -117,13 +112,31 @@ const char *parse_feature_request(const char
*feature_list, const char *feature)
if (!found)
return NULL;
if ((feature_list == found || isspace(found[-1]))
- (!found[len] || isspace(found[len]) || found[len] == '='))
+ (!found[len] || isspace(found[len]) || found[len] == '=')) {
+ if (lenp)
+ *lenp = strcspn(found, \t\n);
return found;
+ }
feature_list = found + 1;
}
return NULL;
}
+const char *parse_feature_request(const char *feature_list, const char
*feature)
+{
+ return parse_feature_request_1(feature_list, feature, NULL);
+}
+
+const char *server_feature(const char *feature, int *len)
+{
+ return parse_feature_request_1(server_capabilities, feature, len);
+}
+
+int server_supports(const char *feature)
+{
+ return !!server_feature(feature, NULL);
+}
+
enum protocol {
PROTO_LOCAL = 1,
PROTO_SSH,
--
1.7.12.rc2.85.g1de7134
--
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
Re: [PATCH 3/4] connect: learn to parse capabilities with values
On Fri, Aug 10, 2012 at 3:58 AM, Jeff King p...@peff.net wrote: + * Parse features of the form feature=value. Returns NULL if the feature + * does not exist, the empty string if it exists but does not have an =, or + * the content to the right of the = until the first space (or end of + * string). The cannot contain literal spaces; double-quoting or similar s/The cannot/The value cannot/ -- ES -- 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
Re: [PATCH 3/4] connect: learn to parse capabilities with values
Jeff King p...@peff.net writes:
+/*
+ * Parse features of the form feature=value. Returns NULL if the feature
+ * does not exist, the empty string if it exists but does not have an =, or
+ * the content to the right of the = until the first space (or end of
+ * string). The cannot contain literal spaces; double-quoting or similar
+ * schemes would break compatibility, since older versions of git treat the
+ * space as a hard-delimiter without any context.
+ *
+ * The return value (if non-NULL) is newly allocated on the heap and belongs
to
+ * the caller.
+ */
+char *parse_feature_request_value(const char *feature_list, const char
*feature)
+{
+ const char *start = parse_feature_request(feature_list, feature);
+ const char *end;
+
+ if (!start || prefixcmp(start, feature))
+ return NULL;
+ start += strlen(feature);
+
+ if (*start == '=')
+ start++;
+ end = strchrnul(start, ' ');
+
+ return xmemdupz(start, end - start);
+}
Having to run strlen(feature) three times in this function (once in
parse_feature_request() as part of strstr() and the edge check of
the found string, once as part of prefixcmp() here, and then an
explicit strlen() to skip it) makes me feel dirty.
It is not wrong per-se, but it is likely that the caller has a
constant string as the feature when it called this function, so
perhaps just changing the function signature of server_supports,
i.e.
const char *server_supports(const char *feature)
{
return parse_feature_request(server_capabilities, feature);
}
to return var=val would be more than sufficient.
Then the existing callers can keep doing
if (server_supports(thin-pack))
if (!server_supports(quiet))
and a new caller can do something like
agent = server_supports(agent);
if (!agent || !agent[5])
... no agent ...
else {
int span = strcspn(agent + 6, \t\n);
printf(I found agent=%.*s!\n, span, agent + 6);
}
which doesn't look too bad.
--
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
Re: [PATCH 3/4] connect: learn to parse capabilities with values
On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
+/*
+ * Parse features of the form feature=value. Returns NULL if the feature
+ * does not exist, the empty string if it exists but does not have an =,
or
+ * the content to the right of the = until the first space (or end of
+ * string). The cannot contain literal spaces; double-quoting or similar
+ * schemes would break compatibility, since older versions of git treat the
+ * space as a hard-delimiter without any context.
+ *
+ * The return value (if non-NULL) is newly allocated on the heap and
belongs to
+ * the caller.
+ */
+char *parse_feature_request_value(const char *feature_list, const char
*feature)
+{
+ const char *start = parse_feature_request(feature_list, feature);
+ const char *end;
+
+ if (!start || prefixcmp(start, feature))
+ return NULL;
+ start += strlen(feature);
+
+ if (*start == '=')
+ start++;
+ end = strchrnul(start, ' ');
+
+ return xmemdupz(start, end - start);
+}
Having to run strlen(feature) three times in this function (once in
parse_feature_request() as part of strstr() and the edge check of
the found string, once as part of prefixcmp() here, and then an
explicit strlen() to skip it) makes me feel dirty.
I thought about that, but it seems like a quite premature optimization.
It is three extra strlens per network conversation. _If_ you have turned
on double-verbosity in fetch-pack. I considered reusing the existing
parse_feature_request function more valuable from a maintenance
standpoint.
I would think the extra memory allocation would dwarf it, anyway.
It is not wrong per-se, but it is likely that the caller has a
constant string as the feature when it called this function, so
perhaps just changing the function signature of server_supports,
i.e.
const char *server_supports(const char *feature)
{
return parse_feature_request(server_capabilities, feature);
}
to return var=val would be more than sufficient.
That was in fact my first iteration, but...
Then the existing callers can keep doing
if (server_supports(thin-pack))
if (!server_supports(quiet))
and a new caller can do something like
agent = server_supports(agent);
if (!agent || !agent[5])
... no agent ...
else {
int span = strcspn(agent + 6, \t\n);
printf(I found agent=%.*s!\n, span, agent + 6);
}
which doesn't look too bad.
I didn't want to force callers to have to deal with ad-hoc parsing.
Anyway, do you think this is even worth doing at this point? I'm
lukewarm on the final two patches due to the existence of
GIT_TRACE_PACKET, which is much more likely to be useful.
-Peff
--
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
Re: [PATCH 3/4] connect: learn to parse capabilities with values
Jeff King p...@peff.net writes:
I would think the extra memory allocation would dwarf it, anyway.
...
and a new caller can do something like
agent = server_supports(agent);
if (!agent || !agent[5])
... no agent ...
else {
int span = strcspn(agent + 6, \t\n);
printf(I found agent=%.*s!\n, span, agent + 6);
}
which doesn't look too bad.
I forgot to mention it, but the above was done also to make it
possible but not mandatory to pay extra allocation penalty. The
caller can choose to parse the string into an int, for example,
without extra allocation. Only the ones that want a string value
and keep a copy around do have to do xmemdupz().
Anyway, do you think this is even worth doing at this point? I'm
lukewarm on the final two patches due to the existence of
GIT_TRACE_PACKET, which is much more likely to be useful.
In the longer term, I think giving callers access to the parameter
value given to a capability is necessary. If we had this facility
in the old days, we wouldn't have done side-band-64k but spelled it
as side-band=64k.
For the agent=foo, certainly we don't need it.
--
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
