[PATCH 0/2] Hi all,

[PATCH 0/2] Hi all,

[D. Ben Knoble]

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, while the second holds the
interesting bits. Preferences on keeping or removing a few assert()
calls in merge-ort.c are welcome.

[1/2] builtin/stash: remove unused header
[2/2] builtin/stash: merge index in-core

 builtin/stash.c  | 77 +++++++++---------------------------------------
 merge-ort.c      |  3 --
 t/t7600-merge.sh |  9 ++++++
 3 files changed, 23 insertions(+), 66 deletions(-)


base-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH 1/2] builtin/stash: remove unused header

[D. Ben Knoble]

Clang complains that oid-array.h is unused. Certainly none of the
oid_array* functions, types, etc., are used, and the
transitively-included hash.h declarations are used but covered by a
pre-existing direct #include of hash.h.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH 1/2] builtin/stash: remove unused header

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

> Clang complains that oid-array.h is unused. Certainly none of the
> oid_array* functions, types, etc., are used, and the


> transitively-included hash.h declarations are used but covered by a
> pre-existing direct #include of hash.h.

Good thing to make sure.

And the correctness of the patch can easily be validated, which
makes this kind of patch no-brainer to accept ;-)

Thanks.

>
> 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

[PATCH 2/2] builtin/stash: merge index in-core

[D. Ben Knoble]

"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.

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.

Reported-by: Eli Barzilay <eli@barzilay.org>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---

Notes (benknoble/commits):
    We *could* leave the asserts in, but then we somewhat uselessly set the
    conflict labels, which I did in the original patch [1]. Phillip
    suggested we don't need them, and I otherwise agree.
    
    [1]: https://lore.kernel.org/git/CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com/
    
    In all the versions of 231e2dd49d (merge-ort: add some high-level
    algorithm structure, 2020-12-13) I could find on the mailing list, the
    "assert(opt->ancestor)" is present without explanation or comment, so
    I'm not in a good place to assess the impact of removing it and its
    compatriots.
    
    Cc: Elijah Newren <newren@gmail.com>

 builtin/stash.c  | 76 +++++++++---------------------------------------
 merge-ort.c      |  3 --
 t/t7600-merge.sh |  9 ++++++
 3 files changed, 23 insertions(+), 65 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index dfea2d2c4c..9fc1a25e3d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	return ret;
 }
 
