Re: [PATCH 3/4] connect: learn to parse capabilities with values
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:54:28
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH 3/4] connect: learn to parse capabilities with values · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 3/4] connect: learn to parse capabilities with values · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH 3/4] connect: learn to parse capabilities with values · Junio C Hamano <hidden>
- 2016-06-15 · [PATCH 3/4] connect: learn to parse capabilities with values · Jeff King <hidden>
- 2016-06-15 · Re: [PATCH 3/4] connect: learn to parse capabilities with values · Eric Sunshine <hidden>
Junio C Hamano [off-list ref] 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().quoted
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 [off-list ref] Signed-off-by: Junio C Hamano <redacted> --- 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