{"thread":{"id":"41889","subject":"[PATCHv3 0/4] Some cleanups","startedAt":"2016-03-31T18:04:02Z","lastAt":"2016-03-31T23:32:52Z","messageCount":13,"participants":["Stefan Beller","Jeff King","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282357","messageId":"1459447446-32260-1-git-send-email-sbeller@google.com","threadId":"41889","inReplyTo":null,"subject":"[PATCHv3 0/4] Some cleanups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T18:04:02Z","receivedAt":"2016-03-31T18:04:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v3:\nThanks Eric, Jeff, Junio for discussion!\n* use git_config_get_value instead of git_config_get_string in patch 1\n* Improve commit message to explain why strbuf_list_free frees more memory\n  (hence is the right thing to do)\n* the bundle code doesn't have a dedicated return variable,\n  but the error path always returns -1  \n* removed a duplicate of\n        +\tif (!bundle_to_stdout)\n        +\t\tclose(bundle_fd);\n  in the bundle patch.\n* This applies on v2.8.0.\n\nv2:\nThanks Eric, Jeff, Junio for discussion, I picked up all hints and here\nis a v2.\n\n* drop the overallocation patches (1&2)\n* use git_config_get_string instead of its _const equivalent, such that\n  we don't need a cast when freeing in git_config_get_notes_strategy\n* Use strbuf_list_free instead of cooking our own.\n* have a dedicated error exit path in bundle.c, create_bundle\n\nv1:\nOne of my first patches to Git were cleanup patches, and I fell back\nto my old pattern here, while thinking on how to write better commit\nmessages for the submodule bugfixes I currently have in flight.\n\nJust some one liners to not leak memory or file descriptors.\n\nThey are bundled as a series, but no patch relies on any predessor.\n\nThis applies on v2.8.0.\n\nThanks,\nStefan\n\nStefan Beller (4):\n  notes: don't leak memory in git_config_get_notes_strategy\n  abbrev_sha1_in_line: don't leak memory\n  bundle: don't leak an fd in case of early return\n  credential-cache, send_request: close fd when done\n\n builtin/notes.c    |  2 +-\n bundle.c           | 17 ++++++++++++-----\n credential-cache.c |  1 +\n wt-status.c        |  4 +---\n 4 files changed, 15 insertions(+), 9 deletions(-)\n\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282358","messageId":"1459447446-32260-2-git-send-email-sbeller@google.com","threadId":"41889","inReplyTo":"1459447446-32260-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T18:04:03Z","receivedAt":"2016-03-31T18:04:03Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`value` is just a temporary scratchpad, so we need to make sure it doesn't\nleak. It is xstrdup'd in `git_config_get_string_const` and\n`parse_notes_merge_strategy` just compares the string against predefined\nvalues, so no need to keep it around longer. Instead of using\n`git_config_get_string_const`, use `git_config_get_value`, which doesn't\nreturn a copy.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/notes.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex ed6f222..fa21f1a 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,\n {\n \tconst char *value;\n \n-\tif (git_config_get_string_const(key, &value))\n+\tif (git_config_get_value(key, &value))\n \t\treturn 1;\n \tif (parse_notes_merge_strategy(value, strategy))\n \t\tgit_die_config(key, \"unknown notes merge strategy %s\", value);\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282360","messageId":"1459447446-32260-3-git-send-email-sbeller@google.com","threadId":"41889","inReplyTo":"1459447446-32260-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 2/4] abbrev_sha1_in_line: don't leak memory","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T18:04:04Z","receivedAt":"2016-03-31T18:04:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"`split` is of type `struct strbuf **`, and currently we are leaking split\nitself as well as each element in split[i]. We have a dedicated free\nfunction for `struct strbuf **`, which takes care of freeing all\nrelated memory.\n\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nNotes:\n    Feel free to drop this patch and use the\n    the proposal, which drops `**split` entirely\n    http://thread.gmane.org/gmane.comp.version-control.git/290232/focus=290318\n\n wt-status.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ef74864..1ea2ebe 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)\n \t\t\t\tstrbuf_addf(line, \"%s\", split[i]->buf);\n \t\t}\n \t}\n-\tfor (i = 0; split[i]; i++)\n-\t\tstrbuf_release(split[i]);\n-\n+\tstrbuf_list_free(split);\n }\n \n static void read_rebase_todolist(const char *fname, struct string_list *lines)\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282361","messageId":"1459447446-32260-4-git-send-email-sbeller@google.com","threadId":"41889","inReplyTo":"1459447446-32260-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 3/4] bundle: don't leak an fd in case of early return","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T18:04:05Z","receivedAt":"2016-03-31T18:04:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In successful operation `write_pack_data` will close the `bundle_fd`,\nbut when we exit early, we need to take care of the file descriptor\nas well as the lock file ourselves. The lock file may be deleted at the\nend of running the program, but we are in library code, so we should\nnot rely on that.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n bundle.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 506ac49..31ae1da 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const char *path,\n \n \t/* write prerequisites */\n \tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n-\t\treturn -1;\n+\t\tgoto err;\n \n \targc = setup_revisions(argc, argv, &revs, NULL);\n \n-\tif (argc > 1)\n-\t\treturn error(_(\"unrecognized argument: %s\"), argv[1]);\n+\tif (argc > 1) {\n+\t\terror(_(\"unrecognized argument: %s\"), argv[1]);\n+\t\tgoto err;\n+\t}\n \n \tobject_array_remove_duplicates(&revs.pending);\n \n@@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tif (!ref_count)\n \t\tdie(_(\"Refusing to create empty bundle.\"));\n \telse if (ref_count < 0)\n-\t\treturn -1;\n+\t\tgoto err;\n \n \t/* write pack */\n \tif (write_pack_data(bundle_fd, &revs))\n-\t\treturn -1;\n+\t\tgoto err;\n \n \tif (!bundle_to_stdout) {\n \t\tif (commit_lock_file(&lock))\n \t\t\tdie_errno(_(\"cannot create '%s'\"), path);\n \t}\n \treturn 0;\n+err:\n+\tif (!bundle_to_stdout)\n+\t\tclose(bundle_fd);\n+\trollback_lock_file(&lock);\n+\treturn -1;\n }\n \n int unbundle(struct bundle_header *header, int bundle_fd, int flags)\n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282359","messageId":"1459447446-32260-5-git-send-email-sbeller@google.com","threadId":"41889","inReplyTo":"1459447446-32260-1-git-send-email-sbeller@google.com","subject":"[PATCHv3 4/4] credential-cache, send_request: close fd when done","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-31T18:04:06Z","receivedAt":"2016-03-31T18:04:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"No need to keep it open any further.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n credential-cache.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex f4afdc6..86e21de 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct strbuf *out)\n \t\twrite_or_die(1, in, r);\n \t\tgot_data = 1;\n \t}\n+\tclose(fd);\n \treturn got_data;\n }\n \n-- \n2.5.0.264.g4004fdc.dirty\n"},{"id":"282372","messageId":"20160331191151.GA5013@sigill.intra.peff.net","threadId":"41889","inReplyTo":"1459447446-32260-4-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 3/4] bundle: don't leak an fd in case of early return","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T19:11:52Z","receivedAt":"2016-03-31T19:11:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2016 at 11:04:05AM -0700, Stefan Beller wrote:\n\n> diff --git a/bundle.c b/bundle.c\n> index 506ac49..31ae1da 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \n>  \t/* write prerequisites */\n>  \tif (compute_and_write_prerequisites(bundle_fd, &revs, argc, argv))\n> -\t\treturn -1;\n> +\t\tgoto err;\n>  \n>  \targc = setup_revisions(argc, argv, &revs, NULL);\n>  \n> -\tif (argc > 1)\n> -\t\treturn error(_(\"unrecognized argument: %s\"), argv[1]);\n> +\tif (argc > 1) {\n> +\t\terror(_(\"unrecognized argument: %s\"), argv[1]);\n> +\t\tgoto err;\n> +\t}\n>  \n>  \tobject_array_remove_duplicates(&revs.pending);\n>  \n> @@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \tif (!ref_count)\n>  \t\tdie(_(\"Refusing to create empty bundle.\"));\n>  \telse if (ref_count < 0)\n> -\t\treturn -1;\n> +\t\tgoto err;\n>  \n>  \t/* write pack */\n>  \tif (write_pack_data(bundle_fd, &revs))\n> -\t\treturn -1;\n> +\t\tgoto err;\n\nSorry for not noticing this before, but if we assume write_pack_data\nalways closes bundle_fd, even on error (and I think it does), then the\nclose() in the \"err\" code path is redundant from this goto, right?\n\nI guess it is harmless, as nobody could have opened a new descriptor in\nthe interim, so our close(bundle_fd) will just get EBADF. But I guess we\ncould also do:\n\n  if (write_pack_data(bundle_fd, &revs)) {\n\tbundle_fd = -1;\n\tgoto err;\n  }\n\nor even:\n\n  ret = write_pack_data(bundle_fd, &revs);\n  bundle_fd = -1; /* closed by write_pack_data */\n  if (ret)\n\tgoto err;\n\nto ensure that nobody (including the non-error code paths) uses it\nagain.\n\nLike I said, I don't think it's buggy in the current code, but it does\nseem a little fragile.\n\n> +err:\n> +\tif (!bundle_to_stdout)\n> +\t\tclose(bundle_fd);\n> +\trollback_lock_file(&lock);\n> +\treturn -1;\n\nSimilarly, I think the rollback_lock_file() call is redundant if\nbundle_to_stdout is set (because we don't have created a lockfile in the\nfirst place). I think this is more explicitly OK, because the lockfile\nkeeps an \"am I initialized\" flag, but perhaps sticking inside the \"if\n(!bundle_to_stdout)\" condition makes it more clear that it's not an\nerror (especially because the \"commit_lock_file\" call above is inside\none).\n\n-Peff\n"},{"id":"282373","messageId":"20160331191252.GB5013@sigill.intra.peff.net","threadId":"41889","inReplyTo":"1459447446-32260-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 0/4] Some cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T19:12:52Z","receivedAt":"2016-03-31T19:12:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2016 at 11:04:02AM -0700, Stefan Beller wrote:\n\n> v3:\n> Thanks Eric, Jeff, Junio for discussion!\n> * use git_config_get_value instead of git_config_get_string in patch 1\n> * Improve commit message to explain why strbuf_list_free frees more memory\n>   (hence is the right thing to do)\n> * the bundle code doesn't have a dedicated return variable,\n>   but the error path always returns -1  \n> * removed a duplicate of\n>         +\tif (!bundle_to_stdout)\n>         +\t\tclose(bundle_fd);\n>   in the bundle patch.\n> * This applies on v2.8.0.\n\nWith the exception of the comments on patch 3, these all look good. I'll\nleave it to Junio to decide whether he wants to polish up his \"get rid\nof strbuf_split\" patch for patch 2. Certainly yours is a strict\nimprovement over what is there.\n\n-Peff\n"},{"id":"282377","messageId":"xmqqio02mokp.fsf@gitster.mtv.corp.google.com","threadId":"41889","inReplyTo":"20160331191252.GB5013@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/4] Some cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T19:31:34Z","receivedAt":"2016-03-31T19:31:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> With the exception of the comments on patch 3, these all look good. I'll\n> leave it to Junio to decide whether he wants to polish up his \"get rid\n> of strbuf_split\" patch for patch 2. Certainly yours is a strict\n> improvement over what is there.\n\nI do not think there were anything further to be polished up.  Other\nthan that, I agree with all of the above.\n\nThanks for reviewing.\n"},{"id":"282379","messageId":"20160331193333.GD5013@sigill.intra.peff.net","threadId":"41889","inReplyTo":"xmqqio02mokp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 0/4] Some cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T19:33:33Z","receivedAt":"2016-03-31T19:33:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2016 at 12:31:34PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > With the exception of the comments on patch 3, these all look good. I'll\n> > leave it to Junio to decide whether he wants to polish up his \"get rid\n> > of strbuf_split\" patch for patch 2. Certainly yours is a strict\n> > improvement over what is there.\n> \n> I do not think there were anything further to be polished up.  Other\n> than that, I agree with all of the above.\n\nWell, by polish up, I meant \"write a commit message for\". :)\n\nThe patch itself looked fine to me.\n\n-Peff\n"},{"id":"282383","messageId":"xmqq60w2mmqk.fsf@gitster.mtv.corp.google.com","threadId":"41889","inReplyTo":"20160331193333.GD5013@sigill.intra.peff.net","subject":"Re: [PATCHv3 0/4] Some cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T20:11:15Z","receivedAt":"2016-03-31T20:11:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Well, by polish up, I meant \"write a commit message for\". :)\n>\n> The patch itself looked fine to me.\n\nI'll throw it in my \"leftover bits\" list.  After queuing 2/4, it now\nneeds a trivial adjustment to the patch text, which would be a good\nspice for a new contributor who is looking for something a bit more\nthan \"somebody did all the work and I can just resend it\".\n"},{"id":"282396","messageId":"CAPig+cRPNt1aNdsONXgX0SkgNiYtTS8vCGQzE2u4+vpd-N-Vew@mail.gmail.com","threadId":"41889","inReplyTo":"1459447446-32260-2-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-31T21:08:30Z","receivedAt":"2016-03-31T21:08:30Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> wrote:\n> `value` is just a temporary scratchpad, so we need to make sure it doesn't\n> leak. It is xstrdup'd in `git_config_get_string_const` and\n> `parse_notes_merge_strategy` just compares the string against predefined\n> values, so no need to keep it around longer. Instead of using\n> `git_config_get_string_const`, use `git_config_get_value`, which doesn't\n> return a copy.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,\n>  {\n>         const char *value;\n>\n> -       if (git_config_get_string_const(key, &value))\n> +       if (git_config_get_value(key, &value))\n\nHmm, doesn't this introduce a rather severe regression? Unless I'm\nmisreading the code (possible), with the original, if 'key' was\nboolean (lacked a value in the config file), then it would complain:\n\n    Missing value for 'floop.blork'\n\nbut, with this change, it will dereference NULL and crash.\n\n(My understanding was that Peff's suggestion to use\ngit_config_get_value() implied a bit of work beyond the simple textual\nsubstitution of 'git_config_get_value' for\n'git_config_get_string_const'.)\n\n>                 return 1;\n>         if (parse_notes_merge_strategy(value, strategy))\n>                 git_die_config(key, \"unknown notes merge strategy %s\", value);\n"},{"id":"282406","messageId":"xmqq7fgil0lu.fsf@gitster.mtv.corp.google.com","threadId":"41889","inReplyTo":"CAPig+cRPNt1aNdsONXgX0SkgNiYtTS8vCGQzE2u4+vpd-N-Vew@mail.gmail.com","subject":"Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-31T22:54:37Z","receivedAt":"2016-03-31T22:54:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> wrote:\n>> `value` is just a temporary scratchpad, so we need to make sure it doesn't\n>> leak. It is xstrdup'd in `git_config_get_string_const` and\n>> `parse_notes_merge_strategy` just compares the string against predefined\n>> values, so no need to keep it around longer. Instead of using\n>> `git_config_get_string_const`, use `git_config_get_value`, which doesn't\n>> return a copy.\n>>\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>> diff --git a/builtin/notes.c b/builtin/notes.c\n>> @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,\n>>  {\n>>         const char *value;\n>>\n>> -       if (git_config_get_string_const(key, &value))\n>> +       if (git_config_get_value(key, &value))\n>\n> Hmm, doesn't this introduce a rather severe regression? Unless I'm\n> misreading the code (possible), with the original, if 'key' was\n> boolean (lacked a value in the config file), then it would complain:\n>\n>     Missing value for 'floop.blork'\n>\n> but, with this change, it will dereference NULL and crash.\n>\n> (My understanding was that Peff's suggestion to use\n> git_config_get_value() implied a bit of work beyond the simple textual\n> substitution of 'git_config_get_value' for\n> 'git_config_get_string_const'.)\n\nYup, thanks for spelling it out.\n"},{"id":"282412","messageId":"20160331233252.GB6120@sigill.intra.peff.net","threadId":"41889","inReplyTo":"CAPig+cRPNt1aNdsONXgX0SkgNiYtTS8vCGQzE2u4+vpd-N-Vew@mail.gmail.com","subject":"Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-31T23:32:52Z","receivedAt":"2016-03-31T23:32:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 31, 2016 at 05:08:30PM -0400, Eric Sunshine wrote:\n\n> On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller <sbeller@google.com> wrote:\n> > `value` is just a temporary scratchpad, so we need to make sure it doesn't\n> > leak. It is xstrdup'd in `git_config_get_string_const` and\n> > `parse_notes_merge_strategy` just compares the string against predefined\n> > values, so no need to keep it around longer. Instead of using\n> > `git_config_get_string_const`, use `git_config_get_value`, which doesn't\n> > return a copy.\n> >\n> > Signed-off-by: Stefan Beller <sbeller@google.com>\n> > ---\n> > diff --git a/builtin/notes.c b/builtin/notes.c\n> > @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,\n> >  {\n> >         const char *value;\n> >\n> > -       if (git_config_get_string_const(key, &value))\n> > +       if (git_config_get_value(key, &value))\n> \n> Hmm, doesn't this introduce a rather severe regression? Unless I'm\n> misreading the code (possible), with the original, if 'key' was\n> boolean (lacked a value in the config file), then it would complain:\n> \n>     Missing value for 'floop.blork'\n> \n> but, with this change, it will dereference NULL and crash.\n> \n> (My understanding was that Peff's suggestion to use\n> git_config_get_value() implied a bit of work beyond the simple textual\n> substitution of 'git_config_get_value' for\n> 'git_config_get_string_const'.)\n\nAh, yeah, I didn't even think about that case. I was thinking that\nwouldn't it be nice if we had:\n\n  const char *git_config_get_string(const char *key);\n\nwhich would be a much more natural interface. But the reason we don't is\nthat we have to represent the \"NULL as boolean true\" case in the first\nplace.\n\nSo I dunno. Getting the NULL-handling for free is rather nice, and maybe\nworth using the normal git_config_get_string(). It's too bad there's not\na variant that just returns a non-allocated pointer, but given that\nthere is already a confusing proliferation of functions to retrieve a\nconfig string, it's hard to justify adding another.\n\n-Peff\n"}]}