{"thread":{"id":"37187","subject":"[PATCH] config.c: change the function signature of `git_config_string()`","startedAt":"2014-07-22T10:49:56Z","lastAt":"2014-07-23T17:25:51Z","messageCount":10,"participants":["Tanay Abhra","Jeff King","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246495","messageId":"1406026196-17877-1-git-send-email-tanayabh@gmail.com","threadId":"37187","inReplyTo":null,"subject":"[PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-22T10:49:56Z","receivedAt":"2014-07-22T10:49:56Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"`git_config_string()` output parameter `dest` is declared as a const\nwhich is unnecessary as the caller of the function is given a strduped\nstring which can be modified without causing any harm.\n\nThus, remove the const from the function signature.\n\nSigned-off-by: Tanay Abhra <tanayabh@gmail.com>\n---\n cache.h  | 2 +-\n config.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 92fc9f1..93d357a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1303,7 +1303,7 @@ extern unsigned long git_config_ulong(const char *, const char *);\n extern int git_config_bool_or_int(const char *, const char *, int *);\n extern int git_config_bool(const char *, const char *);\n extern int git_config_maybe_bool(const char *, const char *);\n-extern int git_config_string(const char **, const char *, const char *);\n+extern int git_config_string(char **, const char *, const char *);\n extern int git_config_pathname(const char **, const char *, const char *);\n extern int git_config_set_in_file(const char *, const char *, const char *);\n extern int git_config_set(const char *, const char *);\ndiff --git a/config.c b/config.c\nindex ba882a1..25e28a7 100644\n--- a/config.c\n+++ b/config.c\n@@ -633,7 +633,7 @@ int git_config_bool(const char *name, const char *value)\n \treturn !!git_config_bool_or_int(name, value, &discard);\n }\n \n-int git_config_string(const char **dest, const char *var, const char *value)\n+int git_config_string(char **dest, const char *var, const char *value)\n {\n \tif (!value)\n \t\treturn config_error_nonbool(var);\n-- \n1.9.0.GIT\n"},{"id":"246496","messageId":"20140722110720.GA386@peff.net","threadId":"37187","inReplyTo":"1406026196-17877-1-git-send-email-tanayabh@gmail.com","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-22T11:07:20Z","receivedAt":"2014-07-22T11:07:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 22, 2014 at 03:49:56AM -0700, Tanay Abhra wrote:\n\n> `git_config_string()` output parameter `dest` is declared as a const\n> which is unnecessary as the caller of the function is given a strduped\n> string which can be modified without causing any harm.\n> \n> Thus, remove the const from the function signature.\n\nYou are correct that it is unnecessary. However, this patch alone is not\nsufficient because of the way const-ness in C works. If I have:\n\n  static const char *some_global;\n\nthen with your patch, calling:\n\n  git_config_string(&some_global, var, value);\n\nwill complain that we are passing a pointer to \"const char *\", not a\npointer to \"char *\". And indeed, compiling with your patch introduces a\nton of compiler warnings.\n\nWe would have to convert each of the variables we pass to it to:\n\n  static char *some_global;\n\nThat's not so bad, but:\n\n  static char *some_global = \"some_default_value\";\n\nis wrong. Such a global sometimes points to const storage (i.e.,\ninitially), and sometimes to allocated storage (if it was loaded from\nconfig). We simply keep the latter as a const pointer (since we would\nnot bother to free it at the end of the program anyway), and that\ndecision influences git_config_string, which is just a helper for\nsetting such variables anyway.\n\nSo I would not mind lifting this unnecessary restriction on\ngit_config_string, but I do not see a way to do it without making the\nrest of the code much uglier (and I do not see a particular advantage in\nmodifying git_config_string here that would make it worth the trouble).\n\n-Peff\n"},{"id":"246498","messageId":"53CE4DCE.1010908@gmail.com","threadId":"37187","inReplyTo":"20140722110720.GA386@peff.net","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-22T11:41:02Z","receivedAt":"2014-07-22T11:41:02Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/22/2014 4:37 PM, Jeff King wrote:\n> On Tue, Jul 22, 2014 at 03:49:56AM -0700, Tanay Abhra wrote:\n> \n>> `git_config_string()` output parameter `dest` is declared as a const\n>> which is unnecessary as the caller of the function is given a strduped\n>> string which can be modified without causing any harm.\n>>\n>> Thus, remove the const from the function signature.\n> \n> You are correct that it is unnecessary. However, this patch alone is not\n> sufficient because of the way const-ness in C works. If I have:\n> \n>   static const char *some_global;\n> \n> then with your patch, calling:\n> \n>   git_config_string(&some_global, var, value);\n> \n> will complain that we are passing a pointer to \"const char *\", not a\n> pointer to \"char *\". And indeed, compiling with your patch introduces a\n> ton of compiler warnings.\n>\n\nI had also thought that the compiler would raise lot of warnings but it didn't\non the first compile.\nNow I checked again and now it complains a lot, maybe it because I was tinkering\nwith my config.mak, dunno.\n\n> We would have to convert each of the variables we pass to it to:\n> \n>   static char *some_global;\n> \n> That's not so bad, but:\n> \n>   static char *some_global = \"some_default_value\";\n> \n> is wrong. Such a global sometimes points to const storage (i.e.,\n> initially), and sometimes to allocated storage (if it was loaded from\n> config). We simply keep the latter as a const pointer (since we would\n> not bother to free it at the end of the program anyway), and that\n> decision influences git_config_string, which is just a helper for\n> setting such variables anyway.\n> \n> So I would not mind lifting this unnecessary restriction on\n> git_config_string, but I do not see a way to do it without making the\n> rest of the code much uglier (and I do not see a particular advantage in\n> modifying git_config_string here that would make it worth the trouble).\n> \n\nYes, you are right. This patch is the conclusion of discussion in [1].\nI used the same function signature as git_config_string for\ngit_config_get_string() which lead to some ugly casts like\n\n+git_config_get_string(\"imap.folder\", (const char**)&imap_folder);\n\nin imap-send.c patch and others. What should we do about such cases, I used\neither an intermediate variable or casts but Junio commented that it would be better\nif the dest parameter was a non-const and that it was a weakness of the config-set\nAPI that demanded the dest to be a const pointer.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/253948/\n"},{"id":"246499","messageId":"vpqsiltsjm7.fsf@anie.imag.fr","threadId":"37187","inReplyTo":"20140722110720.GA386@peff.net","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-22T11:44:48Z","receivedAt":"2014-07-22T11:44:48Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King <peff@peff.net> writes:\n\n> will complain that we are passing a pointer to \"const char *\", not a\n> pointer to \"char *\". And indeed, compiling with your patch introduces a\n> ton of compiler warnings.\n\nTanay: are you not compiling with gcc -Wall -Werror?\n\n(see my earlier message, just create a file config.mak containing\n\n  CFLAGS += -Wdeclaration-after-statement -Wall -Werror\n\n)\n\n> We would have to convert each of the variables we pass to it to:\n>\n>   static char *some_global;\n\nOK, it seems I got convinced too quickly by Junio ;-). The function\nproduces a char * that can be modified, but it also receives a value,\nand the function should keep the \"const\" to allow passing \"const char\n*\".\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246500","messageId":"53CE4F96.3000409@gmail.com","threadId":"37187","inReplyTo":"vpqsiltsjm7.fsf@anie.imag.fr","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Tanay Abhra","fromEmail":"tanayabh@gmail.com","sentAt":"2014-07-22T11:48:38Z","receivedAt":"2014-07-22T11:48:38Z","isPatch":true,"sender":{"key":"tanayabh@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2097229?v=4"},"body":"\n\nOn 7/22/2014 5:14 PM, Matthieu Moy wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> will complain that we are passing a pointer to \"const char *\", not a\n>> pointer to \"char *\". And indeed, compiling with your patch introduces a\n>> ton of compiler warnings.\n> \n> Tanay: are you not compiling with gcc -Wall -Werror?\n> \n> (see my earlier message, just create a file config.mak containing\n> \n>   CFLAGS += -Wdeclaration-after-statement -Wall -Werror\n> \n> )\n>\n\nYes, I was. Dunno why it didn't work then.\n"},{"id":"246501","messageId":"xmqqoawhgzky.fsf@gitster.dls.corp.google.com","threadId":"37187","inReplyTo":"vpqsiltsjm7.fsf@anie.imag.fr","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T15:53:01Z","receivedAt":"2014-07-22T15:53:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> OK, it seems I got convinced too quickly by Junio ;-). The function\n> produces a char * that can be modified, but it also receives a value,\n> and the function should keep the \"const\" to allow passing \"const char\n> *\".\n\nDon't blame me. I never suggested to touch that existing function,\nwith existing call sites.\n"},{"id":"246502","messageId":"vpqsiltfkjo.fsf@anie.imag.fr","threadId":"37187","inReplyTo":"xmqqoawhgzky.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-22T16:03:07Z","receivedAt":"2014-07-22T16:03:07Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n>\n>> OK, it seems I got convinced too quickly by Junio ;-). The function\n>> produces a char * that can be modified, but it also receives a value,\n>> and the function should keep the \"const\" to allow passing \"const char\n>> *\".\n>\n> Don't blame me. I never suggested to touch that existing function,\n> with existing call sites.\n\nI don't understand what you mean. The new git_config_get_string()\nfunction is meant to be used in essentially every places where\ngit_config_string() is currently used, so removing the const from\ngit_config_get_string() raises the same issue as changing the existing\nfunction.\n\nDropping the const means we won't be able to write\n\nconst char *v = \"default\";\n...\ngit_config_get_string(&v, ...);\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246505","messageId":"xmqqk375gx1n.fsf@gitster.dls.corp.google.com","threadId":"37187","inReplyTo":"20140722110720.GA386@peff.net","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-22T16:47:48Z","receivedAt":"2014-07-22T16:47:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I would not mind lifting this unnecessary restriction on\n> git_config_string, but I do not see a way to do it without making the\n> rest of the code much uglier (and I do not see a particular advantage in\n> modifying git_config_string here that would make it worth the trouble).\n\nI do not think changing the existing one is warranted, either.\n\nGiven that C's type system does not allow us to pass \"const char *\"\nas \"char *\" without cast (and vice versa), a function that takes an\nout parameter that points at the location to store the pointer to a\nstring (instead of returning the pointer to a string as value) has\nto take either one of these forms:\n\n\tget_mutable(char **dest);\n        get_const(const char **dest);\n\nand half the callers need to pass their variables with a cast, i.e.\n\n        char *mutable_string;\n\tconst char *const_string;\n\n\tget_mutable((char **)&const_string);\n        get_mutable(&mutable_string);\n\tget_const(&const_string);\n\tget_const((const char **)&mutable_string);\n\nIf we have to cast some [*1*], I'd prefer to see the type describe\nwhat it really is, and I think a function that gives a newly\nallocated storage for the callers' own use is better described by\ntaking a pointer to non-const \"char *\" location.  So I'd encourage a\nnew function being introduced that does not have existing callers to\nuse that as the criterion to decide which to take.\n\nChanging the existing function with existing calling site needs two\nother pros-and-cons evaluation, in addition to the above \"does the\ntype describe what it really is?\", which are \"is it worth the\nchurn?\" and \"does the end result make more sense than the original?\"\n\n\n[Footnote]\n\n*1* We have safe_create_leading_directories_const() that works\naround this for input parameter around its _const less counterpart,\nwhich is ugly but livable solution.\n"},{"id":"246545","messageId":"vpqegxccvq9.fsf@anie.imag.fr","threadId":"37187","inReplyTo":"xmqqk375gx1n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-07-23T08:42:06Z","receivedAt":"2014-07-23T08:42:06Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> *1* We have safe_create_leading_directories_const() that works\n> around this for input parameter around its _const less counterpart,\n> which is ugly but livable solution.\n\nI think it would actually be a reasonable solution to avoid casting here\nand there on the caller side.\n\nAnother option would be to _return_ a non-const char * instead of\noutputing it as a by-address parameter. We'd lose the consiseness of\n\n   return git_config_get_string(...)\n\n(but in cases where the return value is ignored, we do not really care)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"246578","messageId":"xmqqr41cdm1s.fsf@gitster.dls.corp.google.com","threadId":"37187","inReplyTo":"vpqegxccvq9.fsf@anie.imag.fr","subject":"Re: [PATCH] config.c: change the function signature of `git_config_string()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-23T17:25:51Z","receivedAt":"2014-07-23T17:25:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> *1* We have safe_create_leading_directories_const() that works\n>> around this for input parameter around its _const less counterpart,\n>> which is ugly but livable solution.\n>\n> I think it would actually be a reasonable solution to avoid casting here\n> and there on the caller side.\n\n\"Ugly\" primarily refers to the fact that we are forced to do this in\nthe first place by the language.  I agree with you, especially if we\nhave very many call sites, and I suspect config-get-string actually\nwould.\n\n> Another option would be to _return_ a non-const char * instead of\n> outputing it as a by-address parameter.\n\nHere, too, I agree that it is the most C-ish interface.\n"}]}