{"thread":{"id":"61282","subject":"[PATCH 0/4] fix a leak with excludes_file","startedAt":"2024-04-06T14:29:09Z","lastAt":"2024-04-08T19:33:53Z","messageCount":9,"participants":["Rubén Justo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"492387","messageId":"f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com","threadId":"61282","inReplyTo":null,"subject":"[PATCH 0/4] fix a leak with excludes_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-06T14:29:04Z","receivedAt":"2024-04-06T14:29:09Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Having ...\n\t[core]\n        \texcludesFile = /some/global/path/.gitignore\n\n... this triggers a leak:\n\n\t$ GIT_EDITOR=: git rebase -i HEAD\n\tSuccessfully rebased and updated detached HEAD.\n\n\t=================================================================\n\t==...==ERROR: LeakSanitizer: detected memory leaks\n\n\tDirect leak of 60 byte(s) in 1 object(s) allocated from:\n\t    #0  ... in realloc \n\t    #1  ... in xrealloc wrapper.c:137\n\t    #2  ... in strbuf_grow strbuf.c:112\n\t    #3  ... in strbuf_add strbuf.c:311\n\t    #4  ... in strbuf_addstr strbuf.h:310\n\t    #5  ... in interpolate_path path.c:771\n\t    #6  ... in git_config_pathname config.c:1352\n\t    #7  ... in git_default_core_config config.c:1588\n\t    #8  ... in git_default_config config.c:1791\n\t    #9  ... in rebase_config builtin/rebase.c:801\n\t    #10 ... in configset_iter config.c:2161\n\t    #11 ... in repo_config config.c:2540\n\t    #12 ... in git_config config.c:2663\n\t    #13 ... in cmd_rebase builtin/rebase.c:1187\n\t    #14 ... in run_builtin git.c:469\n\t    #15 ... in handle_builtin git.c:724\n\t    #16 ... in run_argv git.c:788\n\t    #17 ... in cmd_main git.c:923\n\t    #18 ... in main common-main.c:62\n\nIt happens because we parse twice the configuration: \n\n\t$ GIT_EDITOR=: gdb --ex \"break git_config\" --ex \"run\" --args git rebase -i HEAD\n\t(gdb) bt\n\t#0  git_config () at config.c:2663\n\t#1  ... in cmd_rebase () at builtin/rebase.c:1187\n\t#2  ... in run_builtin () at git.c:469\n\t#3  ... in handle_builtin () at git.c:724\n\t#4  ... in run_argv () at git.c:788\n\t#5  ... in cmd_main () at git.c:923\n\t#6  ... in main () at common-main.c:62\n\t(gdb) c\n\t(gdb) bt\n\t#0  git_config () at config.c:2663\n\t#1  ... in sequencer_init_config () at sequencer.c:291\n\t#2  ... in get_replay_opts () at builtin/rebase.c:161\n\t#3  ... in do_interactive_rebase () at builtin/rebase.c:271\n\t#4  ... in run_sequencer_rebase () at builtin/rebase.c:339\n\t#5  ... in run_specific_rebase () at builtin/rebase.c:705\n\t#6  ... in cmd_rebase () at builtin/rebase.c:1830\n\t#7  ... in run_builtin () at git.c:469\n\t#8  ... in handle_builtin () at git.c:724\n\t#9  ... in run_argv () at git.c:788\n\t#10 ... in cmd_main () at git.c:923\n\t#11 ... in main () at common-main.c:62\n\nWe call twice to git_config(): first to get the main git-branch(1) options,\nand second to get the ones related to the sequencer.\n\nDue to how git_config() works the global configuration is parsed twice,\ntherefore if core.excludesFile is set, it will be allocated twice.\n\nA free() before the git_config_pathname() can be a simpler fix, but I\nthink this series offers a better approach, perhaps applicable to other\npotential similar leaks.\n\nRubén Justo (4):\n  path.c: introduce strbuf_interpolate_path\n  config.c: introduce git_config_strbuf_pathname\n  environment.c: convert excludes_file to struct strbuf\n  t7300: mark as leak-free\n\n config.c         | 12 +++++++++++-\n config.h         |  2 ++\n dir.c            | 13 +++++++++----\n environment.c    |  2 +-\n environment.h    |  2 +-\n path.c           | 20 ++++++++++++++------\n path.h           |  1 +\n t/t7300-clean.sh |  1 +\n 8 files changed, 40 insertions(+), 13 deletions(-)\n\n-- \n2.44.0.697.g9b33b46f29\n\n"},{"id":"492388","messageId":"eb40f012-e910-49a3-9e3f-2dbab2b169a6@gmail.com","threadId":"61282","inReplyTo":"f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com","subject":"[PATCH 1/4] path.c: introduce strbuf_interpolate_path","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-06T14:31:56Z","receivedAt":"2024-04-06T14:32:01Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Factorize interpolate_path to have a similar function that uses a\nstrbuf, instead of allocating a new string, to return the interpolated\npath.\n\nIt will allow us to avoid some allocs and also some frees, which we will\ntake advantage of in the next commits.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n path.c | 20 ++++++++++++++------\n path.h |  1 +\n 2 files changed, 15 insertions(+), 6 deletions(-)\n\ndiff --git a/path.c b/path.c\nindex 8bb223c92c..7a1a1a9bc0 100644\n--- a/path.c\n+++ b/path.c\n@@ -737,8 +737,16 @@ static struct passwd *getpw_str(const char *username, size_t len)\n char *interpolate_path(const char *path, int real_home)\n {\n \tstruct strbuf user_path = STRBUF_INIT;\n+\n+\treturn strbuf_interpolate_path(path, real_home, &user_path);\n+}\n+\n+char *strbuf_interpolate_path(const char *path, int real_home, struct strbuf* dst)\n+{\n \tconst char *to_copy = path;\n \n+\tstrbuf_reset(dst);\n+\n \tif (!path)\n \t\tgoto return_null;\n \n@@ -754,9 +762,9 @@ char *interpolate_path(const char *path, int real_home)\n \t\t\tif (!home)\n \t\t\t\tgoto return_null;\n \t\t\tif (real_home)\n-\t\t\t\tstrbuf_add_real_path(&user_path, home);\n+\t\t\t\tstrbuf_add_real_path(dst, home);\n \t\t\telse\n-\t\t\t\tstrbuf_addstr(&user_path, home);\n+\t\t\t\tstrbuf_addstr(dst, home);\n #ifdef GIT_WINDOWS_NATIVE\n \t\t\tconvert_slashes(user_path.buf);\n #endif\n@@ -764,14 +772,14 @@ char *interpolate_path(const char *path, int real_home)\n \t\t\tstruct passwd *pw = getpw_str(username, username_len);\n \t\t\tif (!pw)\n \t\t\t\tgoto return_null;\n-\t\t\tstrbuf_addstr(&user_path, pw->pw_dir);\n+\t\t\tstrbuf_addstr(dst, pw->pw_dir);\n \t\t}\n \t\tto_copy = first_slash;\n \t}\n-\tstrbuf_addstr(&user_path, to_copy);\n-\treturn strbuf_detach(&user_path, NULL);\n+\tstrbuf_addstr(dst, to_copy);\n+\treturn dst->buf;\n return_null:\n-\tstrbuf_release(&user_path);\n+\tstrbuf_release(dst);\n \treturn NULL;\n }\n \ndiff --git a/path.h b/path.h\nindex e053effef2..da7e5384a3 100644\n--- a/path.h\n+++ b/path.h\n@@ -185,6 +185,7 @@ int calc_shared_perm(int mode);\n int adjust_shared_perm(const char *path);\n \n char *interpolate_path(const char *path, int real_home);\n+char *strbuf_interpolate_path(const char *path, int real_home, struct strbuf *dst);\n const char *enter_repo(const char *path, int strict);\n const char *remove_leading_path(const char *in, const char *prefix);\n const char *relative_path(const char *in, const char *prefix, struct strbuf *sb);\n-- \n2.44.0.697.g9b33b46f29\n"},{"id":"492389","messageId":"f58d8114-f6fe-4b4e-85be-3b57609a3687@gmail.com","threadId":"61282","inReplyTo":"f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com","subject":"[PATCH 2/4] config.c: introduce git_config_strbuf_pathname","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-06T14:32:17Z","receivedAt":"2024-04-06T14:32:32Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Add a new git_config_strbuf_pathname function, similar to\ngit_config_pathname, that works with a strbuf relying on the recently\nintroduced strbuf_interpolate_path.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n config.c | 10 ++++++++++\n config.h |  2 ++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex eebce8c7e0..9beeb63b50 100644\n--- a/config.c\n+++ b/config.c\n@@ -1355,6 +1355,16 @@ int git_config_pathname(const char **dest, const char *var, const char *value)\n \treturn 0;\n }\n \n+int git_config_strbuf_pathname(struct strbuf *dest, const char *var, const char *value)\n+{\n+\tif (!value)\n+\t\treturn config_error_nonbool(var);\n+\tstrbuf_interpolate_path(value, 0, dest);\n+\tif (!dest->len)\n+\t\tdie(_(\"failed to expand user dir in: '%s'\"), value);\n+\treturn 0;\n+}\n+\n int git_config_expiry_date(timestamp_t *timestamp, const char *var, const char *value)\n {\n \tif (!value)\ndiff --git a/config.h b/config.h\nindex f4966e3749..e405f1c140 100644\n--- a/config.h\n+++ b/config.h\n@@ -22,6 +22,7 @@\n  */\n \n struct object_id;\n+struct strbuf;\n \n /* git_config_parse_key() returns these negated: */\n #define CONFIG_INVALID_KEY 1\n@@ -287,6 +288,7 @@ int git_config_string(const char **, const char *, const char *);\n  * user's home directory when found at the beginning of the path.\n  */\n int git_config_pathname(const char **, const char *, const char *);\n+int git_config_strbuf_pathname(struct strbuf *, const char *, const char *);\n \n int git_config_expiry_date(timestamp_t *, const char *, const char *);\n int git_config_color(char *, const char *, const char *);\n-- \n2.44.0.697.g9b33b46f29\n"},{"id":"492390","messageId":"657f1780-c813-459d-aecc-94a10f7a3b4f@gmail.com","threadId":"61282","inReplyTo":"f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com","subject":"[PATCH 3/4] environment.c: convert excludes_file to struct strbuf","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-06T14:32:39Z","receivedAt":"2024-04-06T14:32:43Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Make excludes_file a strbuf, so that we don't have to worry about freeing\nit if it is set multiple times.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n config.c      |  2 +-\n dir.c         | 13 +++++++++----\n environment.c |  2 +-\n environment.h |  2 +-\n 4 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 9beeb63b50..bc64af8efa 100644\n--- a/config.c\n+++ b/config.c\n@@ -1595,7 +1595,7 @@ static int git_default_core_config(const char *var, const char *value,\n \t\treturn git_config_string(&askpass_program, var, value);\n \n \tif (!strcmp(var, \"core.excludesfile\"))\n-\t\treturn git_config_pathname(&excludes_file, var, value);\n+\t\treturn git_config_strbuf_pathname(&excludes_file, var, value);\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\ndiff --git a/dir.c b/dir.c\nindex 20ebe4cba2..e31ccd8e48 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3386,10 +3386,15 @@ void setup_standard_excludes(struct dir_struct *dir)\n \tdir->exclude_per_dir = \".gitignore\";\n \n \t/* core.excludesfile defaulting to $XDG_CONFIG_HOME/git/ignore */\n-\tif (!excludes_file)\n-\t\texcludes_file = xdg_config_home(\"ignore\");\n-\tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n-\t\tadd_patterns_from_file_1(dir, excludes_file,\n+\tif (!excludes_file.len) {\n+\t\tchar *str = xdg_config_home(\"ignore\");\n+\t\tif (str) {\n+\t\t\tstrbuf_addstr(&excludes_file, str);\n+\t\t\tfree(str);\n+\t\t}\n+\t}\n+\tif (excludes_file.len && !access_or_warn(excludes_file.buf, R_OK, 0))\n+\t\tadd_patterns_from_file_1(dir, excludes_file.buf,\n \t\t\t\t\t dir->untracked ? &dir->internal.ss_excludes_file : NULL);\n \n \t/* per repository user preference */\ndiff --git a/environment.c b/environment.c\nindex a73ba9c12c..b4c66e7153 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -60,7 +60,7 @@ size_t delta_base_cache_limit = 96 * 1024 * 1024;\n unsigned long big_file_threshold = 512 * 1024 * 1024;\n const char *editor_program;\n const char *askpass_program;\n-const char *excludes_file;\n+struct strbuf excludes_file = STRBUF_INIT;\n enum auto_crlf auto_crlf = AUTO_CRLF_FALSE;\n enum eol core_eol = EOL_UNSET;\n int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;\ndiff --git a/environment.h b/environment.h\nindex 05fd94d7be..114e3dde99 100644\n--- a/environment.h\n+++ b/environment.h\n@@ -222,7 +222,7 @@ extern const char *git_log_output_encoding;\n \n extern const char *editor_program;\n extern const char *askpass_program;\n-extern const char *excludes_file;\n+extern struct strbuf excludes_file;\n \n /*\n  * Should we print an ellipsis after an abbreviated SHA-1 value\n-- \n2.44.0.697.g9b33b46f29\n"},{"id":"492391","messageId":"7f759a5a-ae25-4f2b-a38e-4d0d786ca3bc@gmail.com","threadId":"61282","inReplyTo":"f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com","subject":"[PATCH 4/4] t7300: mark as leak-free","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-06T14:32:54Z","receivedAt":"2024-04-06T14:32:59Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since the previous commit this test does not leak.\n\nMark it with TEST_PASSES_SANITIZE_LEAK=true so to make the leak\ncheckers happy.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n t/t7300-clean.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7300-clean.sh b/t/t7300-clean.sh\nindex 1f7201eb60..0aae0dee67 100755\n--- a/t/t7300-clean.sh\n+++ b/t/t7300-clean.sh\n@@ -5,6 +5,7 @@\n \n test_description='git clean basic tests'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n git config clean.requireForce no\n-- \n2.44.0.697.g9b33b46f29\n"},{"id":"492402","messageId":"xmqq7chajs3e.fsf@gitster.g","threadId":"61282","inReplyTo":"f4af1e88-5bd9-4b3c-8691-84dbf0ca3ee2@gmail.com","subject":"Re: [PATCH 0/4] fix a leak with excludes_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-06T17:53:09Z","receivedAt":"2024-04-06T17:53:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> We call twice to git_config(): first to get the main git-branch(1) options,\n> and second to get the ones related to the sequencer.\n\nThe diagnosis of the control flow that leads to overwriting the\nvalue assigned earlier to \"excludes_file\" is quite correct, but\nI find that your fix is somehow not addressing the real issue.\n\nWe used to assume that excludes_file *always* points at a piece of\nmemory that somebody else owns.  But seeing the two places that\nassign to the variable, both actually give the variable a piece of\nmemory allocated for the caller.\n\n - The one in config.c that reads core.excludesfile via\n   git_config_pathname(), which calls interpolate_path(), whose\n   return value is always allocated.\n\n - The one in dir.c that sets up standard excludes uses\n   xdg_config_home() that returns a value obtained from mkpathdup().\n\nThe core of the fix ought to be just like this, but to lose the cast\nthat discards the constness away, the fallouts will be large (if you\nremove \"const\" from the first parameter to git_config_pathname(),\nthe compiler will tell you where the fallouts are).\n\nWe may want to eventually fix the type of the first parameter of the\ngit_config_pathmame() function, but I somehow doubt that your\napproach gets us any closer to that future.  Adding a parallel API\nnext to the existing git_config_pathname() and interpolate_path()\nand convert only these callers that touch excludes_file would not\nhelp other callers that hold the pointer git_config_pathname()\nreturned.\n\n config.c         | 4 +++-\n t/t7300-clean.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git c/config.c w/config.c\nindex eebce8c7e0..ae3652b08f 100644\n--- c/config.c\n+++ w/config.c\n@@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value,\n \tif (!strcmp(var, \"core.askpass\"))\n \t\treturn git_config_string(&askpass_program, var, value);\n \n-\tif (!strcmp(var, \"core.excludesfile\"))\n+\tif (!strcmp(var, \"core.excludesfile\")) {\n+\t\tfree((char *)excludes_file);\n \t\treturn git_config_pathname(&excludes_file, var, value);\n+\t}\n \n \tif (!strcmp(var, \"core.whitespace\")) {\n \t\tif (!value)\ndiff --git c/t/t7300-clean.sh w/t/t7300-clean.sh\nindex 1f7201eb60..0aae0dee67 100755\n--- c/t/t7300-clean.sh\n+++ w/t/t7300-clean.sh\n@@ -5,6 +5,7 @@\n \n test_description='git clean basic tests'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n git config clean.requireForce no\n"},{"id":"492470","messageId":"1323321b-554e-4b16-a814-03710c50db99@gmail.com","threadId":"61282","inReplyTo":"xmqq7chajs3e.fsf@gitster.g","subject":"Re: [PATCH 0/4] fix a leak with excludes_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-07T17:48:48Z","receivedAt":"2024-04-07T17:48:55Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Sat, Apr 06, 2024 at 10:53:09AM -0700, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > We call twice to git_config(): first to get the main git-branch(1) options,\n> > and second to get the ones related to the sequencer.\n\nObviously I meant git-rebase(1) -- or any other command that uses the\nsequencer under the hood.\n\n> Adding a parallel API\n> next to the existing git_config_pathname() and interpolate_path()\n> and convert only these callers that touch excludes_file would not\n> help other callers that hold the pointer git_config_pathname()\n> returned.\n\nIt does not have to be like that.  We may no longer need the current\nand problematic git_config_pathname().  However I did not want to go\nthat far in this series.\n\n>  config.c         | 4 +++-\n>  t/t7300-clean.sh | 1 +\n>  2 files changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git c/config.c w/config.c\n> index eebce8c7e0..ae3652b08f 100644\n> --- c/config.c\n> +++ w/config.c\n> @@ -1584,8 +1584,10 @@ static int git_default_core_config(const char *var, const char *value,\n>  \tif (!strcmp(var, \"core.askpass\"))\n>  \t\treturn git_config_string(&askpass_program, var, value);\n>  \n> -\tif (!strcmp(var, \"core.excludesfile\"))\n> +\tif (!strcmp(var, \"core.excludesfile\")) {\n> +\t\tfree((char *)excludes_file);\n\nAaah, you prefer this :-( ...  this free is what I was referring to, in\nthe message you are replying to, as the simpler fix ...\n\nIt obviously plugs the leak and so my itch is gone;  therefore OK by me\nto the series I have seen that you have already sent with this.\n\nStill, I find the approach in this series more interesting than the\nsimple free.  The strbuf saves us from subsequent free+alloc's, for\nexactly the same \"core.excludesFile\".  The strbuf states clearly, IMHO,\nwho owns the pointer.  And other niceties.\n"},{"id":"492569","messageId":"xmqqy19nbvtt.fsf@gitster.g","threadId":"61282","inReplyTo":"1323321b-554e-4b16-a814-03710c50db99@gmail.com","subject":"Re: [PATCH 0/4] fix a leak with excludes_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T17:36:30Z","receivedAt":"2024-04-08T17:36:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> It does not have to be like that.  We may no longer need the current\n> and problematic git_config_pathname().  However I did not want to go\n> that far in this series.\n\nTrue, but that one and only true interface we will end up with MUST\nNOT be strbuf based one, and that is why I said the patch as posted\nwill not take us into a better future.\n\nAnd as a stop-gap measure, ...\n\n>> -\tif (!strcmp(var, \"core.excludesfile\"))\n>> +\tif (!strcmp(var, \"core.excludesfile\")) {\n>> +\t\tfree((char *)excludes_file);\n>\n> Aaah, you prefer this :-( ...  this free is what I was referring to, in\n> the message you are replying to, as the simpler fix ...\n\n... obviously this would be much more preferrable.  And when the one\nand only true interface comes, we only need to drop the cast from\naround here.\n\nThanks.\n"},{"id":"492578","messageId":"89a2db74-2bb6-4c62-ad8a-bc4fe5fe49fa@gmail.com","threadId":"61282","inReplyTo":"xmqqy19nbvtt.fsf@gitster.g","subject":"Re: [PATCH 0/4] fix a leak with excludes_file","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-04-08T19:33:50Z","receivedAt":"2024-04-08T19:33:53Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Apr 08, 2024 at 10:36:30AM -0700, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > It does not have to be like that.  We may no longer need the current\n> > and problematic git_config_pathname().  However I did not want to go\n> > that far in this series.\n> \n> True, but that one and only true interface we will end up with MUST\n> NOT be strbuf based one, and that is why I said the patch as posted\n> will not take us into a better future.\n\nIt doesn't have to be having a strbuf-based-only interface, either.\n\nIt is about introducing an interface where callers:\n\n\t- can take advantage of a not-always-allocate interface, and\n\n\t- can stop worrying about freeing previous values, when reusing\n\t  variables.\n\nThis latter fixes the leak while the former introduces a nice \"if you\nset n-times the same value, we'll allocate once\".\n"}]}