Hi all,

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 <[email protected]>

---
 builtin/stash.c | 1 -
 1 file changed, 1 deletion(-)

builtin/stash.c +0 -1

index 7a9843413b..dfea2d2c4c 100644
@@ -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

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

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

builtin/stash.c +9 -1

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

merge-ort.c +0 -1

  	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 'failed fast-forward merge with --autostash' '
  	git reset --hard c0 &&
  	git merge-file file file.orig file.5 &&

Hi Ben

Quoted 4 lines · D. Ben Knoble

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.

Quoted 7 lines

builtin/stash.c +1 -1

@@ -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 };
 

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 630

Quoted 6 lines

builtin/stash.c +1 -5

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

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 632

Quoted 1 line

builtin/stash.c +1 -0

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

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 646

Quoted 2 lines

builtin/stash.c +2 -0

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

merge-ort.c static void merge_start(struct merge_options *opt, struct merge_result *result) · line 5038

Quoted 9 lines

merge-ort.c +0 -1

index c410a5d353..f69a49d48a 100644
@@ -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.

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?

Quoted 10 lines · Phillip Wood

Quoted 4 lines · D. Ben Knoble

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!

Quoted 7 lines

Quoted 7 lines

builtin/stash.c +1 -1

@@ -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 };
 

Quoted 10 lines

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 630

Quoted 6 lines

builtin/stash.c +1 -5

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

Quoted 6 lines

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 632

Quoted 2 lines

builtin/stash.c +1 -0

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

Quoted 7 lines

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 646

Quoted 3 lines

builtin/stash.c +2 -0

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

Quoted 20 lines

merge-ort.c static void merge_start(struct merge_options *opt, struct merge_result *result) · line 5038

Quoted 10 lines

merge-ort.c +0 -1

index c410a5d353..f69a49d48a 100644
@@ -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.

Quoted 2 lines

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.

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.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

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

Quoted 3 lines

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!

Quoted 6 lines

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.

Quoted 9 lines

Quoted 6 lines

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 646

Quoted 2 lines

builtin/stash.c +2 -0

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

Quoted 1 line

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)

Quoted 5 lines

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

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 2 lines

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.

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 <[email protected]>

---
  t/t3903-stash.sh | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

t/t3903-stash.sh +0 -0

