[PATCH] stash: reuse cached index entries in --patch temporary index

[PATCH] stash: reuse cached index entries in --patch temporary index

[Adam Johnson via GitGitGadget]

From: Adam Johnson <me@adamj.eu>

`git stash -p` prepares the interactive selection by creating a
temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
running the `add -p` machinery.

That temporary index was created by running `git read-tree HEAD`.  The
resulting index had no useful cached stat data or fsmonitor-valid bits
from the real index.  When `run_add_p()` refreshed that temporary index
before showing the first prompt, it could end up lstat(2)-ing every
tracked file, even in a repository where `git diff` and `git restore -p`
can use fsmonitor to avoid that work.

Create the temporary index in-process instead.  Use `unpack_trees()` to
reset the real index contents to HEAD while writing the result to the
temporary index path.  For paths whose index entries already match HEAD,
`oneway_merge()` reuses the existing cache entries, preserving their
cached stat data and `CE_FSMONITOR_VALID` state.

This makes the refresh performed by `run_add_p()` behave like the one
used by `git restore -p`: unchanged paths can be skipped via fsmonitor
instead of being scanned again.

In a 206k file repository with `core.fsmonitor` enabled and a one-line
change in one file, time to first prompt dropped from 34.774 seconds to
0.659 seconds.

Signed-off-by: Adam Johnson <me@adamj.eu>
---
    stash: reuse cached index entries in --patch temporary index

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2306%2Fadamchainz%2Faj%2Foptimize-stash-patch-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2306/adamchainz/aj/optimize-stash-patch-v1
Pull-Request: https://github.com/git/git/pull/2306

 builtin/stash.c        | 71 ++++++++++++++++++++++++++++++++++++++----
 t/t3904-stash-patch.sh | 18 +++++++++++
 2 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/builtin/stash.c b/builtin/stash.c
index 32dbc97b47..48189cb9f7 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -372,6 +372,57 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	return 0;
 }
 
