{"thread":{"id":"62481","subject":"[BUG REPORT] git fetch --prefetch with incorrect options hangs with 2.47.0","startedAt":"2024-11-11T22:22:24Z","lastAt":"2024-11-12T09:30:07Z","messageCount":7,"participants":["Eric Mills","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"507083","messageId":"SA1PR17MB6501281EF202EA694CF9DC03CA582@SA1PR17MB6501.namprd17.prod.outlook.com","threadId":"62481","inReplyTo":null,"subject":"[BUG REPORT] git fetch --prefetch with incorrect options hangs with 2.47.0","fromName":"Eric Mills","fromEmail":"ermills@epic.com","sentAt":"2024-11-11T22:15:44Z","receivedAt":"2024-11-11T22:22:24Z","isPatch":false,"sender":{"key":"ermills@epic.com","avatar":null},"body":"\n\nEric Mills (he/him) | Epic | Hyperspace\nermills@epic.com | (608) 271-9000\n\n\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nI ran `git fetch --prefetch origin main` with git 2.47 on both my\nWindows machine (2.47.windows.2) and my Mac (2.47.0) and it hangs.\n\nWhen I downgrade, same command succeeds on\n2.46.2.Windows.1 / 2.46.2 (macOS).\n\nWhat did you expect to happen? (Expected behavior)\nI expect the fetch to complete without hanging as it does in 2.46.2.\n\nNote that while investigating this, I learned that `git fetch\n--prefetch` may not be meant to be run with specific refs. The idea\nwas to prefetch `main`, but it seems that in 2.46.2 this prefetches\nall refs from that remote even when a specific ref is given. If\nprefetching specific refs is explicitly not allowed, I would expect a\nmore targeted error message instead of hanging. If it is meant to work,\nI would expect that only that ref is prefetched.\n\nWhat happened instead? (Actual behavior)\nIt hangs to the point that I have to end the process in Task Manager.\n\nWhat's different between what you expected and what actually happened?\nI don't expect the command to hang.\n\nAnything else you want to add:\nTo quickly recap, the hang occurs on both macOS and Windows. It doesn't\nhappen in 2.46.2, but does happen in 2.47.0.\n\nEven though I might be using the command incorrectly, I thought it was\nworth reporting because it seems to be a regression.\n\nI found the issue on Windows, but since I could reproduce it on macOS\n(or something similar on macOS), I'm filing the issue upstream.\n\nOn Windows, I don't get an error. It hangs indefinitely.\nOn macOS, after a few seconds I got the following:\n\ngit(84051,0x20171cf40) malloc: *** error for object 0x600001978390:\npointer being freed was not allocated\n\ngit(84051,0x20171cf40) malloc: *** set a breakpoint in\nmalloc_error_break to debug\n\n[1]    84051 abort      git fetch --prefetch origin main\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.47.0.windows.2\ncpu: x86_64\nbuilt from commit: 1f8a83cba6e88fad4b881885e64cfb89458e3653\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: D:/git-sdk-64-build-installers/usr/bin/sh\nfeature: fsmonitor--daemon\nlibcurl: 8.10.1\nOpenSSL: OpenSSL 3.2.3 3 Sep 2024\nzlib: 1.3.1\nuname: Windows 10.0 22631\ncompiler info: gnuc: 14.2\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n\n\n[Enabled Hooks]\n"},{"id":"507109","messageId":"20241112064951.GA619985@coredump.intra.peff.net","threadId":"62481","inReplyTo":"SA1PR17MB6501281EF202EA694CF9DC03CA582@SA1PR17MB6501.namprd17.prod.outlook.com","subject":"Re: [BUG REPORT] git fetch --prefetch with incorrect options hangs with 2.47.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-12T06:49:51Z","receivedAt":"2024-11-12T06:49:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 11, 2024 at 10:15:44PM +0000, Eric Mills wrote:\n\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> I ran `git fetch --prefetch origin main` with git 2.47 on both my\n> Windows machine (2.47.windows.2) and my Mac (2.47.0) and it hangs.\n> \n> When I downgrade, same command succeeds on\n> 2.46.2.Windows.1 / 2.46.2 (macOS).\n> [...]\n> On Windows, I don't get an error. It hangs indefinitely.\n> On macOS, after a few seconds I got the following:\n> \n> git(84051,0x20171cf40) malloc: *** error for object 0x600001978390:\n> pointer being freed was not allocated\n> \n> git(84051,0x20171cf40) malloc: *** set a breakpoint in\n> malloc_error_break to debug\n\nThanks for the report. I see similar heap corruption problems on Linux.\nBuilding with ASan shows a double-free:\n\n  ==646934==ERROR: AddressSanitizer: attempting double-free on 0x502000002450 in thread T0:\n      #0 0x7f9ab1cf3918 in free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52\n      #1 0x557a1d8082c2 in refspec_clear /home/peff/compile/git/refspec.c:228\n      #2 0x557a1ce92a08 in fetch_one builtin/fetch.c:2136\n      #3 0x557a1ce9889d in cmd_fetch builtin/fetch.c:2443\n      #4 0x557a1cd2e1ca in run_builtin /home/peff/compile/git/git.c:483\n      #5 0x557a1cd2f2d8 in handle_builtin /home/peff/compile/git/git.c:749\n      #6 0x557a1cd2fb3a in run_argv /home/peff/compile/git/git.c:819\n      #7 0x557a1cd30e66 in cmd_main /home/peff/compile/git/git.c:954\n      #8 0x557a1d1370c1 in main /home/peff/compile/git/common-main.c:64\n      #9 0x7f9ab1233d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n      #10 0x7f9ab1233e24 in __libc_start_main_impl ../csu/libc-start.c:360\n      #11 0x557a1cd27290 in _start (/home/peff/compile/git/git+0x129d290) (BuildId: cca88fc4d05f503e4aecc54e9437a56865d5eca1)\n\nBisecting on:\n\n  make SANITIZE=address &&\n  bin-wrappers/git fetch --prefetch origin master\n\nturns up my ea4780307c (fetch: free \"raw\" string when shrinking refspec,\n2024-09-24). I'll see if I can figure out what's going on.\n\n-Peff\n"},{"id":"507114","messageId":"20241112083204.GA2636868@coredump.intra.peff.net","threadId":"62481","inReplyTo":"20241112064951.GA619985@coredump.intra.peff.net","subject":"[PATCH 0/3] double-free with git fetch --prefetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-12T08:32:04Z","receivedAt":"2024-11-12T08:32:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 12, 2024 at 01:49:51AM -0500, Jeff King wrote:\n\n> Bisecting on:\n> \n>   make SANITIZE=address &&\n>   bin-wrappers/git fetch --prefetch origin master\n> \n> turns up my ea4780307c (fetch: free \"raw\" string when shrinking refspec,\n> 2024-09-24). I'll see if I can figure out what's going on.\n\nOK, it turns out to be a fairly simple bug. The hardest part was\nfiguring out why it was not triggering all the time already in the test\nsuite. ;)\n\nPatch 1 is the minimal fix. It is sort-of a regression in v2.47, in that\nit became easier to trigger the bug; but it existed before then. Either\nway, it seems like material for the \"maint\" branch.\n\nThe other two patches are cleanups that I contemplated when doing\nea4780307c. Now that this code has caused _two_ bugs which would have\nbeen impossible with the cleanups, I figured it was worth taking a stab\nat it.\n\nThanks for a clear report.\n\n  [1/3]: fetch: adjust refspec->raw_nr when filtering prefetch refspecs\n  [2/3]: refspec: drop separate raw_nr count\n  [3/3]: refspec: store raw refspecs inside refspec_item\n\n builtin/fetch.c                   |  8 ++------\n builtin/remote.c                  | 16 ++++++++--------\n refspec.c                         | 26 ++++++++++----------------\n refspec.h                         |  6 ++----\n submodule.c                       |  8 ++++----\n t/t5582-fetch-negative-refspec.sh |  4 ++++\n 6 files changed, 30 insertions(+), 38 deletions(-)\n\n-Peff\n\n"},{"id":"507115","messageId":"20241112083400.GA3529122@coredump.intra.peff.net","threadId":"62481","inReplyTo":"20241112083204.GA2636868@coredump.intra.peff.net","subject":"[PATCH 1/3] fetch: adjust refspec->raw_nr when filtering prefetch refspecs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-12T08:34:00Z","receivedAt":"2024-11-12T08:34:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In filter_prefetch_refspecs(), we may remove one or more refspecs if\nthey point into refs/tags/. When we do, we remove the item from the\nrefspec->items array, shifting subsequent items down, and then decrement\nthe refspec->nr count.\n\nWe also remove the item from the refspec->raw array, but fail to\ndecrement refspec->raw_nr. This leaves us with a count that is too high,\nand anybody looking at the \"raw\" array will erroneously see either:\n\n  1. The removed entry, if there were no subsequent items to shift down.\n\n  2. A duplicate of the final entry, as everything is shifted down but\n     there was nothing to overwrite the final item.\n\nThe obvious culprit to run into this is calling refspec_clear(), which\nwill try to free the removed entry (case 1) or double-free the final\nentry (case 2). But even though the bug has existed since the function\nwas added in 2e03115d0c (fetch: add --prefetch option, 2021-04-16), we\ndid not trigger it in the test suite. The --prefetch option is normally\nonly used with configured refspecs, and we never bother to call\nrefspec_clear() on those (they are stored as part of a struct remote,\nwhich is held in a global variable).\n\nBut you could trigger case 2 manually like:\n\n  git fetch --prefetch . refs/tags/foo refs/tags/bar\n\nIronically you couldn't trigger case 1, because the code accidentally\nleaked the string in the raw array, and the two bugs (the leak and the\ndouble-free) cancelled out. But when we fixed the leak in ea4780307c\n(fetch: free \"raw\" string when shrinking refspec, 2024-09-24), it became\npossible to trigger that, too, with a single item:\n\n  git fetch --prefetch . refs/tags/foo\n\nWe can fix both cases by just correctly decrementing \"raw_nr\" when we\nshrink the array. Even though we don't expect people to use --prefetch\nwith command-line refspecs, we'll add a test to make sure it behaves\nwell (like the test just before it, we're just confirming that the\nfiltered prefetch succeeds at all).\n\nReported-by: Eric Mills <ermills@epic.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c                   | 1 +\n t/t5582-fetch-negative-refspec.sh | 4 ++++\n 2 files changed, 5 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d9027e4dc9..9e0cabebe7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -463,6 +463,7 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t\t\t\trs->raw[j - 1] = rs->raw[j];\n \t\t\t}\n \t\t\trs->nr--;\n+\t\t\trs->raw_nr--;\n \t\t\ti--;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 7fa54a4029..b21bf2057d 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -283,4 +283,8 @@ test_expect_success '--prefetch succeeds when refspec becomes empty' '\n \tgit -C one fetch --prefetch\n '\n \n+test_expect_success '--prefetch succeeds with empty command line refspec' '\n+\tgit -C one fetch --prefetch origin +refs/tags/extra\n+'\n+\n test_done\n-- \n2.47.0.508.g57228aee23\n\n"},{"id":"507116","messageId":"20241112083610.GB3529122@coredump.intra.peff.net","threadId":"62481","inReplyTo":"20241112083204.GA2636868@coredump.intra.peff.net","subject":"[PATCH 2/3] refspec: drop separate raw_nr count","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-12T08:36:10Z","receivedAt":"2024-11-12T08:36:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A refspec struct contains zero or more refspec_item structs, along with\nmatching \"raw\" strings. The items and raw strings are kept in separate\narrays, but those arrays will always have the same length (because we\nwrite them only via refspec_append_nodup(), which grows both). This can\nlead to bugs when manipulating the array, since the arrays and lengths\nmust be modified in lockstep. For example, the bug fixed in the previous\ncommit, which forgot to decrement raw_nr.\n\nSo let's get rid of \"raw_nr\" and have only \"nr\", making this kind of bug\nimpossible (and also making it clear that the two are always matched,\nsomething that existing code already assumed but was not guaranteed by\nthe interface).\n\nEven though we'd expect \"alloc\" and \"raw_alloc\" to likewise move in\nlockstep, we still need to keep separate counts there if we want to\ncontinue to use ALLOC_GROW() for both.\n\nConceptually this would all be simpler if refspec_item just held onto\nits own raw string, and we had a single array. But there are callers\nwhich use refspec_item outside of \"struct refspec\" (and so don't hold on\nto a matching \"raw\" string at all), which we'd possibly need to adjust.\nSo let's not worry about refactoring that for now, and just get rid of\nthe redundant count variable. That is the first step on the road to\ncombining them anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c  |  1 -\n builtin/remote.c |  8 ++++----\n refspec.c        | 15 ++++++++-------\n refspec.h        |  1 -\n submodule.c      |  4 ++--\n 5 files changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9e0cabebe7..d9027e4dc9 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -463,7 +463,6 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t\t\t\trs->raw[j - 1] = rs->raw[j];\n \t\t\t}\n \t\t\trs->nr--;\n-\t\t\trs->raw_nr--;\n \t\t\ti--;\n \t\t\tcontinue;\n \t\t}\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 76670ddd8b..875d6c3bad 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -633,11 +633,11 @@ static int migrate_file(struct remote *remote)\n \t\tgit_config_set_multivar(buf.buf, remote->url.v[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n-\tfor (i = 0; i < remote->push.raw_nr; i++)\n+\tfor (i = 0; i < remote->push.nr; i++)\n \t\tgit_config_set_multivar(buf.buf, remote->push.raw[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n-\tfor (i = 0; i < remote->fetch.raw_nr; i++)\n+\tfor (i = 0; i < remote->fetch.nr; i++)\n \t\tgit_config_set_multivar(buf.buf, remote->fetch.raw[i], \"^$\", 0);\n \tif (remote->origin == REMOTE_REMOTES)\n \t\tunlink_or_warn(git_path(\"remotes/%s\", remote->name));\n@@ -759,12 +759,12 @@ static int mv(int argc, const char **argv, const char *prefix)\n \t\tgoto out;\n \t}\n \n-\tif (oldremote->fetch.raw_nr) {\n+\tif (oldremote->fetch.nr) {\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new_name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old_name);\n-\t\tfor (i = 0; i < oldremote->fetch.raw_nr; i++) {\n+\t\tfor (i = 0; i < oldremote->fetch.nr; i++) {\n \t\t\tchar *ptr;\n \n \t\t\tstrbuf_reset(&buf2);\ndiff --git a/refspec.c b/refspec.c\nindex c3cf003443..8e8ee8542d 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -186,10 +186,12 @@ static void refspec_append_nodup(struct refspec *rs, char *refspec)\n \trefspec_item_init_or_die(&item, refspec, rs->fetch);\n \n \tALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);\n-\trs->items[rs->nr++] = item;\n+\trs->items[rs->nr] = item;\n \n-\tALLOC_GROW(rs->raw, rs->raw_nr + 1, rs->raw_alloc);\n-\trs->raw[rs->raw_nr++] = refspec;\n+\tALLOC_GROW(rs->raw, rs->nr + 1, rs->raw_alloc);\n+\trs->raw[rs->nr] = refspec;\n+\n+\trs->nr++;\n }\n \n void refspec_append(struct refspec *rs, const char *refspec)\n@@ -217,18 +219,17 @@ void refspec_clear(struct refspec *rs)\n {\n \tint i;\n \n-\tfor (i = 0; i < rs->nr; i++)\n+\tfor (i = 0; i < rs->nr; i++) {\n \t\trefspec_item_clear(&rs->items[i]);\n+\t\tfree(rs->raw[i]);\n+\t}\n \n \tFREE_AND_NULL(rs->items);\n \trs->alloc = 0;\n \trs->nr = 0;\n \n-\tfor (i = 0; i < rs->raw_nr; i++)\n-\t\tfree(rs->raw[i]);\n \tFREE_AND_NULL(rs->raw);\n \trs->raw_alloc = 0;\n-\trs->raw_nr = 0;\n \n \trs->fetch = 0;\n }\ndiff --git a/refspec.h b/refspec.h\nindex 3760fdaf2b..0461c9def6 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -45,7 +45,6 @@ struct refspec {\n \n \tchar **raw;\n \tint raw_alloc;\n-\tint raw_nr;\n \n \tint fetch;\n };\ndiff --git a/submodule.c b/submodule.c\nindex 74d5766f07..307f73fb5b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1174,7 +1174,7 @@ static int push_submodule(const char *path,\n \t\tif (remote->origin != REMOTE_UNCONFIGURED) {\n \t\t\tint i;\n \t\t\tstrvec_push(&cp.args, remote->name);\n-\t\t\tfor (i = 0; i < rs->raw_nr; i++)\n+\t\t\tfor (i = 0; i < rs->nr; i++)\n \t\t\t\tstrvec_push(&cp.args, rs->raw[i]);\n \t\t}\n \n@@ -1209,7 +1209,7 @@ static void submodule_push_check(const char *path, const char *head,\n \tstrvec_push(&cp.args, head);\n \tstrvec_push(&cp.args, remote->name);\n \n-\tfor (i = 0; i < rs->raw_nr; i++)\n+\tfor (i = 0; i < rs->nr; i++)\n \t\tstrvec_push(&cp.args, rs->raw[i]);\n \n \tprepare_submodule_repo_env(&cp.env);\n-- \n2.47.0.508.g57228aee23\n\n"},{"id":"507117","messageId":"20241112083937.GC3529122@coredump.intra.peff.net","threadId":"62481","inReplyTo":"20241112083204.GA2636868@coredump.intra.peff.net","subject":"[PATCH 3/3] refspec: store raw refspecs inside refspec_item","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-12T08:39:37Z","receivedAt":"2024-11-12T08:39:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The refspec struct keeps two matched arrays: one for the refspec_item\nstructs and one for the original raw refspec strings. The main reason\nfor this is that there are other users of refspec_item that do not care\nabout the raw strings. But it does make managing the refspec struct\nawkward, as we must keep the two arrays in sync. This has led to bugs in\nthe past (both leaks and double-frees).\n\nLet's just store a copy of the raw refspec string directly in each\nrefspec_item struct. This simplifies the handling at a small cost:\n\n  1. Direct callers of refspec_item_init() will now get an extra copy of\n     the refspec string, even if they don't need it. This should be\n     negligible, as the struct is already allocating two strings for the\n     parsed src/dst values (and we tend to only do it sparingly anyway\n     for things like the TAG_REFSPEC literal).\n\n  2. Users of refspec_appendf() will now generate a temporary string,\n     copy it, and then free the result (versus handing off ownership of\n     the temporary string). We could get around this by having a \"nodup\"\n     variant of refspec_item_init(), but it doesn't seem worth the extra\n     complexity for something that is not remotely a hot code path.\n\nCode which accesses refspec->raw now needs to look at refspec->item.raw.\nOther callers which just use refspec_item directly can remain the same.\nWe'll free the allocated string in refspec_item_clear(), which they\nshould be calling anyway to free src/dst.\n\nOne subtle note: refspec_item_init() can return an error, in which case\nwe'll still have set its \"raw\" field. But that is also true of the \"src\"\nand \"dst\" fields, so any caller which does not _clear() the failed item\nis already potentially leaking. In practice most code just calls die()\non an error anyway, but you can see the exception in valid_fetch_refspec(),\nwhich does correctly call _clear() even on error.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fetch.c  |  8 ++------\n builtin/remote.c |  8 ++++----\n refspec.c        | 25 +++++++++----------------\n refspec.h        |  5 ++---\n submodule.c      |  4 ++--\n 5 files changed, 19 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex d9027e4dc9..0874da55d1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -454,14 +454,10 @@ static void filter_prefetch_refspec(struct refspec *rs)\n \t\t\t\t ref_namespace[NAMESPACE_TAGS].ref))) {\n \t\t\tint j;\n \n-\t\t\tfree(rs->items[i].src);\n-\t\t\tfree(rs->items[i].dst);\n-\t\t\tfree(rs->raw[i]);\n+\t\t\trefspec_item_clear(&rs->items[i]);\n \n-\t\t\tfor (j = i + 1; j < rs->nr; j++) {\n+\t\t\tfor (j = i + 1; j < rs->nr; j++)\n \t\t\t\trs->items[j - 1] = rs->items[j];\n-\t\t\t\trs->raw[j - 1] = rs->raw[j];\n-\t\t\t}\n \t\t\trs->nr--;\n \t\t\ti--;\n \t\t\tcontinue;\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 875d6c3bad..9093600965 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -377,7 +377,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \tfor (i = 0; i < states->remote->fetch.nr; i++)\n \t\tif (get_fetch_map(remote_refs, &states->remote->fetch.items[i], &tail, 1))\n \t\t\tdie(_(\"Could not get fetch map for refspec %s\"),\n-\t\t\t\tstates->remote->fetch.raw[i]);\n+\t\t\t\tstates->remote->fetch.items[i].raw);\n \n \tfor (ref = fetch_map; ref; ref = ref->next) {\n \t\tif (omit_name_by_refspec(ref->name, &states->remote->fetch))\n@@ -634,11 +634,11 @@ static int migrate_file(struct remote *remote)\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n \tfor (i = 0; i < remote->push.nr; i++)\n-\t\tgit_config_set_multivar(buf.buf, remote->push.raw[i], \"^$\", 0);\n+\t\tgit_config_set_multivar(buf.buf, remote->push.items[i].raw, \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n \tfor (i = 0; i < remote->fetch.nr; i++)\n-\t\tgit_config_set_multivar(buf.buf, remote->fetch.raw[i], \"^$\", 0);\n+\t\tgit_config_set_multivar(buf.buf, remote->fetch.items[i].raw, \"^$\", 0);\n \tif (remote->origin == REMOTE_REMOTES)\n \t\tunlink_or_warn(git_path(\"remotes/%s\", remote->name));\n \telse if (remote->origin == REMOTE_BRANCHES)\n@@ -768,7 +768,7 @@ static int mv(int argc, const char **argv, const char *prefix)\n \t\t\tchar *ptr;\n \n \t\t\tstrbuf_reset(&buf2);\n-\t\t\tstrbuf_addstr(&buf2, oldremote->fetch.raw[i]);\n+\t\t\tstrbuf_addstr(&buf2, oldremote->fetch.items[i].raw);\n \t\t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\t\tif (ptr) {\n \t\t\t\trefspec_updated = 1;\ndiff --git a/refspec.c b/refspec.c\nindex 8e8ee8542d..994901f55b 100644\n--- a/refspec.c\n+++ b/refspec.c\n@@ -153,6 +153,7 @@ static int parse_refspec(struct refspec_item *item, const char *refspec, int fet\n int refspec_item_init(struct refspec_item *item, const char *refspec, int fetch)\n {\n \tmemset(item, 0, sizeof(*item));\n+\titem->raw = xstrdup(refspec);\n \treturn parse_refspec(item, refspec, fetch);\n }\n \n@@ -167,6 +168,7 @@ void refspec_item_clear(struct refspec_item *item)\n {\n \tFREE_AND_NULL(item->src);\n \tFREE_AND_NULL(item->dst);\n+\tFREE_AND_NULL(item->raw);\n \titem->force = 0;\n \titem->pattern = 0;\n \titem->matching = 0;\n@@ -179,7 +181,7 @@ void refspec_init(struct refspec *rs, int fetch)\n \trs->fetch = fetch;\n }\n \n-static void refspec_append_nodup(struct refspec *rs, char *refspec)\n+void refspec_append(struct refspec *rs, const char *refspec)\n {\n \tstruct refspec_item item;\n \n@@ -188,24 +190,20 @@ static void refspec_append_nodup(struct refspec *rs, char *refspec)\n \tALLOC_GROW(rs->items, rs->nr + 1, rs->alloc);\n \trs->items[rs->nr] = item;\n \n-\tALLOC_GROW(rs->raw, rs->nr + 1, rs->raw_alloc);\n-\trs->raw[rs->nr] = refspec;\n-\n \trs->nr++;\n }\n \n-void refspec_append(struct refspec *rs, const char *refspec)\n-{\n-\trefspec_append_nodup(rs, xstrdup(refspec));\n-}\n-\n void refspec_appendf(struct refspec *rs, const char *fmt, ...)\n {\n \tva_list ap;\n+\tchar *buf;\n \n \tva_start(ap, fmt);\n-\trefspec_append_nodup(rs, xstrvfmt(fmt, ap));\n+\tbuf = xstrvfmt(fmt, ap);\n \tva_end(ap);\n+\n+\trefspec_append(rs, buf);\n+\tfree(buf);\n }\n \n void refspec_appendn(struct refspec *rs, const char **refspecs, int nr)\n@@ -219,18 +217,13 @@ void refspec_clear(struct refspec *rs)\n {\n \tint i;\n \n-\tfor (i = 0; i < rs->nr; i++) {\n+\tfor (i = 0; i < rs->nr; i++)\n \t\trefspec_item_clear(&rs->items[i]);\n-\t\tfree(rs->raw[i]);\n-\t}\n \n \tFREE_AND_NULL(rs->items);\n \trs->alloc = 0;\n \trs->nr = 0;\n \n-\tFREE_AND_NULL(rs->raw);\n-\trs->raw_alloc = 0;\n-\n \trs->fetch = 0;\n }\n \ndiff --git a/refspec.h b/refspec.h\nindex 0461c9def6..69d693c87d 100644\n--- a/refspec.h\n+++ b/refspec.h\n@@ -26,6 +26,8 @@ struct refspec_item {\n \n \tchar *src;\n \tchar *dst;\n+\n+\tchar *raw;\n };\n \n #define REFSPEC_FETCH 1\n@@ -43,9 +45,6 @@ struct refspec {\n \tint alloc;\n \tint nr;\n \n-\tchar **raw;\n-\tint raw_alloc;\n-\n \tint fetch;\n };\n \ndiff --git a/submodule.c b/submodule.c\nindex 307f73fb5b..7ec564854d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1175,7 +1175,7 @@ static int push_submodule(const char *path,\n \t\t\tint i;\n \t\t\tstrvec_push(&cp.args, remote->name);\n \t\t\tfor (i = 0; i < rs->nr; i++)\n-\t\t\t\tstrvec_push(&cp.args, rs->raw[i]);\n+\t\t\t\tstrvec_push(&cp.args, rs->items[i].raw);\n \t\t}\n \n \t\tprepare_submodule_repo_env(&cp.env);\n@@ -1210,7 +1210,7 @@ static void submodule_push_check(const char *path, const char *head,\n \tstrvec_push(&cp.args, remote->name);\n \n \tfor (i = 0; i < rs->nr; i++)\n-\t\tstrvec_push(&cp.args, rs->raw[i]);\n+\t\tstrvec_push(&cp.args, rs->items[i].raw);\n \n \tprepare_submodule_repo_env(&cp.env);\n \tcp.git_cmd = 1;\n-- \n2.47.0.508.g57228aee23\n"},{"id":"507126","messageId":"xmqqttcc4x5u.fsf@gitster.g","threadId":"62481","inReplyTo":"20241112083937.GC3529122@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] refspec: store raw refspecs inside refspec_item","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-12T09:30:05Z","receivedAt":"2024-11-12T09:30:07Z","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> The refspec struct keeps two matched arrays: one for the refspec_item\n> structs and one for the original raw refspec strings. The main reason\n> for this is that there are other users of refspec_item that do not care\n> about the raw strings. But it does make managing the refspec struct\n> awkward, as we must keep the two arrays in sync. This has led to bugs in\n> the past (both leaks and double-frees).\n\nYeah, I was wondering about this while reading [2/3], and it is very\nsatisfying to see that the concluding step for the series ends in\nthis change.\n\nThanks.\n"}]}