> "git stash apply --index" does a 2-step dance to report index conflicts
> before carrying out the main unstash: first, attempt to merge the index
> (and remember the name of the resulting tree). If that succeeds, reset
> the index and carry on unstashing the working tree, then use the
> remembered index tree to unstash the index.
> 
> The "merge the index" step is performed on the actual index by a
> combination of git-diff-tree(1) and git-apply(1), which incurs an extra
> cost to git-reset(1) to cleanup. This also introduces an autostash bug
> when stash.index is true: "git reset" eventually wants to
> remove_merge_branch_state(), which calls save_autostash() due to
> a03b55530a (merge: teach --autostash option, 2020-04-07). This can
> happen from a "git merge --autostash", which itself calls
> save_autostash(). Operating on the file-system in this way is not
> re-entrant, so we end up trying to lock a now-deleted MERGE_AUTOSTASH
> ref [1]. This bug has lurked for a while, but it would have been
> impossible to trigger without the availability of stash.index to force
> the autostash apply into index mode.
> 
> [1]: https://lore.kernel.org/git/CALO-guvbk2TcrVwzdNQ3yRpzHr0HHZ3h1wite0Xp0sUyAT4otA@mail.gmail.com/
> 
> Fortunately, we can achieve 2 goals at once: avoid round-tripping to the
> file-system (and invoking expensive subprocesses) by performing the
> merge in-core. Since the results are never seen, we don't need to set
> the usual branch and ancestor labels.

> @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>   		    oideq(&c_tree, &info->i_tree)) {
>   			has_index = 0;
>   		} else {
> -			struct strbuf out = STRBUF_INIT;
> +			struct merge_result result = { 0 };
>   
> -			if (diff_tree_binary(&out, &info->w_commit)) {
> -				strbuf_release(&out);
> -				return error(_("could not generate diff %s^!."),
> -					     oid_to_hex(&info->w_commit));
> -			}
> +			init_basic_merge_options(&o, the_repository);

>   
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)
> +			o.verbosity = 0;

> +
> +			head = lookup_tree(o.repo, &c_tree);
> +			merge = lookup_tree(o.repo, &info->i_tree);
> +			merge_base = lookup_tree(o.repo, &info->b_tree);
> +
> +			merge_incore_nonrecursive(&o, head, merge, merge_base,
> +						  &result);
> +
> +			if (!result.clean)
>   				return error(_("conflicts in index. "
>   					       "Try without --index."));
>   
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
> -			if (write_index_as_tree(&index_tree, the_repository->index,
> -						repo_get_index_file(the_repository), 0, NULL))
> -				return error(_("could not save index tree"));
> -
> -			reset_head();
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
> +			oidcpy(&index_tree, &result.tree->object.oid);
> +			clear_merge_options(&o);

>   		}
>   	}
>   
> diff --git a/merge-ort.c b/merge-ort.c
> index c410a5d353..f69a49d48a 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
>   	trace2_region_enter("merge", "sanity checks", opt->repo);
>   	assert(opt->repo);
>   
> -	assert(opt->branch1 && opt->branch2);