+static int create_index_from_tree(const struct object_id *tree_id,
+				  const char *index_path)
+{
+	int nr_trees = 1;
+	int ret = 0;
+	struct unpack_trees_options opts;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct tree *tree;
+	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
+	struct lock_file lock_file = LOCK_INIT;
+
+	repo_read_index_preload(the_repository, NULL, 0);
+	if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
+		return -1;
+
+	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
+
+	memset(&opts, 0, sizeof(opts));
+
+	tree = repo_parse_tree_indirect(the_repository, tree_id);
+	if (!tree || repo_parse_tree(the_repository, tree)) {
+		ret = -1;
+		goto done;
+	}
+
+	init_tree_desc(t, &tree->object.oid, tree->buffer, tree->size);
+
+	opts.head_idx = 1;
+	opts.src_index = the_repository->index;
+	opts.dst_index = &dst_istate;
+	opts.merge = 1;
+	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+	opts.fn = oneway_merge;
+
+	if (unpack_trees(nr_trees, t, &opts)) {
+		ret = -1;
+		goto done;
+	}
+
+	if (write_locked_index(&dst_istate, &lock_file, COMMIT_LOCK)) {
+		ret = error(_("unable to write new index file"));
+		goto done;
+	}
+
+done:
+	release_index(&dst_istate);
+	if (ret)
+		rollback_lock_file(&lock_file);
+	return ret;
+}
+
 static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -1321,18 +1372,26 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 		       struct interactive_options *interactive_opts)
 {
 	int ret = 0;
-	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
+	struct commit *head_commit;
+	const struct object_id *head_tree;
 	struct index_state istate = INDEX_STATE_INIT(the_repository);
 	char *old_index_env = NULL, *old_repo_index_file;
 
 	remove_path(stash_index_path.buf);
 
-	cp_read_tree.git_cmd = 1;
-	strvec_pushl(&cp_read_tree.args, "read-tree", "HEAD", NULL);
-	strvec_pushf(&cp_read_tree.env, "GIT_INDEX_FILE=%s",
-		     stash_index_path.buf);
-	if (run_command(&cp_read_tree)) {
+	head_commit = lookup_commit(the_repository, &info->b_commit);
+	if (!head_commit || repo_parse_commit(the_repository, head_commit)) {
+		ret = -1;
+		goto done;
+	}
+	head_tree = get_commit_tree_oid(head_commit);
+	if (!head_tree) {
+		ret = -1;
+		goto done;
+	}
+
+	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
index 90a4ff2c10..4b3241c8cd 100755
--- a/t/t3904-stash-patch.sh
+++ b/t/t3904-stash-patch.sh
@@ -84,6 +84,24 @@ test_expect_success 'none of this moved HEAD' '
 	verify_saved_head
 '
 
+test_expect_success 'stash -p with unmodified tracked files present' '
+	git reset --hard &&
+	echo line1 >alpha &&
+	echo line1 >beta &&
+	git add alpha beta &&
+	git commit -m "add alpha and beta" &&
+	echo line2 >>alpha &&
+	echo y | git stash -p &&
+	echo line1 >expect &&
+	test_cmp expect alpha &&
+	test_cmp expect beta &&
+	git stash pop &&
+	printf "line1\nline2\n" >expect &&
+	test_cmp expect alpha &&
+	echo line1 >expect &&
+	test_cmp expect beta
+'
+
 test_expect_success 'stash -p with split hunk' '
 	git reset --hard &&
 	cat >test <<-\EOF &&

base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf
-- 
gitgitgadget

Re: [PATCH] stash: reuse cached index entries in --patch temporary index

[Junio C Hamano]

"Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Adam Johnson <me@adamj.eu>
>
> `git stash -p` prepares the interactive selection by creating a
> temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
> running the `add -p` machinery.
>
> That temporary index was created by running `git read-tree HEAD`.  The
> resulting index had no useful cached stat data or fsmonitor-valid bits
> from the real index.  When `run_add_p()` refreshed that temporary index
> before showing the first prompt, it could end up lstat(2)-ing every
> tracked file, even in a repository where `git diff` and `git restore -p`
> can use fsmonitor to avoid that work.
>
> Create the temporary index in-process instead.  Use `unpack_trees()` to
> reset the real index contents to HEAD while writing the result to the
> temporary index path.  For paths whose index entries already match HEAD,
> `oneway_merge()` reuses the existing cache entries, preserving their
> cached stat data and `CE_FSMONITOR_VALID` state.

Clever.  As the fsmonitor_valid bit is in-core only, updating the
index in-process would be an obvious and probably the only sensible
way to preserve it.

I however have to wonder if simply replacing the external process
invocation with "git read-tree -m HEAD" (i.e., oneway merge) gives
a similar speed-up.

> This makes the refresh performed by `run_add_p()` behave like the one
> used by `git restore -p`: unchanged paths can be skipped via fsmonitor
> instead of being scanned again.
>
> In a 206k file repository with `core.fsmonitor` enabled and a one-line
> change in one file, time to first prompt dropped from 34.774 seconds to
> 0.659 seconds.

Interesting.

> diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> index 90a4ff2c10..4b3241c8cd 100755
> --- a/t/t3904-stash-patch.sh
> +++ b/t/t3904-stash-patch.sh
> @@ -84,6 +84,24 @@ test_expect_success 'none of this moved HEAD' '
>  	verify_saved_head
>  '
>  
> +test_expect_success 'stash -p with unmodified tracked files present' '
> +	git reset --hard &&
> +	echo line1 >alpha &&
> +	echo line1 >beta &&
> +	git add alpha beta &&
> +	git commit -m "add alpha and beta" &&
> +	echo line2 >>alpha &&
> +	echo y | git stash -p &&
> +	echo line1 >expect &&
> +	test_cmp expect alpha &&
> +	test_cmp expect beta &&
> +	git stash pop &&
> +	printf "line1\nline2\n" >expect &&
> +	test_cmp expect alpha &&
> +	echo line1 >expect &&
> +	test_cmp expect beta
> +'

What I read from the proposed log message is that the change is
purely about performance and should not change any behaviour.  Why
do we need a new test in t/t3904?  I would not have surprised if we
saw a new test in t/perf/, though.

Thanks.

Re: [PATCH] stash: reuse cached index entries in --patch temporary index

[Junio C Hamano]

"Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  2 files changed, 83 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 32dbc97b47..48189cb9f7 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -372,6 +372,57 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  	return 0;
>  }
>  
> +static int create_index_from_tree(const struct object_id *tree_id,
> +				  const char *index_path)
> +{
> +	int nr_trees = 1;
> +	int ret = 0;
> +	struct unpack_trees_options opts;
> +	struct tree_desc t[MAX_UNPACK_TREES];
> +	struct tree *tree;
> +	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
> +	struct lock_file lock_file = LOCK_INIT;
> +
> +	repo_read_index_preload(the_repository, NULL, 0);
> +	if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
> +		return -1;

Is this "non-zero return from refresh_index() leads to a failure"
intended?  The old "git read-tree HEAD" wouldn't have cared if the
original index were unmerged, for example, but with this update, we
will see an immediate failure.  There are other conditions that
refresh_index() flips its local variable has_errors on, which leads
to its non-zero return.

Since "git stash -p" is almost always invoked when the user has
unstaged modifications, I am not sure allowing refresh_index() to
notice and barf is what we want here.

Re: [PATCH] stash: reuse cached index entries in --patch temporary index

[Adam Johnson]

> I however have to wonder if simply replacing the external process
> invocation with "git read-tree -m HEAD" (i.e., oneway merge) gives
> a similar speed-up.

Good idea, I just tried this, but it does not help. The subprocess runs
with GIT_INDEX_FILE set to a temporary index, so oneway_merge
never uses the CE_FSMONITOR_VALID fast path.

The in-process approach is necessary because it lets us set
opts.src_index to the real index with cached stat and fsmonitor data,
before switching GIT_INDEX_FILE.

> What I read from the proposed log message is that the change is
> purely about performance and should not change any behaviour.  Why
> do we need a new test in t/t3904?  I would not have surprised if we
> saw a new test in t/perf/, though.

Ah yeah, my bad. I added this while iterating to catch a bug I introduced,
but it's not necessary for the final patch. Will remove.

On Wed, 20 May 2026, at 03:08, Junio C Hamano wrote:
> "Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > From: Adam Johnson <me@adamj.eu>
> >
> > `git stash -p` prepares the interactive selection by creating a
> > temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
> > running the `add -p` machinery.
> >
> > That temporary index was created by running `git read-tree HEAD`.  The
> > resulting index had no useful cached stat data or fsmonitor-valid bits
> > from the real index.  When `run_add_p()` refreshed that temporary index
> > before showing the first prompt, it could end up lstat(2)-ing every
> > tracked file, even in a repository where `git diff` and `git restore -p`
> > can use fsmonitor to avoid that work.
> >
> > Create the temporary index in-process instead.  Use `unpack_trees()` to
> > reset the real index contents to HEAD while writing the result to the
> > temporary index path.  For paths whose index entries already match HEAD,
> > `oneway_merge()` reuses the existing cache entries, preserving their
> > cached stat data and `CE_FSMONITOR_VALID` state.
> 
> Clever.  As the fsmonitor_valid bit is in-core only, updating the
> index in-process would be an obvious and probably the only sensible
> way to preserve it.
> 
> I however have to wonder if simply replacing the external process
> invocation with "git read-tree -m HEAD" (i.e., oneway merge) gives
> a similar speed-up.
> 
> > This makes the refresh performed by `run_add_p()` behave like the one
> > used by `git restore -p`: unchanged paths can be skipped via fsmonitor
> > instead of being scanned again.
> >
> > In a 206k file repository with `core.fsmonitor` enabled and a one-line
> > change in one file, time to first prompt dropped from 34.774 seconds to
> > 0.659 seconds.
> 
> Interesting.
> 
> > diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> > index 90a4ff2c10..4b3241c8cd 100755
> > --- a/t/t3904-stash-patch.sh
> > +++ b/t/t3904-stash-patch.sh
> > @@ -84,6 +84,24 @@ test_expect_success 'none of this moved HEAD' '
> >  verify_saved_head
> >  '
> >  
> > +test_expect_success 'stash -p with unmodified tracked files present' '
> > + git reset --hard &&
> > + echo line1 >alpha &&
> > + echo line1 >beta &&
> > + git add alpha beta &&
> > + git commit -m "add alpha and beta" &&
> > + echo line2 >>alpha &&
> > + echo y | git stash -p &&
> > + echo line1 >expect &&
> > + test_cmp expect alpha &&
> > + test_cmp expect beta &&
> > + git stash pop &&
> > + printf "line1\nline2\n" >expect &&
> > + test_cmp expect alpha &&
> > + echo line1 >expect &&
> > + test_cmp expect beta
> > +'
> 
> What I read from the proposed log message is that the change is
> purely about performance and should not change any behaviour.  Why
> do we need a new test in t/t3904?  I would not have surprised if we
> saw a new test in t/perf/, though.
> 
> Thanks.
> 

Re: [PATCH] stash: reuse cached index entries in --patch temporary index

[Adam Johnson]

> Is this "non-zero return from refresh_index() leads to a failure"
> intended?

Good catch, it’s not needed. Removing, we can make the call
unconditional.

On Wed, 20 May 2026, at 03:26, Junio C Hamano wrote:
> "Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> >  2 files changed, 83 insertions(+), 6 deletions(-)
> >
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index 32dbc97b47..48189cb9f7 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -372,6 +372,57 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
> >  return 0;
> >  }
> >  
> > +static int create_index_from_tree(const struct object_id *tree_id,
> > +   const char *index_path)
> > +{
> > + int nr_trees = 1;
> > + int ret = 0;
> > + struct unpack_trees_options opts;
> > + struct tree_desc t[MAX_UNPACK_TREES];
> > + struct tree *tree;
> > + struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
> > + struct lock_file lock_file = LOCK_INIT;
> > +
> > + repo_read_index_preload(the_repository, NULL, 0);
> > + if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
> > + return -1;
> 
> Is this "non-zero return from refresh_index() leads to a failure"
> intended?  The old "git read-tree HEAD" wouldn't have cared if the
> original index were unmerged, for example, but with this update, we
> will see an immediate failure.  There are other conditions that
> refresh_index() flips its local variable has_errors on, which leads
> to its non-zero return.
> 
> Since "git stash -p" is almost always invoked when the user has
> unstaged modifications, I am not sure allowing refresh_index() to
> notice and barf is what we want here.
> 

[PATCH v2] stash: reuse cached index entries in --patch temporary index

[Adam Johnson via GitGitGadget]

From: Adam Johnson <me@adamj.eu>

`git stash -p` prepares the interactive selection by creating a
temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
running the `add -p` machinery.

That temporary index was created by running `git read-tree HEAD`.  The
resulting index had no useful cached stat data or fsmonitor-valid bits
from the real index.  When `run_add_p()` refreshed that temporary index
before showing the first prompt, it could end up lstat(2)-ing every
tracked file, even in a repository where `git diff` and `git restore -p`
can use fsmonitor to avoid that work.

Create the temporary index in-process instead.  Use `unpack_trees()` to
reset the real index contents to HEAD while writing the result to the
temporary index path.  For paths whose index entries already match HEAD,
`oneway_merge()` reuses the existing cache entries, preserving their
cached stat data and `CE_FSMONITOR_VALID` state.

This makes the refresh performed by `run_add_p()` behave like the one
used by `git restore -p`: unchanged paths can be skipped via fsmonitor
instead of being scanned again.

In a 206k file repository with `core.fsmonitor` enabled and a one-line
change in one file, time to first prompt dropped from 34.774 seconds to
0.659 seconds. The new perf test file demonstrates similar improvements,
with maen times for without- and with-fsmonitor cases dropping from 6.90
and 6.83 seconds to 0.55 and 0.28 seconds, respectively.

Signed-off-by: Adam Johnson <me@adamj.eu>
---
    stash: reuse cached index entries in --patch temporary index

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2306%2Fadamchainz%2Faj%2Foptimize-stash-patch-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2306/adamchainz/aj/optimize-stash-patch-v2
Pull-Request: https://github.com/git/git/pull/2306

Range-diff vs v1:

 1:  b228160cc4 ! 1:  8785572c4d stash: reuse cached index entries in --patch temporary index
     @@ Commit message
      
          In a 206k file repository with `core.fsmonitor` enabled and a one-line
          change in one file, time to first prompt dropped from 34.774 seconds to
     -    0.659 seconds.
     +    0.659 seconds. The new perf test file demonstrates similar improvements,
     +    with maen times for without- and with-fsmonitor cases dropping from 6.90
     +    and 6.83 seconds to 0.55 and 0.28 seconds, respectively.
      
          Signed-off-by: Adam Johnson <me@adamj.eu>
      
     @@ builtin/stash.c: static int reset_tree(struct object_id *i_tree, int update, int
      +	struct lock_file lock_file = LOCK_INIT;
      +
      +	repo_read_index_preload(the_repository, NULL, 0);
     -+	if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
     -+		return -1;
     ++	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
      +
      +	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
      +
     @@ builtin/stash.c: static int stash_patch(struct stash_info *info, const struct pa
       		goto done;
       	}
      
     - ## t/t3904-stash-patch.sh ##
     -@@ t/t3904-stash-patch.sh: test_expect_success 'none of this moved HEAD' '
     - 	verify_saved_head
     - '
     - 
     -+test_expect_success 'stash -p with unmodified tracked files present' '
     -+	git reset --hard &&
     -+	echo line1 >alpha &&
     -+	echo line1 >beta &&
     -+	git add alpha beta &&
     -+	git commit -m "add alpha and beta" &&
     -+	echo line2 >>alpha &&
     -+	echo y | git stash -p &&
     -+	echo line1 >expect &&
     -+	test_cmp expect alpha &&
     -+	test_cmp expect beta &&
     -+	git stash pop &&
     -+	printf "line1\nline2\n" >expect &&
     -+	test_cmp expect alpha &&
     -+	echo line1 >expect &&
     -+	test_cmp expect beta
     + ## t/perf/p3904-stash-patch.sh (new) ##
     +@@
     ++#!/bin/sh
     ++
     ++test_description="Performance tests for git stash -p"
     ++
     ++. ./perf-lib.sh
     ++
     ++test_perf_fresh_repo
     ++
     ++test_expect_success "setup" '
     ++	mkdir files &&
     ++	test_seq 1 100000 | while read i; do
     ++		echo "content $i" >files/$i.txt || return 1
     ++	done &&
     ++	git add files/ &&
     ++	git commit -q -m "add tracked files" &&
     ++	echo modified >files/1.txt
      +'
      +
     - test_expect_success 'stash -p with split hunk' '
     - 	git reset --hard &&
     - 	cat >test <<-\EOF &&
     ++test_perf "stash -p, no fsmonitor" \
     ++	--setup 'echo modified >files/1.txt' '
     ++	printf "q\n" | git stash -p >/dev/null 2>&1 || true
     ++'
     ++
     ++if test_have_prereq FSMONITOR_DAEMON
     ++then
     ++	test_expect_success "enable builtin fsmonitor" '
     ++		git config core.fsmonitor true &&
     ++		git fsmonitor--daemon start &&
     ++		git update-index --fsmonitor &&
     ++		git status >/dev/null 2>&1
     ++	'
     ++
     ++	test_perf "stash -p, builtin fsmonitor" \
     ++		--setup 'echo modified >files/1.txt && git status >/dev/null 2>&1' '
     ++		printf "q\n" | git stash -p >/dev/null 2>&1 || true
     ++	'
     ++
     ++	test_expect_success "stop builtin fsmonitor" '
     ++		git fsmonitor--daemon stop
     ++	'
     ++fi
     ++
     ++test_done


 builtin/stash.c             | 70 +++++++++++++++++++++++++++++++++----
 t/perf/p3904-stash-patch.sh | 43 +++++++++++++++++++++++
 2 files changed, 107 insertions(+), 6 deletions(-)
 create mode 100755 t/perf/p3904-stash-patch.sh

diff --git a/builtin/stash.c b/builtin/stash.c
index 32dbc97b47..c4809f299a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -372,6 +372,56 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
 	return 0;
 }
 
+static int create_index_from_tree(const struct object_id *tree_id,
+				  const char *index_path)
+{
+	int nr_trees = 1;
+	int ret = 0;
+	struct unpack_trees_options opts;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct tree *tree;
+	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
+	struct lock_file lock_file = LOCK_INIT;
+
+	repo_read_index_preload(the_repository, NULL, 0);
+	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
+
+	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
+
+	memset(&opts, 0, sizeof(opts));
+
+	tree = repo_parse_tree_indirect(the_repository, tree_id);
+	if (!tree || repo_parse_tree(the_repository, tree)) {
+		ret = -1;
+		goto done;
+	}
+
+	init_tree_desc(t, &tree->object.oid, tree->buffer, tree->size);
+
+	opts.head_idx = 1;
+	opts.src_index = the_repository->index;
+	opts.dst_index = &dst_istate;
+	opts.merge = 1;
+	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
+	opts.fn = oneway_merge;
+
+	if (unpack_trees(nr_trees, t, &opts)) {
+		ret = -1;
+		goto done;
+	}
+
+	if (write_locked_index(&dst_istate, &lock_file, COMMIT_LOCK)) {
+		ret = error(_("unable to write new index file"));
+		goto done;
+	}
+
+done:
+	release_index(&dst_istate);
+	if (ret)
+		rollback_lock_file(&lock_file);
+	return ret;
+}
+
 static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
@@ -1321,18 +1371,26 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
 		       struct interactive_options *interactive_opts)
 {
 	int ret = 0;
-	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
 	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
+	struct commit *head_commit;
+	const struct object_id *head_tree;
 	struct index_state istate = INDEX_STATE_INIT(the_repository);
 	char *old_index_env = NULL, *old_repo_index_file;
 
 	remove_path(stash_index_path.buf);
 
-	cp_read_tree.git_cmd = 1;
-	strvec_pushl(&cp_read_tree.args, "read-tree", "HEAD", NULL);
-	strvec_pushf(&cp_read_tree.env, "GIT_INDEX_FILE=%s",
-		     stash_index_path.buf);
-	if (run_command(&cp_read_tree)) {
+	head_commit = lookup_commit(the_repository, &info->b_commit);
+	if (!head_commit || repo_parse_commit(the_repository, head_commit)) {
+		ret = -1;
+		goto done;
+	}
+	head_tree = get_commit_tree_oid(head_commit);
+	if (!head_tree) {
+		ret = -1;
+		goto done;
+	}
+
+	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
 		ret = -1;
 		goto done;
 	}
diff --git a/t/perf/p3904-stash-patch.sh b/t/perf/p3904-stash-patch.sh
new file mode 100755
index 0000000000..4cfce638be
--- /dev/null
+++ b/t/perf/p3904-stash-patch.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description="Performance tests for git stash -p"
+
+. ./perf-lib.sh
+
+test_perf_fresh_repo
+
+test_expect_success "setup" '
+	mkdir files &&
+	test_seq 1 100000 | while read i; do
+		echo "content $i" >files/$i.txt || return 1
+	done &&
+	git add files/ &&
+	git commit -q -m "add tracked files" &&
+	echo modified >files/1.txt
+'
+
+test_perf "stash -p, no fsmonitor" \
+	--setup 'echo modified >files/1.txt' '
+	printf "q\n" | git stash -p >/dev/null 2>&1 || true
+'
+
+if test_have_prereq FSMONITOR_DAEMON
+then
+	test_expect_success "enable builtin fsmonitor" '
+		git config core.fsmonitor true &&
+		git fsmonitor--daemon start &&
+		git update-index --fsmonitor &&
+		git status >/dev/null 2>&1
+	'
+
+	test_perf "stash -p, builtin fsmonitor" \
+		--setup 'echo modified >files/1.txt && git status >/dev/null 2>&1' '
+		printf "q\n" | git stash -p >/dev/null 2>&1 || true
+	'
+
+	test_expect_success "stop builtin fsmonitor" '
+		git fsmonitor--daemon stop
+	'
+fi
+
+test_done

base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf
-- 
gitgitgadget

Re: [PATCH v2] stash: reuse cached index entries in --patch temporary index

[Junio C Hamano]

"Adam Johnson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Adam Johnson <me@adamj.eu>
>
> `git stash -p` prepares the interactive selection by creating a
> temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
> running the `add -p` machinery.
>
> That temporary index was created by running `git read-tree HEAD`.  The
> resulting index had no useful cached stat data or fsmonitor-valid bits
> from the real index.  When `run_add_p()` refreshed that temporary index
> before showing the first prompt, it could end up lstat(2)-ing every
> tracked file, even in a repository where `git diff` and `git restore -p`
> can use fsmonitor to avoid that work.
>
> Create the temporary index in-process instead.  Use `unpack_trees()` to
> reset the real index contents to HEAD while writing the result to the
> temporary index path.  For paths whose index entries already match HEAD,
> `oneway_merge()` reuses the existing cache entries, preserving their
> cached stat data and `CE_FSMONITOR_VALID` state.
>
> This makes the refresh performed by `run_add_p()` behave like the one
> used by `git restore -p`: unchanged paths can be skipped via fsmonitor
> instead of being scanned again.
>
> In a 206k file repository with `core.fsmonitor` enabled and a one-line
> change in one file, time to first prompt dropped from 34.774 seconds to
> 0.659 seconds. The new perf test file demonstrates similar improvements,
> with maen times for without- and with-fsmonitor cases dropping from 6.90
> and 6.83 seconds to 0.55 and 0.28 seconds, respectively.
>
> Signed-off-by: Adam Johnson <me@adamj.eu>
> ---
>     stash: reuse cached index entries in --patch temporary index
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2306%2Fadamchainz%2Faj%2Foptimize-stash-patch-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2306/adamchainz/aj/optimize-stash-patch-v2
> Pull-Request: https://github.com/git/git/pull/2306

The diff relative to the previous round looked good.  I am not a
"stash -p" user myself, but I suspect that there are people who
heavily use it, so I'd feel safer if an extra set of eye looks at
the patch and gives an Ack, but other than that I have no comments
on the patch.  Looking good.

Thanks.


>  builtin/stash.c             | 70 +++++++++++++++++++++++++++++++++----
>  t/perf/p3904-stash-patch.sh | 43 +++++++++++++++++++++++
>  2 files changed, 107 insertions(+), 6 deletions(-)
>  create mode 100755 t/perf/p3904-stash-patch.sh
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 32dbc97b47..c4809f299a 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -372,6 +372,56 @@ static int reset_tree(struct object_id *i_tree, int update, int reset)
>  	return 0;
>  }
>  
> +static int create_index_from_tree(const struct object_id *tree_id,
> +				  const char *index_path)
> +{
> +	int nr_trees = 1;
> +	int ret = 0;
> +	struct unpack_trees_options opts;
> +	struct tree_desc t[MAX_UNPACK_TREES];
> +	struct tree *tree;
> +	struct index_state dst_istate = INDEX_STATE_INIT(the_repository);
> +	struct lock_file lock_file = LOCK_INIT;
> +
> +	repo_read_index_preload(the_repository, NULL, 0);
> +	refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL);
> +
> +	hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR);
> +
> +	memset(&opts, 0, sizeof(opts));
> +
> +	tree = repo_parse_tree_indirect(the_repository, tree_id);
> +	if (!tree || repo_parse_tree(the_repository, tree)) {
> +		ret = -1;
> +		goto done;
> +	}
> +
> +	init_tree_desc(t, &tree->object.oid, tree->buffer, tree->size);
> +
> +	opts.head_idx = 1;
> +	opts.src_index = the_repository->index;
> +	opts.dst_index = &dst_istate;
> +	opts.merge = 1;
> +	opts.reset = UNPACK_RESET_PROTECT_UNTRACKED;
> +	opts.fn = oneway_merge;
> +
> +	if (unpack_trees(nr_trees, t, &opts)) {
> +		ret = -1;
> +		goto done;
> +	}
> +
> +	if (write_locked_index(&dst_istate, &lock_file, COMMIT_LOCK)) {
> +		ret = error(_("unable to write new index file"));
> +		goto done;
> +	}
> +
> +done:
> +	release_index(&dst_istate);
> +	if (ret)
> +		rollback_lock_file(&lock_file);
> +	return ret;
> +}
> +
>  static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
>  {
>  	struct child_process cp = CHILD_PROCESS_INIT;
> @@ -1321,18 +1371,26 @@ static int stash_patch(struct stash_info *info, const struct pathspec *ps,
>  		       struct interactive_options *interactive_opts)
>  {
>  	int ret = 0;
> -	struct child_process cp_read_tree = CHILD_PROCESS_INIT;
>  	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
> +	struct commit *head_commit;
> +	const struct object_id *head_tree;
>  	struct index_state istate = INDEX_STATE_INIT(the_repository);
>  	char *old_index_env = NULL, *old_repo_index_file;
>  
>  	remove_path(stash_index_path.buf);
>  
> -	cp_read_tree.git_cmd = 1;
> -	strvec_pushl(&cp_read_tree.args, "read-tree", "HEAD", NULL);
> -	strvec_pushf(&cp_read_tree.env, "GIT_INDEX_FILE=%s",
> -		     stash_index_path.buf);
> -	if (run_command(&cp_read_tree)) {
> +	head_commit = lookup_commit(the_repository, &info->b_commit);
> +	if (!head_commit || repo_parse_commit(the_repository, head_commit)) {
> +		ret = -1;
> +		goto done;
> +	}
> +	head_tree = get_commit_tree_oid(head_commit);
> +	if (!head_tree) {
> +		ret = -1;
> +		goto done;
> +	}
> +
> +	if (create_index_from_tree(head_tree, stash_index_path.buf)) {
>  		ret = -1;
>  		goto done;
>  	}
> diff --git a/t/perf/p3904-stash-patch.sh b/t/perf/p3904-stash-patch.sh
> new file mode 100755
> index 0000000000..4cfce638be
> --- /dev/null
> +++ b/t/perf/p3904-stash-patch.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description="Performance tests for git stash -p"
> +
> +. ./perf-lib.sh
> +
> +test_perf_fresh_repo
> +
> +test_expect_success "setup" '
> +	mkdir files &&
> +	test_seq 1 100000 | while read i; do
> +		echo "content $i" >files/$i.txt || return 1
> +	done &&
> +	git add files/ &&
> +	git commit -q -m "add tracked files" &&
> +	echo modified >files/1.txt
> +'
> +
> +test_perf "stash -p, no fsmonitor" \
> +	--setup 'echo modified >files/1.txt' '
> +	printf "q\n" | git stash -p >/dev/null 2>&1 || true
> +'
> +
> +if test_have_prereq FSMONITOR_DAEMON
> +then
> +	test_expect_success "enable builtin fsmonitor" '
> +		git config core.fsmonitor true &&
> +		git fsmonitor--daemon start &&
> +		git update-index --fsmonitor &&
> +		git status >/dev/null 2>&1
> +	'
> +
> +	test_perf "stash -p, builtin fsmonitor" \
> +		--setup 'echo modified >files/1.txt && git status >/dev/null 2>&1' '
> +		printf "q\n" | git stash -p >/dev/null 2>&1 || true
> +	'
> +
> +	test_expect_success "stop builtin fsmonitor" '
> +		git fsmonitor--daemon stop
> +	'
> +fi
> +
> +test_done
>
> base-commit: 7bcaabddcf68bd0702697da5904c3b68c52f94cf