{"thread":{"id":"64169","subject":"[BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","startedAt":"2025-09-19T10:18:28Z","lastAt":"2025-09-23T00:48:11Z","messageCount":26,"participants":["Lauri Niskanen","Kristoffer Haugsbakk","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"526768","messageId":"CAMCKZdV+ASXAhYXaTdtB=7YZprCxFUjwEsqQP7i_ccOwx8Lo6Q@mail.gmail.com","threadId":"64169","inReplyTo":null,"subject":"[BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Lauri Niskanen","fromEmail":"ape@ape3000.com","sentAt":"2025-09-19T10:18:15Z","receivedAt":"2025-09-19T10:18:28Z","isPatch":false,"sender":{"key":"ape@ape3000.com","avatar":null},"body":"What did you do before the bug happened?\n\nCreate a repo and a stash entry, then invoke `git stash show -p` with\nan invalid option:\n\ngit init repro\ncd repro\ntouch a\ngit add a\ngit commit -m init\necho x >> a\ngit stash\ngit stash show -p --invalid\n\n\nWhat did you expect to happen?\n\nGit should print a usage / option error and exit cleanly without crashing.\n\n\nWhat happened instead?\n\nfree(): double free detected in tcache 2\n\n\nGDB backtrace:\n#0 __pthread_kill_implementation (threadid=,\nsigno=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44\n#1 0x00007ffff7c98a13 in __pthread_kill_internal (threadid=, signo=6) at pthread_kill.c:89\n#2 0x00007ffff7c3e410 in __GI_raise (sig=sig@entry=6) at\n../sysdeps/posix/raise.c:26\n#3 0x00007ffff7c2557a in __GI_abort () at abort.c:77\n#4 0x00007ffff7c26613 in __libc_message_impl\n(fmt=fmt@entry=0x7ffff7db4355 \"%s\\n\") at\n../sysdeps/posix/libc_fatal.c:138\n#5 0x00007ffff7ca2d65 in malloc_printerr\n(str=str@entry=0x7ffff7db7d78 \"free(): double free detected in tcache\n2\") at malloc.c:5892\n#6 0x00007ffff7ca82e8 in tcache_double_free_verify (e=) at malloc.c:3350\n#7 0x00007ffff7ca80a5 in __GI___libc_free (mem=) at\nmalloc.c:3547\n#8 0x00005555558570ce in strvec_clear (array=0x7fffffffc580) at\n/usr/src/debug/git/git/strvec.c:134\n#9 0x000055555567d9f9 in show_stash (argc=,\nargv=, prefix=, repo=) at\nbuiltin/stash.c:1047\n#10 0x0000555555686f25 in cmd_stash (argc=3, argv=0x5555559bd260,\nprefix=0x0, repo=0x555555992ae0 ) at\nbuiltin/stash.c:2410\n#11 0x000055555555fd43 in run_builtin (p=0x555555984698\n, argc=, argv=,\nrepo=0x555555992ae0 ) at\n/usr/src/debug/git/git/git.c:480\n#12 handle_builtin (args=args@entry=0x7fffffffe1d0) at\n/usr/src/debug/git/git/git.c:746\n#13 0x00005555555608a2 in run_argv (args=0x7fffffffe1d0) at\n/usr/src/debug/git/git/git.c:813\n#14 cmd_main (argc=, argv=) at\n/usr/src/debug/git/git/git.c:953\n#15 0x000055555555d784 in main (argc=5, argv=0x7fffffffe518) at\n/usr/src/debug/git/git/common-main.c:9\n\nValgrind:\n==2507916== Memcheck, a memory error detector\n==2507916== Copyright (C) 2002-2024, and GNU GPL'd, by Julian Seward et al.\n==2507916== Using Valgrind-3.25.1 and LibVEX; rerun with -h for copyright info\n==2507916== Command: git stash show -p --invalid\n==2507916==\n==2507916== Invalid free() / delete / delete[] / realloc()\n==2507916== at 0x4CB18EF: free (vg_replace_malloc.c:989)\n==2507916== by 0x43030CD: strvec_clear (strvec.c:134)\n==2507916== by 0x41299F8: show_stash.lto_priv.0 (stash.c:1047)\n==2507916== by 0x4132F24: cmd_stash (stash.c:2410)\n==2507916== by 0x400BD42: UnknownInlinedFun (git.c:480)\n==2507916== by 0x400BD42: handle_builtin (git.c:746)\n==2507916== by 0x400C8A1: UnknownInlinedFun (git.c:813)\n==2507916== by 0x400C8A1: cmd_main (git.c:953)\n==2507916== by 0x4009783: main (common-main.c:9)\n==2507916== Address 0x4ff4540 is 0 bytes inside a block of size 8 free'd\n==2507916== at 0x4CB18EF: free (vg_replace_malloc.c:989)\n==2507916== by 0x43030CD: strvec_clear (strvec.c:134)\n==2507916== by 0x41299F8: show_stash.lto_priv.0 (stash.c:1047)\n==2507916== by 0x4132F24: cmd_stash (stash.c:2410)\n==2507916== by 0x400BD42: UnknownInlinedFun (git.c:480)\n==2507916== by 0x400BD42: handle_builtin (git.c:746)\n==2507916== by 0x400C8A1: UnknownInlinedFun (git.c:813)\n==2507916== by 0x400C8A1: cmd_main (git.c:953)\n==2507916== by 0x4009783: main (common-main.c:9)\n==2507916== Block was alloc'd at\n==2507916== at 0x4CAE7A8: malloc (vg_replace_malloc.c:446)\n==2507916== by 0x4E6E2AF: strdup (strdup.c:42)\n==2507916== by 0x4129624: UnknownInlinedFun (wrapper.c:43)\n==2507916== by 0x4129624: UnknownInlinedFun (strvec.c:25)\n==2507916== by 0x4129624: show_stash.lto_priv.0 (stash.c:994)\n==2507916== by 0x4132F24: cmd_stash (stash.c:2410)\n==2507916== by 0x400BD42: UnknownInlinedFun (git.c:480)\n==2507916== by 0x400BD42: handle_builtin (git.c:746)\n==2507916== by 0x400C8A1: UnknownInlinedFun (git.c:813)\n==2507916== by 0x400C8A1: cmd_main (git.c:953)\n==2507916== by 0x4009783: main (common-main.c:9)\n==2507916==\nusage: git stash show [-u | --include-untracked | --only-untracked]\n[] []\n\n -u, --[no-]include-untracked\n include untracked files in the stash\n --only-untracked only show untracked files in the stash\n\n==2507916==\n==2507916== HEAP SUMMARY:\n==2507916== in use at exit: 696,349 bytes in 371 blocks\n==2507916== total heap usage: 750 allocs, 380 frees, 1,221,944 bytes allocated\n==2507916==\n==2507916== LEAK SUMMARY:\n==2507916== definitely lost: 3 bytes in 1 blocks\n==2507916== indirectly lost: 0 bytes in 0 blocks\n==2507916== possibly lost: 120 bytes in 3 blocks\n==2507916== still reachable: 696,226 bytes in 367 blocks\n==2507916== suppressed: 0 bytes in 0 blocks\n==2507916== Rerun with --leak-check=full to see details of leaked memory\n==2507916==\n==2507916== For lists of detected and suppressed errors, rerun with: -s\n==2507916== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)\n\n\n[System Info]\ngit version:\ngit version 2.51.0\ncpu: x86_64\nbuilt from commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nlibcurl: 8.15.0\nOpenSSL: OpenSSL 3.5.2 5 Aug 2025\nzlib-ng: 2.2.5\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\ndefault-ref-format: files\ndefault-hash: sha1\nuname: Linux 6.16.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 11 Sep 2025\n17:42:36 +0000 x86_64\ncompiler info: gnuc: 15.2\nlibc info: glibc: 2.42\n$SHELL (typically, interactive shell): /usr/bin/zsh\n\n\n[Enabled Hooks]\n\n\n--\nLauri Niskanen\n"},{"id":"526769","messageId":"1321ff39-6f09-426a-aa75-939ef4e1ad93@app.fastmail.com","threadId":"64169","inReplyTo":"CAMCKZdV+ASXAhYXaTdtB=7YZprCxFUjwEsqQP7i_ccOwx8Lo6Q@mail.gmail.com","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2025-09-19T13:11:41Z","receivedAt":"2025-09-19T13:12:05Z","isPatch":false,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Fri, Sep 19, 2025, at 12:18, Lauri Niskanen wrote:\n> What did you do before the bug happened?\n>\n> Create a repo and a stash entry, then invoke `git stash show -p` with\n> an invalid option:\n>\n> git init repro\n> cd repro\n> touch a\n> git add a\n> git commit -m init\n> echo x >> a\n> git stash\n> git stash show -p --invalid\n>\n>\n> What did you expect to happen?\n>\n> Git should print a usage / option error and exit cleanly without crashing.\n>\n>\n> What happened instead?\n>\n> free(): double free detected in tcache 2\n\nThis bisects to 748bd094 (builtin/stash: fix leak in `show_stash()`,\n2024-06-11) for me.\n\n> GDB backtrace:\n>[snip]\n\n-- \nKristoffer Haugsbakk\n"},{"id":"526787","messageId":"xmqq4isy77qr.fsf@gitster.g","threadId":"64169","inReplyTo":"1321ff39-6f09-426a-aa75-939ef4e1ad93@app.fastmail.com","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-19T16:00:12Z","receivedAt":"2025-09-19T16:00:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n>> What happened instead?\n>>\n>> free(): double free detected in tcache 2\n>\n> This bisects to 748bd094 (builtin/stash: fix leak in `show_stash()`,\n> 2024-06-11) for me.\n\nGood eyes, both of you.\n\nThis comes from the fact that the program calls setup_revisions() on\nthe \"rest of the command line args\" and have it parse as much as it\nunderstands, while leaving the remainder that it did not understand\nthere, with this call.\n\n\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n\tif (argc > 1)\n\t\tgoto usage;\n\nIf you give \"git stash show -p --no-such-option\", revision_args\nbefore setup_revisions() is called would have\n\n .v = { \"show\", \"-p\", \"--no-such-option\", NULL }\n\nand upon returning from the call, we would have\n\n .v = { \"show\", \"--no-such-option\", \"--no-such-option\", NULL }\n\nwith argc == 2. Note that .nr is only passed by value, so across\nthe call to setup_revisions(), it will stay .nr == 3.\n\nThis mongering of .v[] is because the callee parses out and removes\n\"-p\" as an argv element that it understood shifting the remainder in\nthe array, with the expectation that the caller would use the array\nit passed (i.e. .v[]) and the return value from the call (i.e. 2 in\nargc) for further processing. It is not getting the strvec and no\naccess to .nr to adjust, so this is close to the best it can do.\n\nSo there are two ways to fix this.\n\nThe easier, more performant, and closer to the original design\naround the revisions API is to do this:\n\ndiff --git c/builtin/stash.c w/builtin/stash.c\nindex f5ddee5c7f..b6312b1b70 100644\n--- c/builtin/stash.c\n+++ w/builtin/stash.c\n@@ -1016,6 +1016,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n \t}\n \n \targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n+\tfor (i = argc; i < revision_args.nr; i++)\n+\t\trevision_args.v[i] = NULL;\n \tif (argc > 1)\n \t\tgoto usage;\n \tif (!rev.diffopt.output_format) {\n\nA less performant but may in the longer term safer alternative is to\nchange the caller-callee contract around setup_revisions() so that\nthe later \"unused\" slots in the argv array is NULLed before\nreturning to the caller, i.e. instead of leaving\n\n .v = { \"show\", \"--no-such-option\", \"--no-such-option\", NULL }\n\nin the revision_args.v[] array, teach setup_revisions() to leave\n\n .v = { \"show\", \"--no-such-option\", NULL, NULL }\n\nthere (again, we cannot do anything about .nr that is only available\nto the caller).\n"},{"id":"526788","messageId":"20250919164820.GA24939@coredump.intra.peff.net","threadId":"64169","inReplyTo":"xmqq4isy77qr.fsf@gitster.g","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T16:48:20Z","receivedAt":"2025-09-19T16:48:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 19, 2025 at 09:00:12AM -0700, Junio C Hamano wrote:\n\n> The easier, more performant, and closer to the original design\n> around the revisions API is to do this:\n> \n> diff --git c/builtin/stash.c w/builtin/stash.c\n> index f5ddee5c7f..b6312b1b70 100644\n> --- c/builtin/stash.c\n> +++ w/builtin/stash.c\n> @@ -1016,6 +1016,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n> \t}\n> \n> \targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n> +\tfor (i = argc; i < revision_args.nr; i++)\n> +\t\trevision_args.v[i] = NULL;\n> \tif (argc > 1)\n> \t\tgoto usage;\n> \tif (!rev.diffopt.output_format) {\n\nI think we'll have leaked the string holding \"-p\" in this instance,\nthough. We probably need to pass in a setup_revision_opt struct with its\nfree_removed_argv_elements flag set.\n\nThat's true even without your patch, too, of course. I'm mildly\nsurprised that the test suite doesn't hit this in leak-checking mode,\nsince it is a problem any time we rearrange argv. E.g., I think:\n\n git stash show -p --\n\nleaks (I was surprised that \"stash show -p --stat\" didn't leak, but it\ndoesn't seem to rearrange?).\n\nAnother interesting thing about your patch above is that it fills the\nstrvec with a bunch of NULL entries. Which happens to work, because\nfree(NULL) is a noop, but I think may be subtly violating assumptions\nmade about strvecs. Probably:\n\n revision_args.nr = setup_revisions(...);\n\nfits my mental model better, though that is violating a different strvec\ninvariant now (that the .v[.nr] is always NULL). I think\nsetup_revisions() is a little sloppy not to set argv[argc] to NULL\nitself.\n\n> A less performant but may in the longer term safer alternative is to\n> change the caller-callee contract around setup_revisions() so that\n> the later \"unused\" slots in the argv array is NULLed before\n> returning to the caller, i.e. instead of leaving\n> \n> .v = { \"show\", \"--no-such-option\", \"--no-such-option\", NULL }\n> \n> in the revision_args.v[] array, teach setup_revisions() to leave\n> \n> .v = { \"show\", \"--no-such-option\", NULL, NULL }\n> \n> there (again, we cannot do anything about .nr that is only available\n> to the caller).\n\nI think we should consider a fix like this. Grepping for the\nfree_removed_argv_elements option, there are a few other spots that\ncorrectly use that flag, but aren't updating the strvec argc. E.g.,\nbisect_rev_setup(). So they're going to run into the same problem.\n\nI wonder if the best solution is a setup_revisions() wrapper for strvecs\nthat will:\n\n - turn on the free_removed_argv_elements option automatically\n\n - collect the return value of setup_revisions() and use it to fix\n the .nr field of the strvec\n\n - restore the NULL invariant at the end of the array (though I would\n also be happy if setup_revisions() just did this itself)\n\n-Peff\n"},{"id":"526789","messageId":"xmqqldma5qha.fsf@gitster.g","threadId":"64169","inReplyTo":"xmqq4isy77qr.fsf@gitster.g","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-19T16:58:25Z","receivedAt":"2025-09-19T16:58:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> So there are two ways to fix this.\n>\n> The easier, more performant, and closer to the original design\n> around the revisions API is to do this:\n>\n> diff --git c/builtin/stash.c w/builtin/stash.c\n> index f5ddee5c7f..b6312b1b70 100644\n> --- c/builtin/stash.c\n> +++ w/builtin/stash.c\n> @@ -1016,6 +1016,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n> \t}\n> \n> \targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n> +\tfor (i = argc; i < revision_args.nr; i++)\n> +\t\trevision_args.v[i] = NULL;\n> \tif (argc > 1)\n> \t\tgoto usage;\n> \tif (!rev.diffopt.output_format) {\n>\n> A less performant but may in the longer term safer alternative is to\n> change the caller-callee contract around setup_revisions() so that\n> the later \"unused\" slots in the argv array is NULLed before\n> returning to the caller, i.e. instead of leaving\n>\n> .v = { \"show\", \"--no-such-option\", \"--no-such-option\", NULL }\n>\n> in the revision_args.v[] array, teach setup_revisions() to leave\n>\n> .v = { \"show\", \"--no-such-option\", NULL, NULL }\n>\n> there (again, we cannot do anything about .nr that is only available\n> to the caller).\n\nFor completeness, here is how the other approach may look like, but\nI have made my share of off-by-one mistakes all over the place over\nthe ears, so somebody else needs to lend an eyeball and check it for\nsanity.\n\n----- >8 -----\nSubject: revisions: do not leave duplicated cruft from setup_revisions()\n\nThe API contract between setup_revisions() and its callers is that\nthe function gets parameters, parses what it can\nrecognise, and leaves the remainder in argv[] (shifting them to the\nhead, and return the number of arguments left. The caller then is\nsupposed to look at only the early part of argv[] it passed to the\nfunction, up to the argc returned by the function.\n\nBecause by contract the caller is not supposed to touch the\nremainder of the original argv[], the function left them intact. If\nthe incoming parameter were\n\n argc = 3, argv[] = { \"A\", \"-p\", \"--unknown\", NULL }\n\nand only \"-p\" is what the function understands, the function would\nreturn\n\n argc = 2, argv[] = { \"A\", \"--unknown\", , }\n\nto the caller, and it is fine as long as the caller would not look\nbeyond what it is allowed to see. In practice, these \nslots has the original pointer given by the caller, which meant that\nthe same instance of string \"--unknown\" appears at argv[1] as it\ngot copied from argv[2], and argv[2] is not cleared.\n\nRecently however this bit us when the pair is\nprepared in a strvec instance. When trying to clear the strvec\nthat was passed to setup_revisions(), because some later array\nelements are duplicates of earlier elements, strvec_clear() ended up\nfreeing the same string twice X-<.\n\nWe could fix individual callers (in this particular case, letting\nstrvec_clear() touch the original array .v[] beyond the updated argc\nreturned by setup_revisions() is a bug in the caller), but I see no\ngood reason to return argv[] array after modifying it without\ncleaning up the later slots (other than the fact that the callee\nsaves a few cycles because it does not have to bother cleaning array\nslots that no caller is supposed to touch, that is).\n\nTeach the function to assign NULLs to the slot the caller is not\nsupposed to see to help avoiding this class of bugs in the future.\n\nSigned-off-by: Junio C Hamano \n---\n revision.c | 6 ++++++\n t/t3903-stash.sh | 5 +++++\n 2 files changed, 11 insertions(+)\n\ndiff --git c/revision.c w/revision.c\nindex 6ba8f67054..93948e7dc6 100644\n--- c/revision.c\n+++ w/revision.c\n@@ -3174,6 +3174,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevs->show_notes_given = 1;\n \t}\n \n+\t/*\n+\t * NULL out the leftover args we did not understand, which has\n+\t * shallow copies in earlier slots in the array.\n+\t */\n+\twhile (left < argc--)\n+\t\targv[argc] = NULL;\n \treturn left;\n }\n \ndiff --git c/t/t3903-stash.sh w/t/t3903-stash.sh\nindex 0bb4648e36..dd70deb3b3 100755\n--- c/t/t3903-stash.sh\n+++ w/t/t3903-stash.sh\n@@ -69,6 +69,11 @@ test_expect_success 'stash some dirty working directory' '\n \tsetup_stash\n '\n \n+test_expect_success 'controlled error return on unrecognized option' '\n+\ttest_expect_code 129 git stash show -p --no-such 2>usage &&\n+\tgrep -e \"^usage: git stash show\" usage\n+'\n+\n cat >expect < writes:\n\n> I think we'll have leaked the string holding \"-p\" in this instance,\n> though. We probably need to pass in a setup_revision_opt struct with its\n> free_removed_argv_elements flag set.\n>\n> That's true even without your patch, too, of course.\n\nYeah, while I was preparing the \"alternative\", I noticed that option\nbeing paid attention to by the code, but you are right. Anybody who\npasses strvec (which owns its contents, unlike the traditional \"we\ngot this argv[] from the operating system\" callers) needs to flip\nthat bit set, or they would leak.\n\n> I'm mildly\n> surprised that the test suite doesn't hit this in leak-checking mode,\n> since it is a problem any time we rearrange argv. E.g., I think:\n>\n> git stash show -p --\n>\n> leaks (I was surprised that \"stash show -p --stat\" didn't leak, but it\n> doesn't seem to rearrange?).\n\nYeah.\n\n> I wonder if the best solution is a setup_revisions() wrapper for strvecs\n> that will:\n>\n> - turn on the free_removed_argv_elements option automatically\n>\n> - collect the return value of setup_revisions() and use it to fix\n> the .nr field of the strvec\n>\n> - restore the NULL invariant at the end of the array (though I would\n> also be happy if setup_revisions() just did this itself)\n\nThat would be nice. I only did the third one in my \"alternative\"\npatch I sent earlier.\n"},{"id":"526791","messageId":"20250919172007.GA59895@coredump.intra.peff.net","threadId":"64169","inReplyTo":"xmqqldma5qha.fsf@gitster.g","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T17:20:07Z","receivedAt":"2025-09-19T17:20:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 19, 2025 at 09:58:25AM -0700, Junio C Hamano wrote:\n\n> For completeness, here is how the other approach may look like, but\n> I have made my share of off-by-one mistakes all over the place over\n> the ears, so somebody else needs to lend an eyeball and check it for\n> sanity.\n\nI _thought_ there was an off-by-one at first, but I think what you have\nis correct:\n\n> +\t/*\n> +\t * NULL out the leftover args we did not understand, which has\n> +\t * shallow copies in earlier slots in the array.\n> +\t */\n> +\twhile (left < argc--)\n> +\t\targv[argc] = NULL;\n> \treturn left;\n\nWe definitely want argv[left] to be NULL, which I thought at first did\nnot happen because of the \"<\". But because it is post-increment that\nhappens in the loop condition, it works.\n\nI probably would have written:\n\n while (left <= argc)\n\targv[argc--] = NULL;\n\nwhich I think is the same (but I didn't test it, so it probably does\nhave an off-by-one!).\n\nBut really, I do not know that we need to NULL the whole thing. We have\ngiven the caller the reduced argc. The only argv invariant we are\nviolating is that argv[argc] should be NULL (or in this case,\nargv[left]). Anything after argv+left should be considered\nuninitialized. So just:\n\n argv[left] = NULL;\n\nwould be enough, I'd think.\n\n> diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh\n> index 0bb4648e36..dd70deb3b3 100755\n> --- c/t/t3903-stash.sh\n> +++ w/t/t3903-stash.sh\n> @@ -69,6 +69,11 @@ test_expect_success 'stash some dirty working directory' '\n> \tsetup_stash\n> '\n> \n> +test_expect_success 'controlled error return on unrecognized option' '\n> +\ttest_expect_code 129 git stash show -p --no-such 2>usage &&\n> +\tgrep -e \"^usage: git stash show\" usage\n> +'\n\nThis passes now, but fails with SANITIZE=leak. Along with a bunch of\nother tests, as we are now overwriting entries in strvecs with NULL, so\nit has no opportunity to free them. We need to respect the\nsetup_revision_opt to free.\n\nI'm working up a few alternative patches.\n\n-Peff\n"},{"id":"526795","messageId":"xmqqbjn65mxa.fsf@gitster.g","threadId":"64169","inReplyTo":"20250919172007.GA59895@coredump.intra.peff.net","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-19T18:15:13Z","receivedAt":"2025-09-19T18:15:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But really, I do not know that we need to NULL the whole thing. We have\n> given the caller the reduced argc. The only argv invariant we are\n> violating is that argv[argc] should be NULL (or in this case,\n> argv[left]). Anything after argv+left should be considered\n> uninitialized. So just:\n>\n> argv[left] = NULL;\n>\n> would be enough, I'd think.\n\nEven when strvec was passed and more than one element was eaten\nafter parsing? strvec_clear() goes by .nr not stopping at the first\nNULL IIRC.\n\n"},{"id":"526808","messageId":"20250919195626.GA2750626@coredump.intra.peff.net","threadId":"64169","inReplyTo":"xmqqbjn65mxa.fsf@gitster.g","subject":"Re: [BUG] git stash show -p with invalid option aborts with double-free in show_stash() (strvec_clear)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T19:56:26Z","receivedAt":"2025-09-19T19:56:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 19, 2025 at 11:15:13AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But really, I do not know that we need to NULL the whole thing. We have\n> > given the caller the reduced argc. The only argv invariant we are\n> > violating is that argv[argc] should be NULL (or in this case,\n> > argv[left]). Anything after argv+left should be considered\n> > uninitialized. So just:\n> >\n> > argv[left] = NULL;\n> >\n> > would be enough, I'd think.\n> \n> Even when strvec was passed and more than one element was eaten\n> after parsing? strvec_clear() goes by .nr not stopping at the first\n> NULL IIRC.\n\nYes, there is a big can of worms here. ;) It turns out that many spots\nwith strvecs were relying on leaving these entries untouched, and so\nsetting any of them to NULL causes leaks.\n\nI think I've got it mostly worked out, but that's why I haven't sent\npatches yet. Stay tuned.\n\n-Peff\n"},{"id":"526827","messageId":"20250919223351.GA3906184@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919195626.GA2750626@coredump.intra.peff.net","subject":"[PATCH 0/6] fixing double-frees and leaks via setup_revisions()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:33:51Z","receivedAt":"2025-09-19T22:33:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 19, 2025 at 03:56:26PM -0400, Jeff King wrote:\n\n> > > But really, I do not know that we need to NULL the whole thing. We have\n> > > given the caller the reduced argc. The only argv invariant we are\n> > > violating is that argv[argc] should be NULL (or in this case,\n> > > argv[left]). Anything after argv+left should be considered\n> > > uninitialized. So just:\n> > >\n> > > argv[left] = NULL;\n> > >\n> > > would be enough, I'd think.\n> > \n> > Even when strvec was passed and more than one element was eaten\n> > after parsing? strvec_clear() goes by .nr not stopping at the first\n> > NULL IIRC.\n> \n> Yes, there is a big can of worms here. ;) It turns out that many spots\n> with strvecs were relying on leaving these entries untouched, and so\n> setting any of them to NULL causes leaks.\n> \n> I think I've got it mostly worked out, but that's why I haven't sent\n> patches yet. Stay tuned.\n\nOK, here's what I've come up with. It passes the regular test suite, as\nwell as with LSan, ASan, and UBSan. There were definitely some ugly\ngotchas in there.\n\nIt _almost_ makes me think that setup_revisions() should never munge\nargv at all, and should just produce a separate strvec of unknown\noptions. That keeps the concerns separate, and the extra allocation cast\nis trivial. But we'd have to modify every caller to handle the new world\norder.\n\nSo anyway, here it is:\n\n [1/6]: stash: tell setup_revisions() to free our allocated strings\n\n This fixes a real (albeit tiny) leak.\n\n [2/6]: revision: manage memory ownership of argv in setup_revisions()\n\n This fixes the double-free that started us off, but also addresses\n the related leak issues that our earlier discussion had.\n\n [3/6]: revision: add wrapper to setup_revisions() from a strvec\n [4/6]: treewide: use setup_revisions_from_strvec() when we have a strvec\n [5/6]: treewide: pass strvecs around for setup_revisions_from_strvec()\n\n The rest of this is optional. These three introduce a convenience\n wrapper, which I do think makes the code a little shorter and\n eliminates a possible footgun. But AFAIK it's not actually fixing\n any bugs.\n\n [6/6]: revision: retain argv NULL invariant in setup_revisions()\n\n This one does the NULL thing we discussed earlier for all callers.\n It turned up some interesting gotchas! One is that without the\n wrapper from patches 3-5, it introduces leaks for strvec users which\n assume (correctly) that setup_revisions() will never munge their\n argv or need to free anything. And two, lots of callers pass a NULL\n argv to setup_revisions. Yikes.\n\n bisect.c | 5 +---\n builtin/describe.c | 3 ++-\n builtin/pack-objects.c | 6 ++---\n builtin/rebase.c | 3 +--\n builtin/stash.c | 4 +--\n builtin/submodule--helper.c | 10 ++------\n http-push.c | 2 +-\n remote.c | 5 +---\n revision.c | 49 ++++++++++++++++++++++++++++++++++---\n revision.h | 2 ++\n sequencer.c | 7 +++---\n sequencer.h | 4 +--\n shallow.c | 4 +--\n shallow.h | 4 +--\n submodule.c | 2 +-\n t/t3903-stash.sh | 11 +++++++++\n upload-pack.c | 7 +++---\n 17 files changed, 86 insertions(+), 42 deletions(-)\n\n-Peff\n"},{"id":"526829","messageId":"20250919224027.GA594545@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919223351.GA3906184@coredump.intra.peff.net","subject":"[PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:40:27Z","receivedAt":"2025-09-19T22:40:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In \"git stash show\", we do a first pass of parsing our command line\noptions by splitting them into revision args and stash args. These are\nstored in strvecs, and we pass the revision args to setup_revisions().\n\nBut setup_revisions() may modify the argv we pass it, causing us to leak\nsome of the entries. In particular, if it sees a \"--\" string, that will\nbe dropped from argv. This is the same as other cases addressed by\nf92dbdbc6a (revisions API: don't leak memory on argv elements that need\nfree()-ing, 2022-08-02), and we should fix it the same way: by passing\nthe free_removed_argv_elements option to setup_revisions().\n\nI've added a test here which fails when built with SANITIZE=leak because\nit calls \"git stash show --\". This by itself is not a very\ninteresting invocation, because there is nothing after the \"--\", and\nthus the \"--\" is not really doing anything.\n\nBut I think the current parsing in show_stash() is a little\nquestionable. It splits the arguments into revision options and stash\noptions solely based on the presence of a leading dash, with no regard\nto \"--\" at all. So:\n\n git stash show -- foo\n\nwill take \"foo\" as a stash option before we even pass anything to\nsetup_revisions(). And something like:\n\n git stash show -- 1\n\nwill show stash@{1}. But I would expect anything after the \"--\" to be a\npathspec. So in this example it would show only the part of the diff\nthat touched \"foo\". And something like:\n\n git stash show -p 1 -- foo\n\nwould treat \"1\" as a stash and \"foo\" as a pathspec.\n\nThat may be something we want to fix, but I want to focus here on the\nleak-fixing without changing behavior. So this test is a little odd, but\ndoes what we want without locking us in to any particular behavior (we\nonly care that \"--\" by itself does not change the output nor leak).\n\nSigned-off-by: Jeff King \n---\nI wonder if anybody actually cares that \"git stash show -- foo\" will\ntreat \"foo\" as a stash. If not, then it would probably be a fairly easy\n#leftoverbits project to teach it to stop there and retain everything\nafter as a rev argument (which would then treat it like a pathspec).\n\n builtin/stash.c | 3 ++-\n t/t3903-stash.sh | 6 ++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex f5ddee5c7f..e5ab3c4cf5 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -957,6 +957,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n static int show_stash(int argc, const char **argv, const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n+\tstruct setup_revision_opt opt = { .free_removed_argv_elements = 1 };\n \tint i;\n \tint ret = -1;\n \tstruct stash_info info = STASH_INFO_INIT;\n@@ -1015,7 +1016,7 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n+\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);\n \tif (argc > 1)\n \t\tgoto usage;\n \tif (!rev.diffopt.output_format) {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 0bb4648e36..1c9e589bbe 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1741,4 +1741,10 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes\n \t)\n '\n \n+test_expect_success 'stash show handles --' '\n+\tgit stash show >expect &&\n+\tgit stash show -- >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.51.0.568.g6b54b97edf\n\n"},{"id":"526830","messageId":"20250919224556.GB594545@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919223351.GA3906184@coredump.intra.peff.net","subject":"[PATCH 2/6] revision: manage memory ownership of argv in setup_revisions()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:45:56Z","receivedAt":"2025-09-19T22:45:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The setup_revisions() function takes an argc/argv pair and consumes\narguments from it, returning a reduced argc count to the caller. But it\nmay also overwrite entries within the argv array, as it shifts unknown\noptions to the front of argv (so they can be found in the range of\n0..argc-1 after we return).\n\nFor a normal argc/argv coming from the operating system, this is OK.\nWe don't need to worry about memory ownership of the strings in those\nentries. But some callers pass in allocated strings from a strvec, and\nwe do need to care about those.\n\nWe faced a similar issue in f92dbdbc6a (revisions API: don't leak memory\non argv elements that need free()-ing, 2022-08-02), which added an\noption for callers to tell us that elements need to be freed. But the\nimplementation within setup_revisions() was incomplete. It only covered\nthe case of dropping \"--\", but not the movement of unknown options.\n\nWhen we shift argv entries around, we should free the elements we are\nabout to overwrite, so they are not leaked. For example, in:\n\n git stash show -p --invalid\n\nwe will pass this to setup_revisions():\n\n argc = 3, argv[] = { \"show\", \"-p\", \"--invalid\", NULL }\n\nwhich will then return:\n\n argc = 2, argv[] = { \"show\", \"--invalid\", \"--invalid\", NULL }\n\noverwriting the \"-p\" entry, which is leaked unless we free it at that\nmoment.\n\nYou can see in the output above another potential problem. We now have\ntwo copies of the \"--invalid\" string. If the caller does not respect the\nnew argc when free-ing the strings via strvec_clear(), we'll get a\ndouble-free. And git-stash suffers from this, and will crash with the\nabove command.\n\nSo it seems at first glance that the solution is to just assign the\nreduced argc to the strvec.nr field in the caller. Then it would stop\nafter freeing only any copied entries. But that's not always right\neither!\n\nRemember that we are reducing \"argc\" to account for elements we've\nconsumed. So if there isn't an invalid option, we'd turn:\n\n argc = 2, argv[] = { \"show\", \"-p\", NULL }\n\ninto:\n\n argc = 1, argv[] = { \"show\", \"-p\", NULL }\n\nIn that case strvec_clear() must keep looking past the shortened argc we\nreturn to find the original \"-p\" to free. It needs to use the original\nargc to do that.\n\nWe can solve this by turning our argv writes into strict moves, not\ncopies. When we shuffle an unknown option to the front, we'll overwrite\nits old position with NULL. That leaves an argv array that may have NULL\n\"holes\" in it.\n\nSo in the \"--invalid\" example above we get:\n\n argc = 2, argv[] = { \"show\", \"--invalid\", NULL, NULL }\n\nbut something like \"git stash -p --invalid -p\" would yield:\n\n argc = 3, argv[] = { \"show\", \"--invalid\", NULL, \"-p\", NULL }\n\nbecause we move \"--invalid\" to overwrite the first \"-p\", but the second\none is quietly consumed. But strvec_clear() can handle that fine (it\niterates over the \"nr\" field, and passing NULL to free() is OK).\n\nTo ease the implementation, I've introduced a helper function. It's a\nlittle hacky because it must take a double-pointer to set the old\nposition to NULL. Which in turn means we cannot pass \"&arg\", our local\nalias for the current entry we're parsing, but instead \"&argv[i]\", the\npointer in the original array. And to make it even more confusing, we\ndelegate some of this work to handle_revision_opt(), which is passed a\nsubset of the argv array, so is always working on \"&argv[0]\".\n\nLikewise, because handle_revision_opt() only receives the part of argv\nleft to parse, it receives the array to accumulate unknown options as a\nseparate unkc/unkv pair. But we're always working on the same argv\narray, so our strategy works fine. I suspect this would be a bit more\nobvious (and avoid some pointer cleverness) if all functions saw the\nfull argv array and worked with positions within it (and our new helper\nwould take two positions, a src and dst). But that would involve\nrefactoring handle_revision_opt(). I punted on that, as what's here is\nnot too ugly and is all contained within revision.c itself.\n\nThe new test demonstrates that \"git stash show -p --invalid\" no longer\ncrashes with a double-free (because we move instead of copy). And it\npasses with SANITIZE=leak because we free \"-p\" before overwriting.\n\nSigned-off-by: Jeff King \n---\n revision.c | 24 +++++++++++++++++++++---\n t/t3903-stash.sh | 5 +++++\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 6ba8f67054..6c140d969d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2321,6 +2321,24 @@ static timestamp_t parse_age(const char *arg)\n \treturn num;\n }\n \n+static void overwrite_argv(int *argc, const char **argv,\n+\t\t\t const char **value,\n+\t\t\t const struct setup_revision_opt *opt)\n+{\n+\t/*\n+\t * Detect the case when we are overwriting ourselves. The assignment\n+\t * itself would be a noop either way, but this lets us avoid corner\n+\t * cases around the free() and NULL operations.\n+\t */\n+\tif (*value != argv[*argc]) {\n+\t\tif (opt && opt->free_removed_argv_elements)\n+\t\t\tfree((char *)argv[*argc]);\n+\t\targv[*argc] = *value;\n+\t\t*value = NULL;\n+\t}\n+\t(*argc)++;\n+}\n+\n static int handle_revision_opt(struct rev_info *revs, int argc, const char **argv,\n \t\t\t int *unkc, const char **unkv,\n \t\t\t const struct setup_revision_opt* opt)\n@@ -2342,7 +2360,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t starts_with(arg, \"--branches=\") || starts_with(arg, \"--tags=\") ||\n \t starts_with(arg, \"--remotes=\") || starts_with(arg, \"--no-walk=\"))\n \t{\n-\t\tunkv[(*unkc)++] = arg;\n+\t\toverwrite_argv(unkc, unkv, &argv[0], opt);\n \t\treturn 1;\n \t}\n \n@@ -2706,7 +2724,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t} else {\n \t\tint opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);\n \t\tif (!opts)\n-\t\t\tunkv[(*unkc)++] = arg;\n+\t\t\toverwrite_argv(unkc, unkv, &argv[0], opt);\n \t\treturn opts;\n \t}\n \n@@ -3018,7 +3036,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tif (revs->disable_stdin) {\n-\t\t\t\t\targv[left++] = arg;\n+\t\t\t\t\toverwrite_argv(&left, argv, &argv[i], opt);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 1c9e589bbe..7ebeb057d3 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1747,4 +1747,9 @@ test_expect_success 'stash show handles --' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'controlled error return on unrecognized option' '\n+\ttest_expect_code 129 git stash show -p --invalid 2>usage &&\n+\tgrep -e \"^usage: git stash show\" usage\n+'\n+\n test_done\n-- \n2.51.0.568.g6b54b97edf\n\n"},{"id":"526831","messageId":"20250919224847.GC594545@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919223351.GA3906184@coredump.intra.peff.net","subject":"[PATCH 3/6] revision: add wrapper to setup_revisions() from a strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:48:47Z","receivedAt":"2025-09-19T22:48:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The setup_revisions() function was designed to take the argc/argv pair\nfrom the operating system. But we sometimes construct our own argv using\na strvec and pass that in. There are a few gotchas that callers need to\ndeal with here:\n\n 1. You should always pass the free_removed_argv_elements option via\n setup_revision_opt. Otherwise, entries may be leaked if\n setup_revisions() re-shuffles options.\n\n 2. After setup_revisions() returns, the strvec state is odd. We get a\n reduced argc from setup_revisions() telling us how many unknown\n options were left in place. Entries after that in argv may be\n retained, or may be NULL (depending on how the reshuffling\n happened). But the strvec's \"nr\" field still represents the\n original value, and some of the entries it thinks it is still\n storing may be NULL. Callers must be careful with how they access\n it.\n\nSome callers deal with (1), but not all. In practice they are OK because\nthey do not pass any options that would cause setup_revisions() to\nre-shuffle (namely unknown options which may be relayed from the user,\nand the use of the \"--\" separator). But it's probably a good idea to\nconsistently pass this option anyway to future-proof ourselves against\nthe details of setup_revisions() changing.\n\nNo callers address (2), though I don't think there any visible bugs.\nMost of them simply call strvec_clear() and never otherwise look at the\nresult. And in fact, if they naively set foo.nr to the argc returned by\nsetup_revisions(), that would cause leaks! Because setup_revisions()\ndoes not free consumed options[1], we have to leave the \"nr\" field of\nthe strvec at its original value to find and free them during\nstrvec_clear().\n\nSo I don't think there are any bugs to fix here, but we can make things\nsafer and simpler for callers. Let's introduce a helper function that\nsets the free_removed_argv_elements automatically and shrinks the strvec\nto represent the retained options afterwards (taking care to free the\nnow-obsolete entries).\n\nWe'll start by converting all of the call-sites which the\nfree_removed_argv_elements option. There should be no behavior change\nfor them, except that their \"shrunken\" entries are cleaned up\nimmediately, rather than waiting for a strvec_clear() call.\n\n[1] Arguably setup_revisions() should be doing this step for us if we\n told it to free removed options, but there are many existing callers\n which will be broken if it did. Introducing this helper is a\n possible first step towards that.\n\nSigned-off-by: Jeff King \n---\n bisect.c | 5 +----\n builtin/stash.c | 5 ++---\n builtin/submodule--helper.c | 10 ++--------\n remote.c | 5 +----\n revision.c | 19 +++++++++++++++++++\n revision.h | 2 ++\n 6 files changed, 27 insertions(+), 19 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex f24474542e..a6dc76b15c 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -674,9 +674,6 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \t\t\t const char *bad_format, const char *good_format,\n \t\t\t int read_paths)\n {\n-\tstruct setup_revision_opt opt = {\n-\t\t.free_removed_argv_elements = 1,\n-\t};\n \tint i;\n \n \trepo_init_revisions(r, revs, prefix);\n@@ -693,7 +690,7 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,\n \tif (read_paths)\n \t\tread_bisect_paths(rev_argv);\n \n-\tsetup_revisions(rev_argv->nr, rev_argv->v, revs, &opt);\n+\tsetup_revisions_from_strvec(rev_argv, revs, NULL);\n }\n \n static void bisect_common(struct rev_info *revs)\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex e5ab3c4cf5..3b473c4489 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -957,7 +957,6 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n static int show_stash(int argc, const char **argv, const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n-\tstruct setup_revision_opt opt = { .free_removed_argv_elements = 1 };\n \tint i;\n \tint ret = -1;\n \tstruct stash_info info = STASH_INFO_INIT;\n@@ -1016,8 +1015,8 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);\n-\tif (argc > 1)\n+\tsetup_revisions_from_strvec(&revision_args, &rev, NULL);\n+\tif (revision_args.nr > 1)\n \t\tgoto usage;\n \tif (!rev.diffopt.output_format) {\n \t\trev.diffopt.output_format = DIFF_FORMAT_PATCH;\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 07a1935cbe..fcd73abe53 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -616,9 +616,6 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \tstruct rev_info rev = REV_INFO_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *git_dir;\n-\tstruct setup_revision_opt opt = {\n-\t\t.free_removed_argv_elements = 1,\n-\t};\n \n \tif (validate_submodule_path(path) < 0)\n \t\tdie(NULL);\n@@ -655,7 +652,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,\n \n \trepo_init_revisions(the_repository, &rev, NULL);\n \trev.abbrev = 0;\n-\tsetup_revisions(diff_files_args.nr, diff_files_args.v, &rev, &opt);\n+\tsetup_revisions_from_strvec(&diff_files_args, &rev, NULL);\n \trun_diff_files(&rev, 0);\n \n \tif (!diff_result_code(&rev)) {\n@@ -1094,9 +1091,6 @@ static int compute_summary_module_list(struct object_id *head_oid,\n {\n \tstruct strvec diff_args = STRVEC_INIT;\n \tstruct rev_info rev;\n-\tstruct setup_revision_opt opt = {\n-\t\t.free_removed_argv_elements = 1,\n-\t};\n \tstruct module_cb_list list = MODULE_CB_LIST_INIT;\n \tint ret = 0;\n \n@@ -1114,7 +1108,7 @@ static int compute_summary_module_list(struct object_id *head_oid,\n \trepo_init_revisions(the_repository, &rev, info->prefix);\n \trev.abbrev = 0;\n \tprecompose_argv_prefix(diff_args.nr, diff_args.v, NULL);\n-\tsetup_revisions(diff_args.nr, diff_args.v, &rev, &opt);\n+\tsetup_revisions_from_strvec(&diff_args, &rev, NULL);\n \trev.diffopt.output_format = DIFF_FORMAT_NO_OUTPUT | DIFF_FORMAT_CALLBACK;\n \trev.diffopt.format_callback = submodule_summary_callback;\n \trev.diffopt.format_callback_data = &list;\ndiff --git a/remote.c b/remote.c\nindex 81d8fc017e..df9675cd33 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2143,9 +2143,6 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstruct object_id oid;\n \tstruct commit *ours, *theirs;\n \tstruct rev_info revs;\n-\tstruct setup_revision_opt opt = {\n-\t\t.free_removed_argv_elements = 1,\n-\t};\n \tstruct strvec argv = STRVEC_INIT;\n \n \t/* Cannot stat if what we used to build on no longer exists */\n@@ -2180,7 +2177,7 @@ static int stat_branch_pair(const char *branch_name, const char *base,\n \tstrvec_push(&argv, \"--\");\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n-\tsetup_revisions(argv.nr, argv.v, &revs, &opt);\n+\tsetup_revisions_from_strvec(&argv, &revs, NULL);\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \ndiff --git a/revision.c b/revision.c\nindex 6c140d969d..f50f5d8ea2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3195,6 +3195,25 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \treturn left;\n }\n \n+void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,\n+\t\t\t\t struct setup_revision_opt *opt)\n+{\n+\tstruct setup_revision_opt fallback_opt;\n+\tint ret;\n+\n+\tif (!opt) {\n+\t\tmemset(&fallback_opt, 0, sizeof(fallback_opt));\n+\t\topt = &fallback_opt;\n+\t}\n+\topt->free_removed_argv_elements = 1;\n+\n+\tret = setup_revisions(argv->nr, argv->v, revs, opt);\n+\n+\tfor (size_t i = ret; i < argv->nr; i++)\n+\t\tfree((char *)argv->v[i]);\n+\targv->nr = ret;\n+}\n+\n static void release_revisions_cmdline(struct rev_cmdline_info *cmdline)\n {\n \tunsigned int i;\ndiff --git a/revision.h b/revision.h\nindex 21e288c5ba..a28e349044 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -441,6 +441,8 @@ struct setup_revision_opt {\n };\n int setup_revisions(int argc, const char **argv, struct rev_info *revs,\n \t\t struct setup_revision_opt *);\n+void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,\n+\t\t\t\t struct setup_revision_opt *);\n \n /**\n * Free data allocated in a \"struct rev_info\" after it's been\n-- \n2.51.0.568.g6b54b97edf\n\n"},{"id":"526832","messageId":"20250919224907.GD594545@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919223351.GA3906184@coredump.intra.peff.net","subject":"[PATCH 4/6] treewide: use setup_revisions_from_strvec() when we have a strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:49:07Z","receivedAt":"2025-09-19T22:49:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit introduced a wrapper to make using setup_revisions()\nwith a strvec easier and safer. It converted spots that were already\ndoing most of what the wrapper did.\n\nLet's now convert spots where we were not setting up the\nfree_removed_argv_elements flag. As discussed in the previous commit,\nthis probably isn't fixing any bugs or leaks (since these sites wouldn't\ntrigger the re-shuffling of argv that causes them). This is mostly\nfuture-proofing us against setup_revisions() becoming more aggressive\nabout its re-shuffling.\n\nSigned-off-by: Jeff King \n---\n builtin/describe.c | 3 ++-\n http-push.c | 2 +-\n submodule.c | 2 +-\n 3 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 9f4e26d7ff..ffaf8d9f0a 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -580,7 +580,8 @@ static void describe_blob(const struct object_id *oid, struct strbuf *dst)\n \t\t NULL);\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n-\tif (setup_revisions(args.nr, args.v, &revs, NULL) > 1)\n+\tsetup_revisions_from_strvec(&args, &revs, NULL);\n+\tif (args.nr > 1)\n \t\tBUG(\"setup_revisions could not handle all args?\");\n \n \tif (prepare_revision_walk(&revs))\ndiff --git a/http-push.c b/http-push.c\nindex 91a5465afb..4c43ba3bc7 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -1941,7 +1941,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tstrvec_pushf(&commit_argv, \"^%s\",\n \t\t\t\t oid_to_hex(&ref->old_oid));\n \t\trepo_init_revisions(the_repository, &revs, setup_git_directory());\n-\t\tsetup_revisions(commit_argv.nr, commit_argv.v, &revs, NULL);\n+\t\tsetup_revisions_from_strvec(&commit_argv, &revs, NULL);\n \t\trevs.edge_hint = 0; /* just in case */\n \n \t\t/* Generate a list of objects that need to be pushed */\ndiff --git a/submodule.c b/submodule.c\nindex fff3c75570..35c55155f7 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -900,7 +900,7 @@ static void collect_changed_submodules(struct repository *r,\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \trepo_init_revisions(r, &rev, NULL);\n-\tsetup_revisions(argv->nr, argv->v, &rev, &s_r_opt);\n+\tsetup_revisions_from_strvec(argv, &rev, &s_r_opt);\n \twarn_on_object_refname_ambiguity = save_warning;\n \tif (prepare_revision_walk(&rev))\n \t\tdie(_(\"revision walk setup failed\"));\n-- \n2.51.0.568.g6b54b97edf\n\n"},{"id":"526833","messageId":"20250919225048.GE594545@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919223351.GA3906184@coredump.intra.peff.net","subject":"[PATCH 5/6] treewide: pass strvecs around for setup_revisions_from_strvec()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:50:48Z","receivedAt":"2025-09-19T22:50:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The previous commit converted callers of setup_revisions() with a strvec\nto use the safer and easier _from_strvec() variant.\n\nLet's now convert spots that don't directly have a strvec, but receive\nan argc/argv pair that eventually comes from one. We'll instead pass the\nstrvec down to the point where we call setup_revisions().\n\nThat makes these functions slightly less flexible if they were to grow\nother callers that don't use strvecs, but this rigidity is buying us\nsome safety. It is only safe to pass the free_removed_argv_elements\noption to setup_revisions() if we know the elements of argv/argc are\nallocated on the heap. That isn't communicated in the type system when\nwe are passed the bare elements. But if we get a strvec, we know that\nthe elements are allocated strings.\n\nAnd at any rate, each of these modified functions has only a single\ncaller (that has a strvec), so the loss of flexibility is unlikely to\never matter.\n\nSigned-off-by: Jeff King \n---\n builtin/pack-objects.c | 6 +++---\n builtin/rebase.c | 3 +--\n sequencer.c | 7 ++++---\n sequencer.h | 4 ++--\n shallow.c | 4 ++--\n shallow.h | 4 ++--\n upload-pack.c | 7 +++----\n 7 files changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1494afcf3d..5856b5f6bf 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -4650,7 +4650,7 @@ static void get_object_list_path_walk(struct rev_info *revs)\n \t\tdie(_(\"failed to pack objects via path-walk\"));\n }\n \n-static void get_object_list(struct rev_info *revs, int ac, const char **av)\n+static void get_object_list(struct rev_info *revs, struct strvec *argv)\n {\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n@@ -4660,7 +4660,7 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av)\n \tint save_warning;\n \n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, revs, &s_r_opt);\n+\tsetup_revisions_from_strvec(argv, revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n@@ -5229,7 +5229,7 @@ int cmd_pack_objects(int argc,\n \t\t\trevs.include_check = is_not_in_promisor_pack;\n \t\t\trevs.include_check_obj = is_not_in_promisor_pack_obj;\n \t\t}\n-\t\tget_object_list(&revs, rp.nr, rp.v);\n+\t\tget_object_list(&revs, &rp);\n \t\trelease_revisions(&revs);\n \t}\n \tcleanup_preferred_base();\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 67c0352bf8..c468828189 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -299,8 +299,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)\n \t\t\t oid_to_hex(&opts->restrict_revision->object.oid));\n \n \tret = sequencer_make_script(the_repository, &todo_list.buf,\n-\t\t\t\t make_script_args.nr, make_script_args.v,\n-\t\t\t\t flags);\n+\t\t\t\t &make_script_args, flags);\n \tif (ret) {\n \t\terror(_(\"could not generate todo list\"));\n \t\tgoto cleanup;\ndiff --git a/sequencer.c b/sequencer.c\nindex 9ae40a91b2..3d7909a399 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -6064,8 +6064,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,\n \treturn 0;\n }\n \n-int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n-\t\t\t const char **argv, unsigned flags)\n+int sequencer_make_script(struct repository *r, struct strbuf *out,\n+\t\t\t struct strvec *argv, unsigned flags)\n {\n \tchar *format = NULL;\n \tstruct pretty_print_context pp = {0};\n@@ -6106,7 +6106,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n \tpp.fmt = revs.commit_format;\n \tpp.output_encoding = get_log_output_encoding();\n \n-\tif (setup_revisions(argc, argv, &revs, NULL) > 1) {\n+\tsetup_revisions_from_strvec(argv, &revs, NULL);\n+\tif (argv->nr > 1) {\n \t\tret = error(_(\"make_script: unhandled options\"));\n \t\tgoto cleanup;\n \t}\ndiff --git a/sequencer.h b/sequencer.h\nindex 304ba4b4d3..719684c8a9 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -186,8 +186,8 @@ int sequencer_remove_state(struct replay_opts *opts);\n #define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)\n #define TODO_LIST_WARN_SKIPPED_CHERRY_PICKS (1U << 8)\n \n-int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,\n-\t\t\t const char **argv, unsigned flags);\n+int sequencer_make_script(struct repository *r, struct strbuf *out,\n+\t\t\t struct strvec *argv, unsigned flags);\n \n int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags,\n \t\t const char *shortrevisions, const char *onto_name,\ndiff --git a/shallow.c b/shallow.c\nindex ef3adb635f..d9cd4e219c 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -213,7 +213,7 @@ static void show_commit(struct commit *commit, void *data)\n * are marked with shallow_flag. The list of border/shallow commits\n * are also returned.\n */\n-struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n+struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,\n \t\t\t\t\t\t int shallow_flag,\n \t\t\t\t\t\t int not_shallow_flag)\n {\n@@ -232,7 +232,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, NULL);\n+\tsetup_revisions_from_strvec(argv, &revs, NULL);\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\ndiff --git a/shallow.h b/shallow.h\nindex 9bfeade93e..59d54d48e7 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -36,8 +36,8 @@ void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);\n \n struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n-struct commit_list *get_shallow_commits_by_rev_list(\n-\t\tint ac, const char **av, int shallow_flag, int not_shallow_flag);\n+struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,\n+\t\t\t\t\t\t int shallow_flag, int not_shallow_flag);\n int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n \t\t\t const struct oid_array *extra);\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex f78fabc1e1..1e87ae9559 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -913,13 +913,12 @@ static void deepen(struct upload_pack_data *data, int depth)\n }\n \n static void deepen_by_rev_list(struct upload_pack_data *data,\n-\t\t\t int ac,\n-\t\t\t const char **av)\n+\t\t\t struct strvec *argv)\n {\n \tstruct commit_list *result;\n \n \tdisable_commit_graph(the_repository);\n-\tresult = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);\n+\tresult = get_shallow_commits_by_rev_list(argv, SHALLOW, NOT_SHALLOW);\n \tsend_shallow(data, result);\n \tfree_commit_list(result);\n \tsend_unshallow(data);\n@@ -955,7 +954,7 @@ static int send_shallow_list(struct upload_pack_data *data)\n \t\t\tstruct object *o = data->want_obj.objects[i].item;\n \t\t\tstrvec_push(&av, oid_to_hex(&o->oid));\n \t\t}\n-\t\tdeepen_by_rev_list(data, av.nr, av.v);\n+\t\tdeepen_by_rev_list(data, &av);\n \t\tstrvec_clear(&av);\n \t\tret = 1;\n \t} else {\n-- \n2.51.0.568.g6b54b97edf\n\n"},{"id":"526834","messageId":"20250919225146.GF594545@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919223351.GA3906184@coredump.intra.peff.net","subject":"[PATCH 6/6] revision: retain argv NULL invariant in setup_revisions()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T22:51:46Z","receivedAt":"2025-09-19T22:51:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In an argc/argv pair, the entry for argv[argc] is generally NULL. You\ncan iterate by counting up to argc, or by looking for the NULL entry in\nargv.\n\nWhen we pass such a pair to setup_revisions(), it shrinks argc to\naccount for the options we consumed and returns the result to the\ncaller. But it doesn't touch the entries after the reduced argc. So\nargv[argc] will be left pointing at some arbitrary entry rather than\nNULL.\n\nThis isn't the source of any known bugs, since all callers are aware of\nthe limitation and act accordingly. But it's a possible gotcha that may\nbe easy to miss.\n\nLet's set the new argv[argc] to NULL, taking care to free it if the\ncaller asked us to do so.\n\nIt is tempting to do likewise for all of the entries afterwards, too, as\nsome of them may also need to be freed (e.g., if coming from a strvec).\nBut doing so isn't entirely trivial, as we munge argc in the function\n(e.g., when we find \"--\" and move all of the entries after it into the\nprune_data list). It would be possible with some light refactoring, but\nit's probably not worth it. Nobody should ever look at them (they are\nbeyond the revised argc and past the NULL argv entry) outside of strvec\ncleanup, and setup_revisions_from_strvec() already handles this case.\n\nThere's one other interesting gotcha: many callers which do not want to\nprovide arguments just pass 0/NULL for argc/argv. We need to check for\nthis case before assigning the final NULL.\n\nSigned-off-by: Jeff King \n---\n revision.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex f50f5d8ea2..806a1c4c24 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3192,6 +3192,12 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevs->show_notes_given = 1;\n \t}\n \n+\tif (argv) {\n+\t\tif (opt && opt->free_removed_argv_elements)\n+\t\t\tfree((char *)argv[left]);\n+\t\targv[left] = NULL;\n+\t}\n+\n \treturn left;\n }\n \n-- \n2.51.0.568.g6b54b97edf\n"},{"id":"526837","messageId":"20250919230721.GA936044@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919225146.GF594545@coredump.intra.peff.net","subject":"Re: [PATCH 6/6] revision: retain argv NULL invariant in setup_revisions()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T23:07:21Z","receivedAt":"2025-09-19T23:07:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 19, 2025 at 06:51:46PM -0400, Jeff King wrote:\n\n> It is tempting to do likewise for all of the entries afterwards, too, as\n> some of them may also need to be freed (e.g., if coming from a strvec).\n> But doing so isn't entirely trivial, as we munge argc in the function\n> (e.g., when we find \"--\" and move all of the entries after it into the\n> prune_data list). It would be possible with some light refactoring, but\n> it's probably not worth it. Nobody should ever look at them (they are\n> beyond the revised argc and past the NULL argv entry) outside of strvec\n> cleanup, and setup_revisions_from_strvec() already handles this case.\n\nI _think_ that would probably look like this on top (with obvious\ninspiration from your earlier patch), but I don't know if it is\nworthwhile or not:\n\ndiff --git a/revision.c b/revision.c\nindex 806a1c4c24..96188ab4ad 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -3003,14 +3003,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\tseen_dashdash = 0;\n \t\tfor (i = 1; i < argc; i++) {\n \t\t\tconst char *arg = argv[i];\n+\t\t\tint j;\n \t\t\tif (strcmp(arg, \"--\"))\n \t\t\t\tcontinue;\n-\t\t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\t\tfree((char *)argv[i]);\n-\t\t\targv[i] = NULL;\n+\t\t\tfor (j = i; j < argc; j++) {\n+\t\t\t\tif (i != j)\n+\t\t\t\t\tstrvec_push(&prune_data, argv[j]);\n+\t\t\t\tif (opt && opt->free_removed_argv_elements)\n+\t\t\t\t\tfree((char *)argv[j]);\n+\t\t\t\targv[j] = NULL;\n+\t\t\t}\n \t\t\targc = i;\n-\t\t\tif (argv[i + 1])\n-\t\t\t\tstrvec_pushv(&prune_data, argv + i + 1);\n \t\t\tseen_dashdash = 1;\n \t\t\tbreak;\n \t\t}\n@@ -3192,10 +3195,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\trevs->show_notes_given = 1;\n \t}\n \n-\tif (argv) {\n+\twhile (left < argc--) {\n \t\tif (opt && opt->free_removed_argv_elements)\n-\t\t\tfree((char *)argv[left]);\n-\t\targv[left] = NULL;\n+\t\t\tfree((char *)argv[argc]);\n+\t\targv[argc] = NULL;\n \t}\n \n \treturn left;\n@@ -3205,19 +3208,14 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,\n \t\t\t\t struct setup_revision_opt *opt)\n {\n \tstruct setup_revision_opt fallback_opt;\n-\tint ret;\n \n \tif (!opt) {\n \t\tmemset(&fallback_opt, 0, sizeof(fallback_opt));\n \t\topt = &fallback_opt;\n \t}\n \topt->free_removed_argv_elements = 1;\n \n-\tret = setup_revisions(argv->nr, argv->v, revs, opt);\n-\n-\tfor (size_t i = ret; i < argv->nr; i++)\n-\t\tfree((char *)argv->v[i]);\n-\targv->nr = ret;\n+\targv->nr = setup_revisions(argv->nr, argv->v, revs, opt);\n }\n \n static void release_revisions_cmdline(struct rev_cmdline_info *cmdline)\n"},{"id":"526838","messageId":"20250919231127.GB936044@coredump.intra.peff.net","threadId":"64169","inReplyTo":"20250919225048.GE594545@coredump.intra.peff.net","subject":"Re: [PATCH 5/6] treewide: pass strvecs around for setup_revisions_from_strvec()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-19T23:11:27Z","receivedAt":"2025-09-19T23:11:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 19, 2025 at 06:50:48PM -0400, Jeff King wrote:\n\n> diff --git a/shallow.h b/shallow.h\n> index 9bfeade93e..59d54d48e7 100644\n> --- a/shallow.h\n> +++ b/shallow.h\n> @@ -36,8 +36,8 @@ void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);\n> \n> struct commit_list *get_shallow_commits(struct object_array *heads,\n> \t\t\t\t\tint depth, int shallow_flag, int not_shallow_flag);\n> -struct commit_list *get_shallow_commits_by_rev_list(\n> -\t\tint ac, const char **av, int shallow_flag, int not_shallow_flag);\n> +struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,\n> +\t\t\t\t\t\t int shallow_flag, int not_shallow_flag);\n> int write_shallow_commits(struct strbuf *out, int use_pack_protocol,\n> \t\t\t const struct oid_array *extra);\n> \n\nSorry, I missed a hdr-check complaint here. It needs:\n\ndiff --git a/shallow.h b/shallow.h\nindex 59d54d48e7..ad591bd139 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -7,6 +7,7 @@\n #include \"strbuf.h\"\n \n struct oid_array;\n+struct strvec;\n \n void set_alternate_shallow_file(struct repository *r, const char *path, int override);\n int register_shallow(struct repository *r, const struct object_id *oid);\n\non top.\n\n-Peff\n"},{"id":"526847","messageId":"CAPig+cRSQAs79oVmNqvfqhEr6K3T1FR13NwbE50A5CCMJ8RqSA@mail.gmail.com","threadId":"64169","inReplyTo":"20250919224847.GC594545@coredump.intra.peff.net","subject":"Re: [PATCH 3/6] revision: add wrapper to setup_revisions() from a strvec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-09-20T05:10:50Z","receivedAt":"2025-09-20T05:11:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 19, 2025 at 6:51 PM Jeff King wrote:\n> The setup_revisions() function was designed to take the argc/argv pair\n> from the operating system. But we sometimes construct our own argv using\n> a strvec and pass that in. There are a few gotchas that callers need to\n> deal with here:\n> [...]\n> We'll start by converting all of the call-sites which the\n> free_removed_argv_elements option. There should be no behavior change\n> for them, except that their \"shrunken\" entries are cleaned up\n> immediately, rather than waiting for a strvec_clear() call.\n\nThere is some grammatical problem with the first sentence of this\nparagraph which makes it difficult to decipher.\n\n> Signed-off-by: Jeff King \n"},{"id":"526848","messageId":"20250920054828.GA996396@coredump.intra.peff.net","threadId":"64169","inReplyTo":"CAPig+cRSQAs79oVmNqvfqhEr6K3T1FR13NwbE50A5CCMJ8RqSA@mail.gmail.com","subject":"Re: [PATCH 3/6] revision: add wrapper to setup_revisions() from a strvec","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-20T05:48:28Z","receivedAt":"2025-09-20T05:48:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 20, 2025 at 01:10:50AM -0400, Eric Sunshine wrote:\n\n> On Fri, Sep 19, 2025 at 6:51 PM Jeff King wrote:\n> > The setup_revisions() function was designed to take the argc/argv pair\n> > from the operating system. But we sometimes construct our own argv using\n> > a strvec and pass that in. There are a few gotchas that callers need to\n> > deal with here:\n> > [...]\n> > We'll start by converting all of the call-sites which the\n> > free_removed_argv_elements option. There should be no behavior change\n> > for them, except that their \"shrunken\" entries are cleaned up\n> > immediately, rather than waiting for a strvec_clear() call.\n> \n> There is some grammatical problem with the first sentence of this\n> paragraph which makes it difficult to decipher.\n\nUrgh, sorry. Should be:\n\n We'll start by converting all of the call-sites which use the\n free_removed_argv_elements option.\n\n-Peff\n"},{"id":"526942","messageId":"xmqq1pnywkwv.fsf@gitster.g","threadId":"64169","inReplyTo":"20250919224027.GA594545@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-22T15:45:36Z","receivedAt":"2025-09-22T15:45:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In \"git stash show\", we do a first pass of parsing our command line\n> options by splitting them into revision args and stash args. These are\n> stored in strvecs, and we pass the revision args to setup_revisions().\n>\n> But setup_revisions() may modify the argv we pass it, causing us to leak\n> some of the entries. In particular, if it sees a \"--\" string, that will\n> be dropped from argv. This is the same as other cases addressed by\n> f92dbdbc6a (revisions API: don't leak memory on argv elements that need\n> free()-ing, 2022-08-02), and we should fix it the same way: by passing\n> the free_removed_argv_elements option to setup_revisions().\n>\n> I've added a test here which fails when built with SANITIZE=leak because\n> it calls \"git stash show --\". This by itself is not a very\n> interesting invocation, because there is nothing after the \"--\", and\n> thus the \"--\" is not really doing anything.\n>\n> But I think the current parsing in show_stash() is a little\n> questionable. It splits the arguments into revision options and stash\n> options solely based on the presence of a leading dash, with no regard\n> to \"--\" at all. So:\n>\n> git stash show -- foo\n>\n> will take \"foo\" as a stash option before we even pass anything to\n> setup_revisions(). And something like:\n>\n> git stash show -- 1\n>\n> will show stash@{1}. But I would expect anything after the \"--\" to be a\n> pathspec. So in this example it would show only the part of the diff\n> that touched \"foo\". And something like:\n>\n> git stash show -p 1 -- foo\n>\n> would treat \"1\" as a stash and \"foo\" as a pathspec.\n>\n> That may be something we want to fix, but I want to focus here on the\n> leak-fixing without changing behavior. So this test is a little odd, but\n> does what we want without locking us in to any particular behavior (we\n> only care that \"--\" by itself does not change the output nor leak).\n\n\"git stash show\" gives a \"git diff --stat stash~ stash\" (i.e. the\nworktree relative to then-current-HEAD in diffstat format), and \"git\nstash show --\" (no other arguments) gives us \"git diff -p\" for the\nsame, it seems; this is with or without your patch.\n\nWe may care \"--\" by itself does not change the output, but it has\nalready been giving different output without your patch.\n\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 0bb4648e36..1c9e589bbe 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1741,4 +1741,10 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes\n> \t)\n> '\n> \n> +test_expect_success 'stash show handles --' '\n> +\tgit stash show >expect &&\n> +\tgit stash show -- >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n\nIn other words, this test should not pass if the stash stores\nsomething reasonable. The only case is when both \"git stash show\"\nand \"git stash show -p\" would have been silent.\n\nThe commit object pointed at by refs/stash before this test runs has\nthis curious thing.\n\n tree c5f56db0c5c4e40b68ffda4bee76b301c9cc3406\n parent a2a54b9cb0873c567fbab134c4b95020b9419f6c\n parent 182836fa1adbc25d81a137d4e7489cbd0bec744a\n parent f93620bcecd8173d3bd3ad4dff4e69e32ba6f278\n author A U Thor 1112912473 -0700\n committer C O Mitter 1112912473 -0700\n\n WIP on orphan2: a2a54b9 A\n\nThe trees of these three parents reveal why this test passes.\n\n c5f56db0c5c4e40b68ffda4bee76b301c9cc3406\n c5f56db0c5c4e40b68ffda4bee76b301c9cc3406\n adc18b651de078499a4acf1454fadb65352ed961\n\nIn other words, \"git diff refs/stash~$n refs/stash\" for n == 1 & n\n== 2 are empty and this stash entry exists only to record the\nuntracked cruft. Even \"git show -c refs/stash\" would stay silent\nfor a \"merge\" like this, and both \"git stash show\" and \"git stash\nshow -p\" would of course be empty.\n\nI do not think we want to drop this test (we do want the \"handles\nwithout leaking\" part of the test), but we should not expect the\noutput from these commands match.\n\nUnless we are aspiring to introduce a breaking change, that is [*].\n\nPerhaps create a new stash entry that does not show anything in this\ntest and drop that entry with test_when_finished before leaving it?\nOr just run \"git stash show --\" without any check on its output,\npossibly with a prereq that we are running under leak checker?\n\nI dunno.\n\nI only discovered this while merging this and another topic that\nhappen to touch the same t3903 into 'seen'.\n\nThanks.\n\n\n[Footnote]\n\n* I personally find the traditional behaviour nonsense and it may be\ncoming from the crappy command line parsing we have had forever, but\nI am sure people who wrote\n\n git stash show --\n git stash show --end-of-options\n\nout of \"principle\" in their scripts, and assumed that the patch\noutput is the norm for the command even though it should have been\ngiving diffstat, would be unhappy if we suddenly made them behave\nexactly like \"git stash show\" (nothing else on the command line).\n\n"},{"id":"526963","messageId":"20250922190555.GA2205919@coredump.intra.peff.net","threadId":"64169","inReplyTo":"xmqq1pnywkwv.fsf@gitster.g","subject":"Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-22T19:05:55Z","receivedAt":"2025-09-22T19:05:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2025 at 08:45:36AM -0700, Junio C Hamano wrote:\n\n> \"git stash show\" gives a \"git diff --stat stash~ stash\" (i.e. the\n> worktree relative to then-current-HEAD in diffstat format), and \"git\n> stash show --\" (no other arguments) gives us \"git diff -p\" for the\n> same, it seems; this is with or without your patch.\n\nOof, that is certainly unexpected. Ironically I had done all of the\nmanual testing with \"-p --\", because the point of this topic was looking\nat how we passed arguments to setup_revisions(). And then I simplified\nit when I added the test, because it seemed that the \"-p\" would just be\nnoise there.\n\nSo it is surprising that the test passes, but you explained that clearly\nbelow.\n\n> We may care \"--\" by itself does not change the output, but it has\n> already been giving different output without your patch.\n\nI think the difference is probably a bug, but one that is out of scope\nfor what this patch is trying to fix. So even if we wanted to fix it,\nI'd prefer not to deal with it here.\n\n> I do not think we want to drop this test (we do want the \"handles\n> without leaking\" part of the test), but we should not expect the\n> output from these commands match.\n\nYeah, I had originally written just:\n\n test_expect_success 'stash show -- does not leak' '\n\tgit stash show --\n '\n\nbut it felt funny, since the test is doing nothing in a build without\nSANITIZE=leak. If we are OK with that funniness, I can switch back to\nthat.\n\n> I only discovered this while merging this and another topic that\n> happen to touch the same t3903 into 'seen'.\n\nI'm glad you did. I would rather find out about it now while it is fresh\nin my mind, then 3 years from now when we all wonder what the heck is\ngoing on.\n\n> * I personally find the traditional behaviour nonsense and it may be\n> coming from the crappy command line parsing we have had forever, but\n> I am sure people who wrote\n> \n> git stash show --\n> git stash show --end-of-options\n> \n> out of \"principle\" in their scripts, and assumed that the patch\n> output is the norm for the command even though it should have been\n> giving diffstat, would be unhappy if we suddenly made them behave\n> exactly like \"git stash show\" (nothing else on the command line).\n\nYeah, I agree with both points (that the current behavior is nonsense,\nbut people may accidentally have been relying on it). I wouldn't feel\n_too_ bad about saying \"you were relying on nonsense, and we have fixed\nthe bug\" in this case. But I think it should be a separate topic (and\npossibly one that makes \"git stash show -- foo\" do something sensible).\n\n-Peff\n"},{"id":"526966","messageId":"xmqqikhath3d.fsf@gitster.g","threadId":"64169","inReplyTo":"20250922190555.GA2205919@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-22T19:36:22Z","receivedAt":"2025-09-22T19:36:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I had originally written just:\n>\n> test_expect_success 'stash show -- does not leak' '\n> \tgit stash show --\n> '\n>\n> but it felt funny, since the test is doing nothing in a build without\n> SANITIZE=leak. If we are OK with that funniness, I can switch back to\n> that.\n\nWe have a prerequisite for that. Very nice vehicle for\ndocumentation purposes, even though we do not care about a single\n\"stash show\" invocation for correctness or performance reasons.\n\nPerhaps I can squash the following in, unless you have other changes\nin mind.\n\n t/t3903-stash.sh | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git c/t/t3903-stash.sh w/t/t3903-stash.sh\nindex 7ebeb057d3..930c31e547 100755\n--- c/t/t3903-stash.sh\n+++ w/t/t3903-stash.sh\n@@ -1741,10 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes\n \t)\n '\n \n-test_expect_success 'stash show handles --' '\n-\tgit stash show >expect &&\n-\tgit stash show -- >actual &&\n-\ttest_cmp expect actual\n+test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' '\n+\tgit stash show --\n '\n \n test_expect_success 'controlled error return on unrecognized option' '\n"},{"id":"526982","messageId":"20250922202509.GE2205919@coredump.intra.peff.net","threadId":"64169","inReplyTo":"xmqqikhath3d.fsf@gitster.g","subject":"Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-22T20:25:09Z","receivedAt":"2025-09-22T20:25:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2025 at 12:36:22PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, I had originally written just:\n> >\n> > test_expect_success 'stash show -- does not leak' '\n> > \tgit stash show --\n> > '\n> >\n> > but it felt funny, since the test is doing nothing in a build without\n> > SANITIZE=leak. If we are OK with that funniness, I can switch back to\n> > that.\n> \n> We have a prerequisite for that. Very nice vehicle for\n> documentation purposes, even though we do not care about a single\n> \"stash show\" invocation for correctness or performance reasons.\n\nAh, nice. It crossed my mind that we could try marking it as such, but I\nforgot (or never knew) that we already had this prereq available. I\nguess it was mainly used as \"!SANITIZE_LEAK\" before for suppressing\nleaks.\n\n> Perhaps I can squash the following in, unless you have other changes\n> in mind.\n\nThanks, that looks perfect for the code change. But we probably need to\nupdate the discussion of the test in the commit message. Here's what I\ncame up with (replacing patch 1):\n\n-- >8 --\nSubject: [PATCH] stash: tell setup_revisions() to free our allocated strings\n\nIn \"git stash show\", we do a first pass of parsing our command line\noptions by splitting them into revision args and stash args. These are\nstored in strvecs, and we pass the revision args to setup_revisions().\n\nBut setup_revisions() may modify the argv we pass it, causing us to leak\nsome of the entries. In particular, if it sees a \"--\" string, that will\nbe dropped from argv. This is the same as other cases addressed by\nf92dbdbc6a (revisions API: don't leak memory on argv elements that need\nfree()-ing, 2022-08-02), and we should fix it the same way: by passing\nthe free_removed_argv_elements option to setup_revisions().\n\nThe added test here is run only with SANITIZE=leak, without checking its\noutput, because the behavior of stash with \"--\" is a little odd:\n\n 1. Running \"git stash show\" will show --stat output. But running \"git\n stash show --\" will show --patch.\n\n 2. I'd expect a non-option after \"--\" to be treated as a pathspec, so:\n\n git stash show -p 1 -- foo\n\n would look treat \"1\" as a stash (a synonym for stash@{1}) and\n restrict the resulting diff to \"foo\". But it doesn't. We split the\n revision/stash args without any regard to \"--\". So in the example\n above both \"1\" and \"foo\" are stashes. Which is an error, but also:\n\n git stash show -- foo\n\n treats \"foo\" as a stash, not a pathspec.\n\nThese are both oddities that we may want to address (or may not, if we\nwant to retain historical quirks). But they are well outside the scope\nof this patch. So for now we'll just let the tests confirm we aren't\nleaking without otherwise expecting any behavior. If we later address\neither of those points and end up with another test that covers \"stash\nshow --\", we can drop this leak-only test.\n\nSigned-off-by: Jeff King \n---\n builtin/stash.c | 3 ++-\n t/t3903-stash.sh | 4 ++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex f5ddee5c7f..e5ab3c4cf5 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -957,6 +957,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n static int show_stash(int argc, const char **argv, const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n+\tstruct setup_revision_opt opt = { .free_removed_argv_elements = 1 };\n \tint i;\n \tint ret = -1;\n \tstruct stash_info info = STASH_INFO_INIT;\n@@ -1015,7 +1016,7 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n+\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);\n \tif (argc > 1)\n \t\tgoto usage;\n \tif (!rev.diffopt.output_format) {\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 0bb4648e36..daf96aa931 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -1741,4 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes\n \t)\n '\n \n+test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' '\n+\tgit stash show --\n+'\n+\n test_done\n-- \n2.51.0.582.g88c6764cd5\n\n"},{"id":"526990","messageId":"xmqqms6mrxez.fsf@gitster.g","threadId":"64169","inReplyTo":"20250922202509.GE2205919@coredump.intra.peff.net","subject":"Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-09-22T21:26:44Z","receivedAt":"2025-09-22T21:26:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Sep 22, 2025 at 12:36:22PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > Yeah, I had originally written just:\n>> >\n>> > test_expect_success 'stash show -- does not leak' '\n>> > \tgit stash show --\n>> > '\n>> >\n>> > but it felt funny, since the test is doing nothing in a build without\n>> > SANITIZE=leak. If we are OK with that funniness, I can switch back to\n>> > that.\n>> \n>> We have a prerequisite for that. Very nice vehicle for\n>> documentation purposes, even though we do not care about a single\n>> \"stash show\" invocation for correctness or performance reasons.\n>\n> Ah, nice. It crossed my mind that we could try marking it as such, but I\n> forgot (or never knew) that we already had this prereq available. I\n> guess it was mainly used as \"!SANITIZE_LEAK\" before for suppressing\n> leaks.\n>\n>> Perhaps I can squash the following in, unless you have other changes\n>> in mind.\n>\n> Thanks, that looks perfect for the code change. But we probably need to\n> update the discussion of the test in the commit message. Here's what I\n> came up with (replacing patch 1):\n\nThanks, will replace then.\n\n> -- >8 --\n> Subject: [PATCH] stash: tell setup_revisions() to free our allocated strings\n>\n> In \"git stash show\", we do a first pass of parsing our command line\n> options by splitting them into revision args and stash args. These are\n> stored in strvecs, and we pass the revision args to setup_revisions().\n>\n> But setup_revisions() may modify the argv we pass it, causing us to leak\n> some of the entries. In particular, if it sees a \"--\" string, that will\n> be dropped from argv. This is the same as other cases addressed by\n> f92dbdbc6a (revisions API: don't leak memory on argv elements that need\n> free()-ing, 2022-08-02), and we should fix it the same way: by passing\n> the free_removed_argv_elements option to setup_revisions().\n>\n> The added test here is run only with SANITIZE=leak, without checking its\n> output, because the behavior of stash with \"--\" is a little odd:\n>\n> 1. Running \"git stash show\" will show --stat output. But running \"git\n> stash show --\" will show --patch.\n>\n> 2. I'd expect a non-option after \"--\" to be treated as a pathspec, so:\n>\n> git stash show -p 1 -- foo\n>\n> would look treat \"1\" as a stash (a synonym for stash@{1}) and\n> restrict the resulting diff to \"foo\". But it doesn't. We split the\n> revision/stash args without any regard to \"--\". So in the example\n> above both \"1\" and \"foo\" are stashes. Which is an error, but also:\n>\n> git stash show -- foo\n>\n> treats \"foo\" as a stash, not a pathspec.\n\nAll good. I do regret that I didn't resist harder against the \"1 is\nthe same as stash@{1}\" or insisted that the parser be written more\nrobustly if we wanted to support such syntax, but that also is way\ntoo late to change now.\n\n> These are both oddities that we may want to address (or may not, if we\n> want to retain historical quirks). But they are well outside the scope\n> of this patch. So for now we'll just let the tests confirm we aren't\n> leaking without otherwise expecting any behavior. If we later address\n> either of those points and end up with another test that covers \"stash\n> show --\", we can drop this leak-only test.\n\nOK. \n\nBy the way this also solves the original \"git stash show -p --invalid\";\ndo we want to credit the bug reporter and mention that issue as well?\n\nThanks.\n\n> Signed-off-by: Jeff King \n> ---\n> builtin/stash.c | 3 ++-\n> t/t3903-stash.sh | 4 ++++\n> 2 files changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index f5ddee5c7f..e5ab3c4cf5 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -957,6 +957,7 @@ static void diff_include_untracked(const struct stash_info *info, struct diff_op\n> static int show_stash(int argc, const char **argv, const char *prefix,\n> \t\t struct repository *repo UNUSED)\n> {\n> +\tstruct setup_revision_opt opt = { .free_removed_argv_elements = 1 };\n> \tint i;\n> \tint ret = -1;\n> \tstruct stash_info info = STASH_INFO_INIT;\n> @@ -1015,7 +1016,7 @@ static int show_stash(int argc, const char **argv, const char *prefix,\n> \t\t}\n> \t}\n> \n> -\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, NULL);\n> +\targc = setup_revisions(revision_args.nr, revision_args.v, &rev, &opt);\n> \tif (argc > 1)\n> \t\tgoto usage;\n> \tif (!rev.diffopt.output_format) {\n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 0bb4648e36..daf96aa931 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -1741,4 +1741,8 @@ test_expect_success 'submodules does not affect the branch recorded in stash mes\n> \t)\n> '\n> \n> +test_expect_success SANITIZE_LEAK 'stash show handles -- without leaking' '\n> +\tgit stash show --\n> +'\n> +\n> test_done\n"},{"id":"527009","messageId":"20250923004808.GA2271307@coredump.intra.peff.net","threadId":"64169","inReplyTo":"xmqqms6mrxez.fsf@gitster.g","subject":"Re: [PATCH 1/6] stash: tell setup_revisions() to free our allocated strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-09-23T00:48:08Z","receivedAt":"2025-09-23T00:48:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 22, 2025 at 02:26:44PM -0700, Junio C Hamano wrote:\n\n> By the way this also solves the original \"git stash show -p --invalid\";\n> do we want to credit the bug reporter and mention that issue as well?\n\nThat's not handled by this patch, but rather by patch 2 (\"manage memory\nownership...\"). I put this one first because until we start setting the\nfree_removed_argv_elements flag, that second patch won't do anything for\ngit-stash (and I don't think there was a way to trigger the problems\nfrom the other callers which did set it).\n\nThat patch discusses the original issue, but yeah, it could probably\nstand to have a:\n\n Reported-by: Lauri Niskanen \n\ntrailer tacked on.\n\n-Peff\n"}]}