>   	assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE &&
>   	       opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE);
>   	assert(opt->rename_limit >= -1);
> @@ -5409,7 +5407,6 @@ void merge_incore_nonrecursive(struct merge_options *opt,
>   	trace2_region_enter("merge", "incore_nonrecursive", opt->repo);
>   
>   	trace2_region_enter("merge", "merge_start", opt->repo);
> -	assert(opt->ancestor != NULL);
>   	merge_check_renames_reusable(opt, result, merge_base, side1, side2);
>   	merge_start(opt, result);
>   	/*

> +test_expect_success 'fast-forward merge with --autostash, stash.index' '
> +	git reset --hard c0 &&
> +	git stash clear &&
> +	echo staged >>z && git add z &&
> +	git -c stash.index=true merge --autostash c1 2>err &&
> +	test_grep "Applied autostash." err &&
> +	test_stdout_line_count = 0 git stash list
> +'

>
> Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
> ---
>  builtin/stash.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 7a9843413b..dfea2d2c4c 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -31,7 +31,6 @@
>  #include "reflog.h"
>  #include "reflog-walk.h"
>  #include "add-interactive.h"
> -#include "oid-array.h"
>  #include "commit.h"
>  
>  #define INCLUDE_ALL_FILES 2

>
> Hi Ben
>
> On 19/09/2026 22:26, D. Ben Knoble wrote:
> > Fortunately, we can achieve 2 goals at once: avoid round-tripping to the
> > file-system (and invoking expensive subprocesses) by performing the
> > merge in-core. Since the results are never seen, we don't need to set
> > the usual branch and ancestor labels.
>
> When the merge succeeds without conflicts we use the result so it is
> seen. It would be clearer to say that "If there are conflicts we discard
> the result so ...". The rest of the commit message explains the problem
> nicely.

> > @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
> >                   oideq(&c_tree, &info->i_tree)) {
> >                       has_index = 0;
> >               } else {
> > -                     struct strbuf out = STRBUF_INIT;
> > +                     struct merge_result result = { 0 };
> >
> > -                     if (diff_tree_binary(&out, &info->w_commit)) {
> > -                             strbuf_release(&out);
> > -                             return error(_("could not generate diff %s^!."),
> > -                                          oid_to_hex(&info->w_commit));
> > -                     }
> > +                     init_basic_merge_options(&o, the_repository);
>
> This means we potentially use different diff algorithms when merging the
> index and when merging the work tree, let's use the _ui variant here
> instead.

> > +                     o.verbosity = 0;
>
> Looking at the code in merge-ort.c it appears the verbosity option was
> used by the recursive strategy but isn't used anymore so I think we
> could drop this.

> > +                     oidcpy(&index_tree, &result.tree->object.oid);
> > +                     clear_merge_options(&o);
>
> Looking at replay.c:replay_revisions() I think this should be
>
> merge_finalize(&opts, &result);

> > diff --git a/merge-ort.c b/merge-ort.c
> > index c410a5d353..f69a49d48a 100644
> > --- a/merge-ort.c
> > +++ b/merge-ort.c
> > @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
> >       trace2_region_enter("merge", "sanity checks", opt->repo);
> >       assert(opt->repo);
> >
> > -     assert(opt->branch1 && opt->branch2);
>
> This, and the hunk below, make me nervous. Normally assertions like this
> exist because the pointers are unconditionally dereferenced later on.
> Looking at merge_3way() it asserts opt->ancestor is non-NULL and
> dereferences all three labels. t3903 does not appear to have test
> coverage for the index merge failing (if it did I think we'd see a
> SIGSEV), we should probably add a test that checks the command fails
> leaving the index and work tree untouched, and verifies the message on
> stderr.
>
> Lets set some simple, fixed, ancestor and branch names in
> do_apply_stash() above.

> On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 19/09/2026 22:26, D. Ben Knoble wrote:
>>> Fortunately, we can achieve 2 goals at once: avoid round-tripping to the
>>> file-system (and invoking expensive subprocesses) by performing the
>>> merge in-core. Since the results are never seen, we don't need to set
>>> the usual branch and ancestor labels.
>>
>> When the merge succeeds without conflicts we use the result so it is
>> seen. It would be clearer to say that "If there are conflicts we discard
>> the result so ...". The rest of the commit message explains the problem
>> nicely.
> 
> Indeed. This is what I get for (unusually) dashing off the commit
> message up against the clock. Thanks!
> 
>>> @@ -669,29 +625,25 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>>>                    oideq(&c_tree, &info->i_tree)) {
>>>                        has_index = 0;
>>>                } else {
>>> -                     struct strbuf out = STRBUF_INIT;
>>> +                     struct merge_result result = { 0 };
>>>
>>> -                     if (diff_tree_binary(&out, &info->w_commit)) {
>>> -                             strbuf_release(&out);
>>> -                             return error(_("could not generate diff %s^!."),
>>> -                                          oid_to_hex(&info->w_commit));
>>> -                     }
>>> +                     init_basic_merge_options(&o, the_repository);
>>
>> This means we potentially use different diff algorithms when merging the
>> index and when merging the work tree, let's use the _ui variant here
>> instead.
> 
> Yep, you know I'd spotted that and wasn't expecting it to make a
> meaningful difference. It's an easy swap, but I thought that (like
> above, since we don't show the conflict results) the diff algorithm
> wouldn't matter too much.
> 
> Maybe it affects the actual merge-ability, though, in which case I
> agree using the same is important?

> 
>>> +                     o.verbosity = 0;
>>
>> Looking at the code in merge-ort.c it appears the verbosity option was
>> used by the recursive strategy but isn't used anymore so I think we
>> could drop this.
> 
> Intriguing. (Assuming the default "2") There's a "< 5" check in
> path_msg() that wouldn't be affected by dropping this, and a "> 2"
> check in checkout() that… also wouldn't be affected?

> But it might matter if something is setting the verbosity elsewhere
> (config, GIT_MERGE_VERBOSITY), and I think we really want this merge
> to be quiet? I seem to remember reading commits in this area quieting
> "git reset" and so on to keep the noise down.
> 
> So I'm inclined to leave it for now, especially in case it later does get used.

> 
>>> +                     oidcpy(&index_tree, &result.tree->object.oid);
>>> +                     clear_merge_options(&o);
>>
>> Looking at replay.c:replay_revisions() I think this should be
>>
>> merge_finalize(&opts, &result);
> 
> Hm, possibly. It does look like that does more with the "result,"
> which is probably needed.

> On one hand, I thought it could be important not to reuse that struct
> between merges. But if we do use the "ui" init, it might be ok?
> replay_revisions() does use the same struct between calls to
> merge_incore_nonrecursive().
> 
> Oh, but one other thing: we unconditionally reinit the merge options
> later on in do_apply_stash(). We could conditionally initialize there
> ("if (has_index)"), I suppose?

> 
>>> diff --git a/merge-ort.c b/merge-ort.c
>>> index c410a5d353..f69a49d48a 100644
>>> --- a/merge-ort.c
>>> +++ b/merge-ort.c
>>> @@ -5035,8 +5035,6 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
>>>        trace2_region_enter("merge", "sanity checks", opt->repo);
>>>        assert(opt->repo);
>>>
>>> -     assert(opt->branch1 && opt->branch2);
>>
>> This, and the hunk below, make me nervous. Normally assertions like this
>> exist because the pointers are unconditionally dereferenced later on.
>> Looking at merge_3way() it asserts opt->ancestor is non-NULL and
>> dereferences all three labels. t3903 does not appear to have test
>> coverage for the index merge failing (if it did I think we'd see a
>> SIGSEV), we should probably add a test that checks the command fails
>> leaving the index and work tree untouched, and verifies the message on
>> stderr.
>>
>> Lets set some simple, fixed, ancestor and branch names in
>> do_apply_stash() above.
> 
> Funny, I was getting aborts before removing the asserts because I
> hadn't set the labels, aha. Looks like we've come back around to
> keeping the labels.

>
> Hi Ben
>
> On 22/09/2026 13:43, D. Ben Knoble wrote:
> I think there are wierd cases where one diff algorithm results in
> conflicts and another doesn't because they generate different (but
> equally valid) diffs so allowing the user to tweak the algorithm we use
> via init_ui_merge_options() is probably a good idea.

> >>> +                     o.verbosity = 0;
> >>
> >> Looking at the code in merge-ort.c it appears the verbosity option was
> >> used by the recursive strategy but isn't used anymore so I think we
> >> could drop this.
> >
> > Intriguing. (Assuming the default "2") There's a "< 5" check in
> > path_msg() that wouldn't be affected by dropping this, and a "> 2"
> > check in checkout() that… also wouldn't be affected?
>
> The former is not affected because we're cherry-picking so never have an
> inner merge from merging multiple merge bases. The latter is not
> affected because we don't checkout the result!

> > But it might matter if something is setting the verbosity elsewhere
> > (config, GIT_MERGE_VERBOSITY), and I think we really want this merge
> > to be quiet? I seem to remember reading commits in this area quieting
> > "git reset" and so on to keep the noise down.
> >
> > So I'm inclined to leave it for now, especially in case it later does get used.
>
> merge ort does not print anything - it just adds messages to an strmap
> in struct merge_result() which we ignore here. I guess setting it to
> zero might avoid a little work generating the messages.

> >>> +                     oidcpy(&index_tree, &result.tree->object.oid);
> >>> +                     clear_merge_options(&o);
> >>
> >> Looking at replay.c:replay_revisions() I think this should be
> >>
> >> merge_finalize(&opts, &result);
> >
> > Hm, possibly. It does look like that does more with the "result,"
> > which is probably needed.
>
> Oh, we definitely want to free the strmap in the merge result.
>
>  > But it doesn't actually clear the merge options.
>
> Isn't that because there are no allocations in that struct? (obuf is
> unused - it looks like we could clean up the struct by removing the
> members that were used by merge-recursive but are ignored by merge-ort)

> > On one hand, I thought it could be important not to reuse that struct
> > between merges. But if we do use the "ui" init, it might be ok?
> > replay_revisions() does use the same struct between calls to
> > merge_incore_nonrecursive().
> >
> > Oh, but one other thing: we unconditionally reinit the merge options
> > later on in do_apply_stash(). We could conditionally initialize there
> > ("if (has_index)"), I suppose?
>
> I'd just move the call to init_ui_merge_options() above "if (index)". As
> far as I know it should be fine to reuse it - any state is stored in the
> result

> > Funny, I was getting aborts before removing the asserts because I
> > hadn't set the labels, aha. Looks like we've come back around to
> > keeping the labels.
>
> Sorry for that detour

> > I'll probably keep a similar structure as the
> > working tree merge uses, I think.
>
> I'd use fixed names and not bother with all the conditionals around the
> label text to keep it simple.

> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>   		    oideq(&c_tree, &info->i_tree)) {
>   			has_index = 0;
>   		} else {
> -			struct strbuf out = STRBUF_INIT;
> +			struct merge_result result = { 0 };
>   
> -			if (diff_tree_binary(&out, &info->w_commit)) {
> -				strbuf_release(&out);
> -				return error(_("could not generate diff %s^!."),
> -					     oid_to_hex(&info->w_commit));
> -			}
> +			o.branch1 = "Upstream index";

> +			o.branch2 = "Stashed index changes";
> +			o.ancestor = "Stash base";
>   
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)
> +			o.verbosity = 0;
> +
> +			head = lookup_tree(o.repo, &c_tree);
> +			merge = lookup_tree(o.repo, &info->i_tree);
> +			merge_base = lookup_tree(o.repo, &info->b_tree);
> +
> +			merge_incore_nonrecursive(&o, head, merge, merge_base,
> +						  &result);
> +
> +			if (!result.clean)
>   				return error(_("conflicts in index. "
>   					       "Try without --index."));

> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
> -			if (write_index_as_tree(&index_tree, the_repository->index,
> -						repo_get_index_file(the_repository), 0, NULL))
> -				return error(_("could not save index tree"));
> -
> -			reset_head();
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
> +			oidcpy(&index_tree, &result.tree->object.oid);
> +			merge_finalize(&o, &result);
>   		}
>   	}
>   
> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
> index 64fe21717d..8f6109fb91 100755
> --- a/t/t7600-merge.sh
> +++ b/t/t7600-merge.sh
> @@ -801,6 +801,15 @@ verify_no_mergehead () {
>   	test_cmp result.1-5 file
>   '
>   
> +test_expect_success 'fast-forward merge with --autostash, stash.index' '
> +	git reset --hard c0 &&
> +	git stash clear &&
> +	echo staged >>z && git add z &&
> +	git -c stash.index=true merge --autostash c1 2>err &&
> +	test_grep "Applied autostash." err &&
> +	test_stdout_line_count = 0 git stash list
> +'
> +
>   test_expect_success 'failed fast-forward merge with --autostash' '
>   	git reset --hard c0 &&
>   	git merge-file file file.orig file.5 &&

> The next commit will refactor index handling for applied stashes, so
> let's make sure we cover conflicted index merging, too.
> 
> Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
> ---
>   t/t3903-stash.sh | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 721158606f..3958ab3c8d 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -374,6 +374,24 @@ setup_stash() {
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'stash apply --index leaves everything untouched on failure' '
> +	git reset --hard &&
> +	echo test >other-file &&
> +	git add other-file &&
> +	git stash &&
> +	echo unrelated >file &&
> +	echo unrelated >another-file &&
> +	git add another-file &&
> +	git diff-files >expect &&

> +	test_must_fail git stash apply --index 2>err &&
> +	test_grep "conflicts in index. Try without --index" err &&
> +	git diff-files >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_expect_success 'stash -k' '
>   	echo bar3 >file &&
>   	echo bar4 >file2 &&

> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>  		    oideq(&c_tree, &info->i_tree)) {
>  			has_index = 0;
>  		} else {
> -			struct strbuf out = STRBUF_INIT;
> +			struct merge_result result = { 0 };
>  
> -			if (diff_tree_binary(&out, &info->w_commit)) {
> -				strbuf_release(&out);
> -				return error(_("could not generate diff %s^!."),
> -					     oid_to_hex(&info->w_commit));
> -			}
> +			o.branch1 = "Upstream index";
> +			o.branch2 = "Stashed index changes";
> +			o.ancestor = "Stash base";
>  
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)

> +			o.verbosity = 0;
> +
> +			head = lookup_tree(o.repo, &c_tree);
> +			merge = lookup_tree(o.repo, &info->i_tree);
> +			merge_base = lookup_tree(o.repo, &info->b_tree);
> +
> +			merge_incore_nonrecursive(&o, head, merge, merge_base,
> +						  &result);

>
> Hi Ben
>
> I've spotted a memory leak that I missed last time, apart from that this
> looks good.
>
> On 23/09/2026 13:58, D. Ben Knoble wrote:
> > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
> >                   oideq(&c_tree, &info->i_tree)) {
> >                       has_index = 0;
> >               } else {
> > -                     struct strbuf out = STRBUF_INIT;
> > +                     struct merge_result result = { 0 };
> >
> > -                     if (diff_tree_binary(&out, &info->w_commit)) {
> > -                             strbuf_release(&out);
> > -                             return error(_("could not generate diff %s^!."),
> > -                                          oid_to_hex(&info->w_commit));
> > -                     }
> > +                     o.branch1 = "Upstream index";
>
> This is the current index, calling it "upstream" is a bit confusing to
> me but that's not worth a re-roll on its own.

> > +                     o.branch2 = "Stashed index changes";
> > +                     o.ancestor = "Stash base";
> >
> > -                     ret = apply_cached(&out);
> > -                     strbuf_release(&out);
> > -                     if (ret)
> > +                     o.verbosity = 0;
> > +
> > +                     head = lookup_tree(o.repo, &c_tree);
> > +                     merge = lookup_tree(o.repo, &info->i_tree);
> > +                     merge_base = lookup_tree(o.repo, &info->b_tree);
> > +
> > +                     merge_incore_nonrecursive(&o, head, merge, merge_base,
> > +                                               &result);
> > +
> > +                     if (!result.clean)
> >                               return error(_("conflicts in index. "
> >                                              "Try without --index."));
>
> Sorry, I missed this last time, but we should finalize the merge before
> returning to ensure the allocations in result are freed.

>
> Ahh, or perhaps the trees are indeed given in a wrong order, but not
> in a random wrong order.  merge_ort_nonrecursive(), which is *not*
> the function you are using, takes head, merge, and merge_base in
> this order, and that order matches what you wrote.
>
> Perhaps the true culprit in this confusion is that the order in
> which merge_ort_nonrecursive() takes its three trees (head, merge,
> and common) and the order in which merge_incore_nonrecursive() takes
> its trees (merge_base, side1, and side2) are different, and if we
> fix them to match, it would make it easier to work with?

>
> Hi Ben
>
> On 23/09/2026 13:58, D. Ben Knoble wrote:
> > The next commit will refactor index handling for applied stashes, so
> > let's make sure we cover conflicted index merging, too.
> >
> > Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
> > ---
> >   t/t3903-stash.sh | 18 ++++++++++++++++++
> >   1 file changed, 18 insertions(+)
> >
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 721158606f..3958ab3c8d 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -374,6 +374,24 @@ setup_stash() {
> >       test_cmp expect actual
> >   '
> >
> > +test_expect_success 'stash apply --index leaves everything untouched on failure' '
> > +     git reset --hard &&
> > +     echo test >other-file &&
> > +     git add other-file &&
> > +     git stash &&
> > +     echo unrelated >file &&
> > +     echo unrelated >another-file &&
> > +     git add another-file &&
> > +     git diff-files >expect &&
>
> diff-files shows the worktree blobs as null object ids, so comparing
> this before and after stashing only tells us that the same set of files
> have unstaged changes, not that the unstaged changes are the same.
> Adding "-p" would check the worktree files are unchanged.

> > +     echo conflict >other-file &&
> > +     git add other-file &&
>
> I wonder if we should to add "git diff-index --cached HEAD
>  >expect-index" here so we can check the index is unchanged as well. For
> the paths that have unstaged changes we're already checking the index
> object ids via "diff-files", but I think in theory it would be possible
> to have an identical change in the index and worktree that is not picked
> up by that.

> 
> So overall, I'm thinking
> 
> - (old) diff-files only shows file is changed
> - diff-files -p shows us changes for file, better (and won't show the
> other 2 files unless they've become unstaged)
> - diff-index --cached HEAD helps us check all the index changes

> On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Sorry, I missed this last time, but we should finalize the merge before
>> returning to ensure the allocations in result are freed.
> 
> Yeah, I think CI caught this:
> https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31
> 
> But I'm not sure I could have understood what it was telling me
> without your hint, thanks!

> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> 
> Ahh, or perhaps the trees are indeed given in a wrong order, but not
> in a random wrong order.  merge_ort_nonrecursive(), which is *not*
> the function you are using, takes head, merge, and merge_base in
> this order, and that order matches what you wrote.

> Perhaps the true culprit in this confusion is that the order in
> which merge_ort_nonrecursive() takes its three trees (head, merge,
> and common) and the order in which merge_incore_nonrecursive() takes
> its trees (merge_base, side1, and side2) are different, and if we
> fix them to match, it would make it easier to work with?

> The new test in the attached patch will fail with this step but if
> we revert the changes to builtin/stash.c in this step, it passes.
> 
>   t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git c/t/t3903-stash.sh w/t/t3903-stash.sh
> index 3958ab3c8d..0a87e62b11 100755
> --- c/t/t3903-stash.sh
> +++ w/t/t3903-stash.sh
> @@ -374,6 +374,38 @@ test_expect_success 'stash apply -q --index refreshes the index' '
>   	test_cmp expect actual
>   '
>   
> +
> +test_expect_success 'stash apply --index does not revert unrelated upstream index changes' '
> +	test_when_finished "rm -fr playpen" &&
> +	mkdir playpen &&
> +	(
> +		cd playpen &&
> +		git init &&
> +		echo "base1" >file1 &&
> +		echo "base2" >file2 &&
> +		git add file1 file2 &&
> +		git commit -m "initial base" &&
> +
> +		# Make a staged change to file1 and stash it
> +		echo "staged1" >file1 &&
> +		git add file1 &&
> +		git stash &&
> +
> +		# Upstream advances by modifying unrelated file2
> +		echo "upstream2" >file2 &&
> +		git add file2 &&
> +		git commit -m "upstream change to file2" &&
> +
> +		# Apply the stash with --index
> +		git stash apply --index &&
> +
> +		# Verify working tree and index state
> +		test "$(git show :file1)" = "staged1" &&
> +		test "$(git show :file2)" = "upstream2" &&
> +		test "$(git show HEAD:file2)" = "upstream2"
> +	)
> +'
> +
>   test_expect_success 'stash apply --index leaves everything untouched on failure' '
>   	git reset --hard &&
>   	echo test >other-file &&

> On 25/09/2026 13:55, D. Ben Knoble wrote:
> > On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>
> >> Sorry, I missed this last time, but we should finalize the merge before
> >> returning to ensure the allocations in result are freed.
> >
> > Yeah, I think CI caught this:
> > https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:5:31
> >
> > But I'm not sure I could have understood what it was telling me
> > without your hint, thanks!
>
> Yes, that output is terrible - to see the leaks you have to scroll to
> line 28282 of "print test failures" which is ridiculous. See
> https://github.com/benknoble/git/actions/runs/36033463504/job/107747745741#step:10:28282

> > Perhaps the true culprit in this confusion is that the order in
> > which merge_ort_nonrecursive() takes its three trees (head, merge,
> > and common) and the order in which merge_incore_nonrecursive() takes
> > its trees (merge_base, side1, and side2) are different, and if we
> > fix them to match, it would make it easier to work with?
>
> I think it is definitely worth fixing them to take the trees in the same
> order. My preference would be "base", "stage1", "stage2" but so long as
> they match each other I dont object to "stage1", "stage2", "base".
>
> Thanks
>
> Phillip

> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Ahh, or perhaps the trees are indeed given in a wrong order, but not
>> in a random wrong order.  merge_ort_nonrecursive(), which is *not*
>> the function you are using, takes head, merge, and merge_base in
>> this order, and that order matches what you wrote.
>>
>> Perhaps the true culprit in this confusion is that the order in
>> which merge_ort_nonrecursive() takes its three trees (head, merge,
>> and common) and the order in which merge_incore_nonrecursive() takes
>> its trees (merge_base, side1, and side2) are different, and if we
>> fix them to match, it would make it easier to work with?
>
> Indeed, the confusion is that simple ;) Shamefully, we don't have
> enough test coverage to catch that regression, so I'm very glad indeed
> you spotted it.
>
>> The new test in the attached patch will fail with this step but if
>> we revert the changes to builtin/stash.c in this step, it passes.
>
> Any objection to me adding this test as a preparatory patch? There's
> no sign-off, so I don't want to mess up the DCO here.

>> I think it is definitely worth fixing them to take the trees in the same
>> order. My preference would be "base", "stage1", "stage2" but so long as
>> they match each other I dont object to "stage1", "stage2", "base".
>>
>> Thanks
>>
>> Phillip
>
> FWIW, I concur with changing them (and Phillip's preference of order),
> but I'll elect to leave that out of scope for this series.

> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> 
>> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Ahh, or perhaps the trees are indeed given in a wrong order, but not
>>> in a random wrong order.  merge_ort_nonrecursive(), which is *not*
>>> the function you are using, takes head, merge, and merge_base in
>>> this order, and that order matches what you wrote.
>>>
>>> Perhaps the true culprit in this confusion is that the order in
>>> which merge_ort_nonrecursive() takes its three trees (head, merge,
>>> and common) and the order in which merge_incore_nonrecursive() takes
>>> its trees (merge_base, side1, and side2) are different, and if we
>>> fix them to match, it would make it easier to work with?
>>
>> Indeed, the confusion is that simple ;) Shamefully, we don't have
>> enough test coverage to catch that regression, so I'm very glad indeed
>> you spotted it.
>>
>>> The new test in the attached patch will fail with this step but if
>>> we revert the changes to builtin/stash.c in this step, it passes.
>>
>> Any objection to me adding this test as a preparatory patch? There's
>> no sign-off, so I don't want to mess up the DCO here.
> 
> It was written merely as an illustration and is not something I am
> proud of.  For example, creating a totally new playpen repository
> only for a single piece of test and remove the entire thing when the
> single test piece is done was done only to make sure the existing
> test that come later can never be affected.  Also the test only uses
> the most trivial case (a file is added in the stashed change, nobody
> else involved in the stash application has touched the file so there
> is nothing to "merge" in the file).  It was enough to demonstrate
> that the order of arguments given to the function was wrong, but
> we wouldn't catch problems in content-level merge with such a test.
> 
> So, I wouldn't mind if you reused that as one in a series of tests,
> but I'd prefer to see those who are move invested in the topic to
> come up with a bit more realistic scenario.

>
> On 25/09/2026 17:24, Junio C Hamano wrote:
> > "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> >
> >> On Thu, Sep 24, 2026 at 5:59 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>>
> >>> Ahh, or perhaps the trees are indeed given in a wrong order, but not
> >>> in a random wrong order.  merge_ort_nonrecursive(), which is *not*
> >>> the function you are using, takes head, merge, and merge_base in
> >>> this order, and that order matches what you wrote.
> >>>
> >>> Perhaps the true culprit in this confusion is that the order in
> >>> which merge_ort_nonrecursive() takes its three trees (head, merge,
> >>> and common) and the order in which merge_incore_nonrecursive() takes
> >>> its trees (merge_base, side1, and side2) are different, and if we
> >>> fix them to match, it would make it easier to work with?
> >>
> >> Indeed, the confusion is that simple ;) Shamefully, we don't have
> >> enough test coverage to catch that regression, so I'm very glad indeed
> >> you spotted it.
> >>
> >>> The new test in the attached patch will fail with this step but if
> >>> we revert the changes to builtin/stash.c in this step, it passes.
> >>
> >> Any objection to me adding this test as a preparatory patch? There's
> >> no sign-off, so I don't want to mess up the DCO here.
> >
> > It was written merely as an illustration and is not something I am
> > proud of.  For example, creating a totally new playpen repository
> > only for a single piece of test and remove the entire thing when the
> > single test piece is done was done only to make sure the existing
> > test that come later can never be affected.  Also the test only uses
> > the most trivial case (a file is added in the stashed change, nobody
> > else involved in the stash application has touched the file so there
> > is nothing to "merge" in the file).  It was enough to demonstrate
> > that the order of arguments given to the function was wrong, but
> > we wouldn't catch problems in content-level merge with such a test.
> >
> > So, I wouldn't mind if you reused that as one in a series of tests,
> > but I'd prefer to see those who are move invested in the topic to
> > come up with a bit more realistic scenario.
>
> Maybe something like the test below (which I admit I haven't actually
> tested). That checks we merge the file contents and puts the changes in
> the file close enough together so that the old code would fail and has
> different contents for the three merged blobs.
>
> test_write_lines A B C >file &&
> git commit -m xxx file &&
> test_write_lines A B staged >file &&
> git add file &&
> test_write_lines A B unstaged >file &&
> git stash &&
> test_write_lines committed B C >file &&
> git commit -m yyy file &&
> git stash pop --index &&
> git show :file >actual &&
> test_write_lines committed B staged >expect &&
> text_cmp expect actual &&

>
> On 25/09/2026 16:45, Phillip Wood wrote:
> >
> > I think that sounds reasonable, we can delete the index lines from the
> > patch output with sed to make it easier to compare them.
>
> I just opened the test file and realized it has a diff_cmp() function to
> compare diffs ignoring the index lines
>
> Thanks
>
> Phillip

>
> Hi all,
>
> This small patch series fixes a bug reported by Eli Barzilay in the
> interaction between autostashing, staged index entries, and
> stash.index=true.
>
> The first patch is an incidental cleanup, and the second re-arranges one
> line to make the change easier. The third and fourth add missing test
> coverage (which catch breakages from prior incorrect rounds of this
> series), while the last holds the interesting bits.
>
> Changes in v3:

> @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
>  		    oideq(&c_tree, &info->i_tree)) {
>  			has_index = 0;
>  		} else {
> -			struct strbuf out = STRBUF_INIT;
> +			struct merge_result result = { 0 };
>  
> -			if (diff_tree_binary(&out, &info->w_commit)) {
> -				strbuf_release(&out);
> -				return error(_("could not generate diff %s^!."),
> -					     oid_to_hex(&info->w_commit));
> -			}
> +			o.branch1 = "Current index";
> +			o.branch2 = "Stashed index changes";
> +			o.ancestor = "Stash base";
>  
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)
> +			o.verbosity = 0;

> +			head = lookup_tree(o.repo, &c_tree);
> +			merge = lookup_tree(o.repo, &info->i_tree);
> +			merge_base = lookup_tree(o.repo, &info->b_tree);
> +
> +			merge_incore_nonrecursive(&o, merge_base, head, merge,
> +						  &result);
> +
> +			oidcpy(&index_tree, &result.tree->object.oid);
> +			merge_finalize(&o, &result);

> +
> +			if (!result.clean)
>  				return error(_("conflicts in index. "
>  					       "Try without --index."));
> -
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
> -			if (write_index_as_tree(&index_tree, the_repository->index,
> -						repo_get_index_file(the_repository), 0, NULL))
> -				return error(_("could not save index tree"));
> -
> -			reset_head();
> -			discard_index(the_repository->index);
> -			repo_read_index(the_repository);
>  		}
>  	}

> Hi all,
>
> This small patch series fixes a bug reported by Eli Barzilay in the
> interaction between autostashing, staged index entries, and
> stash.index=true.
>
> The first patch is an incidental cleanup, and the second re-arranges one
> line to make the change easier. The third and fourth add missing test
> coverage (which catch breakages from prior incorrect rounds of this
> series), while the last holds the interesting bits.

> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> 
>> Hi all,
>>
>> This small patch series fixes a bug reported by Eli Barzilay in the
>> interaction between autostashing, staged index entries, and
>> stash.index=true.
>>
>> The first patch is an incidental cleanup, and the second re-arranges one
>> line to make the change easier. The third and fourth add missing test
>> coverage (which catch breakages from prior incorrect rounds of this
>> series), while the last holds the interesting bits.
> 
> I may have reported this on the previous round, too, but 'seen'
> seems to break t5520 when this topic is merged.  I'll eject the
> topic from my tree for now in the meantime.

>
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> > @@ -671,29 +627,27 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
> >                   oideq(&c_tree, &info->i_tree)) {
> >                       has_index = 0;
> >               } else {
> > -                     struct strbuf out = STRBUF_INIT;
> > +                     struct merge_result result = { 0 };
> >
> > -                     if (diff_tree_binary(&out, &info->w_commit)) {
> > -                             strbuf_release(&out);
> > -                             return error(_("could not generate diff %s^!."),
> > -                                          oid_to_hex(&info->w_commit));
> > -                     }
> > +                     o.branch1 = "Current index";
> > +                     o.branch2 = "Stashed index changes";
> > +                     o.ancestor = "Stash base";
> >
> > -                     ret = apply_cached(&out);
> > -                     strbuf_release(&out);
> > -                     if (ret)
> > +                     o.verbosity = 0;
>
> We realize that 'o' is a struct merge_options defined on the stack
> for this function, initialized with init_ui_merge_options() fairly
> early on.  It would have initialized '.verbosity' to the default
> verbosity, the merge.verbosity configuration variable, or the
> GIT_MERGE_VERBOSITY environment variable.
>
> You drop the verbosity here, presumably because you want to match
> the previous implementation 'diff-tree | apply --cached' (which I
> guess was fairly quiet, but I do not use 'stash pop --index'
> myself).

> > +
> > +                     if (!result.clean)
> >                               return error(_("conflicts in index. "
> >                                              "Try without --index."));
> > -
> > -                     discard_index(the_repository->index);
> > -                     repo_read_index(the_repository);
> > -                     if (write_index_as_tree(&index_tree, the_repository->index,
> > -                                             repo_get_index_file(the_repository), 0, NULL))
> > -                             return error(_("could not save index tree"));
> > -
> > -                     reset_head();
> > -                     discard_index(the_repository->index);
> > -                     repo_read_index(the_repository);
> >               }
> >       }
>
>
> But the thing is, this is not the end of the function, or the last
> call to the merge machinery using 'o'.  We then use the same 'o' to
> drive another three-way merge.  Yet nobody restores '.verbosity'
> that was unconditionally turned off above for that second merge.

>
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> > +                     merge_incore_nonrecursive(&o, merge_base, head, merge,
> > +                                               &result);
> > +
> > +                     oidcpy(&index_tree, &result.tree->object.oid);
>
> This is risky, isn't it?
>
> If there were catastrophic failure (e.g., missing object that were
> involved in the merge), merge_incore_nonrecursive() may stuff -1 to
> result.clean and return without populating result.tree, and when
> that happens, result.tree->object.oid would be dereferencing NULL.

>
> On 27/09/2026 20:21, Junio C Hamano wrote:
> > "D. Ben Knoble" <ben.knoble@gmail.com> writes:
> >
> >> Hi all,
> >>
> >> This small patch series fixes a bug reported by Eli Barzilay in the
> >> interaction between autostashing, staged index entries, and
> >> stash.index=true.
> >>
> >> The first patch is an incidental cleanup, and the second re-arranges one
> >> line to make the change easier. The third and fourth add missing test
> >> coverage (which catch breakages from prior incorrect rounds of this
> >> series), while the last holds the interesting bits.
> >
> > I may have reported this on the previous round, too, but 'seen'
> > seems to break t5520 when this topic is merged.  I'll eject the
> > topic from my tree for now in the meantime.

> There seems to be something
> funny going on with pull's fork-point detection. If I add GIT_TRACE=1 to
> "git pull --rebase" then on 'seen' I see
>
> trace: built-in: git rebase --no-autostash --onto
> ae9857430e281d178a3755aecfc5e29c46a02306
> f29aa667ce68e4d514557081ca7f54b12e108922
>
> but with this series I see
>
> trace: built-in: git rebase --no-autostash --onto
> ae9857430e281d178a3755aecfc5e29c46a02306
> ae9857430e281d178a3755aecfc5e29c46a02306
>
> so the upstream commit has changed. The previous test also checks the
> fork-point behavior and the failing test just runs "git reset --hard" at
> the start rather than re-creating the reflogs which seems a bit iffy to
> me but I've no idea why this series causes it to fail. I tried a merge
> of 'master' and 'seen' just in case the failure was caused by the base
> I'd used for this series but that passes.

>
> Just leaving some breadcrumb notes…
>
> On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
> >
> > On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > >
> > > On 27/09/2026 20:21, Junio C Hamano wrote:
>
> From my local version of the branch, the following script points at
> 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27):

> On Mon, Sep 28, 2026 at 8:33 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>>
>> Just leaving some breadcrumb notes…
>>
>> On Mon, Sep 28, 2026 at 8:05 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>>>
>>> On Mon, Sep 28, 2026 at 5:50 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>>
>>>> On 27/09/2026 20:21, Junio C Hamano wrote:
>>
>>  From my local version of the branch, the following script points at
>> 4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27):
> 
> And within that topic, bisect points to 2d1fa0323f (rebase,
> cherry-pick, revert: run auto maintenance when done, 2026-09-17)

> in
> t5220.69 as Phillip said.
> 
> expecting success of 5520.69 '--rebase -f with rebased upstream':
> test_when_finished "test_might_fail git rebase --abort" &&
> git reset --hard to-rebase-orig &&
> git pull --rebase -f me copy &&
> echo "conflicting modification" >expect &&
> test_cmp expect file &&
> echo file >expect &&
> test_cmp expect file2
> 
> ++ test_when_finished 'test_might_fail git rebase --abort'
> ++ test 0 = 0
> ++ test_cleanup=$'{ test_might_fail git rebase --abort\n\t\t} || eval_ret=$?; :'
> ++ git reset --hard to-rebase-orig
> HEAD is now at cb9bf26 to-rebase
> ++ git pull --rebase -f me copy
>  From .
>   * branch            copy       -> FETCH_HEAD
> Rebasing (1/4)
> Auto-merging file
> CONFLICT (content): Merge conflict in file
> error: could not apply f29aa66... file
> hint: Resolve all conflicts manually, mark them as resolved with
> hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
> hint: You can instead skip this commit: run "git rebase --skip".
> hint: To abort and get back to the state before "git rebase", run "git
> rebase --abort".
> hint: Disable this message with "git config set advice.mergeConflict false"
> Could not apply f29aa66... # file
> error: last command exited with $?=1
> 
> 

> Oh, when I was thinking about this over lunch I did wonder if that might
> be the culprit. Previously we didn't run "git maintenance --auto" after
> a rebase with the 'merge' backend but with that topic we do, and because
> we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'
> gets triggered it will expire the reflog entries that 'git pull
> --rebase' relies on. As you suggested in another mail, I assume this

> topic has changed something in one of the '--autostash' tests that come
> before the failing test triggers which the new behavior. What that
> something is I'm not sure; off the top of my head I'd expect the number
> of reflog entries in HEAD to be the same but maybe I'm missing
> something. Adding

> git config maintenance.reflog-expire.auto 0
>
> to the 'setup' test fixes the test failure, but it would be good to try
> and understand why this topic triggers the reflog to be expired in case
> there is something nasty happening that we've not thought of.

> On Mon, Sep 28, 2026 at 5:40 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>>
>> > +                     merge_incore_nonrecursive(&o, merge_base, head, merge,
>> > +                                               &result);
>> > +
>> > +                     oidcpy(&index_tree, &result.tree->object.oid);
>>
>> This is risky, isn't it?
>>
>> If there were catastrophic failure (e.g., missing object that were
>> involved in the merge), merge_incore_nonrecursive() may stuff -1 to
>> result.clean and return without populating result.tree, and when
>> that happens, result.tree->object.oid would be dereferencing NULL.
>
> Indeed… unfortunate. Thanks for spotting.

>
> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > Oh, when I was thinking about this over lunch I did wonder if that might
> > be the culprit. Previously we didn't run "git maintenance --auto" after
> > a rebase with the 'merge' backend but with that topic we do, and because
> > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'
> > gets triggered it will expire the reflog entries that 'git pull
> > --rebase' relies on. As you suggested in another mail, I assume this

> and test 69 needs the entry that
> test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the
> reflog empty, "merge-base --fork-point" falls back to the ref itself,
> ae98574 is no ancestor of to-rebase, and pull hands the merge head to
> rebase as the upstream. That is your "--onto ae98... ae98...", and the
> four commits from copy-orig up come back, the first of them
> conflicting with "conflict".
>
> > topic has changed something in one of the '--autostash' tests that come
> > before the failing test triggers which the new behavior. What that
> > something is I'm not sure; off the top of my head I'd expect the number
> > of reflog entries in HEAD to be the same but maybe I'm missing
> > something. Adding
>
> It is eight entries fewer, and they come from the failed merges, not
> from the autostash tests. "git merge" restores a dirty tree with
> "stash apply --index --quiet", and until Ben's series that spawned
> "git reset --quiet --refresh", which writes "reset: moving to HEAD"
> to the reflog. That happens eight times in t5520 before test 68.
>
> Auto maintenance expires reflogs once HEAD's reflog holds a hundred
> entries that the policy would remove, the default of
> maintenance.reflog-expire.auto, and after the first test_tick that is
> every entry. Which run crosses the hundred depends on how many entries
> and maintenance runs came before it. On 'seen' the expiry lands on
> "git commit -m conflict" in test 68, before the fetch writes the entry.
> Eight entries fewer move the crossing past that commit, and the
> maintenance run my topic adds at the end of the rebase in test 68 is
> the next one: after the fetch, before test 69 reads the reflog. Either
> change alone leaves it somewhere harmless, and nothing else is going
> on. The expiry is the usual 90 days applied to entries dated 2005, and
> the only new thing is one more maintenance run per rebase, the same
> one "git commit" and "git fetch" run.

> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> topic has changed something in one of the '--autostash' tests that come
>> before the failing test triggers which the new behavior. What that
>> something is I'm not sure; off the top of my head I'd expect the number
>> of reflog entries in HEAD to be the same but maybe I'm missing
>> something. Adding
> 
> It is eight entries fewer, and they come from the failed merges, not
> from the autostash tests. "git merge" restores a dirty tree with
> "stash apply --index --quiet",

> Auto maintenance expires reflogs once HEAD's reflog holds a hundred
> entries that the policy would remove, the default of
> maintenance.reflog-expire.auto, and after the first test_tick that is
> every entry. Which run crosses the hundred depends on how many entries
> and maintenance runs came before it. On 'seen' the expiry lands on
> "git commit -m conflict" in test 68, before the fetch writes the entry.
> Eight entries fewer move the crossing past that commit, and the
> maintenance run my topic adds at the end of the rebase in test 68 is
> the next one: after the fetch, before test 69 reads the reflog. Either
> change alone leaves it somewhere harmless, and nothing else is going
> on. The expiry is the usual 90 days applied to entries dated 2005, and
> the only new thing is one more maintenance run per rebase, the same
> one "git commit" and "git fetch" run.
> 
>> git config maintenance.reflog-expire.auto 0
>>
>> to the 'setup' test fixes the test failure, but it would be good to try
>> and understand why this topic triggers the reflog to be expired in case
>> there is something nasty happening that we've not thought of.
> 
> I'd pin the expiry itself instead, as ea7d894f44 (t34xx: don't expire
> reflogs where it matters, 2026-02-24) did for the rebase tests:
> 
> git config set gc.reflogExpire never &&
> git config set gc.reflogExpireUnreachable never &&
> 
> That covers a "git gc" as well, which expires reflogs on its own. With
> it, 'seen' plus Ben's series passes t5520 here and no expiry runs
> during the script at all. I sent it as a patch on master:
> <pull.2243.git.1790606282769.gitgitgadget@gmail.com>
> 
> FWIW, any script that reads a reflog after a hundred HEAD updates can
> fall into the same hole. I have not looked further than t5520.
> 
> Thomas

> Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
> ---
>   t/t3903-stash.sh | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 721158606f..9bc99fa252 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -374,6 +374,27 @@ setup_stash() {
>   	test_cmp expect actual
>   '
>   
> +# the later "stash -k" test is not expecting us to muck with file so much, so
> +# reset when finished
> +test_expect_success 'stash apply --index merges the correct trees' '
> +	head=$(git rev-parse HEAD) &&
> +	test_when_finished "git reset --hard $head" &&
> +	test_write_lines A B C >file &&
> +	git commit -m setup file &&
> +	test_write_lines A B staged >file &&
> +	git add file &&
> +	test_write_lines A B unstaged >file &&
> +	git stash &&
> +	test_write_lines committed B C >file &&
> +	git commit -m to-be-merged file &&
> +	git stash pop --index &&
> +	git show :file >actual &&
> +	test_write_lines committed B staged >expect &&
> +	test_cmp expect actual &&
> +	test_write_lines committed B unstaged >expect &&
> +	test_cmp expect file
> +'
> +
>   test_expect_success 'stash -k' '
>   	echo bar3 >file &&
>   	echo bar4 >file2 &&

>
> Hi Ben
>
> On 26/09/2026 13:16, D. Ben Knoble wrote:
> > A future commit will refactor index handling for applied stashes, and we
> > need to take care to get the order of trees right when merging. Add a
> > test that covers this case.
>
> The test looks good, but without the changes in patch 5 it fails and so
> adding it here breaks running "git bisect" on this series. I'd squash
> this into the final patch

> On Mon, Sep 28, 2026 at 11:44 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> The test looks good, but without the changes in patch 5 it fails and so
>> adding it here breaks running "git bisect" on this series. I'd squash
>> this into the final patch
> 
> Interesting. I thought I checked that the test passed sans patch 5,
> but I'll double check. I can't think of a reason it wouldn't offhand,
> but my thoughts on patch 5's changes have become a bit scattered.

>
> Let me see if I understand correctly…
>
> On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem <mail@thomasbachem.com> wrote:
> >
> > On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > > Oh, when I was thinking about this over lunch I did wonder if that might
> > > be the culprit. Previously we didn't run "git maintenance --auto" after
> > > a rebase with the 'merge' backend but with that topic we do, and because
> > > we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'
> > > gets triggered it will expire the reflog entries that 'git pull
> > > --rebase' relies on. As you suggested in another mail, I assume this
>
> > "git pull --rebase" computes the fork point before it fetches, from
> > the reflog of refs/remotes/me/copy,
>
> This is described by the manual for git-rebase under --fork-point,
> which is on unless we have an <upstream> or --keep-base (modulo
> config). Put a pin in this.

> But here's what I can't figure out, returning to that pin from
> earlier: I was a bit surprised to see mention of rebase reading
> reflogs! When I remembered --fork-point, I was even more curious (but
> at least it's obvious that rebase will read the reflogs in some
> scenarios).
>
> What confuses me is that builtin/pull.c:run_rebase() sure looks like
> it provides an <upstream> to the command invocation, so shouldn't
> --fork-point and reflog use be disabled????

> From: Thomas Bachem <mail@thomasbachem.com>
> 
> The "--rebase -f with rebased upstream" test computes its fork point
> from the reflog of refs/remotes/me/copy, and the entry it needs is
> the one that the fetch of the test before it wrote. Like every reflog
> entry the suite writes after test_tick, it is dated 2005, so the
> first "git reflog expire --all" after that fetch removes it. Pull
> then finds no fork point and rebases onto the merge head with the
> merge head as the upstream, and the rewound commits come back as a
> conflict.
> 
> Since 452b12c2e0 (builtin/maintenance: use "geometric" strategy by
> default, 2026-02-24) auto maintenance runs that expiry once the reflog
> of HEAD holds a hundred entries it would remove, the default of
> maintenance.reflog-expire.auto. Which run crosses the threshold
> depends on the entries and maintenance runs before it, so the script
> passed by chance: a stash topic that no longer runs "git reset" from
> "stash apply --index" and a rebase topic that runs auto maintenance
> at the end of "git rebase" together move the expiry between the two
> tests.
> 
> Pin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it
> matters, 2026-02-24) did for the rebase tests. That covers a "git gc"
> as well, which expires reflogs on its own, where turning off the auto
> trigger of the reflog-expire task alone would not.

> 
> Reported-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: D. Ben Knoble <ben.knoble@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Assisted-by: Claude Fable 5.1
> Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
> Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
> ---
>   t/t5520-pull.sh | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 27f38ab3c8..bc818605a5 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -35,6 +35,12 @@ test_pull_autostash_fail () {
>   }
>   
>   test_expect_success setup '
> +	# Commit dates are hardcoded to 2005, and the reflog entries will have
> +	# a matching timestamp. Maintenance may thus immediately expire
> +	# reflogs if it was running.
> +	git config set gc.reflogExpire never &&
> +	git config set gc.reflogExpireUnreachable never &&
> +
>   	echo file >file &&
>   	git add file &&
>   	git commit -a -m original

> 
> Changes in v4:
> • Drop merge verbosity changes altogether. I was going to
>    save-and-restore, but when looking at the index-merge test case (more
>    below) closer, I noticed that "git apply --cached" reports conflicts
>    on stderr. That is, "git stash apply --index" would report conflicts,
>    and silencing the merge takes that away. So instead let's leave the
>    configured verbosity alone.
> • Only copy resulting index merge tree OID when successful
> • Fix interaction with t5520 (new patch 4/5)
> • Squash test from 3/5 into 5/5, since it requires actually merging
>    trees. I've elected to keep it a separate test for now (contrary to
>    Phillip's suggestion) since it's written and working. Adapting
>    existing tests requires quite a bit more digging into implicit context
>    assumptions ;)

> Changes in v3:
> 
> • Change conflict label for current index
> • Fix memory leak of merge_result
> • Fix order of trees to make the correct merge (cherry-pick)
>      • New test (3/5) to validate this
> • Fix test in 4/5 to assert more details of expected state
> 
> Changes in v2:
> 
> • Do give branch labels for the incore merge, although they are never
>    seen (and clarify commit message as a result, also keeping the
>    merge-ort asserts). Phillip was right: without those, we do segfault
>    on conflicts.
> • Use the ui merge options to keep the same diff algorithm.
> • Use merge_finalize instead of clear_merge_options, and reuse the
>    options between merge calls if they are already initialized.
> • Add a new 2/4 to simplify merge options initialization.
> • Add a new 3/4 with a test case for conflicted index merges.
> 
> v1: <cover.1789853192.git.ben.knoble@gmail.com>
> v2: <cover.1790168285.git.ben.knoble@gmail.com>
> v3: <cover.1790425008.git.ben.knoble@gmail.com>
> 
> [1/5] builtin/stash: remove unused header
> [2/5] stash: prepare merge options earlier
> [3/5] t3903: test failed "stash apply --index"
> [4/5] t5520: don't expire reflogs where it matters
> [5/5] builtin/stash: merge index in-core
> 
>   builtin/stash.c  | 91 ++++++++++++------------------------------------
>   t/t3903-stash.sh | 42 ++++++++++++++++++++++
>   t/t5520-pull.sh  |  6 ++++
>   t/t7600-merge.sh |  9 +++++
>   4 files changed, 79 insertions(+), 69 deletions(-)
> 
> Diff-intervalle contre v3 :
> 1:  6a165c4df4 = 1:  6a165c4df4 builtin/stash: remove unused header
> 2:  d9a9e18f3a ! 2:  35b64ae321 stash: prepare merge options earlier
>      @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
>        		return error(_("cannot apply a stash in the middle of a merge"));
>        
>       +	init_ui_merge_options(&o, the_repository);
>      ++
>      ++	if (quiet)
>      ++		o.verbosity = 0;
>       +
>        	if (index) {
>        		if (oideq(&info->b_tree, &info->i_tree) ||
>      @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
>        	o.branch1 = label_ours ? label_ours : "Updated upstream";
>        	o.branch2 = label_theirs ? label_theirs : "Stashed changes";
>        	o.ancestor = label_base ? label_base : "Stash base";
>      +@@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefix,
>      + 	if (oideq(&info->b_tree, &c_tree))
>      + 		o.branch1 = "Version stash was based on";
>      +
>      +-	if (quiet)
>      +-		o.verbosity = 0;
>      +-
>      + 	if (o.verbosity >= 3)
>      + 		printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);
>      +
> 4:  d39e16905d ! 3:  7b0b317ce0 t3903: test failed "stash apply --index"
>      @@ Commit message
>       
>        ## t/t3903-stash.sh ##
>       @@ t/t3903-stash.sh: setup_stash() {
>      - 	test_cmp expect file
>      + 	test_cmp expect actual
>        '
>        
>       +test_expect_success 'stash apply --index leaves everything untouched on failure' '
> 3:  8b5ea5e6f4 ! 4:  2ac371d2dc t3903: test stash --index merges
>      @@
>        ## Metadata ##
>      -Author: D. Ben Knoble <ben.knoble@gmail.com>
>      +Author: Thomas Bachem <mail@thomasbachem.com>
>       
>        ## Commit message ##
>      -    t3903: test stash --index merges
>      +    t5520: don't expire reflogs where it matters
>       
>      -    A future commit will refactor index handling for applied stashes, and we
>      -    need to take care to get the order of trees right when merging. Add a
>      -    test that covers this case.
>      +    The "--rebase -f with rebased upstream" test computes its fork point
>      +    from the reflog of refs/remotes/me/copy, and the entry it needs is
>      +    the one that the fetch of the test before it wrote. Like every reflog
>      +    entry the suite writes after test_tick, it is dated 2005, so the
>      +    first "git reflog expire --all" after that fetch removes it. Pull
>      +    then finds no fork point and rebases onto the merge head with the
>      +    merge head as the upstream, and the rewound commits come back as a
>      +    conflict.
>       
>      -    Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>      +    Since 452b12c2e0 (builtin/maintenance: use "geometric" strategy by
>      +    default, 2026-02-24) auto maintenance runs that expiry once the reflog
>      +    of HEAD holds a hundred entries it would remove, the default of
>      +    maintenance.reflog-expire.auto. Which run crosses the threshold
>      +    depends on the entries and maintenance runs before it, so the script
>      +    passed by chance: a stash topic that no longer runs "git reset" from
>      +    "stash apply --index" and a rebase topic that runs auto maintenance
>      +    at the end of "git rebase" together move the expiry between the two
>      +    tests.
>       
>      - ## t/t3903-stash.sh ##
>      -@@ t/t3903-stash.sh: setup_stash() {
>      - 	test_cmp expect actual
>      - '
>      +    Pin the expiry as ea7d894f44 (t34xx: don't expire reflogs where it
>      +    matters, 2026-02-24) did for the rebase tests. That covers a "git gc"
>      +    as well, which expires reflogs on its own, where turning off the auto
>      +    trigger of the reflog-expire task alone would not.
>      +
>      +    Reported-by: Junio C Hamano <gitster@pobox.com>
>      +    Helped-by: D. Ben Knoble <ben.knoble@gmail.com>
>      +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>      +    Assisted-by: Claude Fable 5.1
>      +    Signed-off-by: Thomas Bachem <mail@thomasbachem.com>
>      +
>      + ## t/t5520-pull.sh ##
>      +@@ t/t5520-pull.sh: test_pull_autostash_fail () {
>      + }
>        
>      -+# the later "stash -k" test is not expecting us to muck with file so much, so
>      -+# reset when finished
>      -+test_expect_success 'stash apply --index merges the correct trees' '
>      -+	head=$(git rev-parse HEAD) &&
>      -+	test_when_finished "git reset --hard $head" &&
>      -+	test_write_lines A B C >file &&
>      -+	git commit -m setup file &&
>      -+	test_write_lines A B staged >file &&
>      -+	git add file &&
>      -+	test_write_lines A B unstaged >file &&
>      -+	git stash &&
>      -+	test_write_lines committed B C >file &&
>      -+	git commit -m to-be-merged file &&
>      -+	git stash pop --index &&
>      -+	git show :file >actual &&
>      -+	test_write_lines committed B staged >expect &&
>      -+	test_cmp expect actual &&
>      -+	test_write_lines committed B unstaged >expect &&
>      -+	test_cmp expect file
>      -+'
>      + test_expect_success setup '
>      ++	# Commit dates are hardcoded to 2005, and the reflog entries will have
>      ++	# a matching timestamp. Maintenance may thus immediately expire
>      ++	# reflogs if it was running.
>      ++	git config set gc.reflogExpire never &&
>      ++	git config set gc.reflogExpireUnreachable never &&
>       +
>      - test_expect_success 'stash -k' '
>      - 	echo bar3 >file &&
>      - 	echo bar4 >file2 &&
>      + 	echo file >file &&
>      + 	git add file &&
>      + 	git commit -a -m original
> 5:  fde7fb7988 ! 5:  e21b832a6e builtin/stash: merge index in-core
>      @@ Commit message
>           we don't see the usual branch and ancestor labels, but the merge
>           subroutines insist on their presence, so use something simple.
>       
>      +    We need to take care to get the order of trees right when merging. Add a
>      +    test that covers this case.
>      +
>           We *could* swap just the git-reset(1) subprocess with our internal
>           reset_tree() and refresh_index(), which would fix the bug. We'd much
>           prefer to clean up these vestiges of the shell-based git-stash, though.
>      @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
>       -			ret = apply_cached(&out);
>       -			strbuf_release(&out);
>       -			if (ret)
>      -+			o.verbosity = 0;
>      -+
>       +			head = lookup_tree(o.repo, &c_tree);
>       +			merge = lookup_tree(o.repo, &info->i_tree);
>       +			merge_base = lookup_tree(o.repo, &info->b_tree);
>      @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
>       +			merge_incore_nonrecursive(&o, merge_base, head, merge,
>       +						  &result);
>       +
>      -+			oidcpy(&index_tree, &result.tree->object.oid);
>      -+			merge_finalize(&o, &result);
>      -+
>      -+			if (!result.clean)
>      ++			if (!result.clean) {
>      ++				merge_finalize(&o, &result);
>        				return error(_("conflicts in index. "
>        					       "Try without --index."));
>       -
>      @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
>       -			reset_head();
>       -			discard_index(the_repository->index);
>       -			repo_read_index(the_repository);
>      ++			} else {
>      ++				oidcpy(&index_tree, &result.tree->object.oid);
>      ++				merge_finalize(&o, &result);
>      ++			}
>        		}
>        	}
>        
>       
>      + ## t/t3903-stash.sh ##
>      +@@ t/t3903-stash.sh: setup_stash() {
>      + 	test_cmp expect-index actual-index
>      + '
>      +
>      ++# the later "stash -k" test is not expecting us to muck with file so much, so
>      ++# reset when finished
>      ++test_expect_success 'stash apply --index merges the correct trees' '
>      ++	head=$(git rev-parse HEAD) &&
>      ++	test_when_finished "git reset --hard $head" &&
>      ++	test_write_lines A B C >file &&
>      ++	git commit -m setup file &&
>      ++	test_write_lines A B staged >file &&
>      ++	git add file &&
>      ++	test_write_lines A B unstaged >file &&
>      ++	git stash &&
>      ++	test_write_lines committed B C >file &&
>      ++	git commit -m to-be-merged file &&
>      ++	git stash pop --index &&
>      ++	git show :file >actual &&
>      ++	test_write_lines committed B staged >expect &&
>      ++	test_cmp expect actual &&
>      ++	test_write_lines committed B unstaged >expect &&
>      ++	test_cmp expect file
>      ++'
>      ++
>      + test_expect_success 'stash -k' '
>      + 	echo bar3 >file &&
>      + 	echo bar4 >file2 &&
>      +
>        ## t/t7600-merge.sh ##
>       @@ t/t7600-merge.sh: verify_no_mergehead () {
>        	test_cmp result.1-5 file
> 
> base-commit: d38352cd43ab9745686d697872408bc3249a153f

> Let me see if I understand correctly…
> 
> On Mon, Sep 28, 2026 at 10:50 AM Thomas Bachem <mail@thomasbachem.com> wrote:
>>
>> On Mon, Sep 28, 2026 at 3:45 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> Oh, when I was thinking about this over lunch I did wonder if that might
>>> be the culprit. Previously we didn't run "git maintenance --auto" after
>>> a rebase with the 'merge' backend but with that topic we do, and because
>>> we set GIT_COMMITTER_DATE to sometime in 2005, if 'git reflog expire'
>>> gets triggered it will expire the reflog entries that 'git pull
>>> --rebase' relies on. As you suggested in another mail, I assume this
> 
>> "git pull --rebase" computes the fork point before it fetches, from
>> the reflog of refs/remotes/me/copy,
> 
> This is described by the manual for git-rebase under --fork-point,
> which is on unless we have an <upstream> or --keep-base (modulo
> config). Put a pin in this.
> 
>> and test 69 needs the entry that
>> test 68's fetch wrote there, copy-orig (f29aa66) to ae98574. With the
>> reflog empty, "merge-base --fork-point" falls back to the ref itself,
>> ae98574 is no ancestor of to-rebase, and pull hands the merge head to
>> rebase as the upstream. That is your "--onto ae98... ae98...", and the
>> four commits from copy-orig up come back, the first of them
>> conflicting with "conflict".
>>
>>> topic has changed something in one of the '--autostash' tests that come
>>> before the failing test triggers which the new behavior. What that
>>> something is I'm not sure; off the top of my head I'd expect the number
>>> of reflog entries in HEAD to be the same but maybe I'm missing
>>> something. Adding
>>
>> It is eight entries fewer, and they come from the failed merges, not
>> from the autostash tests. "git merge" restores a dirty tree with
>> "stash apply --index --quiet", and until Ben's series that spawned
>> "git reset --quiet --refresh", which writes "reset: moving to HEAD"
>> to the reflog. That happens eight times in t5520 before test 68.
>>
>> Auto maintenance expires reflogs once HEAD's reflog holds a hundred
>> entries that the policy would remove, the default of
>> maintenance.reflog-expire.auto, and after the first test_tick that is
>> every entry. Which run crosses the hundred depends on how many entries
>> and maintenance runs came before it. On 'seen' the expiry lands on
>> "git commit -m conflict" in test 68, before the fetch writes the entry.
>> Eight entries fewer move the crossing past that commit, and the
>> maintenance run my topic adds at the end of the rebase in test 68 is
>> the next one: after the fetch, before test 69 reads the reflog. Either
>> change alone leaves it somewhere harmless, and nothing else is going
>> on. The expiry is the usual 90 days applied to entries dated 2005, and
>> the only new thing is one more maintenance run per rebase, the same
>> one "git commit" and "git fetch" run.
> 
> In short, expiry used to happen prior to .68, so the reflog entry
> created in that test which is used by "pull --rebase" in .69 is picked
> up. With fewer reflog entries, expiry happens later, and it just so
> happens to drop the important entry. Darn!

> 
> But here's what I can't figure out, returning to that pin from
> earlier: I was a bit surprised to see mention of rebase reading
> reflogs! When I remembered --fork-point, I was even more curious (but
> at least it's obvious that rebase will read the reflogs in some
> scenarios).
> 
> What confuses me is that builtin/pull.c:run_rebase() sure looks like
> it provides an <upstream> to the command invocation, so shouldn't
> --fork-point and reflog use be disabled????
> 
> I'll try tracing that test myself later, I suppose. It's nice to know
> we have a fix available (thanks for the patch), but it sure feels like
> a hack :) oh well?

> Le 29 sept. 2026 à 11:48, Phillip Wood <phillip.wood123@gmail.com> a écrit :
> 
> Hi Ben
> 
>> On 29/09/2026 13:18, D. Ben Knoble wrote:
>> Changes in v4:
>> • Drop merge verbosity changes altogether. I was going to
>>   save-and-restore, but when looking at the index-merge test case (more
>>   below) closer, I noticed that "git apply --cached" reports conflicts
>>   on stderr. That is, "git stash apply --index" would report conflicts,
>>   and silencing the merge takes that away. So instead let's leave the
>>   configured verbosity alone.
>> • Only copy resulting index merge tree OID when successful
>> • Fix interaction with t5520 (new patch 4/5)
>> • Squash test from 3/5 into 5/5, since it requires actually merging
>>   trees. I've elected to keep it a separate test for now (contrary to
>>   Phillip's suggestion) since it's written and working. Adapting
>>   existing tests requires quite a bit more digging into implicit context
>>   assumptions ;)
> 
> I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me.
> 
> Thanks
> 
> Phillip

>
> "D. Ben Knoble" <ben.knoble@gmail.com> writes:
>
> > +                     merge_incore_nonrecursive(&o, merge_base, head, merge,
> > +                                               &result);
>
> In a hard error from merge_incore_nonrecursive(), result->clean is
> set to -1, which means that ...
>
> > +                     if (!result.clean) {
>
> ... "result.clean is false" is not true here, so we will ...
>
> > +                             merge_finalize(&o, &result);
> >                               return error(_("conflicts in index. "
> >                                              "Try without --index."));
> > +                     } else {
>
> ... come here to access result.tree member, no?
>
> > +                             oidcpy(&index_tree, &result.tree->object.oid);
> > +                             merge_finalize(&o, &result);
> > +                     }
>
> IOW, shouldn't it be more like three-way check,

>
>                         if (result.clean < 0) {
>                                 merge_finalize(&o, &result);
>                                 return error(_("index merge failed."));
>                         } else if (!result.clean) {
>                                 merge_finalize(&o, &result);
>                                 return error(_("conflict in index merge."));
>                         } else {
>                                 ... happy path ...
>                         }
>
> or something like that?

>
>
> > Le 29 sept. 2026 à 11:48, Phillip Wood <phillip.wood123@gmail.com> a écrit :
> >
> > Hi Ben
> >
> >> On 29/09/2026 13:18, D. Ben Knoble wrote:
> >> Changes in v4:
> >> • Drop merge verbosity changes altogether. I was going to
> >>   save-and-restore, but when looking at the index-merge test case (more
> >>   below) closer, I noticed that "git apply --cached" reports conflicts
> >>   on stderr. That is, "git stash apply --index" would report conflicts,
> >>   and silencing the merge takes that away. So instead let's leave the
> >>   configured verbosity alone.
> >> • Only copy resulting index merge tree OID when successful
> >> • Fix interaction with t5520 (new patch 4/5)
> >> • Squash test from 3/5 into 5/5, since it requires actually merging
> >>   trees. I've elected to keep it a separate test for now (contrary to
> >>   Phillip's suggestion) since it's written and working. Adapting
> >>   existing tests requires quite a bit more digging into implicit context
> >>   assumptions ;)
> >
> > I've left a comment on the new patch 4, but everything else in the range-diff looks ready to me.
> >
> > Thanks
> >
> > Phillip
>
> 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 ;)
>
> I’ll probably wait a bit and see how the dust settles, but:
>
> 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.

> 
> Changes in v5:
> • Rebase on synthetic merge for the test interaction with t5520
>    (dropping old 4/5) [59d1ce1b6e (Merge branch 'tb/t5520-reflog-expire'
>    into dk/stash-apply-index-incore, 2026-09-29)]
> • Fix handling of tri-state merge_result.clean

> Changes in v4:
> • Drop merge verbosity changes altogether. I was going to
>    save-and-restore, but when looking at the index-merge test case (more
>    below) closer, I noticed that "git apply --cached" reports conflicts
>    on stderr. That is, "git stash apply --index" would report conflicts,
>    and silencing the merge takes that away. So instead let's leave the
>    configured verbosity alone.
> • Only copy resulting index merge tree OID when successful
> • Fix interaction with t5520 (new patch 4/5)
> • Squash test from 3/5 into 5/5, since it requires actually merging
>    trees. I've elected to keep it a separate test for now (contrary to
>    Phillip's suggestion) since it's written and working. Adapting
>    existing tests requires quite a bit more digging into implicit context
>    assumptions ;)
> 
> Changes in v3:
> 
> • Change conflict label for current index
> • Fix memory leak of merge_result
> • Fix order of trees to make the correct merge (cherry-pick)
>      • New test (3/5) to validate this
> • Fix test in 4/5 to assert more details of expected state
> 
> Changes in v2:
> 
> • Do give branch labels for the incore merge, although they are never
>    seen (and clarify commit message as a result, also keeping the
>    merge-ort asserts). Phillip was right: without those, we do segfault
>    on conflicts.
> • Use the ui merge options to keep the same diff algorithm.
> • Use merge_finalize instead of clear_merge_options, and reuse the
>    options between merge calls if they are already initialized.
> • Add a new 2/4 to simplify merge options initialization.
> • Add a new 3/4 with a test case for conflicted index merges.
> 
> v1: <cover.1789853192.git.ben.knoble@gmail.com>
> v2: <cover.1790168285.git.ben.knoble@gmail.com>
> v3: <cover.1790425008.git.ben.knoble@gmail.com>
> v4: <cover.1790684309.git.ben.knoble@gmail.com>
> 
> [1/4] builtin/stash: remove unused header
> [2/4] stash: prepare merge options earlier
> [3/4] t3903: test failed "stash apply --index"
> [4/4] builtin/stash: merge index in-core
> 
>   builtin/stash.c  | 94 +++++++++++++-----------------------------------
>   t/t3903-stash.sh | 42 ++++++++++++++++++++++
>   t/t7600-merge.sh |  9 +++++
>   3 files changed, 76 insertions(+), 69 deletions(-)
> 
> Diff-intervalle contre v4 :
> 1:  6a165c4df4 = 1:  d8f4c36459 builtin/stash: remove unused header
> 2:  35b64ae321 = 2:  8e99033ef0 stash: prepare merge options earlier
> 3:  7b0b317ce0 = 3:  ee28d0a840 t3903: test failed "stash apply --index"
> 4:  2ac371d2dc < -:  ---------- t5520: don't expire reflogs where it matters
> 5:  e21b832a6e ! 4:  ca3de1d4a3 builtin/stash: merge index in-core
>      @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
>       +			merge_incore_nonrecursive(&o, merge_base, head, merge,
>       +						  &result);
>       +
>      -+			if (!result.clean) {
>      ++			if (result.clean < 0) {
>      ++				merge_finalize(&o, &result);
>      ++				return error(_("index merge failed"));
>      ++			} else if (!result.clean) {
>       +				merge_finalize(&o, &result);
>        				return error(_("conflicts in index. "
>        					       "Try without --index."));
> 
> base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
> prerequisite-patch-id: 601853fa5478b0dbfb260ba02632418e90338219

> Hi Ben
>
> On 30/09/2026 22:24, D. Ben Knoble wrote:
>> 
>> Changes in v5:
>> • Rebase on synthetic merge for the test interaction with t5520
>>    (dropping old 4/5) [59d1ce1b6e (Merge branch 'tb/t5520-reflog-expire'
>>    into dk/stash-apply-index-incore, 2026-09-29)]
>> • Fix handling of tri-state merge_result.clean
>
> The range-diff below looks as expected, thanks for working on this, I'm 
> really pleased to see us removing some subprocesses from "git stash".