-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *w_commit_hex = oid_to_hex(w_commit);
-
-	/*
-	 * Diff-tree would not be very hard to replace with a native function,
-	 * however it should be done together with apply_cached.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
-	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
-
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
-
-static int apply_cached(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Apply currently only reads either from stdin or a file, thus
-	 * apply_all_patches would have to be updated to optionally take a
-	 * buffer.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "apply", "--cached", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
-}
-
-static int reset_head(void)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Reset is overall quite simple, however there is no current public
-	 * API for resetting.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
-
-	return run_command(&cp);
-}
-
 static int is_path_a_directory(const char *path)
 {
 	/*
@@ -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);
 	/*
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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH 2/2] builtin/stash: merge index in-core

[Phillip Wood]

Hi Ben

On 19/09/2026 22:26, D. Ben Knoble wrote:
> "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.

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.

> 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.

Definitely

>   builtin/stash.c  | 76 +++++++++---------------------------------------

Nice diffstat!

> @@ -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.

>   
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)
> +			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.

> +
> +			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);

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.

>   	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
> +'

We check the autostash is applied and is not saved - good

Thanks for working on this, it is really good to get rid of those 
subprocesses.

Phillip

>   test_expect_success 'failed fast-forward merge with --autostash' '
>   	git reset --hard c0 &&
>   	git merge-file file file.orig file.5 &&

Re: [PATCH 2/2] builtin/stash: merge index in-core

[D. Ben Knoble]

On Mon, Sep 21, 2026 at 9:17 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.

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. But it doesn't actually clear the merge
options.

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. I'll probably keep a similar structure as the
working tree merge uses, I think.

A fail-to-merge test also seems like a good idea. Let me mull on that.

Thanks for the review.

-- 
D. Ben Knoble

Re: [PATCH 2/2] builtin/stash: merge index in-core

[D. Ben Knoble]

On Tue, Sep 22, 2026 at 8:43 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> 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?

er, "!has_index" of course (which is what I originally typed and then,
confused, edited).

-- 
D. Ben Knoble

Re: [PATCH 2/2] builtin/stash: merge index in-core

[Phillip Wood]

Hi Ben

On 22/09/2026 13:43, D. Ben Knoble wrote:
> 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?

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

> 
>>> 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.

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.

> A fail-to-merge test also seems like a good idea. Let me mull on that.

That's great

Thanks

Phillip

> Thanks for the review.
> 

Re: [PATCH 2/2] builtin/stash: merge index in-core

[D. Ben Knoble]

Thanks again, Philip :)

On Tue, Sep 22, 2026 at 9:57 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.

Gotcha; I've already queued this locally.

> >>> +                     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!

That's very helpful; I find it challenging right now to navigate the
various call-graphs here :)

> > 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.

Possibly! I still think it signals our intent to be quiet better this way, too.
> >>> +                     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)

Maybe---I was more worried about un-reusable state, but it's true that
the clear function is a no-op right now, heh. So it was a bit of "in
case one day this is mandatory," perhaps.

> > 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

Yeah, that's smarter. Locally I got tripped by the case where we said
--index but skip some work; but it should be fine to unconditionally
initialize those options earlier.

> > 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

No worries.

> > 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.

That's what I ended up with locally, yeah. I finally decided it was
too complicated to do anything else for labels that would be really
hard to find.

I'll get v2 out in the morning, probably.

-- 
D. Ben Knoble

Re: [PATCH 0/2] Hi all,

[D. Ben Knoble]

My apologies for the strange subject; a little mishap when editing the
branch description (I forgot the first line was special).

[PATCH v2 0/4] stash: clean up index-mode test merge

[D. Ben Knoble]

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 adds a new test, while the
fourth holds the interesting bits.

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>

[1/4] builtin/stash: remove unused header
[2/4] stash: prepare merge options earlier
[3/4] t: test failed "stash apply --index"
[4/4] builtin/stash: merge index in-core

 builtin/stash.c  | 83 +++++++++++-------------------------------------
 t/t3903-stash.sh | 18 +++++++++++
 t/t7600-merge.sh |  9 ++++++
 3 files changed, 45 insertions(+), 65 deletions(-)

Diff-intervalle contre v1 :
1:  b6798c8a25 = 1:  b6798c8a25 builtin/stash: remove unused header
-:  ---------- > 2:  1e2343c7fc stash: prepare merge options earlier
-:  ---------- > 3:  5bd4b78cac t: test failed "stash apply --index"
2:  782fe91251 ! 4:  e49936ee12 builtin/stash: merge index in-core
    @@ Commit message
     
         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.
    +    merge in-core. If there are conflicts, we discard the resulting tree, so
    +    we don't see the usual branch and ancestor labels, but the merge
    +    subroutines insist on their presence, so use something simple.
     
         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
    @@ Commit message
         Reported-by: Eli Barzilay <eli@barzilay.org>
         Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    -
    - ## Notes (benknoble/commits) ##
    -    We *could* leave the asserts in, but then we somewhat uselessly set the
    -    conflict labels, which I did in the original patch [1]. Phillip
    -    suggested we don't need them, and I otherwise agree.
    -
    -    [1]: https://lore.kernel.org/git/CALnO6CDfwscMWZktBu3FtXOQVbcBRo76nqK07kMnrzC5cPyZiQ@mail.gmail.com/
    -
    -    In all the versions of 231e2dd49d (merge-ort: add some high-level
    -    algorithm structure, 2020-12-13) I could find on the mailing list, the
    -    "assert(opt->ancestor)" is present without explanation or comment, so
    -    I'm not in a good place to assess the impact of removing it and its
    -    compatriots.
    -
    -    Cc: Elijah Newren <newren@gmail.com>
    -
      ## builtin/stash.c ##
     @@ builtin/stash.c: static int create_index_from_tree(const struct object_id *tree_id,
      	return ret;
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -				return error(_("could not generate diff %s^!."),
     -					     oid_to_hex(&info->w_commit));
     -			}
    -+			init_basic_merge_options(&o, the_repository);
    ++			o.branch1 = "Upstream index";
    ++			o.branch2 = "Stashed index changes";
    ++			o.ancestor = "Stash base";
      
     -			ret = apply_cached(&out);
     -			strbuf_release(&out);
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -			discard_index(the_repository->index);
     -			repo_read_index(the_repository);
     +			oidcpy(&index_tree, &result.tree->object.oid);
    -+			clear_merge_options(&o);
    ++			merge_finalize(&o, &result);
      		}
      	}
      
     
    - ## merge-ort.c ##
    -@@ merge-ort.c: 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);
    -@@ merge-ort.c: 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);
    - 	/*
    -
      ## t/t7600-merge.sh ##
     @@ t/t7600-merge.sh: verify_no_mergehead () {
      	test_cmp result.1-5 file

base-commit: 339ab2a8f14c0c304ae2f28df1a859f3d2cf610c
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v2 1/4] builtin/stash: remove unused header

[D. Ben Knoble]

Clang complains that oid-array.h is unused. Certainly none of the
oid_array* functions, types, etc., are used, and the
transitively-included hash.h declarations are used but covered by a
pre-existing direct #include of hash.h.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v2 2/4] stash: prepare merge options earlier

[D. Ben Knoble]

In a future commit, we will reuse these options for the index merge of
"apply --index", not just for the worktree.

Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index dfea2d2c4c..043a38cc6d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -664,6 +664,8 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 				repo_get_index_file(the_repository), 0, NULL))
 		return error(_("cannot apply a stash in the middle of a merge"));
 
+	init_ui_merge_options(&o, the_repository);
+
 	if (index) {
 		if (oideq(&info->b_tree, &info->i_tree) ||
 		    oideq(&c_tree, &info->i_tree)) {
@@ -695,8 +697,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 		}
 	}
 
-	init_ui_merge_options(&o, the_repository);
-
 	o.branch1 = label_ours ? label_ours : "Updated upstream";
 	o.branch2 = label_theirs ? label_theirs : "Stashed changes";
 	o.ancestor = label_base ? label_base : "Stash base";
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v2 3/4] t: test failed "stash apply --index"

[D. Ben Knoble]

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 &&
+
+	echo conflict >other-file &&
+	git add other-file &&
+	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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v2 3/4] t: test failed "stash apply --index"

[Phillip Wood]

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.

Thanks for adding this test, it is a useful improvement in our coverage.

Phillip

> +	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 &&

Re: [PATCH v2 3/4] t: test failed "stash apply --index"

[D. Ben Knoble]

Hi Phillip,

On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.

I confess I played with diff-files and diff-index manually before
trying to construct this test case, and I still don't totally
understand how they're being used in the test just prior…

Anway, it looks to me like "diff-files -p" is the same as "diff -p"
(albeit without some niceties like color-moved applying automatically
from config), so that would make the test quite a bit more
complicated, no? (The "index $sha1..$sha2" line would change…)

Since we know what the expected contents are, perhaps we can simply
assert on those.

Hm. I spent some time with test_pause in the previous test, and I
think my concerns about that line changing are moot. But, asserting on
the contents is also a bit silly (as that's what the blob IDs are
doing for us in the output).

> > +     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, this test sets up an intermediate state prior to attempting to unstash where

- another-file is new in the index & working tree (content: "unrelated")
- other-file is modified in the index & working tree (content: from
"6" to "conflict")
- file is modified in the working tree (content: from "bar" to "unrelated")

And we should still be there when finished. (I wonder if, like the
previous test, we should have a file that differs from itself in the
index and working tree?)

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

Phew! Thanks for reading my rambling thinking aloud :)

-- 
D. Ben Knoble

Re: [PATCH v2 3/4] t: test failed "stash apply --index"

[Phillip Wood]

Hi Ben

On 25/09/2026 14:36, D. Ben Knoble wrote:
> 
> 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

I think that sounds reasonable, we can delete the index lines from the 
patch output with sed to make it easier to compare them.

Thanks

Phillip

> Phew! Thanks for reading my rambling thinking aloud :)
> 

Re: [PATCH v2 3/4] t: test failed "stash apply --index"

[Phillip Wood]

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

Re: [PATCH v2 3/4] t: test failed "stash apply --index"

[D. Ben Knoble]

On Sat, Sep 26, 2026 at 5:53 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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

Doh!

On the other hand, I don't think we need it. Those lines are showing
blob IDs, which would be stable in our case (fixed hash algorithm over
fixed contents), and we don't make the test dependent on the actual
IDs?

-- 
D. Ben Knoble

[PATCH v2 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

"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. If there are conflicts, we discard the resulting tree, so
we don't see the usual branch and ancestor labels, but the merge
subroutines insist on their presence, so use something simple.

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.

Reported-by: Eli Barzilay <eli@barzilay.org>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c  | 78 ++++++++++--------------------------------------
 t/t7600-merge.sh |  9 ++++++
 2 files changed, 25 insertions(+), 62 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 043a38cc6d..219ca457be 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	return ret;
 }
 
-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *w_commit_hex = oid_to_hex(w_commit);
-
-	/*
-	 * Diff-tree would not be very hard to replace with a native function,
-	 * however it should be done together with apply_cached.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
-	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
-
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
-
-static int apply_cached(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Apply currently only reads either from stdin or a file, thus
-	 * apply_all_patches would have to be updated to optionally take a
-	 * buffer.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "apply", "--cached", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
-}
-
-static int reset_head(void)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Reset is overall quite simple, however there is no current public
-	 * API for resetting.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
-
-	return run_command(&cp);
-}
-
 static int is_path_a_directory(const char *path)
 {
 	/*
@@ -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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Phillip Wood]

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.
Everything else looks fine.

Thanks

Phillip

> -			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 &&

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

Hi Phillip,

On Thu, Sep 24, 2026 at 5:42 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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.

Will fix. The "upstream" verbiage comes from the working tree labels.

> > +                     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.

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

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Phillip Wood]

Hi Ben

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

Thanks

Phillip>

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

On Fri, Sep 25, 2026 at 11:58 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 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

Ah, sorry. My link was sloppy.

I did get that far, but the allocation backtrace doesn't make it
obvious that merge_result is what leaked, and that's where I was
saying an especial thank you ;)

-- 
D. Ben Knoble

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Junio C Hamano]

"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 = "Upstream index";
> +			o.branch2 = "Stashed index changes";
> +			o.ancestor = "Stash base";
>  
> -			ret = apply_cached(&out);
> -			strbuf_release(&out);
> -			if (ret)

So, we used to take a diff between w_commit^2^ and w_commit^2 and
then give the resulting patch to "apply --cached".  w_commit is the
working tree state, w_commit^1 is the HEAD (i.e. b_tree) when the
stash was created (i.e., "diff HEAD w_commit" is the change in the
working tree), w_commit^2 is the contents of the index
(i.e. i_tree), so we are computing a patch that represents what
"diff --cached HEAD" would have shown when we created the stash.
And the goal is to reflect this change on the current HEAD to
recreate the "staged" changes in the current index.

IOW, we want to three-way merge the change that moves you from 
b_tree to i_tree into c_tree.

> +			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);

We are using the merge machinery to perform a cherry-pick of the
changes to go from b_tree to i_tree into c_tree.  but the three
trees involved in this cherry-pick is named unnecessarily
confusingly.

merge-incore-nonrecursive() takes the common ancestor ("merge_base")
and two sides ("side1" and "side2") in this order.  It takes the
changes to go from common to side1 and computes the result of
updating the remainder (side2) with such a change (or vice versa; a
merge is symmetric).  When cherry-picking, the first tree would be
the b_tree, the second tree would be the i_tree, and the target tree
would be the c_tree.

 - b_tree serves as merge_base
 - i_tree serves as side1
 - c_tree serves as side2

Am I following what the code should be doing correctly?

I am wondering if the order of the tree trees in the
merge_incore_nonrecursive() call is correct.  Shouldn't it be

			merge_incore_nonrecursive(&o,
						  merge_base, merge, head,
						  &result);

(or merge and head swapped) if we want to update head (I would call
it side2) in such a way that the change to go from it to the
resulting tree is similar to the change between merge_base (b_tree)
and merge (i_tree)?

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 &&

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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.

Oh, and with the change to the code, it passes again.

diff --git i/builtin/stash.c w/builtin/stash.c
index 219ca457be..44d962cc5d 100644
--- i/builtin/stash.c
+++ w/builtin/stash.c
@@ -639,7 +639,7 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 			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,
+			merge_incore_nonrecursive(&o, merge_base, merge, head,
 						  &result);
 
 			if (!result.clean)
diff --git i/t/t3903-stash.sh w/t/t3903-stash.sh
index 3958ab3c8d..0a87e62b11 100755
--- i/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 &&

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

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.

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Junio C Hamano]

"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.

Thanks.

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Phillip Wood]

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 &&
test_write_lines committed B unstaged >expect &&
test_cmp expect file

Thanks

Phillip

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

On Sat, Sep 26, 2026 at 5:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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 &&

s/text/test ;)

> test_write_lines committed B unstaged >expect &&
> test_cmp expect file

This does fail on the original code (head, base, merge_base) because
the index (git show :file) has "A B staged" lines instead of
"committed B staged" lines.

This test does pass on the new code, but needs some
arrangement/cleanup for the later "stash -k" test to succeed, so I'll
include that in the next round as well.

-- 
D. Ben Knoble

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Phillip Wood]

Hi Junio

On 24/09/2026 22:59, Junio C Hamano wrote:
> "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.

Ouch that's nasty. Well spotted, I missed it when I read the code 
(because the arguments were in the same order as the call to 
merge_ort_nonrecursive()) and the tests we have use the same version of 
the file for "base" and "stage2" so do not notice if they'd been 
transposed. It is rather confusing that two functions that are so 
closely related take their arguments in a different order.

> 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

> 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 &&

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

On Fri, Sep 25, 2026 at 12:04 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Junio
>
> On 24/09/2026 22:59, Junio C Hamano wrote:

[snip]

> > 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

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

Re: [PATCH v2 4/4] builtin/stash: merge index in-core

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

>> 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.

Oh, absolutely it is out of scope for this series.

[PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

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:

• 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>

[1/5] builtin/stash: remove unused header
[2/5] stash: prepare merge options earlier
[3/5] t3903: test stash --index merges
[4/5] t3903: test failed "stash apply --index"
[5/5] builtin/stash: merge index in-core

 builtin/stash.c  | 85 +++++++++++-------------------------------------
 t/t3903-stash.sh | 42 ++++++++++++++++++++++++
 t/t7600-merge.sh |  9 +++++
 3 files changed, 70 insertions(+), 66 deletions(-)

Diff-intervalle contre v2 :
1:  b6798c8a25 = 1:  6a165c4df4 builtin/stash: remove unused header
2:  1e2343c7fc = 2:  d9a9e18f3a stash: prepare merge options earlier
-:  ---------- > 3:  8b5ea5e6f4 t3903: test stash --index merges
3:  5bd4b78cac ! 4:  d39e16905d t: test failed "stash apply --index"
    @@ Metadata
     Author: D. Ben Knoble <ben.knoble@gmail.com>
     
      ## Commit message ##
    -    t: test failed "stash apply --index"
    +    t3903: test failed "stash apply --index"
     
         The next commit will refactor index handling for applied stashes, so
         let's make sure we cover conflicted index merging, too.
     
    +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
    +
      ## t/t3903-stash.sh ##
     @@ t/t3903-stash.sh: setup_stash() {
    - 	test_cmp expect actual
    + 	test_cmp expect file
      '
      
     +test_expect_success 'stash apply --index leaves everything untouched on failure' '
    @@ t/t3903-stash.sh: setup_stash() {
     +	echo unrelated >file &&
     +	echo unrelated >another-file &&
     +	git add another-file &&
    -+	git diff-files >expect &&
    -+
     +	echo conflict >other-file &&
     +	git add other-file &&
    ++	git diff-files -p >expect &&
    ++	git diff-index --cached HEAD >expect-index &&
    ++
     +	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
    ++	git diff-files -p >actual &&
    ++	test_cmp expect actual &&
    ++	git diff-index --cached HEAD >actual-index &&
    ++	test_cmp expect-index actual-index
     +'
     +
      test_expect_success 'stash -k' '
4:  e49936ee12 ! 5:  fde7fb7988 builtin/stash: merge index in-core
    @@ Commit message
     
         Reported-by: Eli Barzilay <eli@barzilay.org>
         Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
     
      ## builtin/stash.c ##
     @@ builtin/stash.c: static int create_index_from_tree(const struct object_id *tree_id,
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     -				return error(_("could not generate diff %s^!."),
     -					     oid_to_hex(&info->w_commit));
     -			}
    -+			o.branch1 = "Upstream index";
    ++			o.branch1 = "Current index";
     +			o.branch2 = "Stashed index changes";
     +			o.ancestor = "Stash base";
      
    @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
     +			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,
    ++			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,
    @@ 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);
    -+			oidcpy(&index_tree, &result.tree->object.oid);
    -+			merge_finalize(&o, &result);
      		}
      	}
      

base-commit: d38352cd43ab9745686d697872408bc3249a153f
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v3 1/5] builtin/stash: remove unused header

[D. Ben Knoble]

Clang complains that oid-array.h is unused. Certainly none of the
oid_array* functions, types, etc., are used, and the
transitively-included hash.h declarations are used but covered by a
pre-existing direct #include of hash.h.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v3 2/5] stash: prepare merge options earlier

[D. Ben Knoble]

In a future commit, we will reuse these options for the index merge of
"apply --index", not just for the worktree.

Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index dfea2d2c4c..043a38cc6d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -664,6 +664,8 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 				repo_get_index_file(the_repository), 0, NULL))
 		return error(_("cannot apply a stash in the middle of a merge"));
 
+	init_ui_merge_options(&o, the_repository);
+
 	if (index) {
 		if (oideq(&info->b_tree, &info->i_tree) ||
 		    oideq(&c_tree, &info->i_tree)) {
@@ -695,8 +697,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 		}
 	}
 
-	init_ui_merge_options(&o, the_repository);
-
 	o.branch1 = label_ours ? label_ours : "Updated upstream";
 	o.branch2 = label_theirs ? label_theirs : "Stashed changes";
 	o.ancestor = label_base ? label_base : "Stash base";
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v3 3/5] t3903: test stash --index merges

[D. Ben Knoble]

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.

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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v3 3/5] t3903: test stash --index merges

[Phillip Wood]

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 and I think we can probably replace an 
existing "stash apply --index" tests that are not so strict with this 
one, rather than adding a new test.

Thanks

Phillip

> 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 &&

Re: [PATCH v3 3/5] t3903: test stash --index merges

[D. Ben Knoble]

On Mon, Sep 28, 2026 at 11:44 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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

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.

> and I think we can probably replace an
> existing "stash apply --index" tests that are not so strict with this
> one, rather than adding a new test.

That's probably a good idea, thanks.

Re: [PATCH v3 3/5] t3903: test stash --index merges

[Phillip Wood]

Hi Ben

On 28/09/2026 16:55, D. Ben Knoble wrote:
> 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.

It fails because it tries to apply a patch that looks like

@@ -1,3 +1,3 @@
  A
  B
-C
+staged

to a file that looks like

committed
B
C

and so the first context line does not match. Because the changes do not 
overlap the merge machinery is perfectly happy. As an aside when we 
clear the worktree changes from "git stash push -p" generate the patch 
with "-U1" to try and avoid problems like this.

Thanks

Phillip

> 
>> and I think we can probably replace an
>> existing "stash apply --index" tests that are not so strict with this
>> one, rather than adding a new test.
> 
> That's probably a good idea, thanks.

[PATCH v3 4/5] t3903: test failed "stash apply --index"

[D. Ben Knoble]

The next commit will refactor index handling for applied stashes, so
let's make sure we cover conflicted index merging, too.

Helped-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 9bc99fa252..11942d875b 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -395,6 +395,27 @@ setup_stash() {
 	test_cmp expect file
 '
 
+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 &&
+	echo conflict >other-file &&
+	git add other-file &&
+	git diff-files -p >expect &&
+	git diff-index --cached HEAD >expect-index &&
+
+	test_must_fail git stash apply --index 2>err &&
+	test_grep "conflicts in index. Try without --index" err &&
+	git diff-files -p >actual &&
+	test_cmp expect actual &&
+	git diff-index --cached HEAD >actual-index &&
+	test_cmp expect-index actual-index
+'
+
 test_expect_success 'stash -k' '
 	echo bar3 >file &&
 	echo bar4 >file2 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v3 5/5] builtin/stash: merge index in-core

[D. Ben Knoble]

"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. If there are conflicts, we discard the resulting tree, so
we don't see the usual branch and ancestor labels, but the merge
subroutines insist on their presence, so use something simple.

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.

Reported-by: Eli Barzilay <eli@barzilay.org>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c  | 80 ++++++++++--------------------------------------
 t/t7600-merge.sh |  9 ++++++
 2 files changed, 26 insertions(+), 63 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 043a38cc6d..ac3b3cf84d 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	return ret;
 }
 
-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *w_commit_hex = oid_to_hex(w_commit);
-
-	/*
-	 * Diff-tree would not be very hard to replace with a native function,
-	 * however it should be done together with apply_cached.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
-	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
-
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
-
-static int apply_cached(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Apply currently only reads either from stdin or a file, thus
-	 * apply_all_patches would have to be updated to optionally take a
-	 * buffer.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "apply", "--cached", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
-}
-
-static int reset_head(void)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Reset is overall quite simple, however there is no current public
-	 * API for resetting.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
-
-	return run_command(&cp);
-}
-
 static int is_path_a_directory(const char *path)
 {
 	/*
@@ -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);
 		}
 	}
 
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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v3 5/5] builtin/stash: merge index in-core

[Junio C Hamano]

"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).

> +			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);

And then the (index) merge is quiet, which is nice.

> +
> +			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.

It is a bit surprising that the existing test suite did not catch
this.  Perhaps we do not test --quiet and the merge.verbosity
configuration in combination?

Anyway, I think you'd need something like the following (caveat
emptor: written against checked out 'seen' while reading the patch,
and not even compile tested).

Thanks.


 builtin/stash.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git c/builtin/stash.c w/builtin/stash.c
index 0f10b9c703..a165419d77 100644
--- c/builtin/stash.c
+++ w/builtin/stash.c
@@ -622,6 +622,9 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 
 	init_ui_merge_options(&o, the_repository);
 
+	if (quiet)
+		o.verbosity = 0;
+
 	if (index) {
 		if (oideq(&info->b_tree, &info->i_tree) ||
 		    oideq(&c_tree, &info->i_tree)) {
@@ -633,8 +636,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 			o.branch2 = "Stashed index changes";
 			o.ancestor = "Stash base";
 
-			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);
@@ -658,9 +659,6 @@ 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);
 

Re: [PATCH v3 5/5] builtin/stash: merge index in-core

[D. Ben Knoble]

On Sun, Sep 27, 2026 at 2:59 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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).

Yes, the original piped "apply --cached" output to a strbuf and discarded it.

> > +
> > +                     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.

But you're right, we should restore the verbosity (which is not what
the sketch patch does exactly). Will fix.

Re: [PATCH v3 5/5] builtin/stash: merge index in-core

[Junio C Hamano]

"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.

Re: [PATCH v3 5/5] builtin/stash: merge index in-core

[D. Ben Knoble]

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.

-- 
D. Ben Knoble

Re: [PATCH v3 5/5] builtin/stash: merge index in-core

[Junio C Hamano]

"D. Ben Knoble" <ben.knoble@gmail.com> writes:

> 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.

I did

	$ git grep merge_incore_nonrecursive \*.c

and read all the current callers.

They all have code to specifically check for the (result.clean < 0)
condition and error out before touching any of the other members of
the result structure, so they seem to be safe.

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

On Sat, Sep 26, 2026 at 8:17 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> 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:

Woops. Contrary to my usual practice of late, I sent this in reply to
v2 rather than v1. Oh well.

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[Junio C Hamano]

"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.

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[Phillip Wood]

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.

I'm a bit stumped by that as the failing test (5520.69 '--rebase -f with 
rebased upstream') does not stash anything. 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.

Thanks

Phillip

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

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:
> > "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.

First I'm hearing about it, but I'll try to bisect seen and see what I can find.

> I'm a bit stumped by that as the failing test (5520.69 '--rebase -f with
> rebased upstream') does not stash anything.

I wonder if a prior test is affected "silently" and we only find out by .69?

> 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.

Thanks Phillip!

-- 
D. Ben Knoble

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

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):

#! /bin/zsh
HEAD=$(git rev-parse HEAD) &&
git merge --no-edit bk/autostash-index-reset &&
if ! ninja -C build; then exit 125; fi &&
meson test -C build t5520-pull # no chain! need to keep going no matter what
code=$? &&
git reset --hard $HEAD &&
exit $code

(using "git bisect start --first-parent origin/seen @")

[Cc: Thomas Bachem <mail@thomasbachem.com> in case you have any immediate ideas]

I don't think the bisect log will interest anyone, but I've attached it anyway.

-- 
D. Ben Knoble

[-- Attachment #2: log --]
[-- Type: application/octet-stream, Size: 1444 bytes --]

# bad: [d41a8dba17a01f8baf9103ba46c39c604dab6356] Merge branch 'kh/format-patch-range-diff-notes' into seen
# good: [0e21fbff4bd10ac5d2ee7f47341f7200cb1a792c] builtin/stash: merge index in-core
git bisect start '--first-parent' 'origin/seen' '@'
# good: [d38352cd43ab9745686d697872408bc3249a153f] A few more fixes before -rc2
git bisect good d38352cd43ab9745686d697872408bc3249a153f
# bad: [b13f6a2d2686eeb18c47a5d31c4a8e04a6b11888] Merge branch 'pp/midx-write-skip-empty' into jch
git bisect bad b13f6a2d2686eeb18c47a5d31c4a8e04a6b11888
# bad: [4f65642eb051c595de1b06d148c46f4e316fafc3] Merge branch 'tb/rerere-lock-grace' into jch
git bisect bad 4f65642eb051c595de1b06d148c46f4e316fafc3
# good: [191f6115f88b68cf050c3723185cdac64605193f] Merge branch 'ps/ref-storage-format' into jch
git bisect good 191f6115f88b68cf050c3723185cdac64605193f
# good: [138c4d2b2f7dad98fb495f9a0b6d8983f3386524] Merge branch 'rr/upload-pack-swap-shallow-wanted-ref' into jch
git bisect good 138c4d2b2f7dad98fb495f9a0b6d8983f3386524
# good: [80b9a513296d51b516138c9b0db4527461d14187] Merge branch 'hd/diff-no-index-reverse-fix' into jch
git bisect good 80b9a513296d51b516138c9b0db4527461d14187
# good: [27b88c9ea2883f4e819a5fa4846c02b92ad589a4] Merge branch 'yt/winansi-die-lasterr-fix' into jch
git bisect good 27b88c9ea2883f4e819a5fa4846c02b92ad589a4
# first 'bad' commit: [4f65642eb051c595de1b06d148c46f4e316fafc3] Merge branch 'tb/rerere-lock-grace' into jch

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

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


-- 
D. Ben Knoble

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[Phillip Wood]

Hi Ben

On 28/09/2026 14:00, D. Ben Knoble wrote:
> 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)

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.

Thanks

Phillip

> 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
> 
> 

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[Thomas Bachem]

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

That is it. I ran t5520 on 'seen' with and without Ben's series under
GIT_TRACE2_EVENT, and the two runs differ in one place: which command's
auto maintenance runs "git reflog expire --all".

"git pull --rebase" computes the fork point before it fetches, from
the reflog of refs/remotes/me/copy, 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.

> 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

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

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?

-- 
D. Ben Knoble

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

On Mon, Sep 28, 2026 at 11:36 AM D. Ben Knoble <ben.knoble@gmail.com> wrote:
>
> 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????

Indeed, from GIT_TRACE2 output I can see we do run

    git rebase --no-autostash --onto ae98… f29a…

but well before that we run

    git merge-base --fork-point refs/remotes/me/copy to-rebase

which is then presumably fed down to the rebase. Interesting.

-- 
D. Ben Knoble

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[Phillip Wood]

Hi Ben

On 28/09/2026 16:36, D. Ben Knoble wrote:
> 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!

Yes, it is incredibly bad luck that the test broke, though I guess it is 
also fortunate as it means we can fix the latent bug in the test.

> 
> 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?

It is a bit confusing that "git pull --rebase" does not use "git rebase 
--fork-point", instead it calls "git merge-base --fork-point" (which is 
where we read the reflog of the remote branch) itself and then passes 
that as the upstream revision to "git rebase". I think this is because 
fork-point handling was added to "git pull" before "--fork-point" 
existed in "git rebase". "git rebase" only looks for a fork-point if its 
upstream argument is a ref, so as "git pull" passes an object id, the 
fork-point detection in rebase is bypassed.

Thanks

Phillip

Re: [PATCH v3 0/5] stash: clean up index-mode test merge

[Phillip Wood]

Hi Thomas

On 28/09/2026 15:50, Thomas Bachem wrote:
> 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",

Thanks for tracking that down, I couldn't see where we'd be calling "git 
stash apply" with "--index" but builtin/merge.c:restore_state() calls 
"git stash apply --index --quiet" rather than calling one of the 
autostash helper functions which do not use "--index".

> 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.

That accounts for the difference in the number of reflog entries. It's 
good to have an explanation for why we're expiring the reflog entries at 
a slightly different time.

Thanks

Phillip
> 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

[PATCH v4 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

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 adds missing test coverage
(which catch breakages from prior incorrect rounds of this series). The
fourth fixes a test interaction with another in-flight topic. The last
holds the interesting bits.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v4 1/5] builtin/stash: remove unused header

[D. Ben Knoble]

Clang complains that oid-array.h is unused. Certainly none of the
oid_array* functions, types, etc., are used, and the
transitively-included hash.h declarations are used but covered by a
pre-existing direct #include of hash.h.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v4 2/5] stash: prepare merge options earlier

[D. Ben Knoble]

In a future commit, we will reuse these options for the index merge of
"apply --index", not just for the worktree.

Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index dfea2d2c4c..d2b736d4e6 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -664,6 +664,11 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 				repo_get_index_file(the_repository), 0, NULL))
 		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) ||
 		    oideq(&c_tree, &info->i_tree)) {
@@ -695,8 +700,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 		}
 	}
 
-	init_ui_merge_options(&o, the_repository);
-
 	o.branch1 = label_ours ? label_ours : "Updated upstream";
 	o.branch2 = label_theirs ? label_theirs : "Stashed changes";
 	o.ancestor = label_base ? label_base : "Stash base";
@@ -704,9 +707,6 @@ 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);
 
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v4 3/5] t3903: test failed "stash apply --index"

[D. Ben Knoble]

The next commit will refactor index handling for applied stashes, so
let's make sure we cover conflicted index merging, too.

Helped-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..70af58e161 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -374,6 +374,27 @@ 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 &&
+	echo conflict >other-file &&
+	git add other-file &&
+	git diff-files -p >expect &&
+	git diff-index --cached HEAD >expect-index &&
+
+	test_must_fail git stash apply --index 2>err &&
+	test_grep "conflicts in index. Try without --index" err &&
+	git diff-files -p >actual &&
+	test_cmp expect actual &&
+	git diff-index --cached HEAD >actual-index &&
+	test_cmp expect-index actual-index
+'
+
 test_expect_success 'stash -k' '
 	echo bar3 >file &&
 	echo bar4 >file2 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v4 4/5] t5520: don't expire reflogs where it matters

[D. Ben Knoble]

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v4 4/5] t5520: don't expire reflogs where it matters

[Phillip Wood]

On 29/09/2026 13:18, D. Ben Knoble wrote:
> 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.

I find this commit message quite hard to understand. From my perspective 
the important points are

  - "git merge" uses "git stash" to clear any uncommitted changes from
    the worktree before it tries each strategy. The stashed changes are
    popped with "--index".

  - switching "git stash pop --index" to use merge_incore_nonrecursive()
    causes "git merge" to stop writing the reflog entries that came from
    "git stash pop --index" running "git reset"

  - that combined with "git rebase" starting to run "git maintenance
    --auto" changed when we expire the reflogs which breaks the fork-
    point detection.

The changes themselves look good

Thanks

Phillip

> 
> 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

[PATCH v4 5/5] builtin/stash: merge index in-core

[D. Ben Knoble]

"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. If there are conflicts, we discard the resulting tree, so
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.

Reported-by: Eli Barzilay <eli@barzilay.org>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c  | 80 ++++++++++--------------------------------------
 t/t3903-stash.sh | 21 +++++++++++++
 t/t7600-merge.sh |  9 ++++++
 3 files changed, 47 insertions(+), 63 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index d2b736d4e6..ec07547376 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	return ret;
 }
 
-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *w_commit_hex = oid_to_hex(w_commit);
-
-	/*
-	 * Diff-tree would not be very hard to replace with a native function,
-	 * however it should be done together with apply_cached.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
-	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
-
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
-
-static int apply_cached(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Apply currently only reads either from stdin or a file, thus
-	 * apply_all_patches would have to be updated to optionally take a
-	 * buffer.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "apply", "--cached", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
-}
-
-static int reset_head(void)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Reset is overall quite simple, however there is no current public
-	 * API for resetting.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
-
-	return run_command(&cp);
-}
-
 static int is_path_a_directory(const char *path)
 {
 	/*
@@ -674,29 +630,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)
+			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);
+
+			if (!result.clean) {
+				merge_finalize(&o, &result);
 				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);
+			} else {
+				oidcpy(&index_tree, &result.tree->object.oid);
+				merge_finalize(&o, &result);
+			}
 		}
 	}
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 70af58e161..70c6031958 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -395,6 +395,27 @@ 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 &&
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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v4 5/5] builtin/stash: merge index in-core

[Junio C Hamano]

"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?

Re: [PATCH v4 5/5] builtin/stash: merge index in-core

[D. Ben Knoble]

On Tue, Sep 29, 2026 at 4:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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,

Yep. Missed that when looking at the result struct. Will fix.

>
>                         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?


-- 
D. Ben Knoble

Re: [PATCH v4 0/5] stash: clean up index-mode test merge

[Phillip Wood]

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

> 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

Re: [PATCH v4 0/5] stash: clean up index-mode test merge

[Ben Knoble]

> 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.

Re: [PATCH v4 0/5] stash: clean up index-mode test merge

[D. Ben Knoble]

On Tue, Sep 29, 2026 at 1:32 PM Ben Knoble <ben.knoble@gmail.com> wrote:
>
>
> > 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.

I realized Junio wasn't CC'd on the prior mail, but since I re-rolled
and the merge base changed, I think I've got it right for v5, which
just went out.

-- 
D. Ben Knoble

[PATCH v5 0/4] stash: clean up index-mode test merge

[D. Ben Knoble]

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 adds missing test coverage
(which catch breakages from prior incorrect rounds of this series). The
fourth fixes a test interaction with another in-flight topic. The last
holds the interesting bits.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v5 1/4] builtin/stash: remove unused header

[D. Ben Knoble]

Clang complains that oid-array.h is unused. Certainly none of the
oid_array* functions, types, etc., are used, and the
transitively-included hash.h declarations are used but covered by a
pre-existing direct #include of hash.h.

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
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v5 2/4] stash: prepare merge options earlier

[D. Ben Knoble]

In a future commit, we will reuse these options for the index merge of
"apply --index", not just for the worktree.

Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index dfea2d2c4c..d2b736d4e6 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -664,6 +664,11 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 				repo_get_index_file(the_repository), 0, NULL))
 		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) ||
 		    oideq(&c_tree, &info->i_tree)) {
@@ -695,8 +700,6 @@ static enum stash_apply_result do_apply_stash(const char *prefix,
 		}
 	}
 
-	init_ui_merge_options(&o, the_repository);
-
 	o.branch1 = label_ours ? label_ours : "Updated upstream";
 	o.branch2 = label_theirs ? label_theirs : "Stashed changes";
 	o.ancestor = label_base ? label_base : "Stash base";
@@ -704,9 +707,6 @@ 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);
 
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v5 3/4] t3903: test failed "stash apply --index"

[D. Ben Knoble]

The next commit will refactor index handling for applied stashes, so
let's make sure we cover conflicted index merging, too.

Helped-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..70af58e161 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -374,6 +374,27 @@ 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 &&
+	echo conflict >other-file &&
+	git add other-file &&
+	git diff-files -p >expect &&
+	git diff-index --cached HEAD >expect-index &&
+
+	test_must_fail git stash apply --index 2>err &&
+	test_grep "conflicts in index. Try without --index" err &&
+	git diff-files -p >actual &&
+	test_cmp expect actual &&
+	git diff-index --cached HEAD >actual-index &&
+	test_cmp expect-index actual-index
+'
+
 test_expect_success 'stash -k' '
 	echo bar3 >file &&
 	echo bar4 >file2 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

[PATCH v5 4/4] builtin/stash: merge index in-core

[D. Ben Knoble]

"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. If there are conflicts, we discard the resulting tree, so
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.

Reported-by: Eli Barzilay <eli@barzilay.org>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: D. Ben Knoble <ben.knoble@gmail.com>
---
 builtin/stash.c  | 83 ++++++++++++------------------------------------
 t/t3903-stash.sh | 21 ++++++++++++
 t/t7600-merge.sh |  9 ++++++
 3 files changed, 50 insertions(+), 63 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index d2b736d4e6..fa3deeecbe 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -422,50 +422,6 @@ static int create_index_from_tree(const struct object_id *tree_id,
 	return ret;
 }
 
-static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *w_commit_hex = oid_to_hex(w_commit);
-
-	/*
-	 * Diff-tree would not be very hard to replace with a native function,
-	 * however it should be done together with apply_cached.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "diff-tree", "--binary", "--no-color", NULL);
-	strvec_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
-
-	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
-}
-
-static int apply_cached(struct strbuf *out)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Apply currently only reads either from stdin or a file, thus
-	 * apply_all_patches would have to be updated to optionally take a
-	 * buffer.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "apply", "--cached", NULL);
-	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
-}
-
-static int reset_head(void)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	/*
-	 * Reset is overall quite simple, however there is no current public
-	 * API for resetting.
-	 */
-	cp.git_cmd = 1;
-	strvec_pushl(&cp.args, "reset", "--quiet", "--refresh", NULL);
-
-	return run_command(&cp);
-}
-
 static int is_path_a_directory(const char *path)
 {
 	/*
@@ -674,29 +630,30 @@ 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)
+			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);
+
+			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."));
-
-			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);
+			} else {
+				oidcpy(&index_tree, &result.tree->object.oid);
+				merge_finalize(&o, &result);
+			}
 		}
 	}
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 70af58e161..70c6031958 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -395,6 +395,27 @@ 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 &&
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 &&
-- 
2.56.0.rc1.315.gc6ed9934b7.dirty

Re: [PATCH v5 0/4] stash: clean up index-mode test merge

[Phillip Wood]

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".

Thanks

Phillip

> 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

Re: [PATCH v5 0/4] stash: clean up index-mode test merge

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> 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".

Thanks for writing and reviewing.  These now look very good to me
too.

Let me mark them for 'next'.