index 721158606f..3958ab3c8d 100755
@@ -374,6 +374,24 @@ setup_stash() {
  	test_cmp expect actual
  '

t/t3903-stash.sh +6 -0

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

Hi Ben

Quoted 16 lines · 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 <[email protected]>

---
  t/t3903-stash.sh | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

t/t3903-stash.sh +0 -0

index 721158606f..3958ab3c8d 100755
@@ -374,6 +374,24 @@ setup_stash() {
      test_cmp expect actual
  '
 

t/t3903-stash.sh setup_stash() { · line 385

Quoted 9 lines

t/t3903-stash.sh +9 -0

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

t/t3903-stash.sh setup_stash() { · line 388

Quoted 2 lines

t/t3903-stash.sh +2 -0

+     echo conflict >other-file &&
+     git add other-file &&

I wonder if we should to add "git diff-index --cached HEAD

Quoted 1 line

expect-index" here so we can check the index is unchanged as well. For

the paths that have unstaged changes we're already checking the index object ids via "diff-files", but I think in theory it would be possible to have an identical change in the index and worktree that is not picked up by that.

So overall, I'm thinking

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

Phew! Thanks for reading my rambling thinking aloud :)

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

Quoted 3 lines · Phillip Wood

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

builtin/stash.c +4 -3

+			o.branch2 = "Stashed index changes";
+			o.ancestor = "Stash base";
 
-			ret = apply_cached(&out);
-			strbuf_release(&out);
-			if (ret)
+			o.verbosity = 0;
+

builtin/stash.c +2 -9

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

t/t7600-merge.sh +9 -0

index 64fe21717d..8f6109fb91 100755
@@ -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 &&

Hi Ben

I've spotted a memory leak that I missed last time, apart from that this looks good.

Quoted 7 lines · D. Ben Knoble

builtin/stash.c +1 -1

@@ -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 };
 

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 632

Quoted 6 lines

builtin/stash.c +1 -5

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

Quoted 13 lines

builtin/stash.c +9 -3

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

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 647

Quoted 5 lines

builtin/stash.c +3 -0

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

Quoted 3 lines · Phillip Wood

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!

Quoted 8 lines · D. Ben Knoble

Quoted 3 lines · Phillip Wood

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

Quoted 2 lines

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

(quoted diff) +1 -3

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

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.

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.

Quoted 11 lines · Junio C Hamano

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.

Quoted 2 lines

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.

Quoted 17 lines · D. Ben Knoble

Quoted 11 lines · Junio C Hamano

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 36 lines · Junio C Hamano

Quoted 17 lines · D. Ben Knoble

Quoted 11 lines · Junio C Hamano

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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

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

"D. Ben Knoble" <[email protected]> writes:

Ahh, or perhaps the trees are indeed given in a wrong order, but not in a random wrong order. merge_ort_nonrecursive(), which is *not* the function you are using, takes head, merge, and merge_base in this order, and that order matches what you wrote.

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

The new test in the attached patch will fail with this step but if we revert the changes to builtin/stash.c in this step, it passes.

  t/t3903-stash.sh | 32 ++++++++++++++++++++++++++++++++
  1 file changed, 32 insertions(+)

t/t3903-stash.sh +32 -0

index 3958ab3c8d..0a87e62b11 100755
@@ -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 &&

Hi Junio

On 24/09/2026 22:59, Junio C Hamano wrote:

Quoted 5 lines

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

Quoted 7 lines

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.

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 <[email protected]>

Signed-off-by D. Ben Knoble <[email protected]>

---
  t/t3903-stash.sh | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)

t/t3903-stash.sh +21 -0

index 721158606f..9bc99fa252 100755
@@ -374,6 +374,27 @@ setup_stash() {
  	test_cmp expect actual
  '
 
+# the later "stash -k" test is not expecting us to muck with file so much, so
+# reset when finished
+test_expect_success 'stash apply --index merges the correct trees' '
+	head=$(git rev-parse HEAD) &&
+	test_when_finished "git reset --hard $head" &&
+	test_write_lines A B C >file &&
+	git commit -m setup file &&
+	test_write_lines A B staged >file &&
+	git add file &&
+	test_write_lines A B unstaged >file &&
+	git stash &&
+	test_write_lines committed B C >file &&
+	git commit -m to-be-merged file &&
+	git stash pop --index &&
+	git show :file >actual &&
+	test_write_lines committed B staged >expect &&
+	test_cmp expect actual &&
+	test_write_lines committed B unstaged >expect &&
+	test_cmp expect file
+'
+
  test_expect_success 'stash -k' '
  	echo bar3 >file &&
  	echo bar4 >file2 &&

Hi Ben

Quoted 3 lines · 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.

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.

Quoted 4 lines · Phillip Wood

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.

Quoted 4 lines

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.

(quoted diff) +1 -4

@@ -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^!."),

(quoted diff) +2 -2

+
+			if (!result.clean)
 				return error(_("conflicts in index. "
 					       "Try without --index."));
-
-			discard_index(the_repository->index);

Quoted 10 lines · D. Ben Knoble

(quoted diff) +1 -4

@@ -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^!."),

Review static enum stash_apply_result do_apply_stash(const char *prefix, · line 636

Quoted 10 lines

(quoted diff) +4 -5

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

Quoted 14 lines

(quoted diff) +2 -10

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

Review static enum stash_apply_result do_apply_stash(const char *prefix, · line 642

Quoted 2 lines

(quoted diff) +0 -0

              }
      }

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.

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 645

Quoted 4 lines · D. Ben Knoble

builtin/stash.c +4 -0

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

Quoted 8 lines · Junio C Hamano

Quoted 4 lines · D. Ben Knoble

builtin/stash.c +4 -0

+                     merge_incore_nonrecursive(&o, merge_base, head, merge,
+                                               &result);
+
+                     oidcpy(&index_tree, &result.tree->object.oid);

Quoted 6 lines

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.

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:

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.

Quoted 6 lines · 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.

Quoted 4 lines

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.

Quoted 8 lines · Junio C Hamano

Quoted 6 lines · 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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 2 lines · Phillip Wood

On 27/09/2026 20:21, Junio C Hamano wrote:

Just leaving some breadcrumb notes…

Quoted 4 lines · D. Ben Knoble

Quoted 2 lines · Phillip Wood

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

Quoted 7 lines · D. Ben Knoble

Just leaving some breadcrumb notes…

Quoted 3 lines · D. Ben Knoble

Quoted 1 line · Phillip Wood

Quoted 4 lines

Quoted 1 line

Quoted 1 line · Junio C Hamano

From my local version of the branch, the following script points at
4f65642eb0 (Merge branch 'tb/rerere-lock-grace' into jch, 2026-09-27):

And within that topic, bisect points to 2d1fa0323f (rebase, cherry-pick, revert: run auto maintenance when done, 2026-09-17)

in
t5220.69 as Phillip said.

expecting success of 5520.69 '--rebase -f with rebased upstream':
test_when_finished "test_might_fail git rebase --abort" && git reset --hard to-rebase-orig &&
git pull --rebase -f me copy &&
echo "conflicting modification" >expect &&
test_cmp expect file &&
echo file >expect &&
test_cmp expect file2

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

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

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

git config maintenance.reflog-expire.auto 0

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

Quoted 6 lines · Phillip Wood

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,

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

Quoted 5 lines

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.

Let me see if I understand correctly…

Quoted 11 lines · Thomas Bachem

Quoted 6 lines · Phillip Wood

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

Let me see if I understand correctly…

Quoted 11 lines · Thomas Bachem

Quoted 6 lines · Phillip Wood

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.

Quoted 20 lines

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

Quoted 5 lines

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.

Quoted 13 lines

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?

Quoted 6 lines · Phillip Wood

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.

Quoted 5 lines

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:
<[email protected]>

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

From: Thomas Bachem <[email protected]>

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 <[email protected]>

Helped-by D. Ben Knoble <[email protected]>

Helped-by Phillip Wood <[email protected]>

Assisted-by Claude Fable 5.1

Signed-off-by Thomas Bachem <[email protected]>

Signed-off-by D. Ben Knoble <[email protected]>

---
  t/t5520-pull.sh | 6 ++++++
  1 file changed, 6 insertions(+)

t/t5520-pull.sh +6 -0

index 27f38ab3c8..bc818605a5 100755
@@ -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

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 644

Quoted 2 lines · D. Ben Knoble

builtin/stash.c +2 -0

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

Quoted 1 line

+ if (!result.clean) {

... "result.clean is false" is not true here, so we will ...

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 650

Quoted 4 lines

builtin/stash.c +2 -0

+                             merge_finalize(&o, &result);
                              return error(_("conflicts in index. "
                                             "Try without --index."));
+                     } else {

... come here to access result.tree member, no?

builtin/stash.c static enum stash_apply_result do_apply_stash(const char *prefix, · line 653

Quoted 3 lines

builtin/stash.c +3 -0

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

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: <[email protected]>
v2: <[email protected]>
v3: <[email protected]>

[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 <[email protected]>
+Author: Thomas Bachem <[email protected]>

## 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 <[email protected]>
+ 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 <[email protected]>
+ Helped-by: D. Ben Knoble <[email protected]>
+ Helped-by: Phillip Wood <[email protected]>
+ Assisted-by: Claude Fable 5.1
+ Signed-off-by: Thomas Bachem <[email protected]>
+
+ ## 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

Hi Ben

Quoted 7 lines · D. Ben Knoble

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

Quoted 8 lines

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

Quoted 26 lines · Phillip Wood

Hi Ben

Quoted 7 lines · D. Ben Knoble

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

Quoted 8 lines

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

Quoted 1 line

Phillip

Thanks Phillip. Pending other positive acks, I’m not sure if I should reroll with Thomas’s new patch, reroll dropping it now there’s a seen topic for it, or just wait ;)

I’ll probably wait a bit and see how the dust settles, but:

Junio if you want to see a reroll hit the list using the new synthetic base to make things nicer for you, I can do so. In particular, I think the last check I made when I saw your mail about the synthetic base had the prior round.

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

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

Changes in v3:

• Change conflict label for current index
• Fix memory leak of merge_result
• Fix order of trees to make the correct merge (cherry-pick)
• New test (3/5) to validate this
• Fix test in 4/5 to assert more details of expected state

Changes in v2:

• Do give branch labels for the incore merge, although they are never seen (and clarify commit message as a result, also keeping the merge-ort asserts). Phillip was right: without those, we do segfault on conflicts.
• Use the ui merge options to keep the same diff algorithm.
• Use merge_finalize instead of clear_merge_options, and reuse the options between merge calls if they are already initialized.
• Add a new 2/4 to simplify merge options initialization.
• Add a new 3/4 with a test case for conflicted index merges.

v1: <[email protected]>
v2: <[email protected]>
v3: <[email protected]>
v4: <[email protected]>

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

  builtin/stash.c  | 94 +++++++++++++-----------------------------------
  t/t3903-stash.sh | 42 ++++++++++++++++++++++
  t/t7600-merge.sh |  9 +++++
  3 files changed, 76 insertions(+), 69 deletions(-)

Diff-intervalle contre v4 :

1:  6a165c4df4 = 1:  d8f4c36459 builtin/stash: remove unused header
2:  35b64ae321 = 2:  8e99033ef0 stash: prepare merge options earlier
3:  7b0b317ce0 = 3:  ee28d0a840 t3903: test failed "stash apply --index"
4:  2ac371d2dc < -:  ---------- t5520: don't expire reflogs where it matters
5:  e21b832a6e ! 4:  ca3de1d4a3 builtin/stash: merge index in-core
     @@ builtin/stash.c: static enum stash_apply_result do_apply_stash(const char *prefi
      +			merge_incore_nonrecursive(&o, merge_base, head, merge,
      +						  &result);
      +
     -+			if (!result.clean) {
     ++			if (result.clean < 0) {
     ++				merge_finalize(&o, &result);
     ++				return error(_("index merge failed"));
     ++			} else if (!result.clean) {
      +				merge_finalize(&o, &result);
       				return error(_("conflicts in index. "
       					       "Try without --index."));

base-commit: a018953688f1b10bddf91bff8747068f5f4746a4
prerequisite-patch-id: 601853fa5478b0dbfb260ba02632418e90338219

Hi Ben

Quoted 1 line · D. Ben Knoble

Quoted 5 lines

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