{"thread":{"id":"58943","subject":"[PATCH] server_supports_v2(): use a separate function for die_on_error","startedAt":"2022-12-13T10:53:12Z","lastAt":"2022-12-13T10:53:12Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"468978","messageId":"Y5hZirNRGLQUZfOZ@coredump.intra.peff.net","threadId":"58943","inReplyTo":null,"subject":"[PATCH] server_supports_v2(): use a separate function for die_on_error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-12-13T10:52:58Z","receivedAt":"2022-12-13T10:53:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The server_supports_v2() helper lets a caller find out if the server\nsupports a feature, and will optionally die if it's not supported. This\nmakes the return value confusing, as it's only meaningful when the\nfunction is not asked to die.\n\nCoverity flagged a new call like:\n\n  /* check that we support \"foo\" */\n  server_supports_v2(\"foo\", 1);\n\ncomplaining that we usually checked the return value, but this time we\ndidn't. But this call is correct, and other ones that did:\n\n  if (server_supports_v2(\"foo\", 1))\n          do_something_with_foo();\n\nare \"wrong\", in the sense that we know the conditional will always be\ntrue (but there's no bug; the code is simply misleading).\n\nLet's split the \"die\" behavior into its own function which returns void,\nand modify each caller to use the correct one.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nArguably this is just churn. It does help static analyzers like Coverity\nunderstand better what's going on, though, and when I looked at the\nwarning it produced, I had to store for a while myself to understand\nwhat was going on. So maybe it's worth doing.\n\nNote that when merged to 'next', it will need these two calls modified\non top (the first one of which is what caused Coverity's useless\nwarning):\n\n    diff --git a/connect.c b/connect.c\n    index 9bb946a238..63e59641c0 100644\n    --- a/connect.c\n    +++ b/connect.c\n    @@ -500,7 +500,7 @@ int get_remote_bundle_uri(int fd_out, struct packet_reader *reader,\n     \tint line_nr = 1;\n     \n     \t/* Assert bundle-uri support */\n    -\tserver_supports_v2(\"bundle-uri\", 1);\n    +\tensure_server_supports_v2(\"bundle-uri\");\n     \n     \t/* (Re-)send capabilities */\n     \tsend_capabilities(fd_out, reader);\n    diff --git a/transport.c b/transport.c\n    index 957dca4923..d1a2035eab 100644\n    --- a/transport.c\n    +++ b/transport.c\n    @@ -382,7 +382,7 @@ static int get_bundle_uri(struct transport *transport)\n     \t * \"Support\" protocol v0 and v2 without bundle-uri support by\n     \t * silently degrading to a NOOP.\n     \t */\n    -\tif (!server_supports_v2(\"bundle-uri\", 0))\n    +\tif (!server_supports_v2(\"bundle-uri\"))\n     \t\treturn 0;\n     \n     \tpacket_reader_init(&reader, data->fd[0], NULL, 0,\n\n\n connect.c    | 21 ++++++++++++---------\n connect.h    |  3 ++-\n fetch-pack.c | 12 ++++++------\n 3 files changed, 20 insertions(+), 16 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 5ea53deda2..eef752f14b 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -66,7 +66,7 @@ static NORETURN void die_initial_contact(int unexpected)\n }\n \n /* Checks if the server supports the capability 'c' */\n-int server_supports_v2(const char *c, int die_on_error)\n+int server_supports_v2(const char *c)\n {\n \tint i;\n \n@@ -76,11 +76,13 @@ int server_supports_v2(const char *c, int die_on_error)\n \t\t    (!*out || *out == '='))\n \t\t\treturn 1;\n \t}\n+\treturn 0;\n+}\n \n-\tif (die_on_error)\n+void ensure_server_supports_v2(const char *c)\n+{\n+\tif (!server_supports_v2(c))\n \t\tdie(_(\"server doesn't support '%s'\"), c);\n-\n-\treturn 0;\n }\n \n int server_feature_v2(const char *c, const char **v)\n@@ -477,7 +479,7 @@ static void send_capabilities(int fd_out, struct packet_reader *reader)\n {\n \tconst char *hash_name;\n \n-\tif (server_supports_v2(\"agent\", 0))\n+\tif (server_supports_v2(\"agent\"))\n \t\tpacket_write_fmt(fd_out, \"agent=%s\", git_user_agent_sanitized());\n \n \tif (server_feature_v2(\"object-format\", &hash_name)) {\n@@ -504,17 +506,18 @@ struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,\n \t\t&transport_options->unborn_head_target : NULL;\n \t*list = NULL;\n \n-\tif (server_supports_v2(\"ls-refs\", 1))\n-\t\tpacket_write_fmt(fd_out, \"command=ls-refs\\n\");\n+\tensure_server_supports_v2(\"ls-refs\");\n+\tpacket_write_fmt(fd_out, \"command=ls-refs\\n\");\n \n \t/* Send capabilities */\n \tsend_capabilities(fd_out, reader);\n \n-\tif (server_options && server_options->nr &&\n-\t    server_supports_v2(\"server-option\", 1))\n+\tif (server_options && server_options->nr) {\n+\t\tensure_server_supports_v2(\"server-option\");\n \t\tfor (i = 0; i < server_options->nr; i++)\n \t\t\tpacket_write_fmt(fd_out, \"server-option=%s\",\n \t\t\t\t\t server_options->items[i].string);\n+\t}\n \n \tpacket_delim(fd_out);\n \t/* When pushing we don't want to request the peeled tags */\ndiff --git a/connect.h b/connect.h\nindex c53586e929..b26f7de784 100644\n--- a/connect.h\n+++ b/connect.h\n@@ -20,7 +20,8 @@ enum protocol_version discover_version(struct packet_reader *reader);\n \n int server_supports_hash(const char *desired, int *feature_supported);\n const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp, int *offset);\n-int server_supports_v2(const char *c, int die_on_error);\n+int server_supports_v2(const char *c);\n+void ensure_server_supports_v2(const char *c);\n int server_feature_v2(const char *c, const char **v);\n int server_supports_feature(const char *c, const char *feature,\n \t\t\t    int die_on_error);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 998fc2fa1e..04016d1e32 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1317,15 +1317,15 @@ static void write_fetch_command_and_capabilities(struct strbuf *req_buf,\n {\n \tconst char *hash_name;\n \n-\tif (server_supports_v2(\"fetch\", 1))\n-\t\tpacket_buf_write(req_buf, \"command=fetch\");\n-\tif (server_supports_v2(\"agent\", 0))\n+\tensure_server_supports_v2(\"fetch\");\n+\tpacket_buf_write(req_buf, \"command=fetch\");\n+\tif (server_supports_v2(\"agent\"))\n \t\tpacket_buf_write(req_buf, \"agent=%s\", git_user_agent_sanitized());\n-\tif (advertise_sid && server_supports_v2(\"session-id\", 0))\n+\tif (advertise_sid && server_supports_v2(\"session-id\"))\n \t\tpacket_buf_write(req_buf, \"session-id=%s\", trace2_session_id());\n-\tif (server_options && server_options->nr &&\n-\t    server_supports_v2(\"server-option\", 1)) {\n+\tif (server_options && server_options->nr) {\n \t\tint i;\n+\t\tensure_server_supports_v2(\"server-option\");\n \t\tfor (i = 0; i < server_options->nr; i++)\n \t\t\tpacket_buf_write(req_buf, \"server-option=%s\",\n \t\t\t\t\t server_options->items[i].string);\n-- \n2.39.0.rc2.569.g45ab4188ae\n"}]}