@@ -66,7 +66,7 @@ static NORETURN void die_initial_contact(int unexpected)
}
/* Checks if the server supports the capability 'c' */
-int server_supports_v2(const char *c, int die_on_error)
+int server_supports_v2(const char *c)
{
int i;
@@ -76,11 +76,13 @@ int server_supports_v2(const char *c, int die_on_error)
(!*out || *out == '='))
return 1;
}
+ return 0;
+}
- if (die_on_error)
+void ensure_server_supports_v2(const char *c)
+{
+ if (!server_supports_v2(c))
die(_("server doesn't support '%s'"), c);
-
- return 0;
}
int server_feature_v2(const char *c, const char **v)
@@ -477,7 +479,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)
{
const char *hash_name;
- if (server_supports_v2("agent", 0))
+ if (server_supports_v2("agent"))
packet_write_fmt(fd_out, "agent=%s", git_user_agent_sanitized());
if (server_feature_v2("object-format", &hash_name)) {
@@ -504,17 +506,18 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
&transport_options->unborn_head_target : NULL;
*list = NULL;
- if (server_supports_v2("ls-refs", 1))
- packet_write_fmt(fd_out, "command=ls-refs\n");
+ ensure_server_supports_v2("ls-refs");
+ packet_write_fmt(fd_out, "command=ls-refs\n");
/* Send capabilities */
send_capabilities(fd_out, reader);
- if (server_options && server_options->nr &&
- server_supports_v2("server-option", 1))
+ if (server_options && server_options->nr) {
+ ensure_server_supports_v2("server-option");
for (i = 0; i < server_options->nr; i++)
packet_write_fmt(fd_out, "server-option=%s",
server_options->items[i].string);
+ }
packet_delim(fd_out);
/* When pushing we don't want to request the peeled tags */
@@ -20,7 +20,8 @@ enum protocol_version discover_version(struct packet_reader *reader);
int server_supports_hash(const char *desired, int *feature_supported);
const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset);
-int server_supports_v2(const char *c, int die_on_error);
+int server_supports_v2(const char *c);
+void ensure_server_supports_v2(const char *c);
int server_feature_v2(const char *c, const char **v);
int server_supports_feature(const char *c, const char *feature,
int die_on_error);
@@ -1317,15 +1317,15 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,
{
const char *hash_name;
- if (server_supports_v2("fetch", 1))
- packet_buf_write(req_buf, "command=fetch");
- if (server_supports_v2("agent", 0))
+ ensure_server_supports_v2("fetch");
+ packet_buf_write(req_buf, "command=fetch");
+ if (server_supports_v2("agent"))
packet_buf_write(req_buf, "agent=%s", git_user_agent_sanitized());
- if (advertise_sid && server_supports_v2("session-id", 0))
+ if (advertise_sid && server_supports_v2("session-id"))
packet_buf_write(req_buf, "session-id=%s", trace2_session_id());
- if (server_options && server_options->nr &&
- server_supports_v2("server-option", 1)) {
+ if (server_options && server_options->nr) {
int i;
+ ensure_server_supports_v2("server-option");
for (i = 0; i < server_options->nr; i++)
packet_buf_write(req_buf, "server-option=%s",
server_options->items[i].string);
The server_supports_v2() helper lets a caller find out if the server supports a feature, and will optionally die if it's not supported. This makes the return value confusing, as it's only meaningful when the function is not asked to die. Coverity flagged a new call like: /* check that we support "foo" */ server_supports_v2("foo", 1); complaining that we usually checked the return value, but this time we didn't. But this call is correct, and other ones that did: if (server_supports_v2("foo", 1)) do_something_with_foo(); are "wrong", in the sense that we know the conditional will always be true (but there's no bug; the code is simply misleading). Let's split the "die" behavior into its own function which returns void, and modify each caller to use the correct one. Signed-off-by: Jeff King <peff@peff.net> --- Arguably this is just churn. It does help static analyzers like Coverity understand better what's going on, though, and when I looked at the warning it produced, I had to store for a while myself to understand what was going on. So maybe it's worth doing. Note that when merged to 'next', it will need these two calls modified on top (the first one of which is what caused Coverity's useless warning): diff --git a/connect.c b/connect.c index 9bb946a238..63e59641c0 100644 --- a/connect.c +++ b/connect.c @@ -500,7 +500,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader, int line_nr = 1; /* Assert bundle-uri support */ - server_supports_v2("bundle-uri", 1); + ensure_server_supports_v2("bundle-uri"); /* (Re-)send capabilities */ send_capabilities(fd_out, reader); diff --git a/transport.c b/transport.c index 957dca4923..d1a2035eab 100644 --- a/transport.c +++ b/transport.c @@ -382,7 +382,7 @@ static int get_bundle_uri(struct transport *transport) * "Support" protocol v0 and v2 without bundle-uri support by * silently degrading to a NOOP. */ - if (!server_supports_v2("bundle-uri", 0)) + if (!server_supports_v2("bundle-uri")) return 0; packet_reader_init(&reader, data->fd[0], NULL, 0, connect.c | 21 ++++++++++++--------- connect.h | 3 ++- fetch-pack.c | 12 ++++++------ 3 files changed, 20 insertions(+), 16 deletions(-)