{"thread":{"id":"64829","subject":"[PATCH 0/4] memory leaks in remote.c","startedAt":"2026-01-19T05:18:59Z","lastAt":"2026-01-20T20:18:10Z","messageCount":15,"participants":["Jeff King","Patrick Steinhardt","Harald Nordgren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"534158","messageId":"20260119051858.GA1991308@coredump.intra.peff.net","threadId":"64829","inReplyTo":null,"subject":"[PATCH 0/4] memory leaks in remote.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-19T05:18:58Z","receivedAt":"2026-01-19T05:18:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes some memory leaks in remote.c. Not urgent, as they are quite\nold, but they are newly triggered in the test suite by Harald's\nhn/status-compare-with-push topic. So I think we'd want to build that\ntopic on top of these.\n\nThe first two are just preparatory cleanups. Patch 3 fixes the leak that\nHarald's series triggers (and adds its own test, of course). Patch 4 is\na hypothetical leak that I don't think can be triggered in practice (so\nit's more of a cleanup).\n\n  [1/4]: remote: return non-const pointer from error_buf()\n  [2/4]: remote: drop const return of tracking_for_push_dest()\n  [3/4]: remote: fix leak in branch_get_push_1() with invalid \"simple\" config\n  [4/4]: remote: always allocate branch.push_tracking_ref\n\n remote.c                | 24 ++++++++++++++----------\n remote.h                |  2 +-\n t/for-each-ref-tests.sh |  9 +++++++++\n 3 files changed, 24 insertions(+), 11 deletions(-)\n\n-Peff\n"},{"id":"534159","messageId":"20260119051945.GA1991523@coredump.intra.peff.net","threadId":"64829","inReplyTo":"20260119051858.GA1991308@coredump.intra.peff.net","subject":"[PATCH 1/4] remote: return non-const pointer from error_buf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-19T05:19:45Z","receivedAt":"2026-01-19T05:19:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have an error_buf() helper that functions a bit like our error()\nhelper, but returns NULL instead of -1. Its return type is \"const char\n*\", but this is overly restrictive. If we use the helper in a function\nthat returns non-const \"char *\", the compiler will complain about\nthe implicit cast from const to non-const.\n\nMeanwhile, the const in the helper is doing nothing useful, as it only\never returns NULL. Let's drop the const, which will let us use it in\nboth types of function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex b756ff6f15..3dc100be83 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1831,7 +1831,7 @@ int branch_merge_matches(struct branch *branch,\n }\n \n __attribute__((format (printf,2,3)))\n-static const char *error_buf(struct strbuf *err, const char *fmt, ...)\n+static char *error_buf(struct strbuf *err, const char *fmt, ...)\n {\n \tif (err) {\n \t\tva_list ap;\n-- \n2.53.0.rc0.338.g08aa8a9473\n\n"},{"id":"534160","messageId":"20260119052026.GB1991523@coredump.intra.peff.net","threadId":"64829","inReplyTo":"20260119051858.GA1991308@coredump.intra.peff.net","subject":"[PATCH 2/4] remote: drop const return of tracking_for_push_dest()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-19T05:20:26Z","receivedAt":"2026-01-19T05:20:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The string returned from tracking_for_push_dest() comes from\napply_refspec(), and thus is always an allocated string (or NULL). We\nshould return a non-const pointer so that the caller knows that\nownership of the string is being transferred.\n\nThis goes back to the function's origin in e291c75a95 (remote.c: add\nbranch_get_push, 2015-05-21). It never really mattered because our\nreturn is just forwarded through branch_get_push_1(), which returns a\nconst string as part of an intentionally hacky memory management scheme\n(see that commit for details).\n\nAs the first step of untangling that hackery, let's drop the extra const\nfrom this helper function (and from the variables that store its\nresult). There should be no functional change (yet).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3dc100be83..5de9619bc7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1869,9 +1869,9 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)\n \treturn branch->merge[0]->dst;\n }\n \n-static const char *tracking_for_push_dest(struct remote *remote,\n-\t\t\t\t\t  const char *refname,\n-\t\t\t\t\t  struct strbuf *err)\n+static char *tracking_for_push_dest(struct remote *remote,\n+\t\t\t\t    const char *refname,\n+\t\t\t\t    struct strbuf *err)\n {\n \tchar *ret;\n \n@@ -1899,7 +1899,7 @@ static const char *branch_get_push_1(struct repository *repo,\n \n \tif (remote->push.nr) {\n \t\tchar *dst;\n-\t\tconst char *ret;\n+\t\tchar *ret;\n \n \t\tdst = apply_refspecs(&remote->push, branch->refname);\n \t\tif (!dst)\n@@ -1929,7 +1929,8 @@ static const char *branch_get_push_1(struct repository *repo,\n \tcase PUSH_DEFAULT_UNSPECIFIED:\n \tcase PUSH_DEFAULT_SIMPLE:\n \t\t{\n-\t\t\tconst char *up, *cur;\n+\t\t\tconst char *up;\n+\t\t\tchar *cur;\n \n \t\t\tup = branch_get_upstream(branch, err);\n \t\t\tif (!up)\n-- \n2.53.0.rc0.338.g08aa8a9473\n\n"},{"id":"534161","messageId":"20260119052208.GC1991523@coredump.intra.peff.net","threadId":"64829","inReplyTo":"20260119051858.GA1991308@coredump.intra.peff.net","subject":"[PATCH 3/4] remote: fix leak in branch_get_push_1() with invalid \"simple\" config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-19T05:22:08Z","receivedAt":"2026-01-19T05:22:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most of the code paths in branch_get_push_1() allocate a string for the\n@{push} value. We then return the result, which is stored in a \"struct\nbranch\", so the value is not leaked.\n\nBut there's one path that does leak: when we are in the \"simple\" push\nmode, we have to check that the @{push} value matches what we'd get for\n@{upstream}. If it doesn't, we return an error, but forget to free the\n@{push} value we computed.\n\nCuriously, the existing tests don't trigger this with LSan, even though\nthey do exercise the code path. As far as I can tell, it should be\ntriggered via:\n\n  git -c push.default=simple \\\n      -c branch.foo.remote=origin \\\n      -c branch.foo.merge=refs/heads/not-foo \\\n      rev-parse foo@{push}\n\nwhich will complain that the upstream (\"not-foo\") does not match the\npush destination (\"foo\"). We do die() shortly after this, but not until\nafter returning from branch_get_push_1(), which is where the leak\nhappens.\n\nSo it seems like a false negative in LSan. However, I can trigger it\nreliably by printing the @{push} value using for-each-ref. This takes a\nlittle more setup (because we need \"foo\" to actually exist to iterate\nover it with for-each-ref), but we can piggy-back on the existing repo\nconfig in t6300.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c                | 4 +++-\n t/for-each-ref-tests.sh | 9 +++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 5de9619bc7..e191b0ff6e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1938,9 +1938,11 @@ static const char *branch_get_push_1(struct repository *repo,\n \t\t\tcur = tracking_for_push_dest(remote, branch->refname, err);\n \t\t\tif (!cur)\n \t\t\t\treturn NULL;\n-\t\t\tif (strcmp(cur, up))\n+\t\t\tif (strcmp(cur, up)) {\n+\t\t\t\tfree(cur);\n \t\t\t\treturn error_buf(err,\n \t\t\t\t\t\t _(\"cannot resolve 'simple' push to a single destination\"));\n+\t\t\t}\n \t\t\treturn cur;\n \t\t}\n \t}\ndiff --git a/t/for-each-ref-tests.sh b/t/for-each-ref-tests.sh\nindex 4593be5fd5..bd2d45c971 100644\n--- a/t/for-each-ref-tests.sh\n+++ b/t/for-each-ref-tests.sh\n@@ -1744,6 +1744,15 @@ test_expect_success ':remotename and :remoteref' '\n \t)\n '\n \n+test_expect_success '%(push) with an invalid push-simple config' '\n+\techo \"refs/heads/main \" >expect &&\n+\tgit -c push.default=simple \\\n+\t    -c remote.pushdefault=myfork \\\n+\t    for-each-ref \\\n+\t    --format=\"%(refname) %(push)\" refs/heads/main >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success \"${git_for_each_ref} --ignore-case ignores case\" '\n \t${git_for_each_ref} --format=\"%(refname)\" refs/heads/MAIN >actual &&\n \ttest_must_be_empty actual &&\n-- \n2.53.0.rc0.338.g08aa8a9473\n\n"},{"id":"534162","messageId":"20260119052320.GD1991523@coredump.intra.peff.net","threadId":"64829","inReplyTo":"20260119051858.GA1991308@coredump.intra.peff.net","subject":"[PATCH 4/4] remote: always allocate branch.push_tracking_ref","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-19T05:23:20Z","receivedAt":"2026-01-19T05:23:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In branch_get_push(), we usually allocate a new string for the @{push}\nref, but will not do so in push.default=upstream mode, where we just\npass back the result of branch_get_upstream() directly.\n\nThis led to a hacky memory management scheme in e291c75a95 (remote.c:\nadd branch_get_push, 2015-05-21): we store the result in the\npush_tracking_ref field of a \"struct branch\", under the assumption that\nthe branch struct will last until the end of the program. So even though\nthe struct doesn't know if it has an allocated string or not, it doesn't\nmatter because we hold on to it either way.\n\nBut that assumption was violated by f5ccb535cc (remote: fix leaking\nconfig strings, 2024-08-22), which added a function to free branch\nstructs. Any struct which is fed to branch_release() is at risk of\nleaking its push_tracking_ref member.\n\nI don't think this can actually be triggered in practice. We rarely\nactually free the branch structs, and we only fill in the\npush_tracking_ref string lazily when it is needed. So triggering the\nleak would require a code path that does both, and I couldn't find one.\n\nStill, this is an ugly trap that may eventually spring on us. Since\nthere is only one code path in branch_get_push() that doesn't allocate,\nlet's just have it copy the string. And then we know that\npush_tracking_ref is always allocated, and we can free it in\nbranch_release().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n remote.c | 7 ++++---\n remote.h | 2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex e191b0ff6e..3e9d9b3e1f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -272,6 +272,7 @@ static void branch_release(struct branch *branch)\n \tfree((char *)branch->refname);\n \tfree(branch->remote_name);\n \tfree(branch->pushremote_name);\n+\tfree(branch->push_tracking_ref);\n \tmerge_clear(branch);\n }\n \n@@ -1883,8 +1884,8 @@ static char *tracking_for_push_dest(struct remote *remote,\n \treturn ret;\n }\n \n-static const char *branch_get_push_1(struct repository *repo,\n-\t\t\t\t     struct branch *branch, struct strbuf *err)\n+static char *branch_get_push_1(struct repository *repo,\n+\t\t\t       struct branch *branch, struct strbuf *err)\n {\n \tstruct remote_state *remote_state = repo->remote_state;\n \tstruct remote *remote;\n@@ -1924,7 +1925,7 @@ static const char *branch_get_push_1(struct repository *repo,\n \t\treturn tracking_for_push_dest(remote, branch->refname, err);\n \n \tcase PUSH_DEFAULT_UPSTREAM:\n-\t\treturn branch_get_upstream(branch, err);\n+\t\treturn xstrdup_or_null(branch_get_upstream(branch, err));\n \n \tcase PUSH_DEFAULT_UNSPECIFIED:\n \tcase PUSH_DEFAULT_SIMPLE:\ndiff --git a/remote.h b/remote.h\nindex 0ca399e183..fc052945ee 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -331,7 +331,7 @@ struct branch {\n \n \tint merge_alloc;\n \n-\tconst char *push_tracking_ref;\n+\tchar *push_tracking_ref;\n };\n \n struct branch *branch_get(const char *name);\n-- \n2.53.0.rc0.338.g08aa8a9473\n"},{"id":"534166","messageId":"aW3QVkpPPHjKVNLC@pks.im","threadId":"64829","inReplyTo":"20260119051945.GA1991523@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] remote: return non-const pointer from error_buf()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-19T06:33:58Z","receivedAt":"2026-01-19T06:34:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 19, 2026 at 12:19:45AM -0500, Jeff King wrote:\n> We have an error_buf() helper that functions a bit like our error()\n> helper, but returns NULL instead of -1. Its return type is \"const char\n> *\", but this is overly restrictive. If we use the helper in a function\n> that returns non-const \"char *\", the compiler will complain about\n> the implicit cast from const to non-const.\n> \n> Meanwhile, the const in the helper is doing nothing useful, as it only\n> ever returns NULL. Let's drop the const, which will let us use it in\n> both types of function.\n\nThis function signature is indeed quite misleading, and I'd argue that\nit continues to be so even after the change. I guess the intent is to\nmake it a bit easier to print an error in functions that return a\nstring.\n\nI'm not really a huge fan of this, but it's not a fault of this patch\nseries, so let's read on.\n\nPatrick\n"},{"id":"534167","messageId":"aW3QWxCNPy9paq9r@pks.im","threadId":"64829","inReplyTo":"20260119052026.GB1991523@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] remote: drop const return of tracking_for_push_dest()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-19T06:34:03Z","receivedAt":"2026-01-19T06:34:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 19, 2026 at 12:20:26AM -0500, Jeff King wrote:\n> The string returned from tracking_for_push_dest() comes from\n> apply_refspec(), and thus is always an allocated string (or NULL). We\n> should return a non-const pointer so that the caller knows that\n> ownership of the string is being transferred.\n> \n> This goes back to the function's origin in e291c75a95 (remote.c: add\n> branch_get_push, 2015-05-21). It never really mattered because our\n> return is just forwarded through branch_get_push_1(), which returns a\n> const string as part of an intentionally hacky memory management scheme\n> (see that commit for details).\n\nOkay, so here we can now also return a `char *` now that `error_buf()`\ngot adapted.\n\n> As the first step of untangling that hackery, let's drop the extra const\n> from this helper function (and from the variables that store its\n> result). There should be no functional change (yet).\n\nYup. The memory handling still feels weird, but as in the preceding\ncommit that's not a fault of this patch series.\n\nPatrick\n"},{"id":"534168","messageId":"aW3QYPvRUkvwKU1E@pks.im","threadId":"64829","inReplyTo":"20260119052208.GC1991523@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] remote: fix leak in branch_get_push_1() with invalid \"simple\" config","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-19T06:34:08Z","receivedAt":"2026-01-19T06:34:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 19, 2026 at 12:22:08AM -0500, Jeff King wrote:\n> diff --git a/remote.c b/remote.c\n> index 5de9619bc7..e191b0ff6e 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1938,9 +1938,11 @@ static const char *branch_get_push_1(struct repository *repo,\n>  \t\t\tcur = tracking_for_push_dest(remote, branch->refname, err);\n>  \t\t\tif (!cur)\n>  \t\t\t\treturn NULL;\n> -\t\t\tif (strcmp(cur, up))\n> +\t\t\tif (strcmp(cur, up)) {\n> +\t\t\t\tfree(cur);\n>  \t\t\t\treturn error_buf(err,\n>  \t\t\t\t\t\t _(\"cannot resolve 'simple' push to a single destination\"));\n> +\t\t\t}\n\nYup, this memory leak was easy to spot in the preceding commit after\nyour refactorings.\n\nPatrick\n"},{"id":"534169","messageId":"aW3QZaYoPQvBkfvd@pks.im","threadId":"64829","inReplyTo":"20260119052320.GD1991523@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] remote: always allocate branch.push_tracking_ref","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-19T06:34:13Z","receivedAt":"2026-01-19T06:34:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 19, 2026 at 12:23:20AM -0500, Jeff King wrote:\n> diff --git a/remote.c b/remote.c\n> index e191b0ff6e..3e9d9b3e1f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1924,7 +1925,7 @@ static const char *branch_get_push_1(struct repository *repo,\n>  \t\treturn tracking_for_push_dest(remote, branch->refname, err);\n>  \n>  \tcase PUSH_DEFAULT_UPSTREAM:\n> -\t\treturn branch_get_upstream(branch, err);\n> +\t\treturn xstrdup_or_null(branch_get_upstream(branch, err));\n>  \n>  \tcase PUSH_DEFAULT_UNSPECIFIED:\n>  \tcase PUSH_DEFAULT_SIMPLE:\n\nMakes sense. I was wondering whether you'd also change\n`branch_get_push_1()` in a subsequent patch, so I'm happy to see this.\n\nThis whole series looks good to me, thanks!\n\nPatrick\n"},{"id":"534181","messageId":"20260119150413.37807-1-haraldnordgren@gmail.com","threadId":"64829","inReplyTo":"20260119051858.GA1991308@coredump.intra.peff.net","subject":"Triangular workflow","fromName":"Harald Nordgren","fromEmail":"haraldnordgren@gmail.com","sentAt":"2026-01-19T15:04:13Z","receivedAt":"2026-01-19T15:04:17Z","isPatch":false,"sender":{"key":"haraldnordgren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9569897?v=4"},"body":"Thanks a lot Jeff!\n\nWould be nice to get this merged ASAP, so I can continue the work on my\nfeature without the memory leak there.\n\n\nHarald\n"},{"id":"534200","messageId":"xmqqikcx3z5x.fsf@gitster.g","threadId":"64829","inReplyTo":"aW3QVkpPPHjKVNLC@pks.im","subject":"Re: [PATCH 1/4] remote: return non-const pointer from error_buf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-20T00:28:42Z","receivedAt":"2026-01-20T00:28:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> This function signature is indeed quite misleading, and I'd argue that\n> it continues to be so even after the change. I guess the intent is to\n> make it a bit easier to print an error in functions that return a\n> string.\n>\n> I'm not really a huge fan of this, but it's not a fault of this patch\n> series, so let's read on.\n\nI concur.  \"If they do not return any useful value, they should be\nvoid\" was my first reaction, but presumably just like \"return\nerror(\"message\");\" is a handy way to give message while signalling\nan error to the caller, these are used to return NULL that signals\nan error?  I do not offhand think of a good longer-term direction to\nimprove this one.\n\n"},{"id":"534201","messageId":"xmqqecnl3z0z.fsf@gitster.g","threadId":"64829","inReplyTo":"20260119051858.GA1991308@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] memory leaks in remote.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-20T00:31:40Z","receivedAt":"2026-01-20T00:31:43Z","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> This fixes some memory leaks in remote.c. Not urgent, as they are quite\n> old, but they are newly triggered in the test suite by Harald's\n> hn/status-compare-with-push topic. So I think we'd want to build that\n> topic on top of these.\n>\n> The first two are just preparatory cleanups. Patch 3 fixes the leak that\n> Harald's series triggers (and adds its own test, of course). Patch 4 is\n> a hypothetical leak that I don't think can be triggered in practice (so\n> it's more of a cleanup).\n>\n>   [1/4]: remote: return non-const pointer from error_buf()\n>   [2/4]: remote: drop const return of tracking_for_push_dest()\n>   [3/4]: remote: fix leak in branch_get_push_1() with invalid \"simple\" config\n>   [4/4]: remote: always allocate branch.push_tracking_ref\n>\n>  remote.c                | 24 ++++++++++++++----------\n>  remote.h                |  2 +-\n>  t/for-each-ref-tests.sh |  9 +++++++++\n>  3 files changed, 24 insertions(+), 11 deletions(-)\n\nAll look sensible.  Will queue.  Thanks.\n"},{"id":"534281","messageId":"20260120193857.GC3295894@coredump.intra.peff.net","threadId":"64829","inReplyTo":"xmqqikcx3z5x.fsf@gitster.g","subject":"Re: [PATCH 1/4] remote: return non-const pointer from error_buf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-20T19:38:57Z","receivedAt":"2026-01-20T19:38:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 19, 2026 at 04:28:42PM -0800, Junio C Hamano wrote:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > This function signature is indeed quite misleading, and I'd argue that\n> > it continues to be so even after the change. I guess the intent is to\n> > make it a bit easier to print an error in functions that return a\n> > string.\n> >\n> > I'm not really a huge fan of this, but it's not a fault of this patch\n> > series, so let's read on.\n> \n> I concur.  \"If they do not return any useful value, they should be\n> void\" was my first reaction, but presumably just like \"return\n> error(\"message\");\" is a handy way to give message while signalling\n> an error to the caller, these are used to return NULL that signals\n> an error?  I do not offhand think of a good longer-term direction to\n> improve this one.\n\nYes, that's exactly the purpose. I don't see many changes that could\nlet it still fulfill that purpose, though perhaps one could argue that\nit is unnecessarily confusing for the small shortening of the code it\nprovides (and ditto for error() itself).\n\nThere is one thing it probably could do: return a \"void *\" instead. That\nwould make it applicable to a wider variety of functions. But it also\nmakes it even more obscure (IMHO), and this is a static-local function\nthat is only used for functions that return strings anyway.\n\nIf we wanted to make it more generic (and I do not think we want to), we\ncan see that it differs from error() in two dimensions:\n\n  - error() writes to stderr, but error_buf() writes to a strbuf (or\n    nowhere if the strbuf is NULL)\n\n  - error() passes along integer \"-1\" to signal error, but error_buf()\n    passes along NULL\n\nSo of the four combinations, we have:\n\n  - stderr / integer: error()\n  - stderr / pointer: not implemented\n  - strbuf / integer: not implemented\n  - strbuf / pointer: error_buf()\n\nOne could imagine a suite of related functions: error_int(),\nerror_null(), error_buf_int(), error_buf_null() that provide all four.\n\nBut I do not see us clamoring to extend the pattern further. ;)\n\n-Peff\n"},{"id":"534282","messageId":"20260120194010.GD3295894@coredump.intra.peff.net","threadId":"64829","inReplyTo":"20260119150413.37807-1-haraldnordgren@gmail.com","subject":"Re: Triangular workflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-20T19:40:10Z","receivedAt":"2026-01-20T19:40:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 19, 2026 at 04:04:13PM +0100, Harald Nordgren wrote:\n\n> Would be nice to get this merged ASAP, so I can continue the work on my\n> feature without the memory leak there.\n\nI imagine it will not get merged until after the upcoming release. The\nusual thing is to build your branch on top in the meantime, though I\ndon't offhand know how good GGG's support is for then sending only your\npatches (you might need to build your PR against the branch that Junio\ncreated when he picked up the topic, but that is only in gitster/git,\nnot git/git).\n\n-Peff\n"},{"id":"534285","messageId":"xmqqbjioyr5s.fsf@gitster.g","threadId":"64829","inReplyTo":"20260120193857.GC3295894@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] remote: return non-const pointer from error_buf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-20T20:18:07Z","receivedAt":"2026-01-20T20:18:10Z","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> On Mon, Jan 19, 2026 at 04:28:42PM -0800, Junio C Hamano wrote:\n>\n>> Patrick Steinhardt <ps@pks.im> writes:\n>> \n>> > This function signature is indeed quite misleading, and I'd argue that\n>> > it continues to be so even after the change. I guess the intent is to\n>> > make it a bit easier to print an error in functions that return a\n>> > string.\n>> >\n>> > I'm not really a huge fan of this, but it's not a fault of this patch\n>> > series, so let's read on.\n>> \n>> I concur.  \"If they do not return any useful value, they should be\n>> void\" was my first reaction, but presumably just like \"return\n>> error(\"message\");\" is a handy way to give message while signalling\n>> an error to the caller, these are used to return NULL that signals\n>> an error?  I do not offhand think of a good longer-term direction to\n>> improve this one.\n>\n> Yes, that's exactly the purpose. I don't see many changes that could\n> let it still fulfill that purpose, though perhaps one could argue that\n> it is unnecessarily confusing for the small shortening of the code it\n> provides (and ditto for error() itself).\n>\n> There is one thing it probably could do: return a \"void *\" instead. That\n> would make it applicable to a wider variety of functions. But it also\n> makes it even more obscure (IMHO), and this is a static-local function\n> that is only used for functions that return strings anyway.\n>\n> If we wanted to make it more generic (and I do not think we want to), we\n> can see that it differs from error() in two dimensions:\n>\n>   - error() writes to stderr, but error_buf() writes to a strbuf (or\n>     nowhere if the strbuf is NULL)\n>\n>   - error() passes along integer \"-1\" to signal error, but error_buf()\n>     passes along NULL\n>\n> So of the four combinations, we have:\n>\n>   - stderr / integer: error()\n>   - stderr / pointer: not implemented\n>   - strbuf / integer: not implemented\n>   - strbuf / pointer: error_buf()\n>\n> One could imagine a suite of related functions: error_int(),\n> error_null(), error_buf_int(), error_buf_null() that provide all four.\n>\n> But I do not see us clamoring to extend the pattern further. ;)\n\n;-).  Perhaps stop being cute and doing\n\n\tif (... error ...) {\n\t\tformat_error(err, _(\"error message\"), ...);\n                return NULL;\n\t}\n\nwithout any magic might be more appropriate for a file-scope static\nthat is only used for a handful of times?  I do not think the\nsituation is bad enough to warrant patch noise like this, but it is\nsufficiently bad that I wish we wrote them in such a more trivial\nway in the first place X-<.\n\n"}]}