{"thread":{"id":"66355","subject":"[PATCH 0/2] Hi all,","startedAt":"2026-09-19T21:27:00Z","lastAt":"2026-10-01T17:47:55Z","messageCount":78,"participants":["D. Ben Knoble","Phillip Wood","Junio C Hamano","Thomas Bachem","Ben Knoble"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"552900","messageId":"cover.1789853192.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":null,"subject":"[PATCH 0/2] Hi all,","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-19T21:26:34Z","receivedAt":"2026-09-19T21:27:00Z","isPatch":true,"body":"This small patch series fixes a bug reported by Eli Barzilay in the\ninteraction between autostashing, staged index entries, and\nstash.index=true.\n\nThe first patch is an incidental cleanup, while the second holds the\ninteresting bits. Preferences on keeping or removing a few assert()\ncalls in merge-ort.c are welcome.\n\n[1/2] builtin/stash: remove unused header\n[2/2] builtin/stash: merge index in-core\n\n builtin/stash.c | 77 +++++++++---------------------------------------\n merge-ort.c | 3 --\n t/t7600-merge.sh | 9 ++++++\n 3 files changed, 23 insertions(+), 66 deletions(-)\n\n\nbase-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"552901","messageId":"b6798c8a25993913d2ba13b8f3b08d602364ca44.1789853192.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1789853192.git.ben.knoble@gmail.com","subject":"[PATCH 1/2] builtin/stash: remove unused header","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-19T21:26:35Z","receivedAt":"2026-09-19T21:27:20Z","isPatch":true,"body":"Clang complains that oid-array.h is unused. Certainly none of the\noid_array* functions, types, etc., are used, and the\ntransitively-included hash.h declarations are used but covered by a\npre-existing direct #include of hash.h.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7a9843413b..dfea2d2c4c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -31,7 +31,6 @@\n #include \"reflog.h\"\n #include \"reflog-walk.h\"\n #include \"add-interactive.h\"\n-#include \"oid-array.h\"\n #include \"commit.h\"\n \n #define INCLUDE_ALL_FILES 2\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"552902","messageId":"782fe91251111fbb28359574d860e4a6d2e45fc0.1789853192.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1789853192.git.ben.knoble@gmail.com","subject":"[PATCH 2/2] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-19T21:26:36Z","receivedAt":"2026-09-19T21:27:53Z","isPatch":true,"body":"\"git stash apply --index\" does a 2-step dance to report index conflicts\nbefore carrying out the main unstash: first, attempt to merge the index\n(and remember the name of the resulting tree). If that succeeds, reset\nthe index and carry on unstashing the working tree, then use the\nremembered index tree to unstash the index.\n\nThe \"merge the index\" step is performed on the actual index by a\ncombination of git-diff-tree(1) and git-apply(1), which incurs an extra\ncost to git-reset(1) to cleanup. This also introduces an autostash bug\nwhen stash.index is true: \"git reset\" eventually wants to\nremove_merge_branch_state(), which calls save_autostash() due to\na03b55530a (merge: teach --autostash option, 2020-04-07). This can\nhappen from a \"git merge --autostash\", which itself calls\nsave_autostash(). Operating on the file-system in this way is not\nre-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH\nref [1]. This bug has lurked for a while, but it would have been\nimpossible to trigger without the availability of stash.index to force\nthe autostash apply into index mode.\n\n[1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/\n\nFortunately, we can achieve 2 goals at once: avoid round-tripping to the\nfile-system (and invoking expensive subprocesses) by performing the\nmerge in-core. Since the results are never seen, we don't need to set\nthe usual branch and ancestor labels.\n\nWe *could* swap just the git-reset(1) subprocess with our internal\nreset_tree() and refresh_index(), which would fix the bug. We'd much\nprefer to clean up these vestiges of the shell-based git-stash, though.\n\nReported-by: Eli Barzilay \nHelped-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n\nNotes (benknoble/commits):\n We *could* leave the asserts in, but then we somewhat uselessly set the\n conflict labels, which I did in the original patch [1]. Phillip\n suggested we don't need them, and I otherwise agree.\n \n [1]: https://lore.kernel.org/git/CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com/\n \n In all the versions of 231e2dd49d (merge-ort: add some high-level\n algorithm structure, 2020-12-13) I could find on the mailing list, the\n \"assert(opt->ancestor)\" is present without explanation or comment, so\n I'm not in a good place to assess the impact of removing it and its\n compatriots.\n \n Cc: Elijah Newren \n\n builtin/stash.c | 76 +++++++++---------------------------------------\n merge-ort.c | 3 --\n t/t7600-merge.sh | 9 ++++++\n 3 files changed, 23 insertions(+), 65 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex dfea2d2c4c..9fc1a25e3d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,\n \treturn ret;\n }\n \n-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *w_commit_hex = oid_to_hex(w_commit);\n-\n-\t/*\n-\t * Diff-tree would not be very hard to replace with a native function,\n-\t * however it should be done together with apply_cached.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n-\tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n-\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n-\n-static int apply_cached(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Apply currently only reads either from stdin or a file, thus\n-\t * apply_all_patches would have to be updated to optionally take a\n-\t * buffer.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n-}\n-\n-static int reset_head(void)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Reset is overall quite simple, however there is no current public\n-\t * API for resetting.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n-\n-\treturn run_command(&cp);\n-}\n-\n static int is_path_a_directory(const char *path)\n {\n \t/*\n@@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t oideq(&c_tree, &info->i_tree)) {\n \t\t\thas_index = 0;\n \t\t} else {\n-\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct merge_result result = { 0 };\n \n-\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n-\t\t\t\tstrbuf_release(&out);\n-\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n-\t\t\t\t\t oid_to_hex(&info->w_commit));\n-\t\t\t}\n+\t\t\tinit_basic_merge_options(&o, the_repository);\n \n-\t\t\tret = apply_cached(&out);\n-\t\t\tstrbuf_release(&out);\n-\t\t\tif (ret)\n+\t\t\to.verbosity = 0;\n+\n+\t\t\thead = lookup_tree(o.repo, &c_tree);\n+\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n+\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n+\t\t\t\t\t\t &result);\n+\n+\t\t\tif (!result.clean)\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n \n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n-\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n-\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n-\t\t\t\treturn error(_(\"could not save index tree\"));\n-\n-\t\t\treset_head();\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n+\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n+\t\t\tclear_merge_options(&o);\n \t\t}\n \t}\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex c410a5d353..f69a49d48a 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \ttrace2_region_enter(\"merge\", \"sanity checks\", opt->repo);\n \tassert(opt->repo);\n \n-\tassert(opt->branch1 && opt->branch2);\n-\n \tassert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&\n \t opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);\n \tassert(opt->rename_limit >= -1);\n@@ -5409,7 +5407,6 @@ void merge_incore_nonrecursive(struct merge_options *opt,\n \ttrace2_region_enter(\"merge\", \"incore_nonrecursive\", opt->repo);\n \n \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n-\tassert(opt->ancestor != NULL);\n \tmerge_check_renames_reusable(opt, result, merge_base, side1, side2);\n \tmerge_start(opt, result);\n \t/*\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 64fe21717d..8f6109fb91 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -801,6 +801,15 @@ verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n '\n \n+test_expect_success 'fast-forward merge with --autostash, stash.index' '\n+\tgit reset --hard c0 &&\n+\tgit stash clear &&\n+\techo staged >>z && git add z &&\n+\tgit -c stash.index=true merge --autostash c1 2>err &&\n+\ttest_grep \"Applied autostash.\" err &&\n+\ttest_stdout_line_count = 0 git stash list\n+'\n+\n test_expect_success 'failed fast-forward merge with --autostash' '\n \tgit reset --hard c0 &&\n \tgit merge-file file file.orig file.5 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"552904","messageId":"CALnO6CDnm3pGp5+gyJeZZbg1EmxWrXkkwka2-EXJPYHNM=e9nQ@mail.gmail.com","threadId":"66355","inReplyTo":"cover.1789853192.git.ben.knoble@gmail.com","subject":"Re: [PATCH 0/2] Hi all,","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-19T21:32:46Z","receivedAt":"2026-09-19T21:33:00Z","isPatch":true,"body":"My apologies for the strange subject; a little mishap when editing the\nbranch description (I forgot the first line was special).\n"},{"id":"552928","messageId":"2551b801-4cb3-4880-ac01-7d14a188ddd4@gmail.com","threadId":"66355","inReplyTo":"782fe91251111fbb28359574d860e4a6d2e45fc0.1789853192.git.ben.knoble@gmail.com","subject":"Re: [PATCH 2/2] builtin/stash: merge index in-core","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-21T13:17:31Z","receivedAt":"2026-09-21T13:17:37Z","isPatch":true,"body":"Hi Ben\n\nOn 19/09/2026 22:26, D. Ben Knoble wrote:\n> \"git stash apply --index\" does a 2-step dance to report index conflicts\n> before carrying out the main unstash: first, attempt to merge the index\n> (and remember the name of the resulting tree). If that succeeds, reset\n> the index and carry on unstashing the working tree, then use the\n> remembered index tree to unstash the index.\n> \n> The \"merge the index\" step is performed on the actual index by a\n> combination of git-diff-tree(1) and git-apply(1), which incurs an extra\n> cost to git-reset(1) to cleanup. This also introduces an autostash bug\n> when stash.index is true: \"git reset\" eventually wants to\n> remove_merge_branch_state(), which calls save_autostash() due to\n> a03b55530a (merge: teach --autostash option, 2020-04-07). This can\n> happen from a \"git merge --autostash\", which itself calls\n> save_autostash(). Operating on the file-system in this way is not\n> re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH\n> ref [1]. This bug has lurked for a while, but it would have been\n> impossible to trigger without the availability of stash.index to force\n> the autostash apply into index mode.\n> \n> [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/\n> \n> Fortunately, we can achieve 2 goals at once: avoid round-tripping to the\n> file-system (and invoking expensive subprocesses) by performing the\n> merge in-core. Since the results are never seen, we don't need to set\n> the usual branch and ancestor labels.\n\nWhen the merge succeeds without conflicts we use the result so it is \nseen. It would be clearer to say that \"If there are conflicts we discard \nthe result so ...\". The rest of the commit message explains the problem \nnicely.\n\n> We *could* swap just the git-reset(1) subprocess with our internal\n> reset_tree() and refresh_index(), which would fix the bug. We'd much\n> prefer to clean up these vestiges of the shell-based git-stash, though.\n\nDefinitely\n\n> builtin/stash.c | 76 +++++++++---------------------------------------\n\nNice diffstat!\n\n> @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> \t\t oideq(&c_tree, &info->i_tree)) {\n> \t\t\thas_index = 0;\n> \t\t} else {\n> -\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\t\tstruct merge_result result = { 0 };\n> \n> -\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n> -\t\t\t\tstrbuf_release(&out);\n> -\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n> -\t\t\t\t\t oid_to_hex(&info->w_commit));\n> -\t\t\t}\n> +\t\t\tinit_basic_merge_options(&o, the_repository);\n\nThis means we potentially use different diff algorithms when merging the \nindex and when merging the work tree, let's use the _ui variant here \ninstead.\n\n> \n> -\t\t\tret = apply_cached(&out);\n> -\t\t\tstrbuf_release(&out);\n> -\t\t\tif (ret)\n> +\t\t\to.verbosity = 0;\n\nLooking at the code in merge-ort.c it appears the verbosity option was \nused by the recursive strategy but isn't used anymore so I think we \ncould drop this.\n\n> +\n> +\t\t\thead = lookup_tree(o.repo, &c_tree);\n> +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n> +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> +\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n> +\t\t\t\t\t\t &result);\n> +\n> +\t\t\tif (!result.clean)\n> \t\t\t\treturn error(_(\"conflicts in index. \"\n> \t\t\t\t\t \"Try without --index.\"));\n> \n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> -\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n> -\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n> -\t\t\t\treturn error(_(\"could not save index tree\"));\n> -\n> -\t\t\treset_head();\n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> +\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n> +\t\t\tclear_merge_options(&o);\n\nLooking at replay.c:replay_revisions() I think this should be\n\nmerge_finalize(&opts, &result);\n\n> \t\t}\n> \t}\n> \n> diff --git a/merge-ort.c b/merge-ort.c\n> index c410a5d353..f69a49d48a 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n> \ttrace2_region_enter(\"merge\", \"sanity checks\", opt->repo);\n> \tassert(opt->repo);\n> \n> -\tassert(opt->branch1 && opt->branch2);\n\nThis, and the hunk below, make me nervous. Normally assertions like this \nexist because the pointers are unconditionally dereferenced later on. \nLooking at merge_3way() it asserts opt->ancestor is non-NULL and \ndereferences all three labels. t3903 does not appear to have test \ncoverage for the index merge failing (if it did I think we'd see a \nSIGSEV), we should probably add a test that checks the command fails \nleaving the index and work tree untouched, and verifies the message on \nstderr.\n\nLets set some simple, fixed, ancestor and branch names in \ndo_apply_stash() above.\n\n> \tassert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&\n> \t opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);\n> \tassert(opt->rename_limit >= -1);\n> @@ -5409,7 +5407,6 @@ void merge_incore_nonrecursive(struct merge_options *opt,\n> \ttrace2_region_enter(\"merge\", \"incore_nonrecursive\", opt->repo);\n> \n> \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n> -\tassert(opt->ancestor != NULL);\n> \tmerge_check_renames_reusable(opt, result, merge_base, side1, side2);\n> \tmerge_start(opt, result);\n> \t/*\n\n> +test_expect_success 'fast-forward merge with --autostash, stash.index' '\n> +\tgit reset --hard c0 &&\n> +\tgit stash clear &&\n> +\techo staged >>z && git add z &&\n> +\tgit -c stash.index=true merge --autostash c1 2>err &&\n> +\ttest_grep \"Applied autostash.\" err &&\n> +\ttest_stdout_line_count = 0 git stash list\n> +'\n\nWe check the autostash is applied and is not saved - good\n\nThanks for working on this, it is really good to get rid of those \nsubprocesses.\n\nPhillip\n\n> test_expect_success 'failed fast-forward merge with --autostash' '\n> \tgit reset --hard c0 &&\n> \tgit merge-file file file.orig file.5 &&\n\n"},{"id":"552935","messageId":"xmqqse32od1c.fsf@gitster.g","threadId":"66355","inReplyTo":"b6798c8a25993913d2ba13b8f3b08d602364ca44.1789853192.git.ben.knoble@gmail.com","subject":"Re: [PATCH 1/2] builtin/stash: remove unused header","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T15:10:55Z","receivedAt":"2026-09-21T15:11:01Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> Clang complains that oid-array.h is unused. Certainly none of the\n> oid_array* functions, types, etc., are used, and the\n\n\n> transitively-included hash.h declarations are used but covered by a\n> pre-existing direct #include of hash.h.\n\nGood thing to make sure.\n\nAnd the correctness of the patch can easily be validated, which\nmakes this kind of patch no-brainer to accept ;-)\n\nThanks.\n\n>\n> Signed-off-by: D. Ben Knoble \n> ---\n> builtin/stash.c | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 7a9843413b..dfea2d2c4c 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -31,7 +31,6 @@\n> #include \"reflog.h\"\n> #include \"reflog-walk.h\"\n> #include \"add-interactive.h\"\n> -#include \"oid-array.h\"\n> #include \"commit.h\"\n> \n> #define INCLUDE_ALL_FILES 2\n"},{"id":"552980","messageId":"CALnO6CDG4Emny7xESxN8GObaXb_P9gPHBZ857hrAvDjiMSqsKQ@mail.gmail.com","threadId":"66355","inReplyTo":"2551b801-4cb3-4880-ac01-7d14a188ddd4@gmail.com","subject":"Re: [PATCH 2/2] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-22T12:43:46Z","receivedAt":"2026-09-22T12:43:59Z","isPatch":true,"body":"On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 19/09/2026 22:26, D. Ben Knoble wrote:\n> > Fortunately, we can achieve 2 goals at once: avoid round-tripping to the\n> > file-system (and invoking expensive subprocesses) by performing the\n> > merge in-core. Since the results are never seen, we don't need to set\n> > the usual branch and ancestor labels.\n>\n> When the merge succeeds without conflicts we use the result so it is\n> seen. It would be clearer to say that \"If there are conflicts we discard\n> the result so ...\". The rest of the commit message explains the problem\n> nicely.\n\nIndeed. This is what I get for (unusually) dashing off the commit\nmessage up against the clock. Thanks!\n\n> > @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> > oideq(&c_tree, &info->i_tree)) {\n> > has_index = 0;\n> > } else {\n> > - struct strbuf out = STRBUF_INIT;\n> > + struct merge_result result = { 0 };\n> >\n> > - if (diff_tree_binary(&out, &info->w_commit)) {\n> > - strbuf_release(&out);\n> > - return error(_(\"could not generate diff %s^!.\"),\n> > - oid_to_hex(&info->w_commit));\n> > - }\n> > + init_basic_merge_options(&o, the_repository);\n>\n> This means we potentially use different diff algorithms when merging the\n> index and when merging the work tree, let's use the _ui variant here\n> instead.\n\nYep, you know I'd spotted that and wasn't expecting it to make a\nmeaningful difference. It's an easy swap, but I thought that (like\nabove, since we don't show the conflict results) the diff algorithm\nwouldn't matter too much.\n\nMaybe it affects the actual merge-ability, though, in which case I\nagree using the same is important?\n\n> > + o.verbosity = 0;\n>\n> Looking at the code in merge-ort.c it appears the verbosity option was\n> used by the recursive strategy but isn't used anymore so I think we\n> could drop this.\n\nIntriguing. (Assuming the default \"2\") There's a \"< 5\" check in\npath_msg() that wouldn't be affected by dropping this, and a \"> 2\"\ncheck in checkout() that… also wouldn't be affected?\n\nBut it might matter if something is setting the verbosity elsewhere\n(config, GIT_MERGE_VERBOSITY), and I think we really want this merge\nto be quiet? I seem to remember reading commits in this area quieting\n\"git reset\" and so on to keep the noise down.\n\nSo I'm inclined to leave it for now, especially in case it later does get used.\n\n> > + oidcpy(&index_tree, &result.tree->object.oid);\n> > + clear_merge_options(&o);\n>\n> Looking at replay.c:replay_revisions() I think this should be\n>\n> merge_finalize(&opts, &result);\n\nHm, possibly. It does look like that does more with the \"result,\"\nwhich is probably needed. But it doesn't actually clear the merge\noptions.\n\nOn one hand, I thought it could be important not to reuse that struct\nbetween merges. But if we do use the \"ui\" init, it might be ok?\nreplay_revisions() does use the same struct between calls to\nmerge_incore_nonrecursive().\n\nOh, but one other thing: we unconditionally reinit the merge options\nlater on in do_apply_stash(). We could conditionally initialize there\n(\"if (has_index)\"), I suppose?\n\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index c410a5d353..f69a49d48a 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n> > trace2_region_enter(\"merge\", \"sanity checks\", opt->repo);\n> > assert(opt->repo);\n> >\n> > - assert(opt->branch1 && opt->branch2);\n>\n> This, and the hunk below, make me nervous. Normally assertions like this\n> exist because the pointers are unconditionally dereferenced later on.\n> Looking at merge_3way() it asserts opt->ancestor is non-NULL and\n> dereferences all three labels. t3903 does not appear to have test\n> coverage for the index merge failing (if it did I think we'd see a\n> SIGSEV), we should probably add a test that checks the command fails\n> leaving the index and work tree untouched, and verifies the message on\n> stderr.\n>\n> Lets set some simple, fixed, ancestor and branch names in\n> do_apply_stash() above.\n\nFunny, I was getting aborts before removing the asserts because I\nhadn't set the labels, aha. Looks like we've come back around to\nkeeping the labels. I'll probably keep a similar structure as the\nworking tree merge uses, I think.\n\nA fail-to-merge test also seems like a good idea. Let me mull on that.\n\nThanks for the review.\n\n-- \nD. Ben Knoble\n"},{"id":"552981","messageId":"CALnO6CBbQToKU-mJdRXL=XsDGMQFD2qPswKDK8sjdf7b1jCLGA@mail.gmail.com","threadId":"66355","inReplyTo":"CALnO6CDG4Emny7xESxN8GObaXb_P9gPHBZ857hrAvDjiMSqsKQ@mail.gmail.com","subject":"Re: [PATCH 2/2] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-22T12:51:39Z","receivedAt":"2026-09-22T12:51:51Z","isPatch":true,"body":"On Tue, Sep 22, 2026 at 8:43 AM D. Ben Knoble wrote:\n>\n> Oh, but one other thing: we unconditionally reinit the merge options\n> later on in do_apply_stash(). We could conditionally initialize there\n> (\"if (has_index)\"), I suppose?\n\ner, \"!has_index\" of course (which is what I originally typed and then,\nconfused, edited).\n\n-- \nD. Ben Knoble\n"},{"id":"552990","messageId":"41d28f9d-b86a-4d65-9a85-656ea9d216e9@gmail.com","threadId":"66355","inReplyTo":"CALnO6CDG4Emny7xESxN8GObaXb_P9gPHBZ857hrAvDjiMSqsKQ@mail.gmail.com","subject":"Re: [PATCH 2/2] builtin/stash: merge index in-core","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-22T13:57:18Z","receivedAt":"2026-09-22T13:57:25Z","isPatch":true,"body":"Hi Ben\n\nOn 22/09/2026 13:43, D. Ben Knoble wrote:\n> On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood wrote:\n>> On 19/09/2026 22:26, D. Ben Knoble wrote:\n>>> Fortunately, we can achieve 2 goals at once: avoid round-tripping to the\n>>> file-system (and invoking expensive subprocesses) by performing the\n>>> merge in-core. Since the results are never seen, we don't need to set\n>>> the usual branch and ancestor labels.\n>>\n>> When the merge succeeds without conflicts we use the result so it is\n>> seen. It would be clearer to say that \"If there are conflicts we discard\n>> the result so ...\". The rest of the commit message explains the problem\n>> nicely.\n> \n> Indeed. This is what I get for (unusually) dashing off the commit\n> message up against the clock. Thanks!\n> \n>>> @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n>>> oideq(&c_tree, &info->i_tree)) {\n>>> has_index = 0;\n>>> } else {\n>>> - struct strbuf out = STRBUF_INIT;\n>>> + struct merge_result result = { 0 };\n>>>\n>>> - if (diff_tree_binary(&out, &info->w_commit)) {\n>>> - strbuf_release(&out);\n>>> - return error(_(\"could not generate diff %s^!.\"),\n>>> - oid_to_hex(&info->w_commit));\n>>> - }\n>>> + init_basic_merge_options(&o, the_repository);\n>>\n>> This means we potentially use different diff algorithms when merging the\n>> index and when merging the work tree, let's use the _ui variant here\n>> instead.\n> \n> Yep, you know I'd spotted that and wasn't expecting it to make a\n> meaningful difference. It's an easy swap, but I thought that (like\n> above, since we don't show the conflict results) the diff algorithm\n> wouldn't matter too much.\n> \n> Maybe it affects the actual merge-ability, though, in which case I\n> agree using the same is important?\n\nI think there are wierd cases where one diff algorithm results in \nconflicts and another doesn't because they generate different (but \nequally valid) diffs so allowing the user to tweak the algorithm we use \nvia init_ui_merge_options() is probably a good idea.\n\n> \n>>> + o.verbosity = 0;\n>>\n>> Looking at the code in merge-ort.c it appears the verbosity option was\n>> used by the recursive strategy but isn't used anymore so I think we\n>> could drop this.\n> \n> Intriguing. (Assuming the default \"2\") There's a \"< 5\" check in\n> path_msg() that wouldn't be affected by dropping this, and a \"> 2\"\n> check in checkout() that… also wouldn't be affected?\n\nThe former is not affected because we're cherry-picking so never have an \ninner merge from merging multiple merge bases. The latter is not \naffected because we don't checkout the result!\n> But it might matter if something is setting the verbosity elsewhere\n> (config, GIT_MERGE_VERBOSITY), and I think we really want this merge\n> to be quiet? I seem to remember reading commits in this area quieting\n> \"git reset\" and so on to keep the noise down.\n> \n> So I'm inclined to leave it for now, especially in case it later does get used.\n\nmerge ort does not print anything - it just adds messages to an strmap \nin struct merge_result() which we ignore here. I guess setting it to \nzero might avoid a little work generating the messages.\n\n> \n>>> + oidcpy(&index_tree, &result.tree->object.oid);\n>>> + clear_merge_options(&o);\n>>\n>> Looking at replay.c:replay_revisions() I think this should be\n>>\n>> merge_finalize(&opts, &result);\n> \n> Hm, possibly. It does look like that does more with the \"result,\"\n> which is probably needed.\n\nOh, we definitely want to free the strmap in the merge result.\n\n > But it doesn't actually clear the merge options.\n\nIsn't that because there are no allocations in that struct? (obuf is \nunused - it looks like we could clean up the struct by removing the \nmembers that were used by merge-recursive but are ignored by merge-ort)\n\n> On one hand, I thought it could be important not to reuse that struct\n> between merges. But if we do use the \"ui\" init, it might be ok?\n> replay_revisions() does use the same struct between calls to\n> merge_incore_nonrecursive().\n> \n> Oh, but one other thing: we unconditionally reinit the merge options\n> later on in do_apply_stash(). We could conditionally initialize there\n> (\"if (has_index)\"), I suppose?\n\nI'd just move the call to init_ui_merge_options() above \"if (index)\". As \nfar as I know it should be fine to reuse it - any state is stored in the \nresult\n\n> \n>>> diff --git a/merge-ort.c b/merge-ort.c\n>>> index c410a5d353..f69a49d48a 100644\n>>> --- a/merge-ort.c\n>>> +++ b/merge-ort.c\n>>> @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n>>> trace2_region_enter(\"merge\", \"sanity checks\", opt->repo);\n>>> assert(opt->repo);\n>>>\n>>> - assert(opt->branch1 && opt->branch2);\n>>\n>> This, and the hunk below, make me nervous. Normally assertions like this\n>> exist because the pointers are unconditionally dereferenced later on.\n>> Looking at merge_3way() it asserts opt->ancestor is non-NULL and\n>> dereferences all three labels. t3903 does not appear to have test\n>> coverage for the index merge failing (if it did I think we'd see a\n>> SIGSEV), we should probably add a test that checks the command fails\n>> leaving the index and work tree untouched, and verifies the message on\n>> stderr.\n>>\n>> Lets set some simple, fixed, ancestor and branch names in\n>> do_apply_stash() above.\n> \n> Funny, I was getting aborts before removing the asserts because I\n> hadn't set the labels, aha. Looks like we've come back around to\n> keeping the labels.\n\nSorry for that detour\n\n> I'll probably keep a similar structure as the\n> working tree merge uses, I think.\n\nI'd use fixed names and not bother with all the conditionals around the \nlabel text to keep it simple.\n\n> A fail-to-merge test also seems like a good idea. Let me mull on that.\n\nThat's great\n\nThanks\n\nPhillip\n\n> Thanks for the review.\n> \n\n"},{"id":"553017","messageId":"CALnO6CDxew2b0X+HMiT0Vai_hj+MaueV9Ht2BOB5zrsZ27QUwg@mail.gmail.com","threadId":"66355","inReplyTo":"41d28f9d-b86a-4d65-9a85-656ea9d216e9@gmail.com","subject":"Re: [PATCH 2/2] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-22T20:34:07Z","receivedAt":"2026-09-22T20:34:22Z","isPatch":true,"body":"Thanks again, Philip :)\n\nOn Tue, Sep 22, 2026 at 9:57 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 22/09/2026 13:43, D. Ben Knoble wrote:\n> I think there are wierd cases where one diff algorithm results in\n> conflicts and another doesn't because they generate different (but\n> equally valid) diffs so allowing the user to tweak the algorithm we use\n> via init_ui_merge_options() is probably a good idea.\n\nGotcha; I've already queued this locally.\n\n> >>> + o.verbosity = 0;\n> >>\n> >> Looking at the code in merge-ort.c it appears the verbosity option was\n> >> used by the recursive strategy but isn't used anymore so I think we\n> >> could drop this.\n> >\n> > Intriguing. (Assuming the default \"2\") There's a \"< 5\" check in\n> > path_msg() that wouldn't be affected by dropping this, and a \"> 2\"\n> > check in checkout() that… also wouldn't be affected?\n>\n> The former is not affected because we're cherry-picking so never have an\n> inner merge from merging multiple merge bases. The latter is not\n> affected because we don't checkout the result!\n\nThat's very helpful; I find it challenging right now to navigate the\nvarious call-graphs here :)\n\n> > But it might matter if something is setting the verbosity elsewhere\n> > (config, GIT_MERGE_VERBOSITY), and I think we really want this merge\n> > to be quiet? I seem to remember reading commits in this area quieting\n> > \"git reset\" and so on to keep the noise down.\n> >\n> > So I'm inclined to leave it for now, especially in case it later does get used.\n>\n> merge ort does not print anything - it just adds messages to an strmap\n> in struct merge_result() which we ignore here. I guess setting it to\n> zero might avoid a little work generating the messages.\n\nPossibly! I still think it signals our intent to be quiet better this way, too.\n> >>> + oidcpy(&index_tree, &result.tree->object.oid);\n> >>> + clear_merge_options(&o);\n> >>\n> >> Looking at replay.c:replay_revisions() I think this should be\n> >>\n> >> merge_finalize(&opts, &result);\n> >\n> > Hm, possibly. It does look like that does more with the \"result,\"\n> > which is probably needed.\n>\n> Oh, we definitely want to free the strmap in the merge result.\n>\n> > But it doesn't actually clear the merge options.\n>\n> Isn't that because there are no allocations in that struct? (obuf is\n> unused - it looks like we could clean up the struct by removing the\n> members that were used by merge-recursive but are ignored by merge-ort)\n\nMaybe---I was more worried about un-reusable state, but it's true that\nthe clear function is a no-op right now, heh. So it was a bit of \"in\ncase one day this is mandatory,\" perhaps.\n\n> > On one hand, I thought it could be important not to reuse that struct\n> > between merges. But if we do use the \"ui\" init, it might be ok?\n> > replay_revisions() does use the same struct between calls to\n> > merge_incore_nonrecursive().\n> >\n> > Oh, but one other thing: we unconditionally reinit the merge options\n> > later on in do_apply_stash(). We could conditionally initialize there\n> > (\"if (has_index)\"), I suppose?\n>\n> I'd just move the call to init_ui_merge_options() above \"if (index)\". As\n> far as I know it should be fine to reuse it - any state is stored in the\n> result\n\nYeah, that's smarter. Locally I got tripped by the case where we said\n--index but skip some work; but it should be fine to unconditionally\ninitialize those options earlier.\n\n> > Funny, I was getting aborts before removing the asserts because I\n> > hadn't set the labels, aha. Looks like we've come back around to\n> > keeping the labels.\n>\n> Sorry for that detour\n\nNo worries.\n\n> > I'll probably keep a similar structure as the\n> > working tree merge uses, I think.\n>\n> I'd use fixed names and not bother with all the conditionals around the\n> label text to keep it simple.\n\nThat's what I ended up with locally, yeah. I finally decided it was\ntoo complicated to do anything else for labels that would be really\nhard to find.\n\nI'll get v2 out in the morning, probably.\n\n-- \nD. Ben Knoble\n"},{"id":"553050","messageId":"cover.1790168285.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1789853192.git.ben.knoble@gmail.com","subject":"[PATCH v2 0/4] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-23T12:58:03Z","receivedAt":"2026-09-23T12:59:15Z","isPatch":true,"body":"Hi all,\n\nThis small patch series fixes a bug reported by Eli Barzilay in the\ninteraction between autostashing, staged index entries, and\nstash.index=true.\n\nThe first patch is an incidental cleanup, and the second re-arranges one\nline to make the change easier. The third adds a new test, while the\nfourth holds the interesting bits.\n\nChanges in v2:\n\n• Do give branch labels for the incore merge, although they are never\n seen (and clarify commit message as a result, also keeping the\n merge-ort asserts). Phillip was right: without those, we do segfault\n on conflicts.\n• Use the ui merge options to keep the same diff algorithm.\n• Use merge_finalize instead of clear_merge_options, and reuse the\n options between merge calls if they are already initialized.\n• Add a new 2/4 to simplify merge options initialization.\n• Add a new 3/4 with a test case for conflicted index merges.\n\nv1: \n\n[1/4] builtin/stash: remove unused header\n[2/4] stash: prepare merge options earlier\n[3/4] t: test failed \"stash apply --index\"\n[4/4] builtin/stash: merge index in-core\n\n builtin/stash.c | 83 +++++++++++-------------------------------------\n t/t3903-stash.sh | 18 +++++++++++\n t/t7600-merge.sh | 9 ++++++\n 3 files changed, 45 insertions(+), 65 deletions(-)\n\nDiff-intervalle contre v1 :\n1: b6798c8a25 = 1: b6798c8a25 builtin/stash: remove unused header\n-: ---------- > 2: 1e2343c7fc stash: prepare merge options earlier\n-: ---------- > 3: 5bd4b78cac t: test failed \"stash apply --index\"\n2: 782fe91251 ! 4: e49936ee12 builtin/stash: merge index in-core\n @@ Commit message\n \n Fortunately, we can achieve 2 goals at once: avoid round-tripping to the\n file-system (and invoking expensive subprocesses) by performing the\n - merge in-core. Since the results are never seen, we don't need to set\n - the usual branch and ancestor labels.\n + merge in-core. If there are conflicts, we discard the resulting tree, so\n + we don't see the usual branch and ancestor labels, but the merge\n + subroutines insist on their presence, so use something simple.\n \n We *could* swap just the git-reset(1) subprocess with our internal\n reset_tree() and refresh_index(), which would fix the bug. We'd much\n @@ Commit message\n Reported-by: Eli Barzilay \n Helped-by: Phillip Wood \n \n -\n - ## Notes (benknoble/commits) ##\n - We *could* leave the asserts in, but then we somewhat uselessly set the\n - conflict labels, which I did in the original patch [1]. Phillip\n - suggested we don't need them, and I otherwise agree.\n -\n - [1]: https://lore.kernel.org/git/CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com/\n -\n - In all the versions of 231e2dd49d (merge-ort: add some high-level\n - algorithm structure, 2020-12-13) I could find on the mailing list, the\n - \"assert(opt->ancestor)\" is present without explanation or comment, so\n - I'm not in a good place to assess the impact of removing it and its\n - compatriots.\n -\n - Cc: Elijah Newren \n -\n ## builtin/stash.c ##\n @@ builtin/stash.c: static int create_index_from_tree(const struct object_id *tree_id,\n \treturn ret;\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n -\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n -\t\t\t\t\t oid_to_hex(&info->w_commit));\n -\t\t\t}\n -+\t\t\tinit_basic_merge_options(&o, the_repository);\n ++\t\t\to.branch1 = \"Upstream index\";\n ++\t\t\to.branch2 = \"Stashed index changes\";\n ++\t\t\to.ancestor = \"Stash base\";\n \n -\t\t\tret = apply_cached(&out);\n -\t\t\tstrbuf_release(&out);\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n -\t\t\tdiscard_index(the_repository->index);\n -\t\t\trepo_read_index(the_repository);\n +\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n -+\t\t\tclear_merge_options(&o);\n ++\t\t\tmerge_finalize(&o, &result);\n \t\t}\n \t}\n \n \n - ## merge-ort.c ##\n -@@ merge-ort.c: static void merge_start(struct merge_options *opt, struct merge_result *result)\n - \ttrace2_region_enter(\"merge\", \"sanity checks\", opt->repo);\n - \tassert(opt->repo);\n - \n --\tassert(opt->branch1 && opt->branch2);\n --\n - \tassert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&\n - \t opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);\n - \tassert(opt->rename_limit >= -1);\n -@@ merge-ort.c: void merge_incore_nonrecursive(struct merge_options *opt,\n - \ttrace2_region_enter(\"merge\", \"incore_nonrecursive\", opt->repo);\n - \n - \ttrace2_region_enter(\"merge\", \"merge_start\", opt->repo);\n --\tassert(opt->ancestor != NULL);\n - \tmerge_check_renames_reusable(opt, result, merge_base, side1, side2);\n - \tmerge_start(opt, result);\n - \t/*\n -\n ## t/t7600-merge.sh ##\n @@ t/t7600-merge.sh: verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n\nbase-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553051","messageId":"b6798c8a25993913d2ba13b8f3b08d602364ca44.1790168285.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790168285.git.ben.knoble@gmail.com","subject":"[PATCH v2 1/4] builtin/stash: remove unused header","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-23T12:58:04Z","receivedAt":"2026-09-23T12:59:16Z","isPatch":true,"body":"Clang complains that oid-array.h is unused. Certainly none of the\noid_array* functions, types, etc., are used, and the\ntransitively-included hash.h declarations are used but covered by a\npre-existing direct #include of hash.h.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7a9843413b..dfea2d2c4c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -31,7 +31,6 @@\n #include \"reflog.h\"\n #include \"reflog-walk.h\"\n #include \"add-interactive.h\"\n-#include \"oid-array.h\"\n #include \"commit.h\"\n \n #define INCLUDE_ALL_FILES 2\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553052","messageId":"1e2343c7fcb17137389d740701336f6c885ba928.1790168285.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790168285.git.ben.knoble@gmail.com","subject":"[PATCH v2 2/4] stash: prepare merge options earlier","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-23T12:58:05Z","receivedAt":"2026-09-23T12:59:16Z","isPatch":true,"body":"In a future commit, we will reuse these options for the index merge of\n\"apply --index\", not just for the worktree.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex dfea2d2c4c..043a38cc6d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -664,6 +664,8 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n \t\treturn error(_(\"cannot apply a stash in the middle of a merge\"));\n \n+\tinit_ui_merge_options(&o, the_repository);\n+\n \tif (index) {\n \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n \t\t oideq(&c_tree, &info->i_tree)) {\n@@ -695,8 +697,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t}\n \t}\n \n-\tinit_ui_merge_options(&o, the_repository);\n-\n \to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n \to.branch2 = label_theirs ? label_theirs : \"Stashed changes\";\n \to.ancestor = label_base ? label_base : \"Stash base\";\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553053","messageId":"5bd4b78cace8ba8c8887c78f739bde3513dfda28.1790168285.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790168285.git.ben.knoble@gmail.com","subject":"[PATCH v2 3/4] t: test failed \"stash apply --index\"","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-23T12:58:06Z","receivedAt":"2026-09-23T12:59:18Z","isPatch":true,"body":"The next commit will refactor index handling for applied stashes, so\nlet's make sure we cover conflicted index merging, too.\n\nSigned-off-by: D. Ben Knoble \n---\n t/t3903-stash.sh | 18 ++++++++++++++++++\n 1 file changed, 18 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 721158606f..3958ab3c8d 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -374,6 +374,24 @@ setup_stash() {\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stash apply --index leaves everything untouched on failure' '\n+\tgit reset --hard &&\n+\techo test >other-file &&\n+\tgit add other-file &&\n+\tgit stash &&\n+\techo unrelated >file &&\n+\techo unrelated >another-file &&\n+\tgit add another-file &&\n+\tgit diff-files >expect &&\n+\n+\techo conflict >other-file &&\n+\tgit add other-file &&\n+\ttest_must_fail git stash apply --index 2>err &&\n+\ttest_grep \"conflicts in index. Try without --index\" err &&\n+\tgit diff-files >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553054","messageId":"e49936ee12aaf5d82a98dddcc618cee01ac3c681.1790168285.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790168285.git.ben.knoble@gmail.com","subject":"[PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-23T12:58:07Z","receivedAt":"2026-09-23T12:59:19Z","isPatch":true,"body":"\"git stash apply --index\" does a 2-step dance to report index conflicts\nbefore carrying out the main unstash: first, attempt to merge the index\n(and remember the name of the resulting tree). If that succeeds, reset\nthe index and carry on unstashing the working tree, then use the\nremembered index tree to unstash the index.\n\nThe \"merge the index\" step is performed on the actual index by a\ncombination of git-diff-tree(1) and git-apply(1), which incurs an extra\ncost to git-reset(1) to cleanup. This also introduces an autostash bug\nwhen stash.index is true: \"git reset\" eventually wants to\nremove_merge_branch_state(), which calls save_autostash() due to\na03b55530a (merge: teach --autostash option, 2020-04-07). This can\nhappen from a \"git merge --autostash\", which itself calls\nsave_autostash(). Operating on the file-system in this way is not\nre-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH\nref [1]. This bug has lurked for a while, but it would have been\nimpossible to trigger without the availability of stash.index to force\nthe autostash apply into index mode.\n\n[1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/\n\nFortunately, we can achieve 2 goals at once: avoid round-tripping to the\nfile-system (and invoking expensive subprocesses) by performing the\nmerge in-core. If there are conflicts, we discard the resulting tree, so\nwe don't see the usual branch and ancestor labels, but the merge\nsubroutines insist on their presence, so use something simple.\n\nWe *could* swap just the git-reset(1) subprocess with our internal\nreset_tree() and refresh_index(), which would fix the bug. We'd much\nprefer to clean up these vestiges of the shell-based git-stash, though.\n\nReported-by: Eli Barzilay \nHelped-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 78 ++++++++++--------------------------------------\n t/t7600-merge.sh | 9 ++++++\n 2 files changed, 25 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 043a38cc6d..219ca457be 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,\n \treturn ret;\n }\n \n-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *w_commit_hex = oid_to_hex(w_commit);\n-\n-\t/*\n-\t * Diff-tree would not be very hard to replace with a native function,\n-\t * however it should be done together with apply_cached.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n-\tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n-\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n-\n-static int apply_cached(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Apply currently only reads either from stdin or a file, thus\n-\t * apply_all_patches would have to be updated to optionally take a\n-\t * buffer.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n-}\n-\n-static int reset_head(void)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Reset is overall quite simple, however there is no current public\n-\t * API for resetting.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n-\n-\treturn run_command(&cp);\n-}\n-\n static int is_path_a_directory(const char *path)\n {\n \t/*\n@@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t oideq(&c_tree, &info->i_tree)) {\n \t\t\thas_index = 0;\n \t\t} else {\n-\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct merge_result result = { 0 };\n \n-\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n-\t\t\t\tstrbuf_release(&out);\n-\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n-\t\t\t\t\t oid_to_hex(&info->w_commit));\n-\t\t\t}\n+\t\t\to.branch1 = \"Upstream index\";\n+\t\t\to.branch2 = \"Stashed index changes\";\n+\t\t\to.ancestor = \"Stash base\";\n \n-\t\t\tret = apply_cached(&out);\n-\t\t\tstrbuf_release(&out);\n-\t\t\tif (ret)\n+\t\t\to.verbosity = 0;\n+\n+\t\t\thead = lookup_tree(o.repo, &c_tree);\n+\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n+\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n+\t\t\t\t\t\t &result);\n+\n+\t\t\tif (!result.clean)\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n \n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n-\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n-\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n-\t\t\t\treturn error(_(\"could not save index tree\"));\n-\n-\t\t\treset_head();\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n+\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n+\t\t\tmerge_finalize(&o, &result);\n \t\t}\n \t}\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 64fe21717d..8f6109fb91 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -801,6 +801,15 @@ verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n '\n \n+test_expect_success 'fast-forward merge with --autostash, stash.index' '\n+\tgit reset --hard c0 &&\n+\tgit stash clear &&\n+\techo staged >>z && git add z &&\n+\tgit -c stash.index=true merge --autostash c1 2>err &&\n+\ttest_grep \"Applied autostash.\" err &&\n+\ttest_stdout_line_count = 0 git stash list\n+'\n+\n test_expect_success 'failed fast-forward merge with --autostash' '\n \tgit reset --hard c0 &&\n \tgit merge-file file file.orig file.5 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553161","messageId":"68e83baa-6ccb-4ca8-a1df-f09d51749c67@gmail.com","threadId":"66355","inReplyTo":"e49936ee12aaf5d82a98dddcc618cee01ac3c681.1790168285.git.ben.knoble@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-24T09:42:09Z","receivedAt":"2026-09-24T09:42:14Z","isPatch":true,"body":"Hi Ben\n\nI've spotted a memory leak that I missed last time, apart from that this \nlooks good.\n\nOn 23/09/2026 13:58, D. Ben Knoble wrote:\n> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> \t\t oideq(&c_tree, &info->i_tree)) {\n> \t\t\thas_index = 0;\n> \t\t} else {\n> -\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\t\tstruct merge_result result = { 0 };\n> \n> -\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n> -\t\t\t\tstrbuf_release(&out);\n> -\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n> -\t\t\t\t\t oid_to_hex(&info->w_commit));\n> -\t\t\t}\n> +\t\t\to.branch1 = \"Upstream index\";\n\nThis is the current index, calling it \"upstream\" is a bit confusing to \nme but that's not worth a re-roll on its own.\n\n> +\t\t\to.branch2 = \"Stashed index changes\";\n> +\t\t\to.ancestor = \"Stash base\";\n> \n> -\t\t\tret = apply_cached(&out);\n> -\t\t\tstrbuf_release(&out);\n> -\t\t\tif (ret)\n> +\t\t\to.verbosity = 0;\n> +\n> +\t\t\thead = lookup_tree(o.repo, &c_tree);\n> +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n> +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> +\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n> +\t\t\t\t\t\t &result);\n> +\n> +\t\t\tif (!result.clean)\n> \t\t\t\treturn error(_(\"conflicts in index. \"\n> \t\t\t\t\t \"Try without --index.\"));\n\nSorry, I missed this last time, but we should finalize the merge before \nreturning to ensure the allocations in result are freed.\nEverything else looks fine.\n\nThanks\n\nPhillip\n\n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> -\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n> -\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n> -\t\t\t\treturn error(_(\"could not save index tree\"));\n> -\n> -\t\t\treset_head();\n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> +\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n> +\t\t\tmerge_finalize(&o, &result);\n> \t\t}\n> \t}\n> \n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 64fe21717d..8f6109fb91 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -801,6 +801,15 @@ verify_no_mergehead () {\n> \ttest_cmp result.1-5 file\n> '\n> \n> +test_expect_success 'fast-forward merge with --autostash, stash.index' '\n> +\tgit reset --hard c0 &&\n> +\tgit stash clear &&\n> +\techo staged >>z && git add z &&\n> +\tgit -c stash.index=true merge --autostash c1 2>err &&\n> +\ttest_grep \"Applied autostash.\" err &&\n> +\ttest_stdout_line_count = 0 git stash list\n> +'\n> +\n> test_expect_success 'failed fast-forward merge with --autostash' '\n> \tgit reset --hard c0 &&\n> \tgit merge-file file file.orig file.5 &&\n\n"},{"id":"553162","messageId":"232f2bf6-04d8-4a54-b4e9-51b5ee79799f@gmail.com","threadId":"66355","inReplyTo":"5bd4b78cace8ba8c8887c78f739bde3513dfda28.1790168285.git.ben.knoble@gmail.com","subject":"Re: [PATCH v2 3/4] t: test failed \"stash apply --index\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-24T09:42:54Z","receivedAt":"2026-09-24T09:43:01Z","isPatch":true,"body":"Hi Ben\n\nOn 23/09/2026 13:58, D. Ben Knoble wrote:\n> The next commit will refactor index handling for applied stashes, so\n> let's make sure we cover conflicted index merging, too.\n> \n> Signed-off-by: D. Ben Knoble \n> ---\n> t/t3903-stash.sh | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n> \n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 721158606f..3958ab3c8d 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -374,6 +374,24 @@ setup_stash() {\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'stash apply --index leaves everything untouched on failure' '\n> +\tgit reset --hard &&\n> +\techo test >other-file &&\n> +\tgit add other-file &&\n> +\tgit stash &&\n> +\techo unrelated >file &&\n> +\techo unrelated >another-file &&\n> +\tgit add another-file &&\n> +\tgit diff-files >expect &&\n\ndiff-files shows the worktree blobs as null object ids, so comparing \nthis before and after stashing only tells us that the same set of files \nhave unstaged changes, not that the unstaged changes are the same. \nAdding \"-p\" would check the worktree files are unchanged.\n\n> +\techo conflict >other-file &&\n> +\tgit add other-file &&\n\nI wonder if we should to add \"git diff-index --cached HEAD \n >expect-index\" here so we can check the index is unchanged as well. For \nthe paths that have unstaged changes we're already checking the index \nobject ids via \"diff-files\", but I think in theory it would be possible \nto have an identical change in the index and worktree that is not picked \nup by that.\n\nThanks for adding this test, it is a useful improvement in our coverage.\n\nPhillip\n\n> +\ttest_must_fail git stash apply --index 2>err &&\n> +\ttest_grep \"conflicts in index. Try without --index\" err &&\n> +\tgit diff-files >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'stash -k' '\n> \techo bar3 >file &&\n> \techo bar4 >file2 &&\n\n"},{"id":"553242","messageId":"xmqqse2yz4y4.fsf@gitster.g","threadId":"66355","inReplyTo":"e49936ee12aaf5d82a98dddcc618cee01ac3c681.1790168285.git.ben.knoble@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-24T21:59:15Z","receivedAt":"2026-09-24T21:59:18Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> \t\t oideq(&c_tree, &info->i_tree)) {\n> \t\t\thas_index = 0;\n> \t\t} else {\n> -\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\t\tstruct merge_result result = { 0 };\n> \n> -\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n> -\t\t\t\tstrbuf_release(&out);\n> -\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n> -\t\t\t\t\t oid_to_hex(&info->w_commit));\n> -\t\t\t}\n> +\t\t\to.branch1 = \"Upstream index\";\n> +\t\t\to.branch2 = \"Stashed index changes\";\n> +\t\t\to.ancestor = \"Stash base\";\n> \n> -\t\t\tret = apply_cached(&out);\n> -\t\t\tstrbuf_release(&out);\n> -\t\t\tif (ret)\n\nSo, we used to take a diff between w_commit^2^ and w_commit^2 and\nthen give the resulting patch to \"apply --cached\". w_commit is the\nworking tree state, w_commit^1 is the HEAD (i.e. b_tree) when the\nstash was created (i.e., \"diff HEAD w_commit\" is the change in the\nworking tree), w_commit^2 is the contents of the index\n(i.e. i_tree), so we are computing a patch that represents what\n\"diff --cached HEAD\" would have shown when we created the stash.\nAnd the goal is to reflect this change on the current HEAD to\nrecreate the \"staged\" changes in the current index.\n\nIOW, we want to three-way merge the change that moves you from \nb_tree to i_tree into c_tree.\n\n> +\t\t\to.verbosity = 0;\n> +\n> +\t\t\thead = lookup_tree(o.repo, &c_tree);\n> +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n> +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> +\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n> +\t\t\t\t\t\t &result);\n\nWe are using the merge machinery to perform a cherry-pick of the\nchanges to go from b_tree to i_tree into c_tree. but the three\ntrees involved in this cherry-pick is named unnecessarily\nconfusingly.\n\nmerge-incore-nonrecursive() takes the common ancestor (\"merge_base\")\nand two sides (\"side1\" and \"side2\") in this order. It takes the\nchanges to go from common to side1 and computes the result of\nupdating the remainder (side2) with such a change (or vice versa; a\nmerge is symmetric). When cherry-picking, the first tree would be\nthe b_tree, the second tree would be the i_tree, and the target tree\nwould be the c_tree.\n\n - b_tree serves as merge_base\n - i_tree serves as side1\n - c_tree serves as side2\n\nAm I following what the code should be doing correctly?\n\nI am wondering if the order of the tree trees in the\nmerge_incore_nonrecursive() call is correct. Shouldn't it be\n\n\t\t\tmerge_incore_nonrecursive(&o,\n\t\t\t\t\t\t merge_base, merge, head,\n\t\t\t\t\t\t &result);\n\n(or merge and head swapped) if we want to update head (I would call\nit side2) in such a way that the change to go from it to the\nresulting tree is similar to the change between merge_base (b_tree)\nand merge (i_tree)?\n\nAhh, or perhaps the trees are indeed given in a wrong order, but not\nin a random wrong order. merge_ort_nonrecursive(), which is *not*\nthe function you are using, takes head, merge, and merge_base in\nthis order, and that order matches what you wrote.\n\nPerhaps the true culprit in this confusion is that the order in\nwhich merge_ort_nonrecursive() takes its three trees (head, merge,\nand common) and the order in which merge_incore_nonrecursive() takes\nits trees (merge_base, side1, and side2) are different, and if we\nfix them to match, it would make it easier to work with?\n\nThe new test in the attached patch will fail with this step but if\nwe revert the changes to builtin/stash.c in this step, it passes.\n\n t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++\n 1 file changed, 32 insertions(+)\n\ndiff --git c/t/t3903-stash.sh w/t/t3903-stash.sh\nindex 3958ab3c8d..0a87e62b11 100755\n--- c/t/t3903-stash.sh\n+++ w/t/t3903-stash.sh\n@@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' '\n \ttest_cmp expect actual\n '\n \n+\n+test_expect_success 'stash apply --index does not revert unrelated upstream index changes' '\n+\ttest_when_finished \"rm -fr playpen\" &&\n+\tmkdir playpen &&\n+\t(\n+\t\tcd playpen &&\n+\t\tgit init &&\n+\t\techo \"base1\" >file1 &&\n+\t\techo \"base2\" >file2 &&\n+\t\tgit add file1 file2 &&\n+\t\tgit commit -m \"initial base\" &&\n+\n+\t\t# Make a staged change to file1 and stash it\n+\t\techo \"staged1\" >file1 &&\n+\t\tgit add file1 &&\n+\t\tgit stash &&\n+\n+\t\t# Upstream advances by modifying unrelated file2\n+\t\techo \"upstream2\" >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m \"upstream change to file2\" &&\n+\n+\t\t# Apply the stash with --index\n+\t\tgit stash apply --index &&\n+\n+\t\t# Verify working tree and index state\n+\t\ttest \"$(git show :file1)\" = \"staged1\" &&\n+\t\ttest \"$(git show :file2)\" = \"upstream2\" &&\n+\t\ttest \"$(git show HEAD:file2)\" = \"upstream2\"\n+\t)\n+'\n+\n test_expect_success 'stash apply --index leaves everything untouched on failure' '\n \tgit reset --hard &&\n \techo test >other-file &&\n"},{"id":"553260","messageId":"xmqqpky2x932.fsf@gitster.g","threadId":"66355","inReplyTo":"xmqqse2yz4y4.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-25T04:12:49Z","receivedAt":"2026-09-25T04:12:52Z","isPatch":true,"body":"Junio C Hamano writes:\n\n> The new test in the attached patch will fail with this step but if\n> we revert the changes to builtin/stash.c in this step, it passes.\n\nOh, and with the change to the code, it passes again.\n\ndiff --git i/builtin/stash.c w/builtin/stash.c\nindex 219ca457be..44d962cc5d 100644\n--- i/builtin/stash.c\n+++ w/builtin/stash.c\n@@ -639,7 +639,7 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n \t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n \n-\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n+\t\t\tmerge_incore_nonrecursive(&o, merge_base, merge, head,\n \t\t\t\t\t\t &result);\n \n \t\t\tif (!result.clean)\ndiff --git i/t/t3903-stash.sh w/t/t3903-stash.sh\nindex 3958ab3c8d..0a87e62b11 100755\n--- i/t/t3903-stash.sh\n+++ w/t/t3903-stash.sh\n@@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' '\n \ttest_cmp expect actual\n '\n \n+\n+test_expect_success 'stash apply --index does not revert unrelated upstream index changes' '\n+\ttest_when_finished \"rm -fr playpen\" &&\n+\tmkdir playpen &&\n+\t(\n+\t\tcd playpen &&\n+\t\tgit init &&\n+\t\techo \"base1\" >file1 &&\n+\t\techo \"base2\" >file2 &&\n+\t\tgit add file1 file2 &&\n+\t\tgit commit -m \"initial base\" &&\n+\n+\t\t# Make a staged change to file1 and stash it\n+\t\techo \"staged1\" >file1 &&\n+\t\tgit add file1 &&\n+\t\tgit stash &&\n+\n+\t\t# Upstream advances by modifying unrelated file2\n+\t\techo \"upstream2\" >file2 &&\n+\t\tgit add file2 &&\n+\t\tgit commit -m \"upstream change to file2\" &&\n+\n+\t\t# Apply the stash with --index\n+\t\tgit stash apply --index &&\n+\n+\t\t# Verify working tree and index state\n+\t\ttest \"$(git show :file1)\" = \"staged1\" &&\n+\t\ttest \"$(git show :file2)\" = \"upstream2\" &&\n+\t\ttest \"$(git show HEAD:file2)\" = \"upstream2\"\n+\t)\n+'\n+\n test_expect_success 'stash apply --index leaves everything untouched on failure' '\n \tgit reset --hard &&\n \techo test >other-file &&\n"},{"id":"553278","messageId":"CALnO6CDpS9GQfONKJs=LAUvwYzYyMby+rGAUtvFQruj-ERXt-g@mail.gmail.com","threadId":"66355","inReplyTo":"68e83baa-6ccb-4ca8-a1df-f09d51749c67@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-25T12:55:03Z","receivedAt":"2026-09-25T12:55:15Z","isPatch":true,"body":"Hi Phillip,\n\nOn Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> I've spotted a memory leak that I missed last time, apart from that this\n> looks good.\n>\n> On 23/09/2026 13:58, D. Ben Knoble wrote:\n> > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> > oideq(&c_tree, &info->i_tree)) {\n> > has_index = 0;\n> > } else {\n> > - struct strbuf out = STRBUF_INIT;\n> > + struct merge_result result = { 0 };\n> >\n> > - if (diff_tree_binary(&out, &info->w_commit)) {\n> > - strbuf_release(&out);\n> > - return error(_(\"could not generate diff %s^!.\"),\n> > - oid_to_hex(&info->w_commit));\n> > - }\n> > + o.branch1 = \"Upstream index\";\n>\n> This is the current index, calling it \"upstream\" is a bit confusing to\n> me but that's not worth a re-roll on its own.\n\nWill fix. The \"upstream\" verbiage comes from the working tree labels.\n\n> > + o.branch2 = \"Stashed index changes\";\n> > + o.ancestor = \"Stash base\";\n> >\n> > - ret = apply_cached(&out);\n> > - strbuf_release(&out);\n> > - if (ret)\n> > + o.verbosity = 0;\n> > +\n> > + head = lookup_tree(o.repo, &c_tree);\n> > + merge = lookup_tree(o.repo, &info->i_tree);\n> > + merge_base = lookup_tree(o.repo, &info->b_tree);\n> > +\n> > + merge_incore_nonrecursive(&o, head, merge, merge_base,\n> > + &result);\n> > +\n> > + if (!result.clean)\n> > return error(_(\"conflicts in index. \"\n> > \"Try without --index.\"));\n>\n> Sorry, I missed this last time, but we should finalize the merge before\n> returning to ensure the allocations in result are freed.\n\nYeah, I think CI caught this:\nhttps://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31\n\nBut I'm not sure I could have understood what it was telling me\nwithout your hint, thanks!\n\n-- \nD. Ben Knoble\n"},{"id":"553279","messageId":"CALnO6CBhoBcVjLXidvii+o_Ump_k9disW177LeSS0118t3oGKg@mail.gmail.com","threadId":"66355","inReplyTo":"xmqqse2yz4y4.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-25T13:00:51Z","receivedAt":"2026-09-25T13:01:03Z","isPatch":true,"body":"On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote:\n>\n> Ahh, or perhaps the trees are indeed given in a wrong order, but not\n> in a random wrong order. merge_ort_nonrecursive(), which is *not*\n> the function you are using, takes head, merge, and merge_base in\n> this order, and that order matches what you wrote.\n>\n> Perhaps the true culprit in this confusion is that the order in\n> which merge_ort_nonrecursive() takes its three trees (head, merge,\n> and common) and the order in which merge_incore_nonrecursive() takes\n> its trees (merge_base, side1, and side2) are different, and if we\n> fix them to match, it would make it easier to work with?\n\nIndeed, the confusion is that simple ;) Shamefully, we don't have\nenough test coverage to catch that regression, so I'm very glad indeed\nyou spotted it.\n\n> The new test in the attached patch will fail with this step but if\n> we revert the changes to builtin/stash.c in this step, it passes.\n\nAny objection to me adding this test as a preparatory patch? There's\nno sign-off, so I don't want to mess up the DCO here.\n"},{"id":"553280","messageId":"CALnO6CDTaunaBby+Gy4B5vxiHES3DHpybv8Eq2JPvQ1cteGzrw@mail.gmail.com","threadId":"66355","inReplyTo":"232f2bf6-04d8-4a54-b4e9-51b5ee79799f@gmail.com","subject":"Re: [PATCH v2 3/4] t: test failed \"stash apply --index\"","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-25T13:36:15Z","receivedAt":"2026-09-25T13:36:27Z","isPatch":true,"body":"Hi Phillip,\n\nOn Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 23/09/2026 13:58, D. Ben Knoble wrote:\n> > The next commit will refactor index handling for applied stashes, so\n> > let's make sure we cover conflicted index merging, too.\n> >\n> > Signed-off-by: D. Ben Knoble \n> > ---\n> > t/t3903-stash.sh | 18 ++++++++++++++++++\n> > 1 file changed, 18 insertions(+)\n> >\n> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> > index 721158606f..3958ab3c8d 100755\n> > --- a/t/t3903-stash.sh\n> > +++ b/t/t3903-stash.sh\n> > @@ -374,6 +374,24 @@ setup_stash() {\n> > test_cmp expect actual\n> > '\n> >\n> > +test_expect_success 'stash apply --index leaves everything untouched on failure' '\n> > + git reset --hard &&\n> > + echo test >other-file &&\n> > + git add other-file &&\n> > + git stash &&\n> > + echo unrelated >file &&\n> > + echo unrelated >another-file &&\n> > + git add another-file &&\n> > + git diff-files >expect &&\n>\n> diff-files shows the worktree blobs as null object ids, so comparing\n> this before and after stashing only tells us that the same set of files\n> have unstaged changes, not that the unstaged changes are the same.\n> Adding \"-p\" would check the worktree files are unchanged.\n\nI confess I played with diff-files and diff-index manually before\ntrying to construct this test case, and I still don't totally\nunderstand how they're being used in the test just prior…\n\nAnway, it looks to me like \"diff-files -p\" is the same as \"diff -p\"\n(albeit without some niceties like color-moved applying automatically\nfrom config), so that would make the test quite a bit more\ncomplicated, no? (The \"index $sha1..$sha2\" line would change…)\n\nSince we know what the expected contents are, perhaps we can simply\nassert on those.\n\nHm. I spent some time with test_pause in the previous test, and I\nthink my concerns about that line changing are moot. But, asserting on\nthe contents is also a bit silly (as that's what the blob IDs are\ndoing for us in the output).\n\n> > + echo conflict >other-file &&\n> > + git add other-file &&\n>\n> I wonder if we should to add \"git diff-index --cached HEAD\n> >expect-index\" here so we can check the index is unchanged as well. For\n> the paths that have unstaged changes we're already checking the index\n> object ids via \"diff-files\", but I think in theory it would be possible\n> to have an identical change in the index and worktree that is not picked\n> up by that.\n\nSo, this test sets up an intermediate state prior to attempting to unstash where\n\n- another-file is new in the index & working tree (content: \"unrelated\")\n- other-file is modified in the index & working tree (content: from\n\"6\" to \"conflict\")\n- file is modified in the working tree (content: from \"bar\" to \"unrelated\")\n\nAnd we should still be there when finished. (I wonder if, like the\nprevious test, we should have a file that differs from itself in the\nindex and working tree?)\n\nSo overall, I'm thinking\n\n- (old) diff-files only shows file is changed\n- diff-files -p shows us changes for file, better (and won't show the\nother 2 files unless they've become unstaged)\n- diff-index --cached HEAD helps us check all the index changes\n\nPhew! Thanks for reading my rambling thinking aloud :)\n\n-- \nD. Ben Knoble\n"},{"id":"553286","messageId":"a9c44afa-583e-45ad-9447-c00144141c32@gmail.com","threadId":"66355","inReplyTo":"CALnO6CDTaunaBby+Gy4B5vxiHES3DHpybv8Eq2JPvQ1cteGzrw@mail.gmail.com","subject":"Re: [PATCH v2 3/4] t: test failed \"stash apply --index\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-25T15:45:42Z","receivedAt":"2026-09-25T15:45:49Z","isPatch":true,"body":"Hi Ben\n\nOn 25/09/2026 14:36, D. Ben Knoble wrote:\n> \n> So overall, I'm thinking\n> \n> - (old) diff-files only shows file is changed\n> - diff-files -p shows us changes for file, better (and won't show the\n> other 2 files unless they've become unstaged)\n> - diff-index --cached HEAD helps us check all the index changes\n\nI think that sounds reasonable, we can delete the index lines from the \npatch output with sed to make it easier to compare them.\n\nThanks\n\nPhillip\n\n> Phew! Thanks for reading my rambling thinking aloud :)\n> \n\n"},{"id":"553287","messageId":"36e1073e-fa55-4d7d-8b8b-ba9ac34976fa@gmail.com","threadId":"66355","inReplyTo":"CALnO6CDpS9GQfONKJs=LAUvwYzYyMby+rGAUtvFQruj-ERXt-g@mail.gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-25T15:58:16Z","receivedAt":"2026-09-25T15:58:25Z","isPatch":true,"body":"Hi Ben\n\nOn 25/09/2026 13:55, D. Ben Knoble wrote:\n> On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote:\n>>\n>> Sorry, I missed this last time, but we should finalize the merge before\n>> returning to ensure the allocations in result are freed.\n> \n> Yeah, I think CI caught this:\n> https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31\n> \n> But I'm not sure I could have understood what it was telling me\n> without your hint, thanks!\n\nYes, that output is terrible - to see the leaks you have to scroll to \nline 28282 of \"print test failures\" which is ridiculous. See \nhttps://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:10:28282\n\nThanks\n\nPhillip>\n\n"},{"id":"553288","messageId":"6e6420e8-3cbd-4975-a781-645e1ffbc1d2@gmail.com","threadId":"66355","inReplyTo":"xmqqse2yz4y4.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-25T16:04:34Z","receivedAt":"2026-09-25T16:04:42Z","isPatch":true,"body":"Hi Junio\n\nOn 24/09/2026 22:59, Junio C Hamano wrote:\n> \"D. Ben Knoble\" writes:\n> \n> Ahh, or perhaps the trees are indeed given in a wrong order, but not\n> in a random wrong order. merge_ort_nonrecursive(), which is *not*\n> the function you are using, takes head, merge, and merge_base in\n> this order, and that order matches what you wrote.\n\nOuch that's nasty. Well spotted, I missed it when I read the code \n(because the arguments were in the same order as the call to \nmerge_ort_nonrecursive()) and the tests we have use the same version of \nthe file for \"base\" and \"stage2\" so do not notice if they'd been \ntransposed. It is rather confusing that two functions that are so \nclosely related take their arguments in a different order.\n\n> Perhaps the true culprit in this confusion is that the order in\n> which merge_ort_nonrecursive() takes its three trees (head, merge,\n> and common) and the order in which merge_incore_nonrecursive() takes\n> its trees (merge_base, side1, and side2) are different, and if we\n> fix them to match, it would make it easier to work with?\n\nI think it is definitely worth fixing them to take the trees in the same \norder. My preference would be \"base\", \"stage1\", \"stage2\" but so long as \nthey match each other I dont object to \"stage1\", \"stage2\", \"base\".\n\nThanks\n\nPhillip\n\n> The new test in the attached patch will fail with this step but if\n> we revert the changes to builtin/stash.c in this step, it passes.\n> \n> t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++\n> 1 file changed, 32 insertions(+)\n> \n> diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh\n> index 3958ab3c8d..0a87e62b11 100755\n> --- c/t/t3903-stash.sh\n> +++ w/t/t3903-stash.sh\n> @@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' '\n> \ttest_cmp expect actual\n> '\n> \n> +\n> +test_expect_success 'stash apply --index does not revert unrelated upstream index changes' '\n> +\ttest_when_finished \"rm -fr playpen\" &&\n> +\tmkdir playpen &&\n> +\t(\n> +\t\tcd playpen &&\n> +\t\tgit init &&\n> +\t\techo \"base1\" >file1 &&\n> +\t\techo \"base2\" >file2 &&\n> +\t\tgit add file1 file2 &&\n> +\t\tgit commit -m \"initial base\" &&\n> +\n> +\t\t# Make a staged change to file1 and stash it\n> +\t\techo \"staged1\" >file1 &&\n> +\t\tgit add file1 &&\n> +\t\tgit stash &&\n> +\n> +\t\t# Upstream advances by modifying unrelated file2\n> +\t\techo \"upstream2\" >file2 &&\n> +\t\tgit add file2 &&\n> +\t\tgit commit -m \"upstream change to file2\" &&\n> +\n> +\t\t# Apply the stash with --index\n> +\t\tgit stash apply --index &&\n> +\n> +\t\t# Verify working tree and index state\n> +\t\ttest \"$(git show :file1)\" = \"staged1\" &&\n> +\t\ttest \"$(git show :file2)\" = \"upstream2\" &&\n> +\t\ttest \"$(git show HEAD:file2)\" = \"upstream2\"\n> +\t)\n> +'\n> +\n> test_expect_success 'stash apply --index leaves everything untouched on failure' '\n> \tgit reset --hard &&\n> \techo test >other-file &&\n\n"},{"id":"553292","messageId":"CALnO6CB1ptzX1QC=ou4V+tRp9RKHSCKoyh5KqXdBCjGuhKxnnQ@mail.gmail.com","threadId":"66355","inReplyTo":"36e1073e-fa55-4d7d-8b8b-ba9ac34976fa@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-25T16:16:17Z","receivedAt":"2026-09-25T16:16:29Z","isPatch":true,"body":"On Fri, Sep 25, 2026 at 11:58 AM Phillip Wood wrote:\n> On 25/09/2026 13:55, D. Ben Knoble wrote:\n> > On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood wrote:\n> >>\n> >> Sorry, I missed this last time, but we should finalize the merge before\n> >> returning to ensure the allocations in result are freed.\n> >\n> > Yeah, I think CI caught this:\n> > https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31\n> >\n> > But I'm not sure I could have understood what it was telling me\n> > without your hint, thanks!\n>\n> Yes, that output is terrible - to see the leaks you have to scroll to\n> line 28282 of \"print test failures\" which is ridiculous. See\n> https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:10:28282\n\nAh, sorry. My link was sloppy.\n\nI did get that far, but the allocation backtrace doesn't make it\nobvious that merge_result is what leaked, and that's where I was\nsaying an especial thank you ;)\n\n-- \nD. Ben Knoble\n"},{"id":"553293","messageId":"CALnO6CDzbUMSAqLgZ_A1xx=XJPN1_HR-tJUDqG4-Q_xV2Ypzkg@mail.gmail.com","threadId":"66355","inReplyTo":"6e6420e8-3cbd-4975-a781-645e1ffbc1d2@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-25T16:17:31Z","receivedAt":"2026-09-25T16:17:45Z","isPatch":true,"body":"On Fri, Sep 25, 2026 at 12:04 PM Phillip Wood wrote:\n>\n> Hi Junio\n>\n> On 24/09/2026 22:59, Junio C Hamano wrote:\n\n[snip]\n\n> > Perhaps the true culprit in this confusion is that the order in\n> > which merge_ort_nonrecursive() takes its three trees (head, merge,\n> > and common) and the order in which merge_incore_nonrecursive() takes\n> > its trees (merge_base, side1, and side2) are different, and if we\n> > fix them to match, it would make it easier to work with?\n>\n> I think it is definitely worth fixing them to take the trees in the same\n> order. My preference would be \"base\", \"stage1\", \"stage2\" but so long as\n> they match each other I dont object to \"stage1\", \"stage2\", \"base\".\n>\n> Thanks\n>\n> Phillip\n\nFWIW, I concur with changing them (and Phillip's preference of order),\nbut I'll elect to leave that out of scope for this series.\n\n-- \nD. Ben Knoble\n"},{"id":"553294","messageId":"xmqqpky1wb76.fsf@gitster.g","threadId":"66355","inReplyTo":"CALnO6CBhoBcVjLXidvii+o_Ump_k9disW177LeSS0118t3oGKg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-25T16:24:45Z","receivedAt":"2026-09-25T16:24:49Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote:\n>>\n>> Ahh, or perhaps the trees are indeed given in a wrong order, but not\n>> in a random wrong order. merge_ort_nonrecursive(), which is *not*\n>> the function you are using, takes head, merge, and merge_base in\n>> this order, and that order matches what you wrote.\n>>\n>> Perhaps the true culprit in this confusion is that the order in\n>> which merge_ort_nonrecursive() takes its three trees (head, merge,\n>> and common) and the order in which merge_incore_nonrecursive() takes\n>> its trees (merge_base, side1, and side2) are different, and if we\n>> fix them to match, it would make it easier to work with?\n>\n> Indeed, the confusion is that simple ;) Shamefully, we don't have\n> enough test coverage to catch that regression, so I'm very glad indeed\n> you spotted it.\n>\n>> The new test in the attached patch will fail with this step but if\n>> we revert the changes to builtin/stash.c in this step, it passes.\n>\n> Any objection to me adding this test as a preparatory patch? There's\n> no sign-off, so I don't want to mess up the DCO here.\n\nIt was written merely as an illustration and is not something I am\nproud of. For example, creating a totally new playpen repository\nonly for a single piece of test and remove the entire thing when the\nsingle test piece is done was done only to make sure the existing\ntest that come later can never be affected. Also the test only uses\nthe most trivial case (a file is added in the stashed change, nobody\nelse involved in the stash application has touched the file so there\nis nothing to \"merge\" in the file). It was enough to demonstrate\nthat the order of arguments given to the function was wrong, but\nwe wouldn't catch problems in content-level merge with such a test.\n\nSo, I wouldn't mind if you reused that as one in a series of tests,\nbut I'd prefer to see those who are move invested in the topic to\ncome up with a bit more realistic scenario.\n\nThanks.\n"},{"id":"553302","messageId":"xmqq33uxwa1y.fsf@gitster.g","threadId":"66355","inReplyTo":"CALnO6CDzbUMSAqLgZ_A1xx=XJPN1_HR-tJUDqG4-Q_xV2Ypzkg@mail.gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-25T16:49:29Z","receivedAt":"2026-09-25T16:49:37Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n>> I think it is definitely worth fixing them to take the trees in the same\n>> order. My preference would be \"base\", \"stage1\", \"stage2\" but so long as\n>> they match each other I dont object to \"stage1\", \"stage2\", \"base\".\n>>\n>> Thanks\n>>\n>> Phillip\n>\n> FWIW, I concur with changing them (and Phillip's preference of order),\n> but I'll elect to leave that out of scope for this series.\n\nOh, absolutely it is out of scope for this series.\n"},{"id":"553343","messageId":"c2bab13f-a9f1-473d-97aa-c201b2060bfd@gmail.com","threadId":"66355","inReplyTo":"xmqqpky1wb76.fsf@gitster.g","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-26T09:51:16Z","receivedAt":"2026-09-26T09:51:25Z","isPatch":true,"body":"On 25/09/2026 17:24, Junio C Hamano wrote:\n> \"D. Ben Knoble\" writes:\n> \n>> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote:\n>>>\n>>> Ahh, or perhaps the trees are indeed given in a wrong order, but not\n>>> in a random wrong order. merge_ort_nonrecursive(), which is *not*\n>>> the function you are using, takes head, merge, and merge_base in\n>>> this order, and that order matches what you wrote.\n>>>\n>>> Perhaps the true culprit in this confusion is that the order in\n>>> which merge_ort_nonrecursive() takes its three trees (head, merge,\n>>> and common) and the order in which merge_incore_nonrecursive() takes\n>>> its trees (merge_base, side1, and side2) are different, and if we\n>>> fix them to match, it would make it easier to work with?\n>>\n>> Indeed, the confusion is that simple ;) Shamefully, we don't have\n>> enough test coverage to catch that regression, so I'm very glad indeed\n>> you spotted it.\n>>\n>>> The new test in the attached patch will fail with this step but if\n>>> we revert the changes to builtin/stash.c in this step, it passes.\n>>\n>> Any objection to me adding this test as a preparatory patch? There's\n>> no sign-off, so I don't want to mess up the DCO here.\n> \n> It was written merely as an illustration and is not something I am\n> proud of. For example, creating a totally new playpen repository\n> only for a single piece of test and remove the entire thing when the\n> single test piece is done was done only to make sure the existing\n> test that come later can never be affected. Also the test only uses\n> the most trivial case (a file is added in the stashed change, nobody\n> else involved in the stash application has touched the file so there\n> is nothing to \"merge\" in the file). It was enough to demonstrate\n> that the order of arguments given to the function was wrong, but\n> we wouldn't catch problems in content-level merge with such a test.\n> \n> So, I wouldn't mind if you reused that as one in a series of tests,\n> but I'd prefer to see those who are move invested in the topic to\n> come up with a bit more realistic scenario.\n\nMaybe something like the test below (which I admit I haven't actually \ntested). That checks we merge the file contents and puts the changes in \nthe file close enough together so that the old code would fail and has \ndifferent contents for the three merged blobs.\n\ntest_write_lines A B C >file &&\ngit commit -m xxx file &&\ntest_write_lines A B staged >file &&\ngit add file &&\ntest_write_lines A B unstaged >file &&\ngit stash &&\ntest_write_lines committed B C >file &&\ngit commit -m yyy file &&\ngit stash pop --index &&\ngit show :file >actual &&\ntest_write_lines committed B staged >expect &&\ntext_cmp expect actual &&\ntest_write_lines committed B unstaged >expect &&\ntest_cmp expect file\n\nThanks\n\nPhillip\n\n\n\n"},{"id":"553344","messageId":"b4023f5d-efba-487e-b273-a4283c50a774@gmail.com","threadId":"66355","inReplyTo":"a9c44afa-583e-45ad-9447-c00144141c32@gmail.com","subject":"Re: [PATCH v2 3/4] t: test failed \"stash apply --index\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-26T09:53:22Z","receivedAt":"2026-09-26T09:53:30Z","isPatch":true,"body":"On 25/09/2026 16:45, Phillip Wood wrote:\n> \n> I think that sounds reasonable, we can delete the index lines from the \n> patch output with sed to make it easier to compare them.\n\nI just opened the test file and realized it has a diff_cmp() function to \ncompare diffs ignoring the index lines\n\nThanks\n\nPhillip\n\n"},{"id":"553348","messageId":"CALnO6CC5bj0-yhoMD3AUGcO=uxX+y4btC=nGZ6QbmOoGr97B3w@mail.gmail.com","threadId":"66355","inReplyTo":"c2bab13f-a9f1-473d-97aa-c201b2060bfd@gmail.com","subject":"Re: [PATCH v2 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:04:28Z","receivedAt":"2026-09-26T12:04:42Z","isPatch":true,"body":"On Sat, Sep 26, 2026 at 5:51 AM Phillip Wood wrote:\n>\n> On 25/09/2026 17:24, Junio C Hamano wrote:\n> > \"D. Ben Knoble\" writes:\n> >\n> >> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano wrote:\n> >>>\n> >>> Ahh, or perhaps the trees are indeed given in a wrong order, but not\n> >>> in a random wrong order. merge_ort_nonrecursive(), which is *not*\n> >>> the function you are using, takes head, merge, and merge_base in\n> >>> this order, and that order matches what you wrote.\n> >>>\n> >>> Perhaps the true culprit in this confusion is that the order in\n> >>> which merge_ort_nonrecursive() takes its three trees (head, merge,\n> >>> and common) and the order in which merge_incore_nonrecursive() takes\n> >>> its trees (merge_base, side1, and side2) are different, and if we\n> >>> fix them to match, it would make it easier to work with?\n> >>\n> >> Indeed, the confusion is that simple ;) Shamefully, we don't have\n> >> enough test coverage to catch that regression, so I'm very glad indeed\n> >> you spotted it.\n> >>\n> >>> The new test in the attached patch will fail with this step but if\n> >>> we revert the changes to builtin/stash.c in this step, it passes.\n> >>\n> >> Any objection to me adding this test as a preparatory patch? There's\n> >> no sign-off, so I don't want to mess up the DCO here.\n> >\n> > It was written merely as an illustration and is not something I am\n> > proud of. For example, creating a totally new playpen repository\n> > only for a single piece of test and remove the entire thing when the\n> > single test piece is done was done only to make sure the existing\n> > test that come later can never be affected. Also the test only uses\n> > the most trivial case (a file is added in the stashed change, nobody\n> > else involved in the stash application has touched the file so there\n> > is nothing to \"merge\" in the file). It was enough to demonstrate\n> > that the order of arguments given to the function was wrong, but\n> > we wouldn't catch problems in content-level merge with such a test.\n> >\n> > So, I wouldn't mind if you reused that as one in a series of tests,\n> > but I'd prefer to see those who are move invested in the topic to\n> > come up with a bit more realistic scenario.\n>\n> Maybe something like the test below (which I admit I haven't actually\n> tested). That checks we merge the file contents and puts the changes in\n> the file close enough together so that the old code would fail and has\n> different contents for the three merged blobs.\n>\n> test_write_lines A B C >file &&\n> git commit -m xxx file &&\n> test_write_lines A B staged >file &&\n> git add file &&\n> test_write_lines A B unstaged >file &&\n> git stash &&\n> test_write_lines committed B C >file &&\n> git commit -m yyy file &&\n> git stash pop --index &&\n> git show :file >actual &&\n> test_write_lines committed B staged >expect &&\n> text_cmp expect actual &&\n\ns/text/test ;)\n\n> test_write_lines committed B unstaged >expect &&\n> test_cmp expect file\n\nThis does fail on the original code (head, base, merge_base) because\nthe index (git show :file) has \"A B staged\" lines instead of\n\"committed B staged\" lines.\n\nThis test does pass on the new code, but needs some\narrangement/cleanup for the later \"stash -k\" test to succeed, so I'll\ninclude that in the next round as well.\n\n-- \nD. Ben Knoble\n"},{"id":"553349","messageId":"CALnO6CAwN=Xx5NUqNg8KZ9gf9Nn+nuSP6Yn3YnxyX5w8HqhkcQ@mail.gmail.com","threadId":"66355","inReplyTo":"b4023f5d-efba-487e-b273-a4283c50a774@gmail.com","subject":"Re: [PATCH v2 3/4] t: test failed \"stash apply --index\"","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:07:50Z","receivedAt":"2026-09-26T12:08:03Z","isPatch":true,"body":"On Sat, Sep 26, 2026 at 5:53 AM Phillip Wood wrote:\n>\n> On 25/09/2026 16:45, Phillip Wood wrote:\n> >\n> > I think that sounds reasonable, we can delete the index lines from the\n> > patch output with sed to make it easier to compare them.\n>\n> I just opened the test file and realized it has a diff_cmp() function to\n> compare diffs ignoring the index lines\n>\n> Thanks\n>\n> Phillip\n\nDoh!\n\nOn the other hand, I don't think we need it. Those lines are showing\nblob IDs, which would be stable in our case (fixed hash algorithm over\nfixed contents), and we don't make the test dependent on the actual\nIDs?\n\n-- \nD. Ben Knoble\n"},{"id":"553350","messageId":"cover.1790425008.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790168285.git.ben.knoble@gmail.com","subject":"[PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:16:43Z","receivedAt":"2026-09-26T12:17:30Z","isPatch":true,"body":"Hi all,\n\nThis small patch series fixes a bug reported by Eli Barzilay in the\ninteraction between autostashing, staged index entries, and\nstash.index=true.\n\nThe first patch is an incidental cleanup, and the second re-arranges one\nline to make the change easier. The third and fourth add missing test\ncoverage (which catch breakages from prior incorrect rounds of this\nseries), while the last holds the interesting bits.\n\nChanges in v3:\n\n• Change conflict label for current index\n• Fix memory leak of merge_result\n• Fix order of trees to make the correct merge (cherry-pick)\n • New test (3/5) to validate this\n• Fix test in 4/5 to assert more details of expected state\n\nChanges in v2:\n\n• Do give branch labels for the incore merge, although they are never\n seen (and clarify commit message as a result, also keeping the\n merge-ort asserts). Phillip was right: without those, we do segfault\n on conflicts.\n• Use the ui merge options to keep the same diff algorithm.\n• Use merge_finalize instead of clear_merge_options, and reuse the\n options between merge calls if they are already initialized.\n• Add a new 2/4 to simplify merge options initialization.\n• Add a new 3/4 with a test case for conflicted index merges.\n\nv1: \nv2: \n\n[1/5] builtin/stash: remove unused header\n[2/5] stash: prepare merge options earlier\n[3/5] t3903: test stash --index merges\n[4/5] t3903: test failed \"stash apply --index\"\n[5/5] builtin/stash: merge index in-core\n\n builtin/stash.c | 85 +++++++++++-------------------------------------\n t/t3903-stash.sh | 42 ++++++++++++++++++++++++\n t/t7600-merge.sh | 9 +++++\n 3 files changed, 70 insertions(+), 66 deletions(-)\n\nDiff-intervalle contre v2 :\n1: b6798c8a25 = 1: 6a165c4df4 builtin/stash: remove unused header\n2: 1e2343c7fc = 2: d9a9e18f3a stash: prepare merge options earlier\n-: ---------- > 3: 8b5ea5e6f4 t3903: test stash --index merges\n3: 5bd4b78cac ! 4: d39e16905d t: test failed \"stash apply --index\"\n @@ Metadata\n Author: D. Ben Knoble \n \n ## Commit message ##\n - t: test failed \"stash apply --index\"\n + t3903: test failed \"stash apply --index\"\n \n The next commit will refactor index handling for applied stashes, so\n let's make sure we cover conflicted index merging, too.\n \n + Helped-by: Phillip Wood \n +\n ## t/t3903-stash.sh ##\n @@ t/t3903-stash.sh: setup_stash() {\n - \ttest_cmp expect actual\n + \ttest_cmp expect file\n '\n \n +test_expect_success 'stash apply --index leaves everything untouched on failure' '\n @@ t/t3903-stash.sh: setup_stash() {\n +\techo unrelated >file &&\n +\techo unrelated >another-file &&\n +\tgit add another-file &&\n -+\tgit diff-files >expect &&\n -+\n +\techo conflict >other-file &&\n +\tgit add other-file &&\n ++\tgit diff-files -p >expect &&\n ++\tgit diff-index --cached HEAD >expect-index &&\n ++\n +\ttest_must_fail git stash apply --index 2>err &&\n +\ttest_grep \"conflicts in index. Try without --index\" err &&\n -+\tgit diff-files >actual &&\n -+\ttest_cmp expect actual\n ++\tgit diff-files -p >actual &&\n ++\ttest_cmp expect actual &&\n ++\tgit diff-index --cached HEAD >actual-index &&\n ++\ttest_cmp expect-index actual-index\n +'\n +\n test_expect_success 'stash -k' '\n4: e49936ee12 ! 5: fde7fb7988 builtin/stash: merge index in-core\n @@ Commit message\n \n Reported-by: Eli Barzilay \n Helped-by: Phillip Wood \n + Helped-by: Junio C Hamano \n \n ## builtin/stash.c ##\n @@ builtin/stash.c: static int create_index_from_tree(const struct object_id *tree_id,\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n -\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n -\t\t\t\t\t oid_to_hex(&info->w_commit));\n -\t\t\t}\n -+\t\t\to.branch1 = \"Upstream index\";\n ++\t\t\to.branch1 = \"Current index\";\n +\t\t\to.branch2 = \"Stashed index changes\";\n +\t\t\to.ancestor = \"Stash base\";\n \n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n +\n -+\t\t\tmerge_incore_nonrecursive(&o, head, merge, merge_base,\n ++\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n +\t\t\t\t\t\t &result);\n +\n ++\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n ++\t\t\tmerge_finalize(&o, &result);\n ++\n +\t\t\tif (!result.clean)\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n - \n +-\n -\t\t\tdiscard_index(the_repository->index);\n -\t\t\trepo_read_index(the_repository);\n -\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n -\t\t\treset_head();\n -\t\t\tdiscard_index(the_repository->index);\n -\t\t\trepo_read_index(the_repository);\n -+\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n -+\t\t\tmerge_finalize(&o, &result);\n \t\t}\n \t}\n \n\nbase-commit: d38352cd43ab9745686d697872408bc3249a153f\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553351","messageId":"6a165c4df456b6bd5e5ab46664b023a45e670926.1790425008.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"[PATCH v3 1/5] builtin/stash: remove unused header","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:16:44Z","receivedAt":"2026-09-26T12:17:32Z","isPatch":true,"body":"Clang complains that oid-array.h is unused. Certainly none of the\noid_array* functions, types, etc., are used, and the\ntransitively-included hash.h declarations are used but covered by a\npre-existing direct #include of hash.h.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7a9843413b..dfea2d2c4c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -31,7 +31,6 @@\n #include \"reflog.h\"\n #include \"reflog-walk.h\"\n #include \"add-interactive.h\"\n-#include \"oid-array.h\"\n #include \"commit.h\"\n \n #define INCLUDE_ALL_FILES 2\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553352","messageId":"d9a9e18f3aa334e6e294b825d22df16830a1d616.1790425008.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"[PATCH v3 2/5] stash: prepare merge options earlier","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:16:45Z","receivedAt":"2026-09-26T12:17:36Z","isPatch":true,"body":"In a future commit, we will reuse these options for the index merge of\n\"apply --index\", not just for the worktree.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex dfea2d2c4c..043a38cc6d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -664,6 +664,8 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n \t\treturn error(_(\"cannot apply a stash in the middle of a merge\"));\n \n+\tinit_ui_merge_options(&o, the_repository);\n+\n \tif (index) {\n \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n \t\t oideq(&c_tree, &info->i_tree)) {\n@@ -695,8 +697,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t}\n \t}\n \n-\tinit_ui_merge_options(&o, the_repository);\n-\n \to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n \to.branch2 = label_theirs ? label_theirs : \"Stashed changes\";\n \to.ancestor = label_base ? label_base : \"Stash base\";\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553353","messageId":"8b5ea5e6f47ee9a57df3a4d97a457d024b3dec00.1790425008.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"[PATCH v3 3/5] t3903: test stash --index merges","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:16:46Z","receivedAt":"2026-09-26T12:17:40Z","isPatch":true,"body":"A future commit will refactor index handling for applied stashes, and we\nneed to take care to get the order of trees right when merging. Add a\ntest that covers this case.\n\nSuggested-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n t/t3903-stash.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 721158606f..9bc99fa252 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -374,6 +374,27 @@ setup_stash() {\n \ttest_cmp expect actual\n '\n \n+# the later \"stash -k\" test is not expecting us to muck with file so much, so\n+# reset when finished\n+test_expect_success 'stash apply --index merges the correct trees' '\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git reset --hard $head\" &&\n+\ttest_write_lines A B C >file &&\n+\tgit commit -m setup file &&\n+\ttest_write_lines A B staged >file &&\n+\tgit add file &&\n+\ttest_write_lines A B unstaged >file &&\n+\tgit stash &&\n+\ttest_write_lines committed B C >file &&\n+\tgit commit -m to-be-merged file &&\n+\tgit stash pop --index &&\n+\tgit show :file >actual &&\n+\ttest_write_lines committed B staged >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_write_lines committed B unstaged >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553354","messageId":"d39e16905da69ee8f00aef939b56708b90ba0c02.1790425008.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"[PATCH v3 4/5] t3903: test failed \"stash apply --index\"","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:16:47Z","receivedAt":"2026-09-26T12:17:42Z","isPatch":true,"body":"The next commit will refactor index handling for applied stashes, so\nlet's make sure we cover conflicted index merging, too.\n\nHelped-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n t/t3903-stash.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 9bc99fa252..11942d875b 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -395,6 +395,27 @@ setup_stash() {\n \ttest_cmp expect file\n '\n \n+test_expect_success 'stash apply --index leaves everything untouched on failure' '\n+\tgit reset --hard &&\n+\techo test >other-file &&\n+\tgit add other-file &&\n+\tgit stash &&\n+\techo unrelated >file &&\n+\techo unrelated >another-file &&\n+\tgit add another-file &&\n+\techo conflict >other-file &&\n+\tgit add other-file &&\n+\tgit diff-files -p >expect &&\n+\tgit diff-index --cached HEAD >expect-index &&\n+\n+\ttest_must_fail git stash apply --index 2>err &&\n+\ttest_grep \"conflicts in index. Try without --index\" err &&\n+\tgit diff-files -p >actual &&\n+\ttest_cmp expect actual &&\n+\tgit diff-index --cached HEAD >actual-index &&\n+\ttest_cmp expect-index actual-index\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553355","messageId":"fde7fb7988b695707c6f2776adc18eec7fe4696a.1790425008.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"[PATCH v3 5/5] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:16:48Z","receivedAt":"2026-09-26T12:17:46Z","isPatch":true,"body":"\"git stash apply --index\" does a 2-step dance to report index conflicts\nbefore carrying out the main unstash: first, attempt to merge the index\n(and remember the name of the resulting tree). If that succeeds, reset\nthe index and carry on unstashing the working tree, then use the\nremembered index tree to unstash the index.\n\nThe \"merge the index\" step is performed on the actual index by a\ncombination of git-diff-tree(1) and git-apply(1), which incurs an extra\ncost to git-reset(1) to cleanup. This also introduces an autostash bug\nwhen stash.index is true: \"git reset\" eventually wants to\nremove_merge_branch_state(), which calls save_autostash() due to\na03b55530a (merge: teach --autostash option, 2020-04-07). This can\nhappen from a \"git merge --autostash\", which itself calls\nsave_autostash(). Operating on the file-system in this way is not\nre-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH\nref [1]. This bug has lurked for a while, but it would have been\nimpossible to trigger without the availability of stash.index to force\nthe autostash apply into index mode.\n\n[1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/\n\nFortunately, we can achieve 2 goals at once: avoid round-tripping to the\nfile-system (and invoking expensive subprocesses) by performing the\nmerge in-core. If there are conflicts, we discard the resulting tree, so\nwe don't see the usual branch and ancestor labels, but the merge\nsubroutines insist on their presence, so use something simple.\n\nWe *could* swap just the git-reset(1) subprocess with our internal\nreset_tree() and refresh_index(), which would fix the bug. We'd much\nprefer to clean up these vestiges of the shell-based git-stash, though.\n\nReported-by: Eli Barzilay \nHelped-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 80 ++++++++++--------------------------------------\n t/t7600-merge.sh | 9 ++++++\n 2 files changed, 26 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 043a38cc6d..ac3b3cf84d 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,\n \treturn ret;\n }\n \n-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *w_commit_hex = oid_to_hex(w_commit);\n-\n-\t/*\n-\t * Diff-tree would not be very hard to replace with a native function,\n-\t * however it should be done together with apply_cached.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n-\tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n-\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n-\n-static int apply_cached(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Apply currently only reads either from stdin or a file, thus\n-\t * apply_all_patches would have to be updated to optionally take a\n-\t * buffer.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n-}\n-\n-static int reset_head(void)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Reset is overall quite simple, however there is no current public\n-\t * API for resetting.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n-\n-\treturn run_command(&cp);\n-}\n-\n static int is_path_a_directory(const char *path)\n {\n \t/*\n@@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t oideq(&c_tree, &info->i_tree)) {\n \t\t\thas_index = 0;\n \t\t} else {\n-\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct merge_result result = { 0 };\n \n-\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n-\t\t\t\tstrbuf_release(&out);\n-\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n-\t\t\t\t\t oid_to_hex(&info->w_commit));\n-\t\t\t}\n+\t\t\to.branch1 = \"Current index\";\n+\t\t\to.branch2 = \"Stashed index changes\";\n+\t\t\to.ancestor = \"Stash base\";\n \n-\t\t\tret = apply_cached(&out);\n-\t\t\tstrbuf_release(&out);\n-\t\t\tif (ret)\n+\t\t\to.verbosity = 0;\n+\n+\t\t\thead = lookup_tree(o.repo, &c_tree);\n+\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n+\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n+\t\t\t\t\t\t &result);\n+\n+\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n+\t\t\tmerge_finalize(&o, &result);\n+\n+\t\t\tif (!result.clean)\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n-\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n-\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n-\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n-\t\t\t\treturn error(_(\"could not save index tree\"));\n-\n-\t\t\treset_head();\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n \t\t}\n \t}\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 64fe21717d..8f6109fb91 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -801,6 +801,15 @@ verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n '\n \n+test_expect_success 'fast-forward merge with --autostash, stash.index' '\n+\tgit reset --hard c0 &&\n+\tgit stash clear &&\n+\techo staged >>z && git add z &&\n+\tgit -c stash.index=true merge --autostash c1 2>err &&\n+\ttest_grep \"Applied autostash.\" err &&\n+\ttest_stdout_line_count = 0 git stash list\n+'\n+\n test_expect_success 'failed fast-forward merge with --autostash' '\n \tgit reset --hard c0 &&\n \tgit merge-file file file.orig file.5 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553356","messageId":"CALnO6CBpV6TiQGKSxEcurwzZEE3rvqO0EryO=5orD8F3Ren8Kg@mail.gmail.com","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-26T12:20:58Z","receivedAt":"2026-09-26T12:21:09Z","isPatch":true,"body":"On Sat, Sep 26, 2026 at 8:17 AM D. Ben Knoble wrote:\n>\n> Hi all,\n>\n> This small patch series fixes a bug reported by Eli Barzilay in the\n> interaction between autostashing, staged index entries, and\n> stash.index=true.\n>\n> The first patch is an incidental cleanup, and the second re-arranges one\n> line to make the change easier. The third and fourth add missing test\n> coverage (which catch breakages from prior incorrect rounds of this\n> series), while the last holds the interesting bits.\n>\n> Changes in v3:\n\nWoops. Contrary to my usual practice of late, I sent this in reply to\nv2 rather than v1. Oh well.\n"},{"id":"553387","messageId":"xmqqo6dir04i.fsf@gitster.g","threadId":"66355","inReplyTo":"fde7fb7988b695707c6f2776adc18eec7fe4696a.1790425008.git.ben.knoble@gmail.com","subject":"Re: [PATCH v3 5/5] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-27T18:59:41Z","receivedAt":"2026-09-27T18:59:45Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> \t\t oideq(&c_tree, &info->i_tree)) {\n> \t\t\thas_index = 0;\n> \t\t} else {\n> -\t\t\tstruct strbuf out = STRBUF_INIT;\n> +\t\t\tstruct merge_result result = { 0 };\n> \n> -\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n> -\t\t\t\tstrbuf_release(&out);\n> -\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n> -\t\t\t\t\t oid_to_hex(&info->w_commit));\n> -\t\t\t}\n> +\t\t\to.branch1 = \"Current index\";\n> +\t\t\to.branch2 = \"Stashed index changes\";\n> +\t\t\to.ancestor = \"Stash base\";\n> \n> -\t\t\tret = apply_cached(&out);\n> -\t\t\tstrbuf_release(&out);\n> -\t\t\tif (ret)\n> +\t\t\to.verbosity = 0;\n\nWe realize that 'o' is a struct merge_options defined on the stack\nfor this function, initialized with init_ui_merge_options() fairly\nearly on. It would have initialized '.verbosity' to the default\nverbosity, the merge.verbosity configuration variable, or the\nGIT_MERGE_VERBOSITY environment variable.\n\nYou drop the verbosity here, presumably because you want to match\nthe previous implementation 'diff-tree | apply --cached' (which I\nguess was fairly quiet, but I do not use 'stash pop --index'\nmyself).\n\n> +\t\t\thead = lookup_tree(o.repo, &c_tree);\n> +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n> +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> +\n> +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n> +\t\t\t\t\t\t &result);\n> +\n> +\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n> +\t\t\tmerge_finalize(&o, &result);\n\nAnd then the (index) merge is quiet, which is nice.\n\n> +\n> +\t\t\tif (!result.clean)\n> \t\t\t\treturn error(_(\"conflicts in index. \"\n> \t\t\t\t\t \"Try without --index.\"));\n> -\n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> -\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n> -\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n> -\t\t\t\treturn error(_(\"could not save index tree\"));\n> -\n> -\t\t\treset_head();\n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> \t\t}\n> \t}\n\n\nBut the thing is, this is not the end of the function, or the last\ncall to the merge machinery using 'o'. We then use the same 'o' to\ndrive another three-way merge. Yet nobody restores '.verbosity'\nthat was unconditionally turned off above for that second merge.\n\nIt is a bit surprising that the existing test suite did not catch\nthis. Perhaps we do not test --quiet and the merge.verbosity\nconfiguration in combination?\n\nAnyway, I think you'd need something like the following (caveat\nemptor: written against checked out 'seen' while reading the patch,\nand not even compile tested).\n\nThanks.\n\n\n builtin/stash.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git c/builtin/stash.c w/builtin/stash.c\nindex 0f10b9c703..a165419d77 100644\n--- c/builtin/stash.c\n+++ w/builtin/stash.c\n@@ -622,6 +622,9 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \n \tinit_ui_merge_options(&o, the_repository);\n \n+\tif (quiet)\n+\t\to.verbosity = 0;\n+\n \tif (index) {\n \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n \t\t oideq(&c_tree, &info->i_tree)) {\n@@ -633,8 +636,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t\to.branch2 = \"Stashed index changes\";\n \t\t\to.ancestor = \"Stash base\";\n \n-\t\t\to.verbosity = 0;\n-\n \t\t\thead = lookup_tree(o.repo, &c_tree);\n \t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n \t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n@@ -658,9 +659,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \tif (oideq(&info->b_tree, &c_tree))\n \t\to.branch1 = \"Version stash was based on\";\n \n-\tif (quiet)\n-\t\to.verbosity = 0;\n-\n \tif (o.verbosity >= 3)\n \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n \n\n"},{"id":"553388","messageId":"xmqqjyo6qz3z.fsf@gitster.g","threadId":"66355","inReplyTo":"cover.1790425008.git.ben.knoble@gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-27T19:21:36Z","receivedAt":"2026-09-27T19:21:39Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> Hi all,\n>\n> This small patch series fixes a bug reported by Eli Barzilay in the\n> interaction between autostashing, staged index entries, and\n> stash.index=true.\n>\n> The first patch is an incidental cleanup, and the second re-arranges one\n> line to make the change easier. The third and fourth add missing test\n> coverage (which catch breakages from prior incorrect rounds of this\n> series), while the last holds the interesting bits.\n\nI may have reported this on the previous round, too, but 'seen'\nseems to break t5520 when this topic is merged. I'll eject the\ntopic from my tree for now in the meantime.\n\n\n"},{"id":"553427","messageId":"xmqqmrt1pvd1.fsf@gitster.g","threadId":"66355","inReplyTo":"fde7fb7988b695707c6f2776adc18eec7fe4696a.1790425008.git.ben.knoble@gmail.com","subject":"Re: [PATCH v3 5/5] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-28T09:40:10Z","receivedAt":"2026-09-28T09:40:14Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n> +\t\t\t\t\t\t &result);\n> +\n> +\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n\nThis is risky, isn't it?\n\nIf there were catastrophic failure (e.g., missing object that were\ninvolved in the merge), merge_incore_nonrecursive() may stuff -1 to\nresult.clean and return without populating result.tree, and when\nthat happens, result.tree->object.oid would be dereferencing NULL.\n\n"},{"id":"553428","messageId":"346c4209-9600-4302-817f-e8f6b364ce6a@gmail.com","threadId":"66355","inReplyTo":"xmqqjyo6qz3z.fsf@gitster.g","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-28T09:50:13Z","receivedAt":"2026-09-28T09:50:17Z","isPatch":true,"body":"On 27/09/2026 20:21, Junio C Hamano wrote:\n> \"D. Ben Knoble\" writes:\n> \n>> Hi all,\n>>\n>> This small patch series fixes a bug reported by Eli Barzilay in the\n>> interaction between autostashing, staged index entries, and\n>> stash.index=true.\n>>\n>> The first patch is an incidental cleanup, and the second re-arranges one\n>> line to make the change easier. The third and fourth add missing test\n>> coverage (which catch breakages from prior incorrect rounds of this\n>> series), while the last holds the interesting bits.\n> \n> I may have reported this on the previous round, too, but 'seen'\n> seems to break t5520 when this topic is merged. I'll eject the\n> topic from my tree for now in the meantime.\n\nI'm a bit stumped by that as the failing test (5520.69 '--rebase -f with \nrebased upstream') does not stash anything. There seems to be something \nfunny going on with pull's fork-point detection. If I add GIT_TRACE=1 to \n\"git pull --rebase\" then on 'seen' I see\n\ntrace: built-in: git rebase --no-autostash --onto \nae9857430e281d178a3755aecfc5e29c46a02306 \nf29aa667ce68e4d514557081ca7f54b12e108922\n\nbut with this series I see\n\ntrace: built-in: git rebase --no-autostash --onto \nae9857430e281d178a3755aecfc5e29c46a02306 \nae9857430e281d178a3755aecfc5e29c46a02306\n\nso the upstream commit has changed. The previous test also checks the \nfork-point behavior and the failing test just runs \"git reset --hard\" at \nthe start rather than re-creating the reflogs which seems a bit iffy to \nme but I've no idea why this series causes it to fail. I tried a merge \nof 'master' and 'seen' just in case the failure was caused by the base \nI'd used for this series but that passes.\n\nThanks\n\nPhillip\n"},{"id":"553449","messageId":"CALnO6CBTsfMsPrkSMHj6bRMqHc3vEfNEJnh6vz=2+_qCf_26Sg@mail.gmail.com","threadId":"66355","inReplyTo":"xmqqo6dir04i.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T12:02:25Z","receivedAt":"2026-09-28T12:02:37Z","isPatch":true,"body":"On Sun, Sep 27, 2026 at 2:59 PM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n> > oideq(&c_tree, &info->i_tree)) {\n> > has_index = 0;\n> > } else {\n> > - struct strbuf out = STRBUF_INIT;\n> > + struct merge_result result = { 0 };\n> >\n> > - if (diff_tree_binary(&out, &info->w_commit)) {\n> > - strbuf_release(&out);\n> > - return error(_(\"could not generate diff %s^!.\"),\n> > - oid_to_hex(&info->w_commit));\n> > - }\n> > + o.branch1 = \"Current index\";\n> > + o.branch2 = \"Stashed index changes\";\n> > + o.ancestor = \"Stash base\";\n> >\n> > - ret = apply_cached(&out);\n> > - strbuf_release(&out);\n> > - if (ret)\n> > + o.verbosity = 0;\n>\n> We realize that 'o' is a struct merge_options defined on the stack\n> for this function, initialized with init_ui_merge_options() fairly\n> early on. It would have initialized '.verbosity' to the default\n> verbosity, the merge.verbosity configuration variable, or the\n> GIT_MERGE_VERBOSITY environment variable.\n>\n> You drop the verbosity here, presumably because you want to match\n> the previous implementation 'diff-tree | apply --cached' (which I\n> guess was fairly quiet, but I do not use 'stash pop --index'\n> myself).\n\nYes, the original piped \"apply --cached\" output to a strbuf and discarded it.\n\n> > +\n> > + if (!result.clean)\n> > return error(_(\"conflicts in index. \"\n> > \"Try without --index.\"));\n> > -\n> > - discard_index(the_repository->index);\n> > - repo_read_index(the_repository);\n> > - if (write_index_as_tree(&index_tree, the_repository->index,\n> > - repo_get_index_file(the_repository), 0, NULL))\n> > - return error(_(\"could not save index tree\"));\n> > -\n> > - reset_head();\n> > - discard_index(the_repository->index);\n> > - repo_read_index(the_repository);\n> > }\n> > }\n>\n>\n> But the thing is, this is not the end of the function, or the last\n> call to the merge machinery using 'o'. We then use the same 'o' to\n> drive another three-way merge. Yet nobody restores '.verbosity'\n> that was unconditionally turned off above for that second merge.\n\nBut you're right, we should restore the verbosity (which is not what\nthe sketch patch does exactly). Will fix.\n"},{"id":"553450","messageId":"CALnO6CCL6-7Ze0az68NRs2PAr+VJJ=ihU0s+C+DK-bsMB+XGww@mail.gmail.com","threadId":"66355","inReplyTo":"xmqqmrt1pvd1.fsf@gitster.g","subject":"Re: [PATCH v3 5/5] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T12:03:12Z","receivedAt":"2026-09-28T12:03:26Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 5:40 AM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > + merge_incore_nonrecursive(&o, merge_base, head, merge,\n> > + &result);\n> > +\n> > + oidcpy(&index_tree, &result.tree->object.oid);\n>\n> This is risky, isn't it?\n>\n> If there were catastrophic failure (e.g., missing object that were\n> involved in the merge), merge_incore_nonrecursive() may stuff -1 to\n> result.clean and return without populating result.tree, and when\n> that happens, result.tree->object.oid would be dereferencing NULL.\n\nIndeed… unfortunate. Thanks for spotting.\n\n-- \nD. Ben Knoble\n"},{"id":"553451","messageId":"CALnO6CCXT1HHUwL8+eYGVL443nO0eoC7vhpoLvC3RXjp39XQYA@mail.gmail.com","threadId":"66355","inReplyTo":"346c4209-9600-4302-817f-e8f6b364ce6a@gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T12:05:18Z","receivedAt":"2026-09-28T12:05:30Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote:\n>\n> On 27/09/2026 20:21, Junio C Hamano wrote:\n> > \"D. Ben Knoble\" writes:\n> >\n> >> Hi all,\n> >>\n> >> This small patch series fixes a bug reported by Eli Barzilay in the\n> >> interaction between autostashing, staged index entries, and\n> >> stash.index=true.\n> >>\n> >> The first patch is an incidental cleanup, and the second re-arranges one\n> >> line to make the change easier. The third and fourth add missing test\n> >> coverage (which catch breakages from prior incorrect rounds of this\n> >> series), while the last holds the interesting bits.\n> >\n> > I may have reported this on the previous round, too, but 'seen'\n> > seems to break t5520 when this topic is merged. I'll eject the\n> > topic from my tree for now in the meantime.\n\nFirst I'm hearing about it, but I'll try to bisect seen and see what I can find.\n\n> I'm a bit stumped by that as the failing test (5520.69 '--rebase -f with\n> rebased upstream') does not stash anything.\n\nI wonder if a prior test is affected \"silently\" and we only find out by .69?\n\n> There seems to be something\n> funny going on with pull's fork-point detection. If I add GIT_TRACE=1 to\n> \"git pull --rebase\" then on 'seen' I see\n>\n> trace: built-in: git rebase --no-autostash --onto\n> ae9857430e281d178a3755aecfc5e29c46a02306\n> f29aa667ce68e4d514557081ca7f54b12e108922\n>\n> but with this series I see\n>\n> trace: built-in: git rebase --no-autostash --onto\n> ae9857430e281d178a3755aecfc5e29c46a02306\n> ae9857430e281d178a3755aecfc5e29c46a02306\n>\n> so the upstream commit has changed. The previous test also checks the\n> fork-point behavior and the failing test just runs \"git reset --hard\" at\n> the start rather than re-creating the reflogs which seems a bit iffy to\n> me but I've no idea why this series causes it to fail. I tried a merge\n> of 'master' and 'seen' just in case the failure was caused by the base\n> I'd used for this series but that passes.\n\nThanks Phillip!\n\n-- \nD. Ben Knoble\n"},{"id":"553457","messageId":"CALnO6CDOo35HAfqn_h2CUUdux9LeOkjM8OdFLkkS1nVexijUvw@mail.gmail.com","threadId":"66355","inReplyTo":"CALnO6CCXT1HHUwL8+eYGVL443nO0eoC7vhpoLvC3RXjp39XQYA@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T12:33:14Z","receivedAt":"2026-09-28T12:33:26Z","isPatch":true,"body":"Just leaving some breadcrumb notes…\n\nOn Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble wrote:\n>\n> On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote:\n> >\n> > On 27/09/2026 20:21, Junio C Hamano wrote:\n\nFrom my local version of the branch, the following script points at\n4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27):\n\n#! /bin/zsh\nHEAD=$(git rev-parse HEAD) &&\ngit merge --no-edit bk/autostash-index-reset &&\nif ! ninja -C build; then exit 125; fi &&\nmeson test -C build t5520-pull # no chain! need to keep going no matter what\ncode=$? &&\ngit reset --hard $HEAD &&\nexit $code\n\n(using \"git bisect start --first-parent origin/seen @\")\n\n[Cc: Thomas Bachem in case you have any immediate ideas]\n\nI don't think the bisect log will interest anyone, but I've attached it anyway.\n\n-- \nD. Ben Knoble\n"},{"id":"553460","messageId":"CALnO6CAf491aNhqcb7K7YcNTSTNLAESmqeLwzEGk_S=ZsOjG9Q@mail.gmail.com","threadId":"66355","inReplyTo":"CALnO6CDOo35HAfqn_h2CUUdux9LeOkjM8OdFLkkS1nVexijUvw@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T13:00:24Z","receivedAt":"2026-09-28T13:00:37Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 8:33 AM D. Ben Knoble wrote:\n>\n> Just leaving some breadcrumb notes…\n>\n> On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble wrote:\n> >\n> > On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote:\n> > >\n> > > On 27/09/2026 20:21, Junio C Hamano wrote:\n>\n> From my local version of the branch, the following script points at\n> 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27):\n\nAnd within that topic, bisect points to 2d1fa0323f (rebase,\ncherry-pick, revert: run auto maintenance when done, 2026-09-17) in\nt5220.69 as Phillip said.\n\nexpecting success of 5520.69 '--rebase -f with rebased upstream':\ntest_when_finished \"test_might_fail git rebase --abort\" &&\ngit reset --hard to-rebase-orig &&\ngit pull --rebase -f me copy &&\necho \"conflicting modification\" >expect &&\ntest_cmp expect file &&\necho file >expect &&\ntest_cmp expect file2\n\n++ test_when_finished 'test_might_fail git rebase --abort'\n++ test 0 = 0\n++ test_cleanup=$'{ test_might_fail git rebase --abort\\n\\t\\t} || eval_ret=$?; :'\n++ git reset --hard to-rebase-orig\nHEAD is now at cb9bf26 to-rebase\n++ git pull --rebase -f me copy\nFrom .\n * branch copy -> FETCH_HEAD\nRebasing (1/4)\nAuto-merging file\nCONFLICT (content): Merge conflict in file\nerror: could not apply f29aa66... file\nhint: Resolve all conflicts manually, mark them as resolved with\nhint: \"git add/rm \", then run \"git rebase --continue\".\nhint: You can instead skip this commit: run \"git rebase --skip\".\nhint: To abort and get back to the state before \"git rebase\", run \"git\nrebase --abort\".\nhint: Disable this message with \"git config set advice.mergeConflict false\"\nCould not apply f29aa66... # file\nerror: last command exited with $?=1\n\n\n-- \nD. Ben Knoble\n"},{"id":"553477","messageId":"a59c4225-f093-4001-b77a-2083dfecce6e@gmail.com","threadId":"66355","inReplyTo":"CALnO6CAf491aNhqcb7K7YcNTSTNLAESmqeLwzEGk_S=ZsOjG9Q@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-28T13:45:35Z","receivedAt":"2026-09-28T13:45:40Z","isPatch":true,"body":"Hi Ben\n\nOn 28/09/2026 14:00, D. Ben Knoble wrote:\n> On Mon, Sep 28, 2026 at 8:33 AM D. Ben Knoble wrote:\n>>\n>> Just leaving some breadcrumb notes…\n>>\n>> On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble wrote:\n>>>\n>>> On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood wrote:\n>>>>\n>>>> On 27/09/2026 20:21, Junio C Hamano wrote:\n>>\n>> From my local version of the branch, the following script points at\n>> 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27):\n> \n> And within that topic, bisect points to 2d1fa0323f (rebase,\n> cherry-pick, revert: run auto maintenance when done, 2026-09-17)\n\nOh, when I was thinking about this over lunch I did wonder if that might \nbe the culprit. Previously we didn't run \"git maintenance --auto\" after \na rebase with the 'merge' backend but with that topic we do, and because \nwe set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire' \ngets triggered it will expire the reflog entries that 'git pull \n--rebase' relies on. As you suggested in another mail, I assume this \ntopic has changed something in one of the '--autostash' tests that come \nbefore the failing test triggers which the new behavior. What that \nsomething is I'm not sure; off the top of my head I'd expect the number \nof reflog entries in HEAD to be the same but maybe I'm missing \nsomething. Adding\n\n\tgit config maintenance.reflog-expire.auto 0\n\nto the 'setup' test fixes the test failure, but it would be good to try \nand understand why this topic triggers the reflog to be expired in case \nthere is something nasty happening that we've not thought of.\n\nThanks\n\nPhillip\n\n> in\n> t5220.69 as Phillip said.\n> \n> expecting success of 5520.69 '--rebase -f with rebased upstream':\n> test_when_finished \"test_might_fail git rebase --abort\" &&\n> git reset --hard to-rebase-orig &&\n> git pull --rebase -f me copy &&\n> echo \"conflicting modification\" >expect &&\n> test_cmp expect file &&\n> echo file >expect &&\n> test_cmp expect file2\n> \n> ++ test_when_finished 'test_might_fail git rebase --abort'\n> ++ test 0 = 0\n> ++ test_cleanup=$'{ test_might_fail git rebase --abort\\n\\t\\t} || eval_ret=$?; :'\n> ++ git reset --hard to-rebase-orig\n> HEAD is now at cb9bf26 to-rebase\n> ++ git pull --rebase -f me copy\n> From .\n> * branch copy -> FETCH_HEAD\n> Rebasing (1/4)\n> Auto-merging file\n> CONFLICT (content): Merge conflict in file\n> error: could not apply f29aa66... file\n> hint: Resolve all conflicts manually, mark them as resolved with\n> hint: \"git add/rm \", then run \"git rebase --continue\".\n> hint: You can instead skip this commit: run \"git rebase --skip\".\n> hint: To abort and get back to the state before \"git rebase\", run \"git\n> rebase --abort\".\n> hint: Disable this message with \"git config set advice.mergeConflict false\"\n> Could not apply f29aa66... # file\n> error: last command exited with $?=1\n> \n> \n\n"},{"id":"553483","messageId":"CAA0xjtpzaWH10pHOQ5j-5Hp1yHEKTDFbsicG6E4w=5nxb_irWw@mail.gmail.com","threadId":"66355","inReplyTo":"a59c4225-f093-4001-b77a-2083dfecce6e@gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"Thomas Bachem","fromEmail":"mail@thomasbachem.com","sentAt":"2026-09-28T14:50:17Z","receivedAt":"2026-09-28T14:50:32Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote:\n> Oh, when I was thinking about this over lunch I did wonder if that might\n> be the culprit. Previously we didn't run \"git maintenance --auto\" after\n> a rebase with the 'merge' backend but with that topic we do, and because\n> we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'\n> gets triggered it will expire the reflog entries that 'git pull\n> --rebase' relies on. As you suggested in another mail, I assume this\n\nThat is it. I ran t5520 on 'seen' with and without Ben's series under\nGIT_TRACE2_EVENT, and the two runs differ in one place: which command's\nauto maintenance runs \"git reflog expire --all\".\n\n\"git pull --rebase\" computes the fork point before it fetches, from\nthe reflog of refs/remotes/me/copy, and test 69 needs the entry that\ntest 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the\nreflog empty, \"merge-base --fork-point\" falls back to the ref itself,\nae98574 is no ancestor of to-rebase, and pull hands the merge head to\nrebase as the upstream. That is your \"--onto ae98... ae98...\", and the\nfour commits from copy-orig up come back, the first of them\nconflicting with \"conflict\".\n\n> topic has changed something in one of the '--autostash' tests that come\n> before the failing test triggers which the new behavior. What that\n> something is I'm not sure; off the top of my head I'd expect the number\n> of reflog entries in HEAD to be the same but maybe I'm missing\n> something. Adding\n\nIt is eight entries fewer, and they come from the failed merges, not\nfrom the autostash tests. \"git merge\" restores a dirty tree with\n\"stash apply --index --quiet\", and until Ben's series that spawned\n\"git reset --quiet --refresh\", which writes \"reset: moving to HEAD\"\nto the reflog. That happens eight times in t5520 before test 68.\n\nAuto maintenance expires reflogs once HEAD's reflog holds a hundred\nentries that the policy would remove, the default of\nmaintenance.reflog-expire.auto, and after the first test_tick that is\nevery entry. Which run crosses the hundred depends on how many entries\nand maintenance runs came before it. On 'seen' the expiry lands on\n\"git commit -m conflict\" in test 68, before the fetch writes the entry.\nEight entries fewer move the crossing past that commit, and the\nmaintenance run my topic adds at the end of the rebase in test 68 is\nthe next one: after the fetch, before test 69 reads the reflog. Either\nchange alone leaves it somewhere harmless, and nothing else is going\non. The expiry is the usual 90 days applied to entries dated 2005, and\nthe only new thing is one more maintenance run per rebase, the same\none \"git commit\" and \"git fetch\" run.\n\n> git config maintenance.reflog-expire.auto 0\n>\n> to the 'setup' test fixes the test failure, but it would be good to try\n> and understand why this topic triggers the reflog to be expired in case\n> there is something nasty happening that we've not thought of.\n\nI'd pin the expiry itself instead, as ea7d894f44 (t34xx: don't expire\nreflogs where it matters, 2026-02-24) did for the rebase tests:\n\ngit config set gc.reflogExpire never &&\ngit config set gc.reflogExpireUnreachable never &&\n\nThat covers a \"git gc\" as well, which expires reflogs on its own. With\nit, 'seen' plus Ben's series passes t5520 here and no expiry runs\nduring the script at all. I sent it as a patch on master:\n\n\nFWIW, any script that reads a reflog after a hundred HEAD updates can\nfall into the same hole. I have not looked further than t5520.\n\nThomas\n"},{"id":"553489","messageId":"xmqq7bk5o0hs.fsf@gitster.g","threadId":"66355","inReplyTo":"CALnO6CCL6-7Ze0az68NRs2PAr+VJJ=ihU0s+C+DK-bsMB+XGww@mail.gmail.com","subject":"Re: [PATCH v3 5/5] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-28T15:32:15Z","receivedAt":"2026-09-28T15:32:18Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> On Mon, Sep 28, 2026 at 5:40 AM Junio C Hamano wrote:\n>>\n>> \"D. Ben Knoble\" writes:\n>>\n>> > + merge_incore_nonrecursive(&o, merge_base, head, merge,\n>> > + &result);\n>> > +\n>> > + oidcpy(&index_tree, &result.tree->object.oid);\n>>\n>> This is risky, isn't it?\n>>\n>> If there were catastrophic failure (e.g., missing object that were\n>> involved in the merge), merge_incore_nonrecursive() may stuff -1 to\n>> result.clean and return without populating result.tree, and when\n>> that happens, result.tree->object.oid would be dereferencing NULL.\n>\n> Indeed… unfortunate. Thanks for spotting.\n\nI did\n\n\t$ git grep merge_incore_nonrecursive \\*.c\n\nand read all the current callers.\n\nThey all have code to specifically check for the (result.clean < 0)\ncondition and error out before touching any of the other members of\nthe result structure, so they seem to be safe.\n\n"},{"id":"553491","messageId":"CALnO6CC-eop86W3VREwGz0seG1pmtd0qS968TyP=mo_G+ZMrSA@mail.gmail.com","threadId":"66355","inReplyTo":"CAA0xjtpzaWH10pHOQ5j-5Hp1yHEKTDFbsicG6E4w=5nxb_irWw@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T15:36:18Z","receivedAt":"2026-09-28T15:36:31Z","isPatch":true,"body":"Let me see if I understand correctly…\n\nOn Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem wrote:\n>\n> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote:\n> > Oh, when I was thinking about this over lunch I did wonder if that might\n> > be the culprit. Previously we didn't run \"git maintenance --auto\" after\n> > a rebase with the 'merge' backend but with that topic we do, and because\n> > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'\n> > gets triggered it will expire the reflog entries that 'git pull\n> > --rebase' relies on. As you suggested in another mail, I assume this\n\n> \"git pull --rebase\" computes the fork point before it fetches, from\n> the reflog of refs/remotes/me/copy,\n\nThis is described by the manual for git-rebase under --fork-point,\nwhich is on unless we have an or --keep-base (modulo\nconfig). Put a pin in this.\n\n> and test 69 needs the entry that\n> test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the\n> reflog empty, \"merge-base --fork-point\" falls back to the ref itself,\n> ae98574 is no ancestor of to-rebase, and pull hands the merge head to\n> rebase as the upstream. That is your \"--onto ae98... ae98...\", and the\n> four commits from copy-orig up come back, the first of them\n> conflicting with \"conflict\".\n>\n> > topic has changed something in one of the '--autostash' tests that come\n> > before the failing test triggers which the new behavior. What that\n> > something is I'm not sure; off the top of my head I'd expect the number\n> > of reflog entries in HEAD to be the same but maybe I'm missing\n> > something. Adding\n>\n> It is eight entries fewer, and they come from the failed merges, not\n> from the autostash tests. \"git merge\" restores a dirty tree with\n> \"stash apply --index --quiet\", and until Ben's series that spawned\n> \"git reset --quiet --refresh\", which writes \"reset: moving to HEAD\"\n> to the reflog. That happens eight times in t5520 before test 68.\n>\n> Auto maintenance expires reflogs once HEAD's reflog holds a hundred\n> entries that the policy would remove, the default of\n> maintenance.reflog-expire.auto, and after the first test_tick that is\n> every entry. Which run crosses the hundred depends on how many entries\n> and maintenance runs came before it. On 'seen' the expiry lands on\n> \"git commit -m conflict\" in test 68, before the fetch writes the entry.\n> Eight entries fewer move the crossing past that commit, and the\n> maintenance run my topic adds at the end of the rebase in test 68 is\n> the next one: after the fetch, before test 69 reads the reflog. Either\n> change alone leaves it somewhere harmless, and nothing else is going\n> on. The expiry is the usual 90 days applied to entries dated 2005, and\n> the only new thing is one more maintenance run per rebase, the same\n> one \"git commit\" and \"git fetch\" run.\n\nIn short, expiry used to happen prior to .68, so the reflog entry\ncreated in that test which is used by \"pull --rebase\" in .69 is picked\nup. With fewer reflog entries, expiry happens later, and it just so\nhappens to drop the important entry. Darn!\n\nBut here's what I can't figure out, returning to that pin from\nearlier: I was a bit surprised to see mention of rebase reading\nreflogs! When I remembered --fork-point, I was even more curious (but\nat least it's obvious that rebase will read the reflogs in some\nscenarios).\n\nWhat confuses me is that builtin/pull.c:run_rebase() sure looks like\nit provides an to the command invocation, so shouldn't\n--fork-point and reflog use be disabled????\n\nI'll try tracing that test myself later, I suppose. It's nice to know\nwe have a fix available (thanks for the patch), but it sure feels like\na hack :) oh well?\n\n-- \nD. Ben Knoble\n"},{"id":"553493","messageId":"ef5e507f-9e26-4e7e-887a-403cf7f282a7@gmail.com","threadId":"66355","inReplyTo":"CAA0xjtpzaWH10pHOQ5j-5Hp1yHEKTDFbsicG6E4w=5nxb_irWw@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-28T15:40:02Z","receivedAt":"2026-09-28T15:40:06Z","isPatch":true,"body":"Hi Thomas\n\nOn 28/09/2026 15:50, Thomas Bachem wrote:\n> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote:\n>>\n>> topic has changed something in one of the '--autostash' tests that come\n>> before the failing test triggers which the new behavior. What that\n>> something is I'm not sure; off the top of my head I'd expect the number\n>> of reflog entries in HEAD to be the same but maybe I'm missing\n>> something. Adding\n> \n> It is eight entries fewer, and they come from the failed merges, not\n> from the autostash tests. \"git merge\" restores a dirty tree with\n> \"stash apply --index --quiet\",\n\nThanks for tracking that down, I couldn't see where we'd be calling \"git \nstash apply\" with \"--index\" but builtin/merge.c:restore_state() calls \n\"git stash apply --index --quiet\" rather than calling one of the \nautostash helper functions which do not use \"--index\".\n\n> and until Ben's series that spawned\n> \"git reset --quiet --refresh\", which writes \"reset: moving to HEAD\"\n> to the reflog. That happens eight times in t5520 before test 68.\n\nThat accounts for the difference in the number of reflog entries. It's \ngood to have an explanation for why we're expiring the reflog entries at \na slightly different time.\n\nThanks\n\nPhillip\n> Auto maintenance expires reflogs once HEAD's reflog holds a hundred\n> entries that the policy would remove, the default of\n> maintenance.reflog-expire.auto, and after the first test_tick that is\n> every entry. Which run crosses the hundred depends on how many entries\n> and maintenance runs came before it. On 'seen' the expiry lands on\n> \"git commit -m conflict\" in test 68, before the fetch writes the entry.\n> Eight entries fewer move the crossing past that commit, and the\n> maintenance run my topic adds at the end of the rebase in test 68 is\n> the next one: after the fetch, before test 69 reads the reflog. Either\n> change alone leaves it somewhere harmless, and nothing else is going\n> on. The expiry is the usual 90 days applied to entries dated 2005, and\n> the only new thing is one more maintenance run per rebase, the same\n> one \"git commit\" and \"git fetch\" run.\n> \n>> git config maintenance.reflog-expire.auto 0\n>>\n>> to the 'setup' test fixes the test failure, but it would be good to try\n>> and understand why this topic triggers the reflog to be expired in case\n>> there is something nasty happening that we've not thought of.\n> \n> I'd pin the expiry itself instead, as ea7d894f44 (t34xx: don't expire\n> reflogs where it matters, 2026-02-24) did for the rebase tests:\n> \n> git config set gc.reflogExpire never &&\n> git config set gc.reflogExpireUnreachable never &&\n> \n> That covers a \"git gc\" as well, which expires reflogs on its own. With\n> it, 'seen' plus Ben's series passes t5520 here and no expiry runs\n> during the script at all. I sent it as a patch on master:\n> \n> \n> FWIW, any script that reads a reflog after a hundred HEAD updates can\n> fall into the same hole. I have not looked further than t5520.\n> \n> Thomas\n\n"},{"id":"553494","messageId":"97f86d82-b5ec-44df-9ccf-8e6cd93e45f4@gmail.com","threadId":"66355","inReplyTo":"8b5ea5e6f47ee9a57df3a4d97a457d024b3dec00.1790425008.git.ben.knoble@gmail.com","subject":"Re: [PATCH v3 3/5] t3903: test stash --index merges","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-28T15:44:43Z","receivedAt":"2026-09-28T15:44:46Z","isPatch":true,"body":"Hi Ben\n\nOn 26/09/2026 13:16, D. Ben Knoble wrote:\n> A future commit will refactor index handling for applied stashes, and we\n> need to take care to get the order of trees right when merging. Add a\n> test that covers this case.\n\nThe test looks good, but without the changes in patch 5 it fails and so \nadding it here breaks running \"git bisect\" on this series. I'd squash \nthis into the final patch and I think we can probably replace an \nexisting \"stash apply --index\" tests that are not so strict with this \none, rather than adding a new test.\n\nThanks\n\nPhillip\n\n> Suggested-by: Phillip Wood \n> Signed-off-by: D. Ben Knoble \n> ---\n> t/t3903-stash.sh | 21 +++++++++++++++++++++\n> 1 file changed, 21 insertions(+)\n> \n> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\n> index 721158606f..9bc99fa252 100755\n> --- a/t/t3903-stash.sh\n> +++ b/t/t3903-stash.sh\n> @@ -374,6 +374,27 @@ setup_stash() {\n> \ttest_cmp expect actual\n> '\n> \n> +# the later \"stash -k\" test is not expecting us to muck with file so much, so\n> +# reset when finished\n> +test_expect_success 'stash apply --index merges the correct trees' '\n> +\thead=$(git rev-parse HEAD) &&\n> +\ttest_when_finished \"git reset --hard $head\" &&\n> +\ttest_write_lines A B C >file &&\n> +\tgit commit -m setup file &&\n> +\ttest_write_lines A B staged >file &&\n> +\tgit add file &&\n> +\ttest_write_lines A B unstaged >file &&\n> +\tgit stash &&\n> +\ttest_write_lines committed B C >file &&\n> +\tgit commit -m to-be-merged file &&\n> +\tgit stash pop --index &&\n> +\tgit show :file >actual &&\n> +\ttest_write_lines committed B staged >expect &&\n> +\ttest_cmp expect actual &&\n> +\ttest_write_lines committed B unstaged >expect &&\n> +\ttest_cmp expect file\n> +'\n> +\n> test_expect_success 'stash -k' '\n> \techo bar3 >file &&\n> \techo bar4 >file2 &&\n\n"},{"id":"553501","messageId":"CALnO6CCX+CvMZcOiyaFB0_nhe0wSv2-E2hx-iTbN4OvSVvNDRw@mail.gmail.com","threadId":"66355","inReplyTo":"97f86d82-b5ec-44df-9ccf-8e6cd93e45f4@gmail.com","subject":"Re: [PATCH v3 3/5] t3903: test stash --index merges","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-28T15:55:01Z","receivedAt":"2026-09-28T15:55:13Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 11:44 AM Phillip Wood wrote:\n>\n> Hi Ben\n>\n> On 26/09/2026 13:16, D. Ben Knoble wrote:\n> > A future commit will refactor index handling for applied stashes, and we\n> > need to take care to get the order of trees right when merging. Add a\n> > test that covers this case.\n>\n> The test looks good, but without the changes in patch 5 it fails and so\n> adding it here breaks running \"git bisect\" on this series. I'd squash\n> this into the final patch\n\nInteresting. I thought I checked that the test passed sans patch 5,\nbut I'll double check. I can't think of a reason it wouldn't offhand,\nbut my thoughts on patch 5's changes have become a bit scattered.\n\n> and I think we can probably replace an\n> existing \"stash apply --index\" tests that are not so strict with this\n> one, rather than adding a new test.\n\nThat's probably a good idea, thanks.\n"},{"id":"553569","messageId":"21a5c1fc-b268-493c-bd61-fa0afdf98bee@gmail.com","threadId":"66355","inReplyTo":"CALnO6CCX+CvMZcOiyaFB0_nhe0wSv2-E2hx-iTbN4OvSVvNDRw@mail.gmail.com","subject":"Re: [PATCH v3 3/5] t3903: test stash --index merges","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-29T09:41:11Z","receivedAt":"2026-09-29T09:41:17Z","isPatch":true,"body":"Hi Ben\n\nOn 28/09/2026 16:55, D. Ben Knoble wrote:\n> On Mon, Sep 28, 2026 at 11:44 AM Phillip Wood wrote:\n>>\n>> The test looks good, but without the changes in patch 5 it fails and so\n>> adding it here breaks running \"git bisect\" on this series. I'd squash\n>> this into the final patch\n> \n> Interesting. I thought I checked that the test passed sans patch 5,\n> but I'll double check. I can't think of a reason it wouldn't offhand,\n> but my thoughts on patch 5's changes have become a bit scattered.\n\nIt fails because it tries to apply a patch that looks like\n\n@@ -1,3 +1,3 @@\n A\n B\n-C\n+staged\n\nto a file that looks like\n\ncommitted\nB\nC\n\nand so the first context line does not match. Because the changes do not \noverlap the merge machinery is perfectly happy. As an aside when we \nclear the worktree changes from \"git stash push -p\" generate the patch \nwith \"-U1\" to try and avoid problems like this.\n\nThanks\n\nPhillip\n\n> \n>> and I think we can probably replace an\n>> existing \"stash apply --index\" tests that are not so strict with this\n>> one, rather than adding a new test.\n> \n> That's probably a good idea, thanks.\n\n"},{"id":"553590","messageId":"CALnO6CDnYmmVfcTrkuQ=hTUDKBAAspYrSxmwM+yVUSnJinN_Xw@mail.gmail.com","threadId":"66355","inReplyTo":"CALnO6CC-eop86W3VREwGz0seG1pmtd0qS968TyP=mo_G+ZMrSA@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T11:38:40Z","receivedAt":"2026-09-29T11:38:54Z","isPatch":true,"body":"On Mon, Sep 28, 2026 at 11:36 AM D. Ben Knoble wrote:\n>\n> Let me see if I understand correctly…\n>\n> On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem wrote:\n> >\n> > On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote:\n> > > Oh, when I was thinking about this over lunch I did wonder if that might\n> > > be the culprit. Previously we didn't run \"git maintenance --auto\" after\n> > > a rebase with the 'merge' backend but with that topic we do, and because\n> > > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'\n> > > gets triggered it will expire the reflog entries that 'git pull\n> > > --rebase' relies on. As you suggested in another mail, I assume this\n>\n> > \"git pull --rebase\" computes the fork point before it fetches, from\n> > the reflog of refs/remotes/me/copy,\n>\n> This is described by the manual for git-rebase under --fork-point,\n> which is on unless we have an or --keep-base (modulo\n> config). Put a pin in this.\n\n\n> But here's what I can't figure out, returning to that pin from\n> earlier: I was a bit surprised to see mention of rebase reading\n> reflogs! When I remembered --fork-point, I was even more curious (but\n> at least it's obvious that rebase will read the reflogs in some\n> scenarios).\n>\n> What confuses me is that builtin/pull.c:run_rebase() sure looks like\n> it provides an to the command invocation, so shouldn't\n> --fork-point and reflog use be disabled????\n\nIndeed, from GIT_TRACE2 output I can see we do run\n\n git rebase --no-autostash --onto ae98… f29a…\n\nbut well before that we run\n\n git merge-base --fork-point refs/remotes/me/copy to-rebase\n\nwhich is then presumably fed down to the rebase. Interesting.\n\n-- \nD. Ben Knoble\n"},{"id":"553594","messageId":"cover.1790684309.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1789853192.git.ben.knoble@gmail.com","subject":"[PATCH v4 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T12:18:26Z","receivedAt":"2026-09-29T12:20:40Z","isPatch":true,"body":"Hi all,\n\nThis small patch series fixes a bug reported by Eli Barzilay in the\ninteraction between autostashing, staged index entries, and\nstash.index=true.\n\nThe first patch is an incidental cleanup, and the second re-arranges one\nline to make the change easier. The third adds missing test coverage\n(which catch breakages from prior incorrect rounds of this series). The\nfourth fixes a test interaction with another in-flight topic. The last\nholds the interesting bits.\n\nChanges in v4:\n• Drop merge verbosity changes altogether. I was going to\n save-and-restore, but when looking at the index-merge test case (more\n below) closer, I noticed that \"git apply --cached\" reports conflicts\n on stderr. That is, \"git stash apply --index\" would report conflicts,\n and silencing the merge takes that away. So instead let's leave the\n configured verbosity alone.\n• Only copy resulting index merge tree OID when successful\n• Fix interaction with t5520 (new patch 4/5)\n• Squash test from 3/5 into 5/5, since it requires actually merging\n trees. I've elected to keep it a separate test for now (contrary to\n Phillip's suggestion) since it's written and working. Adapting\n existing tests requires quite a bit more digging into implicit context\n assumptions ;)\n\nChanges in v3:\n\n• Change conflict label for current index\n• Fix memory leak of merge_result\n• Fix order of trees to make the correct merge (cherry-pick)\n • New test (3/5) to validate this\n• Fix test in 4/5 to assert more details of expected state\n\nChanges in v2:\n\n• Do give branch labels for the incore merge, although they are never\n seen (and clarify commit message as a result, also keeping the\n merge-ort asserts). Phillip was right: without those, we do segfault\n on conflicts.\n• Use the ui merge options to keep the same diff algorithm.\n• Use merge_finalize instead of clear_merge_options, and reuse the\n options between merge calls if they are already initialized.\n• Add a new 2/4 to simplify merge options initialization.\n• Add a new 3/4 with a test case for conflicted index merges.\n\nv1: \nv2: \nv3: \n\n[1/5] builtin/stash: remove unused header\n[2/5] stash: prepare merge options earlier\n[3/5] t3903: test failed \"stash apply --index\"\n[4/5] t5520: don't expire reflogs where it matters\n[5/5] builtin/stash: merge index in-core\n\n builtin/stash.c | 91 ++++++++++++------------------------------------\n t/t3903-stash.sh | 42 ++++++++++++++++++++++\n t/t5520-pull.sh | 6 ++++\n t/t7600-merge.sh | 9 +++++\n 4 files changed, 79 insertions(+), 69 deletions(-)\n\nDiff-intervalle contre v3 :\n1: 6a165c4df4 = 1: 6a165c4df4 builtin/stash: remove unused header\n2: d9a9e18f3a ! 2: 35b64ae321 stash: prepare merge options earlier\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n \t\treturn error(_(\"cannot apply a stash in the middle of a merge\"));\n \n +\tinit_ui_merge_options(&o, the_repository);\n ++\n ++\tif (quiet)\n ++\t\to.verbosity = 0;\n +\n \tif (index) {\n \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n \to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n \to.branch2 = label_theirs ? label_theirs : \"Stashed changes\";\n \to.ancestor = label_base ? label_base : \"Stash base\";\n +@@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefix,\n + \tif (oideq(&info->b_tree, &c_tree))\n + \t\to.branch1 = \"Version stash was based on\";\n + \n +-\tif (quiet)\n +-\t\to.verbosity = 0;\n +-\n + \tif (o.verbosity >= 3)\n + \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n + \n4: d39e16905d ! 3: 7b0b317ce0 t3903: test failed \"stash apply --index\"\n @@ Commit message\n \n ## t/t3903-stash.sh ##\n @@ t/t3903-stash.sh: setup_stash() {\n - \ttest_cmp expect file\n + \ttest_cmp expect actual\n '\n \n +test_expect_success 'stash apply --index leaves everything untouched on failure' '\n3: 8b5ea5e6f4 ! 4: 2ac371d2dc t3903: test stash --index merges\n @@\n ## Metadata ##\n -Author: D. Ben Knoble \n +Author: Thomas Bachem \n \n ## Commit message ##\n - t3903: test stash --index merges\n + t5520: don't expire reflogs where it matters\n \n - A future commit will refactor index handling for applied stashes, and we\n - need to take care to get the order of trees right when merging. Add a\n - test that covers this case.\n + The \"--rebase -f with rebased upstream\" test computes its fork point\n + from the reflog of refs/remotes/me/copy, and the entry it needs is\n + the one that the fetch of the test before it wrote. Like every reflog\n + entry the suite writes after test_tick, it is dated 2005, so the\n + first \"git reflog expire --all\" after that fetch removes it. Pull\n + then finds no fork point and rebases onto the merge head with the\n + merge head as the upstream, and the rewound commits come back as a\n + conflict.\n \n - Suggested-by: Phillip Wood \n + Since 452b12c2e0 (builtin/maintenance: use \"geometric\" strategy by\n + default, 2026-02-24) auto maintenance runs that expiry once the reflog\n + of HEAD holds a hundred entries it would remove, the default of\n + maintenance.reflog-expire.auto. Which run crosses the threshold\n + depends on the entries and maintenance runs before it, so the script\n + passed by chance: a stash topic that no longer runs \"git reset\" from\n + \"stash apply --index\" and a rebase topic that runs auto maintenance\n + at the end of \"git rebase\" together move the expiry between the two\n + tests.\n \n - ## t/t3903-stash.sh ##\n -@@ t/t3903-stash.sh: setup_stash() {\n - \ttest_cmp expect actual\n - '\n + Pin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it\n + matters, 2026-02-24) did for the rebase tests. That covers a \"git gc\"\n + as well, which expires reflogs on its own, where turning off the auto\n + trigger of the reflog-expire task alone would not.\n +\n + Reported-by: Junio C Hamano \n + Helped-by: D. Ben Knoble \n + Helped-by: Phillip Wood \n + Assisted-by: Claude Fable 5.1\n + Signed-off-by: Thomas Bachem \n +\n + ## t/t5520-pull.sh ##\n +@@ t/t5520-pull.sh: test_pull_autostash_fail () {\n + }\n \n -+# the later \"stash -k\" test is not expecting us to muck with file so much, so\n -+# reset when finished\n -+test_expect_success 'stash apply --index merges the correct trees' '\n -+\thead=$(git rev-parse HEAD) &&\n -+\ttest_when_finished \"git reset --hard $head\" &&\n -+\ttest_write_lines A B C >file &&\n -+\tgit commit -m setup file &&\n -+\ttest_write_lines A B staged >file &&\n -+\tgit add file &&\n -+\ttest_write_lines A B unstaged >file &&\n -+\tgit stash &&\n -+\ttest_write_lines committed B C >file &&\n -+\tgit commit -m to-be-merged file &&\n -+\tgit stash pop --index &&\n -+\tgit show :file >actual &&\n -+\ttest_write_lines committed B staged >expect &&\n -+\ttest_cmp expect actual &&\n -+\ttest_write_lines committed B unstaged >expect &&\n -+\ttest_cmp expect file\n -+'\n + test_expect_success setup '\n ++\t# Commit dates are hardcoded to 2005, and the reflog entries will have\n ++\t# a matching timestamp. Maintenance may thus immediately expire\n ++\t# reflogs if it was running.\n ++\tgit config set gc.reflogExpire never &&\n ++\tgit config set gc.reflogExpireUnreachable never &&\n +\n - test_expect_success 'stash -k' '\n - \techo bar3 >file &&\n - \techo bar4 >file2 &&\n + \techo file >file &&\n + \tgit add file &&\n + \tgit commit -a -m original\n5: fde7fb7988 ! 5: e21b832a6e builtin/stash: merge index in-core\n @@ Commit message\n we don't see the usual branch and ancestor labels, but the merge\n subroutines insist on their presence, so use something simple.\n \n + We need to take care to get the order of trees right when merging. Add a\n + test that covers this case.\n +\n We *could* swap just the git-reset(1) subprocess with our internal\n reset_tree() and refresh_index(), which would fix the bug. We'd much\n prefer to clean up these vestiges of the shell-based git-stash, though.\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n -\t\t\tret = apply_cached(&out);\n -\t\t\tstrbuf_release(&out);\n -\t\t\tif (ret)\n -+\t\t\to.verbosity = 0;\n -+\n +\t\t\thead = lookup_tree(o.repo, &c_tree);\n +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n +\t\t\t\t\t\t &result);\n +\n -+\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n -+\t\t\tmerge_finalize(&o, &result);\n -+\n -+\t\t\tif (!result.clean)\n ++\t\t\tif (!result.clean) {\n ++\t\t\t\tmerge_finalize(&o, &result);\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n -\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n -\t\t\treset_head();\n -\t\t\tdiscard_index(the_repository->index);\n -\t\t\trepo_read_index(the_repository);\n ++\t\t\t} else {\n ++\t\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n ++\t\t\t\tmerge_finalize(&o, &result);\n ++\t\t\t}\n \t\t}\n \t}\n \n \n + ## t/t3903-stash.sh ##\n +@@ t/t3903-stash.sh: setup_stash() {\n + \ttest_cmp expect-index actual-index\n + '\n + \n ++# the later \"stash -k\" test is not expecting us to muck with file so much, so\n ++# reset when finished\n ++test_expect_success 'stash apply --index merges the correct trees' '\n ++\thead=$(git rev-parse HEAD) &&\n ++\ttest_when_finished \"git reset --hard $head\" &&\n ++\ttest_write_lines A B C >file &&\n ++\tgit commit -m setup file &&\n ++\ttest_write_lines A B staged >file &&\n ++\tgit add file &&\n ++\ttest_write_lines A B unstaged >file &&\n ++\tgit stash &&\n ++\ttest_write_lines committed B C >file &&\n ++\tgit commit -m to-be-merged file &&\n ++\tgit stash pop --index &&\n ++\tgit show :file >actual &&\n ++\ttest_write_lines committed B staged >expect &&\n ++\ttest_cmp expect actual &&\n ++\ttest_write_lines committed B unstaged >expect &&\n ++\ttest_cmp expect file\n ++'\n ++\n + test_expect_success 'stash -k' '\n + \techo bar3 >file &&\n + \techo bar4 >file2 &&\n +\n ## t/t7600-merge.sh ##\n @@ t/t7600-merge.sh: verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n\nbase-commit: d38352cd43ab9745686d697872408bc3249a153f\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553595","messageId":"6a165c4df456b6bd5e5ab46664b023a45e670926.1790684309.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790684309.git.ben.knoble@gmail.com","subject":"[PATCH v4 1/5] builtin/stash: remove unused header","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T12:18:27Z","receivedAt":"2026-09-29T12:20:41Z","isPatch":true,"body":"Clang complains that oid-array.h is unused. Certainly none of the\noid_array* functions, types, etc., are used, and the\ntransitively-included hash.h declarations are used but covered by a\npre-existing direct #include of hash.h.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7a9843413b..dfea2d2c4c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -31,7 +31,6 @@\n #include \"reflog.h\"\n #include \"reflog-walk.h\"\n #include \"add-interactive.h\"\n-#include \"oid-array.h\"\n #include \"commit.h\"\n \n #define INCLUDE_ALL_FILES 2\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553596","messageId":"35b64ae3217629498ea19c1285edfeb32c5cb53d.1790684309.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790684309.git.ben.knoble@gmail.com","subject":"[PATCH v4 2/5] stash: prepare merge options earlier","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T12:18:28Z","receivedAt":"2026-09-29T12:20:43Z","isPatch":true,"body":"In a future commit, we will reuse these options for the index merge of\n\"apply --index\", not just for the worktree.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex dfea2d2c4c..d2b736d4e6 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -664,6 +664,11 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n \t\treturn error(_(\"cannot apply a stash in the middle of a merge\"));\n \n+\tinit_ui_merge_options(&o, the_repository);\n+\n+\tif (quiet)\n+\t\to.verbosity = 0;\n+\n \tif (index) {\n \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n \t\t oideq(&c_tree, &info->i_tree)) {\n@@ -695,8 +700,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t}\n \t}\n \n-\tinit_ui_merge_options(&o, the_repository);\n-\n \to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n \to.branch2 = label_theirs ? label_theirs : \"Stashed changes\";\n \to.ancestor = label_base ? label_base : \"Stash base\";\n@@ -704,9 +707,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \tif (oideq(&info->b_tree, &c_tree))\n \t\to.branch1 = \"Version stash was based on\";\n \n-\tif (quiet)\n-\t\to.verbosity = 0;\n-\n \tif (o.verbosity >= 3)\n \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n \n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553597","messageId":"7b0b317ce061d672ef143b1628a2d0097a878c87.1790684309.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790684309.git.ben.knoble@gmail.com","subject":"[PATCH v4 3/5] t3903: test failed \"stash apply --index\"","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T12:18:29Z","receivedAt":"2026-09-29T12:20:45Z","isPatch":true,"body":"The next commit will refactor index handling for applied stashes, so\nlet's make sure we cover conflicted index merging, too.\n\nHelped-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n t/t3903-stash.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 721158606f..70af58e161 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -374,6 +374,27 @@ setup_stash() {\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stash apply --index leaves everything untouched on failure' '\n+\tgit reset --hard &&\n+\techo test >other-file &&\n+\tgit add other-file &&\n+\tgit stash &&\n+\techo unrelated >file &&\n+\techo unrelated >another-file &&\n+\tgit add another-file &&\n+\techo conflict >other-file &&\n+\tgit add other-file &&\n+\tgit diff-files -p >expect &&\n+\tgit diff-index --cached HEAD >expect-index &&\n+\n+\ttest_must_fail git stash apply --index 2>err &&\n+\ttest_grep \"conflicts in index. Try without --index\" err &&\n+\tgit diff-files -p >actual &&\n+\ttest_cmp expect actual &&\n+\tgit diff-index --cached HEAD >actual-index &&\n+\ttest_cmp expect-index actual-index\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553598","messageId":"2ac371d2dc1425cc47bf369e88b321d3c0c8c605.1790684309.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790684309.git.ben.knoble@gmail.com","subject":"[PATCH v4 4/5] t5520: don't expire reflogs where it matters","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T12:18:30Z","receivedAt":"2026-09-29T12:20:48Z","isPatch":true,"body":"From: Thomas Bachem \n\nThe \"--rebase -f with rebased upstream\" test computes its fork point\nfrom the reflog of refs/remotes/me/copy, and the entry it needs is\nthe one that the fetch of the test before it wrote. Like every reflog\nentry the suite writes after test_tick, it is dated 2005, so the\nfirst \"git reflog expire --all\" after that fetch removes it. Pull\nthen finds no fork point and rebases onto the merge head with the\nmerge head as the upstream, and the rewound commits come back as a\nconflict.\n\nSince 452b12c2e0 (builtin/maintenance: use \"geometric\" strategy by\ndefault, 2026-02-24) auto maintenance runs that expiry once the reflog\nof HEAD holds a hundred entries it would remove, the default of\nmaintenance.reflog-expire.auto. Which run crosses the threshold\ndepends on the entries and maintenance runs before it, so the script\npassed by chance: a stash topic that no longer runs \"git reset\" from\n\"stash apply --index\" and a rebase topic that runs auto maintenance\nat the end of \"git rebase\" together move the expiry between the two\ntests.\n\nPin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it\nmatters, 2026-02-24) did for the rebase tests. That covers a \"git gc\"\nas well, which expires reflogs on its own, where turning off the auto\ntrigger of the reflog-expire task alone would not.\n\nReported-by: Junio C Hamano \nHelped-by: D. Ben Knoble \nHelped-by: Phillip Wood \nAssisted-by: Claude Fable 5.1\nSigned-off-by: Thomas Bachem \nSigned-off-by: D. Ben Knoble \n---\n t/t5520-pull.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\nindex 27f38ab3c8..bc818605a5 100755\n--- a/t/t5520-pull.sh\n+++ b/t/t5520-pull.sh\n@@ -35,6 +35,12 @@ test_pull_autostash_fail () {\n }\n \n test_expect_success setup '\n+\t# Commit dates are hardcoded to 2005, and the reflog entries will have\n+\t# a matching timestamp. Maintenance may thus immediately expire\n+\t# reflogs if it was running.\n+\tgit config set gc.reflogExpire never &&\n+\tgit config set gc.reflogExpireUnreachable never &&\n+\n \techo file >file &&\n \tgit add file &&\n \tgit commit -a -m original\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553599","messageId":"e21b832a6e1d99416a220bb5ca1f008777ef4e7d.1790684309.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790684309.git.ben.knoble@gmail.com","subject":"[PATCH v4 5/5] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T12:18:31Z","receivedAt":"2026-09-29T12:20:50Z","isPatch":true,"body":"\"git stash apply --index\" does a 2-step dance to report index conflicts\nbefore carrying out the main unstash: first, attempt to merge the index\n(and remember the name of the resulting tree). If that succeeds, reset\nthe index and carry on unstashing the working tree, then use the\nremembered index tree to unstash the index.\n\nThe \"merge the index\" step is performed on the actual index by a\ncombination of git-diff-tree(1) and git-apply(1), which incurs an extra\ncost to git-reset(1) to cleanup. This also introduces an autostash bug\nwhen stash.index is true: \"git reset\" eventually wants to\nremove_merge_branch_state(), which calls save_autostash() due to\na03b55530a (merge: teach --autostash option, 2020-04-07). This can\nhappen from a \"git merge --autostash\", which itself calls\nsave_autostash(). Operating on the file-system in this way is not\nre-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH\nref [1]. This bug has lurked for a while, but it would have been\nimpossible to trigger without the availability of stash.index to force\nthe autostash apply into index mode.\n\n[1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/\n\nFortunately, we can achieve 2 goals at once: avoid round-tripping to the\nfile-system (and invoking expensive subprocesses) by performing the\nmerge in-core. If there are conflicts, we discard the resulting tree, so\nwe don't see the usual branch and ancestor labels, but the merge\nsubroutines insist on their presence, so use something simple.\n\nWe need to take care to get the order of trees right when merging. Add a\ntest that covers this case.\n\nWe *could* swap just the git-reset(1) subprocess with our internal\nreset_tree() and refresh_index(), which would fix the bug. We'd much\nprefer to clean up these vestiges of the shell-based git-stash, though.\n\nReported-by: Eli Barzilay \nHelped-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 80 ++++++++++--------------------------------------\n t/t3903-stash.sh | 21 +++++++++++++\n t/t7600-merge.sh | 9 ++++++\n 3 files changed, 47 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex d2b736d4e6..ec07547376 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,\n \treturn ret;\n }\n \n-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *w_commit_hex = oid_to_hex(w_commit);\n-\n-\t/*\n-\t * Diff-tree would not be very hard to replace with a native function,\n-\t * however it should be done together with apply_cached.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n-\tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n-\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n-\n-static int apply_cached(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Apply currently only reads either from stdin or a file, thus\n-\t * apply_all_patches would have to be updated to optionally take a\n-\t * buffer.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n-}\n-\n-static int reset_head(void)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Reset is overall quite simple, however there is no current public\n-\t * API for resetting.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n-\n-\treturn run_command(&cp);\n-}\n-\n static int is_path_a_directory(const char *path)\n {\n \t/*\n@@ -674,29 +630,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t oideq(&c_tree, &info->i_tree)) {\n \t\t\thas_index = 0;\n \t\t} else {\n-\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct merge_result result = { 0 };\n \n-\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n-\t\t\t\tstrbuf_release(&out);\n-\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n-\t\t\t\t\t oid_to_hex(&info->w_commit));\n-\t\t\t}\n+\t\t\to.branch1 = \"Current index\";\n+\t\t\to.branch2 = \"Stashed index changes\";\n+\t\t\to.ancestor = \"Stash base\";\n \n-\t\t\tret = apply_cached(&out);\n-\t\t\tstrbuf_release(&out);\n-\t\t\tif (ret)\n+\t\t\thead = lookup_tree(o.repo, &c_tree);\n+\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n+\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n+\t\t\t\t\t\t &result);\n+\n+\t\t\tif (!result.clean) {\n+\t\t\t\tmerge_finalize(&o, &result);\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n-\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n-\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n-\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n-\t\t\t\treturn error(_(\"could not save index tree\"));\n-\n-\t\t\treset_head();\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n+\t\t\t} else {\n+\t\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n+\t\t\t\tmerge_finalize(&o, &result);\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 70af58e161..70c6031958 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -395,6 +395,27 @@ setup_stash() {\n \ttest_cmp expect-index actual-index\n '\n \n+# the later \"stash -k\" test is not expecting us to muck with file so much, so\n+# reset when finished\n+test_expect_success 'stash apply --index merges the correct trees' '\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git reset --hard $head\" &&\n+\ttest_write_lines A B C >file &&\n+\tgit commit -m setup file &&\n+\ttest_write_lines A B staged >file &&\n+\tgit add file &&\n+\ttest_write_lines A B unstaged >file &&\n+\tgit stash &&\n+\ttest_write_lines committed B C >file &&\n+\tgit commit -m to-be-merged file &&\n+\tgit stash pop --index &&\n+\tgit show :file >actual &&\n+\ttest_write_lines committed B staged >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_write_lines committed B unstaged >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 64fe21717d..8f6109fb91 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -801,6 +801,15 @@ verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n '\n \n+test_expect_success 'fast-forward merge with --autostash, stash.index' '\n+\tgit reset --hard c0 &&\n+\tgit stash clear &&\n+\techo staged >>z && git add z &&\n+\tgit -c stash.index=true merge --autostash c1 2>err &&\n+\ttest_grep \"Applied autostash.\" err &&\n+\ttest_stdout_line_count = 0 git stash list\n+'\n+\n test_expect_success 'failed fast-forward merge with --autostash' '\n \tgit reset --hard c0 &&\n \tgit merge-file file file.orig file.5 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553609","messageId":"3547f4aa-649a-4f46-868c-0e50dfa69466@gmail.com","threadId":"66355","inReplyTo":"2ac371d2dc1425cc47bf369e88b321d3c0c8c605.1790684309.git.ben.knoble@gmail.com","subject":"Re: [PATCH v4 4/5] t5520: don't expire reflogs where it matters","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-29T15:46:03Z","receivedAt":"2026-09-29T15:46:07Z","isPatch":true,"body":"On 29/09/2026 13:18, D. Ben Knoble wrote:\n> From: Thomas Bachem \n> \n> The \"--rebase -f with rebased upstream\" test computes its fork point\n> from the reflog of refs/remotes/me/copy, and the entry it needs is\n> the one that the fetch of the test before it wrote. Like every reflog\n> entry the suite writes after test_tick, it is dated 2005, so the\n> first \"git reflog expire --all\" after that fetch removes it. Pull\n> then finds no fork point and rebases onto the merge head with the\n> merge head as the upstream, and the rewound commits come back as a\n> conflict.\n> \n> Since 452b12c2e0 (builtin/maintenance: use \"geometric\" strategy by\n> default, 2026-02-24) auto maintenance runs that expiry once the reflog\n> of HEAD holds a hundred entries it would remove, the default of\n> maintenance.reflog-expire.auto. Which run crosses the threshold\n> depends on the entries and maintenance runs before it, so the script\n> passed by chance: a stash topic that no longer runs \"git reset\" from\n> \"stash apply --index\" and a rebase topic that runs auto maintenance\n> at the end of \"git rebase\" together move the expiry between the two\n> tests.\n> \n> Pin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it\n> matters, 2026-02-24) did for the rebase tests. That covers a \"git gc\"\n> as well, which expires reflogs on its own, where turning off the auto\n> trigger of the reflog-expire task alone would not.\n\nI find this commit message quite hard to understand. From my perspective \nthe important points are\n\n - \"git merge\" uses \"git stash\" to clear any uncommitted changes from\n the worktree before it tries each strategy. The stashed changes are\n popped with \"--index\".\n\n - switching \"git stash pop --index\" to use merge_incore_nonrecursive()\n causes \"git merge\" to stop writing the reflog entries that came from\n \"git stash pop --index\" running \"git reset\"\n\n - that combined with \"git rebase\" starting to run \"git maintenance\n --auto\" changed when we expire the reflogs which breaks the fork-\n point detection.\n\nThe changes themselves look good\n\nThanks\n\nPhillip\n\n> \n> Reported-by: Junio C Hamano \n> Helped-by: D. Ben Knoble \n> Helped-by: Phillip Wood \n> Assisted-by: Claude Fable 5.1\n> Signed-off-by: Thomas Bachem \n> Signed-off-by: D. Ben Knoble \n> ---\n> t/t5520-pull.sh | 6 ++++++\n> 1 file changed, 6 insertions(+)\n> \n> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh\n> index 27f38ab3c8..bc818605a5 100755\n> --- a/t/t5520-pull.sh\n> +++ b/t/t5520-pull.sh\n> @@ -35,6 +35,12 @@ test_pull_autostash_fail () {\n> }\n> \n> test_expect_success setup '\n> +\t# Commit dates are hardcoded to 2005, and the reflog entries will have\n> +\t# a matching timestamp. Maintenance may thus immediately expire\n> +\t# reflogs if it was running.\n> +\tgit config set gc.reflogExpire never &&\n> +\tgit config set gc.reflogExpireUnreachable never &&\n> +\n> \techo file >file &&\n> \tgit add file &&\n> \tgit commit -a -m original\n\n"},{"id":"553610","messageId":"d5ac59be-0688-4d60-871a-2ccebc91c58b@gmail.com","threadId":"66355","inReplyTo":"cover.1790684309.git.ben.knoble@gmail.com","subject":"Re: [PATCH v4 0/5] stash: clean up index-mode test merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-29T15:48:14Z","receivedAt":"2026-09-29T15:48:18Z","isPatch":true,"body":"Hi Ben\n\nOn 29/09/2026 13:18, D. Ben Knoble wrote:\n> \n> Changes in v4:\n> • Drop merge verbosity changes altogether. I was going to\n> save-and-restore, but when looking at the index-merge test case (more\n> below) closer, I noticed that \"git apply --cached\" reports conflicts\n> on stderr. That is, \"git stash apply --index\" would report conflicts,\n> and silencing the merge takes that away. So instead let's leave the\n> configured verbosity alone.\n> • Only copy resulting index merge tree OID when successful\n> • Fix interaction with t5520 (new patch 4/5)\n> • Squash test from 3/5 into 5/5, since it requires actually merging\n> trees. I've elected to keep it a separate test for now (contrary to\n> Phillip's suggestion) since it's written and working. Adapting\n> existing tests requires quite a bit more digging into implicit context\n> assumptions ;)\n\nI've left a comment on the new patch 4, but everything else in the \nrange-diff looks ready to me.\n\nThanks\n\nPhillip\n\n> Changes in v3:\n> \n> • Change conflict label for current index\n> • Fix memory leak of merge_result\n> • Fix order of trees to make the correct merge (cherry-pick)\n> • New test (3/5) to validate this\n> • Fix test in 4/5 to assert more details of expected state\n> \n> Changes in v2:\n> \n> • Do give branch labels for the incore merge, although they are never\n> seen (and clarify commit message as a result, also keeping the\n> merge-ort asserts). Phillip was right: without those, we do segfault\n> on conflicts.\n> • Use the ui merge options to keep the same diff algorithm.\n> • Use merge_finalize instead of clear_merge_options, and reuse the\n> options between merge calls if they are already initialized.\n> • Add a new 2/4 to simplify merge options initialization.\n> • Add a new 3/4 with a test case for conflicted index merges.\n> \n> v1: \n> v2: \n> v3: \n> \n> [1/5] builtin/stash: remove unused header\n> [2/5] stash: prepare merge options earlier\n> [3/5] t3903: test failed \"stash apply --index\"\n> [4/5] t5520: don't expire reflogs where it matters\n> [5/5] builtin/stash: merge index in-core\n> \n> builtin/stash.c | 91 ++++++++++++------------------------------------\n> t/t3903-stash.sh | 42 ++++++++++++++++++++++\n> t/t5520-pull.sh | 6 ++++\n> t/t7600-merge.sh | 9 +++++\n> 4 files changed, 79 insertions(+), 69 deletions(-)\n> \n> Diff-intervalle contre v3 :\n> 1: 6a165c4df4 = 1: 6a165c4df4 builtin/stash: remove unused header\n> 2: d9a9e18f3a ! 2: 35b64ae321 stash: prepare merge options earlier\n> @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n> \t\treturn error(_(\"cannot apply a stash in the middle of a merge\"));\n> \n> +\tinit_ui_merge_options(&o, the_repository);\n> ++\n> ++\tif (quiet)\n> ++\t\to.verbosity = 0;\n> +\n> \tif (index) {\n> \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n> @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n> \to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n> \to.branch2 = label_theirs ? label_theirs : \"Stashed changes\";\n> \to.ancestor = label_base ? label_base : \"Stash base\";\n> +@@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefix,\n> + \tif (oideq(&info->b_tree, &c_tree))\n> + \t\to.branch1 = \"Version stash was based on\";\n> +\n> +-\tif (quiet)\n> +-\t\to.verbosity = 0;\n> +-\n> + \tif (o.verbosity >= 3)\n> + \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n> +\n> 4: d39e16905d ! 3: 7b0b317ce0 t3903: test failed \"stash apply --index\"\n> @@ Commit message\n> \n> ## t/t3903-stash.sh ##\n> @@ t/t3903-stash.sh: setup_stash() {\n> - \ttest_cmp expect file\n> + \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'stash apply --index leaves everything untouched on failure' '\n> 3: 8b5ea5e6f4 ! 4: 2ac371d2dc t3903: test stash --index merges\n> @@\n> ## Metadata ##\n> -Author: D. Ben Knoble \n> +Author: Thomas Bachem \n> \n> ## Commit message ##\n> - t3903: test stash --index merges\n> + t5520: don't expire reflogs where it matters\n> \n> - A future commit will refactor index handling for applied stashes, and we\n> - need to take care to get the order of trees right when merging. Add a\n> - test that covers this case.\n> + The \"--rebase -f with rebased upstream\" test computes its fork point\n> + from the reflog of refs/remotes/me/copy, and the entry it needs is\n> + the one that the fetch of the test before it wrote. Like every reflog\n> + entry the suite writes after test_tick, it is dated 2005, so the\n> + first \"git reflog expire --all\" after that fetch removes it. Pull\n> + then finds no fork point and rebases onto the merge head with the\n> + merge head as the upstream, and the rewound commits come back as a\n> + conflict.\n> \n> - Suggested-by: Phillip Wood \n> + Since 452b12c2e0 (builtin/maintenance: use \"geometric\" strategy by\n> + default, 2026-02-24) auto maintenance runs that expiry once the reflog\n> + of HEAD holds a hundred entries it would remove, the default of\n> + maintenance.reflog-expire.auto. Which run crosses the threshold\n> + depends on the entries and maintenance runs before it, so the script\n> + passed by chance: a stash topic that no longer runs \"git reset\" from\n> + \"stash apply --index\" and a rebase topic that runs auto maintenance\n> + at the end of \"git rebase\" together move the expiry between the two\n> + tests.\n> \n> - ## t/t3903-stash.sh ##\n> -@@ t/t3903-stash.sh: setup_stash() {\n> - \ttest_cmp expect actual\n> - '\n> + Pin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it\n> + matters, 2026-02-24) did for the rebase tests. That covers a \"git gc\"\n> + as well, which expires reflogs on its own, where turning off the auto\n> + trigger of the reflog-expire task alone would not.\n> +\n> + Reported-by: Junio C Hamano \n> + Helped-by: D. Ben Knoble \n> + Helped-by: Phillip Wood \n> + Assisted-by: Claude Fable 5.1\n> + Signed-off-by: Thomas Bachem \n> +\n> + ## t/t5520-pull.sh ##\n> +@@ t/t5520-pull.sh: test_pull_autostash_fail () {\n> + }\n> \n> -+# the later \"stash -k\" test is not expecting us to muck with file so much, so\n> -+# reset when finished\n> -+test_expect_success 'stash apply --index merges the correct trees' '\n> -+\thead=$(git rev-parse HEAD) &&\n> -+\ttest_when_finished \"git reset --hard $head\" &&\n> -+\ttest_write_lines A B C >file &&\n> -+\tgit commit -m setup file &&\n> -+\ttest_write_lines A B staged >file &&\n> -+\tgit add file &&\n> -+\ttest_write_lines A B unstaged >file &&\n> -+\tgit stash &&\n> -+\ttest_write_lines committed B C >file &&\n> -+\tgit commit -m to-be-merged file &&\n> -+\tgit stash pop --index &&\n> -+\tgit show :file >actual &&\n> -+\ttest_write_lines committed B staged >expect &&\n> -+\ttest_cmp expect actual &&\n> -+\ttest_write_lines committed B unstaged >expect &&\n> -+\ttest_cmp expect file\n> -+'\n> + test_expect_success setup '\n> ++\t# Commit dates are hardcoded to 2005, and the reflog entries will have\n> ++\t# a matching timestamp. Maintenance may thus immediately expire\n> ++\t# reflogs if it was running.\n> ++\tgit config set gc.reflogExpire never &&\n> ++\tgit config set gc.reflogExpireUnreachable never &&\n> +\n> - test_expect_success 'stash -k' '\n> - \techo bar3 >file &&\n> - \techo bar4 >file2 &&\n> + \techo file >file &&\n> + \tgit add file &&\n> + \tgit commit -a -m original\n> 5: fde7fb7988 ! 5: e21b832a6e builtin/stash: merge index in-core\n> @@ Commit message\n> we don't see the usual branch and ancestor labels, but the merge\n> subroutines insist on their presence, so use something simple.\n> \n> + We need to take care to get the order of trees right when merging. Add a\n> + test that covers this case.\n> +\n> We *could* swap just the git-reset(1) subprocess with our internal\n> reset_tree() and refresh_index(), which would fix the bug. We'd much\n> prefer to clean up these vestiges of the shell-based git-stash, though.\n> @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n> -\t\t\tret = apply_cached(&out);\n> -\t\t\tstrbuf_release(&out);\n> -\t\t\tif (ret)\n> -+\t\t\to.verbosity = 0;\n> -+\n> +\t\t\thead = lookup_tree(o.repo, &c_tree);\n> +\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n> +\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n> @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n> +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n> +\t\t\t\t\t\t &result);\n> +\n> -+\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n> -+\t\t\tmerge_finalize(&o, &result);\n> -+\n> -+\t\t\tif (!result.clean)\n> ++\t\t\tif (!result.clean) {\n> ++\t\t\t\tmerge_finalize(&o, &result);\n> \t\t\t\treturn error(_(\"conflicts in index. \"\n> \t\t\t\t\t \"Try without --index.\"));\n> -\n> @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n> -\t\t\treset_head();\n> -\t\t\tdiscard_index(the_repository->index);\n> -\t\t\trepo_read_index(the_repository);\n> ++\t\t\t} else {\n> ++\t\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n> ++\t\t\t\tmerge_finalize(&o, &result);\n> ++\t\t\t}\n> \t\t}\n> \t}\n> \n> \n> + ## t/t3903-stash.sh ##\n> +@@ t/t3903-stash.sh: setup_stash() {\n> + \ttest_cmp expect-index actual-index\n> + '\n> +\n> ++# the later \"stash -k\" test is not expecting us to muck with file so much, so\n> ++# reset when finished\n> ++test_expect_success 'stash apply --index merges the correct trees' '\n> ++\thead=$(git rev-parse HEAD) &&\n> ++\ttest_when_finished \"git reset --hard $head\" &&\n> ++\ttest_write_lines A B C >file &&\n> ++\tgit commit -m setup file &&\n> ++\ttest_write_lines A B staged >file &&\n> ++\tgit add file &&\n> ++\ttest_write_lines A B unstaged >file &&\n> ++\tgit stash &&\n> ++\ttest_write_lines committed B C >file &&\n> ++\tgit commit -m to-be-merged file &&\n> ++\tgit stash pop --index &&\n> ++\tgit show :file >actual &&\n> ++\ttest_write_lines committed B staged >expect &&\n> ++\ttest_cmp expect actual &&\n> ++\ttest_write_lines committed B unstaged >expect &&\n> ++\ttest_cmp expect file\n> ++'\n> ++\n> + test_expect_success 'stash -k' '\n> + \techo bar3 >file &&\n> + \techo bar4 >file2 &&\n> +\n> ## t/t7600-merge.sh ##\n> @@ t/t7600-merge.sh: verify_no_mergehead () {\n> \ttest_cmp result.1-5 file\n> \n> base-commit: d38352cd43ab9745686d697872408bc3249a153f\n\n"},{"id":"553611","messageId":"54957537-e40d-45b6-886c-5fc433f3d54e@gmail.com","threadId":"66355","inReplyTo":"CALnO6CC-eop86W3VREwGz0seG1pmtd0qS968TyP=mo_G+ZMrSA@mail.gmail.com","subject":"Re: [PATCH v3 0/5] stash: clean up index-mode test merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-09-29T15:54:28Z","receivedAt":"2026-09-29T15:54:31Z","isPatch":true,"body":"Hi Ben\n\nOn 28/09/2026 16:36, D. Ben Knoble wrote:\n> Let me see if I understand correctly…\n> \n> On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem wrote:\n>>\n>> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood wrote:\n>>> Oh, when I was thinking about this over lunch I did wonder if that might\n>>> be the culprit. Previously we didn't run \"git maintenance --auto\" after\n>>> a rebase with the 'merge' backend but with that topic we do, and because\n>>> we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'\n>>> gets triggered it will expire the reflog entries that 'git pull\n>>> --rebase' relies on. As you suggested in another mail, I assume this\n> \n>> \"git pull --rebase\" computes the fork point before it fetches, from\n>> the reflog of refs/remotes/me/copy,\n> \n> This is described by the manual for git-rebase under --fork-point,\n> which is on unless we have an or --keep-base (modulo\n> config). Put a pin in this.\n> \n>> and test 69 needs the entry that\n>> test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the\n>> reflog empty, \"merge-base --fork-point\" falls back to the ref itself,\n>> ae98574 is no ancestor of to-rebase, and pull hands the merge head to\n>> rebase as the upstream. That is your \"--onto ae98... ae98...\", and the\n>> four commits from copy-orig up come back, the first of them\n>> conflicting with \"conflict\".\n>>\n>>> topic has changed something in one of the '--autostash' tests that come\n>>> before the failing test triggers which the new behavior. What that\n>>> something is I'm not sure; off the top of my head I'd expect the number\n>>> of reflog entries in HEAD to be the same but maybe I'm missing\n>>> something. Adding\n>>\n>> It is eight entries fewer, and they come from the failed merges, not\n>> from the autostash tests. \"git merge\" restores a dirty tree with\n>> \"stash apply --index --quiet\", and until Ben's series that spawned\n>> \"git reset --quiet --refresh\", which writes \"reset: moving to HEAD\"\n>> to the reflog. That happens eight times in t5520 before test 68.\n>>\n>> Auto maintenance expires reflogs once HEAD's reflog holds a hundred\n>> entries that the policy would remove, the default of\n>> maintenance.reflog-expire.auto, and after the first test_tick that is\n>> every entry. Which run crosses the hundred depends on how many entries\n>> and maintenance runs came before it. On 'seen' the expiry lands on\n>> \"git commit -m conflict\" in test 68, before the fetch writes the entry.\n>> Eight entries fewer move the crossing past that commit, and the\n>> maintenance run my topic adds at the end of the rebase in test 68 is\n>> the next one: after the fetch, before test 69 reads the reflog. Either\n>> change alone leaves it somewhere harmless, and nothing else is going\n>> on. The expiry is the usual 90 days applied to entries dated 2005, and\n>> the only new thing is one more maintenance run per rebase, the same\n>> one \"git commit\" and \"git fetch\" run.\n> \n> In short, expiry used to happen prior to .68, so the reflog entry\n> created in that test which is used by \"pull --rebase\" in .69 is picked\n> up. With fewer reflog entries, expiry happens later, and it just so\n> happens to drop the important entry. Darn!\n\nYes, it is incredibly bad luck that the test broke, though I guess it is \nalso fortunate as it means we can fix the latent bug in the test.\n\n> \n> But here's what I can't figure out, returning to that pin from\n> earlier: I was a bit surprised to see mention of rebase reading\n> reflogs! When I remembered --fork-point, I was even more curious (but\n> at least it's obvious that rebase will read the reflogs in some\n> scenarios).\n> \n> What confuses me is that builtin/pull.c:run_rebase() sure looks like\n> it provides an to the command invocation, so shouldn't\n> --fork-point and reflog use be disabled????\n> \n> I'll try tracing that test myself later, I suppose. It's nice to know\n> we have a fix available (thanks for the patch), but it sure feels like\n> a hack :) oh well?\n\nIt is a bit confusing that \"git pull --rebase\" does not use \"git rebase \n--fork-point\", instead it calls \"git merge-base --fork-point\" (which is \nwhere we read the reflog of the remote branch) itself and then passes \nthat as the upstream revision to \"git rebase\". I think this is because \nfork-point handling was added to \"git pull\" before \"--fork-point\" \nexisted in \"git rebase\". \"git rebase\" only looks for a fork-point if its \nupstream argument is a ref, so as \"git pull\" passes an object id, the \nfork-point detection in rebase is bypassed.\n\nThanks\n\nPhillip\n\n"},{"id":"553620","messageId":"F407EDB6-80C5-45AA-B8DE-CCD61DB663F7@gmail.com","threadId":"66355","inReplyTo":"d5ac59be-0688-4d60-871a-2ccebc91c58b@gmail.com","subject":"Re: [PATCH v4 0/5] stash: clean up index-mode test merge","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-29T17:31:57Z","receivedAt":"2026-09-29T17:32:11Z","isPatch":true,"body":"\n> Le 29 sept. 2026 à 11:48, Phillip Wood a écrit :\n> \n> Hi Ben\n> \n>> On 29/09/2026 13:18, D. Ben Knoble wrote:\n>> Changes in v4:\n>> • Drop merge verbosity changes altogether. I was going to\n>> save-and-restore, but when looking at the index-merge test case (more\n>> below) closer, I noticed that \"git apply --cached\" reports conflicts\n>> on stderr. That is, \"git stash apply --index\" would report conflicts,\n>> and silencing the merge takes that away. So instead let's leave the\n>> configured verbosity alone.\n>> • Only copy resulting index merge tree OID when successful\n>> • Fix interaction with t5520 (new patch 4/5)\n>> • Squash test from 3/5 into 5/5, since it requires actually merging\n>> trees. I've elected to keep it a separate test for now (contrary to\n>> Phillip's suggestion) since it's written and working. Adapting\n>> existing tests requires quite a bit more digging into implicit context\n>> assumptions ;)\n> \n> I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me.\n> \n> Thanks\n> \n> Phillip\n\nThanks Phillip. Pending other positive acks, I’m not sure if I should reroll with Thomas’s new patch, reroll dropping it now there’s a seen topic for it, or just wait ;)\n\nI’ll probably wait a bit and see how the dust settles, but:\n\nJunio if you want to see a reroll hit the list using the new synthetic base to make things nicer for you, I can do so. In particular, I think the last check I made when I saw your mail about the synthetic base had the prior round."},{"id":"553637","messageId":"xmqq4if7g6u1.fsf@gitster.g","threadId":"66355","inReplyTo":"e21b832a6e1d99416a220bb5ca1f008777ef4e7d.1790684309.git.ben.knoble@gmail.com","subject":"Re: [PATCH v4 5/5] builtin/stash: merge index in-core","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-29T20:07:02Z","receivedAt":"2026-09-29T20:07:06Z","isPatch":true,"body":"\"D. Ben Knoble\" writes:\n\n> +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n> +\t\t\t\t\t\t &result);\n\nIn a hard error from merge_incore_nonrecursive(), result->clean is\nset to -1, which means that ...\n\n> +\t\t\tif (!result.clean) {\n\n... \"result.clean is false\" is not true here, so we will ...\n\n> +\t\t\t\tmerge_finalize(&o, &result);\n> \t\t\t\treturn error(_(\"conflicts in index. \"\n> \t\t\t\t\t \"Try without --index.\"));\n> +\t\t\t} else {\n\n... come here to access result.tree member, no?\n\n> +\t\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n> +\t\t\t\tmerge_finalize(&o, &result);\n> +\t\t\t}\n\nIOW, shouldn't it be more like three-way check,\n\n\t\t\tif (result.clean < 0) {\n\t\t\t\tmerge_finalize(&o, &result);\n return error(_(\"index merge failed.\"));\n\t\t\t} else if (!result.clean) {\n\t\t\t\tmerge_finalize(&o, &result);\n return error(_(\"conflict in index merge.\"));\n\t\t\t} else {\n\t\t\t\t... happy path ...\n\t\t\t}\n\nor something like that?\n"},{"id":"553665","messageId":"CALnO6CDDAomqb+MqRw10Kj048gL5+k+3k_4kVxbjTB1YrK3fXg@mail.gmail.com","threadId":"66355","inReplyTo":"xmqq4if7g6u1.fsf@gitster.g","subject":"Re: [PATCH v4 5/5] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T01:29:25Z","receivedAt":"2026-09-30T01:29:37Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 4:07 PM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > + merge_incore_nonrecursive(&o, merge_base, head, merge,\n> > + &result);\n>\n> In a hard error from merge_incore_nonrecursive(), result->clean is\n> set to -1, which means that ...\n>\n> > + if (!result.clean) {\n>\n> ... \"result.clean is false\" is not true here, so we will ...\n>\n> > + merge_finalize(&o, &result);\n> > return error(_(\"conflicts in index. \"\n> > \"Try without --index.\"));\n> > + } else {\n>\n> ... come here to access result.tree member, no?\n>\n> > + oidcpy(&index_tree, &result.tree->object.oid);\n> > + merge_finalize(&o, &result);\n> > + }\n>\n> IOW, shouldn't it be more like three-way check,\n\nYep. Missed that when looking at the result struct. Will fix.\n\n>\n> if (result.clean < 0) {\n> merge_finalize(&o, &result);\n> return error(_(\"index merge failed.\"));\n> } else if (!result.clean) {\n> merge_finalize(&o, &result);\n> return error(_(\"conflict in index merge.\"));\n> } else {\n> ... happy path ...\n> }\n>\n> or something like that?\n\n\n-- \nD. Ben Knoble\n"},{"id":"553763","messageId":"d8f4c3645977a555c5bdeb1111597f54d6906d92.1790803471.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790803471.git.ben.knoble@gmail.com","subject":"[PATCH v5 1/4] builtin/stash: remove unused header","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T21:24:38Z","receivedAt":"2026-09-30T21:25:51Z","isPatch":true,"body":"Clang complains that oid-array.h is unused. Certainly none of the\noid_array* functions, types, etc., are used, and the\ntransitively-included hash.h declarations are used but covered by a\npre-existing direct #include of hash.h.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 7a9843413b..dfea2d2c4c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -31,7 +31,6 @@\n #include \"reflog.h\"\n #include \"reflog-walk.h\"\n #include \"add-interactive.h\"\n-#include \"oid-array.h\"\n #include \"commit.h\"\n \n #define INCLUDE_ALL_FILES 2\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553765","messageId":"cover.1790803471.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1789853192.git.ben.knoble@gmail.com","subject":"[PATCH v5 0/4] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T21:24:37Z","receivedAt":"2026-09-30T21:25:51Z","isPatch":true,"body":"Hi all,\n\nThis small patch series fixes a bug reported by Eli Barzilay in the\ninteraction between autostashing, staged index entries, and\nstash.index=true.\n\nThe first patch is an incidental cleanup, and the second re-arranges one\nline to make the change easier. The third adds missing test coverage\n(which catch breakages from prior incorrect rounds of this series). The\nfourth fixes a test interaction with another in-flight topic. The last\nholds the interesting bits.\n\nChanges in v5:\n• Rebase on synthetic merge for the test interaction with t5520\n (dropping old 4/5) [59d1ce1b6e (Merge branch 'tb/t5520-reflog-expire'\n into dk/stash-apply-index-incore, 2026-09-29)]\n• Fix handling of tri-state merge_result.clean\n\nChanges in v4:\n• Drop merge verbosity changes altogether. I was going to\n save-and-restore, but when looking at the index-merge test case (more\n below) closer, I noticed that \"git apply --cached\" reports conflicts\n on stderr. That is, \"git stash apply --index\" would report conflicts,\n and silencing the merge takes that away. So instead let's leave the\n configured verbosity alone.\n• Only copy resulting index merge tree OID when successful\n• Fix interaction with t5520 (new patch 4/5)\n• Squash test from 3/5 into 5/5, since it requires actually merging\n trees. I've elected to keep it a separate test for now (contrary to\n Phillip's suggestion) since it's written and working. Adapting\n existing tests requires quite a bit more digging into implicit context\n assumptions ;)\n\nChanges in v3:\n\n• Change conflict label for current index\n• Fix memory leak of merge_result\n• Fix order of trees to make the correct merge (cherry-pick)\n • New test (3/5) to validate this\n• Fix test in 4/5 to assert more details of expected state\n\nChanges in v2:\n\n• Do give branch labels for the incore merge, although they are never\n seen (and clarify commit message as a result, also keeping the\n merge-ort asserts). Phillip was right: without those, we do segfault\n on conflicts.\n• Use the ui merge options to keep the same diff algorithm.\n• Use merge_finalize instead of clear_merge_options, and reuse the\n options between merge calls if they are already initialized.\n• Add a new 2/4 to simplify merge options initialization.\n• Add a new 3/4 with a test case for conflicted index merges.\n\nv1: \nv2: \nv3: \nv4: \n\n[1/4] builtin/stash: remove unused header\n[2/4] stash: prepare merge options earlier\n[3/4] t3903: test failed \"stash apply --index\"\n[4/4] builtin/stash: merge index in-core\n\n builtin/stash.c | 94 +++++++++++++-----------------------------------\n t/t3903-stash.sh | 42 ++++++++++++++++++++++\n t/t7600-merge.sh | 9 +++++\n 3 files changed, 76 insertions(+), 69 deletions(-)\n\nDiff-intervalle contre v4 :\n1: 6a165c4df4 = 1: d8f4c36459 builtin/stash: remove unused header\n2: 35b64ae321 = 2: 8e99033ef0 stash: prepare merge options earlier\n3: 7b0b317ce0 = 3: ee28d0a840 t3903: test failed \"stash apply --index\"\n4: 2ac371d2dc < -: ---------- t5520: don't expire reflogs where it matters\n5: e21b832a6e ! 4: ca3de1d4a3 builtin/stash: merge index in-core\n @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n +\t\t\t\t\t\t &result);\n +\n -+\t\t\tif (!result.clean) {\n ++\t\t\tif (result.clean < 0) {\n ++\t\t\t\tmerge_finalize(&o, &result);\n ++\t\t\t\treturn error(_(\"index merge failed\"));\n ++\t\t\t} else if (!result.clean) {\n +\t\t\t\tmerge_finalize(&o, &result);\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\nprerequisite-patch-id: 601853fa5478b0dbfb260ba02632418e90338219\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553766","messageId":"ee28d0a8406a7e03eed1251e9aac307c4e014116.1790803471.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790803471.git.ben.knoble@gmail.com","subject":"[PATCH v5 3/4] t3903: test failed \"stash apply --index\"","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T21:24:40Z","receivedAt":"2026-09-30T21:25:52Z","isPatch":true,"body":"The next commit will refactor index handling for applied stashes, so\nlet's make sure we cover conflicted index merging, too.\n\nHelped-by: Phillip Wood \nSigned-off-by: D. Ben Knoble \n---\n t/t3903-stash.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 721158606f..70af58e161 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -374,6 +374,27 @@ setup_stash() {\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'stash apply --index leaves everything untouched on failure' '\n+\tgit reset --hard &&\n+\techo test >other-file &&\n+\tgit add other-file &&\n+\tgit stash &&\n+\techo unrelated >file &&\n+\techo unrelated >another-file &&\n+\tgit add another-file &&\n+\techo conflict >other-file &&\n+\tgit add other-file &&\n+\tgit diff-files -p >expect &&\n+\tgit diff-index --cached HEAD >expect-index &&\n+\n+\ttest_must_fail git stash apply --index 2>err &&\n+\ttest_grep \"conflicts in index. Try without --index\" err &&\n+\tgit diff-files -p >actual &&\n+\ttest_cmp expect actual &&\n+\tgit diff-index --cached HEAD >actual-index &&\n+\ttest_cmp expect-index actual-index\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553764","messageId":"8e99033ef025003c35bd82069ea89b1509e8bd9b.1790803471.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790803471.git.ben.knoble@gmail.com","subject":"[PATCH v5 2/4] stash: prepare merge options earlier","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T21:24:39Z","receivedAt":"2026-09-30T21:25:53Z","isPatch":true,"body":"In a future commit, we will reuse these options for the index merge of\n\"apply --index\", not just for the worktree.\n\nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex dfea2d2c4c..d2b736d4e6 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -664,6 +664,11 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n \t\treturn error(_(\"cannot apply a stash in the middle of a merge\"));\n \n+\tinit_ui_merge_options(&o, the_repository);\n+\n+\tif (quiet)\n+\t\to.verbosity = 0;\n+\n \tif (index) {\n \t\tif (oideq(&info->b_tree, &info->i_tree) ||\n \t\t oideq(&c_tree, &info->i_tree)) {\n@@ -695,8 +700,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t}\n \t}\n \n-\tinit_ui_merge_options(&o, the_repository);\n-\n \to.branch1 = label_ours ? label_ours : \"Updated upstream\";\n \to.branch2 = label_theirs ? label_theirs : \"Stashed changes\";\n \to.ancestor = label_base ? label_base : \"Stash base\";\n@@ -704,9 +707,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \tif (oideq(&info->b_tree, &c_tree))\n \t\to.branch1 = \"Version stash was based on\";\n \n-\tif (quiet)\n-\t\to.verbosity = 0;\n-\n \tif (o.verbosity >= 3)\n \t\tprintf_ln(_(\"Merging %s with %s\"), o.branch1, o.branch2);\n \n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553767","messageId":"ca3de1d4a3895fcce620eb4ab5b8a1cc708a7362.1790803471.git.ben.knoble@gmail.com","threadId":"66355","inReplyTo":"cover.1790803471.git.ben.knoble@gmail.com","subject":"[PATCH v5 4/4] builtin/stash: merge index in-core","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T21:24:41Z","receivedAt":"2026-09-30T21:25:55Z","isPatch":true,"body":"\"git stash apply --index\" does a 2-step dance to report index conflicts\nbefore carrying out the main unstash: first, attempt to merge the index\n(and remember the name of the resulting tree). If that succeeds, reset\nthe index and carry on unstashing the working tree, then use the\nremembered index tree to unstash the index.\n\nThe \"merge the index\" step is performed on the actual index by a\ncombination of git-diff-tree(1) and git-apply(1), which incurs an extra\ncost to git-reset(1) to cleanup. This also introduces an autostash bug\nwhen stash.index is true: \"git reset\" eventually wants to\nremove_merge_branch_state(), which calls save_autostash() due to\na03b55530a (merge: teach --autostash option, 2020-04-07). This can\nhappen from a \"git merge --autostash\", which itself calls\nsave_autostash(). Operating on the file-system in this way is not\nre-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH\nref [1]. This bug has lurked for a while, but it would have been\nimpossible to trigger without the availability of stash.index to force\nthe autostash apply into index mode.\n\n[1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/\n\nFortunately, we can achieve 2 goals at once: avoid round-tripping to the\nfile-system (and invoking expensive subprocesses) by performing the\nmerge in-core. If there are conflicts, we discard the resulting tree, so\nwe don't see the usual branch and ancestor labels, but the merge\nsubroutines insist on their presence, so use something simple.\n\nWe need to take care to get the order of trees right when merging. Add a\ntest that covers this case.\n\nWe *could* swap just the git-reset(1) subprocess with our internal\nreset_tree() and refresh_index(), which would fix the bug. We'd much\nprefer to clean up these vestiges of the shell-based git-stash, though.\n\nReported-by: Eli Barzilay \nHelped-by: Phillip Wood \nHelped-by: Junio C Hamano \nSigned-off-by: D. Ben Knoble \n---\n builtin/stash.c | 83 ++++++++++++------------------------------------\n t/t3903-stash.sh | 21 ++++++++++++\n t/t7600-merge.sh | 9 ++++++\n 3 files changed, 50 insertions(+), 63 deletions(-)\n\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex d2b736d4e6..fa3deeecbe 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,\n \treturn ret;\n }\n \n-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *w_commit_hex = oid_to_hex(w_commit);\n-\n-\t/*\n-\t * Diff-tree would not be very hard to replace with a native function,\n-\t * however it should be done together with apply_cached.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"diff-tree\", \"--binary\", \"--no-color\", NULL);\n-\tstrvec_pushf(&cp.args, \"%s^2^..%s^2\", w_commit_hex, w_commit_hex);\n-\n-\treturn pipe_command(&cp, NULL, 0, out, 0, NULL, 0);\n-}\n-\n-static int apply_cached(struct strbuf *out)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Apply currently only reads either from stdin or a file, thus\n-\t * apply_all_patches would have to be updated to optionally take a\n-\t * buffer.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"apply\", \"--cached\", NULL);\n-\treturn pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);\n-}\n-\n-static int reset_head(void)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\t/*\n-\t * Reset is overall quite simple, however there is no current public\n-\t * API for resetting.\n-\t */\n-\tcp.git_cmd = 1;\n-\tstrvec_pushl(&cp.args, \"reset\", \"--quiet\", \"--refresh\", NULL);\n-\n-\treturn run_command(&cp);\n-}\n-\n static int is_path_a_directory(const char *path)\n {\n \t/*\n@@ -674,29 +630,30 @@ static enum stash_apply_result do_apply_stash(const char *prefix,\n \t\t oideq(&c_tree, &info->i_tree)) {\n \t\t\thas_index = 0;\n \t\t} else {\n-\t\t\tstruct strbuf out = STRBUF_INIT;\n+\t\t\tstruct merge_result result = { 0 };\n \n-\t\t\tif (diff_tree_binary(&out, &info->w_commit)) {\n-\t\t\t\tstrbuf_release(&out);\n-\t\t\t\treturn error(_(\"could not generate diff %s^!.\"),\n-\t\t\t\t\t oid_to_hex(&info->w_commit));\n-\t\t\t}\n+\t\t\to.branch1 = \"Current index\";\n+\t\t\to.branch2 = \"Stashed index changes\";\n+\t\t\to.ancestor = \"Stash base\";\n \n-\t\t\tret = apply_cached(&out);\n-\t\t\tstrbuf_release(&out);\n-\t\t\tif (ret)\n+\t\t\thead = lookup_tree(o.repo, &c_tree);\n+\t\t\tmerge = lookup_tree(o.repo, &info->i_tree);\n+\t\t\tmerge_base = lookup_tree(o.repo, &info->b_tree);\n+\n+\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n+\t\t\t\t\t\t &result);\n+\n+\t\t\tif (result.clean < 0) {\n+\t\t\t\tmerge_finalize(&o, &result);\n+\t\t\t\treturn error(_(\"index merge failed\"));\n+\t\t\t} else if (!result.clean) {\n+\t\t\t\tmerge_finalize(&o, &result);\n \t\t\t\treturn error(_(\"conflicts in index. \"\n \t\t\t\t\t \"Try without --index.\"));\n-\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n-\t\t\tif (write_index_as_tree(&index_tree, the_repository->index,\n-\t\t\t\t\t\trepo_get_index_file(the_repository), 0, NULL))\n-\t\t\t\treturn error(_(\"could not save index tree\"));\n-\n-\t\t\treset_head();\n-\t\t\tdiscard_index(the_repository->index);\n-\t\t\trepo_read_index(the_repository);\n+\t\t\t} else {\n+\t\t\t\toidcpy(&index_tree, &result.tree->object.oid);\n+\t\t\t\tmerge_finalize(&o, &result);\n+\t\t\t}\n \t\t}\n \t}\n \ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex 70af58e161..70c6031958 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -395,6 +395,27 @@ setup_stash() {\n \ttest_cmp expect-index actual-index\n '\n \n+# the later \"stash -k\" test is not expecting us to muck with file so much, so\n+# reset when finished\n+test_expect_success 'stash apply --index merges the correct trees' '\n+\thead=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git reset --hard $head\" &&\n+\ttest_write_lines A B C >file &&\n+\tgit commit -m setup file &&\n+\ttest_write_lines A B staged >file &&\n+\tgit add file &&\n+\ttest_write_lines A B unstaged >file &&\n+\tgit stash &&\n+\ttest_write_lines committed B C >file &&\n+\tgit commit -m to-be-merged file &&\n+\tgit stash pop --index &&\n+\tgit show :file >actual &&\n+\ttest_write_lines committed B staged >expect &&\n+\ttest_cmp expect actual &&\n+\ttest_write_lines committed B unstaged >expect &&\n+\ttest_cmp expect file\n+'\n+\n test_expect_success 'stash -k' '\n \techo bar3 >file &&\n \techo bar4 >file2 &&\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 64fe21717d..8f6109fb91 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -801,6 +801,15 @@ verify_no_mergehead () {\n \ttest_cmp result.1-5 file\n '\n \n+test_expect_success 'fast-forward merge with --autostash, stash.index' '\n+\tgit reset --hard c0 &&\n+\tgit stash clear &&\n+\techo staged >>z && git add z &&\n+\tgit -c stash.index=true merge --autostash c1 2>err &&\n+\ttest_grep \"Applied autostash.\" err &&\n+\ttest_stdout_line_count = 0 git stash list\n+'\n+\n test_expect_success 'failed fast-forward merge with --autostash' '\n \tgit reset --hard c0 &&\n \tgit merge-file file file.orig file.5 &&\n-- \n2.56.0.rc1.315.gc6ed9934b7.dirty\n\n"},{"id":"553768","messageId":"CALnO6CALq2V0Nmx=VE8X79VVhNxa_xiH3dtv+QixaHsB1=K4iA@mail.gmail.com","threadId":"66355","inReplyTo":"F407EDB6-80C5-45AA-B8DE-CCD61DB663F7@gmail.com","subject":"Re: [PATCH v4 0/5] stash: clean up index-mode test merge","fromName":"D. Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-09-30T21:26:38Z","receivedAt":"2026-09-30T21:26:50Z","isPatch":true,"body":"On Tue, Sep 29, 2026 at 1:32 PM Ben Knoble wrote:\n>\n>\n> > Le 29 sept. 2026 à 11:48, Phillip Wood a écrit :\n> >\n> > Hi Ben\n> >\n> >> On 29/09/2026 13:18, D. Ben Knoble wrote:\n> >> Changes in v4:\n> >> • Drop merge verbosity changes altogether. I was going to\n> >> save-and-restore, but when looking at the index-merge test case (more\n> >> below) closer, I noticed that \"git apply --cached\" reports conflicts\n> >> on stderr. That is, \"git stash apply --index\" would report conflicts,\n> >> and silencing the merge takes that away. So instead let's leave the\n> >> configured verbosity alone.\n> >> • Only copy resulting index merge tree OID when successful\n> >> • Fix interaction with t5520 (new patch 4/5)\n> >> • Squash test from 3/5 into 5/5, since it requires actually merging\n> >> trees. I've elected to keep it a separate test for now (contrary to\n> >> Phillip's suggestion) since it's written and working. Adapting\n> >> existing tests requires quite a bit more digging into implicit context\n> >> assumptions ;)\n> >\n> > I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me.\n> >\n> > Thanks\n> >\n> > Phillip\n>\n> Thanks Phillip. Pending other positive acks, I’m not sure if I should reroll with Thomas’s new patch, reroll dropping it now there’s a seen topic for it, or just wait ;)\n>\n> I’ll probably wait a bit and see how the dust settles, but:\n>\n> Junio if you want to see a reroll hit the list using the new synthetic base to make things nicer for you, I can do so. In particular, I think the last check I made when I saw your mail about the synthetic base had the prior round.\n\nI realized Junio wasn't CC'd on the prior mail, but since I re-rolled\nand the merge base changed, I think I've got it right for v5, which\njust went out.\n\n-- \nD. Ben Knoble\n"},{"id":"553843","messageId":"d3adb734-2b84-4d7b-b245-5407ee410eb4@gmail.com","threadId":"66355","inReplyTo":"cover.1790803471.git.ben.knoble@gmail.com","subject":"Re: [PATCH v5 0/4] stash: clean up index-mode test merge","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2026-10-01T15:52:19Z","receivedAt":"2026-10-01T15:52:29Z","isPatch":true,"body":"Hi Ben\n\nOn 30/09/2026 22:24, D. Ben Knoble wrote:\n> \n> Changes in v5:\n> • Rebase on synthetic merge for the test interaction with t5520\n> (dropping old 4/5) [59d1ce1b6e (Merge branch 'tb/t5520-reflog-expire'\n> into dk/stash-apply-index-incore, 2026-09-29)]\n> • Fix handling of tri-state merge_result.clean\n\nThe range-diff below looks as expected, thanks for working on this, I'm \nreally pleased to see us removing some subprocesses from \"git stash\".\n\nThanks\n\nPhillip\n\n> Changes in v4:\n> • Drop merge verbosity changes altogether. I was going to\n> save-and-restore, but when looking at the index-merge test case (more\n> below) closer, I noticed that \"git apply --cached\" reports conflicts\n> on stderr. That is, \"git stash apply --index\" would report conflicts,\n> and silencing the merge takes that away. So instead let's leave the\n> configured verbosity alone.\n> • Only copy resulting index merge tree OID when successful\n> • Fix interaction with t5520 (new patch 4/5)\n> • Squash test from 3/5 into 5/5, since it requires actually merging\n> trees. I've elected to keep it a separate test for now (contrary to\n> Phillip's suggestion) since it's written and working. Adapting\n> existing tests requires quite a bit more digging into implicit context\n> assumptions ;)\n> \n> Changes in v3:\n> \n> • Change conflict label for current index\n> • Fix memory leak of merge_result\n> • Fix order of trees to make the correct merge (cherry-pick)\n> • New test (3/5) to validate this\n> • Fix test in 4/5 to assert more details of expected state\n> \n> Changes in v2:\n> \n> • Do give branch labels for the incore merge, although they are never\n> seen (and clarify commit message as a result, also keeping the\n> merge-ort asserts). Phillip was right: without those, we do segfault\n> on conflicts.\n> • Use the ui merge options to keep the same diff algorithm.\n> • Use merge_finalize instead of clear_merge_options, and reuse the\n> options between merge calls if they are already initialized.\n> • Add a new 2/4 to simplify merge options initialization.\n> • Add a new 3/4 with a test case for conflicted index merges.\n> \n> v1: \n> v2: \n> v3: \n> v4: \n> \n> [1/4] builtin/stash: remove unused header\n> [2/4] stash: prepare merge options earlier\n> [3/4] t3903: test failed \"stash apply --index\"\n> [4/4] builtin/stash: merge index in-core\n> \n> builtin/stash.c | 94 +++++++++++++-----------------------------------\n> t/t3903-stash.sh | 42 ++++++++++++++++++++++\n> t/t7600-merge.sh | 9 +++++\n> 3 files changed, 76 insertions(+), 69 deletions(-)\n> \n> Diff-intervalle contre v4 :\n> 1: 6a165c4df4 = 1: d8f4c36459 builtin/stash: remove unused header\n> 2: 35b64ae321 = 2: 8e99033ef0 stash: prepare merge options earlier\n> 3: 7b0b317ce0 = 3: ee28d0a840 t3903: test failed \"stash apply --index\"\n> 4: 2ac371d2dc < -: ---------- t5520: don't expire reflogs where it matters\n> 5: e21b832a6e ! 4: ca3de1d4a3 builtin/stash: merge index in-core\n> @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi\n> +\t\t\tmerge_incore_nonrecursive(&o, merge_base, head, merge,\n> +\t\t\t\t\t\t &result);\n> +\n> -+\t\t\tif (!result.clean) {\n> ++\t\t\tif (result.clean < 0) {\n> ++\t\t\t\tmerge_finalize(&o, &result);\n> ++\t\t\t\treturn error(_(\"index merge failed\"));\n> ++\t\t\t} else if (!result.clean) {\n> +\t\t\t\tmerge_finalize(&o, &result);\n> \t\t\t\treturn error(_(\"conflicts in index. \"\n> \t\t\t\t\t \"Try without --index.\"));\n> \n> base-commit: a018953688f1b10bddf91bff8747068f5f4746a4\n> prerequisite-patch-id: 601853fa5478b0dbfb260ba02632418e90338219\n\n"},{"id":"553858","messageId":"xmqq4if55n3u.fsf@gitster.g","threadId":"66355","inReplyTo":"d3adb734-2b84-4d7b-b245-5407ee410eb4@gmail.com","subject":"Re: [PATCH v5 0/4] stash: clean up index-mode test merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-10-01T17:47:49Z","receivedAt":"2026-10-01T17:47:55Z","isPatch":true,"body":"Phillip Wood writes:\n\n> Hi Ben\n>\n> On 30/09/2026 22:24, D. Ben Knoble wrote:\n>> \n>> Changes in v5:\n>> • Rebase on synthetic merge for the test interaction with t5520\n>> (dropping old 4/5) [59d1ce1b6e (Merge branch 'tb/t5520-reflog-expire'\n>> into dk/stash-apply-index-incore, 2026-09-29)]\n>> • Fix handling of tri-state merge_result.clean\n>\n> The range-diff below looks as expected, thanks for working on this, I'm \n> really pleased to see us removing some subprocesses from \"git stash\".\n\nThanks for writing and reviewing. These now look very good to me\ntoo.\n\nLet me mark them for 'next'.\n"}]}