{"thread":{"id":"18242","subject":"[RFH Patch 2/2] http_init(): Fix config file parsing","startedAt":"2009-03-10T02:19:44Z","lastAt":"2009-03-10T11:07:56Z","messageCount":3,"participants":["Junio C Hamano","Jay Soffian","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"107535","messageId":"7vfxhmce67.fsf@gitster.siamese.dyndns.org","threadId":"18242","inReplyTo":null,"subject":"[RFH Patch 2/2] http_init(): Fix config file parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-10T02:19:44Z","receivedAt":"2009-03-10T02:19:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We honor the command line options, environment variables, variables in\nrepository configuration file, variables in user's global configuration\nfile, variables in the system configuration file, and then finally use\nbuilt-in default.  To implement this semantics, the code should:\n\n - start from built-in default values;\n\n - call git_config() with the configuration parser callback, which\n   implements \"later definition overrides earlier ones\" logic\n   (git_config() reads the system's, user's and then repository's\n   configuration file in this order);\n\n - override the result from the above with environment variables if set;\n\n - override the result from the above with command line options.\n\nThe initialization code http_init() for http transfer got this wrong, and\nimplemented a \"first one wins, ignoring the later ones\" in http_options(),\nto compensate this mistake, read environment variables before calling\ngit_config().  This is all wrong.\n\nAs a second class citizen, the http codepath hasn't been audited as\nclosely as other parts of the system, but we should try to bring sanity to\nthe existing codebase before inviting contributors to improve on it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is what I need help from people who actually do use http transport\n   and extra set of eyeballs to spot silly mistakes.\n\n http.c |   66 +++++++++++++++++++++++----------------------------------------\n 1 files changed, 24 insertions(+), 42 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex d1ead66..f4f0bf6 100644\n--- a/http.c\n+++ b/http.c\n@@ -94,52 +94,33 @@ static void process_curl_messages(void)\n static int http_options(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(\"http.sslverify\", var)) {\n-\t\tif (curl_ssl_verify == -1)\n-\t\t\tcurl_ssl_verify = git_config_bool(var, value);\n-\t\treturn 0;\n-\t}\n-\n-\tif (!strcmp(\"http.sslcert\", var)) {\n-\t\tif (ssl_cert == NULL)\n-\t\t\treturn git_config_string(&ssl_cert, var, value);\n+\t\tcurl_ssl_verify = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(\"http.sslcert\", var))\n+\t\treturn git_config_string(&ssl_cert, var, value);\n #if LIBCURL_VERSION_NUM >= 0x070902\n-\tif (!strcmp(\"http.sslkey\", var)) {\n-\t\tif (ssl_key == NULL)\n-\t\t\treturn git_config_string(&ssl_key, var, value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(\"http.sslkey\", var))\n+\t\treturn git_config_string(&ssl_key, var, value);\n #endif\n #if LIBCURL_VERSION_NUM >= 0x070908\n-\tif (!strcmp(\"http.sslcapath\", var)) {\n-\t\tif (ssl_capath == NULL)\n-\t\t\treturn git_config_string(&ssl_capath, var, value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(\"http.sslcapath\", var))\n+\t\treturn git_config_string(&ssl_capath, var, value);\n #endif\n-\tif (!strcmp(\"http.sslcainfo\", var)) {\n-\t\tif (ssl_cainfo == NULL)\n-\t\t\treturn git_config_string(&ssl_cainfo, var, value);\n-\t\treturn 0;\n-\t}\n-\n+\tif (!strcmp(\"http.sslcainfo\", var))\n+\t\treturn git_config_string(&ssl_cainfo, var, value);\n #ifdef USE_CURL_MULTI\n \tif (!strcmp(\"http.maxrequests\", var)) {\n-\t\tif (max_requests == -1)\n-\t\t\tmax_requests = git_config_int(var, value);\n+\t\tmax_requests = git_config_int(var, value);\n \t\treturn 0;\n \t}\n #endif\n-\n \tif (!strcmp(\"http.lowspeedlimit\", var)) {\n-\t\tif (curl_low_speed_limit == -1)\n-\t\t\tcurl_low_speed_limit = (long)git_config_int(var, value);\n+\t\tcurl_low_speed_limit = (long)git_config_int(var, value);\n \t\treturn 0;\n \t}\n \tif (!strcmp(\"http.lowspeedtime\", var)) {\n-\t\tif (curl_low_speed_time == -1)\n-\t\t\tcurl_low_speed_time = (long)git_config_int(var, value);\n+\t\tcurl_low_speed_time = (long)git_config_int(var, value);\n \t\treturn 0;\n \t}\n \n@@ -147,11 +128,8 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\tcurl_ftp_no_epsv = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n-\tif (!strcmp(\"http.proxy\", var)) {\n-\t\tif (curl_http_proxy == NULL)\n-\t\t\treturn git_config_string(&curl_http_proxy, var, value);\n-\t\treturn 0;\n-\t}\n+\tif (!strcmp(\"http.proxy\", var))\n+\t\treturn git_config_string(&curl_http_proxy, var, value);\n \n \t/* Fall back on the default ones */\n \treturn git_default_config(var, value, cb);\n@@ -217,6 +195,8 @@ void http_init(struct remote *remote)\n \tchar *low_speed_limit;\n \tchar *low_speed_time;\n \n+\tgit_config(http_options, NULL);\n+\n \tcurl_global_init(CURL_GLOBAL_ALL);\n \n \tif (remote && remote->http_proxy)\n@@ -241,14 +221,18 @@ void http_init(struct remote *remote)\n \tif (getenv(\"GIT_SSL_NO_VERIFY\"))\n \t\tcurl_ssl_verify = 0;\n \n-\tssl_cert = getenv(\"GIT_SSL_CERT\");\n+\tif (getenv(\"GIT_SSL_CERT\"))\n+\t\tssl_cert = getenv(\"GIT_SSL_CERT\");\n #if LIBCURL_VERSION_NUM >= 0x070902\n-\tssl_key = getenv(\"GIT_SSL_KEY\");\n+\tif (getenv(\"GIT_SSL_KEY\"))\n+\t\tssl_key = getenv(\"GIT_SSL_KEY\");\n #endif\n #if LIBCURL_VERSION_NUM >= 0x070908\n-\tssl_capath = getenv(\"GIT_SSL_CAPATH\");\n+\tif (getenv(\"GIT_SSL_CAPATH\"))\n+\t\tssl_capath = getenv(\"GIT_SSL_CAPATH\");\n #endif\n-\tssl_cainfo = getenv(\"GIT_SSL_CAINFO\");\n+\tif (getenv(\"GIT_SSL_CAINFO\"))\n+\t\tssl_cainfo = getenv(\"GIT_SSL_CAINFO\");\n \n \tlow_speed_limit = getenv(\"GIT_HTTP_LOW_SPEED_LIMIT\");\n \tif (low_speed_limit != NULL)\n@@ -257,8 +241,6 @@ void http_init(struct remote *remote)\n \tif (low_speed_time != NULL)\n \t\tcurl_low_speed_time = strtol(low_speed_time, NULL, 10);\n \n-\tgit_config(http_options, NULL);\n-\n \tif (curl_ssl_verify == -1)\n \t\tcurl_ssl_verify = 1;\n \n-- \n1.6.2.206.g5bda76\n"},{"id":"107537","messageId":"76718490903092005n66bca743t67eb7f0ff830d9d7@mail.gmail.com","threadId":"18242","inReplyTo":"7vfxhmce67.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFH Patch 2/2] http_init(): Fix config file parsing","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-03-10T03:05:53Z","receivedAt":"2009-03-10T03:05:53Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Mar 9, 2009 at 10:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>  * This is what I need help from people who actually do use http transport\n>   and extra set of eyeballs to spot silly mistakes.\n\nIt looks sane to me.\n\n> @@ -241,14 +221,18 @@ void http_init(struct remote *remote)\n>        if (getenv(\"GIT_SSL_NO_VERIFY\"))\n>                curl_ssl_verify = 0;\n>\n> -       ssl_cert = getenv(\"GIT_SSL_CERT\");\n> +       if (getenv(\"GIT_SSL_CERT\"))\n> +               ssl_cert = getenv(\"GIT_SSL_CERT\");\n>  #if LIBCURL_VERSION_NUM >= 0x070902\n> -       ssl_key = getenv(\"GIT_SSL_KEY\");\n> +       if (getenv(\"GIT_SSL_KEY\"))\n> +               ssl_key = getenv(\"GIT_SSL_KEY\");\n>  #endif\n>  #if LIBCURL_VERSION_NUM >= 0x070908\n> -       ssl_capath = getenv(\"GIT_SSL_CAPATH\");\n> +       if (getenv(\"GIT_SSL_CAPATH\"))\n> +               ssl_capath = getenv(\"GIT_SSL_CAPATH\");\n>  #endif\n> -       ssl_cainfo = getenv(\"GIT_SSL_CAINFO\");\n> +       if (getenv(\"GIT_SSL_CAINFO\"))\n> +               ssl_cainfo = getenv(\"GIT_SSL_CAINFO\");\n\nWould these be a little cleaner with a temporary variable. e.g.\n\nchar *value;\n\nif ((value = getenv(\"GIT_SSL_CERT\")))\n    ssl_cert = value;\n\n\nj.\n"},{"id":"107564","messageId":"alpine.DEB.1.00.0903101206440.14295@intel-tinevez-2-302","threadId":"18242","inReplyTo":"76718490903092005n66bca743t67eb7f0ff830d9d7@mail.gmail.com","subject":"Re: [RFH Patch 2/2] http_init(): Fix config file parsing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-03-10T11:07:56Z","receivedAt":"2009-03-10T11:07:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 9 Mar 2009, Jay Soffian wrote:\n\n> On Mon, Mar 9, 2009 at 10:19 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> \n> > @@ -241,14 +221,18 @@ void http_init(struct remote *remote)\n> >        if (getenv(\"GIT_SSL_NO_VERIFY\"))\n> >                curl_ssl_verify = 0;\n> >\n> > -       ssl_cert = getenv(\"GIT_SSL_CERT\");\n> > +       if (getenv(\"GIT_SSL_CERT\"))\n> > +               ssl_cert = getenv(\"GIT_SSL_CERT\");\n> >  #if LIBCURL_VERSION_NUM >= 0x070902\n> > -       ssl_key = getenv(\"GIT_SSL_KEY\");\n> > +       if (getenv(\"GIT_SSL_KEY\"))\n> > +               ssl_key = getenv(\"GIT_SSL_KEY\");\n> >  #endif\n> >  #if LIBCURL_VERSION_NUM >= 0x070908\n> > -       ssl_capath = getenv(\"GIT_SSL_CAPATH\");\n> > +       if (getenv(\"GIT_SSL_CAPATH\"))\n> > +               ssl_capath = getenv(\"GIT_SSL_CAPATH\");\n> >  #endif\n> > -       ssl_cainfo = getenv(\"GIT_SSL_CAINFO\");\n> > +       if (getenv(\"GIT_SSL_CAINFO\"))\n> > +               ssl_cainfo = getenv(\"GIT_SSL_CAINFO\");\n> \n> Would these be a little cleaner with a temporary variable. e.g.\n> \n> char *value;\n> \n> if ((value = getenv(\"GIT_SSL_CERT\")))\n>     ssl_cert = value;\n\nNah, you should go the full nine yards right away:\n\nstatic void set_from_env(const char **variable, const char *name)\n{\n\tconst char *value = getenv(name);\n\tif (value)\n\t\t*variable = value;\n}\n\nCiao,\nDscho\n"}]}